diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index db365a4f8..ca56605d9 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -1908,7 +1908,7 @@ func TestBackend_PathFetchValidRaw(t *testing.T) { issueCrtAsPem := resp.Data["certificate"].(string) issuedCrt := parseCert(t, issueCrtAsPem) - expectedSerial := certutil.GetHexFormatted(issuedCrt.SerialNumber.Bytes(), ":") + expectedSerial := serialFromCert(issuedCrt) expectedCert := []byte(issueCrtAsPem) // get der cert diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go index 428777e41..88046de8e 100644 --- a/builtin/logical/pki/crl_test.go +++ b/builtin/logical/pki/crl_test.go @@ -303,6 +303,123 @@ func TestCrlRebuilder(t *testing.T) { "initial crl time: %#v not before next crl rebuild time: %#v", crl1.ThisUpdate, crl3.ThisUpdate) } +func TestBYOC(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"]) + oldRoot := resp.Data["certificate"].(string) + + // 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": "75s", + "no_store": "true", + }) + 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["certificate"]) + + _, err = CBWrite(b, s, "revoke", map[string]interface{}{ + "certificate": resp.Data["certificate"], + }) + require.NoError(t, err) + + // Issue a second leaf, but hold onto it for now. + resp, err = CBWrite(b, s, "issue/local-testing", map[string]interface{}{ + "common_name": "testing2", + }) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotEmpty(t, resp.Data["certificate"]) + notStoredCert := resp.Data["certificate"].(string) + + // Update the role to make things stored and issue another cert. + _, err = CBWrite(b, s, "roles/stored-testing", map[string]interface{}{ + "allow_any_name": true, + "enforce_hostnames": false, + "key_type": "ec", + "ttl": "75s", + "no_store": "false", + }) + require.NoError(t, err) + + // Issue a leaf cert and ensure we can revoke it. + resp, err = CBWrite(b, s, "issue/stored-testing", map[string]interface{}{ + "common_name": "testing", + }) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotEmpty(t, resp.Data["certificate"]) + storedCert := resp.Data["certificate"].(string) + + // Delete the root and regenerate a new one. + _, err = CBDelete(b, s, "issuer/default") + require.NoError(t, err) + + resp, err = CBList(b, s, "issuers") + require.NoError(t, err) + require.Equal(t, len(resp.Data), 0) + + _, 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) + + // Issue a new leaf and revoke that one. + resp, err = CBWrite(b, s, "issue/local-testing", map[string]interface{}{ + "common_name": "testing3", + }) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotEmpty(t, resp.Data["certificate"]) + + _, err = CBWrite(b, s, "revoke", map[string]interface{}{ + "certificate": resp.Data["certificate"], + }) + require.NoError(t, err) + + // Now attempt to revoke the earlier leaves. The first should fail since + // we deleted its issuer, but the stored one should succeed. + _, err = CBWrite(b, s, "revoke", map[string]interface{}{ + "certificate": notStoredCert, + }) + require.Error(t, err) + + _, err = CBWrite(b, s, "revoke", map[string]interface{}{ + "certificate": storedCert, + }) + require.NoError(t, err) + + // Import the old root again and revoke the no stored leaf should work. + _, err = CBWrite(b, s, "issuers/import/bundle", map[string]interface{}{ + "pem_bundle": oldRoot, + }) + require.NoError(t, err) + + _, err = CBWrite(b, s, "revoke", map[string]interface{}{ + "certificate": notStoredCert, + }) + require.NoError(t, err) +} + func requestCrlFromBackend(t *testing.T, s logical.Storage, b *backend) *logical.Response { crlReq := &logical.Request{ Operation: logical.ReadOperation, diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index ae8e2f875..8273ff45d 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -15,7 +15,6 @@ import ( "github.com/hashicorp/vault/sdk/helper/consts" - "github.com/hashicorp/vault/sdk/helper/certutil" "github.com/hashicorp/vault/sdk/helper/errutil" "github.com/hashicorp/vault/sdk/logical" ) @@ -151,7 +150,7 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st } colonSerial := strings.ReplaceAll(strings.ToLower(serial), "-", ":") - if colonSerial == certutil.GetHexFormatted(parsedBundle.Certificate.SerialNumber.Bytes(), ":") { + if colonSerial == serialFromCert(parsedBundle.Certificate) { return logical.ErrorResponse(fmt.Sprintf("adding issuer (id: %v) to its own CRL is not allowed", issuer)), nil } } diff --git a/builtin/logical/pki/path_revoke.go b/builtin/logical/pki/path_revoke.go index fa061e4d4..cb40c4f8e 100644 --- a/builtin/logical/pki/path_revoke.go +++ b/builtin/logical/pki/path_revoke.go @@ -2,6 +2,8 @@ package pki import ( "context" + "crypto/x509" + "encoding/pem" "fmt" "strings" @@ -20,6 +22,11 @@ func pathRevoke(b *backend) *framework.Path { Description: `Certificate serial number, in colon- or hyphen-separated octal`, }, + "certificate": { + Type: framework.TypeString, + Description: `Certificate to revoke in PEM format; must be +signed by an issuer in this mount.`, + }, }, Operations: map[logical.Operation]framework.OperationHandler{ @@ -56,12 +63,165 @@ func pathRotateCRL(b *backend) *framework.Path { } } +func (b *backend) pathRevokeWriteHandleCertificate(ctx context.Context, req *logical.Request, data *framework.FieldData, certPem string) (string, []byte, error) { + // This function handles just the verification of the certificate against + // the global issuer set, checking whether or not it is importable. + // + // We return the parsed serial number, an optionally-nil byte array to + // write out to disk, and an error if one occurred. + if b.useLegacyBundleCaStorage() { + // We require listing all issuers from the 1.11 method. If we're + // still using the legacy CA bundle but with the newer certificate + // attribute, we err and require the operator to upgrade and migrate + // prior to servicing new requests. + return "", nil, errutil.UserError{Err: "unable to process BYOC revocation until CA issuer migration has completed"} + } + + // First start by parsing the certificate. + if len(certPem) < 75 { + // See note in pathImportIssuers about this check. + return "", nil, errutil.UserError{Err: "provided certificate data was too short; perhaps a path was passed to the API rather than the contents of a PEM file"} + } + + pemBlock, _ := pem.Decode([]byte(certPem)) + if pemBlock == nil { + return "", nil, errutil.UserError{Err: "certificate contains no PEM data"} + } + + certReference, err := x509.ParseCertificate(pemBlock.Bytes) + if err != nil { + return "", nil, errutil.UserError{Err: fmt.Sprintf("certificate could not be parsed: %v", err)} + } + + // Ensure we have a well-formed serial number before continuing. + serial := serialFromCert(certReference) + if len(serial) == 0 { + return "", nil, errutil.UserError{Err: fmt.Sprintf("invalid serial number on presented certificate")} + } + + // We have two approaches here: we could start verifying against issuers + // (which involves fetching and parsing them), or we could see if, by + // some chance we've already imported it (cheap). The latter tells us + // if we happen to have a serial number collision (which shouldn't + // happen in practice) versus an already-imported cert (which might + // happen and its fine to handle safely). + // + // Start with the latter since its cheaper. Fetch the cert (by serial) + // and if it exists, compare the contents. + certEntry, err := fetchCertBySerial(ctx, b, req, req.Path, serial) + if err != nil { + return serial, nil, err + } + + if certEntry != nil { + // As seen with importing issuers, it is best to parse the certificate + // and compare parsed values, rather than attempting to infer equality + // from the raw data. + certReferenceStored, err := x509.ParseCertificate(certEntry.Value) + if err != nil { + return serial, nil, err + } + + if !areCertificatesEqual(certReference, certReferenceStored) { + // Here we refuse the import with an error because the two certs + // are unequal but we would've otherwise overwritten the existing + // copy. + return serial, nil, fmt.Errorf("certificate with same serial but unequal value already present in this cluster's storage; refusing to revoke") + } else { + // Otherwise, we can return without an error as we've already + // imported this certificate, likely when we issued it. We don't + // need to re-verify the signature as we assume it was already + // verified when it was imported. + return serial, nil, nil + } + } + + // Otherwise, we must not have a stored copy. From here on out, the second + // parameter (except in error cases) should be the copy to write out. + // + // Fetch and iterate through each issuer. + sc := b.makeStorageContext(ctx, req.Storage) + issuers, err := sc.listIssuers() + if err != nil { + return serial, nil, err + } + + foundMatchingIssuer := false + for _, issuerId := range issuers { + issuer, err := sc.fetchIssuerById(issuerId) + if err != nil { + return serial, nil, err + } + + issuerCert, err := issuer.GetCertificate() + if err != nil { + return serial, nil, err + } + + if err := certReference.CheckSignatureFrom(issuerCert); err == nil { + // If the signature was valid, we found our match and can safely + // exit. + foundMatchingIssuer = true + break + } + } + + if foundMatchingIssuer { + return serial, certReference.Raw, nil + } + + return serial, nil, errutil.UserError{Err: fmt.Sprintf("unable to verify signature on presented cert from any present issuer in this mount; certificates from previous CAs will need to have their issuing CA and key re-imported if revocation is necessary")} +} + func (b *backend) pathRevokeWrite(ctx context.Context, req *logical.Request, data *framework.FieldData, _ *roleEntry) (*logical.Response, error) { - serial := data.Get("serial_number").(string) + rawSerial, haveSerial := data.GetOk("serial_number") + rawCertificate, haveCert := data.GetOk("certificate") + + if !haveSerial && !haveCert { + return logical.ErrorResponse("The serial number or certificate to revoke must be provided."), nil + } else if haveSerial && haveCert { + return logical.ErrorResponse("Must provide either the certificate or the serial to revoke; not both."), nil + } + + var serial string + if haveSerial { + // Easy case: this cert should be in storage already. + serial = rawSerial.(string) + } else { + // Otherwise, we've gotta parse the certificate from the request and + // then import it into cluster-local storage. Before writing the + // certificate (and forwarding), we want to verify this certificate + // was actually signed by one of our present issuers. + var err error + var certBytes []byte + serial, certBytes, err = b.pathRevokeWriteHandleCertificate(ctx, req, data, rawCertificate.(string)) + if err != nil { + return nil, err + } + + // At this point, a forward operation will occur if we're on a standby + // node as we're now attempting to write the bytes of the cert out to + // disk. + if certBytes != nil { + err = req.Storage.Put(ctx, &logical.StorageEntry{ + Key: "certs/" + serial, + Value: certBytes, + }) + if err != nil { + return nil, err + } + } + + // Finally, we have a valid serial number to use for BYOC revocation! + } + if len(serial) == 0 { return logical.ErrorResponse("The serial number must be provided"), nil } + // Assumption: this check is cheap. Call this twice, in the cert-import + // case, to allow cert verification to get rejected on the standby node, + // but we still need it to protect the serial number case. if b.System().ReplicationState().HasState(consts.ReplicationPerformanceStandby) { return nil, logical.ErrReadOnly } diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index 3019a0951..dacd685d2 100644 --- a/builtin/logical/pki/storage.go +++ b/builtin/logical/pki/storage.go @@ -669,7 +669,7 @@ func (sc *storageContext) importIssuer(certValue string, issuerName string) (*is return nil, false, fmt.Errorf("bad issuer: potentially multiple PEM blobs in one certificate storage entry:\n%v", result.Certificate) } - result.SerialNumber = strings.TrimSpace(certutil.GetHexFormatted(issuerCert.SerialNumber.Bytes(), ":")) + result.SerialNumber = serialFromCert(issuerCert) // Before we return below, we need to iterate over _all_ keys and see if // one of them a public key matching this certificate, and if so, update our diff --git a/builtin/logical/pki/util.go b/builtin/logical/pki/util.go index 35887d858..9e774a43a 100644 --- a/builtin/logical/pki/util.go +++ b/builtin/logical/pki/util.go @@ -2,6 +2,7 @@ package pki import ( "crypto" + "crypto/x509" "fmt" "regexp" "strings" @@ -25,6 +26,10 @@ var ( errKeyNameInUse = errutil.UserError{Err: "key name already in use"} ) +func serialFromCert(cert *x509.Certificate) string { + return strings.TrimSpace(certutil.GetHexFormatted(cert.SerialNumber.Bytes(), ":")) +} + func normalizeSerial(serial string) string { return strings.ReplaceAll(strings.ToLower(serial), ":", "-") } diff --git a/changelog/16564.txt b/changelog/16564.txt new file mode 100644 index 000000000..90a5524d8 --- /dev/null +++ b/changelog/16564.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Allow revocation of certificates with explicitly provided certificate (bring your own certificate / BYOC). +``` diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index 38f1f260b..24f4cbfff 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -834,9 +834,16 @@ successful revocation will rotate the CRL. #### Parameters -- `serial_number` `(string: )` - Specifies the serial number of the +~> Note: either `serial_number` or `certificate` (but not both) must be + specified on requests to this endpoint. + +- `serial_number` `(string: )` - Specifies the serial number of the certificate to revoke, in hyphen-separated or colon-separated hexadecimal. +- `certificate` `(string: )` - Specifies the certificate to revoke, + in PEM format. This certificate must have been signed by one of the issuers + in this mount in order to be accepted for revocation. + #### Sample Payload ```json