Return OCSP errors on cert auth login failures (#20234)

* Return OCSP errors on cert auth login failures

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Switch to immediately returning the first match

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

---------

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

builtin/credential/cert/path_login.go

@@ -15,14 +15,15 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/hashicorp/vault/sdk/helper/ocsp"
-
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/certutil"
+	"github.com/hashicorp/vault/sdk/helper/cidrutil"
+	"github.com/hashicorp/vault/sdk/helper/ocsp"
 	"github.com/hashicorp/vault/sdk/helper/policyutil"
 	"github.com/hashicorp/vault/sdk/logical"
 
-	"github.com/hashicorp/vault/sdk/helper/cidrutil"
+	"github.com/hashicorp/errwrap"
+	"github.com/hashicorp/go-multierror"
 	glob "github.com/ryanuber/go-glob"
 )
 
@@ -271,16 +272,27 @@ func (b *backend) verifyCredentials(ctx context.Context, req *logical.Request, d
 
 	// If trustedNonCAs is not empty it means that client had registered a non-CA cert
 	// with the backend.
+	var retErr error
 	if len(trustedNonCAs) != 0 {
 		for _, trustedNonCA := range trustedNonCAs {
 			tCert := trustedNonCA.Certificates[0]
 			// Check for client cert being explicitly listed in the config (and matching other constraints)
 			if tCert.SerialNumber.Cmp(clientCert.SerialNumber) == 0 &&
 				bytes.Equal(tCert.AuthorityKeyId, clientCert.AuthorityKeyId) {
 				matches, err := b.matchesConstraints(ctx, clientCert, trustedNonCA.Certificates, trustedNonCA, verifyConf)
-				if err != nil {
-					return nil, nil, err
+
+				// matchesConstraints returns an error when OCSP verification fails,
+				// but some other path might still give us success. Add to the
+				// retErr multierror, but avoid duplicates. This way, if we reach a
+				// failure later, we can give additional context.
+				//
+				// XXX: If matchesConstraints is updated to generate additional,
+				// immediately fatal errors, we likely need to extend it to return
+				// another boolean (fatality) or other detection scheme.
+				if err != nil && (retErr == nil || !errwrap.Contains(retErr, err.Error())) {
+					retErr = multierror.Append(retErr, err)
 				}
+
 				if matches {
 					return trustedNonCA, nil, nil
 				}
@@ -291,37 +303,48 @@ func (b *backend) verifyCredentials(ctx context.Context, req *logical.Request, d
 	// If no trusted chain was found, client is not authenticated
 	// This check happens after checking for a matching configured non-CA certs
 	if len(trustedChains) == 0 {
+		if retErr == nil {
+			return nil, logical.ErrorResponse(fmt.Sprintf("invalid certificate or no client certificate supplied; additionally got errors during verification: %v", retErr)), nil
+		}
 		return nil, logical.ErrorResponse("invalid certificate or no client certificate supplied"), nil
 	}
 
 	// Search for a ParsedCert that intersects with the validated chains and any additional constraints
-	matches := make([]*ParsedCert, 0)
 	for _, trust := range trusted { // For each ParsedCert in the config
 		for _, tCert := range trust.Certificates { // For each certificate in the entry
 			for _, chain := range trustedChains { // For each root chain that we matched
 				for _, cCert := range chain { // For each cert in the matched chain
 					if tCert.Equal(cCert) { // ParsedCert intersects with matched chain
 						match, err := b.matchesConstraints(ctx, clientCert, chain, trust, verifyConf) // validate client cert + matched chain against the config
-						if err != nil {
-							return nil, nil, err
+
+						// See note above.
+						if err != nil && (retErr == nil || !errwrap.Contains(retErr, err.Error())) {
+							retErr = multierror.Append(retErr, err)
 						}
-						if match {
-							// Add the match to the list
-							matches = append(matches, trust)
+
+						// Return the first matching entry (for backwards
+						// compatibility, we continue to just pick the first
+						// one if we have multiple matches).
+						//
+						// Here, we return directly: this means that any
+						// future OCSP errors would be ignored; in the future,
+						// if these become fatal, we could revisit this
+						// choice and choose the first match after evaluating
+						// all possible candidates.
+						if match && err == nil {
+							return trust, nil, nil
 						}
 					}
 				}
 			}
 		}
 	}
 
-	// Fail on no matches
-	if len(matches) == 0 {
-		return nil, logical.ErrorResponse("no chain matching all constraints could be found for this login certificate"), nil
+	if retErr != nil {
+		return nil, logical.ErrorResponse(fmt.Sprintf("no chain matching all constraints could be found for this login certificate; additionally got errors during verification: %v", retErr)), nil
 	}
 
-	// Return the first matching entry (for backwards compatibility, we continue to just pick one if multiple match)
-	return matches[0], nil, nil
+	return nil, logical.ErrorResponse("no chain matching all constraints could be found for this login certificate"), nil
 }
 
 func (b *backend) matchesConstraints(ctx context.Context, clientCert *x509.Certificate, trustedChain []*x509.Certificate,
@@ -608,6 +631,12 @@ func (b *backend) checkForCertInOCSP(ctx context.Context, clientCert *x509.Certi
 	defer b.ocspClientMutex.RUnlock()
 	err := b.ocspClient.VerifyLeafCertificate(ctx, clientCert, chain[1], conf)
 	if err != nil {
+		// We want to preserve error messages when they have additional,
+		// potentially useful information. Just having a revoked cert
+		// isn't additionally useful.
+		if !strings.Contains(err.Error(), "has been revoked") {
+			return false, err
+		}
 		return false, nil
 	}
 	return true, nil

changelog/20234.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+auth/cert: Better return OCSP validation errors during login to the caller.
+```