From 5e034a1b011c6ebb19cad35c576764440d1f3d27 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Wed, 1 Feb 2023 13:50:56 -0500 Subject: [PATCH 1/3] Fix race accessing b.crls within cert auth - Discovered by CircleCI the pathLogin, pathLoginRenew paths access and reloads the b.crls member variable without a lock. - Also discovered that pathLoginResolveRole never populated an empty b.crls before usage within b.verifyCredentials --- builtin/credential/cert/path_crls.go | 10 ++++++++++ builtin/credential/cert/path_login.go | 19 ++++++++++--------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/builtin/credential/cert/path_crls.go b/builtin/credential/cert/path_crls.go index 787c5572dccd..75edcc32c46d 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_login.go b/builtin/credential/cert/path_login.go index 8547e9209cdc..7edc8979183b 100644 --- a/builtin/credential/cert/path_login.go +++ b/builtin/credential/cert/path_login.go @@ -48,6 +48,11 @@ func pathLogin(b *backend) *framework.Path { func (b *backend) pathLoginResolveRole(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { var matched *ParsedCert + // Make sure b.crls is populated if it was invalidated by replication + if err := b.populateCrlsIfNil(ctx, req.Storage); err != nil { + return nil, err + } + if verifyResp, resp, err := b.verifyCredentials(ctx, req, data); err != nil { return nil, err } else if resp != nil { @@ -90,11 +95,9 @@ 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 - } + // Probably invalidated due to replication, but we need these to proceed + if err := b.populateCrlsIfNil(ctx, req.Storage); err != nil { + return nil, err } var matched *ParsedCert @@ -173,10 +176,8 @@ 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 err := b.populateCrlsIfNil(ctx, req.Storage); err != nil { + return nil, err } if !config.DisableBinding { From 5327c5a0f85f5f9add96360f3c4c0130430e5f2c Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Wed, 1 Feb 2023 13:59:43 -0500 Subject: [PATCH 2/3] Add cl --- changelog/18945.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/18945.txt diff --git a/changelog/18945.txt b/changelog/18945.txt new file mode 100644 index 000000000000..a6f6a66305ac --- /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 +``` From b1b4fba0f66fbfc302412b3950de09cd7f69e803 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Wed, 1 Feb 2023 15:30:21 -0500 Subject: [PATCH 3/3] Misc cleanup - Introduce a login path wrapper instead of repeating in all the various login methods the crl reloading - Cleanup updatedConfig, never returned an error and nothing looked at the error returned - Make the test within TestCRLFetch a little less timing sensitive as I was able to trigger a failure due to my machine taking more than 150ms to load the new CRL --- builtin/credential/cert/backend.go | 6 ++--- builtin/credential/cert/path_crls_test.go | 20 ++++++++++------ builtin/credential/cert/path_login.go | 28 +++++++++++------------ 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/builtin/credential/cert/backend.go b/builtin/credential/cert/backend.go index 72089037a716..567ef8163a8d 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_test.go b/builtin/credential/cert/path_crls_test.go index aa293a52cd6d..9ca1f1243c19 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 7edc8979183b..59c48e76f28d 100644 --- a/builtin/credential/cert/path_login.go +++ b/builtin/credential/cert/path_login.go @@ -39,19 +39,26 @@ 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 - // Make sure b.crls is populated if it was invalidated by replication - if err := b.populateCrlsIfNil(ctx, req.Storage); err != nil { - return nil, err - } if verifyResp, resp, err := b.verifyCredentials(ctx, req, data); err != nil { return nil, err @@ -95,11 +102,6 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra b.updatedConfig(config) } - // Probably invalidated due to replication, but we need these to proceed - if err := b.populateCrlsIfNil(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 @@ -176,10 +178,6 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f b.updatedConfig(config) } - if err := b.populateCrlsIfNil(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 {