diff --git a/builtin/credential/cert/backend.go b/builtin/credential/cert/backend.go index 72089037a..567ef8163 100644 --- a/builtin/credential/cert/backend.go +++ b/builtin/credential/cert/backend.go @@ -43,7 +43,7 @@ func Backend() *backend { pathListCRLs(&b), pathCRLs(&b), }, - AuthRenew: b.pathLoginRenew, + AuthRenew: b.loginPathWrapper(b.pathLoginRenew), Invalidate: b.invalidate, BackendType: logical.TypeCredential, InitializeFunc: b.initialize, @@ -101,12 +101,12 @@ func (b *backend) initOCSPClient(cacheSize int) { }, cacheSize) } -func (b *backend) updatedConfig(config *config) error { +func (b *backend) updatedConfig(config *config) { b.ocspClientMutex.Lock() defer b.ocspClientMutex.Unlock() b.initOCSPClient(config.OcspCacheSize) b.configUpdated.Store(false) - return nil + return } func (b *backend) fetchCRL(ctx context.Context, storage logical.Storage, name string, crl *CRLInfo) error { diff --git a/builtin/credential/cert/path_crls.go b/builtin/credential/cert/path_crls.go index 787c5572d..75edcc32c 100644 --- a/builtin/credential/cert/path_crls.go +++ b/builtin/credential/cert/path_crls.go @@ -71,6 +71,16 @@ using the same name as specified here.`, } } +func (b *backend) populateCrlsIfNil(ctx context.Context, storage logical.Storage) error { + b.crlUpdateMutex.RLock() + if b.crls == nil { + b.crlUpdateMutex.RUnlock() + return b.lockThenpopulateCRLs(ctx, storage) + } + b.crlUpdateMutex.RUnlock() + return nil +} + func (b *backend) lockThenpopulateCRLs(ctx context.Context, storage logical.Storage) error { b.crlUpdateMutex.Lock() defer b.crlUpdateMutex.Unlock() diff --git a/builtin/credential/cert/path_crls_test.go b/builtin/credential/cert/path_crls_test.go index aa293a52c..9ca1f1243 100644 --- a/builtin/credential/cert/path_crls_test.go +++ b/builtin/credential/cert/path_crls_test.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "crypto/x509" "crypto/x509/pkix" + "fmt" "io/ioutil" "math/big" "net/http" @@ -14,6 +15,8 @@ import ( "testing" "time" + "github.com/hashicorp/vault/helper/testhelpers/corehelpers" + "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/helper/certutil" "github.com/hashicorp/vault/sdk/logical" @@ -162,7 +165,7 @@ func TestCRLFetch(t *testing.T) { b.crlUpdateMutex.Lock() if len(b.crls["testcrl"].Serials) != 1 { - t.Fatalf("wrong number of certs in CRL") + t.Fatalf("wrong number of certs in CRL got %d, expected 1", len(b.crls["testcrl"].Serials)) } b.crlUpdateMutex.Unlock() @@ -188,11 +191,14 @@ func TestCRLFetch(t *testing.T) { // Give ourselves a little extra room on slower CI systems to ensure we // can fetch the new CRL. - time.Sleep(150 * time.Millisecond) + corehelpers.RetryUntil(t, 2*time.Second, func() error { + b.crlUpdateMutex.Lock() + defer b.crlUpdateMutex.Unlock() - b.crlUpdateMutex.Lock() - if len(b.crls["testcrl"].Serials) != 2 { - t.Fatalf("wrong number of certs in CRL") - } - b.crlUpdateMutex.Unlock() + serialCount := len(b.crls["testcrl"].Serials) + if serialCount != 2 { + return fmt.Errorf("CRL refresh did not occur serial count %d", serialCount) + } + return nil + }) } diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go index 8547e9209..59c48e76f 100644 --- a/builtin/credential/cert/path_login.go +++ b/builtin/credential/cert/path_login.go @@ -39,15 +39,27 @@ func pathLogin(b *backend) *framework.Path { }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: b.pathLogin, + logical.UpdateOperation: b.loginPathWrapper(b.pathLogin), logical.AliasLookaheadOperation: b.pathLoginAliasLookahead, - logical.ResolveRoleOperation: b.pathLoginResolveRole, + logical.ResolveRoleOperation: b.loginPathWrapper(b.pathLoginResolveRole), }, } } +func (b *backend) loginPathWrapper(wrappedOp func(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error)) framework.OperationFunc { + return func(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + // Make sure that the CRLs have been loaded before processing a login request, + // they might have been nil'd by an invalidate func call. + if err := b.populateCrlsIfNil(ctx, req.Storage); err != nil { + return nil, err + } + return wrappedOp(ctx, req, data) + } +} + func (b *backend) pathLoginResolveRole(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { var matched *ParsedCert + if verifyResp, resp, err := b.verifyCredentials(ctx, req, data); err != nil { return nil, err } else if resp != nil { @@ -90,13 +102,6 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra b.updatedConfig(config) } - if b.crls == nil { - // Probably invalidated due to replication, but we need these to proceed - if err := b.populateCRLs(ctx, req.Storage); err != nil { - return nil, err - } - } - var matched *ParsedCert if verifyResp, resp, err := b.verifyCredentials(ctx, req, data); err != nil { return nil, err @@ -173,12 +178,6 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f b.updatedConfig(config) } - if b.crls == nil { - if err := b.populateCRLs(ctx, req.Storage); err != nil { - return nil, err - } - } - if !config.DisableBinding { var matched *ParsedCert if verifyResp, resp, err := b.verifyCredentials(ctx, req, d); err != nil { diff --git a/changelog/18945.txt b/changelog/18945.txt new file mode 100644 index 000000000..a6f6a6630 --- /dev/null +++ b/changelog/18945.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/cert: Address a race condition accessing the loaded crls without a lock +```