diff --git a/builtin/credential/approle/path_login.go b/builtin/credential/approle/path_login.go index 1ba7fc0c5a1f..300ee9409f89 100644 --- a/builtin/credential/approle/path_login.go +++ b/builtin/credential/approle/path_login.go @@ -52,7 +52,7 @@ func (b *backend) pathLoginUpdateAliasLookahead(req *logical.Request, data *fram func (b *backend) pathLoginUpdate(req *logical.Request, data *framework.FieldData) (*logical.Response, error) { role, roleName, metadata, _, err := b.validateCredentials(req, data) if err != nil || role == nil { - return logical.ErrorResponse(fmt.Sprintf("failed to validate SecretID: %s", err)), nil + return logical.ErrorResponse(fmt.Sprintf("failed to validate credentials: %v", err)), nil } // Always include the role name, for later filtering @@ -94,8 +94,12 @@ func (b *backend) pathLoginRenew(req *logical.Request, data *framework.FieldData return nil, fmt.Errorf("failed to fetch role_name during renewal") } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + // Ensure that the Role still exists. - role, err := b.roleEntry(req.Storage, roleName) + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, fmt.Errorf("failed to validate role %s during renewal:%s", roleName, err) } diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index b9f7e5b98de4..ec5a22ea148b 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -509,10 +509,20 @@ the role.`, // pathRoleExistenceCheck returns whether the role with the given name exists or not. func (b *backend) pathRoleExistenceCheck(req *logical.Request, data *framework.FieldData) (bool, error) { - role, err := b.roleEntry(req.Storage, data.Get("role_name").(string)) + roleName := data.Get("role_name").(string) + if roleName == "" { + return false, fmt.Errorf("missing role_name") + } + + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return false, err } + return role != nil, nil } @@ -537,13 +547,17 @@ func (b *backend) pathRoleSecretIDList(req *logical.Request, data *framework.Fie return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + // Get the role entry role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err } if role == nil { - return logical.ErrorResponse(fmt.Sprintf("role %s does not exist", roleName)), nil + return logical.ErrorResponse(fmt.Sprintf("role %q does not exist", roleName)), nil } // Guard the list operation with an outer lock @@ -552,7 +566,7 @@ func (b *backend) pathRoleSecretIDList(req *logical.Request, data *framework.Fie roleNameHMAC, err := createHMAC(role.HMACKey, roleName) if err != nil { - return nil, fmt.Errorf("failed to create HMAC of role_name: %s", err) + return nil, fmt.Errorf("failed to create HMAC of role_name: %v", err) } // Listing works one level at a time. Get the first level of data @@ -618,9 +632,8 @@ func validateRoleConstraints(role *roleStorageEntry) error { return nil } -// setRoleEntry grabs a write lock and stores the options on an role into the -// storage. Also creates a reverse index from the role's RoleID to the role -// itself. +// setRoleEntry persists the role and creates an index from roleID to role +// name. func (b *backend) setRoleEntry(s logical.Storage, roleName string, role *roleStorageEntry, previousRoleID string) error { if roleName == "" { return fmt.Errorf("missing role name") @@ -641,7 +654,7 @@ func (b *backend) setRoleEntry(s logical.Storage, roleName string, role *roleSto return err } if entry == nil { - return fmt.Errorf("failed to create storage entry for role %s", roleName) + return fmt.Errorf("failed to create storage entry for role %q", roleName) } // Check if the index from the role_id to role already exists @@ -680,7 +693,7 @@ func (b *backend) setRoleEntry(s logical.Storage, roleName string, role *roleSto }) } -// roleEntry grabs the read lock and fetches the options of an role from the storage +// roleEntry reads the role from storage func (b *backend) roleEntry(s logical.Storage, roleName string) (*roleStorageEntry, error) { if roleName == "" { return nil, fmt.Errorf("missing role_name") @@ -688,11 +701,6 @@ func (b *backend) roleEntry(s logical.Storage, roleName string) (*roleStorageEnt var role roleStorageEntry - lock := b.roleLock(roleName) - - lock.RLock() - defer lock.RUnlock() - if entry, err := s.Get("role/" + strings.ToLower(roleName)); err != nil { return nil, err } else if entry == nil { @@ -712,6 +720,10 @@ func (b *backend) pathRoleCreateUpdate(req *logical.Request, data *framework.Fie return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + // Check if the role already exists role, err := b.roleEntry(req.Storage, roleName) if err != nil { @@ -722,13 +734,13 @@ func (b *backend) pathRoleCreateUpdate(req *logical.Request, data *framework.Fie if role == nil && req.Operation == logical.CreateOperation { hmacKey, err := uuid.GenerateUUID() if err != nil { - return nil, fmt.Errorf("failed to create role_id: %s\n", err) + return nil, fmt.Errorf("failed to create role_id: %v\n", err) } role = &roleStorageEntry{ HMACKey: hmacKey, } } else if role == nil { - return nil, fmt.Errorf("role entry not found during update operation") + return logical.ErrorResponse(fmt.Sprintf("invalid role name")), nil } previousRoleID := role.RoleID @@ -737,12 +749,12 @@ func (b *backend) pathRoleCreateUpdate(req *logical.Request, data *framework.Fie } else if req.Operation == logical.CreateOperation { roleID, err := uuid.GenerateUUID() if err != nil { - return nil, fmt.Errorf("failed to generate role_id: %s\n", err) + return nil, fmt.Errorf("failed to generate role_id: %v\n", err) } role.RoleID = roleID } if role.RoleID == "" { - return logical.ErrorResponse("invalid role_id"), nil + return logical.ErrorResponse("invalid role_id supplied, or failed to generate a role_id"), nil } if bindSecretIDRaw, ok := data.GetOk("bind_secret_id"); ok { @@ -780,7 +792,7 @@ func (b *backend) pathRoleCreateUpdate(req *logical.Request, data *framework.Fie role.Period = time.Second * time.Duration(data.Get("period").(int)) } if role.Period > b.System().MaxLeaseTTL() { - return logical.ErrorResponse(fmt.Sprintf("'period' of '%s' is greater than the backend's maximum lease TTL of '%s'", role.Period.String(), b.System().MaxLeaseTTL().String())), nil + return logical.ErrorResponse(fmt.Sprintf("period of %q is greater than the backend's maximum lease TTL of %q", role.Period.String(), b.System().MaxLeaseTTL().String())), nil } if secretIDNumUsesRaw, ok := data.GetOk("secret_id_num_uses"); ok { @@ -843,32 +855,78 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) return logical.ErrorResponse("missing role_name"), nil } - if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { + lock := b.roleLock(roleName) + lock.RLock() + lockRelease := lock.RUnlock + + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) + if err != nil { + lockRelease() return nil, err - } else if role == nil { + } + + if role == nil { + lockRelease() return nil, nil - } else { - // Convert the 'time.Duration' values to second. - role.SecretIDTTL /= time.Second - role.TokenTTL /= time.Second - role.TokenMaxTTL /= time.Second - role.Period /= time.Second + } - // Create a map of data to be returned and remove sensitive information from it - data := structs.New(role).Map() - delete(data, "role_id") - delete(data, "hmac_key") + respData := map[string]interface{}{ + "bind_secret_id": role.BindSecretID, + "bound_cidr_list": role.BoundCIDRList, + "period": role.Period / time.Second, + "policies": role.Policies, + "secret_id_num_uses": role.SecretIDNumUses, + "secret_id_ttl": role.SecretIDTTL / time.Second, + "token_max_ttl": role.TokenMaxTTL / time.Second, + "token_num_uses": role.TokenNumUses, + "token_ttl": role.TokenTTL / time.Second, + } - resp := &logical.Response{ - Data: data, - } + resp := &logical.Response{ + Data: respData, + } + + if err := validateRoleConstraints(role); err != nil { + resp.AddWarning("Role does not have any constraints set on it. Updates to this role will require a constraint to be set") + } - if err := validateRoleConstraints(role); err != nil { - resp.AddWarning("Role does not have any constraints set on it. Updates to this role will require a constraint to be set") + // For sanity, verify that the index still exists. If the index is missing, + // add one and return a warning so it can be reported. + roleIDIndex, err := b.roleIDEntry(req.Storage, role.RoleID) + if err != nil { + lockRelease() + return nil, err + } + + if roleIDIndex == nil { + // Switch to a write lock + lock.RUnlock() + lock.Lock() + lockRelease = lock.Unlock + + // Check again if the index is missing + roleIDIndex, err = b.roleIDEntry(req.Storage, role.RoleID) + if err != nil { + lockRelease() + return nil, err } - return resp, nil + if roleIDIndex == nil { + // Create a new index + err = b.setRoleIDEntry(req.Storage, role.RoleID, &roleIDStorageEntry{ + Name: roleName, + }) + if err != nil { + lockRelease() + return nil, fmt.Errorf("failed to create secondary index for role_id %q: %v", role.RoleID, err) + } + resp.AddWarning("Role identifier was missing an index back to role name. A new index has been added. Please report this observation.") + } } + + lockRelease() + + return resp, nil } // pathRoleDelete removes the role from the storage @@ -878,6 +936,10 @@ func (b *backend) pathRoleDelete(req *logical.Request, data *framework.FieldData return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -886,19 +948,14 @@ func (b *backend) pathRoleDelete(req *logical.Request, data *framework.FieldData return nil, nil } - // Acquire the lock before deleting the secrets. - lock := b.roleLock(roleName) - lock.Lock() - defer lock.Unlock() - // Just before the role is deleted, remove all the SecretIDs issued as part of the role. if err = b.flushRoleSecrets(req.Storage, roleName, role.HMACKey); err != nil { - return nil, fmt.Errorf("failed to invalidate the secrets belonging to role '%s': %s", roleName, err) + return nil, fmt.Errorf("failed to invalidate the secrets belonging to role %q: %v", roleName, err) } // Delete the reverse mapping from RoleID to the role if err = b.roleIDEntryDelete(req.Storage, role.RoleID); err != nil { - return nil, fmt.Errorf("failed to delete the mapping from RoleID to role '%s': %s", roleName, err) + return nil, fmt.Errorf("failed to delete the mapping from RoleID to role %q: %v", roleName, err) } // After deleting the SecretIDs and the RoleID, delete the role itself @@ -921,25 +978,29 @@ func (b *backend) pathRoleSecretIDLookupUpdate(req *logical.Request, data *frame return logical.ErrorResponse("missing secret_id"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + // Fetch the role role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err } if role == nil { - return nil, fmt.Errorf("role %s does not exist", roleName) + return nil, fmt.Errorf("role %q does not exist", roleName) } // Create the HMAC of the secret ID using the per-role HMAC key secretIDHMAC, err := createHMAC(role.HMACKey, secretID) if err != nil { - return nil, fmt.Errorf("failed to create HMAC of secret_id: %s", err) + return nil, fmt.Errorf("failed to create HMAC of secret_id: %v", err) } // Create the HMAC of the roleName using the per-role HMAC key roleNameHMAC, err := createHMAC(role.HMACKey, roleName) if err != nil { - return nil, fmt.Errorf("failed to create HMAC of role_name: %s", err) + return nil, fmt.Errorf("failed to create HMAC of role_name: %v", err) } // Create the index at which the secret_id would've been stored @@ -996,22 +1057,26 @@ func (b *backend) pathRoleSecretIDDestroyUpdateDelete(req *logical.Request, data return logical.ErrorResponse("missing secret_id"), nil } + roleLock := b.roleLock(roleName) + roleLock.RLock() + defer roleLock.RUnlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err } if role == nil { - return nil, fmt.Errorf("role %s does not exist", roleName) + return nil, fmt.Errorf("role %q does not exist", roleName) } secretIDHMAC, err := createHMAC(role.HMACKey, secretID) if err != nil { - return nil, fmt.Errorf("failed to create HMAC of secret_id: %s", err) + return nil, fmt.Errorf("failed to create HMAC of secret_id: %v", err) } roleNameHMAC, err := createHMAC(role.HMACKey, roleName) if err != nil { - return nil, fmt.Errorf("failed to create HMAC of role_name: %s", err) + return nil, fmt.Errorf("failed to create HMAC of role_name: %v", err) } entryIndex := fmt.Sprintf("secret_id/%s/%s", roleNameHMAC, secretIDHMAC) @@ -1036,7 +1101,7 @@ func (b *backend) pathRoleSecretIDDestroyUpdateDelete(req *logical.Request, data // Delete the storage entry that corresponds to the SecretID if err := req.Storage.Delete(entryIndex); err != nil { - return nil, fmt.Errorf("failed to delete SecretID: %s", err) + return nil, fmt.Errorf("failed to delete secret_id: %v", err) } return nil, nil @@ -1059,12 +1124,16 @@ func (b *backend) pathRoleSecretIDAccessorLookupUpdate(req *logical.Request, dat // Get the role details to fetch the RoleID and accessor to get // the HMACed SecretID. + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err } if role == nil { - return nil, fmt.Errorf("role %s does not exist", roleName) + return nil, fmt.Errorf("role %q does not exist", roleName) } accessorEntry, err := b.secretIDAccessorEntry(req.Storage, secretIDAccessor) @@ -1072,12 +1141,12 @@ func (b *backend) pathRoleSecretIDAccessorLookupUpdate(req *logical.Request, dat return nil, err } if accessorEntry == nil { - return nil, fmt.Errorf("failed to find accessor entry for secret_id_accessor:%s\n", secretIDAccessor) + return nil, fmt.Errorf("failed to find accessor entry for secret_id_accessor: %q\n", secretIDAccessor) } roleNameHMAC, err := createHMAC(role.HMACKey, roleName) if err != nil { - return nil, fmt.Errorf("failed to create HMAC of role_name: %s", err) + return nil, fmt.Errorf("failed to create HMAC of role_name: %v", err) } entryIndex := fmt.Sprintf("secret_id/%s/%s", roleNameHMAC, accessorEntry.SecretIDHMAC) @@ -1105,7 +1174,7 @@ func (b *backend) pathRoleSecretIDAccessorDestroyUpdateDelete(req *logical.Reque return nil, err } if role == nil { - return nil, fmt.Errorf("role %s does not exist", roleName) + return nil, fmt.Errorf("role %q does not exist", roleName) } accessorEntry, err := b.secretIDAccessorEntry(req.Storage, secretIDAccessor) @@ -1113,12 +1182,12 @@ func (b *backend) pathRoleSecretIDAccessorDestroyUpdateDelete(req *logical.Reque return nil, err } if accessorEntry == nil { - return nil, fmt.Errorf("failed to find accessor entry for secret_id_accessor:%s\n", secretIDAccessor) + return nil, fmt.Errorf("failed to find accessor entry for secret_id_accessor: %q\n", secretIDAccessor) } roleNameHMAC, err := createHMAC(role.HMACKey, roleName) if err != nil { - return nil, fmt.Errorf("failed to create HMAC of role_name: %s", err) + return nil, fmt.Errorf("failed to create HMAC of role_name: %v", err) } entryIndex := fmt.Sprintf("secret_id/%s/%s", roleNameHMAC, accessorEntry.SecretIDHMAC) @@ -1134,7 +1203,7 @@ func (b *backend) pathRoleSecretIDAccessorDestroyUpdateDelete(req *logical.Reque // Delete the storage entry that corresponds to the SecretID if err := req.Storage.Delete(entryIndex); err != nil { - return nil, fmt.Errorf("failed to delete SecretID: %s", err) + return nil, fmt.Errorf("failed to delete secret_id: %v", err) } return nil, nil @@ -1146,6 +1215,11 @@ func (b *backend) pathRoleBoundCIDRListUpdate(req *logical.Request, data *framew return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + + // Re-read the role after grabbing the lock role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1154,11 +1228,6 @@ func (b *backend) pathRoleBoundCIDRListUpdate(req *logical.Request, data *framew return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.BoundCIDRList = strings.TrimSpace(data.Get("bound_cidr_list").(string)) if role.BoundCIDRList == "" { return logical.ErrorResponse("missing bound_cidr_list"), nil @@ -1167,7 +1236,7 @@ func (b *backend) pathRoleBoundCIDRListUpdate(req *logical.Request, data *framew if role.BoundCIDRList != "" { valid, err := cidrutil.ValidateCIDRListString(role.BoundCIDRList, ",") if err != nil { - return nil, fmt.Errorf("failed to validate CIDR blocks: %q", err) + return nil, fmt.Errorf("failed to validate CIDR blocks: %v", err) } if !valid { return logical.ErrorResponse("failed to validate CIDR blocks"), nil @@ -1183,6 +1252,10 @@ func (b *backend) pathRoleBoundCIDRListRead(req *logical.Request, data *framewor return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1202,6 +1275,10 @@ func (b *backend) pathRoleBoundCIDRListDelete(req *logical.Request, data *framew return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1210,11 +1287,6 @@ func (b *backend) pathRoleBoundCIDRListDelete(req *logical.Request, data *framew return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - // Deleting a field implies setting the value to it's default value. role.BoundCIDRList = data.GetDefaultOrZero("bound_cidr_list").(string) @@ -1227,6 +1299,10 @@ func (b *backend) pathRoleBindSecretIDUpdate(req *logical.Request, data *framewo return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1235,11 +1311,6 @@ func (b *backend) pathRoleBindSecretIDUpdate(req *logical.Request, data *framewo return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - if bindSecretIDRaw, ok := data.GetOk("bind_secret_id"); ok { role.BindSecretID = bindSecretIDRaw.(bool) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1254,6 +1325,10 @@ func (b *backend) pathRoleBindSecretIDRead(req *logical.Request, data *framework return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1273,6 +1348,10 @@ func (b *backend) pathRoleBindSecretIDDelete(req *logical.Request, data *framewo return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1281,11 +1360,6 @@ func (b *backend) pathRoleBindSecretIDDelete(req *logical.Request, data *framewo return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - // Deleting a field implies setting the value to it's default value. role.BindSecretID = data.GetDefaultOrZero("bind_secret_id").(bool) @@ -1298,6 +1372,10 @@ func (b *backend) pathRolePoliciesUpdate(req *logical.Request, data *framework.F return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1311,11 +1389,6 @@ func (b *backend) pathRolePoliciesUpdate(req *logical.Request, data *framework.F return logical.ErrorResponse("missing policies"), nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.Policies = policyutil.ParsePolicies(policiesRaw) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1327,6 +1400,10 @@ func (b *backend) pathRolePoliciesRead(req *logical.Request, data *framework.Fie return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1346,6 +1423,10 @@ func (b *backend) pathRolePoliciesDelete(req *logical.Request, data *framework.F return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1354,11 +1435,6 @@ func (b *backend) pathRolePoliciesDelete(req *logical.Request, data *framework.F return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.Policies = []string{} return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1370,6 +1446,10 @@ func (b *backend) pathRoleSecretIDNumUsesUpdate(req *logical.Request, data *fram return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1378,11 +1458,6 @@ func (b *backend) pathRoleSecretIDNumUsesUpdate(req *logical.Request, data *fram return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - if numUsesRaw, ok := data.GetOk("secret_id_num_uses"); ok { role.SecretIDNumUses = numUsesRaw.(int) if role.SecretIDNumUses < 0 { @@ -1400,6 +1475,10 @@ func (b *backend) pathRoleRoleIDUpdate(req *logical.Request, data *framework.Fie return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1408,11 +1487,6 @@ func (b *backend) pathRoleRoleIDUpdate(req *logical.Request, data *framework.Fie return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - previousRoleID := role.RoleID role.RoleID = data.Get("role_id").(string) if role.RoleID == "" { @@ -1428,6 +1502,10 @@ func (b *backend) pathRoleRoleIDRead(req *logical.Request, data *framework.Field return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1447,6 +1525,10 @@ func (b *backend) pathRoleSecretIDNumUsesRead(req *logical.Request, data *framew return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1466,6 +1548,10 @@ func (b *backend) pathRoleSecretIDNumUsesDelete(req *logical.Request, data *fram return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1474,11 +1560,6 @@ func (b *backend) pathRoleSecretIDNumUsesDelete(req *logical.Request, data *fram return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.SecretIDNumUses = data.GetDefaultOrZero("secret_id_num_uses").(int) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1490,6 +1571,10 @@ func (b *backend) pathRoleSecretIDTTLUpdate(req *logical.Request, data *framewor return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1498,11 +1583,6 @@ func (b *backend) pathRoleSecretIDTTLUpdate(req *logical.Request, data *framewor return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - if secretIDTTLRaw, ok := data.GetOk("secret_id_ttl"); ok { role.SecretIDTTL = time.Second * time.Duration(secretIDTTLRaw.(int)) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1517,6 +1597,10 @@ func (b *backend) pathRoleSecretIDTTLRead(req *logical.Request, data *framework. return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1537,6 +1621,10 @@ func (b *backend) pathRoleSecretIDTTLDelete(req *logical.Request, data *framewor return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1545,11 +1633,6 @@ func (b *backend) pathRoleSecretIDTTLDelete(req *logical.Request, data *framewor return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.SecretIDTTL = time.Second * time.Duration(data.GetDefaultOrZero("secret_id_ttl").(int)) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1561,6 +1644,10 @@ func (b *backend) pathRolePeriodUpdate(req *logical.Request, data *framework.Fie return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1569,15 +1656,10 @@ func (b *backend) pathRolePeriodUpdate(req *logical.Request, data *framework.Fie return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - if periodRaw, ok := data.GetOk("period"); ok { role.Period = time.Second * time.Duration(periodRaw.(int)) if role.Period > b.System().MaxLeaseTTL() { - return logical.ErrorResponse(fmt.Sprintf("'period' of '%s' is greater than the backend's maximum lease TTL of '%s'", role.Period.String(), b.System().MaxLeaseTTL().String())), nil + return logical.ErrorResponse(fmt.Sprintf("period of %q is greater than the backend's maximum lease TTL of %q", role.Period.String(), b.System().MaxLeaseTTL().String())), nil } return nil, b.setRoleEntry(req.Storage, roleName, role, "") } else { @@ -1591,6 +1673,10 @@ func (b *backend) pathRolePeriodRead(req *logical.Request, data *framework.Field return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1611,6 +1697,10 @@ func (b *backend) pathRolePeriodDelete(req *logical.Request, data *framework.Fie return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1619,11 +1709,6 @@ func (b *backend) pathRolePeriodDelete(req *logical.Request, data *framework.Fie return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.Period = time.Second * time.Duration(data.GetDefaultOrZero("period").(int)) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1635,6 +1720,10 @@ func (b *backend) pathRoleTokenNumUsesUpdate(req *logical.Request, data *framewo return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1643,11 +1732,6 @@ func (b *backend) pathRoleTokenNumUsesUpdate(req *logical.Request, data *framewo return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - if tokenNumUsesRaw, ok := data.GetOk("token_num_uses"); ok { role.TokenNumUses = tokenNumUsesRaw.(int) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1662,6 +1746,10 @@ func (b *backend) pathRoleTokenNumUsesRead(req *logical.Request, data *framework return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1681,6 +1769,10 @@ func (b *backend) pathRoleTokenNumUsesDelete(req *logical.Request, data *framewo return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1689,11 +1781,6 @@ func (b *backend) pathRoleTokenNumUsesDelete(req *logical.Request, data *framewo return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.TokenNumUses = data.GetDefaultOrZero("token_num_uses").(int) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1705,6 +1792,10 @@ func (b *backend) pathRoleTokenTTLUpdate(req *logical.Request, data *framework.F return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1713,11 +1804,6 @@ func (b *backend) pathRoleTokenTTLUpdate(req *logical.Request, data *framework.F return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - if tokenTTLRaw, ok := data.GetOk("token_ttl"); ok { role.TokenTTL = time.Second * time.Duration(tokenTTLRaw.(int)) if role.TokenMaxTTL > time.Duration(0) && role.TokenTTL > role.TokenMaxTTL { @@ -1735,6 +1821,10 @@ func (b *backend) pathRoleTokenTTLRead(req *logical.Request, data *framework.Fie return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1755,6 +1845,10 @@ func (b *backend) pathRoleTokenTTLDelete(req *logical.Request, data *framework.F return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1763,11 +1857,6 @@ func (b *backend) pathRoleTokenTTLDelete(req *logical.Request, data *framework.F return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.TokenTTL = time.Second * time.Duration(data.GetDefaultOrZero("token_ttl").(int)) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1779,6 +1868,10 @@ func (b *backend) pathRoleTokenMaxTTLUpdate(req *logical.Request, data *framewor return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1787,11 +1880,6 @@ func (b *backend) pathRoleTokenMaxTTLUpdate(req *logical.Request, data *framewor return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - if tokenMaxTTLRaw, ok := data.GetOk("token_max_ttl"); ok { role.TokenMaxTTL = time.Second * time.Duration(tokenMaxTTLRaw.(int)) if role.TokenMaxTTL > time.Duration(0) && role.TokenTTL > role.TokenMaxTTL { @@ -1809,6 +1897,10 @@ func (b *backend) pathRoleTokenMaxTTLRead(req *logical.Request, data *framework. return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { return nil, err } else if role == nil { @@ -1829,6 +1921,10 @@ func (b *backend) pathRoleTokenMaxTTLDelete(req *logical.Request, data *framewor return logical.ErrorResponse("missing role_name"), nil } + lock := b.roleLock(roleName) + lock.Lock() + defer lock.Unlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err @@ -1837,11 +1933,6 @@ func (b *backend) pathRoleTokenMaxTTLDelete(req *logical.Request, data *framewor return nil, nil } - lock := b.roleLock(roleName) - - lock.Lock() - defer lock.Unlock() - role.TokenMaxTTL = time.Second * time.Duration(data.GetDefaultOrZero("token_max_ttl").(int)) return nil, b.setRoleEntry(req.Storage, roleName, role, "") @@ -1850,7 +1941,7 @@ func (b *backend) pathRoleTokenMaxTTLDelete(req *logical.Request, data *framewor func (b *backend) pathRoleSecretIDUpdate(req *logical.Request, data *framework.FieldData) (*logical.Response, error) { secretID, err := uuid.GenerateUUID() if err != nil { - return nil, fmt.Errorf("failed to generate SecretID:%s", err) + return nil, fmt.Errorf("failed to generate secret_id: %v", err) } return b.handleRoleSecretIDCommon(req, data, secretID) } @@ -1869,12 +1960,16 @@ func (b *backend) handleRoleSecretIDCommon(req *logical.Request, data *framework return logical.ErrorResponse("missing secret_id"), nil } + lock := b.roleLock(roleName) + lock.RLock() + defer lock.RUnlock() + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { return nil, err } if role == nil { - return logical.ErrorResponse(fmt.Sprintf("role %s does not exist", roleName)), nil + return logical.ErrorResponse(fmt.Sprintf("role %q does not exist", roleName)), nil } if !role.BindSecretID { @@ -1887,7 +1982,7 @@ func (b *backend) handleRoleSecretIDCommon(req *logical.Request, data *framework if cidrList != "" { valid, err := cidrutil.ValidateCIDRListString(cidrList, ",") if err != nil { - return nil, fmt.Errorf("failed to validate CIDR blocks: %q", err) + return nil, fmt.Errorf("failed to validate CIDR blocks: %v", err) } if !valid { return logical.ErrorResponse("failed to validate CIDR blocks"), nil @@ -1914,7 +2009,7 @@ func (b *backend) handleRoleSecretIDCommon(req *logical.Request, data *framework } if secretIDStorage, err = b.registerSecretIDEntry(req.Storage, roleName, secretID, role.HMACKey, secretIDStorage); err != nil { - return nil, fmt.Errorf("failed to store SecretID: %s", err) + return nil, fmt.Errorf("failed to store secret_id: %v", err) } return &logical.Response{ diff --git a/builtin/credential/approle/path_role_test.go b/builtin/credential/approle/path_role_test.go index fa3e68150918..0c70e73f02ea 100644 --- a/builtin/credential/approle/path_role_test.go +++ b/builtin/credential/approle/path_role_test.go @@ -2,6 +2,7 @@ package approle import ( "reflect" + "strings" "testing" "time" @@ -10,6 +11,87 @@ import ( "github.com/mitchellh/mapstructure" ) +func TestAppRole_RoleReadSetIndex(t *testing.T) { + var resp *logical.Response + var err error + + b, storage := createBackendWithStorage(t) + + roleReq := &logical.Request{ + Path: "role/testrole", + Operation: logical.CreateOperation, + Storage: storage, + Data: map[string]interface{}{ + "bind_secret_id": true, + }, + } + + // Create a role + resp, err = b.HandleRequest(roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("bad: resp: %#v\n err: %v\n", resp, err) + } + + roleIDReq := &logical.Request{ + Path: "role/testrole/role-id", + Operation: logical.ReadOperation, + Storage: storage, + } + + // Get the role ID + resp, err = b.HandleRequest(roleIDReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("bad: resp: %#v\n err: %v\n", resp, err) + } + roleID := resp.Data["role_id"].(string) + + // Delete the role ID index + err = b.roleIDEntryDelete(storage, roleID) + if err != nil { + t.Fatal(err) + } + + // Read the role again. This should add the index and return a warning + roleReq.Operation = logical.ReadOperation + resp, err = b.HandleRequest(roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("bad: resp: %#v\n err: %v\n", resp, err) + } + + // Check if the warning is being returned + if !strings.Contains(resp.Warnings[0], "Role identifier was missing an index back to role name.") { + t.Fatalf("bad: expected a warning in the response") + } + + roleIDIndex, err := b.roleIDEntry(storage, roleID) + if err != nil { + t.Fatal(err) + } + + // Check if the index has been successfully created + if roleIDIndex == nil || roleIDIndex.Name != "testrole" { + t.Fatalf("bad: expected role to have an index") + } + + roleReq.Operation = logical.UpdateOperation + roleReq.Data = map[string]interface{}{ + "bind_secret_id": true, + "policies": "default", + } + + // Check if updating and reading of roles work and that there are no lock + // contentions dangling due to previous operation + resp, err = b.HandleRequest(roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("bad: resp: %#v\n err: %v\n", resp, err) + } + roleReq.Operation = logical.ReadOperation + resp, err = b.HandleRequest(roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("bad: resp: %#v\n err: %v\n", resp, err) + } +} + func TestAppRole_CIDRSubset(t *testing.T) { var resp *logical.Response var err error diff --git a/builtin/credential/approle/validation.go b/builtin/credential/approle/validation.go index ac834c5e1155..092b8f3665c6 100644 --- a/builtin/credential/approle/validation.go +++ b/builtin/credential/approle/validation.go @@ -75,15 +75,19 @@ func (b *backend) validateRoleID(s logical.Storage, roleID string) (*roleStorage return nil, "", err } if roleIDIndex == nil { - return nil, "", fmt.Errorf("failed to find secondary index for role_id %q\n", roleID) + return nil, "", fmt.Errorf("invalid role_id %q\n", roleID) } + lock := b.roleLock(roleIDIndex.Name) + lock.RLock() + defer lock.RUnlock() + role, err := b.roleEntry(s, roleIDIndex.Name) if err != nil { return nil, "", err } if role == nil { - return nil, "", fmt.Errorf("role %q referred by the SecretID does not exist", roleIDIndex.Name) + return nil, "", fmt.Errorf("role %q referred by the role_id %q does not exist anymore", roleIDIndex.Name, roleID) } return role, roleIDIndex.Name, nil