From ba72e7887acc37ef4ad29b7cd1ec8e5bc36aeabc Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 15 Sep 2016 17:49:14 -0400 Subject: [PATCH] Safely manipulate secret id accessors --- builtin/credential/approle/backend.go | 30 +++++++--- builtin/credential/approle/path_role.go | 11 ++-- builtin/credential/approle/validation.go | 72 +++++++++++++++++++----- 3 files changed, 83 insertions(+), 30 deletions(-) diff --git a/builtin/credential/approle/backend.go b/builtin/credential/approle/backend.go index 3e96c3c38..306ae4a9c 100644 --- a/builtin/credential/approle/backend.go +++ b/builtin/credential/approle/backend.go @@ -20,22 +20,28 @@ type backend struct { // Guard to clean-up the expired SecretID entries tidySecretIDCASGuard uint32 - // Map of locks to make changes to role entries. This will be initiated - // to a predefined number of locks when the backend is created, and - // will be indexed based on salted role names. + // Map of locks to make changes to role entries. These will be + // initialized to a predefined number of locks when the backend is + // created, and will be indexed based on salted role names. roleLocksMap map[string]*sync.RWMutex // Map of locks to make changes to the storage entries of RoleIDs - // generated. This will be initiated to a predefined number of locks + // generated. These will be initialized to a predefined number of locks // when the backend is created, and will be indexed based on the salted // RoleIDs. roleIDLocksMap map[string]*sync.RWMutex // Map of locks to make changes to the storage entries of SecretIDs - // generated. This will be initiated to a predefined number of locks + // generated. These will be initialized to a predefined number of locks // when the backend is created, and will be indexed based on the HMAC-ed // SecretIDs. secretIDLocksMap map[string]*sync.RWMutex + + // Map of locks to make changes to the storage entries of + // SecretIDAccessors generated. These will be initialized to a + // predefined number of locks when the backend is created, and will be + // indexed based on the SecretIDAccessors itself. + secretIDAccessorLocksMap map[string]*sync.RWMutex } func Factory(conf *logical.BackendConfig) (logical.Backend, error) { @@ -63,11 +69,14 @@ func Backend(conf *logical.BackendConfig) (*backend, error) { // Create the map of locks to modify the registered roles roleLocksMap: make(map[string]*sync.RWMutex, 257), - // Create the map of locks to modify the generated RoleIDs. + // Create the map of locks to modify the generated RoleIDs roleIDLocksMap: make(map[string]*sync.RWMutex, 257), - // Create the map of locks to modify the generated SecretIDs. + // Create the map of locks to modify the generated SecretIDs secretIDLocksMap: make(map[string]*sync.RWMutex, 257), + + // Create the map of locks to modify the generated SecretIDAccessors + secretIDAccessorLocksMap: make(map[string]*sync.RWMutex, 257), } // Create 256 locks each for managing RoleID and SecretIDs. This will avoid @@ -86,12 +95,17 @@ func Backend(conf *logical.BackendConfig) (*backend, error) { return nil, fmt.Errorf("failed to create secret ID locks: %v", err) } + if err = locksutil.CreateLocks(b.secretIDAccessorLocksMap, 256); err != nil { + return nil, fmt.Errorf("failed to create secret ID accessor locks: %v", err) + } + // Have an extra lock to use in case the indexing does not result in a lock. // This happens if the indexing value is not beginning with hex characters. // These locks can be used for listing purposes as well. b.roleLocksMap["custom"] = &sync.RWMutex{} - b.secretIDLocksMap["custom"] = &sync.RWMutex{} b.roleIDLocksMap["custom"] = &sync.RWMutex{} + b.secretIDLocksMap["custom"] = &sync.RWMutex{} + b.secretIDAccessorLocksMap["custom"] = &sync.RWMutex{} // Attach the paths and secrets that are to be handled by the backend b.Backend = &framework.Backend{ diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 29b641c00..3b33a8165 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -920,11 +920,9 @@ func (b *backend) pathRoleSecretIDSecretIDDelete(req *logical.Request, data *fra return nil, err } - accessorEntryIndex := "accessor/" + b.salt.SaltID(result.SecretIDAccessor) - // Delete the accessor of the SecretID first - if err := req.Storage.Delete(accessorEntryIndex); err != nil { - return nil, fmt.Errorf("failed to delete accessor storage entry: %s", err) + if err := b.deleteSecretIDAccessorEntry(req.Storage, result.SecretIDAccessor); err != nil { + return nil, err } // Delete the storage entry that corresponds to the SecretID @@ -1014,15 +1012,14 @@ func (b *backend) pathRoleSecretIDAccessorDelete(req *logical.Request, data *fra } entryIndex := fmt.Sprintf("secret_id/%s/%s", roleNameHMAC, accessorEntry.SecretIDHMAC) - accessorEntryIndex := "accessor/" + b.salt.SaltID(secretIDAccessor) lock := b.secretIDLock(accessorEntry.SecretIDHMAC) lock.Lock() defer lock.Unlock() // Delete the accessor of the SecretID first - if err := req.Storage.Delete(accessorEntryIndex); err != nil { - return nil, fmt.Errorf("failed to delete accessor storage entry: %s", err) + if err := b.deleteSecretIDAccessorEntry(req.Storage, secretIDAccessor); err != nil { + return nil, err } // Delete the storage entry that corresponds to the SecretID diff --git a/builtin/credential/approle/validation.go b/builtin/credential/approle/validation.go index 91c0368db..4e32e2ba5 100644 --- a/builtin/credential/approle/validation.go +++ b/builtin/credential/approle/validation.go @@ -222,9 +222,9 @@ func (b *backend) validateBindSecretID(s logical.Storage, roleName, secretID, hm // the storage but do not fail the validation request. Subsequest // requests to use the same SecretID will fail. if result.SecretIDNumUses == 1 { - accessorEntryIndex := "accessor/" + b.salt.SaltID(result.SecretIDAccessor) - if err := s.Delete(accessorEntryIndex); err != nil { - return false, nil, fmt.Errorf("failed to delete accessor storage entry: %s", err) + // Delete the secret IDs accessor first + if err := b.deleteSecretIDAccessorEntry(s, result.SecretIDAccessor); err != nil { + return false, nil, err } if err := s.Delete(entryIndex); err != nil { return false, nil, fmt.Errorf("failed to delete SecretID: %s", err) @@ -243,8 +243,8 @@ func (b *backend) validateBindSecretID(s logical.Storage, roleName, secretID, hm return true, result.Metadata, nil } -// Creates a SHA256 HMAC of the given 'value' using the given 'key' -// and returns a hex encoded string. +// Creates a SHA256 HMAC of the given 'value' using the given 'key' and returns +// a hex encoded string. func createHMAC(key, value string) (string, error) { if key == "" { return "", fmt.Errorf("invalid HMAC key") @@ -254,10 +254,9 @@ func createHMAC(key, value string) (string, error) { return hex.EncodeToString(hm.Sum(nil)), nil } -// secretIDLock is used to get a lock from the pre-initialized map -// of locks. Map is indexed based on the first 2 characters of the -// secretIDHMAC. If the input is not hex encoded or if empty, a -// "custom" lock will be returned. +// secretIDLock is used to get a lock from the pre-initialized map of locks. +// Map is indexed based on the first 2 characters of the secretIDHMAC. If the +// input is not hex encoded or if empty, a "custom" lock will be returned. func (b *backend) secretIDLock(secretIDHMAC string) *sync.RWMutex { var lock *sync.RWMutex var ok bool @@ -265,12 +264,31 @@ func (b *backend) secretIDLock(secretIDHMAC string) *sync.RWMutex { lock, ok = b.secretIDLocksMap[secretIDHMAC[0:2]] } if !ok || lock == nil { - // Fall back for custom SecretIDs + // Fall back for custom lock to make sure that this method + // never returns nil lock = b.secretIDLocksMap["custom"] } return lock } +// secretIDAccessorLock is used to get a lock from the pre-initialized map +// of locks. Map is indexed based on the first 2 characters of the +// secretIDAccessor. If the input is not hex encoded or if empty, a "custom" +// lock will be returned. +func (b *backend) secretIDAccessorLock(secretIDAccessor string) *sync.RWMutex { + var lock *sync.RWMutex + var ok bool + if len(secretIDAccessor) >= 2 { + lock, ok = b.secretIDAccessorLocksMap[secretIDAccessor[0:2]] + } + if !ok || lock == nil { + // Fall back for custom lock to make sure that this method + // never returns nil + lock = b.secretIDAccessorLocksMap["custom"] + } + return lock +} + // registerSecretIDEntry creates a new storage entry for the given SecretID. func (b *backend) registerSecretIDEntry(s logical.Storage, roleName, secretID, hmacKey string, secretEntry *secretIDStorageEntry) (*secretIDStorageEntry, error) { secretIDHMAC, err := createHMAC(hmacKey, secretID) @@ -331,7 +349,7 @@ func (b *backend) registerSecretIDEntry(s logical.Storage, roleName, secretID, h } // Before storing the SecretID, store its accessor. - if err := b.createAccessor(s, secretEntry, secretIDHMAC); err != nil { + if err := b.createSecretIDAccessorEntry(s, secretEntry, secretIDHMAC); err != nil { return nil, err } @@ -345,8 +363,7 @@ func (b *backend) registerSecretIDEntry(s logical.Storage, roleName, secretID, h } // secretIDAccessorEntry is used to read the storage entry that maps an -// accessor to a secret_id. This method should be called when the lock -// for the corresponding SecretID is held. +// accessor to a secret_id. func (b *backend) secretIDAccessorEntry(s logical.Storage, secretIDAccessor string) (*secretIDAccessorStorageEntry, error) { if secretIDAccessor == "" { return nil, fmt.Errorf("missing secretIDAccessor") @@ -357,6 +374,10 @@ func (b *backend) secretIDAccessorEntry(s logical.Storage, secretIDAccessor stri // Create index entry, mapping the accessor to the token ID entryIndex := "accessor/" + b.salt.SaltID(secretIDAccessor) + accessorLock := b.secretIDAccessorLock(secretIDAccessor) + accessorLock.RLock() + defer accessorLock.RUnlock() + if entry, err := s.Get(entryIndex); err != nil { return nil, err } else if entry == nil { @@ -368,10 +389,10 @@ func (b *backend) secretIDAccessorEntry(s logical.Storage, secretIDAccessor stri return &result, nil } -// createAccessor creates an identifier for the SecretID. A storage index, +// createSecretIDAccessorEntry creates an identifier for the SecretID. A storage index, // mapping the accessor to the SecretID is also created. This method should // be called when the lock for the corresponding SecretID is held. -func (b *backend) createAccessor(s logical.Storage, entry *secretIDStorageEntry, secretIDHMAC string) error { +func (b *backend) createSecretIDAccessorEntry(s logical.Storage, entry *secretIDStorageEntry, secretIDHMAC string) error { // Create a random accessor accessorUUID, err := uuid.GenerateUUID() if err != nil { @@ -381,6 +402,11 @@ func (b *backend) createAccessor(s logical.Storage, entry *secretIDStorageEntry, // Create index entry, mapping the accessor to the token ID entryIndex := "accessor/" + b.salt.SaltID(entry.SecretIDAccessor) + + accessorLock := b.secretIDAccessorLock(accessorUUID) + accessorLock.Lock() + defer accessorLock.Unlock() + if entry, err := logical.StorageEntryJSON(entryIndex, &secretIDAccessorStorageEntry{ SecretIDHMAC: secretIDHMAC, }); err != nil { @@ -392,6 +418,22 @@ func (b *backend) createAccessor(s logical.Storage, entry *secretIDStorageEntry, return nil } +// deleteSecretIDAccessorEntry deletes the storage index mapping the accessor to a SecretID. +func (b *backend) deleteSecretIDAccessorEntry(s logical.Storage, secretIDAccessor string) error { + accessorEntryIndex := "accessor/" + b.salt.SaltID(secretIDAccessor) + + accessorLock := b.secretIDAccessorLock(secretIDAccessor) + accessorLock.Lock() + defer accessorLock.Unlock() + + // Delete the accessor of the SecretID first + if err := s.Delete(accessorEntryIndex); err != nil { + return fmt.Errorf("failed to delete accessor storage entry: %s", err) + } + + return nil +} + // flushRoleSecrets deletes all the SecretIDs that belong to the given // RoleID. func (b *backend) flushRoleSecrets(s logical.Storage, roleName, hmacKey string) error {