From df8484f7af3ce48eef3ecb8690bc389962e9e497 Mon Sep 17 00:00:00 2001 From: Vishal Nayak Date: Fri, 4 May 2018 10:15:16 -0400 Subject: [PATCH] approle: Make invalid role_id a 400 error instead of 500 (#4470) * make invalid role_id a 400 error * remove single-use validateCredentials function * remove single-use validateBindSecretID function * adjust the error message for CIDR check failure * locking updates as review feedback --- builtin/credential/approle/path_login.go | 185 +++++++++++++++++++- builtin/credential/approle/validation.go | 207 ----------------------- 2 files changed, 177 insertions(+), 215 deletions(-) diff --git a/builtin/credential/approle/path_login.go b/builtin/credential/approle/path_login.go index 722f5ea7a..71e5afb05 100644 --- a/builtin/credential/approle/path_login.go +++ b/builtin/credential/approle/path_login.go @@ -4,8 +4,10 @@ import ( "context" "fmt" "strings" + "time" "github.com/hashicorp/errwrap" + "github.com/hashicorp/vault/helper/cidrutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" ) @@ -51,14 +53,181 @@ func (b *backend) pathLoginUpdateAliasLookahead(ctx context.Context, req *logica // Returns the Auth object indicating the authentication and authorization information // if the credentials provided are validated by the backend. func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - role, roleName, metadata, _, userErr, intErr := b.validateCredentials(ctx, req, data) - switch { - case intErr != nil: - return nil, errwrap.Wrapf("failed to validate credentials: {{err}}", intErr) - case userErr != nil: - return logical.ErrorResponse(fmt.Sprintf("failed to validate credentials: %v", userErr)), nil - case role == nil: - return logical.ErrorResponse("failed to validate credentials; could not find role"), nil + + // RoleID must be supplied during every login + roleID := strings.TrimSpace(data.Get("role_id").(string)) + if roleID == "" { + return logical.ErrorResponse("missing role_id"), nil + } + + // Look for the storage entry that maps the roleID to role + roleIDIndex, err := b.roleIDEntry(ctx, req.Storage, roleID) + if err != nil { + return nil, err + } + if roleIDIndex == nil { + return logical.ErrorResponse(fmt.Sprintf("invalid role_id %q", roleID)), nil + } + + roleName := roleIDIndex.Name + + roleLock := b.roleLock(roleName) + roleLock.RLock() + + role, err := b.roleEntry(ctx, req.Storage, roleName) + roleLock.RUnlock() + if err != nil { + return nil, err + } + if role == nil { + return logical.ErrorResponse(fmt.Sprintf("invalid role_id %q", roleID)), nil + } + + var metadata map[string]string + if role.BindSecretID { + secretID := strings.TrimSpace(data.Get("secret_id").(string)) + if secretID == "" { + return logical.ErrorResponse("missing secret_id"), nil + } + + if role.LowerCaseRoleName { + roleName = strings.ToLower(roleName) + } + + secretIDHMAC, err := createHMAC(role.HMACKey, secretID) + if err != nil { + return nil, errwrap.Wrapf("failed to create HMAC of secret_id: {{err}}", err) + } + + roleNameHMAC, err := createHMAC(role.HMACKey, roleName) + if err != nil { + return nil, errwrap.Wrapf("failed to create HMAC of role_name: {{err}}", err) + } + + entryIndex := fmt.Sprintf("%s%s/%s", role.SecretIDPrefix, roleNameHMAC, secretIDHMAC) + + secretIDLock := b.secretIDLock(secretIDHMAC) + secretIDLock.RLock() + + entry, err := b.nonLockedSecretIDStorageEntry(ctx, req.Storage, role.SecretIDPrefix, roleNameHMAC, secretIDHMAC) + if err != nil { + secretIDLock.RUnlock() + return nil, err + } else if entry == nil { + secretIDLock.RUnlock() + return logical.ErrorResponse(fmt.Sprintf("invalid secret_id %q", secretID)), nil + } + + switch { + case entry.SecretIDNumUses == 0: + defer secretIDLock.RUnlock() + // + // SecretIDNumUses will be zero only if the usage limit was not set at all, + // in which case, the SecretID will remain to be valid as long as it is not + // expired. + // + + // Ensure that the CIDRs on the secret ID are still a subset of that of + // role's + err = verifyCIDRRoleSecretIDSubset(entry.CIDRList, role.BoundCIDRList) + if err != nil { + return nil, err + } + + // If CIDR restrictions are present on the secret ID, check if the + // source IP complies to it + if len(entry.CIDRList) != 0 { + if req.Connection == nil || req.Connection.RemoteAddr == "" { + return nil, fmt.Errorf("failed to get connection information") + } + + belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(req.Connection.RemoteAddr, entry.CIDRList) + if !belongs || err != nil { + return logical.ErrorResponse(errwrap.Wrapf(fmt.Sprintf("source address %q unauthorized through CIDR restrictions on the secret ID: {{err}}", req.Connection.RemoteAddr), err).Error()), nil + } + } + default: + // + // If the SecretIDNumUses is non-zero, it means that its use-count should be updated + // in the storage. Switch the lock from a `read` to a `write` and update + // the storage entry. + // + + secretIDLock.RUnlock() + secretIDLock.Lock() + defer secretIDLock.Unlock() + + // Lock switching may change the data. Refresh the contents. + entry, err = b.nonLockedSecretIDStorageEntry(ctx, req.Storage, role.SecretIDPrefix, roleNameHMAC, secretIDHMAC) + if err != nil { + return nil, err + } + if entry == nil { + return logical.ErrorResponse(fmt.Sprintf("invalid secret_id %q", secretID)), nil + } + + // If there exists a single use left, delete the SecretID entry from + // the storage but do not fail the validation request. Subsequent + // requests to use the same SecretID will fail. + if entry.SecretIDNumUses == 1 { + // Delete the secret IDs accessor first + err = b.deleteSecretIDAccessorEntry(ctx, req.Storage, entry.SecretIDAccessor, role.SecretIDPrefix) + if err != nil { + return nil, err + } + err = req.Storage.Delete(ctx, entryIndex) + if err != nil { + return nil, errwrap.Wrapf("failed to delete secret ID: {{err}}", err) + } + } else { + // If the use count is greater than one, decrement it and update the last updated time. + entry.SecretIDNumUses -= 1 + entry.LastUpdatedTime = time.Now() + + sEntry, err := logical.StorageEntryJSON(entryIndex, &entry) + if err != nil { + return nil, err + } + + err = req.Storage.Put(ctx, sEntry) + if err != nil { + return nil, err + } + } + + // Ensure that the CIDRs on the secret ID are still a subset of that of + // role's + err = verifyCIDRRoleSecretIDSubset(entry.CIDRList, role.BoundCIDRList) + if err != nil { + return nil, err + } + + // If CIDR restrictions are present on the secret ID, check if the + // source IP complies to it + if len(entry.CIDRList) != 0 { + if req.Connection == nil || req.Connection.RemoteAddr == "" { + return nil, fmt.Errorf("failed to get connection information") + } + + belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(req.Connection.RemoteAddr, entry.CIDRList) + if err != nil || !belongs { + return logical.ErrorResponse(errwrap.Wrapf(fmt.Sprintf("source address %q unauthorized by CIDR restrictions on the secret ID: {{err}}", req.Connection.RemoteAddr), err).Error()), nil + } + } + } + + metadata = entry.Metadata + } + + if len(role.BoundCIDRList) != 0 { + if req.Connection == nil || req.Connection.RemoteAddr == "" { + return nil, fmt.Errorf("failed to get connection information") + } + + belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(req.Connection.RemoteAddr, role.BoundCIDRList) + if err != nil || !belongs { + return logical.ErrorResponse(errwrap.Wrapf(fmt.Sprintf("source address %q unauthorized by CIDR restrictions on the role: {{err}}", req.Connection.RemoteAddr), err).Error()), nil + } } // Always include the role name, for later filtering diff --git a/builtin/credential/approle/validation.go b/builtin/credential/approle/validation.go index d42d68fd0..bdccad57b 100644 --- a/builtin/credential/approle/validation.go +++ b/builtin/credential/approle/validation.go @@ -6,7 +6,6 @@ import ( "crypto/sha256" "encoding/hex" "fmt" - "strings" "time" "github.com/hashicorp/errwrap" @@ -14,7 +13,6 @@ import ( "github.com/hashicorp/vault/helper/cidrutil" "github.com/hashicorp/vault/helper/locksutil" "github.com/hashicorp/vault/logical" - "github.com/hashicorp/vault/logical/framework" ) // secretIDStorageEntry represents the information stored in storage @@ -68,211 +66,6 @@ type secretIDAccessorStorageEntry struct { SecretIDHMAC string `json:"secret_id_hmac" structs:"secret_id_hmac" mapstructure:"secret_id_hmac"` } -// Checks if the Role represented by the RoleID still exists -func (b *backend) validateRoleID(ctx context.Context, s logical.Storage, roleID string) (*roleStorageEntry, string, error) { - // Look for the storage entry that maps the roleID to role - roleIDIndex, err := b.roleIDEntry(ctx, s, roleID) - if err != nil { - return nil, "", err - } - if roleIDIndex == nil { - return nil, "", fmt.Errorf("invalid role_id %q", roleID) - } - - lock := b.roleLock(roleIDIndex.Name) - lock.RLock() - defer lock.RUnlock() - - role, err := b.roleEntry(ctx, s, roleIDIndex.Name) - if err != nil { - return nil, "", err - } - if role == nil { - return nil, "", fmt.Errorf("role %q referred by the role_id %q does not exist anymore", roleIDIndex.Name, roleID) - } - - return role, roleIDIndex.Name, nil -} - -// Validates the supplied RoleID and SecretID -func (b *backend) validateCredentials(ctx context.Context, req *logical.Request, data *framework.FieldData) (*roleStorageEntry, string, map[string]string, string, error, error) { - metadata := make(map[string]string) - // RoleID must be supplied during every login - roleID := strings.TrimSpace(data.Get("role_id").(string)) - if roleID == "" { - return nil, "", metadata, "", fmt.Errorf("missing role_id"), nil - } - - // Validate the RoleID and get the Role entry - role, roleName, err := b.validateRoleID(ctx, req.Storage, roleID) - if err != nil { - return nil, "", metadata, "", nil, err - } - if role == nil || roleName == "" { - return nil, "", metadata, "", fmt.Errorf("failed to validate role_id"), nil - } - - var secretID string - if role.BindSecretID { - // If 'bind_secret_id' was set on role, look for the field 'secret_id' - // to be specified and validate it. - secretID = strings.TrimSpace(data.Get("secret_id").(string)) - if secretID == "" { - return nil, "", metadata, "", fmt.Errorf("missing secret_id"), nil - } - - if role.LowerCaseRoleName { - roleName = strings.ToLower(roleName) - } - - // Check if the SecretID supplied is valid. If use limit was specified - // on the SecretID, it will be decremented in this call. - var valid bool - valid, metadata, err = b.validateBindSecretID(ctx, req, roleName, secretID, role) - if err != nil { - return nil, "", metadata, "", nil, err - } - if !valid { - return nil, "", metadata, "", fmt.Errorf("invalid secret_id %q", secretID), nil - } - } - - if len(role.BoundCIDRList) != 0 { - // If 'bound_cidr_list' was set, verify the CIDR restrictions - if req.Connection == nil || req.Connection.RemoteAddr == "" { - return nil, "", metadata, "", fmt.Errorf("failed to get connection information"), nil - } - - belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(req.Connection.RemoteAddr, role.BoundCIDRList) - if err != nil { - return nil, "", metadata, "", nil, errwrap.Wrapf("failed to verify the CIDR restrictions set on the role: {{err}}", err) - } - if !belongs { - return nil, "", metadata, "", fmt.Errorf("source address %q unauthorized through CIDR restrictions on the role", req.Connection.RemoteAddr), nil - } - } - - return role, roleName, metadata, secretID, nil, nil -} - -// validateBindSecretID is used to determine if the given SecretID is a valid one. -func (b *backend) validateBindSecretID(ctx context.Context, req *logical.Request, roleName, secretID string, - role *roleStorageEntry) (bool, map[string]string, error) { - secretIDHMAC, err := createHMAC(role.HMACKey, secretID) - if err != nil { - return false, nil, errwrap.Wrapf("failed to create HMAC of secret_id: {{err}}", err) - } - - roleNameHMAC, err := createHMAC(role.HMACKey, roleName) - if err != nil { - return false, nil, errwrap.Wrapf("failed to create HMAC of role_name: {{err}}", err) - } - - entryIndex := fmt.Sprintf("%s%s/%s", role.SecretIDPrefix, roleNameHMAC, secretIDHMAC) - - // SecretID locks are always index based on secretIDHMACs. This helps - // acquiring the locks when the SecretIDs are listed. This allows grabbing - // the correct locks even if the SecretIDs are not known in plaintext. - lock := b.secretIDLock(secretIDHMAC) - lock.RLock() - - result, err := b.nonLockedSecretIDStorageEntry(ctx, req.Storage, role.SecretIDPrefix, roleNameHMAC, secretIDHMAC) - if err != nil { - lock.RUnlock() - return false, nil, err - } else if result == nil { - lock.RUnlock() - return false, nil, nil - } - - // SecretIDNumUses will be zero only if the usage limit was not set at all, - // in which case, the SecretID will remain to be valid as long as it is not - // expired. - if result.SecretIDNumUses == 0 { - // Ensure that the CIDRs on the secret ID are still a subset of that of - // role's - if err := verifyCIDRRoleSecretIDSubset(result.CIDRList, - role.BoundCIDRList); err != nil { - return false, nil, err - } - - // If CIDR restrictions are present on the secret ID, check if the - // source IP complies to it - if len(result.CIDRList) != 0 { - if req.Connection == nil || req.Connection.RemoteAddr == "" { - return false, nil, fmt.Errorf("failed to get connection information") - } - - if belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(req.Connection.RemoteAddr, result.CIDRList); !belongs || err != nil { - return false, nil, errwrap.Wrapf(fmt.Sprintf("source address %q unauthorized through CIDR restrictions on the secret ID: {{err}}", req.Connection.RemoteAddr), err) - } - } - - lock.RUnlock() - return true, result.Metadata, nil - } - - // If the SecretIDNumUses is non-zero, it means that its use-count should be updated - // in the storage. Switch the lock from a `read` to a `write` and update - // the storage entry. - lock.RUnlock() - - lock.Lock() - defer lock.Unlock() - - // Lock switching may change the data. Refresh the contents. - result, err = b.nonLockedSecretIDStorageEntry(ctx, req.Storage, role.SecretIDPrefix, roleNameHMAC, secretIDHMAC) - if err != nil { - return false, nil, err - } - if result == nil { - return false, nil, nil - } - - // If there exists a single use left, delete the SecretID entry from - // the storage but do not fail the validation request. Subsequent - // requests to use the same SecretID will fail. - if result.SecretIDNumUses == 1 { - // Delete the secret IDs accessor first - if err := b.deleteSecretIDAccessorEntry(ctx, req.Storage, result.SecretIDAccessor, role.SecretIDPrefix); err != nil { - return false, nil, err - } - if err := req.Storage.Delete(ctx, entryIndex); err != nil { - return false, nil, errwrap.Wrapf("failed to delete secret ID: {{err}}", 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, errwrap.Wrapf("failed to create storage entry while decrementing the secret ID use count: {{err}}", err) - } else if err = req.Storage.Put(ctx, entry); err != nil { - return false, nil, errwrap.Wrapf("failed to decrement the secret ID use count: {{err}}", err) - } - } - - // Ensure that the CIDRs on the secret ID are still a subset of that of - // role's - if err := verifyCIDRRoleSecretIDSubset(result.CIDRList, - role.BoundCIDRList); err != nil { - return false, nil, err - } - - // If CIDR restrictions are present on the secret ID, check if the - // source IP complies to it - if len(result.CIDRList) != 0 { - if req.Connection == nil || req.Connection.RemoteAddr == "" { - return false, nil, fmt.Errorf("failed to get connection information") - } - - if belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(req.Connection.RemoteAddr, result.CIDRList); !belongs || err != nil { - return false, nil, errwrap.Wrapf(fmt.Sprintf("source address %q unauthorized through CIDR restrictions on the secret ID: {{err}}", req.Connection.RemoteAddr), err) - } - } - - return true, result.Metadata, nil -} - // verifyCIDRRoleSecretIDSubset checks if the CIDR blocks set on the secret ID // are a subset of CIDR blocks set on the role func verifyCIDRRoleSecretIDSubset(secretIDCIDRs []string, roleBoundCIDRList []string) error {