From 5a12dc932c2c2e6fdb0fc232e63aac0c4b5fed05 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 9 Nov 2017 11:22:51 -0500 Subject: [PATCH 1/5] avoid race conditions in approle --- builtin/credential/approle/path_login.go | 8 +- builtin/credential/approle/path_role.go | 333 +++++++++++++---------- builtin/credential/approle/validation.go | 8 +- 3 files changed, 205 insertions(+), 144 deletions(-) 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..30133afb8bb6 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,6 +855,10 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) 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 { @@ -878,6 +894,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 +906,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 +936,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 +1015,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 +1059,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 +1082,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 +1099,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 +1132,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 +1140,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 +1161,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 +1173,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 +1186,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 +1194,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 +1210,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 +1233,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 +1245,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 +1257,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 +1269,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 +1283,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 +1306,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 +1318,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 +1330,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 +1347,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 +1358,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 +1381,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 +1393,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 +1404,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 +1416,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 +1433,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 +1445,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 +1460,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 +1483,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 +1506,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 +1518,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 +1529,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 +1541,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 +1555,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 +1579,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 +1591,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 +1602,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 +1614,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 +1631,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 +1655,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 +1667,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 +1678,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 +1690,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 +1704,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 +1727,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 +1739,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 +1750,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 +1762,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 +1779,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 +1803,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 +1815,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 +1826,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 +1838,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 +1855,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 +1879,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 +1891,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 +1899,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 +1918,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 +1940,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 +1967,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/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 From 599734cc049b3f2e0bd601c82b9706a30b101baa Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 9 Nov 2017 12:03:58 -0500 Subject: [PATCH 2/5] return a warning from role read if secondary index is missing --- builtin/credential/approle/path_role.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 30133afb8bb6..f4c606c314d1 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -883,6 +883,18 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) resp.AddWarning("Role does not have any constraints set on it. Updates to this role will require a constraint to be set") } + // People have been complaining about role becoming useless despite + // being able to read it. For sanity, verify that the index from + // role_id to role_name still exists. If the index is missing, return a + // warning. + roleIDIndex, err := b.roleIDEntry(req.Storage, role.ID) + if err != nil { + return nil, err + } + if roleIDIndex == nil { + resp.AddWarning("Role identifier to role name index is missing") + } + return resp, nil } } From 03f50e10f538937e64bcc85be1846598830def87 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 9 Nov 2017 15:04:21 -0500 Subject: [PATCH 3/5] Create a role ID index if a role is missing one --- builtin/credential/approle/path_role.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index f4c606c314d1..0a0f2ff6da79 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -884,15 +884,24 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) } // People have been complaining about role becoming useless despite - // being able to read it. For sanity, verify that the index from - // role_id to role_name still exists. If the index is missing, return a - // warning. - roleIDIndex, err := b.roleIDEntry(req.Storage, role.ID) + // being able to read it. It seems that the reason for that is not + // being able to find the role_id to role_name index. 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 { return nil, err } if roleIDIndex == nil { - resp.AddWarning("Role identifier to role name index is missing") + // The index should never be nil for a valid role. If it is, create + // a new one. + err = b.setRoleIDEntry(req.Storage, role.RoleID, &roleIDStorageEntry{ + Name: roleName, + }) + if err != nil { + 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.") } return resp, nil From 6d67c3f5e2f79e2e309bc152cb870e6211c08703 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 10 Nov 2017 10:47:37 -0500 Subject: [PATCH 4/5] Fix locking in approle read and add test --- builtin/credential/approle/path_role.go | 80 ++++++++++++------- builtin/credential/approle/path_role_test.go | 82 ++++++++++++++++++++ 2 files changed, 134 insertions(+), 28 deletions(-) diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 0a0f2ff6da79..4ab907e4faa5 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -857,55 +857,79 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) lock := b.roleLock(roleName) lock.RLock() - defer lock.RUnlock() + writeLockHeld := false - if role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)); err != nil { + role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) + if err != nil { + lock.RUnlock() return nil, err - } else if role == nil { + } + + if role == nil { + lock.RUnlock() 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") + // Convert the 'time.Duration' values to second. + role.SecretIDTTL /= time.Second + role.TokenTTL /= time.Second + role.TokenMaxTTL /= time.Second + role.Period /= time.Second - resp := &logical.Response{ - Data: data, - } + // Create a map of data to be returned and remove sensitive information from it + respData := structs.New(role).Map() + delete(respData, "role_id") + delete(respData, "hmac_key") - 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") - } + 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") + } - // People have been complaining about role becoming useless despite - // being able to read it. It seems that the reason for that is not - // being able to find the role_id to role_name index. 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) + // 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 { + lock.RUnlock() + return nil, err + } + + if roleIDIndex == nil { + // Switch to a write lock + lock.RUnlock() + lock.Lock() + writeLockHeld = true + + // Check again if the index is missing + roleIDIndex, err = b.roleIDEntry(req.Storage, role.RoleID) if err != nil { + lock.Unlock() return nil, err } + if roleIDIndex == nil { - // The index should never be nil for a valid role. If it is, create - // a new one. + // Create a new index err = b.setRoleIDEntry(req.Storage, role.RoleID, &roleIDStorageEntry{ Name: roleName, }) if err != nil { + lock.Unlock() 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.") } + } - return resp, nil + if writeLockHeld { + lock.Unlock() + } else { + lock.RUnlock() } + + return resp, nil } // pathRoleDelete removes the role from the storage 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 From 007075eaeb7972ac082ae2fa9aadac6de5b67a4a Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 10 Nov 2017 11:18:41 -0500 Subject: [PATCH 5/5] address review feedback --- builtin/credential/approle/path_role.go | 41 ++++++++++++------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 4ab907e4faa5..ec5a22ea148b 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -857,29 +857,30 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) lock := b.roleLock(roleName) lock.RLock() - writeLockHeld := false + lockRelease := lock.RUnlock role, err := b.roleEntry(req.Storage, strings.ToLower(roleName)) if err != nil { - lock.RUnlock() + lockRelease() return nil, err } if role == nil { - lock.RUnlock() + lockRelease() return nil, nil } - // 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 - respData := structs.New(role).Map() - delete(respData, "role_id") - delete(respData, "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: respData, @@ -893,7 +894,7 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) // add one and return a warning so it can be reported. roleIDIndex, err := b.roleIDEntry(req.Storage, role.RoleID) if err != nil { - lock.RUnlock() + lockRelease() return nil, err } @@ -901,12 +902,12 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) // Switch to a write lock lock.RUnlock() lock.Lock() - writeLockHeld = true + lockRelease = lock.Unlock // Check again if the index is missing roleIDIndex, err = b.roleIDEntry(req.Storage, role.RoleID) if err != nil { - lock.Unlock() + lockRelease() return nil, err } @@ -916,18 +917,14 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData) Name: roleName, }) if err != nil { - lock.Unlock() + 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.") } } - if writeLockHeld { - lock.Unlock() - } else { - lock.RUnlock() - } + lockRelease() return resp, nil }