Deprecate the BlockedKeyTable feature flag (letsencrypt#4881)

aarongable · web-flow · commit 91d4e235ad64 · 2020-06-22T16:35:37.000-07:00
This commit consists of three classes of changes: 1) Changing various command main.go files to always behave as they would have when features.BlockedKeyTable was true. Also changing one test in the same manner. 2) Removing the BlockedKeyTable flag from configuration in config-next, because the flag is already live. 3) Moving the BlockedKeyTable flag to the "deprecated" section of features.go, and regenerating featureflag_strings.go. A future change will remove the BlockedKeyTable flag (and other similarly deprecated flags) from features.go entirely. Fixes letsencrypt#4873
diff --git a/cmd/boulder-ca/main.go b/cmd/boulder-ca/main.go
@@ -165,11 +165,7 @@ func main() {
 	cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to SA")
 	sa := bgrpc.NewStorageAuthorityClient(sapb.NewStorageAuthorityClient(conn))
 
-	var blockedKeyFunc goodkey.BlockedKeyCheckFunc
-	if features.Enabled(features.BlockedKeyTable) {
-		blockedKeyFunc = sa.KeyBlocked
-	}
-	kp, err := goodkey.NewKeyPolicy(c.CA.WeakKeyFile, c.CA.BlockedKeyFile, blockedKeyFunc)
+	kp, err := goodkey.NewKeyPolicy(c.CA.WeakKeyFile, c.CA.BlockedKeyFile, sa.KeyBlocked)
 	cmd.FailOnError(err, "Unable to create key policy")
 
 	var orphanQueue *goque.Queue
diff --git a/cmd/boulder-ra/main.go b/cmd/boulder-ra/main.go
@@ -204,11 +204,7 @@ func main() {
 		pendingAuthorizationLifetime = time.Duration(c.RA.PendingAuthorizationLifetimeDays) * 24 * time.Hour
 	}
 
-	var blockedKeyFunc goodkey.BlockedKeyCheckFunc
-	if features.Enabled(features.BlockedKeyTable) {
-		blockedKeyFunc = sac.KeyBlocked
-	}
-	kp, err := goodkey.NewKeyPolicy(c.RA.WeakKeyFile, c.RA.BlockedKeyFile, blockedKeyFunc)
+	kp, err := goodkey.NewKeyPolicy(c.RA.WeakKeyFile, c.RA.BlockedKeyFile, sac.KeyBlocked)
 	cmd.FailOnError(err, "Unable to create key policy")
 
 	if c.RA.MaxNames == 0 {
diff --git a/cmd/boulder-wfe/main.go b/cmd/boulder-wfe/main.go
@@ -143,12 +143,8 @@ func main() {
 	clk := cmd.Clock()
 
 	rac, sac, rns, npm := setupWFE(c, logger, stats, clk)
-	var blockedKeyFunc goodkey.BlockedKeyCheckFunc
-	if features.Enabled(features.BlockedKeyTable) {
-		blockedKeyFunc = sac.KeyBlocked
-	}
 	// don't load any weak keys, but do load blocked keys
-	kp, err := goodkey.NewKeyPolicy("", c.WFE.BlockedKeyFile, blockedKeyFunc)
+	kp, err := goodkey.NewKeyPolicy("", c.WFE.BlockedKeyFile, sac.KeyBlocked)
 	cmd.FailOnError(err, "Unable to create key policy")
 	wfe, err := wfe.NewWebFrontEndImpl(stats, clk, kp, rns, npm, logger)
 	cmd.FailOnError(err, "Unable to create WFE")
diff --git a/cmd/boulder-wfe2/main.go b/cmd/boulder-wfe2/main.go
@@ -316,12 +316,8 @@ func main() {
 	clk := cmd.Clock()
 
 	rac, sac, rns, npm := setupWFE(c, logger, stats, clk)
-	var blockedKeyFunc goodkey.BlockedKeyCheckFunc
-	if features.Enabled(features.BlockedKeyTable) {
-		blockedKeyFunc = sac.KeyBlocked
-	}
 	// don't load any weak keys, but do load blocked keys
-	kp, err := goodkey.NewKeyPolicy("", c.WFE.BlockedKeyFile, blockedKeyFunc)
+	kp, err := goodkey.NewKeyPolicy("", c.WFE.BlockedKeyFile, sac.KeyBlocked)
 	cmd.FailOnError(err, "Unable to create key policy")
 
 	if c.WFE.StaleTimeout.Duration == 0 {
diff --git a/features/featureflag_string.go b/features/featureflag_string.go
diff --git a/features/features.go b/features/features.go
@@ -18,6 +18,7 @@ const (
 	CheckRenewalFirst
 	ParallelCheckFailedValidation
 	DeleteUnusedChallenges
+	BlockedKeyTable
 
 	//   Currently in-use features
 	// Check CAA and respect validationmethods parameter.
@@ -49,12 +50,8 @@ const (
 	StoreIssuerInfo
 	// StoreKeyHashes enables storage of SPKI hashes associated with certificates.
 	StoreKeyHashes
-	// BlockedKeyTable enables storage, and checking, of the blockedKeys table in addition
-	// to the blocked key list
-	BlockedKeyTable
 	// StoreRevokerInfo enables storage of the revoker and a bool indicating if the row
-	// was checked for extant unrevoked certificates in the blockedKeys table. It should
-	// only be enabled if BlockedKeyTable is also enabled.
+	// was checked for extant unrevoked certificates in the blockedKeys table.
 	StoreRevokerInfo
 	// RestrictRSAKeySizes enables restriction of acceptable RSA public key moduli to
 	// the common sizes (2048, 3072, and 4096 bits).
@@ -84,7 +81,6 @@ var features = map[FeatureFlag]bool{
 	StoreIssuerInfo:               false,
 	WriteIssuedNamesPrecert:       false,
 	StoreKeyHashes:                false,
-	BlockedKeyTable:               false,
 	StoreRevokerInfo:              false,
 	RestrictRSAKeySizes:           false,
 	FasterNewOrdersRateLimit:      false,
diff --git a/ra/ra.go b/ra/ra.go
@@ -1676,7 +1676,7 @@ func (ra *RegistrationAuthorityImpl) revokeCertificate(ctx context.Context, cert
 	if err != nil {
 		return err
 	}
-	if features.Enabled(features.BlockedKeyTable) && reason == ocsp.KeyCompromise {
+	if reason == ocsp.KeyCompromise {
 		digest, err := core.KeyDigest(cert.PublicKey)
 		if err != nil {
 			return err
diff --git a/ra/ra_test.go b/ra/ra_test.go
@@ -3835,10 +3835,6 @@ func TestRevocationAddBlockedKey(t *testing.T) {
 	_, _, ra, _, cleanUp := initAuthorities(t)
 	defer cleanUp()
 
-	err := features.Set(map[string]bool{"BlockedKeyTable": true})
-	test.AssertNotError(t, err, "features.Set failed")
-	defer features.Reset()
-
 	mockSA := mockSABlockedKey{}
 	ra.SA = &mockSA
 	ra.CA = &mockCAOCSP{}
diff --git a/sa/_db/migrations/20200407130407_AddKeyHashTable.sql b/sa/_db/migrations/20200407130407_AddKeyHashTable.sql
diff --git a/sa/_db/migrations/20200414124347_AddBlockedKeysTable.sql b/sa/_db/migrations/20200414124347_AddBlockedKeysTable.sql
diff --git a/sa/_db/migrations/20200420135619_AddRowsBlockedKeys.sql b/sa/_db/migrations/20200420135619_AddRowsBlockedKeys.sql
diff --git a/sa/precertificates_test.go b/sa/precertificates_test.go
@@ -4,8 +4,6 @@ import (
 	"bytes"
 	"crypto/sha256"
 	"fmt"
-	"os"
-	"strings"
 	"testing"
 	"time"
 
@@ -32,10 +30,6 @@ func findIssuedName(dbMap db.OneSelector, name string) (string, error) {
 }
 
 func TestAddPrecertificate(t *testing.T) {
-	if !strings.HasSuffix(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") {
-		return
-	}
-
 	sa, clk, cleanUp := initSA(t)
 	defer cleanUp()
 
@@ -111,10 +105,6 @@ func TestAddPrecertificate(t *testing.T) {
 }
 
 func TestAddPrecertificateKeyHash(t *testing.T) {
-	if !strings.HasSuffix(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") {
-		return
-	}
-
 	sa, _, cleanUp := initSA(t)
 	defer cleanUp()
 	reg := satest.CreateWorkingRegistration(t, sa)
diff --git a/sa/sa_test.go b/sa/sa_test.go
@@ -12,9 +12,7 @@ import (
 	"math/big"
 	"math/bits"
 	"net"
-	"os"
 	"reflect"
-	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -2251,10 +2249,6 @@ func TestSerialExists(t *testing.T) {
 }
 
 func TestBlockedKey(t *testing.T) {
-	if !strings.HasSuffix(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") {
-		return
-	}
-
 	sa, _, cleanUp := initSA(t)
 	defer cleanUp()
 
@@ -2323,10 +2317,6 @@ func TestAddBlockedKeyUnknownSource(t *testing.T) {
 }
 
 func TestBlockedKeyRevokedBy(t *testing.T) {
-	if !strings.HasSuffix(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") {
-		return
-	}
-
 	sa, _, cleanUp := initSA(t)
 	defer cleanUp()
 
diff --git a/test/config-next/ca-a.json b/test/config-next/ca-a.json
@@ -137,8 +137,7 @@
     "maxConcurrentRPCServerRequests": 100000,
     "orphanQueueDir": "/tmp/orphaned-certificates-a",
     "features": {
-      "StoreIssuerInfo": true,
-      "BlockedKeyTable": true
+      "StoreIssuerInfo": true
     }
   },
 
diff --git a/test/config-next/ca-b.json b/test/config-next/ca-b.json
@@ -138,8 +138,7 @@
     "maxConcurrentRPCServerRequests": 100000,
     "orphanQueueDir": "/tmp/orphaned-certificates-b",
     "features": {
-      "StoreIssuerInfo": true,
-      "BlockedKeyTable": true
+      "StoreIssuerInfo": true
     }
   },
 
diff --git a/test/config-next/ra.json b/test/config-next/ra.json
@@ -47,7 +47,6 @@
       ]
     },
     "features": {
-      "BlockedKeyTable": true,
       "StoreRevokerInfo": true,
       "RestrictRSAKeySizes": true,
       "FasterNewOrdersRateLimit": true
diff --git a/test/config-next/wfe.json b/test/config-next/wfe.json
@@ -39,8 +39,7 @@
       }
     },
     "features": {
-      "StripDefaultSchemePort": true,
-      "BlockedKeyTable": true
+      "StripDefaultSchemePort": true
     }
   },
 
diff --git a/test/config-next/wfe2.json b/test/config-next/wfe2.json
@@ -53,8 +53,7 @@
     "features": {
       "MandatoryPOSTAsGET": true,
       "PrecertificateRevocation": true,
-      "StripDefaultSchemePort": true,
-      "BlockedKeyTable": true
+      "StripDefaultSchemePort": true
     }
   },
 

Original file line number	Diff line number	Diff line change
`@@ -1676,7 +1676,7 @@ func (ra *RegistrationAuthorityImpl) revokeCertificate(ctx context.Context, cert`
`1676`	`1676`	`if err != nil {`
`1677`	`1677`	`return err`
`1678`	`1678`	`}`
`1679`		`- if features.Enabled(features.BlockedKeyTable) && reason == ocsp.KeyCompromise {`
	`1679`	`+ if reason == ocsp.KeyCompromise {`
`1680`	`1680`	`digest, err := core.KeyDigest(cert.PublicKey)`
`1681`	`1681`	`if err != nil {`
`1682`	`1682`	`return err`