From ff81ab8a30171daa167b908f4ec2e9b64ad2d000 Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Mon, 20 Nov 2023 10:58:50 -0500 Subject: [PATCH] backport of commit 53040690a2bb89a71b723cd888411182295abcd6 (#24195) Co-authored-by: Steven Clark --- builtin/logical/pki/path_ocsp.go | 5 ++- builtin/logical/pki/path_ocsp_test.go | 60 +++++++++++++++++++------ changelog/24192.txt | 3 ++ website/content/api-docs/secret/pki.mdx | 5 ++- 4 files changed, 56 insertions(+), 17 deletions(-) create mode 100644 changelog/24192.txt diff --git a/builtin/logical/pki/path_ocsp.go b/builtin/logical/pki/path_ocsp.go index 6d80c87d5..1c90ce486 100644 --- a/builtin/logical/pki/path_ocsp.go +++ b/builtin/logical/pki/path_ocsp.go @@ -510,11 +510,14 @@ func genResponse(cfg *crlConfig, caBundle *certutil.ParsedCertBundle, info *ocsp Status: info.ocspStatus, SerialNumber: info.serialNumber, ThisUpdate: curTime, - NextUpdate: curTime.Add(duration), ExtraExtensions: []pkix.Extension{}, SignatureAlgorithm: revSigAlg, } + if duration > 0 { + template.NextUpdate = curTime.Add(duration) + } + if info.ocspStatus == ocsp.Revoked { template.RevokedAt = *info.revocationTimeUTC template.RevocationReason = ocsp.Unspecified diff --git a/builtin/logical/pki/path_ocsp_test.go b/builtin/logical/pki/path_ocsp_test.go index 07031690f..a3d214041 100644 --- a/builtin/logical/pki/path_ocsp_test.go +++ b/builtin/logical/pki/path_ocsp_test.go @@ -15,6 +15,7 @@ import ( "strconv" "strings" "testing" + "time" "github.com/hashicorp/go-secure-stdlib/parseutil" vaulthttp "github.com/hashicorp/vault/http" @@ -467,6 +468,18 @@ func TestOcsp_HigherLevel(t *testing.T) { require.Equal(t, certToRevoke.SerialNumber, ocspResp.SerialNumber) } +// TestOcsp_NextUpdate make sure that we are setting the appropriate values +// for the NextUpdate field within our responses. +func TestOcsp_NextUpdate(t *testing.T) { + // Within the runOcspRequestTest, with a ocspExpiry of 0, + // we will validate that NextUpdate was not set in the response + runOcspRequestTest(t, "POST", "ec", 0, 0, crypto.SHA256, 0) + + // Within the runOcspRequestTest, with a ocspExpiry of 24 hours, we will validate + // that NextUpdate is set and has a time 24 hours larger than ThisUpdate + runOcspRequestTest(t, "POST", "ec", 0, 0, crypto.SHA256, 24*time.Hour) +} + func TestOcsp_ValidRequests(t *testing.T) { type caKeyConf struct { keyType string @@ -506,13 +519,15 @@ func TestOcsp_ValidRequests(t *testing.T) { localTT.reqHash) t.Run(testName, func(t *testing.T) { runOcspRequestTest(t, localTT.reqType, localTT.keyConf.keyType, localTT.keyConf.keyBits, - localTT.keyConf.sigBits, localTT.reqHash) + localTT.keyConf.sigBits, localTT.reqHash, 12*time.Hour) }) } } -func runOcspRequestTest(t *testing.T, requestType string, caKeyType string, caKeyBits int, caKeySigBits int, requestHash crypto.Hash) { - b, s, testEnv := setupOcspEnvWithCaKeyConfig(t, caKeyType, caKeyBits, caKeySigBits) +func runOcspRequestTest(t *testing.T, requestType string, caKeyType string, + caKeyBits int, caKeySigBits int, requestHash crypto.Hash, ocspExpiry time.Duration, +) { + b, s, testEnv := setupOcspEnvWithCaKeyConfig(t, caKeyType, caKeyBits, caKeySigBits, ocspExpiry) // Non-revoked cert resp, err := SendOcspRequest(t, b, s, requestType, testEnv.leafCertIssuer1, testEnv.issuer1, requestHash) @@ -574,17 +589,28 @@ func runOcspRequestTest(t *testing.T, requestType string, caKeyType string, caKe 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 := parseutil.ParseDurationSecond(defaultCrlConfig.OcspExpiry) + resp, err = CBRead(b, s, "config/crl") + requireSuccessNonNilResponse(t, resp, err, "failed reading from config/crl") + requireFieldsSetInResp(t, resp, "ocsp_expiry") + ocspExpiryRaw := resp.Data["ocsp_expiry"].(string) + expectedDiff, err := parseutil.ParseDurationSecond(ocspExpiryRaw) 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)) + thisUpdate := ocspResp.ThisUpdate + require.Less(t, time.Since(thisUpdate), 10*time.Second, "expected ThisUpdate field to be within the last 10 seconds") + if expectedDiff != 0 { + nextUpdate := ocspResp.NextUpdate + require.False(t, nextUpdate.IsZero(), "nextUpdate field value should have been a non-zero time") + require.True(t, thisUpdate.Before(nextUpdate), + fmt.Sprintf("thisUpdate %s, should have been before nextUpdate: %s", thisUpdate, nextUpdate)) + nextUpdateDiff := nextUpdate.Sub(thisUpdate) + 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)) + } else { + // With the config value set to 0, we shouldn't have a NextUpdate field set + require.True(t, ocspResp.NextUpdate.IsZero(), "nextUpdate value was not zero as expected was: %v", ocspResp.NextUpdate) + } requireOcspSignatureAlgoForKey(t, testEnv.issuer2.SignatureAlgorithm, ocspResp.SignatureAlgorithm) requireOcspResponseSignedBy(t, ocspResp, testEnv.issuer2) } @@ -610,16 +636,22 @@ type ocspTestEnv struct { } func setupOcspEnv(t *testing.T, keyType string) (*backend, logical.Storage, *ocspTestEnv) { - return setupOcspEnvWithCaKeyConfig(t, keyType, 0, 0) + return setupOcspEnvWithCaKeyConfig(t, keyType, 0, 0, 12*time.Hour) } -func setupOcspEnvWithCaKeyConfig(t *testing.T, keyType string, caKeyBits int, caKeySigBits int) (*backend, logical.Storage, *ocspTestEnv) { +func setupOcspEnvWithCaKeyConfig(t *testing.T, keyType string, caKeyBits int, caKeySigBits int, ocspExpiry time.Duration) (*backend, logical.Storage, *ocspTestEnv) { b, s := CreateBackendWithStorage(t) var issuerCerts []*x509.Certificate var leafCerts []*x509.Certificate var issuerIds []issuerID var keyIds []keyID + resp, err := CBWrite(b, s, "config/crl", map[string]interface{}{ + "ocsp_enable": true, + "ocsp_expiry": fmt.Sprintf("%ds", int(ocspExpiry.Seconds())), + }) + requireSuccessNonNilResponse(t, resp, err, "config/crl failed") + for i := 0; i < 2; i++ { resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{ "key_type": keyType, diff --git a/changelog/24192.txt b/changelog/24192.txt new file mode 100644 index 000000000..97a26746b --- /dev/null +++ b/changelog/24192.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: Do not set nextUpdate field in OCSP responses when ocsp_expiry is 0 +``` diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index b69b343cb..0a8a4b4ee 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -3807,8 +3807,9 @@ the CRL. - `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. + (controls the NextUpdate field), useful for OCSP stapling refresh durations. If set to 0 + the NextUpdate field is not set, indicating newer revocation information is available + all the time. - `auto_rebuild` `(bool: false)` - Enables or disables periodic rebuilding of the CRL upon expiry.