WFE2: Remove ACME13KeyRollover feature and legacy code. (#3999)

The draft 13 ACME v2 key change behaviour is now mandatory and we can remove the legacy WFE2 code.

features/featureflag_string.go

@@ -4,9 +4,9 @@ package features
 
 import "strconv"
 
-const _FeatureFlag_name = "unusedReusePendingAuthzCancelCTSubmissionsCountCertificatesExactIPv6FirstEnforceChallengeDisableEmbedSCTsWildcardDomainsForceConsistentStatusRPCHeadroomVAChecksGSBEnforceV2ContentTypeEnforceOverlappingWildcardsOrderReadyStatusPerformValidationRPCAllowRenewalFirstRLTLSSNIRevalidationCAAValidationMethodsCAAAccountURIACME13KeyRolloverProbeCTLogsSimplifiedVAHTTPHeadNonceStatusOK"
+const _FeatureFlag_name = "unusedReusePendingAuthzCancelCTSubmissionsCountCertificatesExactIPv6FirstEnforceChallengeDisableEmbedSCTsWildcardDomainsForceConsistentStatusRPCHeadroomVAChecksGSBEnforceV2ContentTypeEnforceOverlappingWildcardsOrderReadyStatusPerformValidationRPCACME13KeyRolloverAllowRenewalFirstRLTLSSNIRevalidationCAAValidationMethodsCAAAccountURIProbeCTLogsSimplifiedVAHTTPHeadNonceStatusOK"
 
-var _FeatureFlag_index = [...]uint16{0, 6, 23, 42, 64, 73, 96, 105, 120, 141, 152, 163, 183, 210, 226, 246, 265, 283, 303, 316, 333, 344, 360, 377}
+var _FeatureFlag_index = [...]uint16{0, 6, 23, 42, 64, 73, 96, 105, 120, 141, 152, 163, 183, 210, 226, 246, 263, 282, 300, 320, 333, 344, 360, 377}
 
 func (i FeatureFlag) String() string {
 	if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) {

features/features.go

@@ -26,6 +26,7 @@ const (
 	EnforceOverlappingWildcards
 	OrderReadyStatus
 	PerformValidationRPC
+	ACME13KeyRollover
 
 	//   Currently in-use features
 	AllowRenewalFirstRL
@@ -35,8 +36,6 @@ const (
 	CAAValidationMethods
 	// Check CAA and respect accounturi parameter.
 	CAAAccountURI
-	// Honour draft-ietf-acme-13's keyrollover
-	ACME13KeyRollover
 	// ProbeCTLogs enables HTTP probes to CT logs from the publisher
 	ProbeCTLogs
 	// SimplifiedVAHTTP enables the simplified VA http-01 rewrite that doesn't use

test/config-next/wfe2.json

@@ -38,7 +38,6 @@
     "features": {
       "EnforceV2ContentType": true,
       "RPCHeadroom": true,
-      "ACME13KeyRollover": true,
       "HeadNonceStatusOK": true
     }
   },

wfe2/verify.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/letsencrypt/boulder/core"
 	berrors "github.com/letsencrypt/boulder/errors"
-	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/probs"
 	"github.com/letsencrypt/boulder/web"
 )
@@ -629,31 +628,19 @@ func (wfe *WebFrontEndImpl) validSelfAuthenticatedPOST(
 	return wfe.validSelfAuthenticatedJWS(jws, request, logEvent)
 }
 
-// rolloverOperation is a struct representing a requested rollover operation
+// rolloverRequest is a client request to change the key for the account ID
-// from the specified old key to the new key for the given account ID. It is
+// provided from the specified old key to a new key (the embedded JWK in the
-// composed based on a rolloverRequest or a rolloverRequestDraft13 based on the
+// inner JWS).
-// `features.ACME13KeyRollover` feature flag.
-type rolloverOperation struct {
-	OldKey  jose.JSONWebKey
-	NewKey  jose.JSONWebKey
-	Account string
-}
-
-// rolloverRequest is a client request to change the account ID provided to the
-// new key provided. This is the legacy format used by ACMEv2 pre-draft13.
-// TODO(@cpu): Delete this type once features.ACME13KeyRollover is active
-// everywhere.
 type rolloverRequest struct {
-	NewKey  jose.JSONWebKey
+	OldKey  jose.JSONWebKey
 	Account string
 }
 
-// rolloverRequestDraft13 is a client request to change the key for the account
+// rolloverOperation is a struct representing a requested rollover operation
-// ID provided from the specified old key to a new key (the embedded JWK in the
+// from the specified old key to the new key for the given account ID.
-// inner JWS). This type is used when features.ACME13KeyRollover is enabled.
+type rolloverOperation struct {
-type rolloverRequestDraft13 struct {
+	rolloverRequest
-	OldKey  jose.JSONWebKey
+	NewKey jose.JSONWebKey
-	Account string
 }
 
 // validKeyRollover checks if the innerJWS is a valid key rollover operation
@@ -665,17 +652,12 @@ type rolloverRequestDraft13 struct {
 // 2) the inner JWS has the same "url" header as the outer JWS
 // 3) the inner JWS is self-authenticated with an embedded JWK
 //
-// If features.ACME13KeyRollover is enabled this function verifies that the
+// This function verifies that the inner JWS' body is a rolloverRequest instance
-// inner JWS' body is a rolloverRequestDraft13 instance that specifies the
+// that specifies the correct oldKey. The returned rolloverOperation's NewKey
-// correct oldKey. The returned rolloverOperation's NewKey field will be set to
+// field will be set to the JWK from the inner JWS.
-// the JWK from the inner JWS.
-// If features.ACME13KeyRollover is disabled this function verifies that the
-// inner JWS' body is a rolloverRequest instance and that the key in newKey
-// validates the inner JWS signature. The returned rolloverOperations' NewKey
-// field will be set to the JWK from the rolloverRequest.
 //
-// In both cases A *rolloverOperation object and is returned if successfully
+// If the request is valid a *rolloverOperation object is returned,
-// validated, otherwise a problem is returned. The caller is left to verify
+// otherwise a problem is returned. The caller is left to verify
 // whether the new key is appropriate (e.g. isn't being used by another existing
 // account) and that the account field of the rollover object matches the
 // account that verified the outer JWS.
@@ -720,53 +702,36 @@ func (wfe *WebFrontEndImpl) validKeyRollover(
 		return nil, prob
 	}
 
-	result := &rolloverOperation{OldKey: *oldKey}
+	var req rolloverRequest
-	if !features.Enabled(features.ACME13KeyRollover) {
+	if json.Unmarshal(innerPayload, &req) != nil {
-		// Unmarshal the inner JWS' key roll over request
+		wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverUnmarshalFailed"}).Inc()
-		var req rolloverRequest
+		return nil, probs.Malformed(
-		if json.Unmarshal(innerPayload, &req) != nil {
+			"Inner JWS payload did not parse as JSON key rollover object")
-			wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverUnmarshalFailed"}).Inc()
-			return nil, probs.Malformed(
-				"Inner JWS payload did not parse as JSON key rollover object")
-		}
-
-		// Verify that the key roll over request's NewKey *also* validates the inner
-		// JWS. So far we've only checked that the JWK embedded in the inner JWS valides
-		// the JWS.
-		if _, err := innerJWS.Verify(req.NewKey); err != nil {
-			wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverJWSNewKeyVerifyFailed"}).Inc()
-			return nil, probs.Malformed("Inner JWS does not verify with specified new key")
-		}
-		result.Account = req.Account
-		// Use the JWK specified in the key rollover request in pre-draft-13 key rollovers
-		result.NewKey = req.NewKey
-	} else {
-		var req rolloverRequestDraft13
-		if json.Unmarshal(innerPayload, &req) != nil {
-			wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverUnmarshalFailed"}).Inc()
-			return nil, probs.Malformed(
-				"Inner JWS payload did not parse as JSON key rollover object")
-		}
-
-		// If there's no oldkey specified fail before trying to use
-		// core.PublicKeyEqual on a nil argument.
-		if req.OldKey.Key == nil {
-			wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverWrongOldKey"}).Inc()
-			return nil, probs.Malformed("Inner JWS does not contain old key field matching current account key")
-		}
-
-		// If ACME13KeyRollover is enabled then we must validate that the inner JWS'
-		// rollover request specifies the correct oldKey.
-		if keysEqual, err := core.PublicKeysEqual(req.OldKey.Key, oldKey.Key); err != nil {
-			return nil, probs.Malformed("Unable to compare new and old keys: %s", err.Error())
-		} else if !keysEqual {
-			wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverWrongOldKey"}).Inc()
-			return nil, probs.Malformed("Inner JWS does not contain old key field matching current account key")
-		}
-		result.Account = req.Account
-		// Use the JWK pulled from the inner JWS as the new key for draft-13 key rollovers
-		result.NewKey = *jwk
 	}
 
-	return result, nil
+	// If there's no oldkey specified fail before trying to use
+	// core.PublicKeyEqual on a nil argument.
+	if req.OldKey.Key == nil {
+		wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverWrongOldKey"}).Inc()
+		return nil, probs.Malformed("Inner JWS does not contain old key field matching current account key")
+	}
+
+	// We must validate that the inner JWS' rollover request specifies the correct
+	// oldKey.
+	if keysEqual, err := core.PublicKeysEqual(req.OldKey.Key, oldKey.Key); err != nil {
+		return nil, probs.Malformed("Unable to compare new and old keys: %s", err.Error())
+	} else if !keysEqual {
+		wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverWrongOldKey"}).Inc()
+		return nil, probs.Malformed("Inner JWS does not contain old key field matching current account key")
+	}
+
+	// Return a rolloverOperation populated with the validated old JWK, the
+	// requested account, and the new JWK extracted from the inner JWS.
+	return &rolloverOperation{
+		rolloverRequest: rolloverRequest{
+			OldKey:  *oldKey,
+			Account: req.Account,
+		},
+		NewKey: *jwk,
+	}, nil
 }

wfe2/wfe_test.go

@@ -2233,13 +2233,6 @@ func TestKeyRollover(t *testing.T) {
 	existingKey, err := rsa.GenerateKey(rand.Reader, 2048)
 	test.AssertNotError(t, err, "Error creating random 2048 RSA key")
 
-	existingJWK := &jose.JSONWebKey{
-		Key:       &existingKey.PublicKey,
-		Algorithm: keyAlgForKey(t, existingKey),
-	}
-	existingJWKJSON, err := existingJWK.MarshalJSON()
-	test.AssertNotError(t, err, "Error marshaling random JWK")
-
 	newKeyBytes, err := ioutil.ReadFile("../test/test-key-5.der")
 	test.AssertNotError(t, err, "Failed to read ../test/test-key-5.der")
 	newKeyPriv, err := x509.ParsePKCS1PrivateKey(newKeyBytes)
@@ -2257,57 +2250,37 @@ func TestKeyRollover(t *testing.T) {
 		}`)
 
 	testCases := []struct {
-		Name              string
+		Name             string
-		ACME13KeyRollover bool
+		Payload          string
-		Payload           string
+		ExpectedResponse string
-		ExpectedResponse  string
+		NewKey           crypto.Signer
-		NewKey            crypto.Signer
+		ErrorStatType    string
-		ErrorStatType     string
 	}{
 		{
 			Name:    "Missing account URL",
-			Payload: `{"newKey":` + string(existingJWKJSON) + `}`,
+			Payload: `{"oldKey":` + test1KeyPublicJSON + `}`,
 			ExpectedResponse: `{
 		     "type": "` + probs.V2ErrorNS + `malformed",
 		     "detail": "Inner key rollover request specified Account \"\", but outer JWS has Key ID \"http://localhost/acme/acct/1\"",
 		     "status": 400
 		   }`,
-			NewKey:        existingKey,
+			NewKey:        newKeyPriv,
 			ErrorStatType: "KeyRolloverMismatchedAccount",
 		},
 		{
-			Name:    "Missing new key from inner payload",
+			Name:    "incorrect old key",
-			Payload: `{"account":"http://localhost/acme/acct/1"}`,
+			Payload: `{"oldKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/acct/1"}`,
 			ExpectedResponse: `{
 		     "type": "` + probs.V2ErrorNS + `malformed",
-		     "detail": "Inner JWS does not verify with specified new key",
+		     "detail": "Inner JWS does not contain old key field matching current account key",
 		     "status": 400
 		   }`,
-			ErrorStatType: "KeyRolloverJWSNewKeyVerifyFailed",
+			NewKey:        newKeyPriv,
-		},
+			ErrorStatType: "KeyRolloverWrongOldKey",
-		{
-			Name:    "New key is the same as the old key",
-			Payload: `{"newKey":{"kty":"RSA","n":"yNWVhtYEKJR21y9xsHV-PD_bYwbXSeNuFal46xYxVfRL5mqha7vttvjB_vc7Xg2RvgCxHPCqoxgMPTzHrZT75LjCwIW2K_klBYN8oYvTwwmeSkAz6ut7ZxPv-nZaT5TJhGk0NT2kh_zSpdriEJ_3vW-mqxYbbBmpvHqsa1_zx9fSuHYctAZJWzxzUZXykbWMWQZpEiE0J4ajj51fInEzVn7VxV-mzfMyboQjujPh7aNJxAWSq4oQEJJDgWwSh9leyoJoPpONHxh5nEE5AjE01FkGICSxjpZsF-w8hOTI3XXohUdu29Se26k2B0PolDSuj0GIQU6-W9TdLXSjBb2SpQ","e":"AQAB"},"account":"http://localhost/acme/acct/1"}`,
-			ExpectedResponse: `{
-		     "type": "` + probs.V2ErrorNS + `malformed",
-		     "detail": "New key specified by rollover request is the same as the old key",
-		     "status": 400
-		   }`,
-			ErrorStatType: "KeyRolloverUnchangedKey",
-		},
-		{
-			Name:    "Inner JWS signed by the wrong key",
-			Payload: `{"newKey":` + string(existingJWKJSON) + `,"account":"http://localhost/acme/acct/1"}`,
-			ExpectedResponse: `{
-		     "type": "` + probs.V2ErrorNS + `malformed",
-		     "detail": "Inner JWS does not verify with specified new key",
-		     "status": 400
-		   }`,
-			ErrorStatType: "KeyRolloverJWSNewKeyVerifyFailed",
 		},
 		{
 			Name:    "Valid key rollover request, key exists",
-			Payload: `{"newKey":` + string(existingJWKJSON) + `,"account":"http://localhost/acme/acct/1"}`,
+			Payload: `{"oldKey":` + test1KeyPublicJSON + `,"account":"http://localhost/acme/acct/1"}`,
 			ExpectedResponse: `{
                           "type": "urn:ietf:params:acme:error:malformed",
                           "detail": "New key is already in use for a different account",
@@ -2317,104 +2290,7 @@ func TestKeyRollover(t *testing.T) {
 		},
 		{
 			Name:    "Valid key rollover request",
-			Payload: `{"newKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/acct/1"}`,
+			Payload: `{"oldKey":` + test1KeyPublicJSON + `,"account":"http://localhost/acme/acct/1"}`,
-			ExpectedResponse: `{
-		     "id": 1,
-		     "key": ` + string(newJWKJSON) + `,
-		     "contact": [
-		       "mailto:person@mail.com"
-		     ],
-		     "agreement": "http://example.invalid/terms",
-		     "initialIp": "",
-		     "createdAt": "0001-01-01T00:00:00Z",
-		     "status": "valid"
-		   }`,
-			NewKey: newKeyPriv,
-		},
-		{
-			Name:    "Valid key rollover request, added ACME13KeyRollover compat",
-			Payload: `{"newKey":` + string(newJWKJSON) + `, "oldKey":` + test1KeyPublicJSON + `, "account":"http://localhost/acme/acct/1"}`,
-			ExpectedResponse: `{
-		     "id": 1,
-		     "key": ` + string(newJWKJSON) + `,
-		     "contact": [
-		       "mailto:person@mail.com"
-		     ],
-		     "agreement": "http://example.invalid/terms",
-		     "initialIp": "",
-		     "createdAt": "0001-01-01T00:00:00Z",
-		     "status": "valid"
-		   }`,
-			NewKey: newKeyPriv,
-		},
-		{
-			Name:              "ACME13KeyRollover, legacy rollover request",
-			ACME13KeyRollover: true,
-			Payload:           `{"newKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/acct/1"}`,
-			ExpectedResponse: `{
-		     "type": "` + probs.V2ErrorNS + `malformed",
-		     "detail": "Inner JWS does not contain old key field matching current account key",
-		     "status": 400
-		   }`,
-			NewKey:        newKeyPriv,
-			ErrorStatType: "KeyRolloverWrongOldKey",
-		},
-		{
-			Name:              "ACME13KeyRollover, Missing account URL",
-			ACME13KeyRollover: true,
-			Payload:           `{"oldKey":` + test1KeyPublicJSON + `}`,
-			ExpectedResponse: `{
-		     "type": "` + probs.V2ErrorNS + `malformed",
-		     "detail": "Inner key rollover request specified Account \"\", but outer JWS has Key ID \"http://localhost/acme/acct/1\"",
-		     "status": 400
-		   }`,
-			NewKey:        newKeyPriv,
-			ErrorStatType: "KeyRolloverMismatchedAccount",
-		},
-		{
-			Name:              "ACME13KeyRollover, incorrect old key",
-			ACME13KeyRollover: true,
-			Payload:           `{"oldKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/acct/1"}`,
-			ExpectedResponse: `{
-		     "type": "` + probs.V2ErrorNS + `malformed",
-		     "detail": "Inner JWS does not contain old key field matching current account key",
-		     "status": 400
-		   }`,
-			NewKey:        newKeyPriv,
-			ErrorStatType: "KeyRolloverWrongOldKey",
-		},
-		{
-			Name:              "ACME13KeyRollover, Valid key rollover request, key exists",
-			ACME13KeyRollover: true,
-			Payload:           `{"oldKey":` + test1KeyPublicJSON + `,"account":"http://localhost/acme/acct/1"}`,
-			ExpectedResponse: `{
-                          "type": "urn:ietf:params:acme:error:malformed",
-                          "detail": "New key is already in use for a different account",
-                          "status": 409
-                        }`,
-			NewKey: existingKey,
-		},
-		{
-			Name:              "ACME13KeyRollover, Valid key rollover request",
-			ACME13KeyRollover: true,
-			Payload:           `{"oldKey":` + test1KeyPublicJSON + `,"account":"http://localhost/acme/acct/1"}`,
-			ExpectedResponse: `{
-		     "id": 1,
-		     "key": ` + string(newJWKJSON) + `,
-		     "contact": [
-		       "mailto:person@mail.com"
-		     ],
-		     "agreement": "http://example.invalid/terms",
-		     "initialIp": "",
-		     "createdAt": "0001-01-01T00:00:00Z",
-		     "status": "valid"
-		   }`,
-			NewKey: newKeyPriv,
-		},
-		{
-			Name:              "ACME13KeyRollover, Valid key rollover request, legacy compat",
-			ACME13KeyRollover: true,
-			Payload:           `{"oldKey":` + test1KeyPublicJSON + `, "newKey":` + string(newJWKJSON) + `, "account":"http://localhost/acme/acct/1"}`,
 			ExpectedResponse: `{
 		     "id": 1,
 		     "key": ` + string(newJWKJSON) + `,
@@ -2432,10 +2308,6 @@ func TestKeyRollover(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.Name, func(t *testing.T) {
-			if tc.ACME13KeyRollover {
-				_ = features.Set(map[string]bool{"ACME13KeyRollover": true})
-				defer features.Reset()
-			}
 			wfe.stats.joseErrorCount.Reset()
 			responseWriter.Body.Reset()
 			_, _, inner := signRequestEmbed(t, tc.NewKey, "http://localhost/key-change", tc.Payload, wfe.nonceService)