diff --git a/builtin/logical/pki/acme_state.go b/builtin/logical/pki/acme_state.go index 66746fb7b8..89084facec 100644 --- a/builtin/logical/pki/acme_state.go +++ b/builtin/logical/pki/acme_state.go @@ -519,6 +519,54 @@ func (a *acmeState) ListOrderIds(ac *acmeContext, accountId string) ([]string, e return orderIds, nil } +type acmeCertEntry struct { + Serial string `json:"-"` + Account string `json:"-"` + Order string `json:"order"` +} + +func (a *acmeState) TrackIssuedCert(ac *acmeContext, accountId string, serial string, orderId string) error { + path := acmeAccountPrefix + accountId + "/certs/" + normalizeSerial(serial) + entry := acmeCertEntry{ + Order: orderId, + } + + json, err := logical.StorageEntryJSON(path, &entry) + if err != nil { + return fmt.Errorf("error serializing acme cert entry: %w", err) + } + + if err = ac.sc.Storage.Put(ac.sc.Context, json); err != nil { + return fmt.Errorf("error writing acme cert entry: %w", err) + } + + return nil +} + +func (a *acmeState) GetIssuedCert(ac *acmeContext, accountId string, serial string) (*acmeCertEntry, error) { + path := acmeAccountPrefix + accountId + "/certs/" + normalizeSerial(serial) + + entry, err := ac.sc.Storage.Get(ac.sc.Context, path) + if err != nil { + return nil, fmt.Errorf("error loading acme cert entry: %w", err) + } + + if entry == nil { + return nil, fmt.Errorf("no certificate with this serial was issued for this account") + } + + var cert acmeCertEntry + err = entry.DecodeJSON(&cert) + if err != nil { + return nil, fmt.Errorf("error decoding acme cert entry: %w", err) + } + + cert.Serial = denormalizeSerial(serial) + cert.Account = accountId + + return &cert, nil +} + func getAuthorizationPath(accountId string, authId string) string { return acmeAccountPrefix + accountId + "/authorizations/" + authId } diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index e99635501d..9cc2e7995f 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -243,6 +243,7 @@ func Backend(conf *logical.BackendConfig) *backend { acmePaths = append(acmePaths, pathAcmeFetchOrderCert(&b)...) acmePaths = append(acmePaths, pathAcmeChallenge(&b)...) acmePaths = append(acmePaths, pathAcmeAuthorization(&b)...) + acmePaths = append(acmePaths, pathAcmeRevoke(&b)...) for _, acmePath := range acmePaths { b.Backend.Paths = append(b.Backend.Paths, acmePath) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index f2cb753d21..78ed4e3328 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -6816,6 +6816,7 @@ func TestProperAuthing(t *testing.T) { paths[acmePrefix+"acme/directory"] = shouldBeUnauthedReadList paths[acmePrefix+"acme/new-nonce"] = shouldBeUnauthedReadList paths[acmePrefix+"acme/new-account"] = shouldBeUnauthedWriteOnly + paths[acmePrefix+"acme/revoke-cert"] = shouldBeUnauthedWriteOnly paths[acmePrefix+"acme/new-order"] = shouldBeUnauthedWriteOnly paths[acmePrefix+"acme/orders"] = shouldBeUnauthedWriteOnly paths[acmePrefix+"acme/account/hrKmDYTvicHoHGVN2-3uzZV_BPGdE0W_dNaqYTtYqeo="] = shouldBeUnauthedWriteOnly diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index b1f2ef8d52..6930204143 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/builtin/logical/pki/path_acme_order.go @@ -268,6 +268,11 @@ func (b *backend) acmeFinalizeOrderHandler(ac *acmeContext, _ *logical.Request, return nil, err } + if err := b.acmeState.TrackIssuedCert(ac, order.AccountId, hyphenSerialNumber, order.OrderId); err != nil { + b.Logger().Warn("orphaned generated ACME certificate due to error saving account->cert->order reference", "serial_number", hyphenSerialNumber, "error", err) + return nil, err + } + order.Status = ACMEOrderValid order.CertificateSerialNumber = hyphenSerialNumber order.CertificateExpiry = signedCertBundle.Certificate.NotAfter diff --git a/builtin/logical/pki/path_acme_revoke.go b/builtin/logical/pki/path_acme_revoke.go new file mode 100644 index 0000000000..6d9486017a --- /dev/null +++ b/builtin/logical/pki/path_acme_revoke.go @@ -0,0 +1,180 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package pki + +import ( + "bytes" + "crypto" + "crypto/x509" + "encoding/base64" + "fmt" + "time" + + "github.com/hashicorp/vault/sdk/framework" + "github.com/hashicorp/vault/sdk/logical" +) + +func pathAcmeRevoke(b *backend) []*framework.Path { + return buildAcmeFrameworkPaths(b, patternAcmeRevoke, "/revoke-cert") +} + +func patternAcmeRevoke(b *backend, pattern string) *framework.Path { + fields := map[string]*framework.FieldSchema{} + addFieldsForACMEPath(fields, pattern) + addFieldsForACMERequest(fields) + + return &framework.Path{ + Pattern: pattern, + Fields: fields, + Operations: map[logical.Operation]framework.OperationHandler{ + logical.UpdateOperation: &framework.PathOperation{ + Callback: b.acmeParsedWrapper(b.acmeRevocationHandler), + ForwardPerformanceSecondary: false, + ForwardPerformanceStandby: true, + }, + }, + + HelpSynopsis: "", + HelpDescription: "", + } +} + +func (b *backend) acmeRevocationHandler(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}) (*logical.Response, error) { + var cert *x509.Certificate + + rawCertificate, present := data["certificate"] + if present { + certBase64, ok := rawCertificate.(string) + if !ok { + return nil, fmt.Errorf("invalid type (%T; expected string) for field 'certificate': %w", rawCertificate, ErrMalformed) + } + + certBytes, err := base64.RawURLEncoding.DecodeString(certBase64) + if err != nil { + return nil, fmt.Errorf("failed to base64 decode certificate: %v: %w", err, ErrMalformed) + } + + cert, err = x509.ParseCertificate(certBytes) + if err != nil { + return nil, fmt.Errorf("failed to parse certificate: %v: %w", err, ErrMalformed) + } + } else { + return nil, fmt.Errorf("bad request was lacking required field 'certificate': %w", ErrMalformed) + } + + rawReason, present := data["reason"] + if present { + reason, ok := rawReason.(float64) + if !ok { + return nil, fmt.Errorf("invalid type (%T; expected float64) for field 'reason': %w", rawReason, ErrMalformed) + } + + if int(reason) != 0 { + return nil, fmt.Errorf("Vault does not support revocation reasons (got %v; expected omitted or 0/unspecified): %w", int(reason), ErrBadRevocationReason) + } + } + + // If the certificate expired, there's no point in revoking it. + if cert.NotAfter.Before(time.Now()) { + return nil, fmt.Errorf("refusing to revoke expired certificate: %w", ErrMalformed) + } + + // Fetch the CRL config as we need it to ultimately do the + // revocation. This should be cached and thus relatively fast. + config, err := b.crlBuilder.getConfigWithUpdate(acmeCtx.sc) + if err != nil { + return nil, fmt.Errorf("unable to revoke certificate: failed reading revocation config: %v: %w", err, ErrServerInternal) + } + + // Load our certificate from storage to ensure it exists and matches + // what was given to us. + serial := serialFromCert(cert) + certEntry, err := fetchCertBySerial(acmeCtx.sc, "certs/", serial) + if err != nil { + return nil, fmt.Errorf("unable to revoke certificate: err reading global cert entry: %v: %w", err, ErrServerInternal) + } + if certEntry == nil { + return nil, fmt.Errorf("unable to revoke certificate: no global cert entry found: %w", ErrServerInternal) + } + + // Validate that the provided certificate matches the stored + // certificate. This completes the chain of: + // + // provided_auth -> provided_cert == stored cert. + // + // Allowing revocation to be safe. + // + // We use the non-subtle unsafe bytes equality check here as we have + // already fetched this certificate from storage, thus already leaking + // timing information that this cert exists. The user could thus simply + // fetch the cert from Vault matching this serial number via the unauthed + // pki/certs/:serial API endpoint. + if !bytes.Equal(certEntry.Value, cert.Raw) { + return nil, fmt.Errorf("unable to revoke certificate: supplied certificate does not match CA's stored value: %w", ErrMalformed) + } + + // Check if it was already revoked; in this case, we do not need to + // revoke it again and want to respond with an appropriate error message. + revEntry, err := fetchCertBySerial(acmeCtx.sc, "revoked/", serial) + if err != nil { + return nil, fmt.Errorf("unable to revoke certificate: err reading revocation entry: %v: %w", err, ErrServerInternal) + } + if revEntry != nil { + return nil, fmt.Errorf("unable to revoke certificate: %w", ErrAlreadyRevoked) + } + + // Finally, do the relevant permissions/authorization check as + // appropriate based on the type of revocation happening. + if !userCtx.Existing { + return b.acmeRevocationByPoP(acmeCtx, r, fields, userCtx, data, cert, config) + } + + return b.acmeRevocationByAccount(acmeCtx, r, fields, userCtx, data, cert, config) +} + +func (b *backend) acmeRevocationByPoP(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}, cert *x509.Certificate, config *crlConfig) (*logical.Response, error) { + // Since this account does not exist, ensure we've gotten a private key + // matching the certificate's public key. + signer, ok := userCtx.Key.Key.(crypto.Signer) + if !ok { + return nil, fmt.Errorf("unable to revoke certificate: unable to parse JWS key of type (%T): %w", userCtx.Key.Key, ErrMalformed) + } + + // Ensure that our PoP is indeed valid. + if err := validatePrivateKeyMatchesCert(signer, cert); err != nil { + return nil, fmt.Errorf("unable to revoke certificate: unable to verify proof of possession: %v: %w", err, ErrMalformed) + } + + // Now it is safe to revoke. + b.revokeStorageLock.Lock() + defer b.revokeStorageLock.Unlock() + + return revokeCert(acmeCtx.sc, config, cert) +} + +func (b *backend) acmeRevocationByAccount(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}, cert *x509.Certificate, config *crlConfig) (*logical.Response, error) { + // Fetch the account; disallow revocations from non-valid-status + // accounts. + account, err := b.acmeState.LoadAccount(acmeCtx, userCtx.Kid) + if err != nil { + return nil, fmt.Errorf("failed to lookup account: %w", err) + } + if account.Status != StatusValid { + return nil, fmt.Errorf("account isn't presently valid: %w", ErrUnauthorized) + } + + // We only support certificates issued by this user, we don't support + // cross-account revocations. + serial := serialFromCert(cert) + acmeEntry, err := b.acmeState.GetIssuedCert(acmeCtx, userCtx.Kid, serial) + if err != nil || acmeEntry == nil { + return nil, fmt.Errorf("unable to revoke certificate: %v: %w", err, ErrMalformed) + } + + // Now it is safe to revoke. + b.revokeStorageLock.Lock() + defer b.revokeStorageLock.Unlock() + + return revokeCert(acmeCtx.sc, config, cert) +} diff --git a/builtin/logical/pki/path_revoke.go b/builtin/logical/pki/path_revoke.go index 27b4d5b8fe..fac46bc813 100644 --- a/builtin/logical/pki/path_revoke.go +++ b/builtin/logical/pki/path_revoke.go @@ -5,6 +5,7 @@ package pki import ( "context" + "crypto" "crypto/ecdsa" "crypto/ed25519" "crypto/rsa" @@ -446,6 +447,10 @@ func (b *backend) pathRevokeWriteHandleKey(req *logical.Request, certReference * return fmt.Errorf("failed to parse provided private key: %w", err) } + return validatePrivateKeyMatchesCert(signer, certReference) +} + +func validatePrivateKeyMatchesCert(signer crypto.Signer, certReference *x509.Certificate) error { // Finally, verify if the cert and key match. This code has been // cribbed from the Go TLS config code, with minor modifications. // @@ -453,7 +458,6 @@ func (b *backend) pathRevokeWriteHandleKey(req *logical.Request, certReference * // components and ensure we validate exponent and curve information // as well. // - // // See: https://github.com/golang/go/blob/c6a2dada0df8c2d75cf3ae599d7caed77d416fa2/src/crypto/tls/tls.go#L304-L331 switch certPub := certReference.PublicKey.(type) { case *rsa.PublicKey: diff --git a/builtin/logical/pkiext/acme_test.go b/builtin/logical/pkiext/acme_test.go index 394c9207c1..e31331fe54 100644 --- a/builtin/logical/pkiext/acme_test.go +++ b/builtin/logical/pkiext/acme_test.go @@ -65,19 +65,52 @@ func CheckCertBot(t *testing.T, vaultNetwork string, vaultNodeID string, directo "--agree-tos", "--no-verify-ssl", "--standalone", + "--non-interactive", "--server", directory, "-d", hostname, } logCatCmd := []string{"cat", "/var/log/letsencrypt/letsencrypt.log"} stdout, stderr, retcode, err = runner.RunCmdWithOutput(ctx, result.Container.ID, certbotCmd) - t.Logf("Certbot Command: %v\nstdout: %v\nstderr: %v\n", certbotCmd, string(stdout), string(stderr)) + t.Logf("Certbot Issue Command: %v\nstdout: %v\nstderr: %v\n", certbotCmd, string(stdout), string(stderr)) if err != nil || retcode != 0 { logsStdout, logsStderr, _, _ := runner.RunCmdWithOutput(ctx, result.Container.ID, logCatCmd) t.Logf("Certbot logs\nstdout: %v\nstderr: %v\n", string(logsStdout), string(logsStderr)) } - require.NoError(t, err, "got error running command") - require.Equal(t, 0, retcode, "expected zero retcode") + require.NoError(t, err, "got error running issue command") + require.Equal(t, 0, retcode, "expected zero retcode issue command result") + + certbotRevokeCmd := []string{ + "certbot", + "revoke", + "--no-eff-email", + "--email", "certbot.client@dadgarcorp.com", + "--agree-tos", + "--no-verify-ssl", + "--non-interactive", + "--no-delete-after-revoke", + "--cert-name", hostname, + } + + stdout, stderr, retcode, err = runner.RunCmdWithOutput(ctx, result.Container.ID, certbotRevokeCmd) + t.Logf("Certbot Revoke Command: %v\nstdout: %v\nstderr: %v\n", certbotRevokeCmd, string(stdout), string(stderr)) + if err != nil || retcode != 0 { + logsStdout, logsStderr, _, _ := runner.RunCmdWithOutput(ctx, result.Container.ID, logCatCmd) + t.Logf("Certbot logs\nstdout: %v\nstderr: %v\n", string(logsStdout), string(logsStderr)) + } + require.NoError(t, err, "got error running revoke command") + require.Equal(t, 0, retcode, "expected zero retcode revoke command result") + + // Revoking twice should fail. + stdout, stderr, retcode, err = runner.RunCmdWithOutput(ctx, result.Container.ID, certbotRevokeCmd) + t.Logf("Certbot Double Revoke Command: %v\nstdout: %v\nstderr: %v\n", certbotRevokeCmd, string(stdout), string(stderr)) + if err != nil || retcode == 0 { + logsStdout, logsStderr, _, _ := runner.RunCmdWithOutput(ctx, result.Container.ID, logCatCmd) + t.Logf("Certbot logs\nstdout: %v\nstderr: %v\n", string(logsStdout), string(logsStderr)) + } + + require.NoError(t, err, "got error running double revoke command") + require.NotEqual(t, 0, retcode, "expected non-zero retcode double revoke command result") runner.Stop(ctx, result.Container.ID) }