Skip to content

Commit

Permalink
Cleanup changes around issuer revocation (#16874)
Browse files Browse the repository at this point in the history 
* Refactor CRL tests to use /sys/mounts

Thanks Steve for the approach! This also address nits from Kit.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Skip CRL building steps when disabled

This skips a number of steps during CRL build when it is disabled (and
forceNew is not set). In particular, we avoid fetching issuers, we avoid
associating issuers with revocation entries (and building that in-memory
mapping), making CRL building more efficient.

This means that there'll again be very little overhead on clusters with
the CRL disabled.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Prevent revoking roots from appearing on own CRLs

This change ensures that when marking a root as revoked, it no longer
appears on its own CRL. Very few clients support this event (as
generally only leaves/intermediates are checked for presence on a
parent's CRL) and it is technically undefined behavior (if the root is
revoked, its own CRL should be untrusted and thus including it on its
own CRL isn't a safe/correct distribution channel).

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Ensure stability of revInfo issuer identification

As mentioned by Kit, iterating through each revInfoEntry and associating
the first issuer which matches it can cause churn when many (equivalent)
issuers are in the system and issuers come and go (via CRLSigning usage,
which has been modified in this release as well). Because we'd not
include issuers without CRLSigning usage, we'd cause our verification
helper, isRevInfoIssuerValid, to think the issuer ID is no longer value
(when instead, it just lacks crlSigning bits).

We address this by pulling in all issuers we know of for the
identification. This allows us to keep valid-but-not-for-signing
issuers, and use other representatives of their identity set for
signing/building the CRL (if they are enabled for such usage).

As a side effect, we now no longer place these entries on the default
CRL in the event all issuers in the CRL set are without the usage.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

This is only for the last commit.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
  • Loading branch information
@cipherboy
cipherboy committed Aug 25, 2022
1 parent 247a019 commit f06a6f7
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 44 deletions.
4 changes: 2 additions & 2 deletions builtin/logical/pki/chain_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand Down
60 changes: 24 additions & 36 deletions builtin/logical/pki/crl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
97 changes: 91 additions & 6 deletions builtin/logical/pki/crl_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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...)
Expand All @@ -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)
Expand All @@ -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()
Expand Down Expand Up @@ -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)
Expand All @@ -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{
Expand Down
3 changes: 3 additions & 0 deletions changelog/16874.txt
View file
Original file line number Diff line number Diff line change
@@ -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.
```

0 comments on commit f06a6f7

Please sign in to comment.