diff --git a/builtin/logical/pki/chain_test.go b/builtin/logical/pki/chain_test.go index 63f4622bb4..746a378cba 100644 --- a/builtin/logical/pki/chain_test.go +++ b/builtin/logical/pki/chain_test.go @@ -535,7 +535,7 @@ func (c CBIssueLeaf) RevokeLeaf(t testing.TB, b *backend, s logical.Storage, kno t.Fatalf("failed to revoke issued certificate (%v) under role %v / issuer %v: expected response parameter revocation_time was missing from response:\n%v", api_serial, c.Role, c.Issuer, resp.Data) } - if !hasCRL && isDefault { + if !hasCRL { // Nothing further we can test here. We could re-enable CRL building // and check that it works, but that seems like a stretch. Other // issuers might be functionally the same as this issuer (and thus @@ -614,7 +614,7 @@ func (c CBIssueLeaf) RevokeLeaf(t testing.TB, b *backend, s logical.Storage, kno } } - t.Fatalf("expected to find certificate with serial [%v] on issuer %v's CRL but was missing: %v revoked certs\n\nCRL:\n[%v]\n\nLeaf:\n[%v]\n\nIssuer:\n[%v]\n", api_serial, c.Issuer, len(crl.TBSCertList.RevokedCertificates), raw_crl, raw_cert, raw_issuer) + t.Fatalf("expected to find certificate with serial [%v] on issuer %v's CRL but was missing: %v revoked certs\n\nCRL:\n[%v]\n\nLeaf:\n[%v]\n\nIssuer (hasCRL: %v):\n[%v]\n", api_serial, c.Issuer, len(crl.TBSCertList.RevokedCertificates), raw_crl, raw_cert, hasCRL, raw_issuer) } } diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go index b1d8d75197..141b1f4670 100644 --- a/builtin/logical/pki/crl_test.go +++ b/builtin/logical/pki/crl_test.go @@ -694,8 +694,14 @@ func TestIssuerRevocation(t *testing.T) { _, err = CBRead(b, s, "crl/rotate") require.NoError(t, err) + // Ensure the old cert isn't on its own CRL. + crl := getParsedCrlFromBackend(t, b, s, "issuer/root2/crl/der") + if requireSerialNumberInCRL(nil, crl.TBSCertList, revokedRootSerial) { + t.Fatalf("the serial number %v should not be on its own CRL as self-CRL appearance should not occur", revokedRootSerial) + } + // Ensure the old cert isn't on the one's CRL. - crl := getParsedCrlFromBackend(t, b, s, "issuer/root/crl/der") + crl = getParsedCrlFromBackend(t, b, s, "issuer/root/crl/der") if requireSerialNumberInCRL(nil, crl.TBSCertList, revokedRootSerial) { t.Fatalf("the serial number %v should not be on %v's CRL as they're separate roots", revokedRootSerial, oldRootSerial) } @@ -807,6 +813,8 @@ func TestAutoRebuild(t *testing.T) { LogicalBackends: map[string]logical.Factory{ "pki": Factory, }, + // See notes below about usage of /sys/raw for reading cluster + // storage without barrier encryption. EnableRaw: true, } oldPeriod := vault.SetRollbackPeriodForTesting(newPeriod) @@ -912,43 +920,23 @@ func TestAutoRebuild(t *testing.T) { // Now, we want to test the issuer identification on revocation. This // only happens as a distinct "step" when CRL building isn't done on - // each reovcation. Pull the storage from the cluster and verify the - // revInfo contains a matching cert. Some of this code is cribbed from - // kvv2_upgrade_test.go. - var pkiMount string - storage := cluster.Cores[0].UnderlyingStorage - mounts, err := storage.List(ctx, "logical/") + // each revocation. Pull the storage from the cluster (via the sys/raw + // endpoint which requires the mount UUID) and verify the revInfo contains + // a matching issuer. + resp, err = client.Logical().Read("sys/mounts/pki") require.NoError(t, err) - require.NotEmpty(t, mounts) - for _, mount := range mounts { - // For whatever reason, OIDC gets provisioned as the first mount, - // but I'm not convinced that's a stable list. Let's look inside - // each mount until we find a revoked certs folder that we'd expect - // if its a real PKI mount. This is because mounts are just UUID - // strings... - mountFolders, err := storage.List(ctx, "logical/"+mount) - require.NoError(t, err) - - isPkiMount := false - for _, folder := range mountFolders { - if folder == "revoked/" { - isPkiMount = true - break - } - } - - if isPkiMount { - pkiMount = mount - break - } - } + require.NotNil(t, resp) + require.NotNil(t, resp.Data) + require.NotEmpty(t, resp.Data["uuid"]) + pkiMount := resp.Data["uuid"].(string) require.NotEmpty(t, pkiMount) - revEntryPath := "logical/" + pkiMount + revokedPath + strings.ReplaceAll(newLeafSerial, ":", "-") - // storage above is a physical storage copy, not a logical storage. This - // difference means, if we were to do a storage.Get(...) on the above - // path, we'd read the barrier-encrypted value. This is less than useful - // for decoding, and fetching the proper storage view is a touch much - // work. So, assert EnableRaw above and (ab)use it here. + revEntryPath := "logical/" + pkiMount + "/" + revokedPath + strings.ReplaceAll(newLeafSerial, ":", "-") + + // storage from cluster.Core[0] is a physical storage copy, not a logical + // storage. This difference means, if we were to do a storage.Get(...) + // on the above path, we'd read the barrier-encrypted value. This is less + // than useful for decoding, and fetching the proper storage view is a + // touch much work. So, assert EnableRaw above and (ab)use it here. resp, err = client.Logical().Read("sys/raw/" + revEntryPath) require.NoError(t, err) require.NotNil(t, resp) diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index dff978914f..2b6f61aecc 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -440,6 +440,18 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b return fmt.Errorf("error building CRL: while updating config: %v", err) } + if globalCRLConfig.Disable && !forceNew { + // We build a single long-lived empty CRL in the event that we disable + // the CRL, but we don't keep updating it with newer, more-valid empty + // CRLs in the event that we later re-enable it. This is a historical + // behavior. + // + // 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 + } + if !b.useLegacyBundleCaStorage() { issuers, err = sc.listIssuers() if err != nil { @@ -479,11 +491,20 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b continue } - // Skip entries which aren't enabled for CRL signing. - if err := thisEntry.EnsureUsage(CRLSigningUsage); err != nil { - continue - } - + // n.b.: issuer usage check has been delayed. This occurred because + // we want to ensure any issuer (representative of a larger set) can + // be used to associate revocation entries and we won't bother + // rewriting that entry (causing churn) if the particular selected + // issuer lacks CRL signing capabilities. + // + // The result is that this map (and the other maps) contain all the + // issuers we know about, and only later do we check crlSigning before + // choosing our representative. + // + // The other side effect (making this not compatible with Vault 1.11 + // behavior) is that _identified_ certificates whose issuer set is + // not allowed for crlSigning will no longer appear on the default + // issuer's CRL. issuerIDEntryMap[issuer] = thisEntry thisCert, err := thisEntry.GetCertificate() @@ -529,10 +550,24 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b } var revokedCerts []pkix.RevokedCertificate - representative := issuersSet[0] + representative := issuerID("") var crlIdentifier crlID var crlIdIssuer issuerID for _, issuerId := range issuersSet { + // Skip entries which aren't enabled for CRL signing. We don't + // particularly care which issuer is ultimately chosen as the + // set representative for signing at this point, other than + // that it has crl-signing usage. + if err := issuerIDEntryMap[issuerId].EnsureUsage(CRLSigningUsage); err != nil { + continue + } + + // Prefer to use the default as the representative of this + // set, if it is a member. + // + // If it is, we'll also pull in the unassigned certs to remain + // compatible with Vault's earlier, potentially questionable + // behavior. if issuerId == config.DefaultIssuerId { if len(unassignedCerts) > 0 { revokedCerts = append(revokedCerts, unassignedCerts...) @@ -541,10 +576,18 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b representative = issuerId } + // Otherwise, use any other random issuer if we've not yet + // chosen one. + if representative == issuerID("") { + representative = issuerId + } + + // Pull in the revoked certs associated with this member. if thisRevoked, ok := revokedCertsMap[issuerId]; ok && len(thisRevoked) > 0 { revokedCerts = append(revokedCerts, thisRevoked...) } + // Finally, check our crlIdentifier. if thisCRLId, ok := crlConfig.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) @@ -555,6 +598,13 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b } } + if representative == "" { + // 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. + continue + } + if len(crlIdentifier) == 0 { // Create a new random UUID for this CRL if none exists. crlIdentifier = genCRLId() @@ -656,6 +706,13 @@ func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCe return nil, nil, errutil.InternalError{Err: fmt.Sprintf("error fetching list of revoked certs: %s", err)} } + // Build a mapping of issuer serial -> certificate. + issuerSerialCertMap := make(map[string][]*x509.Certificate, len(issuerIDCertMap)) + for _, cert := range issuerIDCertMap { + serialStr := serialFromCert(cert) + issuerSerialCertMap[serialStr] = append(issuerSerialCertMap[serialStr], cert) + } + for _, serial := range revokedSerials { var revInfo revocationInfo revokedEntry, err := req.Storage.Get(ctx, revokedPath+serial) @@ -682,6 +739,34 @@ func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCe return nil, nil, errutil.InternalError{Err: fmt.Sprintf("unable to parse stored revoked certificate with serial %s: %s", serial, err)} } + // We want to skip issuer certificate's revocationEntries for two + // reasons: + // + // 1. We canonically use augmentWithRevokedIssuers to handle this + // case and this entry is just a backup. This prevents the issue + // of duplicate serial numbers on the CRL from both paths. + // 2. We want to avoid a root's serial from appearing on its own + // CRL. If it is a cross-signed or re-issued variant, this is OK, + // but in the case we mark the root itself as "revoked", we want + // to avoid it appearing on the CRL as that is definitely + // undefined/little-supported behavior. + // + // This hash map lookup should be faster than byte comparison against + // each issuer proactively. + if candidates, present := issuerSerialCertMap[serialFromCert(revokedCert)]; present { + revokedCertIsIssuer := false + for _, candidate := range candidates { + if bytes.Equal(candidate.Raw, revokedCert.Raw) { + revokedCertIsIssuer = true + break + } + } + + if revokedCertIsIssuer { + continue + } + } + // NOTE: We have to change this to UTC time because the CRL standard // mandates it but Go will happily encode the CRL without this. newRevCert := pkix.RevokedCertificate{ diff --git a/changelog/16874.txt b/changelog/16874.txt new file mode 100644 index 0000000000..f1dafa0173 --- /dev/null +++ b/changelog/16874.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Improve stability of association of revoked cert with its parent issuer; when an issuer loses crl-signing usage, do not place certs on default issuer's CRL. +```