Fix race accessing b.crls within cert auth (#18945) (#18949)

* 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

* Add cl

* 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

Co-authored-by: Steven Clark <[email protected]>

builtin/credential/cert/backend.go

@@ -42,7 +42,7 @@ func Backend() *backend {
 			pathCerts(&b),
 			pathCRLs(&b),
 		},
-		AuthRenew:    b.pathLoginRenew,
+		AuthRenew:    b.loginPathWrapper(b.pathLoginRenew),
 		Invalidate:   b.invalidate,
 		BackendType:  logical.TypeCredential,
 		PeriodicFunc: b.updateCRLs,

builtin/credential/cert/path_crls.go

@@ -49,6 +49,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()

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"
@@ -17,6 +18,8 @@ import (
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/certutil"
 	"github.com/hashicorp/vault/sdk/logical"
+	"github.com/hashicorp/vault/vault"
+
 	"github.com/stretchr/testify/require"
 )
 
@@ -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)
+	vault.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
+	})
 }

builtin/credential/cert/path_login.go

@@ -37,15 +37,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 {
@@ -82,13 +94,6 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra
 		return nil, err
 	}
 
-	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
@@ -162,12 +167,6 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f
 		return nil, err
 	}
 
-	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 {

changelog/18945.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+auth/cert: Address a race condition accessing the loaded crls without a lock
+```