Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Identify issuer on revocation #16763

Merged
merged 2 commits into from Aug 24, 2022
Merged

Identify issuer on revocation #16763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93da855
Identify issuer on revocation
cipherboy Aug 17, 2022
cd923f9
Add test to ensure issuers are identified
cipherboy Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 59 additions & 2 deletions builtin/logical/pki/crl_test.go
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@ package pki
import (
"context"
"encoding/asn1"
"encoding/json"
"fmt"
"strings"
"testing"
Expand Down Expand Up @@ -806,6 +807,7 @@ func TestAutoRebuild(t *testing.T) {
LogicalBackends: map[string]logical.Factory{
"pki": Factory,
},
EnableRaw: true,
cipherboy marked this conversation as resolved.
Show resolved Hide resolved
}
oldPeriod := vault.SetRollbackPeriodForTesting(newPeriod)
cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
Expand All @@ -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{}{
Expand All @@ -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)
Expand Down Expand Up @@ -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
cipherboy marked this conversation as resolved.
Show resolved Hide resolved
// 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
cipherboy marked this conversation as resolved.
Show resolved Hide resolved
// 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
Expand Down
37 changes: 24 additions & 13 deletions builtin/logical/pki/crl_util.go
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Would be great to add a comment here that this isn't stable - that if multiple issuers could have possibly issued this certificate, it's picking the first one in the map.

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)
Expand Down Expand Up @@ -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.
Expand Down