luxolus
/
open-vault
2
0
Fork 0
Code Issues 1 Pull Requests Releases Activity

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>
This commit is contained in:
Alexander Scheel 2023-04-19 08:54:45 -04:00 committed by GitHub
parent 5424eb2e8f
commit 45acac0e64
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 48 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
builtin/credential/cert/path_login.go
View File

@ -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

3
changelog/20234.txt Normal file
View File

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