bad-key-revoker: also deactivate accounts with blocked keys

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
 	}

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,15 +109,15 @@ 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),
 	)
 	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)
 

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)

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")
 

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)

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'@'%';

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,

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,