From 7528f6a467c82b02308433267223485a7ee8f6d3 Mon Sep 17 00:00:00 2001 From: Bugs5382 <12115015+Bugs5382@users.noreply.github.com> Date: Sun, 1 Nov 2026 23:55:08 -0500 Subject: [PATCH] fix(ocsp): answer OCSP for old-key leaves with the previous responder after a re-key A re-key discards the old CA key, so leaves it issued could no longer get an OCSP response that verifies against the certificate they chain to. The commit now moves the responder, its key and the old CA certificate to a separate slot in the same transaction, requests naming the old key are answered from it, and the slot is dropped once the responder expires. Responses also echo the request's CertID hash algorithm (RFC 6960 section 4.2.1); they always used SHA-1 before, which a SHA-256 request can't match. Closes #387 Signed-off-by: Bugs5382 <12115015+Bugs5382@users.noreply.github.com> --- internal/init/ocsp_previous_test.go | 360 ++++++++++++++++++++++++++++ internal/init/ocsp_responder.go | 73 ++++++ internal/init/revocation.go | 13 +- internal/init/run.go | 3 + internal/node/store.go | 120 +++++++++- internal/revocation/ocsp.go | 34 ++- internal/revocation/ocsp_test.go | 63 +++++ internal/storage/etcd/etcd.go | 13 + 8 files changed, 666 insertions(+), 13 deletions(-) create mode 100644 internal/init/ocsp_previous_test.go diff --git a/internal/init/ocsp_previous_test.go b/internal/init/ocsp_previous_test.go new file mode 100644 index 0000000..39738c7 --- /dev/null +++ b/internal/init/ocsp_previous_test.go @@ -0,0 +1,360 @@ +package init + +/* +Copyright The CryptOS Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import ( + "context" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "testing" + "time" + + "golang.org/x/crypto/ocsp" + + "github.com/CryptOS-PKI/cryptos-node/internal/bootstrap" + "github.com/CryptOS-PKI/cryptos-node/internal/config" + "github.com/CryptOS-PKI/cryptos-node/internal/node" + "github.com/CryptOS-PKI/cryptos-node/internal/revocation" + "github.com/CryptOS-PKI/cryptos-node/internal/storage/etcd" +) + +// rotationRig wires an established Intermediate the way run.go does, with a +// real rekeyer and enroller, so a test can issue leaves, re-key and query /ocsp. +type rotationRig struct { + store *node.Store + revStore *revocation.Store + parent *rekeyParent + rekeyer *nodeRekeyer + signer *node.CASigner + responder *ocspResponder + ocspFn func(context.Context, []byte) ([]byte, error) +} + +func newRotationRig(t *testing.T) (*rotationRig, context.Context) { + t.Helper() + srv, err := etcd.Open(t.TempDir()) + if err != nil { + t.Fatalf("etcd.Open: %v", err) + } + t.Cleanup(func() { _ = srv.Close() }) + cli, err := srv.Client() + if err != nil { + t.Fatalf("etcd.Client: %v", err) + } + t.Cleanup(func() { _ = cli.Close() }) + store, err := node.New(cli) + if err != nil { + t.Fatalf("node.New: %v", err) + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + revStore := revocation.NewStore(cli) + parent := newRekeyParent(t) + + key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader) + if err != nil { + t.Fatalf("GenerateKey: %v", err) + } + blob, err := x509.MarshalECPrivateKey(key) + if err != nil { + t.Fatalf("MarshalECPrivateKey: %v", err) + } + pub, err := x509.MarshalPKIXPublicKey(&key.PublicKey) + if err != nil { + t.Fatalf("MarshalPKIXPublicKey: %v", err) + } + csr, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{Subject: pkix.Name{CommonName: "Child Issuing CA"}}, key) + if err != nil { + t.Fatalf("CreateCertificateRequest: %v", err) + } + if err := store.StageSubordinate(ctx, csr, blob, pub); err != nil { + t.Fatalf("StageSubordinate: %v", err) + } + if err := store.CommitSubordinateCert(ctx, [][]byte{parent.signIntermediate(t, &key.PublicKey, ""), parent.der}); err != nil { + t.Fatalf("CommitSubordinateCert: %v", err) + } + + backend := NewSoftRootBackend() + load := func(ctx context.Context) (crypto.Signer, func(), error) { + priv, pub, _, err := store.RootKeyBlobs(ctx) + if err != nil { + return nil, nil, err + } + s, err := backend.LoadKey(priv, pub) + if err != nil { + return nil, nil, err + } + return s, func() { _ = s.Close() }, nil + } + issuer := func(ctx context.Context) (*x509.Certificate, error) { + id, err := store.Identity(ctx) + if err != nil { + return nil, err + } + return x509.ParseCertificate(id.ChainDer[0]) + } + + trust, err := bootstrap.LoadTrust(certPEM(parent.der), "") + if err != nil { + t.Fatalf("LoadTrust: %v", err) + } + enr, err := node.NewSubordinateEnroller(store, trust) + if err != nil { + t.Fatalf("NewSubordinateEnroller: %v", err) + } + rekeyer, err := newRekeyer(store, backend, rekeyConfig(), enr) + if err != nil { + t.Fatalf("newRekeyer: %v", err) + } + + cfg := &config.Config{PKI: config.PKI{Profiles: []config.CertificateProfile{ + {Name: "leaf", ValidityDays: 30, KeyUsage: []string{"digital_signature"}, ExtKeyUsage: []string{"server_auth"}}, + }}} + signer := node.NewCASigner(load, issuer, func(context.Context) (*config.Config, error) { return cfg, nil }). + WithRecorder(IssuedRecorder(revStore)) + revoker := &nodeRevoker{store: revStore, crlBuilder: revocation.NewCRLBuilder(revStore, time.Hour), load: load, issuer: issuer} + responder := newOCSPResponder(store, load, issuer, 0) + return &rotationRig{ + store: store, + revStore: revStore, + parent: parent, + rekeyer: rekeyer, + signer: signer, + responder: responder, + ocspFn: revoker.ocspFn(revocation.NewOCSPResponder(revStore), responder), + }, ctx +} + +// issueLeaf returns a new leaf and the CA certificate that issued it. +func (r *rotationRig) issueLeaf(t *testing.T, ctx context.Context) (leaf, ca *x509.Certificate) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader) + if err != nil { + t.Fatalf("GenerateKey: %v", err) + } + csr, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{Subject: pkix.Name{CommonName: "svc.example"}}, key) + if err != nil { + t.Fatalf("CreateCertificateRequest: %v", err) + } + chain, _, err := r.signer.IssueLeafForNames(ctx, csr, "leaf", nil) + if err != nil { + t.Fatalf("IssueLeafForNames: %v", err) + } + if leaf, err = x509.ParseCertificate(chain[0]); err != nil { + t.Fatalf("ParseCertificate leaf: %v", err) + } + if ca, err = x509.ParseCertificate(chain[1]); err != nil { + t.Fatalf("ParseCertificate CA: %v", err) + } + return leaf, ca +} + +func (r *rotationRig) ensureResponder(t *testing.T, ctx context.Context) *x509.Certificate { + t.Helper() + cert, _, err := r.responder.ensure(ctx) + if err != nil { + t.Fatalf("ensure: %v", err) + } + return cert +} + +// reKey runs a full rotation: begin, parent signature, complete. +func (r *rotationRig) reKey(t *testing.T, ctx context.Context) { + t.Helper() + csr, err := r.rekeyer.BeginRotation(ctx) + if err != nil { + t.Fatalf("BeginRotation: %v", err) + } + der := r.parent.signCSR(t, csr, "Child Issuing CA") + if _, err := r.rekeyer.CompleteRotation(ctx, [][]byte{der, r.parent.der}); err != nil { + t.Fatalf("CompleteRotation: %v", err) + } +} + +// query asks /ocsp about leaf, naming issuer in the CertID, and validates the +// response against that same issuer, as a relying party does. +func (r *rotationRig) query(t *testing.T, ctx context.Context, leaf, issuer *x509.Certificate, opts *ocsp.RequestOptions) (*ocsp.Response, error) { + t.Helper() + reqDER, err := ocsp.CreateRequest(leaf, issuer, opts) + if err != nil { + t.Fatalf("ocsp.CreateRequest: %v", err) + } + body, err := r.ocspFn(ctx, reqDER) + if err != nil { + t.Fatalf("ocspFn: %v", err) + } + return ocsp.ParseResponseForCert(body, leaf, issuer) +} + +func TestOCSPAnswersAnOldKeyLeafWithThePreviousResponder(t *testing.T) { + r, ctx := newRotationRig(t) + leaf, oldCA := r.issueLeaf(t, ctx) + oldResponder := r.ensureResponder(t, ctx) + + r.reKey(t, ctx) + + got, err := r.query(t, ctx, leaf, oldCA, nil) + if err != nil { + t.Fatalf("response does not validate against the old Intermediate: %v", err) + } + if got.Status != ocsp.Good { + t.Errorf("status = %d, want Good", got.Status) + } + if got.Certificate == nil || got.Certificate.SerialNumber.Cmp(oldResponder.SerialNumber) != 0 { + t.Errorf("response embeds responder %v, want the retired responder %s", got.Certificate, oldResponder.SerialNumber.Text(16)) + } +} + +func TestOCSPReportsARevokedOldKeyLeaf(t *testing.T) { + r, ctx := newRotationRig(t) + leaf, oldCA := r.issueLeaf(t, ctx) + r.ensureResponder(t, ctx) + r.reKey(t, ctx) + + if _, err := r.revStore.Revoke(ctx, leaf.SerialNumber.Text(16), ocsp.KeyCompromise, time.Now().UTC()); err != nil { + t.Fatalf("Revoke: %v", err) + } + + got, err := r.query(t, ctx, leaf, oldCA, nil) + if err != nil { + t.Fatalf("response does not validate against the old Intermediate: %v", err) + } + if got.Status != ocsp.Revoked || got.RevocationReason != ocsp.KeyCompromise { + t.Errorf("status = %d reason = %d, want Revoked keyCompromise", got.Status, got.RevocationReason) + } +} + +func TestOCSPCurrentKeyRequestsKeepTheCurrentResponder(t *testing.T) { + r, ctx := newRotationRig(t) + r.issueLeaf(t, ctx) + oldResponder := r.ensureResponder(t, ctx) + r.reKey(t, ctx) + + leaf, newCA := r.issueLeaf(t, ctx) + got, err := r.query(t, ctx, leaf, newCA, nil) + if err != nil { + t.Fatalf("response does not validate against the new Intermediate: %v", err) + } + if got.Certificate == nil || got.Certificate.SerialNumber.Cmp(oldResponder.SerialNumber) == 0 { + t.Error("a request naming the current key was answered by the retired responder") + } +} + +// RFC 6960 section 4.2.1: the response names the same CertID as the request, +// including its hash algorithm. +func TestOCSPEchoesTheHashAlgorithmOfTheRequest(t *testing.T) { + r, ctx := newRotationRig(t) + oldLeaf, oldCA := r.issueLeaf(t, ctx) + r.ensureResponder(t, ctx) + r.reKey(t, ctx) + newLeaf, newCA := r.issueLeaf(t, ctx) + + for _, tc := range []struct { + name string + leaf, issuer *x509.Certificate + }{ + {"previous key", oldLeaf, oldCA}, + {"current key", newLeaf, newCA}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := r.query(t, ctx, tc.leaf, tc.issuer, &ocsp.RequestOptions{Hash: crypto.SHA256}) + if err != nil { + t.Fatalf("response does not validate: %v", err) + } + if got.IssuerHash != crypto.SHA256 { + t.Errorf("response CertID hash = %v, want SHA-256", got.IssuerHash) + } + }) + } +} + +func TestSecondReKeyReplacesThePreviousResponder(t *testing.T) { + r, ctx := newRotationRig(t) + r.ensureResponder(t, ctx) + r.reKey(t, ctx) + + secondLeaf, secondCA := r.issueLeaf(t, ctx) + secondResponder := r.ensureResponder(t, ctx) + r.reKey(t, ctx) + + got, err := r.query(t, ctx, secondLeaf, secondCA, nil) + if err != nil { + t.Fatalf("response does not validate against the second key: %v", err) + } + if got.Certificate == nil || got.Certificate.SerialNumber.Cmp(secondResponder.SerialNumber) != 0 { + t.Error("the slot does not hold the responder retired by the second re-key") + } +} + +// With no OCSP traffic or ticker between two re-keys, the stored responder is +// still the one under the first key. It must not be paired with the second CA +// certificate; the slot keeps the first key's pair, which still matches. +func TestReKeyKeepsTheSlotWhenTheStoredResponderIsStale(t *testing.T) { + r, ctx := newRotationRig(t) + firstLeaf, firstCA := r.issueLeaf(t, ctx) + r.ensureResponder(t, ctx) + r.reKey(t, ctx) + r.reKey(t, ctx) + + got, err := r.query(t, ctx, firstLeaf, firstCA, nil) + if err != nil { + t.Fatalf("response does not validate against the first key: %v", err) + } + if got.Status != ocsp.Good { + t.Errorf("status = %d, want Good", got.Status) + } +} + +func TestExpiredPreviousResponderIsDropped(t *testing.T) { + r, ctx := newRotationRig(t) + retired := r.ensureResponder(t, ctx) + r.reKey(t, ctx) + + if err := r.responder.dropExpiredPrevious(ctx, retired.NotAfter.Add(-time.Minute)); err != nil { + t.Fatalf("dropExpiredPrevious before expiry: %v", err) + } + if _, _, _, ok, err := r.store.PreviousOCSPResponder(ctx); err != nil || !ok { + t.Fatalf("slot before expiry ok=%v err=%v, want it kept", ok, err) + } + + if err := r.responder.dropExpiredPrevious(ctx, retired.NotAfter.Add(time.Minute)); err != nil { + t.Fatalf("dropExpiredPrevious after expiry: %v", err) + } + if _, _, _, ok, err := r.store.PreviousOCSPResponder(ctx); err != nil || ok { + t.Fatalf("slot after expiry ok=%v err=%v, want it empty", ok, err) + } +} + +// An expired slot must not answer even if the ticker has not dropped it yet. +func TestExpiredPreviousResponderDoesNotAnswer(t *testing.T) { + r, ctx := newRotationRig(t) + leaf, oldCA := r.issueLeaf(t, ctx) + retired := r.ensureResponder(t, ctx) + r.reKey(t, ctx) + + reqDER, err := ocsp.CreateRequest(leaf, oldCA, nil) + if err != nil { + t.Fatalf("ocsp.CreateRequest: %v", err) + } + if _, ok := r.responder.previousFor(ctx, reqDER, retired.NotAfter.Add(time.Minute)); ok { + t.Error("an expired previous responder was selected") + } +} diff --git a/internal/init/ocsp_responder.go b/internal/init/ocsp_responder.go index 38765da..467014a 100644 --- a/internal/init/ocsp_responder.go +++ b/internal/init/ocsp_responder.go @@ -28,6 +28,7 @@ import ( "github.com/CryptOS-PKI/cryptos-node/internal/ca" "github.com/CryptOS-PKI/cryptos-node/internal/node" + "github.com/CryptOS-PKI/cryptos-node/internal/revocation" ) // defaultOCSPResponderValidity is the lifetime of a minted delegated OCSP @@ -192,3 +193,75 @@ func (r *ocspResponder) mint(ctx context.Context) (*x509.Certificate, crypto.Sig } return cert, key, nil } + +// previousResponder is the responder a CA re-key retired, with the old CA +// certificate it chains to. +type previousResponder struct { + cert *x509.Certificate + key crypto.Signer + issuer *x509.Certificate +} + +// loadPrevious returns the unexpired contents of the previous-responder slot, +// and the raw certificate for dropping it. ok is false when the slot is empty, +// unusable or expired. It never touches the CA key: that key is gone. +func (r *ocspResponder) loadPrevious(ctx context.Context, now time.Time) (*previousResponder, []byte, bool) { + certDER, keyBlob, caDER, ok, err := r.store.PreviousOCSPResponder(ctx) + if err != nil { + log.Printf("OCSP responder: read previous-responder slot: %v", err) + return nil, nil, false + } + if !ok { + return nil, nil, false + } + cert, err := x509.ParseCertificate(certDER) + if err != nil { + log.Printf("OCSP responder: parse previous responder: %v", err) + return nil, certDER, false + } + if now.After(cert.NotAfter) { + return nil, certDER, false + } + issuer, err := x509.ParseCertificate(caDER) + if err != nil { + log.Printf("OCSP responder: parse previous CA certificate: %v", err) + return nil, certDER, false + } + key, err := parseNodeKey(keyBlob) + if err != nil { + log.Printf("OCSP responder: parse previous responder key: %v", err) + return nil, certDER, false + } + return &previousResponder{cert: cert, key: key, issuer: issuer}, certDER, true +} + +// previousFor returns the retired responder when reqDER names the CA key it +// chains to. A malformed request, or one naming any other issuer, reports +// ok=false and takes the current-responder path. +func (r *ocspResponder) previousFor(ctx context.Context, reqDER []byte, now time.Time) (*previousResponder, bool) { + prev, _, ok := r.loadPrevious(ctx, now) + if !ok { + return nil, false + } + named, err := revocation.RequestNamesIssuer(reqDER, prev.issuer) + if err != nil { + log.Printf("OCSP responder: match request to the previous CA: %v", err) + return nil, false + } + return prev, named +} + +// dropExpiredPrevious empties the previous-responder slot once its responder +// has expired or can no longer be read, so the retired key does not outlive +// its use. +func (r *ocspResponder) dropExpiredPrevious(ctx context.Context, now time.Time) error { + _, certDER, ok := r.loadPrevious(ctx, now) + if ok || certDER == nil { + return nil + } + if err := r.store.DropPreviousOCSPResponder(ctx, certDER); err != nil { + return fmt.Errorf("init: drop previous OCSP responder: %w", err) + } + log.Printf("OCSP responder: dropped the responder kept for the previous CA key") + return nil +} diff --git a/internal/init/revocation.go b/internal/init/revocation.go index 0eb4d76..0e55abf 100644 --- a/internal/init/revocation.go +++ b/internal/init/revocation.go @@ -22,6 +22,7 @@ import ( "encoding/hex" "errors" "fmt" + "log" "time" nodev1 "github.com/CryptOS-PKI/cryptos-node/gen/go/cryptos/node/v1" @@ -225,8 +226,18 @@ func (r *nodeRevoker) caCertFn() func(ctx context.Context) ([]byte, error) { // signing with the delegated responder key rather than the CA key. If the // responder cannot be ensured (CA key unavailable) the request fails; the HTTP // layer maps that to internalError. +// +// A request naming the CA key a re-key retired is answered by the responder kept +// for that key, checked first because it needs neither the CA key nor a current +// responder. Its leaves chain only to the old CA certificate, so an answer from +// the current responder would not verify (#387). func (r *nodeRevoker) ocspFn(resp *revocation.OCSPResponder, responder *ocspResponder) func(ctx context.Context, reqDER []byte) ([]byte, error) { return func(ctx context.Context, reqDER []byte) ([]byte, error) { + now := time.Now().UTC() + if prev, ok := responder.previousFor(ctx, reqDER, now); ok { + log.Printf("OCSP: answering for the previous CA key with responder serial %s", prev.cert.SerialNumber.Text(16)) + return resp.Respond(ctx, reqDER, prev.issuer, prev.cert, prev.key, now) + } responderCert, responderKey, err := responder.ensure(ctx) if err != nil { return nil, fmt.Errorf("init: ensure OCSP responder: %w", err) @@ -235,7 +246,7 @@ func (r *nodeRevoker) ocspFn(resp *revocation.OCSPResponder, responder *ocspResp if err != nil { return nil, fmt.Errorf("init: load issuer for OCSP: %w", err) } - return resp.Respond(ctx, reqDER, issuer, responderCert, responderKey, time.Now().UTC()) + return resp.Respond(ctx, reqDER, issuer, responderCert, responderKey, now) } } diff --git a/internal/init/run.go b/internal/init/run.go index 006d465..d61a1b2 100644 --- a/internal/init/run.go +++ b/internal/init/run.go @@ -885,6 +885,9 @@ func boot(ctx context.Context, shutdown *shutdownRequests) (err error) { if _, _, err := ocspResponderMgr.ensure(ctx); err != nil { log.Printf("OCSP responder: renewal ensure failed: %v", err) } + if err := ocspResponderMgr.dropExpiredPrevious(ctx, time.Now()); err != nil { + log.Printf("OCSP responder: %v", err) + } } check() t := time.NewTicker(30 * time.Second) diff --git a/internal/node/store.go b/internal/node/store.go index bf5759f..099dfbf 100644 --- a/internal/node/store.go +++ b/internal/node/store.go @@ -19,11 +19,13 @@ limitations under the License. import ( "context" "crypto/sha256" + "crypto/x509" "encoding/hex" "encoding/json" "encoding/pem" "errors" "fmt" + "log" "strconv" "time" @@ -566,7 +568,9 @@ func (s *Store) RotationKeyBlobs(ctx context.Context) (private, public []byte, o // transaction is guarded on the rotation key-blob existing so a commit with no // staged rotation does not apply and returns ErrNoRotation, which makes commit // re-entry after a crash idempotent. The old key is discarded; certificates it -// already signed keep validating until they expire. Chain verification (that +// already signed keep validating until they expire. The delegated OCSP +// responder under the old key moves to the previous-responder slot in the same +// transaction, so OCSP keeps working for those certificates. Chain verification (that // the chain roots to the pinned parent anchor and that the leaf carries the // staged rotation key) is the caller's responsibility (the enroller verifies // before calling this). @@ -597,17 +601,24 @@ func (s *Store) CommitRotation(ctx context.Context, chainDER [][]byte) error { return ErrNoRotation } + retire, err := s.retireResponderOps(ctx) + if err != nil { + return fmt.Errorf("node: CommitRotation: %w", err) + } + + ops := []clientv3.Op{ + clientv3.OpPut(etcd.KeyRootKeyBlob, string(keyPriv)), + clientv3.OpPut(etcd.KeyRootKeyPublic, string(keyPub)), + clientv3.OpPut(etcd.KeyIdentityChain, string(chainJSON)), + clientv3.OpPut(etcd.KeyRootCert, string(leafDER)), + clientv3.OpDelete(etcd.KeyRotationCSR), + clientv3.OpDelete(etcd.KeyRotationKeyBlob), + clientv3.OpDelete(etcd.KeyRotationKeyPublic), + } + ops = append(ops, retire...) resp, err := s.cli.Txn(ctx). If(clientv3.Compare(clientv3.CreateRevision(etcd.KeyRotationKeyBlob), "!=", 0)). - Then( - clientv3.OpPut(etcd.KeyRootKeyBlob, string(keyPriv)), - clientv3.OpPut(etcd.KeyRootKeyPublic, string(keyPub)), - clientv3.OpPut(etcd.KeyIdentityChain, string(chainJSON)), - clientv3.OpPut(etcd.KeyRootCert, string(leafDER)), - clientv3.OpDelete(etcd.KeyRotationCSR), - clientv3.OpDelete(etcd.KeyRotationKeyBlob), - clientv3.OpDelete(etcd.KeyRotationKeyPublic), - ). + Then(ops...). Commit() if err != nil { return fmt.Errorf("node: CommitRotation: txn: %w", err) @@ -618,6 +629,53 @@ func (s *Store) CommitRotation(ctx context.Context, chainDER [][]byte) error { return nil } +// retireResponderOps returns the operations that move the stored OCSP responder +// and the CA certificate it chains to into the previous-responder slot. It +// returns none when there is nothing valid to keep: no responder has been +// minted, or the stored one is not signed by the CA being retired, which +// happens when two re-keys land before the responder is re-minted. Pairing that +// responder with this CA certificate would produce responses no client accepts, +// so the slot keeps what it has, a pair that still matches. +func (s *Store) retireResponderOps(ctx context.Context) ([]clientv3.Op, error) { + certDER, keyBlob, _, ok, err := s.OCSPResponder(ctx) + if err != nil { + return nil, fmt.Errorf("read OCSP responder: %w", err) + } + if !ok { + log.Printf("node: re-key: no OCSP responder stored, nothing to keep for the retiring CA key") + return nil, nil + } + caKV, ok, err := s.getKV(ctx, etcd.KeyRootCert) + if err != nil { + return nil, fmt.Errorf("read CA certificate: %w", err) + } + if !ok { + return nil, nil + } + cert, err := x509.ParseCertificate(certDER) + if err != nil { + log.Printf("node: re-key: stored OCSP responder does not parse, not keeping it: %v", err) + return nil, nil + } + caCert, err := x509.ParseCertificate(caKV.Value) + if err != nil { + log.Printf("node: re-key: CA certificate does not parse, not keeping the OCSP responder: %v", err) + return nil, nil + } + if err := cert.CheckSignatureFrom(caCert); err != nil { + log.Printf("node: re-key: OCSP responder (serial %s) is not signed by the retiring CA certificate, not keeping it: %v", + cert.SerialNumber.Text(16), err) + return nil, nil + } + log.Printf("node: re-key: keeping OCSP responder (serial %s) for the retiring CA key until %s", + cert.SerialNumber.Text(16), cert.NotAfter.UTC().Format(time.RFC3339)) + return []clientv3.Op{ + clientv3.OpPut(etcd.KeyOCSPPreviousResponderCert, string(certDER)), + clientv3.OpPut(etcd.KeyOCSPPreviousResponderKeyBlob, string(keyBlob)), + clientv3.OpPut(etcd.KeyOCSPPreviousResponderCACert, string(caKV.Value)), + }, nil +} + // CommitRenewal atomically replaces an established subordinate's CA // certificate with a re-certification of the SAME key. The chain is leaf-first // (chainDER[0] is the renewed certificate). In a single guarded transaction it @@ -788,6 +846,48 @@ func (s *Store) OCSPResponder(ctx context.Context) (certDER, keyBlob, keyPublic true, nil } +// PreviousOCSPResponder returns the responder retired by the last CA re-key: +// its certificate (DER), its private key blob, and the CA certificate (DER) it +// chains to. ok is false when the slot is empty. +func (s *Store) PreviousOCSPResponder(ctx context.Context) (certDER, keyBlob, caCertDER []byte, ok bool, err error) { + certKV, okCert, err := s.getKV(ctx, etcd.KeyOCSPPreviousResponderCert) + if err != nil { + return nil, nil, nil, false, err + } + blobKV, okBlob, err := s.getKV(ctx, etcd.KeyOCSPPreviousResponderKeyBlob) + if err != nil { + return nil, nil, nil, false, err + } + caKV, okCA, err := s.getKV(ctx, etcd.KeyOCSPPreviousResponderCACert) + if err != nil { + return nil, nil, nil, false, err + } + if !okCert || !okBlob || !okCA { + return nil, nil, nil, false, nil + } + return append([]byte(nil), certKV.Value...), + append([]byte(nil), blobKV.Value...), + append([]byte(nil), caKV.Value...), + true, nil +} + +// DropPreviousOCSPResponder empties the previous-responder slot if it still +// holds certDER, so a re-key that filled the slot in the meantime is not undone. +func (s *Store) DropPreviousOCSPResponder(ctx context.Context, certDER []byte) error { + _, err := s.cli.Txn(ctx). + If(clientv3.Compare(clientv3.Value(etcd.KeyOCSPPreviousResponderCert), "=", string(certDER))). + Then( + clientv3.OpDelete(etcd.KeyOCSPPreviousResponderCert), + clientv3.OpDelete(etcd.KeyOCSPPreviousResponderKeyBlob), + clientv3.OpDelete(etcd.KeyOCSPPreviousResponderCACert), + ). + Commit() + if err != nil { + return fmt.Errorf("node: DropPreviousOCSPResponder: txn: %w", err) + } + return nil +} + // getString returns the value at key as a string and whether it exists. func (s *Store) getString(ctx context.Context, key string) (string, bool, error) { kv, ok, err := s.getKV(ctx, key) diff --git a/internal/revocation/ocsp.go b/internal/revocation/ocsp.go index a2a046b..4e49c5c 100644 --- a/internal/revocation/ocsp.go +++ b/internal/revocation/ocsp.go @@ -17,9 +17,12 @@ limitations under the License. */ import ( + "bytes" "context" "crypto" "crypto/x509" + "crypto/x509/pkix" + "encoding/asn1" "fmt" "time" @@ -63,8 +66,11 @@ func (r *OCSPResponder) Respond(ctx context.Context, reqDER []byte, issuer, resp tmpl := ocsp.Response{ SerialNumber: req.SerialNumber, - ThisUpdate: now, - NextUpdate: now.Add(ocspNextUpdate), + // RFC 6960 section 4.2.1: the response names the requested CertID, so it + // is hashed with the algorithm the client chose. + IssuerHash: req.HashAlgorithm, + ThisUpdate: now, + NextUpdate: now.Add(ocspNextUpdate), // Embed the delegated responder certificate so a relying party can // validate that the response signer (responderKey) chains to issuer via // a cert carrying id-kp-OCSPSigning, rather than expecting the CA key to @@ -92,3 +98,27 @@ func (r *OCSPResponder) Respond(ctx context.Context, reqDER []byte, issuer, resp } return respDER, nil } + +// RequestNamesIssuer reports whether the CertID in reqDER names issuer, which +// RFC 6960 section 4.1.1 defines as both the hash of the issuer's DN and the +// hash of its public key (the BIT STRING value, without tag and length). It +// fails on a malformed request. +func RequestNamesIssuer(reqDER []byte, issuer *x509.Certificate) (bool, error) { + req, err := ocsp.ParseRequest(reqDER) + if err != nil { + return false, fmt.Errorf("revocation: RequestNamesIssuer: parse request: %w", err) + } + var spki struct { + Algorithm pkix.AlgorithmIdentifier + PublicKey asn1.BitString + } + if _, err := asn1.Unmarshal(issuer.RawSubjectPublicKeyInfo, &spki); err != nil { + return false, fmt.Errorf("revocation: RequestNamesIssuer: parse issuer public key: %w", err) + } + nameHash := req.HashAlgorithm.New() + nameHash.Write(issuer.RawSubject) + keyHash := req.HashAlgorithm.New() + keyHash.Write(spki.PublicKey.RightAlign()) + return bytes.Equal(nameHash.Sum(nil), req.IssuerNameHash) && + bytes.Equal(keyHash.Sum(nil), req.IssuerKeyHash), nil +} diff --git a/internal/revocation/ocsp_test.go b/internal/revocation/ocsp_test.go index 0205467..95b0091 100644 --- a/internal/revocation/ocsp_test.go +++ b/internal/revocation/ocsp_test.go @@ -24,6 +24,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/asn1" + "math/big" "testing" "time" @@ -141,3 +142,65 @@ func TestOCSPGoodRevokedUnknown(t *testing.T) { t.Fatalf("unknown serial status=%d want %d", parsed.Status, ocsp.Unknown) } } + +func TestRequestNamesIssuer(t *testing.T) { + newCA := func(cn string, key crypto.Signer) *x509.Certificate { + t.Helper() + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: cn}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IsCA: true, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, key.Public(), key) + if err != nil { + t.Fatalf("CreateCertificate: %v", err) + } + cert, err := x509.ParseCertificate(der) + if err != nil { + t.Fatalf("ParseCertificate: %v", err) + } + return cert + } + newKey := func() crypto.Signer { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader) + if err != nil { + t.Fatalf("GenerateKey: %v", err) + } + return key + } + key := newKey() + named := newCA("Issuing CA", key) + leaf := &x509.Certificate{SerialNumber: big.NewInt(7)} + + for _, hash := range []crypto.Hash{crypto.SHA1, crypto.SHA256, crypto.SHA384, crypto.SHA512} { + reqDER, err := ocsp.CreateRequest(leaf, named, &ocsp.RequestOptions{Hash: hash}) + if err != nil { + t.Fatalf("ocsp.CreateRequest %v: %v", hash, err) + } + for _, tc := range []struct { + name string + issuer *x509.Certificate + want bool + }{ + {"same name and key", named, true}, + {"same name, other key", newCA("Issuing CA", newKey()), false}, + {"other name, same key", newCA("Other CA", key), false}, + } { + got, err := RequestNamesIssuer(reqDER, tc.issuer) + if err != nil { + t.Fatalf("%v %s: %v", hash, tc.name, err) + } + if got != tc.want { + t.Errorf("%v %s = %v, want %v", hash, tc.name, got, tc.want) + } + } + } + + if _, err := RequestNamesIssuer([]byte("not a request"), named); err == nil { + t.Error("a malformed request did not fail") + } +} diff --git a/internal/storage/etcd/etcd.go b/internal/storage/etcd/etcd.go index 4e5aa7b..581b41e 100644 --- a/internal/storage/etcd/etcd.go +++ b/internal/storage/etcd/etcd.go @@ -111,6 +111,19 @@ const ( // responder key. KeyOCSPResponderKeyPublic = "/cryptos/pki/ocsp-responder/key-public" + // KeyOCSPPreviousResponderCert holds the DER of the responder retired by the + // last CA re-key. It answers OCSP for leaves of the discarded CA key until + // it expires, at most the responder validity after the re-key. + KeyOCSPPreviousResponderCert = "/cryptos/pki/ocsp-responder/previous/cert" + + // KeyOCSPPreviousResponderKeyBlob holds the retired responder's private key + // in the same encoding as KeyOCSPResponderKeyBlob. + KeyOCSPPreviousResponderKeyBlob = "/cryptos/pki/ocsp-responder/previous/key-blob" + + // KeyOCSPPreviousResponderCACert holds the DER of the CA certificate the + // retired responder chains to. + KeyOCSPPreviousResponderCACert = "/cryptos/pki/ocsp-responder/previous/ca-cert" + // PrefixCeremonyManifests stores one signed Ceremony Manifest per // ceremony run, keyed by ceremony ID under this prefix. PrefixCeremonyManifests = "/cryptos/ceremony/manifests/"