Restrict WFE account cache to POST-as-GET requests

test/integration/key_rollover_test.go

@@ -41,7 +41,7 @@ func TestAccountKeyChange(t *testing.T) {
 	key3, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
 	test.AssertNotError(t, err, "creating P-384 account key")
 
-	acct3, err := c.AccountKeyChange(acct1, key3)
+	acct3, err := c.AccountKeyChange(acct2, key3)
 	test.AssertNotError(t, err, "rolling over account key")
 	test.AssertEquals(t, acct3.URL, acct1.URL)
 }

wfe2/cache.go

@@ -98,6 +98,12 @@ func (ac *accountCache) GetRegistration(ctx context.Context, regID *sapb.Registr
 	return copied, nil
 }
 
+func (ac *accountCache) purgeRegistration(regID int64) {
+	ac.Lock()
+	ac.cache.Remove(regID)
+	ac.Unlock()
+}
+
 func (ac *accountCache) queryAndStore(ctx context.Context, regID *sapb.RegistrationID) (*corepb.Registration, error) {
 	account, err := ac.under.GetRegistration(ctx, regID)
 	if err != nil {

wfe2/cache_test.go

@@ -107,6 +107,22 @@ func TestCacheExpires(t *testing.T) {
 	test.AssertEquals(t, len(backend.requests), 2)
 }
 
+func TestCachePurgeRegistration(t *testing.T) {
+	ctx := context.Background()
+	backend := &recordingBackend{}
+	cache := NewAccountCache(backend, 10, time.Second, clock.NewFake(), metrics.NoopRegisterer)
+
+	_, err := cache.GetRegistration(ctx, &sapb.RegistrationID{Id: 1234})
+	test.AssertNotError(t, err, "getting registration")
+	test.AssertEquals(t, len(backend.requests), 1)
+
+	cache.purgeRegistration(1234)
+
+	_, err = cache.GetRegistration(ctx, &sapb.RegistrationID{Id: 1234})
+	test.AssertNotError(t, err, "getting registration")
+	test.AssertEquals(t, len(backend.requests), 2)
+}
+
 type wrongIDBackend struct{}
 
 func (wib wrongIDBackend) GetRegistration(

wfe2/verify.go

@@ -489,9 +489,10 @@ func (wfe *WebFrontEndImpl) acctIDFromURL(acctURL string, request *http.Request)
 // authentication and does not contain an embedded JWK. Callers should have
 // acquired headers from a bJSONWebSignature.
 func (wfe *WebFrontEndImpl) lookupJWK(
-	header jose.Header,
 	ctx context.Context,
+	header jose.Header,
 	request *http.Request,
+	accountGetter AccountGetter,
 	logEvent *web.RequestEvent) (*jose.JSONWebKey, *core.Registration, error) {
 	// We expect the request to be using an embedded Key ID auth type and to not
 	// contain the mutually exclusive embedded JWK.
@@ -506,8 +507,8 @@ func (wfe *WebFrontEndImpl) lookupJWK(
 		return nil, nil, err
 	}
 
-	// Try to find the account for this account ID
-	account, err := wfe.accountGetter.GetRegistration(ctx, &sapb.RegistrationID{Id: accountID})
+	// Try to find the account for this account ID.
+	account, err := accountGetter.GetRegistration(ctx, &sapb.RegistrationID{Id: accountID})
 	if err != nil {
 		// If the account isn't found, return a suitable error
 		if errors.Is(err, berrors.NotFound) {
@@ -601,12 +602,13 @@ func (wfe *WebFrontEndImpl) validJWSForKey(
 // JSONWebSignature, and a pointer to the JWK's associated account. If any of
 // these conditions are not met or an error occurs only a error is returned.
 func (wfe *WebFrontEndImpl) validJWSForAccount(
+	ctx context.Context,
 	jws *bJSONWebSignature,
 	request *http.Request,
-	ctx context.Context,
+	accountGetter AccountGetter,
 	logEvent *web.RequestEvent) ([]byte, *bJSONWebSignature, *core.Registration, error) {
 	// Lookup the account and JWK for the key ID that authenticated the JWS
-	pubKey, account, err := wfe.lookupJWK(jws.Signatures[0].Header, ctx, request, logEvent)
+	pubKey, account, err := wfe.lookupJWK(ctx, jws.Signatures[0].Header, request, accountGetter, logEvent)
 	if err != nil {
 		return nil, nil, nil, err
 	}
@@ -620,43 +622,62 @@ func (wfe *WebFrontEndImpl) validJWSForAccount(
 	return payload, jws, account, nil
 }
 
-// validPOSTForAccount checks that a given POST request has a valid JWS
-// using `validJWSForAccount`. If valid, the authenticated JWS body and the
-// registration that authenticated the body are returned. Otherwise a error is
-// returned. The returned JWS body may be empty if the request is a POST-as-GET
-// request.
+// validPOSTForAccount checks that a given POST request has a valid JWS using
+// `validJWSForAccount` and the WFE's database/SA connection. If valid, the
+// authenticated JWS body and the registration that authenticated the body are
+// returned. Otherwise an error is returned.
+//
+// This function is not ideal for validating POST-as-GET requests; although it
+// will work correctly, it will not validate that the request body is empty.
+// Those requests should be validated via validPOSTAsGETForAccount.
 func (wfe *WebFrontEndImpl) validPOSTForAccount(
-	request *http.Request,
 	ctx context.Context,
+	request *http.Request,
 	logEvent *web.RequestEvent) ([]byte, *bJSONWebSignature, *core.Registration, error) {
 	// Parse the JWS from the POST request
 	jws, err := wfe.parseJWSRequest(request)
 	if err != nil {
 		return nil, nil, nil, err
 	}
-	return wfe.validJWSForAccount(jws, request, ctx, logEvent)
+
+	// Use wfe.sa (i.e. the real database) as the AccountGetter for all POST
+	// requests, as POSTs may modify the database. This prevents a stale account
+	// cache from letting an old key perform mutating operations, like rotating
+	// to a new key.
+	return wfe.validJWSForAccount(ctx, jws, request, wfe.sa, logEvent)
 }
 
 // validPOSTAsGETForAccount checks that a given POST request is valid using
-// `validPOSTForAccount`. It additionally validates that the JWS request payload
-// is empty, indicating that it is a POST-as-GET request per ACME draft 15+
-// section 6.3 "GET and POST-as-GET requests". If a non empty payload is
-// provided in the JWS the invalidPOSTAsGETErr error is returned. This
-// function is useful only for endpoints that do not need to handle both POSTs
-// with a body and POST-as-GET requests (e.g. Order, Certificate).
+// `validJWSForAccount` and the wfe's read-through account cache. It
+// additionally validates that the JWS request payload is empty. If a non empty
+// payload is provided, it returns MalformedError.
+//
+// This function must only be used to validate POST-as-GET requests, it must
+// not be used to validate mutating POST requests.
 func (wfe *WebFrontEndImpl) validPOSTAsGETForAccount(
-	request *http.Request,
 	ctx context.Context,
+	request *http.Request,
 	logEvent *web.RequestEvent) (*core.Registration, error) {
-	// Call validPOSTForAccount to verify the JWS and extract the body.
-	body, _, reg, err := wfe.validPOSTForAccount(request, ctx, logEvent)
+	// Parse the JWS from the POST request
+	jws, err := wfe.parseJWSRequest(request)
 	if err != nil {
 		return nil, err
 	}
+
+	// Use the account cache to look up the account. We are willing to accept
+	// idempotent read-only POST-as-GET requests authenticated by a key in the
+	// possibly-stale account cache, because such requests are relatively safe
+	// and relatively high-volume.
+	body, _, reg, err := wfe.validJWSForAccount(ctx, jws, request, wfe.accountCache, logEvent)
+	if err != nil {
+		return nil, err
+	}
+
 	// Verify the POST-as-GET payload is empty
 	if string(body) != "" {
 		return nil, berrors.MalformedError("POST-as-GET requests must have an empty payload")
 	}
+
 	// To make log analysis easier we choose to elevate the pseudo ACME HTTP
 	// method "POST-as-GET" to the logEvent's Method, replacing the
 	// http.MethodPost value.

wfe2/verify_test.go

@@ -24,9 +24,12 @@ import (
 	"github.com/letsencrypt/boulder/goodkey"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
 	"github.com/letsencrypt/boulder/grpc/noncebalancer"
+	"github.com/letsencrypt/boulder/metrics"
+	"github.com/letsencrypt/boulder/nonce"
 	noncepb "github.com/letsencrypt/boulder/nonce/proto"
 	sapb "github.com/letsencrypt/boulder/sa/proto"
 	"github.com/letsencrypt/boulder/test"
+	inmemnonce "github.com/letsencrypt/boulder/test/inmem/nonce"
 	"github.com/letsencrypt/boulder/web"
 )
 
@@ -1175,7 +1178,7 @@ func TestLookupJWK(t *testing.T) {
 			in := tc.JWS.Signatures[0].Header
 			inputLogEvent := newRequestEvent()
 
-			gotJWK, gotAcct, gotErr := wfe.lookupJWK(in, context.Background(), tc.Request, inputLogEvent)
+			gotJWK, gotAcct, gotErr := wfe.lookupJWK(t.Context(), in, tc.Request, wfe.accountCache, inputLogEvent)
 			if tc.WantErrDetail == "" {
 				if gotErr != nil {
 					t.Fatalf("lookupJWK(%#v) = %#v, want nil", in, gotErr)
@@ -1392,7 +1395,7 @@ func TestValidPOSTForAccount(t *testing.T) {
 			wfe.stats.joseErrorCount.Reset()
 			inputLogEvent := newRequestEvent()
 
-			gotPayload, gotJWS, gotAcct, gotErr := wfe.validPOSTForAccount(tc.Request, context.Background(), inputLogEvent)
+			gotPayload, gotJWS, gotAcct, gotErr := wfe.validPOSTForAccount(t.Context(), tc.Request, inputLogEvent)
 			if tc.WantErrDetail == "" {
 				if gotErr != nil {
 					t.Fatalf("validPOSTForAccount(%#v) = %#v, want nil", tc.Request, gotErr)
@@ -1420,8 +1423,103 @@ func TestValidPOSTForAccount(t *testing.T) {
 	}
 }
 
+type staticRegistrationGetter struct {
+	sapb.StorageAuthorityReadOnlyClient
+	registration *corepb.Registration
+	calls        int
+}
+
+func (getter *staticRegistrationGetter) GetRegistration(
+	_ context.Context,
+	in *sapb.RegistrationID,
+	_ ...grpc.CallOption,
+) (*corepb.Registration, error) {
+	getter.calls++
+	if in.Id == getter.registration.Id {
+		return getter.registration, nil
+	}
+	return nil, berrors.NotFound
+}
+
+type rejectingRegistrationGetter struct {
+	sapb.StorageAuthorityReadOnlyClient
+	t *testing.T
+}
+
+func (getter rejectingRegistrationGetter) GetRegistration(
+	_ context.Context,
+	_ *sapb.RegistrationID,
+	_ ...grpc.CallOption,
+) (*corepb.Registration, error) {
+	getter.t.Fatal("validPOSTForCurrentAccount used cached account getter")
+	return nil, nil
+}
+
+func registrationForAuthTest(key []byte, status core.AcmeStatus) *corepb.Registration {
+	return &corepb.Registration{
+		Id:        1,
+		Key:       key,
+		Agreement: agreementURL,
+		Status:    string(status),
+	}
+}
+
+func setupAuthOnlyWFE(
+	t *testing.T,
+	freshAccount *corepb.Registration,
+) (WebFrontEndImpl, requestSigner, *staticRegistrationGetter) {
+	t.Helper()
+
+	rncKey := []byte("b8c758dd85e113ea340ce0b3a99f389d40a308548af94d1730a7692c1874f1f")
+	noncePrefix := nonce.DerivePrefix("192.168.1.1:8080", rncKey)
+	nonceService, err := nonce.NewNonceService(metrics.NoopRegisterer, 100, noncePrefix)
+	test.AssertNotError(t, err, "making nonceService")
+
+	inmemNonceService := &inmemnonce.NonceService{Impl: nonceService}
+	freshGetter := &staticRegistrationGetter{registration: freshAccount}
+	wfe := WebFrontEndImpl{
+		sa:           freshGetter,
+		rnc:          inmemNonceService,
+		rncKey:       rncKey,
+		accountCache: rejectingRegistrationGetter{t: t},
+		stats:        initStats(metrics.NoopRegisterer),
+	}
+
+	return wfe, requestSigner{t, inmemNonceService.AsSource()}, freshGetter
+}
+
+func TestValidPOSTForCurrentAccountRejectsCachedDeactivatedAccount(t *testing.T) {
+	wfe, signer, freshGetter := setupAuthOnlyWFE(
+		t,
+		registrationForAuthTest([]byte(test1KeyPublicJSON), core.StatusDeactivated),
+	)
+
+	_, _, body := signer.byKeyID(1, nil, "http://localhost/test", "{}")
+	request := makePostRequestWithPath("test", body)
+
+	_, _, _, err := wfe.validPOSTForAccount(ctx, request, newRequestEvent())
+	test.AssertErrorIs(t, err, berrors.Unauthorized)
+	test.AssertContains(t, err.Error(), `Account is not valid, has status "deactivated"`)
+	test.AssertEquals(t, freshGetter.calls, 1)
+}
+
+func TestValidPOSTForCurrentAccountRejectsCachedPreRolloverKey(t *testing.T) {
+	wfe, signer, freshGetter := setupAuthOnlyWFE(
+		t,
+		registrationForAuthTest([]byte(test2KeyPublicJSON), core.StatusValid),
+	)
+
+	_, _, body := signer.byKeyID(1, nil, "http://localhost/test", "{}")
+	request := makePostRequestWithPath("test", body)
+
+	_, _, _, err := wfe.validPOSTForAccount(ctx, request, newRequestEvent())
+	test.AssertErrorIs(t, err, berrors.Malformed)
+	test.AssertContains(t, err.Error(), "JWS verification error")
+	test.AssertEquals(t, freshGetter.calls, 1)
+}
+
 // TestValidPOSTAsGETForAccount tests POST-as-GET processing. Because
-// wfe.validPOSTAsGETForAccount calls `wfe.validPOSTForAccount` to do all
+// wfe.validPOSTAsGETForAccount uses the configured account getter for all
 // processing except the empty body test we do not duplicate the
 // `TestValidPOSTForAccount` testcases here.
 func TestValidPOSTAsGETForAccount(t *testing.T) {
@@ -1459,7 +1557,7 @@ func TestValidPOSTAsGETForAccount(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.Name, func(t *testing.T) {
 			ev := newRequestEvent()
-			_, gotErr := wfe.validPOSTAsGETForAccount(tc.Request, context.Background(), ev)
+			_, gotErr := wfe.validPOSTAsGETForAccount(t.Context(), tc.Request, ev)
 			if tc.WantErrDetail == "" {
 				if gotErr != nil {
 					t.Fatalf("validPOSTAsGETForAccount(%#v) = %#v, want nil", tc.Request, gotErr)
@@ -1497,7 +1595,7 @@ func (sa mockSADifferentStoredKey) GetRegistration(_ context.Context, _ *sapb.Re
 func TestValidPOSTForAccountSwappedKey(t *testing.T) {
 	wfe, _, signer := setupWFE(t)
 	wfe.sa = &mockSADifferentStoredKey{}
-	wfe.accountGetter = wfe.sa
+	wfe.accountCache = wfe.sa
 	event := newRequestEvent()
 
 	payload := `{"resource":"ima-payload"}`
@@ -1508,7 +1606,7 @@ func TestValidPOSTForAccountSwappedKey(t *testing.T) {
 	// Ensure that ValidPOSTForAccount produces an error since the
 	// mockSADifferentStoredKey will return a different key than the one we used to
 	// sign the request
-	_, _, _, err := wfe.validPOSTForAccount(request, ctx, event)
+	_, _, _, err := wfe.validPOSTForAccount(ctx, request, event)
 	test.AssertError(t, err, "No error returned for request signed by wrong key")
 	test.AssertErrorIs(t, err, berrors.Malformed)
 	test.AssertContains(t, err.Error(), "JWS verification error")