From 45acac0e647d27546f3cab3e3838175980d723aa Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Wed, 19 Apr 2023 08:54:45 -0400 Subject: [PATCH] Return OCSP errors on cert auth login failures (#20234) * Return OCSP errors on cert auth login failures Signed-off-by: Alexander Scheel * Switch to immediately returning the first match Signed-off-by: Alexander Scheel * Add changelog entry Signed-off-by: Alexander Scheel --------- Signed-off-by: Alexander Scheel --- builtin/credential/cert/path_login.go | 61 ++++++++++++++++++++------- changelog/20234.txt | 3 ++ 2 files changed, 48 insertions(+), 16 deletions(-) create mode 100644 changelog/20234.txt diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go index 705257e0c..1aa7a1084 100644 --- a/builtin/credential/cert/path_login.go +++ b/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,6 +272,7 @@ 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] @@ -278,9 +280,19 @@ func (b *backend) verifyCredentials(ctx context.Context, req *logical.Request, d 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,23 +303,36 @@ 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 } } } @@ -315,13 +340,11 @@ func (b *backend) verifyCredentials(ctx context.Context, req *logical.Request, d } } - // 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 diff --git a/changelog/20234.txt b/changelog/20234.txt new file mode 100644 index 000000000..1f20bdc5a --- /dev/null +++ b/changelog/20234.txt @@ -0,0 +1,3 @@ +```release-note:improvement +auth/cert: Better return OCSP validation errors during login to the caller. +```