From 0d9cbb63736b55a752fce9fb90cbd06f4fea37d5 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 1 Jul 2026 16:08:37 -0700 Subject: [PATCH] bad-key-revoker: also deactivate accounts with blocked keys --- cmd/bad-key-revoker/main.go | 22 +++++++ cmd/bad-key-revoker/main_test.go | 82 +++++++++++++++++++++++---- core/util.go | 12 ++++ core/util_test.go | 41 ++++++++++++++ features/features.go | 5 ++ sa/db/02-users_next.sql | 2 +- sa/model.go | 3 +- test/config-next/bad-key-revoker.json | 5 +- 8 files changed, 156 insertions(+), 16 deletions(-) diff --git a/cmd/bad-key-revoker/main.go b/cmd/bad-key-revoker/main.go index e068bbd27e6..f4bae8b1f92 100644 --- a/cmd/bad-key-revoker/main.go +++ b/cmd/bad-key-revoker/main.go @@ -3,6 +3,7 @@ package notmain import ( "context" "database/sql" + "encoding/base64" "encoding/hex" "errors" "flag" @@ -22,6 +23,7 @@ import ( "github.com/letsencrypt/boulder/config" "github.com/letsencrypt/boulder/core" "github.com/letsencrypt/boulder/db" + "github.com/letsencrypt/boulder/features" bgrpc "github.com/letsencrypt/boulder/grpc" rapb "github.com/letsencrypt/boulder/ra/proto" "github.com/letsencrypt/boulder/revocation" @@ -236,6 +238,22 @@ func (bkr *badKeyRevoker) invoke(ctx context.Context) (work bool, err error) { slog.Int64("revokedBy", unchecked.RevokedBy), ) + if features.Get().DeactivateBadKeyAccounts { + // Deactivate the account, if any, which uses this key. The registrations + // table ensures that jwk_sha256 is unique. However, it stores the jwk_sha256 + // column in base64, unlike the keyHashToSerial table. We do this before the + // certs so that we can still early-exit if there are zero certs to process. + _, err = bkr.dbMap.ExecContext( + ctx, "UPDATE registrations SET status = ? WHERE jwk_sha256 = ? AND status = ? LIMIT 1", + string(core.StatusDeactivated), + base64.StdEncoding.EncodeToString(unchecked.KeyHash), + string(core.StatusValid), + ) + if err != nil { + return false, fmt.Errorf("deactivating corresponding account: %w", err) + } + } + // select all unrevoked, unexpired serials associated with the blocked key hash unrevokedCerts, err := bkr.findUnrevoked(ctx, unchecked) if err != nil { @@ -305,6 +323,8 @@ type Config struct { // the database's maximum replication lag, and always well under 24 // hours. MaxExpectedReplicationLag config.Duration `validate:"-"` + + Features features.Config } Syslog blog.Config @@ -324,6 +344,8 @@ func main() { err := cmd.ReadConfigFile(*configPath, &config) cmd.FailOnError(err, "Failed reading config file") + features.Set(config.BadKeyRevoker.Features) + if *debugAddr != "" { config.BadKeyRevoker.DebugAddr = *debugAddr } diff --git a/cmd/bad-key-revoker/main_test.go b/cmd/bad-key-revoker/main_test.go index 86ad9ba7aed..b93c6beb8ec 100644 --- a/cmd/bad-key-revoker/main_test.go +++ b/cmd/bad-key-revoker/main_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rand" "database/sql" + "encoding/base64" "errors" "fmt" "sync" @@ -18,6 +19,7 @@ import ( "github.com/letsencrypt/boulder/blog" "github.com/letsencrypt/boulder/core" "github.com/letsencrypt/boulder/db" + "github.com/letsencrypt/boulder/features" rapb "github.com/letsencrypt/boulder/ra/proto" "github.com/letsencrypt/boulder/sa" "github.com/letsencrypt/boulder/test" @@ -93,7 +95,12 @@ func TestSelectUncheckedRows(t *testing.T) { test.AssertEquals(t, row.RevokedBy, int64(1)) } -func insertRegistration(t *testing.T, dbMap *db.WrappedMap, fc clock.Clock) int64 { +// insertRegistration inserts a valid registration with a random key hash and +// returns its ID along with the raw (un-encoded) key hash. The key hash is +// stored in the jwk_sha256 column base64-encoded, matching production, so that +// blocking the returned hash will cause bad-key-revoker to find and deactivate +// this account. +func insertRegistration(t *testing.T, dbMap *db.WrappedMap, fc clock.Clock) (int64, []byte) { t.Helper() jwkHash := make([]byte, 32) _, err := rand.Read(jwkHash) @@ -102,7 +109,7 @@ func insertRegistration(t *testing.T, dbMap *db.WrappedMap, fc clock.Clock) int6 context.Background(), "INSERT INTO registrations (jwk, jwk_sha256, agreement, createdAt, status) VALUES (?, ?, ?, ?, ?)", []byte{}, - fmt.Sprintf("%x", jwkHash), + base64.StdEncoding.EncodeToString(jwkHash), "yes", fc.Now(), string(core.StatusValid), @@ -110,7 +117,7 @@ func insertRegistration(t *testing.T, dbMap *db.WrappedMap, fc clock.Clock) int6 test.AssertNotError(t, err, "failed to insert test registrations row") regID, err := res.LastInsertId() test.AssertNotError(t, err, "failed to get registration ID") - return regID + return regID, jwkHash } type ExpiredStatus bool @@ -225,7 +232,7 @@ func TestFindUnrevoked(t *testing.T) { fc := clock.NewFake() - regID := insertRegistration(t, dbMap, fc) + regID, _ := insertRegistration(t, dbMap, fc) bkr := &badKeyRevoker{ dbMap: dbMap, @@ -312,7 +319,7 @@ func TestCertificateAbsent(t *testing.T) { } // populate DB with all the test data - regIDA := insertRegistration(t, dbMap, fc) + regIDA, _ := insertRegistration(t, dbMap, fc) hashA := randHash(t) insertBlockedRow(t, dbMap, fcBeforeRepLag(fc, bkr), hashA, regIDA, false) @@ -354,10 +361,10 @@ func TestInvoke(t *testing.T) { } // populate DB with all the test data - regIDA := insertRegistration(t, dbMap, fc) - regIDB := insertRegistration(t, dbMap, fc) - regIDC := insertRegistration(t, dbMap, fc) - regIDD := insertRegistration(t, dbMap, fc) + regIDA, _ := insertRegistration(t, dbMap, fc) + regIDB, _ := insertRegistration(t, dbMap, fc) + regIDC, _ := insertRegistration(t, dbMap, fc) + regIDD, _ := insertRegistration(t, dbMap, fc) hashA := randHash(t) insertBlockedRow(t, dbMap, fcBeforeRepLag(fc, bkr), hashA, regIDC, false) insertGoodCert(t, dbMap, fc, hashA, "ff", regIDA) @@ -397,6 +404,57 @@ func TestInvoke(t *testing.T) { test.AssertEquals(t, noWork, true) } +// TestInvokeDeactivatesAccount checks that when a blocked key hash corresponds +// to an existing account, invoking bad-key-revoker updates that account's +// status to "deactivated". +func TestInvokeDeactivatesAccount(t *testing.T) { + features.Set(features.Config{DeactivateBadKeyAccounts: true}) + defer features.Reset() + + ctx := context.Background() + + dbMap, err := sa.DBMapForTest(vars.DBConnSAFullPerms) + if err != nil { + t.Fatalf("setting up db client: %s", err) + } + defer test.ResetBoulderTestDatabase(t)() + + fc := clock.NewFake() + + mr := &mockRevoker{} + bkr := &badKeyRevoker{ + dbMap: dbMap, + maxRevocations: 10, + serialBatchSize: 1, + raClient: mr, + logger: blog.NewMock(), + clk: fc, + maxExpectedReplicationLag: time.Second * 22, + keysToProcess: prometheus.NewGauge(prometheus.GaugeOpts{}), + certsRevoked: prometheus.NewCounter(prometheus.CounterOpts{}), + } + + // Create an account, then block its key. + regID, hashA := insertRegistration(t, dbMap, fc) + insertBlockedRow(t, dbMap, fcBeforeRepLag(fc, bkr), hashA, regID, false) + + _, err = bkr.invoke(ctx) + if err != nil { + t.Fatalf("invoke failed: %s", err) + } + + var status struct { + Status string + } + err = dbMap.SelectOne(ctx, &status, "SELECT status FROM registrations WHERE id = ?", regID) + if err != nil { + t.Fatalf("selecting registration status: %s", err) + } + if status.Status != string(core.StatusDeactivated) { + t.Errorf("account status = %q, want %q", status.Status, string(core.StatusDeactivated)) + } +} + func TestInvokeRevokerHasNoExtantCerts(t *testing.T) { // This test checks that when the user who revoked the initial // certificate that added the row to blockedKeys doesn't have any @@ -422,9 +480,9 @@ func TestInvokeRevokerHasNoExtantCerts(t *testing.T) { } // populate DB with all the test data - regIDA := insertRegistration(t, dbMap, fc) - regIDB := insertRegistration(t, dbMap, fc) - regIDC := insertRegistration(t, dbMap, fc) + regIDA, _ := insertRegistration(t, dbMap, fc) + regIDB, _ := insertRegistration(t, dbMap, fc) + regIDC, _ := insertRegistration(t, dbMap, fc) hashA := randHash(t) diff --git a/core/util.go b/core/util.go index e1228fbe6b5..97892b122d0 100644 --- a/core/util.go +++ b/core/util.go @@ -112,6 +112,9 @@ func KeyDigest(key crypto.PublicKey) (Sha256Digest, error) { case jose.JSONWebKey: return KeyDigest(t.Key) default: + // Marshalling the key to DER ensures that this has the exact same result + // as computing the hash over the RawSubjectPublicKeyInfo of a cert with + // the same key. keyDER, err := x509.MarshalPKIXPublicKey(key) if err != nil { return Sha256Digest{}, err @@ -130,6 +133,15 @@ func KeyDigestB64(key crypto.PublicKey) (string, error) { return base64.StdEncoding.EncodeToString(digest[:]), nil } +// CertKeyDigest is exactly the same as KeyDigest, except that it computes its +// hash over the SubjectPublicKeyInfo of a certificate, rather than an in-memory +// crypto.PublicKey. This is here to ensure that the methods used to compute +// hashes of cert keys and account keys never diverge, since bad-key-revoker +// checks both when a new key is blocked. +func CertKeyDigest(cert *x509.Certificate) Sha256Digest { + return sha256.Sum256(cert.RawSubjectPublicKeyInfo) +} + // KeyDigestEquals determines whether two public keys have the same digest. func KeyDigestEquals(j, k crypto.PublicKey) bool { digestJ, errJ := KeyDigestB64(j) diff --git a/core/util_test.go b/core/util_test.go index a64a073d73e..a36b7307ab5 100644 --- a/core/util_test.go +++ b/core/util_test.go @@ -2,6 +2,11 @@ package core import ( "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" "encoding/json" "errors" "fmt" @@ -117,6 +122,42 @@ func TestKeyDigestEquals(t *testing.T) { test.Assert(t, !KeyDigestEquals(struct{}{}, struct{}{}), "Unknown key types should not match anything") } +// TestCertKeyDigest ensures that CertKeyDigest (which hashes a certificate's +// SubjectPublicKeyInfo) and KeyDigest (which hashes an in-memory public key, +// usually from a parsed JWK) produce the same result for the same underlying +// key. bad-key-revoker relies on these two paths never diverging. +func TestCertKeyDigest(t *testing.T) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("generating ECDSA key: %s", err) + } + + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "example.com"}, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour), + BasicConstraintsValid: true, + } + certDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &priv.PublicKey, priv) + if err != nil { + t.Fatalf("self-signing certificate: %s", err) + } + cert, err := x509.ParseCertificate(certDER) + if err != nil { + t.Fatalf("parsing certificate: %s", err) + } + + keyDigest, err := KeyDigest(&priv.PublicKey) + if err != nil { + t.Fatalf("computing KeyDigest of public key: %s", err) + } + + if CertKeyDigest(cert) != keyDigest { + t.Errorf("CertKeyDigest = %x, want %x (KeyDigest of same key)", CertKeyDigest(cert), keyDigest) + } +} + func TestIsAnyNilOrZero(t *testing.T) { test.Assert(t, IsAnyNilOrZero(nil), "Nil seen as non-zero") diff --git a/features/features.go b/features/features.go index 42dd6b7ebb0..dd4aed6445a 100644 --- a/features/features.go +++ b/features/features.go @@ -68,6 +68,11 @@ type Config struct { // during certificate issuance. This flag must be set to true in the // RA and VA services for full functionality. DNSPersist01Enabled bool + + // DeactivateBadKeyAccounts controls whether bad-key-revoker will attempt + // to find and deactivate accounts using keys which have been added to the + // blockedKeys table. + DeactivateBadKeyAccounts bool } var fMu = new(sync.RWMutex) diff --git a/sa/db/02-users_next.sql b/sa/db/02-users_next.sql index 986a7110b59..67394065f99 100644 --- a/sa/db/02-users_next.sql +++ b/sa/db/02-users_next.sql @@ -71,10 +71,10 @@ GRANT SELECT ON precertificates TO 'cert_checker'@'%'; -- Bad Key Revoker GRANT SELECT,UPDATE ON blockedKeys TO 'badkeyrevoker'@'%'; +GRANT SELECT,UPDATE ON registrations TO 'badkeyrevoker'@'%'; GRANT SELECT ON keyHashToSerial TO 'badkeyrevoker'@'%'; GRANT SELECT ON certificateStatus TO 'badkeyrevoker'@'%'; GRANT SELECT ON precertificates TO 'badkeyrevoker'@'%'; -GRANT SELECT ON registrations TO 'badkeyrevoker'@'%'; -- ProxySQL -- GRANT ALL PRIVILEGES ON monitor TO 'proxysql'@'%'; diff --git a/sa/model.go b/sa/model.go index 86fd0303347..af5b15ce296 100644 --- a/sa/model.go +++ b/sa/model.go @@ -2,7 +2,6 @@ package sa import ( "context" - "crypto/sha256" "crypto/x509" "database/sql" "encoding/base64" @@ -1025,7 +1024,7 @@ func addKeyHash(ctx context.Context, db db.Inserter, cert *x509.Certificate) err if cert.RawSubjectPublicKeyInfo == nil { return errors.New("certificate has a nil RawSubjectPublicKeyInfo") } - h := sha256.Sum256(cert.RawSubjectPublicKeyInfo) + h := core.CertKeyDigest(cert) khm := &keyHashModel{ KeyHash: h[:], CertNotAfter: cert.NotAfter, diff --git a/test/config-next/bad-key-revoker.json b/test/config-next/bad-key-revoker.json index b031643865b..e7d9d858490 100644 --- a/test/config-next/bad-key-revoker.json +++ b/test/config-next/bad-key-revoker.json @@ -23,7 +23,10 @@ "findCertificatesBatchSize": 10, "interval": "50ms", "backoffIntervalMax": "2s", - "maxExpectedReplicationLag": "100ms" + "maxExpectedReplicationLag": "100ms", + "features": { + "DeactivateBadKeyAccounts": true + } }, "syslog": { "stdoutlevel": 4,