diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 84347b0f1b..4ad0dfc6dd 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -604,6 +604,11 @@ func (b *backend) setRoleEntry(s logical.Storage, roleName string, role *roleSto return err } + // If previousRoleID is still intact, don't create another one + if previousRoleID != "" { + return nil + } + // Create a storage entry for reverse mapping of RoleID to role. // Note that secondary index is created when the roleLock is held. return b.setRoleIDEntry(s, role.RoleID, &roleIDStorageEntry{ @@ -617,7 +622,7 @@ func (b *backend) roleEntry(s logical.Storage, roleName string) (*roleStorageEnt return nil, fmt.Errorf("missing role_name") } - var result roleStorageEntry + var role roleStorageEntry lock := b.roleLock(roleName) @@ -628,11 +633,11 @@ func (b *backend) roleEntry(s logical.Storage, roleName string) (*roleStorageEnt return nil, err } else if entry == nil { return nil, nil - } else if err := entry.DecodeJSON(&result); err != nil { + } else if err := entry.DecodeJSON(&role); err != nil { return nil, err } - return &result, nil + return &role, nil } // pathRoleCreateUpdate registers a new role with the backend or updates the options @@ -896,9 +901,15 @@ func (b *backend) secretIDCommon(s logical.Storage, entryIndex, secretIDHMAC str d["expiration_time"] = result.ExpirationTime.Format(time.RFC3339Nano) d["last_updated_time"] = result.LastUpdatedTime.Format(time.RFC3339Nano) - return &logical.Response{ + resp := &logical.Response{ Data: d, - }, nil + } + + if _, ok := d["SecretIDNumUses"]; ok { + resp.AddWarning("The field SecretIDNumUses is deprecated and will be removed in a future release; refer to secret_id_num_uses instead") + } + + return resp, nil } func (b *backend) pathRoleSecretIDSecretIDDelete(req *logical.Request, data *framework.FieldData) (*logical.Response, error) { diff --git a/builtin/credential/approle/validation.go b/builtin/credential/approle/validation.go index baaeef7fd0..133082da21 100644 --- a/builtin/credential/approle/validation.go +++ b/builtin/credential/approle/validation.go @@ -52,6 +52,9 @@ type secretIDStorageEntry struct { // CIDRList is a set of CIDR blocks that impose source address // restrictions on the usage of SecretID CIDRList []string `json:"cidr_list" structs:"cidr_list" mapstructure:"cidr_list"` + + // This is a deprecated field + SecretIDNumUsesDeprecated int `json:"SecretIDNumUses" structs:"SecretIDNumUses" mapstructure:"SecretIDNumUses"` } // Represents the payload of the storage entry of the accessor that maps to a @@ -72,7 +75,7 @@ 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:%s\n", roleID) + return nil, "", fmt.Errorf("failed to find secondary index for role_id %q\n", roleID) } role, err := b.roleEntry(s, roleIDIndex.Name) @@ -80,7 +83,7 @@ func (b *backend) validateRoleID(s logical.Storage, roleID string) (*roleStorage return nil, "", err } if role == nil { - return nil, "", fmt.Errorf("role %s referred by the SecretID does not exist", roleIDIndex.Name) + return nil, "", fmt.Errorf("role %q referred by the SecretID does not exist", roleIDIndex.Name) } return role, roleIDIndex.Name, nil @@ -125,7 +128,7 @@ func (b *backend) validateCredentials(req *logical.Request, data *framework.Fiel return nil, "", metadata, err } if !valid { - return nil, "", metadata, fmt.Errorf("invalid secret_id: %s\n", secretID) + return nil, "", metadata, fmt.Errorf("invalid secret_id %q", secretID) } } @@ -152,12 +155,12 @@ func (b *backend) validateBindSecretID(req *logical.Request, roleName, secretID, hmacKey, roleBoundCIDRList string) (bool, map[string]string, error) { secretIDHMAC, err := createHMAC(hmacKey, secretID) if err != nil { - return false, nil, fmt.Errorf("failed to create HMAC of secret_id: %s", err) + return false, nil, fmt.Errorf("failed to create HMAC of secret_id: %v", err) } roleNameHMAC, err := createHMAC(hmacKey, roleName) if err != nil { - return false, nil, fmt.Errorf("failed to create HMAC of role_name: %s", err) + return false, nil, fmt.Errorf("failed to create HMAC of role_name: %v", err) } entryIndex := fmt.Sprintf("secret_id/%s/%s", roleNameHMAC, secretIDHMAC) @@ -168,16 +171,13 @@ func (b *backend) validateBindSecretID(req *logical.Request, roleName, secretID, lock := b.secretIDLock(secretIDHMAC) lock.RLock() - result := secretIDStorageEntry{} - if entry, err := req.Storage.Get(entryIndex); err != nil { + result, err := b.nonLockedSecretIDStorageEntry(req.Storage, roleNameHMAC, secretIDHMAC) + if err != nil { lock.RUnlock() return false, nil, err - } else if entry == nil { + } else if result == nil { lock.RUnlock() return false, nil, nil - } else if err := entry.DecodeJSON(&result); err != nil { - lock.RUnlock() - return false, nil, err } // SecretIDNumUses will be zero only if the usage limit was not set at all, @@ -216,13 +216,12 @@ func (b *backend) validateBindSecretID(req *logical.Request, roleName, secretID, defer lock.Unlock() // Lock switching may change the data. Refresh the contents. - result = secretIDStorageEntry{} - if entry, err := req.Storage.Get(entryIndex); err != nil { + result, err = b.nonLockedSecretIDStorageEntry(req.Storage, roleNameHMAC, secretIDHMAC) + if err != nil { return false, nil, err - } else if entry == nil { + } + if result == nil { return false, nil, nil - } else if err := entry.DecodeJSON(&result); err != nil { - return false, nil, err } // If there exists a single use left, delete the SecretID entry from @@ -234,16 +233,16 @@ func (b *backend) validateBindSecretID(req *logical.Request, roleName, secretID, return false, nil, err } if err := req.Storage.Delete(entryIndex); err != nil { - return false, nil, fmt.Errorf("failed to delete SecretID: %s", err) + return false, nil, fmt.Errorf("failed to delete secret ID: %v", err) } } else { // If the use count is greater than one, decrement it and update the last updated time. result.SecretIDNumUses -= 1 result.LastUpdatedTime = time.Now() if entry, err := logical.StorageEntryJSON(entryIndex, &result); err != nil { - return false, nil, fmt.Errorf("failed to decrement the use count for SecretID:%s", secretID) + return false, nil, fmt.Errorf("failed to decrement the use count for secret ID %q", secretID) } else if err = req.Storage.Put(entry); err != nil { - return false, nil, fmt.Errorf("failed to decrement the use count for SecretID:%s", secretID) + return false, nil, fmt.Errorf("failed to decrement the use count for secret ID %q", secretID) } } @@ -335,23 +334,102 @@ func (b *backend) secretIDAccessorLock(secretIDAccessor string) *sync.RWMutex { 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) - if err != nil { - return nil, fmt.Errorf("failed to create HMAC of secret_id: %s", err) +// nonLockedSecretIDStorageEntry fetches the secret ID properties from physical +// storage. The entry will be indexed based on the given HMACs of both role +// name and the secret ID. This method will not acquire secret ID lock to fetch +// the storage entry. Locks need to be acquired before calling this method. +func (b *backend) nonLockedSecretIDStorageEntry(s logical.Storage, roleNameHMAC, secretIDHMAC string) (*secretIDStorageEntry, error) { + if secretIDHMAC == "" { + return nil, fmt.Errorf("missing secret ID HMAC") } - roleNameHMAC, err := createHMAC(hmacKey, roleName) + + if roleNameHMAC == "" { + return nil, fmt.Errorf("missing role name HMAC") + } + + // Prepare the storage index at which the secret ID will be stored + entryIndex := fmt.Sprintf("secret_id/%s/%s", roleNameHMAC, secretIDHMAC) + + entry, err := s.Get(entryIndex) if err != nil { - return nil, fmt.Errorf("failed to create HMAC of role_name: %s", err) + return nil, err + } + if entry == nil { + return nil, nil + } + + result := secretIDStorageEntry{} + if err := entry.DecodeJSON(&result); err != nil { + return nil, err + } + + // TODO: Remove this upgrade bit in future releases + persistNeeded := false + if result.SecretIDNumUsesDeprecated != 0 { + if result.SecretIDNumUses == 0 || + result.SecretIDNumUsesDeprecated < result.SecretIDNumUses { + result.SecretIDNumUses = result.SecretIDNumUsesDeprecated + persistNeeded = true + } + if result.SecretIDNumUses < result.SecretIDNumUsesDeprecated { + result.SecretIDNumUsesDeprecated = result.SecretIDNumUses + persistNeeded = true + } + } + + if persistNeeded { + if err := b.nonLockedSetSecretIDStorageEntry(s, roleNameHMAC, secretIDHMAC, &result); err != nil { + return nil, fmt.Errorf("failed to upgrade role storage entry", err) + } + } + + return &result, nil +} + +// nonLockedSetSecretIDStorageEntry creates or updates a secret ID entry at the +// physical storage. The entry will be indexed based on the given HMACs of both +// role name and the secret ID. This method will not acquire secret ID lock to +// create/update the storage entry. Locks need to be acquired before calling +// this method. +func (b *backend) nonLockedSetSecretIDStorageEntry(s logical.Storage, roleNameHMAC, secretIDHMAC string, secretEntry *secretIDStorageEntry) error { + if secretIDHMAC == "" { + return fmt.Errorf("missing secret ID HMAC") + } + + if roleNameHMAC == "" { + return fmt.Errorf("missing role name HMAC") + } + + if secretEntry == nil { + return fmt.Errorf("nil secret entry") } entryIndex := fmt.Sprintf("secret_id/%s/%s", roleNameHMAC, secretIDHMAC) + if entry, err := logical.StorageEntryJSON(entryIndex, secretEntry); err != nil { + return err + } else if err = s.Put(entry); err != nil { + return err + } + + return nil +} + +// 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) + if err != nil { + return nil, fmt.Errorf("failed to create HMAC of secret ID: %v", err) + } + roleNameHMAC, err := createHMAC(hmacKey, roleName) + if err != nil { + return nil, fmt.Errorf("failed to create HMAC of role_name: %v", err) + } + lock := b.secretIDLock(secretIDHMAC) lock.RLock() - entry, err := s.Get(entryIndex) + entry, err := b.nonLockedSecretIDStorageEntry(s, roleNameHMAC, secretIDHMAC) if err != nil { lock.RUnlock() return nil, err @@ -368,7 +446,7 @@ func (b *backend) registerSecretIDEntry(s logical.Storage, roleName, secretID, h defer lock.Unlock() // But before saving a new entry, check if the secretID entry was created during the lock switch. - entry, err = s.Get(entryIndex) + entry, err = b.nonLockedSecretIDStorageEntry(s, roleNameHMAC, secretIDHMAC) if err != nil { return nil, err } @@ -376,7 +454,9 @@ func (b *backend) registerSecretIDEntry(s logical.Storage, roleName, secretID, h return nil, fmt.Errorf("SecretID is already registered") } + // // Create a new entry for the SecretID + // // Set the creation time for the SecretID currentTime := time.Now() @@ -399,9 +479,7 @@ func (b *backend) registerSecretIDEntry(s logical.Storage, roleName, secretID, h return nil, err } - if entry, err := logical.StorageEntryJSON(entryIndex, secretEntry); err != nil { - return nil, err - } else if err = s.Put(entry); err != nil { + if err := b.nonLockedSetSecretIDStorageEntry(s, roleNameHMAC, secretIDHMAC, secretEntry); err != nil { return nil, err } @@ -458,7 +536,7 @@ func (b *backend) createSecretIDAccessorEntry(s logical.Storage, entry *secretID }); err != nil { return err } else if err = s.Put(entry); err != nil { - return fmt.Errorf("failed to persist accessor index entry: %s", err) + return fmt.Errorf("failed to persist accessor index entry: %v", err) } return nil @@ -474,7 +552,7 @@ func (b *backend) deleteSecretIDAccessorEntry(s logical.Storage, secretIDAccesso // 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 fmt.Errorf("failed to delete accessor storage entry: %v", err) } return nil @@ -485,7 +563,7 @@ func (b *backend) deleteSecretIDAccessorEntry(s logical.Storage, secretIDAccesso func (b *backend) flushRoleSecrets(s logical.Storage, roleName, hmacKey string) error { roleNameHMAC, err := createHMAC(hmacKey, roleName) if err != nil { - return fmt.Errorf("failed to create HMAC of role_name: %s", err) + return fmt.Errorf("failed to create HMAC of role_name: %v", err) } // Acquire the custom lock to perform listing of SecretIDs @@ -504,7 +582,7 @@ func (b *backend) flushRoleSecrets(s logical.Storage, roleName, hmacKey string) entryIndex := fmt.Sprintf("secret_id/%s/%s", roleNameHMAC, secretIDHMAC) if err := s.Delete(entryIndex); err != nil { lock.Unlock() - return fmt.Errorf("error deleting SecretID %s from storage: %s", secretIDHMAC, err) + return fmt.Errorf("error deleting SecretID %q from storage: %v", secretIDHMAC, err) } lock.Unlock() } diff --git a/builtin/credential/approle/validation_test.go b/builtin/credential/approle/validation_test.go new file mode 100644 index 0000000000..1111da05fc --- /dev/null +++ b/builtin/credential/approle/validation_test.go @@ -0,0 +1,61 @@ +package approle + +import ( + "testing" + + "github.com/hashicorp/vault/logical" +) + +func TestAppRole_SecretIDNumUsesUpgrade(t *testing.T) { + var resp *logical.Response + var err error + + b, storage := createBackendWithStorage(t) + + roleData := map[string]interface{}{ + "secret_id_num_uses": 10, + } + + roleReq := &logical.Request{ + Operation: logical.CreateOperation, + Path: "role/role1", + Storage: storage, + Data: roleData, + } + + resp, err = b.HandleRequest(roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + + secretIDReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "role/role1/secret-id", + Storage: storage, + } + + resp, err = b.HandleRequest(secretIDReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + + secretID := resp.Data["secret_id"].(string) + if secretID == "" { + t.Fatalf("expected non empty secret ID") + } + + secretIDReq.Operation = logical.ReadOperation + secretIDReq.Path = "role/role1/secret-id/" + secretID + + resp, err = b.HandleRequest(secretIDReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + + // Check if the response contains the value set for secret_id_num_uses + // and not SecretIDNumUses + if resp.Data["secret_id_num_uses"] != 10 || + resp.Data["SecretIDNumUses"] != 0 { + t.Fatal("invalid secret_id_num_uses") + } +}