From 319dc82bb8cc6b6d9a5d4493e25705403ed9bd4e Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Wed, 24 Aug 2022 12:23:27 -0400 Subject: [PATCH] Identify issuer on revocation (#16763) * Identify issuer on revocation When we attempt to revoke a leaf certificate, we already parse all of the issuers within the mount (to x509.Certificate) to ensure we don't accidentally revoke an issuer via the leaf revocation endpoint. We can reuse this information to associate the issuer (via issuer/subject comparison and signature checking) to the revoked cert in its revocation info. This will help OCSP, avoiding the case where the OCSP handler needs to associate a certificate to its issuer. Signed-off-by: Alexander Scheel * Add test to ensure issuers are identified Signed-off-by: Alexander Scheel Signed-off-by: Alexander Scheel --- builtin/logical/pki/crl_test.go | 61 +++++++++++++++++++++++++++++++-- builtin/logical/pki/crl_util.go | 37 +++++++++++++------- 2 files changed, 83 insertions(+), 15 deletions(-) diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go index 8ab23e7f0e64e..b1d8d75197c35 100644 --- a/builtin/logical/pki/crl_test.go +++ b/builtin/logical/pki/crl_test.go @@ -3,6 +3,7 @@ package pki import ( "context" "encoding/asn1" + "encoding/json" "fmt" "strings" "testing" @@ -806,6 +807,7 @@ func TestAutoRebuild(t *testing.T) { LogicalBackends: map[string]logical.Factory{ "pki": Factory, }, + EnableRaw: true, } oldPeriod := vault.SetRollbackPeriodForTesting(newPeriod) cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ @@ -827,12 +829,16 @@ func TestAutoRebuild(t *testing.T) { require.NoError(t, err) // Generate root. - _, err = client.Logical().Write("pki/root/generate/internal", map[string]interface{}{ + resp, err := client.Logical().Write("pki/root/generate/internal", map[string]interface{}{ "ttl": "40h", "common_name": "Root X1", "key_type": "ec", }) require.NoError(t, err) + require.NotNil(t, resp) + require.NotEmpty(t, resp.Data) + require.NotEmpty(t, resp.Data["issuer_id"]) + rootIssuer := resp.Data["issuer_id"].(string) // Setup a testing role. _, err = client.Logical().Write("pki/roles/local-testing", map[string]interface{}{ @@ -844,7 +850,7 @@ func TestAutoRebuild(t *testing.T) { // Regression test: ensure we respond with the default values for CRL // config when we haven't set any values yet. - resp, err := client.Logical().Read("pki/config/crl") + resp, err = client.Logical().Read("pki/config/crl") require.NoError(t, err) require.NotNil(t, resp) require.NotNil(t, resp.Data) @@ -904,6 +910,57 @@ func TestAutoRebuild(t *testing.T) { }) require.NoError(t, err) + // 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/") + 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.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. + resp, err = client.Logical().Read("sys/raw/" + revEntryPath) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotNil(t, resp.Data) + require.NotEmpty(t, resp.Data["value"]) + revEntryValue := resp.Data["value"].(string) + fmt.Println(resp) + var revInfo revocationInfo + err = json.Unmarshal([]byte(revEntryValue), &revInfo) + require.NoError(t, err) + require.Equal(t, revInfo.CertificateIssuer, issuerID(rootIssuer)) + // Serial should not appear on CRL. crl = getCrlCertificateList(t, client, "pki") thisCRLNumber := crl.Version diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index 305f1a3b960cd..dff978914fefc 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -253,6 +253,7 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st issuers = []issuerID{legacyBundleShimID} } + issuerIDCertMap := make(map[issuerID]*x509.Certificate, len(issuers)) for _, issuer := range issuers { _, bundle, caErr := sc.fetchCertBundleByIssuerId(issuer, false) if caErr != nil { @@ -281,6 +282,8 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st if colonSerial == serialFromCert(parsedBundle.Certificate) { return logical.ErrorResponse(fmt.Sprintf("adding issuer (id: %v) to its own CRL is not allowed", issuer)), nil } + + issuerIDCertMap[issuer] = parsedBundle.Certificate } alreadyRevoked := false @@ -352,6 +355,10 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st revInfo.RevocationTime = currTime.Unix() revInfo.RevocationTimeUTC = currTime.UTC() + // We may not find an issuer with this certificate; that's fine so + // ignore the return value. + associateRevokedCertWithIsssuer(&revInfo, cert, issuerIDCertMap) + revEntry, err = logical.StorageEntryJSON(revokedPath+normalizeSerial(serial), revInfo) if err != nil { return nil, fmt.Errorf("error creating revocation entry") @@ -626,6 +633,20 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b return nil } +func associateRevokedCertWithIsssuer(revInfo *revocationInfo, revokedCert *x509.Certificate, issuerIDCertMap map[issuerID]*x509.Certificate) bool { + for issuerId, issuerCert := range issuerIDCertMap { + if bytes.Equal(revokedCert.RawIssuer, issuerCert.RawSubject) { + if err := revokedCert.CheckSignatureFrom(issuerCert); err == nil { + // Valid mapping. Add it to the specified entry. + revInfo.CertificateIssuer = issuerId + return true + } + } + } + + return false +} + func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCertMap map[issuerID]*x509.Certificate) ([]pkix.RevokedCertificate, map[issuerID][]pkix.RevokedCertificate, error) { var unassignedCerts []pkix.RevokedCertificate revokedCertsMap := make(map[issuerID][]pkix.RevokedCertificate) @@ -687,23 +708,13 @@ func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCe } // Now we need to assign the revoked certificate to an issuer. - foundParent := false - for issuerId, issuerCert := range issuerIDCertMap { - if bytes.Equal(revokedCert.RawIssuer, issuerCert.RawSubject) { - if err := revokedCert.CheckSignatureFrom(issuerCert); err == nil { - // Valid mapping. Add it to the specified entry. - revokedCertsMap[issuerId] = append(revokedCertsMap[issuerId], newRevCert) - revInfo.CertificateIssuer = issuerId - foundParent = true - break - } - } - } - + foundParent := associateRevokedCertWithIsssuer(&revInfo, revokedCert, issuerIDCertMap) if !foundParent { // If the parent isn't found, add it to the unassigned bucket. unassignedCerts = append(unassignedCerts, newRevCert) } else { + revokedCertsMap[revInfo.CertificateIssuer] = append(revokedCertsMap[revInfo.CertificateIssuer], newRevCert) + // When the CertificateIssuer field wasn't found on the existing // entry (or was invalid), and we've found a new value for it, // we should update the entry to make future CRL builds faster.