Cache trusted cert values, invalidating when anything changes (#25421) (

#25465) * Cache trusted cert values, invalidating when anything changes * rename to something more indicative * defer * changelog * Use an LRU cache rather than a static map so we can't use too much memory. Add docs, unit tests * Don't add to cache if disabled. But this races if just a bool, so make the disabled an atomic
hashicorp · Feb 20, 2024 · 3a7fc7b · 3a7fc7b
1 parent ee628a3
commit 3a7fc7b
Show file tree

Hide file tree

Showing 10 changed files with 120 additions and 16 deletions.
diff --git a/builtin/credential/cert/backend.go b/builtin/credential/cert/backend.go
@@ -16,12 +16,19 @@ import (
 
 	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-multierror"
+	lru "github.com/hashicorp/golang-lru/v2"
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/ocsp"
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
-const operationPrefixCert = "cert"
+const (
+	operationPrefixCert = "cert"
+	trustedCertPath     = "cert/"
+
+	defaultRoleCacheSize = 200
+	maxRoleCacheSize     = 10000
+)
 
 func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, error) {
 	b := Backend()
@@ -32,7 +39,11 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend,
 }
 
 func Backend() *backend {
-	var b backend
+	// ignoring the error as it only can occur with <= 0 size
+	cache, _ := lru.New[string, *trusted](defaultRoleCacheSize)
+	b := backend{
+		trustedCache: cache,
+	}
 	b.Backend = &framework.Backend{
 		Help: backendHelp,
 		PathsSpecial: &logical.Paths{
@@ -59,6 +70,13 @@ func Backend() *backend {
 	return &b
 }
 
+type trusted struct {
+	pool          *x509.CertPool
+	trusted       []*ParsedCert
+	trustedNonCAs []*ParsedCert
+	ocspConf      *ocsp.VerifyConfig
+}
+
 type backend struct {
 	*framework.Backend
 	MapCertId *framework.PathMap
@@ -68,6 +86,9 @@ type backend struct {
 	ocspClientMutex sync.RWMutex
 	ocspClient      *ocsp.Client
 	configUpdated   atomic.Bool
+
+	trustedCache         *lru.Cache[string, *trusted]
+	trustedCacheDisabled atomic.Bool
 }
 
 func (b *backend) initialize(ctx context.Context, req *logical.InitializationRequest) error {
@@ -98,6 +119,7 @@ func (b *backend) invalidate(_ context.Context, key string) {
 	case key == "config":
 		b.configUpdated.Store(true)
 	}
+	b.flushTrustedCache()
 }
 
 func (b *backend) initOCSPClient(cacheSize int) {
@@ -109,9 +131,21 @@ func (b *backend) initOCSPClient(cacheSize int) {
 func (b *backend) updatedConfig(config *config) {
 	b.ocspClientMutex.Lock()
 	defer b.ocspClientMutex.Unlock()
+
+	switch {
+	case config.RoleCacheSize < 0:
+		// Just to clean up memory
+		b.trustedCacheDisabled.Store(true)
+		b.trustedCache.Purge()
+	case config.RoleCacheSize == 0:
+		config.RoleCacheSize = defaultRoleCacheSize
+		fallthrough
+	default:
+		b.trustedCache.Resize(config.RoleCacheSize)
+		b.trustedCacheDisabled.Store(false)
+	}
 	b.initOCSPClient(config.OcspCacheSize)
 	b.configUpdated.Store(false)
-	return
 }
 
 func (b *backend) fetchCRL(ctx context.Context, storage logical.Storage, name string, crl *CRLInfo) error {
@@ -161,6 +195,12 @@ func (b *backend) storeConfig(ctx context.Context, storage logical.Storage, conf
 	return nil
 }
 
+func (b *backend) flushTrustedCache() {
+	if b.trustedCache != nil { // defensive
+		b.trustedCache.Purge()
+	}
+}
+
 const backendHelp = `
 The "cert" credential provider allows authentication using
 TLS client certificates. A client connects to Vault and uses

diff --git a/builtin/credential/cert/path_certs.go b/builtin/credential/cert/path_certs.go
@@ -235,7 +235,7 @@ certificate.`,
 }
 
 func (b *backend) Cert(ctx context.Context, s logical.Storage, n string) (*CertEntry, error) {
-	entry, err := s.Get(ctx, "cert/"+strings.ToLower(n))
+	entry, err := s.Get(ctx, trustedCertPath+strings.ToLower(n))
 	if err != nil {
 		return nil, err
 	}
@@ -268,15 +268,16 @@ func (b *backend) Cert(ctx context.Context, s logical.Storage, n string) (*CertE
 }
 
 func (b *backend) pathCertDelete(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
-	err := req.Storage.Delete(ctx, "cert/"+strings.ToLower(d.Get("name").(string)))
+	defer b.flushTrustedCache()
+	err := req.Storage.Delete(ctx, trustedCertPath+strings.ToLower(d.Get("name").(string)))
 	if err != nil {
 		return nil, err
 	}
 	return nil, nil
 }
 
 func (b *backend) pathCertList(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
-	certs, err := req.Storage.List(ctx, "cert/")
+	certs, err := req.Storage.List(ctx, trustedCertPath)
 	if err != nil {
 		return nil, err
 	}
@@ -333,6 +334,7 @@ func (b *backend) pathCertRead(ctx context.Context, req *logical.Request, d *fra
 }
 
 func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
+	defer b.flushTrustedCache()
 	name := strings.ToLower(d.Get("name").(string))
 
 	cert, err := b.Cert(ctx, req.Storage, name)
@@ -475,7 +477,7 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
 	}
 
 	// Store it
-	entry, err := logical.StorageEntryJSON("cert/"+name, cert)
+	entry, err := logical.StorageEntryJSON(trustedCertPath+name, cert)
 	if err != nil {
 		return nil, err
 	}

diff --git a/builtin/credential/cert/path_config.go b/builtin/credential/cert/path_config.go
@@ -11,7 +11,7 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
-const maxCacheSize = 100000
+const maxOcspCacheSize = 100000
 
 func pathConfig(b *backend) *framework.Path {
 	return &framework.Path{
@@ -37,6 +37,11 @@ func pathConfig(b *backend) *framework.Path {
 				Default:     100,
 				Description: `The size of the in memory OCSP response cache, shared by all configured certs`,
 			},
+			"role_cache_size": {
+				Type:        framework.TypeInt,
+				Default:     defaultRoleCacheSize,
+				Description: `The size of the in memory role cache`,
+			},
 		},
 
 		Operations: map[logical.Operation]framework.OperationHandler{
@@ -70,11 +75,18 @@ func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, dat
 	}
 	if cacheSizeRaw, ok := data.GetOk("ocsp_cache_size"); ok {
 		cacheSize := cacheSizeRaw.(int)
-		if cacheSize < 2 || cacheSize > maxCacheSize {
-			return logical.ErrorResponse("invalid cache size, must be >= 2 and <= %d", maxCacheSize), nil
+		if cacheSize < 2 || cacheSize > maxOcspCacheSize {
+			return logical.ErrorResponse("invalid ocsp cache size, must be >= 2 and <= %d", maxOcspCacheSize), nil
 		}
 		config.OcspCacheSize = cacheSize
 	}
+	if cacheSizeRaw, ok := data.GetOk("role_cache_size"); ok {
+		cacheSize := cacheSizeRaw.(int)
+		if (cacheSize < 0 && cacheSize != -1) || cacheSize > maxRoleCacheSize {
+			return logical.ErrorResponse("invalid role cache size, must be <= %d or -1 to disable role caching", maxRoleCacheSize), nil
+		}
+		config.RoleCacheSize = cacheSize
+	}
 	if err := b.storeConfig(ctx, req.Storage, config); err != nil {
 		return nil, err
 	}
@@ -91,6 +103,7 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, d *f
 		"disable_binding":                cfg.DisableBinding,
 		"enable_identity_alias_metadata": cfg.EnableIdentityAliasMetadata,
 		"ocsp_cache_size":                cfg.OcspCacheSize,
+		"role_cache_size":                cfg.RoleCacheSize,
 	}
 
 	return &logical.Response{
@@ -119,4 +132,5 @@ type config struct {
 	DisableBinding              bool `json:"disable_binding"`
 	EnableIdentityAliasMetadata bool `json:"enable_identity_alias_metadata"`
 	OcspCacheSize               int  `json:"ocsp_cache_size"`
+	RoleCacheSize               int  `json:"role_cache_size"`
 }
diff --git a/builtin/credential/cert/path_crls.go b/builtin/credential/cert/path_crls.go
@@ -190,6 +190,7 @@ func (b *backend) pathCRLDelete(ctx context.Context, req *logical.Request, d *fr
 
 	b.crlUpdateMutex.Lock()
 	defer b.crlUpdateMutex.Unlock()
+	defer b.flushTrustedCache()
 
 	_, ok := b.crls[name]
 	if !ok {
@@ -313,6 +314,8 @@ func (b *backend) setCRL(ctx context.Context, storage logical.Storage, certList
 	}
 
 	b.crls[name] = crlInfo
+	b.flushTrustedCache()
+
 	return err
 }
 

diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go
@@ -257,7 +257,7 @@ func (b *backend) verifyCredentials(ctx context.Context, req *logical.Request, d
 	}
 
 	// Load the trusted certificates and other details
-	roots, trusted, trustedNonCAs, verifyConf := b.loadTrustedCerts(ctx, req.Storage, certName)
+	roots, trusted, trustedNonCAs, verifyConf := b.getTrustedCerts(ctx, req.Storage, certName)
 
 	// Get the list of full chains matching the connection and validates the
 	// certificate itself
@@ -581,18 +581,29 @@ func (b *backend) certificateExtensionsMetadata(clientCert *x509.Certificate, co
 	return metadata
 }
 
+// getTrustedCerts is used to load all the trusted certificates from the backend, cached
+
+func (b *backend) getTrustedCerts(ctx context.Context, storage logical.Storage, certName string) (pool *x509.CertPool, trusted []*ParsedCert, trustedNonCAs []*ParsedCert, conf *ocsp.VerifyConfig) {
+	if !b.trustedCacheDisabled.Load() {
+		if trusted, found := b.trustedCache.Get(certName); found {
+			return trusted.pool, trusted.trusted, trusted.trustedNonCAs, trusted.ocspConf
+		}
+	}
+	return b.loadTrustedCerts(ctx, storage, certName)
+}
+
 // loadTrustedCerts is used to load all the trusted certificates from the backend
-func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage, certName string) (pool *x509.CertPool, trusted []*ParsedCert, trustedNonCAs []*ParsedCert, conf *ocsp.VerifyConfig) {
+func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage, certName string) (pool *x509.CertPool, trustedCerts []*ParsedCert, trustedNonCAs []*ParsedCert, conf *ocsp.VerifyConfig) {
 	pool = x509.NewCertPool()
-	trusted = make([]*ParsedCert, 0)
+	trustedCerts = make([]*ParsedCert, 0)
 	trustedNonCAs = make([]*ParsedCert, 0)
 
 	var names []string
 	if certName != "" {
 		names = append(names, certName)
 	} else {
 		var err error
-		names, err = storage.List(ctx, "cert/")
+		names, err = storage.List(ctx, trustedCertPath)
 		if err != nil {
 			b.Logger().Error("failed to list trusted certs", "error", err)
 			return
@@ -601,7 +612,7 @@ func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage,
 
 	conf = &ocsp.VerifyConfig{}
 	for _, name := range names {
-		entry, err := b.Cert(ctx, storage, strings.TrimPrefix(name, "cert/"))
+		entry, err := b.Cert(ctx, storage, strings.TrimPrefix(name, trustedCertPath))
 		if err != nil {
 			b.Logger().Error("failed to load trusted cert", "name", name, "error", err)
 			continue
@@ -630,7 +641,7 @@ func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage,
 			}
 
 			// Create a ParsedCert entry
-			trusted = append(trusted, &ParsedCert{
+			trustedCerts = append(trustedCerts, &ParsedCert{
 				Entry:        entry,
 				Certificates: parsed,
 			})
@@ -646,6 +657,15 @@ func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage,
 			conf.QueryAllServers = conf.QueryAllServers || entry.OcspQueryAllServers
 		}
 	}
+
+	if !b.trustedCacheDisabled.Load() {
+		b.trustedCache.Add(certName, &trusted{
+			pool:          pool,
+			trusted:       trustedCerts,
+			trustedNonCAs: trustedNonCAs,
+			ocspConf:      conf,
+		})
+	}
 	return
 }
 

diff --git a/builtin/credential/cert/path_login_test.go b/builtin/credential/cert/path_login_test.go
@@ -94,6 +94,10 @@ func TestCert_RoleResolve(t *testing.T) {
 			testAccStepCert(t, "web", ca, "foo", allowed{dns: "example.com"}, false),
 			testAccStepLoginWithName(t, connState, "web"),
 			testAccStepResolveRoleWithName(t, connState, "web"),
+			// Test with caching disabled
+			testAccStepSetRoleCacheSize(t, -1),
+			testAccStepLoginWithName(t, connState, "web"),
+			testAccStepResolveRoleWithName(t, connState, "web"),
 		},
 	})
 }
@@ -151,10 +155,23 @@ func TestCert_RoleResolveWithoutProvidingCertName(t *testing.T) {
 			testAccStepCert(t, "web", ca, "foo", allowed{dns: "example.com"}, false),
 			testAccStepLoginWithName(t, connState, "web"),
 			testAccStepResolveRoleWithEmptyDataMap(t, connState, "web"),
+			testAccStepSetRoleCacheSize(t, -1),
+			testAccStepLoginWithName(t, connState, "web"),
+			testAccStepResolveRoleWithEmptyDataMap(t, connState, "web"),
 		},
 	})
 }
 
+func testAccStepSetRoleCacheSize(t *testing.T, size int) logicaltest.TestStep {
+	return logicaltest.TestStep{
+		Operation: logical.UpdateOperation,
+		Path:      "config",
+		Data: map[string]interface{}{
+			"role_cache_size": size,
+		},
+	}
+}
+
 func testAccStepResolveRoleWithEmptyDataMap(t *testing.T, connState tls.ConnectionState, certName string) logicaltest.TestStep {
 	return logicaltest.TestStep{
 		Operation:       logical.ResolveRoleOperation,

diff --git a/changelog/25421.txt b/changelog/25421.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+auth/cert: Cache trusted certs to reduce memory usage and improve performance of logins.
+```
diff --git a/go.mod b/go.mod
@@ -115,6 +115,7 @@ require (
 	github.com/hashicorp/go-uuid v1.0.3
 	github.com/hashicorp/go-version v1.6.0
 	github.com/hashicorp/golang-lru v0.5.4
+	github.com/hashicorp/golang-lru/v2 v2.0.7
 	github.com/hashicorp/hcl v1.0.1-vault-5
 	github.com/hashicorp/hcl/v2 v2.16.2
 	github.com/hashicorp/hcp-link v0.2.1

diff --git a/go.sum b/go.sum
@@ -2101,6 +2101,8 @@ github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ
 github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
 github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc=
 github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4=
+github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k=
+github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM=
 github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
 github.com/hashicorp/hcl v1.0.1-vault-5 h1:kI3hhbbyzr4dldA8UdTb7ZlVVlI2DACdCfz31RPDgJM=
 github.com/hashicorp/hcl v1.0.1-vault-5/go.mod h1:XYhtn6ijBSAj6n4YqAaf7RBPS4I06AItNorpy+MoQNM=

diff --git a/website/content/api-docs/auth/cert.mdx b/website/content/api-docs/auth/cert.mdx
@@ -360,6 +360,8 @@ Configuration options for the method.
   `allowed_metadata_extensions` will be stored in the alias
 - `ocsp_cache_size` `(int: 100)` - The size of the OCSP response LRU cache.  Note
   that this cache is used for all configured certificates.
+- `role_cache_size` `(int: 200)` - The size of the role cache.  Use `-1` to disable 
+  role caching.
 
 ### Sample payload