From 12aa43508465f0da89ea2e60aa603d3b770beeba Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 2 Jul 2026 19:19:44 -0700 Subject: [PATCH] Drastically simplify changes to verify.go --- wfe2/verify.go | 115 +++++++++++++++++--------------------------- wfe2/verify_test.go | 24 ++++----- wfe2/wfe.go | 53 ++++++++++---------- 3 files changed, 82 insertions(+), 110 deletions(-) diff --git a/wfe2/verify.go b/wfe2/verify.go index 6bb528f9837..f6550c443df 100644 --- a/wfe2/verify.go +++ b/wfe2/verify.go @@ -300,9 +300,9 @@ func (wfe *WebFrontEndImpl) validPOSTURL( func (wfe *WebFrontEndImpl) matchJWSURLs(outer, inner jose.Header) error { // Verify that the outer JWS has a non-empty URL header. This is strictly // defensive since the expectation is that endpoints using `matchJWSURLs` - // have received at least one of their JWS from an account-authenticated - // verifier, which checks the outer JWS has the expected URL header before - // processing the inner JWS. + // have received at least one of their JWS from calling validPOSTForAccount(), + // which checks the outer JWS has the expected URL header before processing + // the inner JWS. outerURL, ok := outer.ExtraHeaders[jose.HeaderKey("url")].(string) if !ok || len(outerURL) == 0 { wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverOuterJWSNoURL"}).Inc() @@ -489,19 +489,11 @@ 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, - request *http.Request, - logEvent *web.RequestEvent) (*jose.JSONWebKey, *core.Registration, error) { - return wfe.lookupJWKUsing(header, ctx, request, logEvent, wfe.accountGetter) -} - -func (wfe *WebFrontEndImpl) lookupJWKUsing( header jose.Header, - ctx context.Context, request *http.Request, - logEvent *web.RequestEvent, - accountGetter AccountGetter) (*jose.JSONWebKey, *core.Registration, error) { + 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. if err := wfe.enforceJWSAuthType(header, embeddedKeyID); err != nil { @@ -610,27 +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( - jws *bJSONWebSignature, - request *http.Request, ctx context.Context, - logEvent *web.RequestEvent) ([]byte, *bJSONWebSignature, *core.Registration, error) { - return wfe.validJWSForAccountUsing(jws, request, ctx, logEvent, wfe.accountGetter) -} - -func (wfe *WebFrontEndImpl) validJWSForAccountUsing( jws *bJSONWebSignature, request *http.Request, - ctx context.Context, - logEvent *web.RequestEvent, - accountGetter AccountGetter) ([]byte, *bJSONWebSignature, *core.Registration, error) { + 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.lookupJWKUsing( - jws.Signatures[0].Header, - ctx, - request, - logEvent, - accountGetter, - ) + pubKey, account, err := wfe.lookupJWK(ctx, jws.Signatures[0].Header, request, accountGetter, logEvent) if err != nil { return nil, nil, nil, err } @@ -644,65 +622,62 @@ func (wfe *WebFrontEndImpl) validJWSForAccountUsing( return payload, jws, account, nil } -// validPOSTForAccount checks a POST request against the configured account -// getter, which may be the WFE-local account cache. Use -// validPOSTForCurrentAccount before account-authenticated requests cross -// issuance or account mutation boundaries. +// 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) -} -// validPOSTForCurrentAccount checks a POST request against current SA account -// state, bypassing the WFE-local account cache. Use it before account-authenticated -// requests cross issuance or account mutation boundaries. -func (wfe *WebFrontEndImpl) validPOSTForCurrentAccount( - request *http.Request, - ctx context.Context, - logEvent *web.RequestEvent) ([]byte, *bJSONWebSignature, *core.Registration, error) { - return wfe.validPOSTForAccountUsing(request, ctx, logEvent, wfe.sa) + // 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) } -func (wfe *WebFrontEndImpl) validPOSTForAccountUsing( - request *http.Request, +// validPOSTAsGETForAccount checks that a given POST request is valid using +// `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( ctx context.Context, - logEvent *web.RequestEvent, - accountGetter AccountGetter) ([]byte, *bJSONWebSignature, *core.Registration, error) { + request *http.Request, + logEvent *web.RequestEvent) (*core.Registration, error) { // Parse the JWS from the POST request jws, err := wfe.parseJWSRequest(request) if err != nil { - return nil, nil, nil, err + return nil, err } - return wfe.validJWSForAccountUsing(jws, request, ctx, logEvent, accountGetter) -} -// validPOSTAsGETForAccount checks that a given POST request is valid using -// the configured account getter, which may be the WFE-local account cache. 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). -func (wfe *WebFrontEndImpl) validPOSTAsGETForAccount( - request *http.Request, - ctx context.Context, - logEvent *web.RequestEvent) (*core.Registration, error) { - body, _, reg, err := wfe.validPOSTForAccount(request, ctx, logEvent) + // 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. @@ -793,9 +768,9 @@ type rolloverOperation struct { // validKeyRollover checks if the innerJWS is a valid key rollover operation // given the outer JWS that carried it. It is assumed that the outerJWS has -// already been validated per the normal ACME process. It is *critical* this is -// the case since `validKeyRollover` does not check the outerJWS signature. This -// function checks that: +// already been validated per the normal ACME process using `validPOSTForAccount`. +// It is *critical* this is the case since `validKeyRollover` does not check the +// outerJWS signature. This function checks that: // 1) the inner JWS is valid and well formed // 2) the inner JWS has the same "url" header as the outer JWS // 3) the inner JWS is self-authenticated with an embedded JWK @@ -844,8 +819,8 @@ func (wfe *WebFrontEndImpl) validKeyRollover( return nil, berrors.MalformedError("Inner JWS does not verify with embedded JWK") } // NOTE(@cpu): we do not stomp the web.RequestEvent's payload here since that is set - // from the outerJWS by the account-authenticated verifier and contains the - // inner JWS and inner payload already. + // from the outerJWS in validPOSTForAccount and contains the inner JWS and inner + // payload already. // Verify that the outer and inner JWS protected URL headers match if err := wfe.matchJWSURLs(outerJWS.Signatures[0].Header, innerJWS.Signatures[0].Header); err != nil { diff --git a/wfe2/verify_test.go b/wfe2/verify_test.go index 5ce13ab045d..a2a81dd9a63 100644 --- a/wfe2/verify_test.go +++ b/wfe2/verify_test.go @@ -1178,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) @@ -1395,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) @@ -1478,11 +1478,11 @@ func setupAuthOnlyWFE( inmemNonceService := &inmemnonce.NonceService{Impl: nonceService} freshGetter := &staticRegistrationGetter{registration: freshAccount} wfe := WebFrontEndImpl{ - sa: freshGetter, - rnc: inmemNonceService, - rncKey: rncKey, - accountGetter: rejectingRegistrationGetter{t: t}, - stats: initStats(metrics.NoopRegisterer), + sa: freshGetter, + rnc: inmemNonceService, + rncKey: rncKey, + accountCache: rejectingRegistrationGetter{t: t}, + stats: initStats(metrics.NoopRegisterer), } return wfe, requestSigner{t, inmemNonceService.AsSource()}, freshGetter @@ -1497,7 +1497,7 @@ func TestValidPOSTForCurrentAccountRejectsCachedDeactivatedAccount(t *testing.T) _, _, body := signer.byKeyID(1, nil, "http://localhost/test", "{}") request := makePostRequestWithPath("test", body) - _, _, _, err := wfe.validPOSTForCurrentAccount(request, ctx, newRequestEvent()) + _, _, _, 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) @@ -1512,7 +1512,7 @@ func TestValidPOSTForCurrentAccountRejectsCachedPreRolloverKey(t *testing.T) { _, _, body := signer.byKeyID(1, nil, "http://localhost/test", "{}") request := makePostRequestWithPath("test", body) - _, _, _, err := wfe.validPOSTForCurrentAccount(request, ctx, newRequestEvent()) + _, _, _, 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) @@ -1557,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) @@ -1595,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"}` @@ -1606,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") diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 049c70e5a92..978c104eee9 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -105,11 +105,11 @@ type WebFrontEndImpl struct { rnc nonce.Redeemer // rncKey is the HMAC key used to derive the prefix of nonce backends used // for nonce redemption. - rncKey []byte - accountGetter AccountGetter - log blog.Logger - clk clock.Clock - stats wfe2Stats + rncKey []byte + accountCache AccountGetter + log blog.Logger + clk clock.Clock + stats wfe2Stats // certificateChains maps IssuerNameIDs to slice of []byte containing a leading // newline and one or more PEM encoded certificates separated by a newline, @@ -190,7 +190,7 @@ type accountCachePurger interface { } func (wfe *WebFrontEndImpl) purgeCachedAccount(regID int64) { - cache, ok := wfe.accountGetter.(accountCachePurger) + cache, ok := wfe.accountCache.(accountCachePurger) if ok { cache.purgeRegistration(regID) } @@ -213,7 +213,7 @@ func NewWebFrontEndImpl( gnc nonce.Getter, rnc nonce.Redeemer, rncKey []byte, - accountGetter AccountGetter, + accountCache AccountGetter, limiter *ratelimits.Limiter, txnBuilder *ratelimits.TransactionBuilder, certProfiles map[string]string, @@ -265,7 +265,7 @@ func NewWebFrontEndImpl( gnc: gnc, rnc: rnc, rncKey: rncKey, - accountGetter: accountGetter, + accountCache: accountCache, limiter: limiter, txnBuilder: txnBuilder, certProfiles: certProfiles, @@ -549,7 +549,7 @@ func (wfe *WebFrontEndImpl) Directory( directoryEndpoints["renewalInfo"] = strings.TrimRight(renewalInfoPath, "/") if request.Method == http.MethodPost { - acct, err := wfe.validPOSTAsGETForAccount(request, ctx, logEvent) + acct, err := wfe.validPOSTAsGETForAccount(ctx, request, logEvent) if err != nil { wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) return @@ -609,7 +609,7 @@ func (wfe *WebFrontEndImpl) Nonce( response http.ResponseWriter, request *http.Request) { if request.Method == http.MethodPost { - acct, err := wfe.validPOSTAsGETForAccount(request, ctx, logEvent) + acct, err := wfe.validPOSTAsGETForAccount(ctx, request, logEvent) if err != nil { wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) return @@ -1026,14 +1026,8 @@ func (wfe *WebFrontEndImpl) revokeCertBySubscriberKey( request *http.Request, logEvent *web.RequestEvent) error { // For Key ID revocations we authenticate the outer JWS by using - // `validJWSForAccount` similar to other WFE endpoints - jwsBody, _, acct, err := wfe.validJWSForAccountUsing( - outerJWS, - request, - ctx, - logEvent, - wfe.sa, - ) + // `validJWSForAccount` similar to other WFE endpoints, bypassing the cache. + jwsBody, _, acct, err := wfe.validJWSForAccount(ctx, outerJWS, request, wfe.sa, logEvent) if err != nil { return err } @@ -1126,7 +1120,7 @@ func (wfe *WebFrontEndImpl) RevokeCertificate( // The ACME specification handles the verification of revocation requests // differently from other endpoints. For this reason we do *not* immediately - // authenticate the request against an account like most other endpoints. + // call `wfe.validPOSTForAccount` like all of the other endpoints. // For this endpoint we need to accept a JWS with an embedded JWK, or a JWS // with an embedded key ID, handling each case differently in terms of which // certificates are authorized to be revoked by the requester @@ -1344,9 +1338,10 @@ func (wfe *WebFrontEndImpl) postChallenge( authz core.Authorization, challengeIndex int, logEvent *web.RequestEvent) { - body, _, currAcct, err := wfe.validPOSTForCurrentAccount(request, ctx, logEvent) + body, _, currAcct, err := wfe.validPOSTForAccount(ctx, request, logEvent) addRequesterHeader(response, logEvent.Requester) if err != nil { + // validPOSTForAccount handles its own setting of logEvent.Errors wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) return } @@ -1435,9 +1430,10 @@ func (wfe *WebFrontEndImpl) Account( logEvent *web.RequestEvent, response http.ResponseWriter, request *http.Request) { - body, _, currAcct, err := wfe.validPOSTForCurrentAccount(request, ctx, logEvent) + body, _, currAcct, err := wfe.validPOSTForAccount(ctx, request, logEvent) addRequesterHeader(response, logEvent.Requester) if err != nil { + // validPOSTForAccount handles its own setting of logEvent.Errors wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) return } @@ -1590,7 +1586,7 @@ func (wfe *WebFrontEndImpl) Authorization( // B) a POST-as-GET to query the authorization details if request.Method == "POST" { // Both POST options need to be authenticated by an account - body, _, acct, err := wfe.validPOSTForCurrentAccount(request, ctx, logEvent) + body, _, acct, err := wfe.validPOSTForAccount(ctx, request, logEvent) addRequesterHeader(response, logEvent.Requester) if err != nil { wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) @@ -1706,7 +1702,7 @@ func (wfe *WebFrontEndImpl) Certificate(ctx context.Context, logEvent *web.Reque // Any POSTs to the Certificate endpoint should be POST-as-GET requests. There are // no POSTs with a body allowed for this endpoint. if request.Method == "POST" { - acct, err := wfe.validPOSTAsGETForAccount(request, ctx, logEvent) + acct, err := wfe.validPOSTAsGETForAccount(ctx, request, logEvent) if err != nil { wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) return @@ -1960,8 +1956,8 @@ func (wfe *WebFrontEndImpl) KeyRollover( logEvent *web.RequestEvent, response http.ResponseWriter, request *http.Request) { - // Validate the outer JWS against current account state before changing keys. - outerBody, outerJWS, acct, err := wfe.validPOSTForCurrentAccount(request, ctx, logEvent) + // Validate the outer JWS on the key rollover in standard fashion. + outerBody, outerJWS, acct, err := wfe.validPOSTForAccount(ctx, request, logEvent) addRequesterHeader(response, logEvent.Requester) if err != nil { wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) @@ -2348,9 +2344,10 @@ func (wfe *WebFrontEndImpl) NewOrder( logEvent *web.RequestEvent, response http.ResponseWriter, request *http.Request) { - body, _, acct, err := wfe.validPOSTForCurrentAccount(request, ctx, logEvent) + body, _, acct, err := wfe.validPOSTForAccount(ctx, request, logEvent) addRequesterHeader(response, logEvent.Requester) if err != nil { + // validPOSTForAccount handles its own setting of logEvent.Errors wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) return } @@ -2526,7 +2523,7 @@ func (wfe *WebFrontEndImpl) GetOrder(ctx context.Context, logEvent *web.RequestE // Any POSTs to the Order endpoint should be POST-as-GET requests. There are // no POSTs with a body allowed for this endpoint. if request.Method == http.MethodPost { - acct, err := wfe.validPOSTAsGETForAccount(request, ctx, logEvent) + acct, err := wfe.validPOSTAsGETForAccount(ctx, request, logEvent) if err != nil { wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err) return @@ -2603,7 +2600,7 @@ func (wfe *WebFrontEndImpl) GetOrder(ctx context.Context, logEvent *web.RequestE func (wfe *WebFrontEndImpl) FinalizeOrder(ctx context.Context, logEvent *web.RequestEvent, response http.ResponseWriter, request *http.Request) { // Validate the POST body signature and get the authenticated account for this // finalize order request - body, _, acct, err := wfe.validPOSTForCurrentAccount(request, ctx, logEvent) + body, _, acct, err := wfe.validPOSTForAccount(ctx, request, logEvent) addRequesterHeader(response, logEvent.Requester) if err != nil { wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to validate JWS"), err)