From 189a77630792a92d22733f7debd68a5f23814ec8 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Wed, 19 Apr 2023 12:55:37 -0400 Subject: [PATCH] Add warnings to crl rebuilds, allowing notifying operator of empty issuer equivalency sets (#20253) * Add infrastructure for warnings on CRL rebuilds Signed-off-by: Alexander Scheel * Add warning on issuer missing KU for CRL Signing When an entire issuer equivalency class is missing CRL signing usage (but otherwise has key material present), we should add a warning so operators can either correct this issuer or create an equivalent version with KU specified. Resolves: https://github.com/hashicorp/vault/issues/20137 Signed-off-by: Alexander Scheel * Add tests for issuer warnings Signed-off-by: Alexander Scheel * Add changelog entry Signed-off-by: Alexander Scheel * Fix return order of CRL builders Signed-off-by: Alexander Scheel --------- Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend.go | 20 ++- builtin/logical/pki/cert_util.go | 10 +- builtin/logical/pki/crl_test.go | 113 ++++++++++++- builtin/logical/pki/crl_util.go | 180 +++++++++++++-------- builtin/logical/pki/path_config_crl.go | 9 +- builtin/logical/pki/path_fetch_issuers.go | 13 +- builtin/logical/pki/path_manage_issuers.go | 10 +- builtin/logical/pki/path_revoke.go | 17 +- builtin/logical/pki/path_root.go | 5 +- builtin/logical/pki/path_tidy.go | 20 ++- changelog/20253.txt | 3 + 11 files changed, 314 insertions(+), 86 deletions(-) create mode 100644 changelog/20253.txt diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index 0dd1abd3e..509f22915 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -600,16 +600,32 @@ func (b *backend) periodicFunc(ctx context.Context, request *logical.Request) er } // Then attempt to rebuild the CRLs if required. - if err := b.crlBuilder.rebuildIfForced(sc); err != nil { + warnings, err := b.crlBuilder.rebuildIfForced(sc) + if err != nil { return err } + if len(warnings) > 0 { + msg := "During rebuild of complete CRL, got the following warnings:" + for index, warning := range warnings { + msg = fmt.Sprintf("%v\n %d. %v", msg, index+1, warning) + } + b.Logger().Warn(msg) + } // If a delta CRL was rebuilt above as part of the complete CRL rebuild, // this will be a no-op. However, if we do need to rebuild delta CRLs, // this would cause us to do so. - if err := b.crlBuilder.rebuildDeltaCRLsIfForced(sc, false); err != nil { + warnings, err = b.crlBuilder.rebuildDeltaCRLsIfForced(sc, false) + if err != nil { return err } + if len(warnings) > 0 { + msg := "During rebuild of delta CRL, got the following warnings:" + for index, warning := range warnings { + msg = fmt.Sprintf("%v\n %d. %v", msg, index+1, warning) + } + b.Logger().Warn(msg) + } return nil } diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index 435a89dc7..c9e99bd16 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -177,9 +177,17 @@ func fetchCertBySerial(sc *storageContext, prefix, serial string) (*logical.Stor legacyPath = "revoked/" + colonSerial path = "revoked/" + hyphenSerial case serial == legacyCRLPath || serial == deltaCRLPath || serial == unifiedCRLPath || serial == unifiedDeltaCRLPath: - if err = sc.Backend.crlBuilder.rebuildIfForced(sc); err != nil { + warnings, err := sc.Backend.crlBuilder.rebuildIfForced(sc) + if err != nil { return nil, err } + if len(warnings) > 0 { + msg := "During rebuild of CRL for cert fetch, got the following warnings:" + for index, warning := range warnings { + msg = fmt.Sprintf("%v\n %d. %v", msg, index+1, warning) + } + sc.Backend.Logger().Warn(msg) + } unified := serial == unifiedCRLPath || serial == unifiedDeltaCRLPath path, err = sc.resolveIssuerCRLPath(defaultRef, unified) diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go index a494a7b2c..5b121a4b6 100644 --- a/builtin/logical/pki/crl_test.go +++ b/builtin/logical/pki/crl_test.go @@ -415,15 +415,18 @@ func TestCrlRebuilder(t *testing.T) { cb := newCRLBuilder(true /* can rebuild and write CRLs */) // Force an initial build - err = cb.rebuild(sc, true) + warnings, err := cb.rebuild(sc, true) require.NoError(t, err, "Failed to rebuild CRL") + require.Empty(t, warnings, "unexpectedly got warnings rebuilding CRL") resp := requestCrlFromBackend(t, s, b) crl1 := parseCrlPemBytes(t, resp.Data["http_raw_body"].([]byte)) // We shouldn't rebuild within this call. - err = cb.rebuildIfForced(sc) + warnings, err = cb.rebuildIfForced(sc) require.NoError(t, err, "Failed to rebuild if forced CRL") + require.Empty(t, warnings, "unexpectedly got warnings rebuilding CRL") + resp = requestCrlFromBackend(t, s, b) crl2 := parseCrlPemBytes(t, resp.Data["http_raw_body"].([]byte)) require.Equal(t, crl1.ThisUpdate, crl2.ThisUpdate, "According to the update field, we rebuilt the CRL") @@ -439,8 +442,9 @@ func TestCrlRebuilder(t *testing.T) { // This should rebuild the CRL cb.requestRebuildIfActiveNode(b) - err = cb.rebuildIfForced(sc) + warnings, err = cb.rebuildIfForced(sc) require.NoError(t, err, "Failed to rebuild if forced CRL") + require.Empty(t, warnings, "unexpectedly got warnings rebuilding CRL") resp = requestCrlFromBackend(t, s, b) schema.ValidateResponse(t, schema.GetResponseSchema(t, b.Route("crl/pem"), logical.ReadOperation), resp, true) @@ -1325,3 +1329,106 @@ func requestCrlFromBackend(t *testing.T, s logical.Storage, b *backend) *logical require.False(t, resp.IsError(), "crl error response: %v", resp) return resp } + +func TestCRLWarningsEmptyKeyUsage(t *testing.T) { + t.Parallel() + + b, s := CreateBackendWithStorage(t) + + // Generated using OpenSSL with a configuration lacking KeyUsage on + // the CA certificate. + cert := `-----BEGIN CERTIFICATE----- +MIIDBjCCAe6gAwIBAgIBATANBgkqhkiG9w0BAQsFADATMREwDwYDVQQDDAhyb290 +LW9sZDAeFw0yMDAxMDEwMTAxMDFaFw0yMTAxMDEwMTAxMDFaMBMxETAPBgNVBAMM +CHJvb3Qtb2xkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAzqhSZxAL +PwFhCIPL1jFPq6jxp1wFgo6YNSfVI13gfaGIjfErxsQUbosmlEuTeOc50zXXN3kb +SDufy5Yi1OeSkFZRdJ78zdKzsEDIVR1ukUngVsSrt05gdNMJlh8XOPbcrJo78jYG +lRgtkkFSc/wCu+ue6JqkfKrbUY/G9WK0UM8ppHm1Ux67ZGoypyEgaqqxKHBRC4Yl +D+lAs1vP4C6cavqdUMKgAPTKmMBzlbpCuYPLHSzWh9Com3WQSqCbrlo3uH5RT3V9 +5Gjuk3mMUhY1l6fRL7wG3f+4x+DS+ICQNT0o4lnMxpIsiTh0cEHUFgY7G0iHWYPj +CIN8UDhpZIpoCQIDAQABo2UwYzAdBgNVHQ4EFgQUJlHk3PN7pfC22FGxAb0rWgQt +L4cwHwYDVR0jBBgwFoAUJlHk3PN7pfC22FGxAb0rWgQtL4cwDAYDVR0TBAUwAwEB +/zATBgNVHSUEDDAKBggrBgEFBQcDATANBgkqhkiG9w0BAQsFAAOCAQEAcaU0FbXb +FfXluBrjKfOzVKz+kvQ1CVv3xe3MBkS6wvqybBjJCFChnqCPxEe57BdSbBXNU5LZ +zCR/OqYas4Csv9+msSn9BI2FSMAmfMDTsp5/6iIQJqlJx9L8a7bjzVMGX6QJm/3x +S/EgGsMETAgewQXeu4jhI6StgJ2V/4Ofe498hYw4LAiBapJmkU/nHezWodNBZJ7h +LcLOzVj0Hu5MZplGBgJFgRqBCVVkqXA0q7tORuhNzYtNdJFpv3pZIhvVFFu3HUPf +wYQPhLye5WNtosz5xKe8X0Q9qp8g6azMTk+5Qe7u1d8MYAA2AIlGuKUvPHRruOmN +NC+gQnS7AK1lCw== +-----END CERTIFICATE-----` + privKey := `-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDOqFJnEAs/AWEI +g8vWMU+rqPGnXAWCjpg1J9UjXeB9oYiN8SvGxBRuiyaUS5N45znTNdc3eRtIO5/L +liLU55KQVlF0nvzN0rOwQMhVHW6RSeBWxKu3TmB00wmWHxc49tysmjvyNgaVGC2S +QVJz/AK7657omqR8qttRj8b1YrRQzymkebVTHrtkajKnISBqqrEocFELhiUP6UCz +W8/gLpxq+p1QwqAA9MqYwHOVukK5g8sdLNaH0KibdZBKoJuuWje4flFPdX3kaO6T +eYxSFjWXp9EvvAbd/7jH4NL4gJA1PSjiWczGkiyJOHRwQdQWBjsbSIdZg+MIg3xQ +OGlkimgJAgMBAAECggEABKmCdmXDwy+eR0ll41aoc/hzPzHRxADAiU51Pf+DrYHj +6UPcF3db+KR2Adl0ocEhqlSoHs3CIk6KC9c+wOvagBwaaVWe4WvT9vF3M4he8rMm +dv6n2xJPFcOfDz5zUSssjk5KdOvoGRv7BzYnDIvOafvmUVwPwuo92Wizddy8saf4 +Xuea0Cupz1PELPKkbXcAqb+TzbAZrwdPj1Y7vTe/KGE4+aoDqCW/sFB1E0UsMGlt +/yfGwFP48b7kdkqSpcEQW5H8+WL3TfqRcolCD9To4vo2J+1Po0S/8qPNRvkNQDDX +AypHtrXFBOWHpJgXT4rKyH+ZGJchrCRDblt9s/sNQwKBgQD7NytvYET3pWemYiX+ +MB9uc6cPuMFONvlzjA9T6dbOSi/HLaeDoW027aMUZqb7QeaQCoWcUwh13dI2SZq0 +5+l9hei4JkWjoDhbWmPe7zDuQr3UMl0CSk3egz3BSHkjAhRAuUxK0QLKGB23zWxz +k8mUWYZaZRA39C6aqMt/jbJjDwKBgQDSl+eO+DjpwPzrjPSphpF4xYo4XDje9ovK +9q4KTHye7Flc3cMCX3WZBmzdt0zbqu6gWZjJH0XbWX/+SkJBGh77XWD0FeQnU7Vk +ipoeb8zTsCVxD9EytQuXti3cqBgClcCMvLKgLOJIcNYTnygojwg3t+jboQqbtV7p +VpQfAC6jZwKBgQCxJ46x1CnOmg4l/0DbqAQCV/yP0bI//fSbz0Ff459fimF3DHL9 +GHF0MtC2Kk3HEgoNud3PB58Hv43mSrGWsZSuuCgM9LBXWz1i7rNPG05eNyK26W09 +mDihmduK2hjS3zx5CDMM76gP7EHIxEyelLGqtBdS18JAMypKVo5rPPl3cQKBgQCG +ueXLImQOr4dfDntLpSqV0BLAQcekZKhEPZJURmCHr37wGXNzpixurJyjL2w9MFqf +PRKwwJAJZ3Wp8kn2qkZd23x2Szb+LeBjJQS6Kh4o44zgixTz0r1K3qLygptxs+pO +Xz4LmQte+skKHo0rfW3tb3vKXnmR6fOBZgE23//2SwKBgHck44hoE1Ex2gDEfIq1 +04OBoS1cpuc9ge4uHEmv+8uANjzwlsYf8hY1qae513MGixRBOkxcI5xX/fYPQV9F +t3Jfh8QX85JjnGntuXuraYZJMUjpwXr3QHPx0jpvAM3Au5j6qD3biC9Vrwq9Chkg +hbiiPARizZA/Tsna/9ox1qDT +-----END PRIVATE KEY-----` + resp, err := CBWrite(b, s, "issuers/import/bundle", map[string]interface{}{ + "pem_bundle": cert + "\n" + privKey, + }) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotEmpty(t, resp.Warnings) + originalWarnings := resp.Warnings + + resp, err = CBRead(b, s, "crl/rotate") + require.NoError(t, err) + require.NotNil(t, resp) + require.NotEmpty(t, resp.Warnings) + + // All CRL-specific warnings should've already occurred earlier on the + // import's CRL rebuild. + for _, warning := range resp.Warnings { + require.Contains(t, originalWarnings, warning) + } + + // Deleting the issuer and key should remove the warning. + _, err = CBDelete(b, s, "root") + require.NoError(t, err) + + resp, err = CBRead(b, s, "crl/rotate") + require.NoError(t, err) + require.NotNil(t, resp) + require.Empty(t, resp.Warnings) + + // Adding back just the cert shouldn't cause CRL rebuild warnings. + resp, err = CBWrite(b, s, "issuers/import/bundle", map[string]interface{}{ + "pem_bundle": cert, + }) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotNil(t, resp.Data) + require.NotNil(t, resp.Data["mapping"]) + require.NotEmpty(t, resp.Data["mapping"]) + require.Equal(t, len(resp.Data["mapping"].(map[string]string)), 1) + for key, value := range resp.Data["mapping"].(map[string]string) { + require.NotEmpty(t, key) + require.Empty(t, value) + } + + resp, err = CBRead(b, s, "crl/rotate") + require.NoError(t, err) + require.NotNil(t, resp) + require.Empty(t, resp.Warnings) +} diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index f6e72bc39..e17b2a006 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -301,16 +301,16 @@ func (cb *crlBuilder) flushCRLBuildTimeInvalidation(sc *storageContext) error { // rebuildIfForced is to be called by readers or periodic functions that might need to trigger // a refresh of the CRL before the read occurs. -func (cb *crlBuilder) rebuildIfForced(sc *storageContext) error { +func (cb *crlBuilder) rebuildIfForced(sc *storageContext) ([]string, error) { if cb.forceRebuild.Load() { return cb._doRebuild(sc, true, _enforceForceFlag) } - return nil + return nil, nil } // rebuild is to be called by various write apis that know the CRL is to be updated and can be now. -func (cb *crlBuilder) rebuild(sc *storageContext, forceNew bool) error { +func (cb *crlBuilder) rebuild(sc *storageContext, forceNew bool) ([]string, error) { return cb._doRebuild(sc, forceNew, _ignoreForceFlag) } @@ -329,7 +329,7 @@ func (cb *crlBuilder) requestRebuildIfActiveNode(b *backend) { cb.forceRebuild.Store(true) } -func (cb *crlBuilder) _doRebuild(sc *storageContext, forceNew bool, ignoreForceFlag bool) error { +func (cb *crlBuilder) _doRebuild(sc *storageContext, forceNew bool, ignoreForceFlag bool) ([]string, error) { cb._builder.Lock() defer cb._builder.Unlock() // Re-read the lock in case someone beat us to the punch between the previous load op. @@ -346,7 +346,7 @@ func (cb *crlBuilder) _doRebuild(sc *storageContext, forceNew bool, ignoreForceF return buildCRLs(sc, myForceNew) } - return nil + return nil, nil } func (cb *crlBuilder) _getPresentDeltaWALForClearing(sc *storageContext, path string) ([]string, error) { @@ -415,7 +415,7 @@ func (cb *crlBuilder) clearUnifiedDeltaWAL(sc *storageContext, walSerials []stri return cb._clearDeltaWAL(sc, walSerials, unifiedDeltaWALPrefix) } -func (cb *crlBuilder) rebuildDeltaCRLsIfForced(sc *storageContext, override bool) error { +func (cb *crlBuilder) rebuildDeltaCRLsIfForced(sc *storageContext, override bool) ([]string, error) { // Delta CRLs use the same expiry duration as the complete CRL. Because // we always rebuild the complete CRL and then the delta CRL, we can // be assured that the delta CRL always expires after a complete CRL, @@ -427,18 +427,18 @@ func (cb *crlBuilder) rebuildDeltaCRLsIfForced(sc *storageContext, override bool // within our time window for updating it. cfg, err := cb.getConfigWithUpdate(sc) if err != nil { - return err + return nil, err } if !cfg.EnableDelta { // We explicitly do not update the last check time here, as we // want to persist the last rebuild window if it hasn't been set. - return nil + return nil, nil } deltaRebuildDuration, err := time.ParseDuration(cfg.DeltaRebuildInterval) if err != nil { - return err + return nil, err } // Acquire CRL building locks before we get too much further. @@ -454,7 +454,7 @@ func (cb *crlBuilder) rebuildDeltaCRLsIfForced(sc *storageContext, override bool // If we're still before the time of our next rebuild check, we can // safely return here even if we have certs. We'll wait for a bit, // retrigger this check, and then do the rebuild. - return nil + return nil, nil } // Update our check time. If we bail out below (due to storage errors @@ -465,16 +465,16 @@ func (cb *crlBuilder) rebuildDeltaCRLsIfForced(sc *storageContext, override bool rebuildLocal, err := cb._shouldRebuildLocalCRLs(sc, override) if err != nil { - return err + return nil, fmt.Errorf("error determining if local CRLs should be rebuilt: %w", err) } rebuildUnified, err := cb._shouldRebuildUnifiedCRLs(sc, override) if err != nil { - return err + return nil, fmt.Errorf("error determining if unified CRLs should be rebuilt: %w", err) } if !rebuildLocal && !rebuildUnified { - return nil + return nil, nil } // Finally, we must've needed to do the rebuild. Execute! @@ -601,14 +601,14 @@ func (cb *crlBuilder) _shouldRebuildUnifiedCRLs(sc *storageContext, override boo return shouldRebuild, nil } -func (cb *crlBuilder) rebuildDeltaCRLs(sc *storageContext, forceNew bool) error { +func (cb *crlBuilder) rebuildDeltaCRLs(sc *storageContext, forceNew bool) ([]string, error) { cb._builder.Lock() defer cb._builder.Unlock() return cb.rebuildDeltaCRLsHoldingLock(sc, forceNew) } -func (cb *crlBuilder) rebuildDeltaCRLsHoldingLock(sc *storageContext, forceNew bool) error { +func (cb *crlBuilder) rebuildDeltaCRLsHoldingLock(sc *storageContext, forceNew bool) ([]string, error) { return buildAnyCRLs(sc, forceNew, true /* building delta */) } @@ -1064,7 +1064,7 @@ func revokeCert(sc *storageContext, config *crlConfig, cert *x509.Certificate) ( // already rebuilt the full CRL so the Delta WAL will be cleared // afterwards. Writing an entry only to immediately remove it // isn't necessary. - crlErr := sc.Backend.crlBuilder.rebuild(sc, false) + warnings, crlErr := sc.Backend.crlBuilder.rebuild(sc, false) if crlErr != nil { switch crlErr.(type) { case errutil.UserError: @@ -1073,6 +1073,9 @@ func revokeCert(sc *storageContext, config *crlConfig, cert *x509.Certificate) ( return nil, fmt.Errorf("error encountered during CRL building: %w", crlErr) } } + for index, warning := range warnings { + resp.AddWarning(fmt.Sprintf("Warning %d during CRL rebuild: %v", index+1, warning)) + } } else if config.EnableDelta { if err := writeRevocationDeltaWALs(sc, config, resp, failedWritingUnifiedCRL, hyphenSerial, colonSerial); err != nil { return nil, fmt.Errorf("failed to write WAL entries for Delta CRLs: %w", err) @@ -1165,11 +1168,11 @@ func writeSpecificRevocationDeltaWALs(sc *storageContext, hyphenSerial string, c return nil } -func buildCRLs(sc *storageContext, forceNew bool) error { +func buildCRLs(sc *storageContext, forceNew bool) ([]string, error) { return buildAnyCRLs(sc, forceNew, false) } -func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error { +func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) ([]string, error) { // In order to build all CRLs, we need knowledge of all issuers. Any two // issuers with the same keys _and_ subject should have the same CRL since // they're functionally equivalent. @@ -1204,7 +1207,7 @@ func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error { // buildCRL. globalCRLConfig, err := sc.Backend.crlBuilder.getConfigWithUpdate(sc) if err != nil { - return fmt.Errorf("error building CRL: while updating config: %w", err) + return nil, fmt.Errorf("error building CRL: while updating config: %w", err) } if globalCRLConfig.Disable && !forceNew { @@ -1216,13 +1219,13 @@ func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error { // So, since tidy can now associate issuers on revocation entries, we // can skip the rest of this function and exit early without updating // anything. - return nil + return nil, nil } if !sc.Backend.useLegacyBundleCaStorage() { issuers, err = sc.listIssuers() if err != nil { - return fmt.Errorf("error building CRL: while listing issuers: %w", err) + return nil, fmt.Errorf("error building CRL: while listing issuers: %w", err) } } else { // Here, we hard-code the legacy issuer entry instead of using the @@ -1236,13 +1239,13 @@ func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error { // Users should upgrade symmetrically, rather than attempting // backward compatibility for new features across disparate versions. if isDelta { - return nil + return []string{"refusing to rebuild delta CRL with legacy bundle; finish migrating to newer issuer storage layout"}, nil } } issuersConfig, err := sc.getIssuersConfig() if err != nil { - return fmt.Errorf("error building CRLs: while getting the default config: %w", err) + return nil, fmt.Errorf("error building CRLs: while getting the default config: %w", err) } // We map issuerID->entry for fast lookup and also issuerID->Cert for @@ -1259,7 +1262,7 @@ func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error { // legacy path is automatically ignored. thisEntry, _, err := sc.fetchCertBundleByIssuerId(issuer, false) if err != nil { - return fmt.Errorf("error building CRLs: unable to fetch specified issuer (%v): %w", issuer, err) + return nil, fmt.Errorf("error building CRLs: unable to fetch specified issuer (%v): %w", issuer, err) } if len(thisEntry.KeyID) == 0 { @@ -1284,7 +1287,7 @@ func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error { thisCert, err := thisEntry.GetCertificate() if err != nil { - return fmt.Errorf("error building CRLs: unable to parse issuer (%v)'s certificate: %w", issuer, err) + return nil, fmt.Errorf("error building CRLs: unable to parse issuer (%v)'s certificate: %w", issuer, err) } issuerIDCertMap[issuer] = thisCert @@ -1299,19 +1302,27 @@ func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error { // Now we do two calls: building the cluster-local CRL, and potentially // building the global CRL if we're on the active node of the performance // primary. - currLocalDeltaSerials, err := buildAnyLocalCRLs(sc, issuersConfig, globalCRLConfig, + currLocalDeltaSerials, localWarnings, err := buildAnyLocalCRLs(sc, issuersConfig, globalCRLConfig, issuers, issuerIDEntryMap, issuerIDCertMap, keySubjectIssuersMap, wasLegacy, forceNew, isDelta) if err != nil { - return err + return nil, err } - currUnifiedDeltaSerials, err := buildAnyUnifiedCRLs(sc, issuersConfig, globalCRLConfig, + currUnifiedDeltaSerials, unifiedWarnings, err := buildAnyUnifiedCRLs(sc, issuersConfig, globalCRLConfig, issuers, issuerIDEntryMap, issuerIDCertMap, keySubjectIssuersMap, wasLegacy, forceNew, isDelta) if err != nil { - return err + return nil, err + } + + var warnings []string + for _, warning := range localWarnings { + warnings = append(warnings, fmt.Sprintf("warning from local CRL rebuild: %v", warning)) + } + for _, warning := range unifiedWarnings { + warnings = append(warnings, fmt.Sprintf("warning from unified CRL rebuild: %v", warning)) } // Finally, we decide if we need to rebuild the Delta CRLs again, for both @@ -1320,17 +1331,21 @@ func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error { // After we've confirmed the primary CRLs have built OK, go ahead and // clear the delta CRL WAL and rebuild it. if err := sc.Backend.crlBuilder.clearLocalDeltaWAL(sc, currLocalDeltaSerials); err != nil { - return fmt.Errorf("error building CRLs: unable to clear Delta WAL: %w", err) + return nil, fmt.Errorf("error building CRLs: unable to clear Delta WAL: %w", err) } if err := sc.Backend.crlBuilder.clearUnifiedDeltaWAL(sc, currUnifiedDeltaSerials); err != nil { - return fmt.Errorf("error building CRLs: unable to clear Delta WAL: %w", err) + return nil, fmt.Errorf("error building CRLs: unable to clear Delta WAL: %w", err) } - if err := sc.Backend.crlBuilder.rebuildDeltaCRLsHoldingLock(sc, forceNew); err != nil { - return fmt.Errorf("error building CRLs: unable to rebuild empty Delta WAL: %w", err) + deltaWarnings, err := sc.Backend.crlBuilder.rebuildDeltaCRLsHoldingLock(sc, forceNew) + if err != nil { + return nil, fmt.Errorf("error building CRLs: unable to rebuild empty Delta WAL: %w", err) + } + for _, warning := range deltaWarnings { + warnings = append(warnings, fmt.Sprintf("warning from delta CRL rebuild: %v", warning)) } } - return nil + return warnings, nil } func getLastWALSerial(sc *storageContext, path string) (string, error) { @@ -1363,8 +1378,9 @@ func buildAnyLocalCRLs( wasLegacy bool, forceNew bool, isDelta bool, -) ([]string, error) { +) ([]string, []string, error) { var err error + var warnings []string // Before we load cert entries, we want to store the last seen delta WAL // serial number. The subsequent List will have at LEAST that certificate @@ -1375,7 +1391,7 @@ func buildAnyLocalCRLs( if isDelta { lastDeltaSerial, err = getLastWALSerial(sc, localDeltaWALLastRevokedSerial) if err != nil { - return nil, err + return nil, nil, err } } @@ -1386,7 +1402,7 @@ func buildAnyLocalCRLs( if !isDelta { currDeltaCerts, err = sc.Backend.crlBuilder.getPresentLocalDeltaWALForClearing(sc) if err != nil { - return nil, fmt.Errorf("error building CRLs: unable to get present delta WAL entries for removal: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to get present delta WAL entries for removal: %w", err) } } @@ -1401,7 +1417,7 @@ func buildAnyLocalCRLs( // a separate pool for those. unassignedCerts, revokedCertsMap, err = getLocalRevokedCertEntries(sc, issuerIDCertMap, isDelta) if err != nil { - return nil, fmt.Errorf("error building CRLs: unable to get revoked certificate entries: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to get revoked certificate entries: %w", err) } if !isDelta { @@ -1414,7 +1430,7 @@ func buildAnyLocalCRLs( // duplicate this serial number on the delta, hence the above // guard for isDelta. if err := augmentWithRevokedIssuers(issuerIDEntryMap, issuerIDCertMap, revokedCertsMap); err != nil { - return nil, fmt.Errorf("error building CRLs: unable to parse revoked issuers: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to parse revoked issuers: %w", err) } } } @@ -1423,21 +1439,25 @@ func buildAnyLocalCRLs( // CRLs. internalCRLConfig, err := sc.getLocalCRLConfig() if err != nil { - return nil, fmt.Errorf("error building CRLs: unable to fetch cluster-local CRL configuration: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to fetch cluster-local CRL configuration: %w", err) } - if err := buildAnyCRLsWithCerts(sc, issuersConfig, globalCRLConfig, internalCRLConfig, + rebuildWarnings, err := buildAnyCRLsWithCerts(sc, issuersConfig, globalCRLConfig, internalCRLConfig, issuers, issuerIDEntryMap, keySubjectIssuersMap, unassignedCerts, revokedCertsMap, - forceNew, false /* isUnified */, isDelta); err != nil { - return nil, fmt.Errorf("error building CRLs: %w", err) + forceNew, false /* isUnified */, isDelta) + if err != nil { + return nil, nil, fmt.Errorf("error building CRLs: %w", err) + } + if len(rebuildWarnings) > 0 { + warnings = append(warnings, rebuildWarnings...) } // Finally, persist our potentially updated local CRL config. Only do this // if we didn't have a legacy CRL bundle. if !wasLegacy { if err := sc.setLocalCRLConfig(internalCRLConfig); err != nil { - return nil, fmt.Errorf("error building CRLs: unable to persist updated cluster-local CRL config: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to persist updated cluster-local CRL config: %w", err) } } @@ -1453,17 +1473,17 @@ func buildAnyLocalCRLs( lastDeltaBuildEntry, err := logical.StorageEntryJSON(localDeltaWALLastBuildSerial, deltaInfo) if err != nil { - return nil, fmt.Errorf("error creating last delta CRL rebuild serial entry: %w", err) + return nil, nil, fmt.Errorf("error creating last delta CRL rebuild serial entry: %w", err) } err = sc.Storage.Put(sc.Context, lastDeltaBuildEntry) if err != nil { - return nil, fmt.Errorf("error persisting last delta CRL rebuild info: %w", err) + return nil, nil, fmt.Errorf("error persisting last delta CRL rebuild info: %w", err) } } } - return currDeltaCerts, nil + return currDeltaCerts, warnings, nil } func buildAnyUnifiedCRLs( @@ -1477,19 +1497,20 @@ func buildAnyUnifiedCRLs( wasLegacy bool, forceNew bool, isDelta bool, -) ([]string, error) { +) ([]string, []string, error) { var err error + var warnings []string // Unified CRL can only be built by the main cluster. b := sc.Backend if b.System().ReplicationState().HasState(consts.ReplicationDRSecondary|consts.ReplicationPerformanceStandby) || (!b.System().LocalMount() && b.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary)) { - return nil, nil + return nil, nil, nil } // Unified CRL should only be built if enabled. if !globalCRLConfig.UnifiedCRL && !forceNew { - return nil, nil + return nil, nil, nil } // Before we load cert entries, we want to store the last seen delta WAL @@ -1503,14 +1524,14 @@ func buildAnyUnifiedCRLs( if isDelta { clusters, err := sc.Storage.List(sc.Context, unifiedDeltaWALPrefix) if err != nil { - return nil, fmt.Errorf("error listing clusters for unified delta WAL building: %w", err) + return nil, nil, fmt.Errorf("error listing clusters for unified delta WAL building: %w", err) } for index, cluster := range clusters { path := unifiedDeltaWALPrefix + cluster + deltaWALLastRevokedSerialName serial, err := getLastWALSerial(sc, path) if err != nil { - return nil, fmt.Errorf("error getting last written Delta WAL serial for cluster (%v / %v): %w", index, cluster, err) + return nil, nil, fmt.Errorf("error getting last written Delta WAL serial for cluster (%v / %v): %w", index, cluster, err) } lastDeltaSerial[cluster] = serial @@ -1524,7 +1545,7 @@ func buildAnyUnifiedCRLs( if !isDelta { currDeltaCerts, err = sc.Backend.crlBuilder.getPresentUnifiedDeltaWALForClearing(sc) if err != nil { - return nil, fmt.Errorf("error building CRLs: unable to get present delta WAL entries for removal: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to get present delta WAL entries for removal: %w", err) } } @@ -1539,7 +1560,7 @@ func buildAnyUnifiedCRLs( // a separate pool for those. unassignedCerts, revokedCertsMap, err = getUnifiedRevokedCertEntries(sc, issuerIDCertMap, isDelta) if err != nil { - return nil, fmt.Errorf("error building CRLs: unable to get revoked certificate entries: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to get revoked certificate entries: %w", err) } if !isDelta { @@ -1552,7 +1573,7 @@ func buildAnyUnifiedCRLs( // duplicate this serial number on the delta, hence the above // guard for isDelta. if err := augmentWithRevokedIssuers(issuerIDEntryMap, issuerIDCertMap, revokedCertsMap); err != nil { - return nil, fmt.Errorf("error building CRLs: unable to parse revoked issuers: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to parse revoked issuers: %w", err) } } } @@ -1561,21 +1582,25 @@ func buildAnyUnifiedCRLs( // CRLs. internalCRLConfig, err := sc.getUnifiedCRLConfig() if err != nil { - return nil, fmt.Errorf("error building CRLs: unable to fetch cluster-local CRL configuration: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to fetch cluster-local CRL configuration: %w", err) } - if err := buildAnyCRLsWithCerts(sc, issuersConfig, globalCRLConfig, internalCRLConfig, + rebuildWarnings, err := buildAnyCRLsWithCerts(sc, issuersConfig, globalCRLConfig, internalCRLConfig, issuers, issuerIDEntryMap, keySubjectIssuersMap, unassignedCerts, revokedCertsMap, - forceNew, true /* isUnified */, isDelta); err != nil { - return nil, fmt.Errorf("error building CRLs: %w", err) + forceNew, true /* isUnified */, isDelta) + if err != nil { + return nil, nil, fmt.Errorf("error building CRLs: %w", err) + } + if len(rebuildWarnings) > 0 { + warnings = append(warnings, rebuildWarnings...) } // Finally, persist our potentially updated local CRL config. Only do this // if we didn't have a legacy CRL bundle. if !wasLegacy { if err := sc.setUnifiedCRLConfig(internalCRLConfig); err != nil { - return nil, fmt.Errorf("error building CRLs: unable to persist updated cluster-local CRL config: %w", err) + return nil, nil, fmt.Errorf("error building CRLs: unable to persist updated cluster-local CRL config: %w", err) } } @@ -1599,17 +1624,17 @@ func buildAnyUnifiedCRLs( deltaInfo := lastDeltaInfo{Serial: serial} lastDeltaBuildEntry, err := logical.StorageEntryJSON(path, deltaInfo) if err != nil { - return nil, fmt.Errorf("error creating last delta CRL rebuild serial entry: %w", err) + return nil, nil, fmt.Errorf("error creating last delta CRL rebuild serial entry: %w", err) } err = sc.Storage.Put(sc.Context, lastDeltaBuildEntry) if err != nil { - return nil, fmt.Errorf("error persisting last delta CRL rebuild info: %w", err) + return nil, nil, fmt.Errorf("error persisting last delta CRL rebuild info: %w", err) } } } - return currDeltaCerts, nil + return currDeltaCerts, warnings, nil } func buildAnyCRLsWithCerts( @@ -1625,9 +1650,10 @@ func buildAnyCRLsWithCerts( forceNew bool, isUnified bool, isDelta bool, -) error { +) ([]string, error) { // Now we can call buildCRL once, on an arbitrary/representative issuer // from each of these (keyID, subject) sets. + var warnings []string for _, subjectIssuersMap := range keySubjectIssuersMap { for _, issuersSet := range subjectIssuersMap { if len(issuersSet) == 0 { @@ -1675,7 +1701,7 @@ func buildAnyCRLsWithCerts( // Finally, check our crlIdentifier. if thisCRLId, ok := internalCRLConfig.IssuerIDCRLMap[issuerId]; ok && len(thisCRLId) > 0 { if len(crlIdentifier) > 0 && crlIdentifier != thisCRLId { - return fmt.Errorf("error building CRLs: two issuers with same keys/subjects (%v vs %v) have different internal CRL IDs: %v vs %v", issuerId, crlIdIssuer, thisCRLId, crlIdentifier) + return nil, fmt.Errorf("error building CRLs: two issuers with same keys/subjects (%v vs %v) have different internal CRL IDs: %v vs %v", issuerId, crlIdIssuer, thisCRLId, crlIdentifier) } crlIdentifier = thisCRLId @@ -1687,6 +1713,24 @@ func buildAnyCRLsWithCerts( // Skip this set for the time being; while we have valid // issuers and associated keys, this occurred because we lack // crl-signing usage on all issuers in this set. + // + // But, tell the user about this, so they can either correct + // this by reissuing the CA certificate or adding an equivalent + // version with KU bits if the CA cert lacks KU altogether. + // + // See also: https://github.com/hashicorp/vault/issues/20137 + warning := "Issuer equivalency set with associated keys lacked an issuer with CRL Signing KeyUsage; refusing to rebuild CRL for this group of issuers: " + var issuers []string + for _, issuerId := range issuersSet { + issuers = append(issuers, issuerId.String()) + } + warning += strings.Join(issuers, ",") + + // We only need this warning once. :-) + if !isUnified && !isDelta { + warnings = append(warnings, warning) + } + continue } @@ -1727,7 +1771,7 @@ func buildAnyCRLsWithCerts( // Lastly, build the CRL. nextUpdate, err := buildCRL(sc, globalCRLConfig, forceNew, representative, revokedCerts, crlIdentifier, crlNumber, isUnified, isDelta, lastCompleteNumber) if err != nil { - return fmt.Errorf("error building CRLs: unable to build CRL for issuer (%v): %w", representative, err) + return nil, fmt.Errorf("error building CRLs: unable to build CRL for issuer (%v): %w", representative, err) } internalCRLConfig.CRLExpirationMap[crlIdentifier] = *nextUpdate @@ -1776,13 +1820,13 @@ func buildAnyCRLsWithCerts( if !stillHaveIssuerForID { if err := sc.Storage.Delete(sc.Context, "crls/"+crlId.String()); err != nil { - return fmt.Errorf("error building CRLs: unable to clean up deleted issuers' CRL: %w", err) + return nil, fmt.Errorf("error building CRLs: unable to clean up deleted issuers' CRL: %w", err) } } } // All good :-) - return nil + return warnings, nil } func isRevInfoIssuerValid(revInfo *revocationInfo, issuerIDCertMap map[issuerID]*x509.Certificate) bool { diff --git a/builtin/logical/pki/path_config_crl.go b/builtin/logical/pki/path_config_crl.go index e787eba28..0249e6f08 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -421,6 +421,8 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra b.crlBuilder.markConfigDirty() b.crlBuilder.reloadConfigIfRequired(sc) + resp := genResponseFromCrlConfig(config) + // Note this only affects/happens on the main cluster node, if you need to // notify something based on a configuration change on all server types // have a look at crlBuilder::reloadConfigIfRequired @@ -429,7 +431,7 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra // auto-rebuild and we aren't now or equivalently, we changed our // mind about delta CRLs and need a new complete one or equivalently, // we changed our mind about unified CRLs), rotate the CRLs. - crlErr := b.crlBuilder.rebuild(sc, true) + warnings, crlErr := b.crlBuilder.rebuild(sc, true) if crlErr != nil { switch crlErr.(type) { case errutil.UserError: @@ -438,9 +440,12 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra return nil, fmt.Errorf("error encountered during CRL building: %w", crlErr) } } + for index, warning := range warnings { + resp.AddWarning(fmt.Sprintf("Warning %d during CRL rebuild: %v", index+1, warning)) + } } - return genResponseFromCrlConfig(config), nil + return resp, nil } func genResponseFromCrlConfig(config *crlConfig) *logical.Response { diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index 3be342fd6..ed20b2122 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/builtin/logical/pki/path_fetch_issuers.go @@ -1192,9 +1192,20 @@ func (b *backend) pathGetIssuerCRL(ctx context.Context, req *logical.Request, da } sc := b.makeStorageContext(ctx, req.Storage) - if err := b.crlBuilder.rebuildIfForced(sc); err != nil { + warnings, err := b.crlBuilder.rebuildIfForced(sc) + if err != nil { return nil, err } + if len(warnings) > 0 { + // Since this is a fetch of a specific CRL, this most likely comes + // from an automated system of some sort; these warnings would be + // ignored and likely meaningless. Log them instead. + msg := "During rebuild of CRL on issuer CRL fetch, got the following warnings:" + for index, warning := range warnings { + msg = fmt.Sprintf("%v\n %d. %v", msg, index+1, warning) + } + b.Logger().Warn(msg) + } var certificate []byte var contentType string diff --git a/builtin/logical/pki/path_manage_issuers.go b/builtin/logical/pki/path_manage_issuers.go index 4d74393a6..285c254f2 100644 --- a/builtin/logical/pki/path_manage_issuers.go +++ b/builtin/logical/pki/path_manage_issuers.go @@ -388,7 +388,7 @@ func (b *backend) pathImportIssuers(ctx context.Context, req *logical.Request, d } if len(createdIssuers) > 0 { - err := b.crlBuilder.rebuild(sc, true) + warnings, err := b.crlBuilder.rebuild(sc, true) if err != nil { // Before returning, check if the error message includes the // string "PSS". If so, it indicates we might've wanted to modify @@ -403,6 +403,9 @@ func (b *backend) pathImportIssuers(ctx context.Context, req *logical.Request, d return nil, err } + for index, warning := range warnings { + response.AddWarning(fmt.Sprintf("Warning %d during CRL rebuild: %v", index+1, warning)) + } var issuersWithKeys []string for _, issuer := range createdIssuers { @@ -709,7 +712,7 @@ func (b *backend) pathRevokeIssuer(ctx context.Context, req *logical.Request, da } // Rebuild the CRL to include the newly revoked issuer. - crlErr := b.crlBuilder.rebuild(sc, false) + warnings, crlErr := b.crlBuilder.rebuild(sc, false) if crlErr != nil { switch crlErr.(type) { case errutil.UserError: @@ -725,6 +728,9 @@ func (b *backend) pathRevokeIssuer(ctx context.Context, req *logical.Request, da // Impossible. return nil, err } + for index, warning := range warnings { + response.AddWarning(fmt.Sprintf("Warning %d during CRL rebuild: %v", index+1, warning)) + } // For sanity, we'll add a warning message here if there's no other // issuer which verifies this issuer. diff --git a/builtin/logical/pki/path_revoke.go b/builtin/logical/pki/path_revoke.go index 154367fcb..27b4d5b8f 100644 --- a/builtin/logical/pki/path_revoke.go +++ b/builtin/logical/pki/path_revoke.go @@ -651,7 +651,7 @@ func (b *backend) pathRotateCRLRead(ctx context.Context, req *logical.Request, _ defer b.revokeStorageLock.RUnlock() sc := b.makeStorageContext(ctx, req.Storage) - crlErr := b.crlBuilder.rebuild(sc, false) + warnings, crlErr := b.crlBuilder.rebuild(sc, false) if crlErr != nil { switch crlErr.(type) { case errutil.UserError: @@ -661,11 +661,17 @@ func (b *backend) pathRotateCRLRead(ctx context.Context, req *logical.Request, _ } } - return &logical.Response{ + resp := &logical.Response{ Data: map[string]interface{}{ "success": true, }, - }, nil + } + + for index, warning := range warnings { + resp.AddWarning(fmt.Sprintf("Warning %d during CRL rebuild: %v", index+1, warning)) + } + + return resp, nil } func (b *backend) pathRotateDeltaCRLRead(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) { @@ -678,7 +684,7 @@ func (b *backend) pathRotateDeltaCRLRead(ctx context.Context, req *logical.Reque isEnabled := cfg.EnableDelta - crlErr := b.crlBuilder.rebuildDeltaCRLsIfForced(sc, true) + warnings, crlErr := b.crlBuilder.rebuildDeltaCRLsIfForced(sc, true) if crlErr != nil { switch crlErr.(type) { case errutil.UserError: @@ -697,6 +703,9 @@ func (b *backend) pathRotateDeltaCRLRead(ctx context.Context, req *logical.Reque if !isEnabled { resp.AddWarning("requested rebuild of delta CRL when delta CRL is not enabled; this is a no-op") } + for index, warning := range warnings { + resp.AddWarning(fmt.Sprintf("Warning %d during CRL rebuild: %v", index+1, warning)) + } return resp, nil } diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index d46663dd6..fc5476bef 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -297,10 +297,13 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, b.ifCountEnabledIncrementTotalCertificatesCount(certsCounted, key) // Build a fresh CRL - err = b.crlBuilder.rebuild(sc, true) + warnings, err = b.crlBuilder.rebuild(sc, true) if err != nil { return nil, err } + for index, warning := range warnings { + resp.AddWarning(fmt.Sprintf("Warning %d during CRL rebuild: %v", index+1, warning)) + } if parsedBundle.Certificate.MaxPathLen == 0 { resp.AddWarning("Max path length of the generated certificate is zero. This certificate cannot be used to issue intermediate CA certificates.") diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index d32c78a45..8748ccca0 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/builtin/logical/pki/path_tidy.go @@ -977,9 +977,17 @@ func (b *backend) doTidyRevocationStore(ctx context.Context, req *logical.Reques } if !config.AutoRebuild { - if err := b.crlBuilder.rebuild(sc, false); err != nil { + warnings, err := b.crlBuilder.rebuild(sc, false) + if err != nil { return err } + if len(warnings) > 0 { + msg := "During rebuild of CRL for tidy, got the following warnings:" + for index, warning := range warnings { + msg = fmt.Sprintf("%v\n %d. %v", msg, index+1, warning) + } + b.Logger().Warn(msg) + } } } @@ -1081,9 +1089,17 @@ func (b *backend) doTidyExpiredIssuers(ctx context.Context, req *logical.Request b.revokeStorageLock.Lock() defer b.revokeStorageLock.Unlock() - if err := b.crlBuilder.rebuild(sc, false); err != nil { + warnings, err := b.crlBuilder.rebuild(sc, false) + if err != nil { return err } + if len(warnings) > 0 { + msg := "During rebuild of CRL for tidy, got the following warnings:" + for index, warning := range warnings { + msg = fmt.Sprintf("%v\n %d. %v", msg, index+1, warning) + } + b.Logger().Warn(msg) + } } return nil diff --git a/changelog/20253.txt b/changelog/20253.txt new file mode 100644 index 000000000..19edae1bc --- /dev/null +++ b/changelog/20253.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Add warning when issuer lacks KeyUsage during CRL rebuilds; expose in logs and on rotation. +```