Allow marking issuers as revoked (#16621)

* Allow marking issuers as revoked

This allows PKI's issuers to be considered revoked and appear on each
others' CRLs. We disable issuance (via removing the usage) and prohibit
modifying the usage via the regular issuer management interface.

A separate endpoint is necessary because issuers (especially if signed
by a third-party CA using incremental serial numbers) might share a
serial number (e.g., an intermediate under cross-signing might share the
same number as an external root or an unrelated intermediate).

When the next CRL rebuild happens, this issuer will then appear on
others issuers CRLs, if they validate this issuer's certificate.

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

* Add changelog entry

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

* Add documentation on revoking issuers

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

* Add tests for issuer revocation semantics

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

* Notate that CRLs will be rebuilt

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

* Fix timestamp field from _utc -> to _rfc3339

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

* Ensure serial-based accesses shows as revoked

Thanks Kit!

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

* Add warning when revoking default issuer

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

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This commit is contained in:
Alexander Scheel 2022-08-18 18:08:31 -04:00 committed by GitHub
parent a0ba3202a8
commit 0c22c76907
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 491 additions and 11 deletions

View File

@ -141,6 +141,7 @@ func Backend(conf *logical.BackendConfig) *backend {
pathCrossSignIntermediate(&b),
pathConfigIssuers(&b),
pathReplaceRoot(&b),
pathRevokeIssuer(&b),
// Key APIs
pathListKeys(&b),

View File

@ -578,6 +578,147 @@ func TestPoP(t *testing.T) {
require.NoError(t, err)
}
func TestIssuerRevocation(t *testing.T) {
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["serial_number"])
// oldRoot := resp.Data["certificate"].(string)
oldRootSerial := resp.Data["serial_number"].(string)
// Create a second root CA. We'll revoke this one and ensure it
// doesn't appear on the former's CRL.
resp, err = CBWrite(b, s, "root/generate/internal", map[string]interface{}{
"common_name": "root2 example.com",
"issuer_name": "root2",
"key_type": "ec",
})
require.NoError(t, err)
require.NotNil(t, resp)
require.NotEmpty(t, resp.Data["certificate"])
require.NotEmpty(t, resp.Data["serial_number"])
// revokedRoot := resp.Data["certificate"].(string)
revokedRootSerial := resp.Data["serial_number"].(string)
// Shouldn't be able to revoke it by serial number.
_, err = CBWrite(b, s, "revoke", map[string]interface{}{
"serial_number": revokedRootSerial,
})
require.Error(t, err)
// Revoke it.
resp, err = CBWrite(b, s, "issuer/root2/revoke", map[string]interface{}{})
require.NoError(t, err)
require.NotNil(t, resp)
require.NotZero(t, resp.Data["revocation_time"])
// Regenerate the CRLs
_, err = CBRead(b, s, "crl/rotate")
require.NoError(t, err)
// Ensure the old cert isn't on the one's CRL.
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)
}
// Create a role and ensure we can't use the revoked root.
_, err = CBWrite(b, s, "roles/local-testing", map[string]interface{}{
"allow_any_name": true,
"enforce_hostnames": false,
"key_type": "ec",
"ttl": "75s",
})
require.NoError(t, err)
// Issue a leaf cert and ensure it fails (because the issuer is revoked).
_, err = CBWrite(b, s, "issuer/root2/issue/local-testing", map[string]interface{}{
"common_name": "testing",
})
require.Error(t, err)
// Issue an intermediate and ensure we can revoke it.
resp, err = CBWrite(b, s, "intermediate/generate/internal", map[string]interface{}{
"common_name": "intermediate example.com",
})
require.NoError(t, err)
require.NotNil(t, resp)
require.NotEmpty(t, resp.Data["csr"])
intCsr := resp.Data["csr"].(string)
resp, err = CBWrite(b, s, "root/sign-intermediate", map[string]interface{}{
"ttl": "30h",
"csr": intCsr,
})
require.NoError(t, err)
require.NotNil(t, resp)
require.NotEmpty(t, resp.Data["certificate"])
require.NotEmpty(t, resp.Data["serial_number"])
intCert := resp.Data["certificate"].(string)
intCertSerial := resp.Data["serial_number"].(string)
resp, err = CBWrite(b, s, "intermediate/set-signed", map[string]interface{}{
"certificate": intCert,
})
require.NoError(t, err)
require.NotNil(t, resp)
require.NotEmpty(t, resp.Data["imported_issuers"])
importedIssuers := resp.Data["imported_issuers"].([]string)
require.Equal(t, len(importedIssuers), 1)
intId := importedIssuers[0]
_, err = CBPatch(b, s, "issuer/"+intId, map[string]interface{}{
"issuer_name": "int1",
})
require.NoError(t, err)
// Now issue a leaf with the intermediate.
resp, err = CBWrite(b, s, "issuer/int1/issue/local-testing", map[string]interface{}{
"common_name": "testing",
})
require.NoError(t, err)
require.NotNil(t, resp)
require.NotEmpty(t, resp.Data["certificate"])
require.NotEmpty(t, resp.Data["serial_number"])
issuedSerial := resp.Data["serial_number"].(string)
// Now revoke the intermediate.
resp, err = CBWrite(b, s, "issuer/int1/revoke", map[string]interface{}{})
require.NoError(t, err)
require.NotNil(t, resp)
require.NotZero(t, resp.Data["revocation_time"])
// Update the CRLs and ensure it appears.
_, err = CBRead(b, s, "crl/rotate")
require.NoError(t, err)
crl = getParsedCrlFromBackend(t, b, s, "issuer/root/crl/der")
requireSerialNumberInCRL(t, crl.TBSCertList, intCertSerial)
// Ensure we can still revoke the issued leaf.
resp, err = CBWrite(b, s, "revoke", map[string]interface{}{
"serial_number": issuedSerial,
})
require.NoError(t, err)
require.NotNil(t, resp)
// Ensure it appears on the intermediate's CRL.
_, err = CBRead(b, s, "crl/rotate")
require.NoError(t, err)
crl = getParsedCrlFromBackend(t, b, s, "issuer/int1/crl/der")
requireSerialNumberInCRL(t, crl.TBSCertList, issuedSerial)
// Ensure we can't fetch the intermediate's cert by serial any more.
resp, err = CBRead(b, s, "cert/"+intCertSerial)
require.NoError(t, err)
require.NotNil(t, resp)
require.NotEmpty(t, resp.Data["revocation_time"])
}
func requestCrlFromBackend(t *testing.T, s logical.Storage, b *backend) *logical.Response {
crlReq := &logical.Request{
Operation: logical.ReadOperation,

View File

@ -364,6 +364,10 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
return fmt.Errorf("error building CRLs: unable to get revoked certificate entries: %v", err)
}
if err := augmentWithRevokedIssuers(issuerIDEntryMap, issuerIDCertMap, revokedCertsMap); err != nil {
return fmt.Errorf("error building CRLs: unable to parse revoked issuers: %v", err)
}
// Now we can call buildCRL once, on an arbitrary/representative issuer
// from each of these (keyID, subject) sets.
for _, subjectIssuersMap := range keySubjectIssuersMap {
@ -570,6 +574,40 @@ func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCe
return unassignedCerts, revokedCertsMap, nil
}
func augmentWithRevokedIssuers(issuerIDEntryMap map[issuerID]*issuerEntry, issuerIDCertMap map[issuerID]*x509.Certificate, revokedCertsMap map[issuerID][]pkix.RevokedCertificate) error {
// When setup our maps with the legacy CA bundle, we only have a
// single entry here. This entry is never revoked, so the outer loop
// will exit quickly.
for ourIssuerID, ourIssuer := range issuerIDEntryMap {
if !ourIssuer.Revoked {
continue
}
ourCert := issuerIDCertMap[ourIssuerID]
ourRevCert := pkix.RevokedCertificate{
SerialNumber: ourCert.SerialNumber,
RevocationTime: ourIssuer.RevocationTimeUTC,
}
for otherIssuerID := range issuerIDEntryMap {
if otherIssuerID == ourIssuerID {
continue
}
// Find all _other_ certificates which verify this issuer,
// allowing us to add this revoked issuer to this issuer's
// CRL.
otherCert := issuerIDCertMap[otherIssuerID]
if err := ourCert.CheckSignatureFrom(otherCert); err == nil {
// Valid signature; add our result.
revokedCertsMap[otherIssuerID] = append(revokedCertsMap[otherIssuerID], ourRevCert)
}
}
}
return nil
}
// Builds a CRL by going through the list of revoked certificates and building
// a new CRL with the stored revocation times and serial numbers.
func buildCRL(sc *storageContext, forceNew bool, thisIssuerId issuerID, revoked []pkix.RevokedCertificate, identifier crlID, crlNumber int64) error {

View File

@ -6,6 +6,7 @@ import (
"encoding/pem"
"fmt"
"strings"
"time"
"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/helper/certutil"
@ -196,18 +197,26 @@ func respondReadIssuer(issuer *issuerEntry) (*logical.Response, error) {
revSigAlgStr = ""
}
data := map[string]interface{}{
"issuer_id": issuer.ID,
"issuer_name": issuer.Name,
"key_id": issuer.KeyID,
"certificate": issuer.Certificate,
"manual_chain": respManualChain,
"ca_chain": issuer.CAChain,
"leaf_not_after_behavior": issuer.LeafNotAfterBehavior.String(),
"usage": issuer.Usage.Names(),
"revocation_signature_algorithm": revSigAlgStr,
"revoked": issuer.Revoked,
}
if issuer.Revoked {
data["revocation_time"] = issuer.RevocationTime
data["revocation_time_rfc3339"] = issuer.RevocationTimeUTC.Format(time.RFC3339Nano)
}
return &logical.Response{
Data: map[string]interface{}{
"issuer_id": issuer.ID,
"issuer_name": issuer.Name,
"key_id": issuer.KeyID,
"certificate": issuer.Certificate,
"manual_chain": respManualChain,
"ca_chain": issuer.CAChain,
"leaf_not_after_behavior": issuer.LeafNotAfterBehavior.String(),
"usage": issuer.Usage.Names(),
"revocation_signature_algorithm": revSigAlgStr,
},
Data: data,
}, nil
}
@ -308,6 +317,11 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da
}
if newUsage != issuer.Usage {
if issuer.Revoked && newUsage.HasUsage(IssuanceUsage) {
// Forbid allowing cert signing on its usage.
return logical.ErrorResponse(fmt.Sprintf("This issuer was revoked; unable to modify its usage to include certificate signing again. Reissue this certificate (preferably with a new key) and modify that entry instead.")), nil
}
issuer.Usage = newUsage
modified = true
}
@ -461,6 +475,11 @@ func (b *backend) pathPatchIssuer(ctx context.Context, req *logical.Request, dat
return logical.ErrorResponse(fmt.Sprintf("Unable to parse specified usages: %v - valid values are %v", rawUsage, AllIssuerUsages.Names())), nil
}
if newUsage != issuer.Usage {
if issuer.Revoked && newUsage.HasUsage(IssuanceUsage) {
// Forbid allowing cert signing on its usage.
return logical.ErrorResponse(fmt.Sprintf("This issuer was revoked; unable to modify its usage to include certificate signing again. Reissue this certificate (preferably with a new key) and modify that entry instead.")), nil
}
issuer.Usage = newUsage
modified = true
}

View File

@ -3,11 +3,14 @@ package pki
import (
"bytes"
"context"
"crypto/x509"
"encoding/pem"
"fmt"
"strings"
"time"
"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/helper/errutil"
"github.com/hashicorp/vault/sdk/logical"
)
@ -318,3 +321,227 @@ either take PEM-formatted certificates, and, if :type="bundle", unencrypted
secret-keys.
`
)
func pathRevokeIssuer(b *backend) *framework.Path {
fields := addIssuerRefField(map[string]*framework.FieldSchema{})
return &framework.Path{
Pattern: "issuer/" + framework.GenericNameRegex(issuerRefParam) + "/revoke",
Fields: fields,
Operations: map[logical.Operation]framework.OperationHandler{
logical.UpdateOperation: &framework.PathOperation{
Callback: b.pathRevokeIssuer,
// Read more about why these flags are set in backend.go
ForwardPerformanceStandby: true,
ForwardPerformanceSecondary: true,
},
},
HelpSynopsis: pathRevokeIssuerHelpSyn,
HelpDescription: pathRevokeIssuerHelpDesc,
}
}
func (b *backend) pathRevokeIssuer(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
// Since we're planning on updating issuers here, grab the lock so we've
// got a consistent view.
b.issuersLock.Lock()
defer b.issuersLock.Unlock()
// Issuer revocation can't work on the legacy cert bundle.
if b.useLegacyBundleCaStorage() {
return logical.ErrorResponse("cannot revoke issuer until migration has completed"), nil
}
issuerName := getIssuerRef(data)
if len(issuerName) == 0 {
return logical.ErrorResponse("missing issuer reference"), nil
}
// Fetch the issuer.
sc := b.makeStorageContext(ctx, req.Storage)
ref, err := sc.resolveIssuerReference(issuerName)
if err != nil {
return nil, err
}
if ref == "" {
return logical.ErrorResponse("unable to resolve issuer id for reference: " + issuerName), nil
}
issuer, err := sc.fetchIssuerById(ref)
if err != nil {
return nil, err
}
// If its already been revoked, just return the read results sans warnings
// like we would otherwise.
if issuer.Revoked {
return respondReadIssuer(issuer)
}
// When revoking, we want to forbid new certificate issuance. We allow
// new revocations of leaves issued by this issuer to trigger a CRL
// rebuild still.
issuer.Revoked = true
if issuer.Usage.HasUsage(IssuanceUsage) {
issuer.Usage.ToggleUsage(IssuanceUsage)
}
currTime := time.Now()
issuer.RevocationTime = currTime.Unix()
issuer.RevocationTimeUTC = currTime.UTC()
err = sc.writeIssuer(issuer)
if err != nil {
return nil, err
}
// Now, if the parent issuer exists within this mount, we'd have written
// a storage entry for this certificate, making it appear as any other
// leaf. We need to add a revocationInfo entry for this into storage,
// so that it appears as if it was revoked.
//
// This is a _necessary_ but not necessarily _sufficient_ step to
// consider an arbitrary issuer revoked and the former step (setting
// issuer.Revoked = true) is more correct: if two intermediates have the
// same serial number, and one appears somehow in the storage but from a
// different issuer, we'd only include one in the CRLs, but we'd want to
// include both in two separate CRLs. Hence, the former is the condition
// we check in CRL building, but this step satisfies other guarantees
// within Vault.
certEntry, err := fetchCertBySerial(ctx, b, req, "certs/", issuer.SerialNumber)
if err == nil && certEntry != nil {
// We've inverted this error check as it doesn't matter; we already
// consider this certificate revoked.
storageCert, err := x509.ParseCertificate(certEntry.Value)
if err != nil {
return nil, fmt.Errorf("error parsing stored certificate value: %v", err)
}
issuerCert, err := issuer.GetCertificate()
if err != nil {
return nil, fmt.Errorf("error parsing issuer certificate value: %v", err)
}
if bytes.Equal(issuerCert.Raw, storageCert.Raw) {
// If the issuer is on disk at its serial number is the same as
// our issuer, we know we can write the revocation entry. Since
// Vault has historically forbid revocation of non-stored certs
// and issuers, we're the only ones to write this entry, so we
// don't need the write guard that exists in crl_util.go for the
// general case (forbidding a newer revocation time).
//
// We'll let a cleanup pass or CRL build identify the issuer for
// us.
revInfo := revocationInfo{
CertificateBytes: issuerCert.Raw,
RevocationTime: issuer.RevocationTime,
RevocationTimeUTC: issuer.RevocationTimeUTC,
}
revEntry, err := logical.StorageEntryJSON(revokedPath+normalizeSerial(issuer.SerialNumber), revInfo)
if err != nil {
return nil, fmt.Errorf("error creating revocation entry for issuer: %v", err)
}
err = req.Storage.Put(ctx, revEntry)
if err != nil {
return nil, fmt.Errorf("error saving revoked issuer to new location: %v", err)
}
}
}
// Rebuild the CRL to include the newly revoked issuer.
crlErr := b.crlBuilder.rebuild(ctx, b, req, false)
if crlErr != nil {
switch crlErr.(type) {
case errutil.UserError:
return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", crlErr)), nil
default:
return nil, fmt.Errorf("error encountered during CRL building: %w", crlErr)
}
}
// Finally, respond with the issuer's updated data.
response, err := respondReadIssuer(issuer)
if err != nil {
// Impossible.
return nil, err
}
// For sanity, we'll add a warning message here if there's no other
// issuer which verifies this issuer.
ourCert, err := issuer.GetCertificate()
if err != nil {
return nil, err
}
allIssuers, err := sc.listIssuers()
if err != nil {
return nil, err
}
isSelfSigned := false
haveOtherIssuer := false
for _, candidateID := range allIssuers {
candidate, err := sc.fetchIssuerById(candidateID)
if err != nil {
return nil, err
}
candidateCert, err := candidate.GetCertificate()
if err != nil {
// Returning this error is fine because more things will fail
// if this issuer can't parse.
return nil, err
}
if err := ourCert.CheckSignatureFrom(candidateCert); err == nil {
// Signature verification is a success. This means we have a
// parent for this cert. But notice above we didn't filter out
// ourselves: we want to see if this is a self-signed cert. So
// check that now.
if candidate.ID == issuer.ID {
isSelfSigned = true
} else {
haveOtherIssuer = true
}
}
// If we have both possible warning candidates, no sense continuing
// to check signatures; exit.
if isSelfSigned && haveOtherIssuer {
break
}
}
if isSelfSigned {
response.AddWarning("This issuer is a self-signed (potentially root) certificate. This means it may not be considered revoked if there is not an external, cross-signed variant of this certificate. This issuer's serial number will not appear on its own CRL.")
}
if !haveOtherIssuer {
response.AddWarning("This issuer lacks another parent issuer within the mount. This means it will not appear on any other CRLs and may not be considered revoked by clients. Consider adding this issuer to its issuer's CRL as well if it is not self-signed.")
}
config, err := sc.getIssuersConfig()
if err == nil && config != nil && config.DefaultIssuerId == issuer.ID {
response.AddWarning("This issuer is currently configured as the default issuer for this mount; operations such as certificate issuance may not work until a new default issuer is selected.")
}
return response, nil
}
const (
pathRevokeIssuerHelpSyn = `Revoke the specified issuer certificate.`
pathRevokeIssuerHelpDesc = `
This endpoint allows revoking the specified issuer certificates.
This is useful when the issuer and its parent exist within the same PKI
mount point (utilizing the multi-issuer functionality). If no suitable
parent is found, this revocation may not appear on any CRL in this mount.
Once revoked, issuers cannot be unrevoked and may not be used to sign any
more certificates.
`
)

View File

@ -7,6 +7,7 @@ import (
"crypto/x509"
"fmt"
"strings"
"time"
"github.com/hashicorp/go-uuid"
"github.com/hashicorp/vault/sdk/helper/certutil"
@ -146,6 +147,9 @@ type issuerEntry struct {
LeafNotAfterBehavior certutil.NotAfterBehavior `json:"not_after_behavior"`
Usage issuerUsage `json:"usage"`
RevocationSigAlg x509.SignatureAlgorithm `json:"revocation_signature_algorithm"`
Revoked bool `json:"revoked"`
RevocationTime int64 `json:"revocation_time"`
RevocationTimeUTC time.Time `json:"revocation_time_utc"`
}
type localCRLConfigEntry struct {

2
changelog/16621.txt Normal file
View File

@ -0,0 +1,2 @@
```release-note:improvement
secrets/pki: Allow revocation of issuers within the same mount.

View File

@ -45,6 +45,7 @@ update your API calls accordingly.
- [Import CA Certificates and Keys](#import-ca-certificates-and-keys)
- [Read Issuer](#read-issuer)
- [Update Issuer](#update-issuer)
- [Revoke Issuer](#revoke-issuer)
- [Delete Issuer](#delete-issuer)
- [Import Key](#import-key)
- [Read Key](#read-key)
@ -2032,6 +2033,52 @@ $ curl \
}
```
### Revoke Issuer
This endpoint allows an operator to revoke an issuer certificate, marking it
unable to issue new certificates and adding it to other issuers' CRLs, if they
have signed this issuer's certificate. This will cause all CRLs to be rebuilt.
~> **Warning**: This operation cannot be undone!
| Method | Path |
| :------ | :------------------------------- |
| `POST` | `/pki/issuer/:issuer_ref/revoke` |
#### Parameters
No parameters.
#### Sample Request
```shell-session
$ curl \
--header "X-Vault-Token: ..." \
--request POST \
http://127.0.0.1:8200/v1/pki/issuer/old-intermediate/revoke
```
#### Sample Response
```text
{
"data": {
"ca_chain": [
"-----BEGIN CERTIFICATE-----\nMIIDFDCCAfygAwIBAgIUXgxy54mKooz5soqQoRINazH/3pQwDQYJKoZIhvcNAQEL\n...",
"-----BEGIN CERTIFICATE-----\nMIIDFTCCAf2gAwIBAgIUUo/qwLm5AyqUWqFHw1MlgwUtS/kwDQYJKoZIhvcNAQEL\n..."
],
"certificate": "-----BEGIN CERTIFICATE-----\nMIIDFDCCAfygAwIBAgIUXgxy54mKooz5soqQoRINazH/3pQwDQYJKoZIhvcNAQEL\n...",
"issuer_id": "7545992c-1910-0898-9e64-d575549fbe9c",
"issuer_name": "old-intermediate",
"key_id": "baadd98d-ec5a-66ac-06b7-dfc91c02c9cf",
"leaf_not_after_behavior": "truncate",
"manual_chain": null,
"usage": "read-only,issuing-certificates,crl-signing"
"revocation_time": 1433269787,
}
}
```
### Delete Issuer
This endpoint deletes the specified issuer. A warning is emitted and the

View File

@ -445,6 +445,7 @@ For these personas, we suggest the following ACLs, in condensed, tabular form:
| `/config/keys` | Read, Write | Yes | | | | |
| `/config/urls` | Read, Write | Yes | | | | |
| `/issuer/:issuer_ref` | Write | Yes | | | | |
| `/issuer/:issuer_ref/revoke` | Write | Yes | | | | |
| `/issuer/:issuer_ref/sign-intermediate` | Write | Yes | | | | |
| `/issuer/issuer_ref/sign-self-issued` | Write | Yes | | | | |
| `/issuers/generate/+/+` | Write | Yes | | | | |