Add ocsp_expiry configuration field to PKI crl config (#16888)

* Add ocsp_expiry configuration field to PKI crl config

 - Add a new configurable duration field to the crl configuration to
   allow operator control of how long an OCSP response can be cached
   for.
 - This is useful for how long a server like NGINX/Apache is
   allowed to cache the response for OCSP stapling.
 - A value of 0 means no one should cache the response.
 - Address an issue discovered that we did not upgrade existing crl
   configurations properly

* PR feedback

builtin/logical/pki/crl_test.go

@@ -32,58 +32,104 @@ func TestBackend_CRL_EnableDisableRoot(t *testing.T) {
 	crlEnableDisableTestForBackend(t, b, s, []string{caSerial})
 }
 
+func TestBackend_CRLConfigUpdate(t *testing.T) {
+	b, s := createBackendWithStorage(t)
+
+	// Write a legacy config to storage.
+	type legacyConfig struct {
+		Expiry  string `json:"expiry"`
+		Disable bool   `json:"disable"`
+	}
+	oldConfig := legacyConfig{Expiry: "24h", Disable: false}
+	entry, err := logical.StorageEntryJSON("config/crl", oldConfig)
+	require.NoError(t, err, "generate storage entry objection with legacy config")
+	err = s.Put(ctx, entry)
+	require.NoError(t, err, "failed writing legacy config")
+
+	// Now lets read it.
+	resp, err := CBRead(b, s, "config/crl")
+	requireSuccessNonNilResponse(t, resp, err)
+	requireFieldsSetInResp(t, resp, "disable", "expiry", "ocsp_disable", "auto_rebuild", "auto_rebuild_grace_period")
+
+	require.Equal(t, "24h", resp.Data["expiry"])
+	require.Equal(t, false, resp.Data["disable"])
+	require.Equal(t, defaultCrlConfig.OcspDisable, resp.Data["ocsp_disable"])
+	require.Equal(t, defaultCrlConfig.OcspExpiry, resp.Data["ocsp_expiry"])
+	require.Equal(t, defaultCrlConfig.AutoRebuild, resp.Data["auto_rebuild"])
+	require.Equal(t, defaultCrlConfig.AutoRebuildGracePeriod, resp.Data["auto_rebuild_grace_period"])
+}
+
 func TestBackend_CRLConfig(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
-		expiry      string
-		disable     bool
-		ocspDisable bool
+		expiry                 string
+		disable                bool
+		ocspDisable            bool
+		ocspExpiry             string
+		autoRebuild            bool
+		autoRebuildGracePeriod string
 	}{
-		{expiry: "24h", disable: true, ocspDisable: true},
-		{expiry: "16h", disable: false, ocspDisable: true},
-		{expiry: "8h", disable: true, ocspDisable: false},
+		{expiry: "24h", disable: true, ocspDisable: true, ocspExpiry: "72h", autoRebuild: false, autoRebuildGracePeriod: "36h"},
+		{expiry: "16h", disable: false, ocspDisable: true, ocspExpiry: "0h", autoRebuild: true, autoRebuildGracePeriod: "1h"},
+		{expiry: "8h", disable: true, ocspDisable: false, ocspExpiry: "24h", autoRebuild: false, autoRebuildGracePeriod: "24h"},
 	}
 	for _, tc := range tests {
 		name := fmt.Sprintf("%s-%t-%t", tc.expiry, tc.disable, tc.ocspDisable)
 		t.Run(name, func(t *testing.T) {
 			b, s := createBackendWithStorage(t)
 
 			resp, err := CBWrite(b, s, "config/crl", map[string]interface{}{
-				"expiry":       tc.expiry,
-				"disable":      tc.disable,
-				"ocsp_disable": tc.ocspDisable,
+				"expiry":                    tc.expiry,
+				"disable":                   tc.disable,
+				"ocsp_disable":              tc.ocspDisable,
+				"ocsp_expiry":               tc.ocspExpiry,
+				"auto_rebuild":              tc.autoRebuild,
+				"auto_rebuild_grace_period": tc.autoRebuildGracePeriod,
 			})
 			requireSuccessNilResponse(t, resp, err)
 
 			resp, err = CBRead(b, s, "config/crl")
 			requireSuccessNonNilResponse(t, resp, err)
-			requireFieldsSetInResp(t, resp, "disable", "expiry", "ocsp_disable")
+			requireFieldsSetInResp(t, resp, "disable", "expiry", "ocsp_disable", "auto_rebuild", "auto_rebuild_grace_period")
 
 			require.Equal(t, tc.expiry, resp.Data["expiry"])
 			require.Equal(t, tc.disable, resp.Data["disable"])
 			require.Equal(t, tc.ocspDisable, resp.Data["ocsp_disable"])
+			require.Equal(t, tc.ocspExpiry, resp.Data["ocsp_expiry"])
+			require.Equal(t, tc.autoRebuild, resp.Data["auto_rebuild"])
+			require.Equal(t, tc.autoRebuildGracePeriod, resp.Data["auto_rebuild_grace_period"])
 		})
 	}
 
 	badValueTests := []struct {
-		expiry      string
-		disable     string
-		ocspDisable string
+		expiry                 string
+		disable                string
+		ocspDisable            string
+		ocspExpiry             string
+		autoRebuild            string
+		autoRebuildGracePeriod string
 	}{
-		{expiry: "not a duration", disable: "true", ocspDisable: "true"},
-		{expiry: "16h", disable: "not a boolean", ocspDisable: "true"},
-		{expiry: "8h", disable: "true", ocspDisable: "not a boolean"},
+		{expiry: "not a duration", disable: "true", ocspDisable: "true", ocspExpiry: "72h", autoRebuild: "true", autoRebuildGracePeriod: "1d"},
+		{expiry: "16h", disable: "not a boolean", ocspDisable: "true", ocspExpiry: "72h", autoRebuild: "true", autoRebuildGracePeriod: "1d"},
+		{expiry: "8h", disable: "true", ocspDisable: "not a boolean", ocspExpiry: "72h", autoRebuild: "true", autoRebuildGracePeriod: "1d"},
+		{expiry: "8h", disable: "true", ocspDisable: "true", ocspExpiry: "not a duration", autoRebuild: "true", autoRebuildGracePeriod: "1d"},
+		{expiry: "8h", disable: "true", ocspDisable: "true", ocspExpiry: "-1", autoRebuild: "true", autoRebuildGracePeriod: "1d"},
+		{expiry: "8h", disable: "true", ocspDisable: "true", ocspExpiry: "72h", autoRebuild: "not a boolean", autoRebuildGracePeriod: "1d"},
+		{expiry: "8h", disable: "true", ocspDisable: "true", ocspExpiry: "-1", autoRebuild: "true", autoRebuildGracePeriod: "not a duration"},
 	}
 	for _, tc := range badValueTests {
 		name := fmt.Sprintf("bad-%s-%s-%s", tc.expiry, tc.disable, tc.ocspDisable)
 		t.Run(name, func(t *testing.T) {
 			b, s := createBackendWithStorage(t)
 
 			_, err := CBWrite(b, s, "config/crl", map[string]interface{}{
-				"expiry":       tc.expiry,
-				"disable":      tc.disable,
-				"ocsp_disable": tc.ocspDisable,
+				"expiry":                    tc.expiry,
+				"disable":                   tc.disable,
+				"ocsp_disable":              tc.ocspDisable,
+				"ocsp_expiry":               tc.ocspExpiry,
+				"auto_rebuild":              tc.autoRebuild,
+				"auto_rebuild_grace_period": tc.autoRebuildGracePeriod,
 			})
 			require.Error(t, err)
 		})

builtin/logical/pki/ocsp.go

@@ -129,7 +129,7 @@ func (b *backend) ocspHandler(ctx context.Context, request *logical.Request, dat
 			// Since we were not able to find a matching issuer for the incoming request
 			// generate an Unknown OCSP response. This might turn into an Unauthorized if
 			// we find out that we don't have a default issuer or it's missing the proper Usage flags
-			return generateUnknownResponse(sc, ocspReq), nil
+			return generateUnknownResponse(cfg, sc, ocspReq), nil
 		}
 		if errors.Is(err, ErrMissingOcspUsage) {
 			// If we did find a matching issuer but aren't allowed to sign, the spec says
@@ -141,7 +141,7 @@ func (b *backend) ocspHandler(ctx context.Context, request *logical.Request, dat
 		return logAndReturnInternalError(b, err), nil
 	}
 
-	byteResp, err := genResponse(caBundle, ocspStatus, ocspReq.HashAlgorithm)
+	byteResp, err := genResponse(cfg, caBundle, ocspStatus, ocspReq.HashAlgorithm)
 	if err != nil {
 		return logAndReturnInternalError(b, err), nil
 	}
@@ -155,7 +155,7 @@ func (b *backend) ocspHandler(ctx context.Context, request *logical.Request, dat
 	}, nil
 }
 
-func generateUnknownResponse(sc *storageContext, ocspReq *ocsp.Request) *logical.Response {
+func generateUnknownResponse(cfg *crlConfig, sc *storageContext, ocspReq *ocsp.Request) *logical.Response {
 	// Generate an Unknown OCSP response, signing with the default issuer from the mount as we did
 	// not match the request's issuer. If no default issuer can be used, return with Unauthorized as there
 	// isn't much else we can do at this point.
@@ -189,7 +189,7 @@ func generateUnknownResponse(sc *storageContext, ocspReq *ocsp.Request) *logical
 		ocspStatus:   ocsp.Unknown,
 	}
 
-	byteResp, err := genResponse(caBundle, info, ocspReq.HashAlgorithm)
+	byteResp, err := genResponse(cfg, caBundle, info, ocspReq.HashAlgorithm)
 	if err != nil {
 		return logAndReturnInternalError(sc.Backend, err)
 	}
@@ -384,14 +384,18 @@ func doesRequestMatchIssuer(parsedBundle *certutil.ParsedCertBundle, req *ocsp.R
 	return bytes.Equal(req.IssuerKeyHash, issuerKeyHash) && bytes.Equal(req.IssuerNameHash, issuerNameHash), nil
 }
 
-func genResponse(caBundle *certutil.ParsedCertBundle, info *ocspRespInfo, reqHash crypto.Hash) ([]byte, error) {
+func genResponse(cfg *crlConfig, caBundle *certutil.ParsedCertBundle, info *ocspRespInfo, reqHash crypto.Hash) ([]byte, error) {
 	curTime := time.Now()
+	duration, err := time.ParseDuration(cfg.OcspExpiry)
+	if err != nil {
+		return nil, err
+	}
 	template := ocsp.Response{
 		IssuerHash:      reqHash,
 		Status:          info.ocspStatus,
 		SerialNumber:    info.serialNumber,
 		ThisUpdate:      curTime,
-		NextUpdate:      curTime,
+		NextUpdate:      curTime.Add(duration),
 		Certificate:     caBundle.Certificate,
 		ExtraExtensions: []pkix.Extension{},
 	}

builtin/logical/pki/ocsp_test.go

@@ -16,6 +16,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/stretchr/testify/require"
@@ -457,6 +458,18 @@ func runOcspRequestTest(t *testing.T, requestType string, caKeyType string, requ
 	require.Equal(t, 0, ocspResp.RevocationReason)
 	require.Equal(t, testEnv.leafCertIssuer2.SerialNumber, ocspResp.SerialNumber)
 
+	// Verify that our thisUpdate and nextUpdate fields are updated as expected
+	thisUpdate := ocspResp.ThisUpdate
+	nextUpdate := ocspResp.NextUpdate
+	require.True(t, thisUpdate.Before(nextUpdate),
+		fmt.Sprintf("thisUpdate %s, should have been before nextUpdate: %s", thisUpdate, nextUpdate))
+	nextUpdateDiff := nextUpdate.Sub(thisUpdate)
+	expectedDiff, err := time.ParseDuration(defaultCrlConfig.OcspExpiry)
+	require.NoError(t, err, "failed to parse default ocsp expiry value")
+	require.Equal(t, expectedDiff, nextUpdateDiff,
+		fmt.Sprintf("the delta between thisUpdate %s and nextUpdate: %s should have been around: %s but was %s",
+			thisUpdate, nextUpdate, defaultCrlConfig.OcspExpiry, nextUpdateDiff))
+
 	requireOcspSignatureAlgoForKey(t, testEnv.issuer2.PublicKey, ocspResp.SignatureAlgorithm)
 	requireOcspResponseSignedBy(t, ocspResp, testEnv.issuer2.PublicKey)
 }

builtin/logical/pki/path_config_crl.go

@@ -10,20 +10,26 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
+const latestCrlConfigVersion = 1
+
 // CRLConfig holds basic CRL configuration information
 type crlConfig struct {
+	Version                int    `json:"version"`
 	Expiry                 string `json:"expiry"`
 	Disable                bool   `json:"disable"`
 	OcspDisable            bool   `json:"ocsp_disable"`
 	AutoRebuild            bool   `json:"auto_rebuild"`
 	AutoRebuildGracePeriod string `json:"auto_rebuild_grace_period"`
+	OcspExpiry             string `json:"ocsp_expiry"`
 }
 
 // Implicit default values for the config if it does not exist.
 var defaultCrlConfig = crlConfig{
+	Version:                latestCrlConfigVersion,
 	Expiry:                 "72h",
 	Disable:                false,
 	OcspDisable:            false,
+	OcspExpiry:             "12h",
 	AutoRebuild:            false,
 	AutoRebuildGracePeriod: "12h",
 }
@@ -46,6 +52,12 @@ valid; defaults to 72 hours`,
 				Type:        framework.TypeBool,
 				Description: `If set to true, ocsp unauthorized responses will be returned.`,
 			},
+			"ocsp_expiry": {
+				Type: framework.TypeString,
+				Description: `The amount of time an OCSP response will be valid (controls 
+the NextUpdate field); defaults to 12 hours`,
+				Default: "1h",
+			},
 			"auto_rebuild": {
 				Type:        framework.TypeBool,
 				Description: `If set to true, enables automatic rebuilding of the CRL`,
@@ -86,6 +98,7 @@ func (b *backend) pathCRLRead(ctx context.Context, req *logical.Request, _ *fram
 			"expiry":                    config.Expiry,
 			"disable":                   config.Disable,
 			"ocsp_disable":              config.OcspDisable,
+			"ocsp_expiry":               config.OcspExpiry,
 			"auto_rebuild":              config.AutoRebuild,
 			"auto_rebuild_grace_period": config.AutoRebuildGracePeriod,
 		},
@@ -117,6 +130,18 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra
 		config.OcspDisable = ocspDisableRaw.(bool)
 	}
 
+	if expiryRaw, ok := d.GetOk("ocsp_expiry"); ok {
+		expiry := expiryRaw.(string)
+		duration, err := time.ParseDuration(expiry)
+		if err != nil {
+			return logical.ErrorResponse(fmt.Sprintf("given ocsp_expiry could not be decoded: %s", err)), nil
+		}
+		if duration < 0 {
+			return logical.ErrorResponse(fmt.Sprintf("ocsp_expiry must be greater than or equal to 0 got: %s", duration)), nil
+		}
+		config.OcspExpiry = expiry
+	}
+
 	oldAutoRebuild := config.AutoRebuild
 	if autoRebuildRaw, ok := d.GetOk("auto_rebuild"); ok {
 		config.AutoRebuild = autoRebuildRaw.(bool)

builtin/logical/pki/storage.go

@@ -1120,5 +1120,14 @@ func (sc *storageContext) getRevocationConfig() (*crlConfig, error) {
 		return nil, err
 	}
 
+	if result.Version == 0 {
+		// Automatically update existing configurations.
+		result.OcspDisable = defaultCrlConfig.OcspDisable
+		result.OcspExpiry = defaultCrlConfig.OcspExpiry
+		result.AutoRebuild = defaultCrlConfig.AutoRebuild
+		result.AutoRebuildGracePeriod = defaultCrlConfig.AutoRebuildGracePeriod
+		result.Version = 1
+	}
+
 	return &result, nil
 }

website/content/api-docs/secret/pki.mdx

@@ -2989,7 +2989,10 @@ $ curl \
   "data": {
     "disable": false,
     "expiry": "72h",
-    "ocsp_disable": false
+    "ocsp_disable": false,
+    "ocsp_expiry": "12h",
+    "auto_rebuild": false,
+    "auto_rebuild_grace_period": "12h"
   },
   "auth": null
 }
@@ -3031,6 +3034,9 @@ the CRL.
 - `expiry` `(string: "72h")` - The amount of time the generated CRL should be valid.
 - `disable` `(bool: false)` - Disables or enables CRL building.
 - `ocsp_disable` `(bool: false)` - Disables or enables the OCSP responder in Vault.
+- `ocsp_expiry` `(string: "12h")` - The amount of time an OCSP response can be cached for,
+  (controls the NextUpdate field), useful for OCSP stapling refresh durations. Setting to 0
+  should effectively disable caching in third party systems.
 - `auto_rebuild` `(bool: false)` - Enables or disables periodic rebuilding of
   the CRL upon expiry.
 - `auto_rebuild_grace_period` `(string: "12h")` - Grace period before CRL expiry
@@ -3043,6 +3049,7 @@ the CRL.
   "expiry": "48h",
   "disable": "false",
   "ocsp_disable": "false",
+  "ocsp_expiry": "12h",
   "auto_rebuild": "true",
   "auto_rebuild_grace_period": "8h"
 }