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

@@ -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 {
+	// RoleID must be supplied during every login
-	case intErr != nil:
+	roleID := strings.TrimSpace(data.Get("role_id").(string))
-		return nil, errwrap.Wrapf("failed to validate credentials: {{err}}", intErr)
+	if roleID == "" {
-	case userErr != nil:
+		return logical.ErrorResponse("missing role_id"), 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
+	// 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

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 {