Add cached OCSP client support to Cert Auth (#17093)

* wip * Add cached OCSP client support to Cert Auth * ->pointer * Code cleanup * Fix unit tests * Use an LRU cache, and only persist up to 1000 of the most recently used values to stay under the storage entry limit * Fix caching, add fail open mode parameter to cert auth roles * reduce logging * Add the retry client and GET then POST logic * Drop persisted cache, make cache size configurable, allow for parallel testing of multiple servers * dead code * Update builtin/credential/cert/path_certs.go Co-authored-by: Alexander Scheel <[email protected]> * Hook invalidate to reinit the ocsp cache size * locking * Conditionally init the ocsp client * Remove cache size config from cert configs, it's a backend global * Add field * Remove strangely complex validity logic * Address more feedback * Rework error returning logic * More edge cases * MORE edge cases * Add a test matrix with a builtin responder * changelog * Use an atomic for configUpdated * Actually use ocsp_enabled, and bind to a random port for testing * Update builtin/credential/cert/path_login.go Co-authored-by: Alexander Scheel <[email protected]> * Refactor unit tests * Add status to cache * Make some functions private * Rename for testing, and attribute * Up to date gofumpt * remove hash from key, and disable the vault dependent unit test * Comment out TestMultiOCSP * imports * more imports * Address semgrep results * Attempt to pass some sort of logging to test_responder * fix overzealous search&replace Co-authored-by: Alexander Scheel <[email protected]>
hashicorp · Nov 21, 2022 · a611748 · a611748
1 parent 56a10bb
commit a611748
Show file tree

Hide file tree

Showing 13 changed files with 2,287 additions and 60 deletions.
diff --git a/builtin/credential/cert/backend.go b/builtin/credential/cert/backend.go
@@ -8,10 +8,13 @@ import (
 	"net/http"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
+	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/vault/sdk/framework"
+	"github.com/hashicorp/vault/sdk/helper/ocsp"
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
@@ -20,6 +23,13 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend,
 	if err := b.Setup(ctx, conf); err != nil {
 		return nil, err
 	}
+	bConf, err := b.Config(ctx, conf.StorageView)
+	if err != nil {
+		return nil, err
+	}
+	if bConf != nil {
+		b.updatedConfig(bConf)
+	}
 	if err := b.lockThenpopulateCRLs(ctx, conf.StorageView); err != nil {
 		return nil, err
 	}
@@ -50,16 +60,18 @@ func Backend() *backend {
 	}
 
 	b.crlUpdateMutex = &sync.RWMutex{}
-
 	return &b
 }
 
 type backend struct {
 	*framework.Backend
 	MapCertId *framework.PathMap
 
-	crls           map[string]CRLInfo
-	crlUpdateMutex *sync.RWMutex
+	crls            map[string]CRLInfo
+	crlUpdateMutex  *sync.RWMutex
+	ocspClientMutex sync.RWMutex
+	ocspClient      *ocsp.Client
+	configUpdated   atomic.Bool
 }
 
 func (b *backend) invalidate(_ context.Context, key string) {
@@ -68,9 +80,25 @@ func (b *backend) invalidate(_ context.Context, key string) {
 		b.crlUpdateMutex.Lock()
 		defer b.crlUpdateMutex.Unlock()
 		b.crls = nil
+	case key == "config":
+		b.configUpdated.Store(true)
 	}
 }
 
+func (b *backend) initOCSPClient(cacheSize int) {
+	b.ocspClient = ocsp.New(func() hclog.Logger {
+		return b.Logger()
+	}, cacheSize)
+}
+
+func (b *backend) updatedConfig(config *config) error {
+	b.ocspClientMutex.Lock()
+	defer b.ocspClientMutex.Unlock()
+	b.initOCSPClient(config.OcspCacheSize)
+	b.configUpdated.Store(false)
+	return nil
+}
+
 func (b *backend) fetchCRL(ctx context.Context, storage logical.Storage, name string, crl *CRLInfo) error {
 	response, err := http.Get(crl.CDP.Url)
 	if err != nil {
@@ -105,6 +133,19 @@ func (b *backend) updateCRLs(ctx context.Context, req *logical.Request) error {
 	return errs.ErrorOrNil()
 }
 
+func (b *backend) storeConfig(ctx context.Context, storage logical.Storage, config *config) error {
+	entry, err := logical.StorageEntryJSON("config", config)
+	if err != nil {
+		return err
+	}
+
+	if err := storage.Put(ctx, entry); err != nil {
+		return err
+	}
+	b.updatedConfig(config)
+	return nil
+}
+
 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/backend_test.go b/builtin/credential/cert/backend_test.go
@@ -1092,12 +1092,13 @@ func TestBackend_CRLs(t *testing.T) {
 }
 
 func testFactory(t *testing.T) logical.Backend {
+	storage := &logical.InmemStorage{}
 	b, err := Factory(context.Background(), &logical.BackendConfig{
 		System: &logical.StaticSystemView{
 			DefaultLeaseTTLVal: 1000 * time.Second,
 			MaxLeaseTTLVal:     1800 * time.Second,
 		},
-		StorageView: &logical.InmemStorage{},
+		StorageView: storage,
 	})
 	if err != nil {
 		t.Fatalf("error: %s", err)
@@ -1923,27 +1924,33 @@ type allowed struct {
 	metadata_ext         string // allowed metadata extensions to add to identity alias
 }
 
-func testAccStepCert(
-	t *testing.T, name string, cert []byte, policies string, testData allowed, expectError bool,
-) logicaltest.TestStep {
+func testAccStepCert(t *testing.T, name string, cert []byte, policies string, testData allowed, expectError bool) logicaltest.TestStep {
+	return testAccStepCertWithExtraParams(t, name, cert, policies, testData, expectError, nil)
+}
+
+func testAccStepCertWithExtraParams(t *testing.T, name string, cert []byte, policies string, testData allowed, expectError bool, extraParams map[string]interface{}) logicaltest.TestStep {
+	data := map[string]interface{}{
+		"certificate":                  string(cert),
+		"policies":                     policies,
+		"display_name":                 name,
+		"allowed_names":                testData.names,
+		"allowed_common_names":         testData.common_names,
+		"allowed_dns_sans":             testData.dns,
+		"allowed_email_sans":           testData.emails,
+		"allowed_uri_sans":             testData.uris,
+		"allowed_organizational_units": testData.organizational_units,
+		"required_extensions":          testData.ext,
+		"allowed_metadata_extensions":  testData.metadata_ext,
+		"lease":                        1000,
+	}
+	for k, v := range extraParams {
+		data[k] = v
+	}
 	return logicaltest.TestStep{
 		Operation: logical.UpdateOperation,
 		Path:      "certs/" + name,
 		ErrorOk:   expectError,
-		Data: map[string]interface{}{
-			"certificate":                  string(cert),
-			"policies":                     policies,
-			"display_name":                 name,
-			"allowed_names":                testData.names,
-			"allowed_common_names":         testData.common_names,
-			"allowed_dns_sans":             testData.dns,
-			"allowed_email_sans":           testData.emails,
-			"allowed_uri_sans":             testData.uris,
-			"allowed_organizational_units": testData.organizational_units,
-			"required_extensions":          testData.ext,
-			"allowed_metadata_extensions":  testData.metadata_ext,
-			"lease":                        1000,
-		},
+		Data:      data,
 		Check: func(resp *logical.Response) error {
 			if resp == nil && expectError {
 				return fmt.Errorf("expected error but received nil")

diff --git a/builtin/credential/cert/path_certs.go b/builtin/credential/cert/path_certs.go
@@ -7,7 +7,8 @@ import (
 	"strings"
 	"time"
 
-	sockaddr "github.com/hashicorp/go-sockaddr"
+	"github.com/hashicorp/go-sockaddr"
+
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/tokenutil"
 	"github.com/hashicorp/vault/sdk/logical"
@@ -47,7 +48,32 @@ Must be x509 PEM encoded.`,
 					EditType: "file",
 				},
 			},
-
+			"ocsp_enabled": {
+				Type:        framework.TypeBool,
+				Description: `Whether to attempt OCSP verification of certificates at login`,
+			},
+			"ocsp_ca_certificates": {
+				Type:        framework.TypeString,
+				Description: `Any additional CA certificates needed to communicate with OCSP servers`,
+				DisplayAttrs: &framework.DisplayAttributes{
+					EditType: "file",
+				},
+			},
+			"ocsp_servers_override": {
+				Type: framework.TypeCommaStringSlice,
+				Description: `A comma-separated list of OCSP server addresses.  If unset, the OCSP server is determined 
+from the AuthorityInformationAccess extension on the certificate being inspected.`,
+			},
+			"ocsp_fail_open": {
+				Type:        framework.TypeBool,
+				Default:     false,
+				Description: "If set to true, if an OCSP revocation cannot be made successfully, login will proceed rather than failing.  If false, failing to get an OCSP status fails the request.",
+			},
+			"ocsp_query_all_servers": {
+				Type:        framework.TypeBool,
+				Default:     false,
+				Description: "If set to true, rather than accepting the first successful OCSP response, query all servers and consider the certificate valid only if all servers agree.",
+			},
 			"allowed_names": {
 				Type: framework.TypeCommaStringSlice,
 				Description: `A comma-separated list of names.
@@ -294,6 +320,21 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
 	if certificateRaw, ok := d.GetOk("certificate"); ok {
 		cert.Certificate = certificateRaw.(string)
 	}
+	if ocspCertificatesRaw, ok := d.GetOk("ocsp_ca_certificates"); ok {
+		cert.OcspCaCertificates = ocspCertificatesRaw.(string)
+	}
+	if ocspEnabledRaw, ok := d.GetOk("ocsp_enabled"); ok {
+		cert.OcspEnabled = ocspEnabledRaw.(bool)
+	}
+	if ocspServerOverrides, ok := d.GetOk("ocsp_servers_override"); ok {
+		cert.OcspServersOverride = ocspServerOverrides.([]string)
+	}
+	if ocspFailOpen, ok := d.GetOk("ocsp_fail_open"); ok {
+		cert.OcspFailOpen = ocspFailOpen.(bool)
+	}
+	if ocspQueryAll, ok := d.GetOk("ocsp_query_all_servers"); ok {
+		cert.OcspQueryAllServers = ocspQueryAll.(bool)
+	}
 	if displayNameRaw, ok := d.GetOk("display_name"); ok {
 		cert.DisplayName = displayNameRaw.(string)
 	}
@@ -399,7 +440,7 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
 			}
 		}
 		if !clientAuth {
-			return logical.ErrorResponse("non-CA certificates should have TLS client authentication set as an extended key usage"), nil
+			return logical.ErrorResponse("nonCA certificates should have TLS client authentication set as an extended key usage"), nil
 		}
 	}
 
@@ -438,6 +479,12 @@ type CertEntry struct {
 	RequiredExtensions         []string
 	AllowedMetadataExtensions  []string
 	BoundCIDRs                 []*sockaddr.SockAddrMarshaler
+
+	OcspCaCertificates  string
+	OcspEnabled         bool
+	OcspServersOverride []string
+	OcspFailOpen        bool
+	OcspQueryAllServers bool
 }
 
 const pathCertHelpSyn = `
@@ -449,6 +496,7 @@ This endpoint allows you to create, read, update, and delete trusted certificate
 that are allowed to authenticate.
 
 Deleting a certificate will not revoke auth for prior authenticated connections.
-To do this, do a revoke on "login". If you don't need to revoke login immediately,
+To do this, do a revoke on "login". If you don'log need to revoke login immediately,
 then the next renew will cause the lease to expire.
+
 `
diff --git a/builtin/credential/cert/path_config.go b/builtin/credential/cert/path_config.go
@@ -8,6 +8,8 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
+const maxCacheSize = 100000
+
 func pathConfig(b *backend) *framework.Path {
 	return &framework.Path{
 		Pattern: "config",
@@ -22,6 +24,11 @@ func pathConfig(b *backend) *framework.Path {
 				Default:     false,
 				Description: `If set, metadata of the certificate including the metadata corresponding to allowed_metadata_extensions will be stored in the alias. Defaults to false.`,
 			},
+			"ocsp_cache_size": {
+				Type:        framework.TypeInt,
+				Default:     100,
+				Description: `The size of the in memory OCSP response cache, shared by all configured certs`,
+			},
 		},
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
@@ -32,18 +39,25 @@ func pathConfig(b *backend) *framework.Path {
 }
 
 func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	disableBinding := data.Get("disable_binding").(bool)
-	enableIdentityAliasMetadata := data.Get("enable_identity_alias_metadata").(bool)
-
-	entry, err := logical.StorageEntryJSON("config", config{
-		DisableBinding:              disableBinding,
-		EnableIdentityAliasMetadata: enableIdentityAliasMetadata,
-	})
+	config, err := b.Config(ctx, req.Storage)
 	if err != nil {
 		return nil, err
 	}
 
-	if err := req.Storage.Put(ctx, entry); err != nil {
+	if disableBindingRaw, ok := data.GetOk("disable_binding"); ok {
+		config.DisableBinding = disableBindingRaw.(bool)
+	}
+	if enableIdentityAliasMetadataRaw, ok := data.GetOk("enable_identity_alias_metadata"); ok {
+		config.EnableIdentityAliasMetadata = enableIdentityAliasMetadataRaw.(bool)
+	}
+	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
+		}
+		config.OcspCacheSize = cacheSize
+	}
+	if err := b.storeConfig(ctx, req.Storage, config); err != nil {
 		return nil, err
 	}
 	return nil, nil
@@ -58,6 +72,7 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, d *f
 	data := map[string]interface{}{
 		"disable_binding":                cfg.DisableBinding,
 		"enable_identity_alias_metadata": cfg.EnableIdentityAliasMetadata,
+		"ocsp_cache_size":                cfg.OcspCacheSize,
 	}
 
 	return &logical.Response{
@@ -85,4 +100,5 @@ func (b *backend) Config(ctx context.Context, s logical.Storage) (*config, error
 type config struct {
 	DisableBinding              bool `json:"disable_binding"`
 	EnableIdentityAliasMetadata bool `json:"enable_identity_alias_metadata"`
+	OcspCacheSize               int  `json:"ocsp_cache_size"`
 }