Let PKI tidy associate revoked certs with their issuers

builtin/logical/pki/backend.go

@@ -220,9 +220,10 @@ const (
 
 type tidyStatus struct {
 	// Parameters used to initiate the operation
-	safetyBuffer     int
-	tidyCertStore    bool
-	tidyRevokedCerts bool
+	safetyBuffer      int
+	tidyCertStore     bool
+	tidyRevokedCerts  bool
+	tidyRevokedAssocs bool
 
 	// Status
 	state                   tidyStatusState
@@ -232,6 +233,7 @@ type tidyStatus struct {
 	message                 string
 	certStoreDeletedCount   uint
 	revokedCertDeletedCount uint
+	missingIssuerCertCount  uint
 }
 
 const backendHelp = `

builtin/logical/pki/backend_test.go

@@ -3873,16 +3873,18 @@ func TestBackend_RevokePlusTidy_Intermediate(t *testing.T) {
 			t.Fatal(err)
 		}
 		expectedData := map[string]interface{}{
-			"safety_buffer":              json.Number("1"),
-			"tidy_cert_store":            true,
-			"tidy_revoked_certs":         true,
-			"state":                      "Finished",
-			"error":                      nil,
-			"time_started":               nil,
-			"time_finished":              nil,
-			"message":                    nil,
-			"cert_store_deleted_count":   json.Number("1"),
-			"revoked_cert_deleted_count": json.Number("1"),
+			"safety_buffer":                         json.Number("1"),
+			"tidy_cert_store":                       true,
+			"tidy_revoked_certs":                    true,
+			"tidy_revoked_cert_issuer_associations": false,
+			"state":                                 "Finished",
+			"error":                                 nil,
+			"time_started":                          nil,
+			"time_finished":                         nil,
+			"message":                               nil,
+			"cert_store_deleted_count":              json.Number("1"),
+			"revoked_cert_deleted_count":            json.Number("1"),
+			"missing_issuer_cert_count":             json.Number("0"),
 		}
 		// Let's copy the times from the response so that we can use deep.Equal()
 		timeStarted, ok := tidyStatus.Data["time_started"]

builtin/logical/pki/crl_test.go

@@ -1052,6 +1052,145 @@ func TestAutoRebuild(t *testing.T) {
 	}
 }
 
+func TestTidyIssuerAssociation(t *testing.T) {
+	t.Parallel()
+
+	b, s := createBackendWithStorage(t)
+
+	// Create a root CA.
+	resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{
+		"common_name": "root example.com",
+		"issuer_name": "root",
+		"key_type":    "ec",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotEmpty(t, resp.Data["certificate"])
+	require.NotEmpty(t, resp.Data["issuer_id"])
+	rootCert := resp.Data["certificate"].(string)
+	rootID := resp.Data["issuer_id"].(issuerID)
+
+	// Create a role for issuance.
+	_, err = CBWrite(b, s, "roles/local-testing", map[string]interface{}{
+		"allow_any_name":    true,
+		"enforce_hostnames": false,
+		"key_type":          "ec",
+		"ttl":               "75m",
+	})
+	require.NoError(t, err)
+
+	// Issue a leaf cert and ensure we can revoke it.
+	resp, err = CBWrite(b, s, "issue/local-testing", map[string]interface{}{
+		"common_name": "testing",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotEmpty(t, resp.Data["serial_number"])
+	leafSerial := resp.Data["serial_number"].(string)
+
+	_, err = CBWrite(b, s, "revoke", map[string]interface{}{
+		"serial_number": leafSerial,
+	})
+	require.NoError(t, err)
+
+	// This leaf's revInfo entry should have an issuer associated
+	// with it.
+	entry, err := s.Get(ctx, revokedPath+normalizeSerial(leafSerial))
+	require.NoError(t, err)
+	require.NotNil(t, entry)
+	require.NotNil(t, entry.Value)
+
+	var leafInfo revocationInfo
+	err = entry.DecodeJSON(&leafInfo)
+	require.NoError(t, err)
+	require.Equal(t, rootID, leafInfo.CertificateIssuer)
+
+	// Now remove the root and run tidy.
+	_, err = CBDelete(b, s, "issuer/default")
+	require.NoError(t, err)
+	_, err = CBWrite(b, s, "tidy", map[string]interface{}{
+		"tidy_revoked_cert_issuer_associations": true,
+	})
+	require.NoError(t, err)
+
+	// Wait for tidy to finish.
+	for {
+		time.Sleep(125 * time.Millisecond)
+
+		resp, err = CBRead(b, s, "tidy-status")
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		require.NotNil(t, resp.Data)
+		require.NotEmpty(t, resp.Data["state"])
+		state := resp.Data["state"].(string)
+
+		if state == "Finished" {
+			break
+		}
+		if state == "Error" {
+			t.Fatalf("unexpected state for tidy operation: Error:\nStatus: %v", resp.Data)
+		}
+	}
+
+	// Ensure we don't have an association on this leaf any more.
+	entry, err = s.Get(ctx, revokedPath+normalizeSerial(leafSerial))
+	require.NoError(t, err)
+	require.NotNil(t, entry)
+	require.NotNil(t, entry.Value)
+
+	err = entry.DecodeJSON(&leafInfo)
+	require.NoError(t, err)
+	require.Empty(t, leafInfo.CertificateIssuer)
+
+	// Now, re-import the root and try again.
+	resp, err = CBWrite(b, s, "issuers/import/bundle", map[string]interface{}{
+		"pem_bundle": rootCert,
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotNil(t, resp.Data)
+	require.NotNil(t, resp.Data["imported_issuers"])
+	importedIssuers := resp.Data["imported_issuers"].([]string)
+	require.Equal(t, 1, len(importedIssuers))
+	newRootID := importedIssuers[0]
+	require.NotEmpty(t, newRootID)
+
+	// Re-run tidy...
+	_, err = CBWrite(b, s, "tidy", map[string]interface{}{
+		"tidy_revoked_cert_issuer_associations": true,
+	})
+	require.NoError(t, err)
+
+	// Wait for tidy to finish.
+	for {
+		time.Sleep(125 * time.Millisecond)
+
+		resp, err = CBRead(b, s, "tidy-status")
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		require.NotNil(t, resp.Data)
+		require.NotEmpty(t, resp.Data["state"])
+		state := resp.Data["state"].(string)
+
+		if state == "Finished" {
+			break
+		}
+		if state == "Error" {
+			t.Fatalf("unexpected state for tidy operation: Error:\nStatus: %v", resp.Data)
+		}
+	}
+
+	// Finally, double-check we associated things correctly.
+	entry, err = s.Get(ctx, revokedPath+normalizeSerial(leafSerial))
+	require.NoError(t, err)
+	require.NotNil(t, entry)
+	require.NotNil(t, entry.Value)
+
+	err = entry.DecodeJSON(&leafInfo)
+	require.NoError(t, err)
+	require.Equal(t, newRootID, string(leafInfo.CertificateIssuer))
+}
+
 func requestCrlFromBackend(t *testing.T, s logical.Storage, b *backend) *logical.Response {
 	crlReq := &logical.Request{
 		Operation: logical.ReadOperation,

builtin/logical/pki/crl_util.go

@@ -222,30 +222,17 @@ func (cb *crlBuilder) _doRebuild(ctx context.Context, b *backend, request *logic
 	return nil
 }
 
-// Revokes a cert, and tries to be smart about error recovery
-func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial string, fromLease bool) (*logical.Response, error) {
-	// As this backend is self-contained and this function does not hook into
-	// third parties to manage users or resources, if the mount is tainted,
-	// revocation doesn't matter anyways -- the CRL that would be written will
-	// be immediately blown away by the view being cleared. So we can simply
-	// fast path a successful exit.
-	if b.System().Tainted() {
-		return nil, nil
-	}
-
-	// Validate that no issuers match the serial number to be revoked. We need
-	// to gracefully degrade to the legacy cert bundle when it is required, as
-	// secondary PR clusters might not have been upgraded, but still need to
-	// handle revoking certs.
+// Helper function to fetch a map of issuerID->parsed cert for revocation
+// usage. Unlike other paths, this needs to handle the legacy bundle
+// more gracefully than rejecting it outright.
+func fetchIssuerMapForRevocationChecking(sc *storageContext) (map[issuerID]*x509.Certificate, error) {
 	var err error
 	var issuers []issuerID
 
-	sc := b.makeStorageContext(ctx, req.Storage)
-
-	if !b.useLegacyBundleCaStorage() {
+	if !sc.Backend.useLegacyBundleCaStorage() {
 		issuers, err = sc.listIssuers()
 		if err != nil {
-			return logical.ErrorResponse(fmt.Sprintf("could not fetch issuers list: %v", err)), nil
+			return nil, fmt.Errorf("could not fetch issuers list: %v", err)
 		}
 	} else {
 		// Hack: this isn't a real issuerID, but it works for fetchCAInfo
@@ -257,19 +244,14 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st
 	for _, issuer := range issuers {
 		_, bundle, caErr := sc.fetchCertBundleByIssuerId(issuer, false)
 		if caErr != nil {
-			switch caErr.(type) {
-			case errutil.UserError:
-				return logical.ErrorResponse(fmt.Sprintf("could not fetch the CA certificate for issuer id %v: %s", issuer, caErr)), nil
-			default:
-				return nil, fmt.Errorf("error fetching CA certificate for issuer id %v: %s", issuer, caErr)
-			}
+			return nil, fmt.Errorf("error fetching CA certificate for issuer id %v: %s", issuer, caErr)
 		}
 
 		if bundle == nil {
 			return nil, fmt.Errorf("faulty reference: %v - CA info not found", issuer)
 		}
 
-		parsedBundle, err := parseCABundle(ctx, b, bundle)
+		parsedBundle, err := parseCABundle(sc.Context, sc.Backend, bundle)
 		if err != nil {
 			return nil, errutil.InternalError{Err: err.Error()}
 		}
@@ -278,12 +260,41 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st
 			return nil, errutil.InternalError{Err: "stored CA information not able to be parsed"}
 		}
 
+		issuerIDCertMap[issuer] = parsedBundle.Certificate
+	}
+
+	return issuerIDCertMap, nil
+}
+
+// Revokes a cert, and tries to be smart about error recovery
+func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial string, fromLease bool) (*logical.Response, error) {
+	// As this backend is self-contained and this function does not hook into
+	// third parties to manage users or resources, if the mount is tainted,
+	// revocation doesn't matter anyways -- the CRL that would be written will
+	// be immediately blown away by the view being cleared. So we can simply
+	// fast path a successful exit.
+	if b.System().Tainted() {
+		return nil, nil
+	}
+
+	// Validate that no issuers match the serial number to be revoked. We need
+	// to gracefully degrade to the legacy cert bundle when it is required, as
+	// secondary PR clusters might not have been upgraded, but still need to
+	// handle revoking certs.
+	sc := b.makeStorageContext(ctx, req.Storage)
+
+	issuerIDCertMap, err := fetchIssuerMapForRevocationChecking(sc)
+	if err != nil {
+		return nil, err
+	}
+
+	// Ensure we don't revoke an issuer via this API; use /issuer/:issuer_ref/revoke
+	// instead.
+	for issuer, certificate := range issuerIDCertMap {
 		colonSerial := strings.ReplaceAll(strings.ToLower(serial), "-", ":")
-		if colonSerial == serialFromCert(parsedBundle.Certificate) {
+		if colonSerial == serialFromCert(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
@@ -683,6 +694,17 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
 	return nil
 }
 
+func isRevInfoIssuerValid(revInfo *revocationInfo, issuerIDCertMap map[issuerID]*x509.Certificate) bool {
+	if len(revInfo.CertificateIssuer) > 0 {
+		issuerId := revInfo.CertificateIssuer
+		if _, issuerExists := issuerIDCertMap[issuerId]; issuerExists {
+			return true
+		}
+	}
+
+	return false
+}
+
 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) {
@@ -782,12 +804,9 @@ func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCe
 		// prefer it to manually checking each issuer signature, assuming it
 		// appears valid. It's highly unlikely for two different issuers
 		// to have the same id (after the first was deleted).
-		if len(revInfo.CertificateIssuer) > 0 {
-			issuerId := revInfo.CertificateIssuer
-			if _, issuerExists := issuerIDCertMap[issuerId]; issuerExists {
-				revokedCertsMap[issuerId] = append(revokedCertsMap[issuerId], newRevCert)
-				continue
-			}
+		if isRevInfoIssuerValid(&revInfo, issuerIDCertMap) {
+			revokedCertsMap[revInfo.CertificateIssuer] = append(revokedCertsMap[revInfo.CertificateIssuer], newRevCert)
+			continue
 
 			// Otherwise, fall through and update the entry.
 		}