From 53f3cb91c0ee73e9e93a92626a2af59183894a41 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 21 Nov 2024 16:51:36 -0800 Subject: [PATCH 1/2] wfe: rename deprecated paths and handlers (#7837) Now that the paths with an account (and no `-v3`) are the default, rename the old-style path constants and handlers to reflect that they are deprecated. Part of #7683. --- wfe2/wfe.go | 65 +++++++++++++++++++++++++----------------------- wfe2/wfe_test.go | 62 ++++++++++++++++++++++----------------------- 2 files changed, 65 insertions(+), 62 deletions(-) diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 3b1315c45a4..2a135c07e09 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -56,19 +56,22 @@ const ( newAcctPath = "/acme/new-acct" acctPath = "/acme/acct/" // When we moved to authzv2, we used a "-v3" suffix to avoid confusion - // regarding ACMEv2. - authzPath = "/acme/authz-v3/" - authzPathWithAcct = "/acme/authz/" - challengePath = "/acme/chall-v3/" - challengePathWithAcct = "/acme/chall/" - certPath = "/acme/cert/" - revokeCertPath = "/acme/revoke-cert" - buildIDPath = "/build" - rolloverPath = "/acme/key-change" - newNoncePath = "/acme/new-nonce" - newOrderPath = "/acme/new-order" - orderPath = "/acme/order/" - finalizeOrderPath = "/acme/finalize/" + // regarding ACMEv2. More recently we moved back to using plain `/acme/authz/` + // and `/acme/chall/`, so the `-v3` paths are deprecated. + // TODO(#7683): Remove authz-v3 and chall-v3 once the new paths have been + // the default in prod for 30 days. + deprecatedAuthzPath = "/acme/authz-v3/" + authzPathWithAcct = "/acme/authz/" + deprecatedChallengePath = "/acme/chall-v3/" + challengePathWithAcct = "/acme/chall/" + certPath = "/acme/cert/" + revokeCertPath = "/acme/revoke-cert" + buildIDPath = "/build" + rolloverPath = "/acme/key-change" + newNoncePath = "/acme/new-nonce" + newOrderPath = "/acme/new-order" + orderPath = "/acme/order/" + finalizeOrderPath = "/acme/finalize/" getAPIPrefix = "/get/" getOrderPath = getAPIPrefix + "order/" @@ -434,15 +437,15 @@ func (wfe *WebFrontEndImpl) Handler(stats prometheus.Registerer, oTelHTTPOptions // TODO(@cpu): After November 1st, 2020 support for "GET" to the following // endpoints will be removed, leaving only POST-as-GET support. wfe.HandleFunc(m, orderPath, wfe.GetOrder, "GET", "POST") - wfe.HandleFunc(m, authzPath, wfe.AuthorizationHandler, "GET", "POST") - wfe.HandleFunc(m, authzPathWithAcct, wfe.AuthorizationHandlerWithAccount, "GET", "POST") - wfe.HandleFunc(m, challengePath, wfe.ChallengeHandler, "GET", "POST") - wfe.HandleFunc(m, challengePathWithAcct, wfe.ChallengeHandlerWithAccount, "GET", "POST") + wfe.HandleFunc(m, deprecatedAuthzPath, wfe.DeprecatedAuthorizationHandler, "GET", "POST") + wfe.HandleFunc(m, authzPathWithAcct, wfe.AuthorizationHandler, "GET", "POST") + wfe.HandleFunc(m, deprecatedChallengePath, wfe.DeprecatedChallengeHandler, "GET", "POST") + wfe.HandleFunc(m, challengePathWithAcct, wfe.ChallengeHandler, "GET", "POST") wfe.HandleFunc(m, certPath, wfe.Certificate, "GET", "POST") // Boulder-specific GET-able resource endpoints wfe.HandleFunc(m, getOrderPath, wfe.GetOrder, "GET") - wfe.HandleFunc(m, getAuthzPath, wfe.AuthorizationHandler, "GET") - wfe.HandleFunc(m, getChallengePath, wfe.ChallengeHandler, "GET") + wfe.HandleFunc(m, getAuthzPath, wfe.DeprecatedAuthorizationHandler, "GET") + wfe.HandleFunc(m, getChallengePath, wfe.DeprecatedChallengeHandler, "GET") wfe.HandleFunc(m, getCertPath, wfe.Certificate, "GET") // Endpoint for draft-ietf-acme-ari @@ -1087,9 +1090,9 @@ func (wfe *WebFrontEndImpl) RevokeCertificate( response.WriteHeader(http.StatusOK) } -// ChallengeHandler handles POST requests to challenge URLs of the form /acme/chall-v3//. +// DeprecatedChallengeHandler handles POST requests to challenge URLs of the form /acme/chall-v3//. // Such requests are clients' responses to the server's challenges. -func (wfe *WebFrontEndImpl) ChallengeHandler( +func (wfe *WebFrontEndImpl) DeprecatedChallengeHandler( ctx context.Context, logEvent *web.RequestEvent, response http.ResponseWriter, @@ -1100,11 +1103,11 @@ func (wfe *WebFrontEndImpl) ChallengeHandler( return } - wfe.Challenge(ctx, logEvent, challengePath, response, request, slug[0], slug[1]) + wfe.Challenge(ctx, logEvent, deprecatedChallengePath, response, request, slug[0], slug[1]) } -// ChallengeHandlerWithAccount handles POST requests to challenge URLs of the form /acme/chall/{regID}/{authzID}/{challID}. -func (wfe *WebFrontEndImpl) ChallengeHandlerWithAccount( +// ChallengeHandler handles POST requests to challenge URLs of the form /acme/chall/{regID}/{authzID}/{challID}. +func (wfe *WebFrontEndImpl) ChallengeHandler( ctx context.Context, logEvent *web.RequestEvent, response http.ResponseWriter, @@ -1216,7 +1219,7 @@ func (wfe *WebFrontEndImpl) prepChallengeForDisplay( challenge *core.Challenge, ) { // Update the challenge URL to be relative to the HTTP request Host - challenge.URL = web.RelativeEndpoint(request, fmt.Sprintf("%s%s/%s", challengePath, authz.ID, challenge.StringID())) + challenge.URL = web.RelativeEndpoint(request, fmt.Sprintf("%s%s/%s", deprecatedChallengePath, authz.ID, challenge.StringID())) if handlerPath == challengePathWithAcct || handlerPath == authzPathWithAcct { challenge.URL = web.RelativeEndpoint(request, fmt.Sprintf("%s%d/%s/%s", challengePathWithAcct, authz.RegistrationID, authz.ID, challenge.StringID())) } @@ -1556,17 +1559,17 @@ func (wfe *WebFrontEndImpl) deactivateAuthorization( return true } -// AuthorizationHandler handles requests to authorization URLs of the form /acme/authz/{authzID}. -func (wfe *WebFrontEndImpl) AuthorizationHandler( +// DeprecatedAuthorizationHandler handles requests to authorization URLs of the form /acme/authz/{authzID}. +func (wfe *WebFrontEndImpl) DeprecatedAuthorizationHandler( ctx context.Context, logEvent *web.RequestEvent, response http.ResponseWriter, request *http.Request) { - wfe.Authorization(ctx, authzPath, logEvent, response, request, request.URL.Path) + wfe.Authorization(ctx, deprecatedAuthzPath, logEvent, response, request, request.URL.Path) } -// AuthorizationHandlerWithAccount handles requests to authorization URLs of the form /acme/authz/{regID}/{authzID}. -func (wfe *WebFrontEndImpl) AuthorizationHandlerWithAccount( +// AuthorizationHandler handles requests to authorization URLs of the form /acme/authz/{regID}/{authzID}. +func (wfe *WebFrontEndImpl) AuthorizationHandler( ctx context.Context, logEvent *web.RequestEvent, response http.ResponseWriter, @@ -2796,5 +2799,5 @@ func urlForAuthz(handlerPath string, authz core.Authorization, request *http.Req return web.RelativeEndpoint(request, fmt.Sprintf("%s%d/%s", authzPathWithAcct, authz.RegistrationID, authz.ID)) } - return web.RelativeEndpoint(request, authzPath+authz.ID) + return web.RelativeEndpoint(request, deprecatedAuthzPath+authz.ID) } diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 2f17544642b..306cb89aa12 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -1067,13 +1067,13 @@ func TestHTTPMethods(t *testing.T) { // TODO(@cpu): Remove GET authz support, support only POST-as-GET { Name: "Authz path should be GET or POST only", - Path: authzPath, + Path: deprecatedAuthzPath, Allowed: getOrPost, }, // TODO(@cpu): Remove GET challenge support, support only POST-as-GET { Name: "Challenge path should be GET or POST only", - Path: challengePath, + Path: deprecatedChallengePath, Allowed: getOrPost, }, // TODO(@cpu): Remove GET certificate support, support only POST-as-GET @@ -1182,7 +1182,7 @@ func TestGetChallengeHandler(t *testing.T) { test.AssertNotError(t, err, "Could not make NewRequest") req.URL.Path = fmt.Sprintf("1/%s", challSlug) - wfe.ChallengeHandler(ctx, newRequestEvent(), resp, req) + wfe.DeprecatedChallengeHandler(ctx, newRequestEvent(), resp, req) test.AssertEquals(t, resp.Code, http.StatusOK) test.AssertEquals(t, resp.Header().Get("Location"), challengeURL) test.AssertEquals(t, resp.Header().Get("Content-Type"), "application/json") @@ -1216,7 +1216,7 @@ func TestGetChallengeHandlerWithAccount(t *testing.T) { test.AssertNotError(t, err, "Could not make NewRequest") req.URL.Path = fmt.Sprintf("1/1/%s", challSlug) - wfe.ChallengeHandlerWithAccount(ctx, newRequestEvent(), resp, req) + wfe.ChallengeHandler(ctx, newRequestEvent(), resp, req) test.AssertEquals(t, resp.Code, http.StatusOK) test.AssertEquals(t, resp.Header().Get("Location"), challengeURL) test.AssertEquals(t, resp.Header().Get("Content-Type"), "application/json") @@ -1299,7 +1299,7 @@ func TestChallengeHandler(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { responseWriter := httptest.NewRecorder() - wfe.ChallengeHandler(ctx, newRequestEvent(), responseWriter, tc.Request) + wfe.DeprecatedChallengeHandler(ctx, newRequestEvent(), responseWriter, tc.Request) // Check the response code, headers and body match expected headers := responseWriter.Header() body := responseWriter.Body.String() @@ -1378,7 +1378,7 @@ func TestChallengeHandlerWithAccount(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { responseWriter := httptest.NewRecorder() - wfe.ChallengeHandlerWithAccount(ctx, newRequestEvent(), responseWriter, tc.Request) + wfe.ChallengeHandler(ctx, newRequestEvent(), responseWriter, tc.Request) // Check the response code, headers and body match expected headers := responseWriter.Header() body := responseWriter.Body.String() @@ -1412,7 +1412,7 @@ func TestUpdateChallengeHandlerFinalizedAuthz(t *testing.T) { signedURL := "http://localhost/1/7TyhFQ" _, _, jwsBody := signer.byKeyID(1, nil, signedURL, `{}`) request := makePostRequestWithPath("1/7TyhFQ", jwsBody) - wfe.ChallengeHandler(ctx, newRequestEvent(), responseWriter, request) + wfe.DeprecatedChallengeHandler(ctx, newRequestEvent(), responseWriter, request) body := responseWriter.Body.String() test.AssertUnmarshaledEquals(t, body, `{ @@ -1434,7 +1434,7 @@ func TestUpdateChallengeHandlerWithAccountFinalizedAuthz(t *testing.T) { signedURL := "http://localhost/1/1/7TyhFQ" _, _, jwsBody := signer.byKeyID(1, nil, signedURL, `{}`) request := makePostRequestWithPath("1/1/7TyhFQ", jwsBody) - wfe.ChallengeHandlerWithAccount(ctx, newRequestEvent(), responseWriter, request) + wfe.ChallengeHandler(ctx, newRequestEvent(), responseWriter, request) body := responseWriter.Body.String() test.AssertUnmarshaledEquals(t, body, `{ @@ -1459,7 +1459,7 @@ func TestUpdateChallengeHandlerRAError(t *testing.T) { responseWriter := httptest.NewRecorder() request := makePostRequestWithPath("2/7TyhFQ", jwsBody) - wfe.ChallengeHandler(ctx, newRequestEvent(), responseWriter, request) + wfe.DeprecatedChallengeHandler(ctx, newRequestEvent(), responseWriter, request) // The result should be an internal server error problem. body := responseWriter.Body.String() @@ -1484,7 +1484,7 @@ func TestUpdateChallengeHandlerWithAccountRAError(t *testing.T) { responseWriter := httptest.NewRecorder() request := makePostRequestWithPath("1/2/7TyhFQ", jwsBody) - wfe.ChallengeHandlerWithAccount(ctx, newRequestEvent(), responseWriter, request) + wfe.ChallengeHandler(ctx, newRequestEvent(), responseWriter, request) // The result should be an internal server error problem. body := responseWriter.Body.String() @@ -1802,7 +1802,7 @@ func TestGetAuthorizationHandler(t *testing.T) { // Expired authorizations should be inaccessible authzURL := "3" responseWriter := httptest.NewRecorder() - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ Method: "GET", URL: mustParseURL(authzURL), }) @@ -1812,7 +1812,7 @@ func TestGetAuthorizationHandler(t *testing.T) { responseWriter.Body.Reset() // Ensure that a valid authorization can't be reached with an invalid URL - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ URL: mustParseURL("1d"), Method: "GET", }) @@ -1824,7 +1824,7 @@ func TestGetAuthorizationHandler(t *testing.T) { responseWriter = httptest.NewRecorder() // Ensure that a POST-as-GET to an authorization works - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, postAsGet) + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, postAsGet) test.AssertEquals(t, responseWriter.Code, http.StatusOK) body := responseWriter.Body.String() test.AssertUnmarshaledEquals(t, body, ` @@ -1852,7 +1852,7 @@ func TestGetAuthorizationHandlerWithAccount(t *testing.T) { // Expired authorizations should be inaccessible authzURL := "1/3" responseWriter := httptest.NewRecorder() - wfe.AuthorizationHandlerWithAccount(ctx, newRequestEvent(), responseWriter, &http.Request{ + wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ Method: "GET", URL: mustParseURL(authzURL), }) @@ -1862,7 +1862,7 @@ func TestGetAuthorizationHandlerWithAccount(t *testing.T) { responseWriter.Body.Reset() // Ensure that a valid authorization can't be reached with an invalid URL - wfe.AuthorizationHandlerWithAccount(ctx, newRequestEvent(), responseWriter, &http.Request{ + wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ URL: mustParseURL("1/1d"), Method: "GET", }) @@ -1874,7 +1874,7 @@ func TestGetAuthorizationHandlerWithAccount(t *testing.T) { responseWriter = httptest.NewRecorder() // Ensure that a POST-as-GET to an authorization works - wfe.AuthorizationHandlerWithAccount(ctx, newRequestEvent(), responseWriter, postAsGet) + wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, postAsGet) test.AssertEquals(t, responseWriter.Code, http.StatusOK) body := responseWriter.Body.String() test.AssertUnmarshaledEquals(t, body, ` @@ -1902,7 +1902,7 @@ func TestAuthorizationHandler500(t *testing.T) { wfe, _, _ := setupWFE(t) responseWriter := httptest.NewRecorder() - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ Method: "GET", URL: mustParseURL("4"), }) @@ -1920,7 +1920,7 @@ func TestAuthorizationHandlerWithAccount500(t *testing.T) { wfe, _, _ := setupWFE(t) responseWriter := httptest.NewRecorder() - wfe.AuthorizationHandlerWithAccount(ctx, newRequestEvent(), responseWriter, &http.Request{ + wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ Method: "GET", URL: mustParseURL("1/4"), }) @@ -1969,7 +1969,7 @@ func TestAuthorizationChallengeHandlerNamespace(t *testing.T) { wfe.ra = &RAWithFailedChallenge{clk: clk} responseWriter := httptest.NewRecorder() - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ Method: "GET", URL: mustParseURL("6"), }) @@ -1990,7 +1990,7 @@ func TestAuthorizationChallengeHandlerWithAccountNamespace(t *testing.T) { wfe.ra = &RAWithFailedChallenge{clk: clk} responseWriter := httptest.NewRecorder() - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, &http.Request{ Method: "GET", URL: mustParseURL("6"), }) @@ -2647,7 +2647,7 @@ func TestDeactivateAuthorizationHandler(t *testing.T) { _, _, body := signer.byKeyID(1, nil, "http://localhost/1", payload) request := makePostRequestWithPath("1", body) - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, request) + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{"type": "`+probs.ErrorNS+`malformed","detail": "Invalid status value","status": 400}`) @@ -2657,7 +2657,7 @@ func TestDeactivateAuthorizationHandler(t *testing.T) { _, _, body = signer.byKeyID(1, nil, "http://localhost/1", payload) request = makePostRequestWithPath("1", body) - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, request) + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{ @@ -2688,7 +2688,7 @@ func TestDeactivateAuthorizationHandlerWithAccount(t *testing.T) { _, _, body := signer.byKeyID(1, nil, "http://localhost/1", payload) request := makePostRequestWithPath("1", body) - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, request) + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{"type": "`+probs.ErrorNS+`malformed","detail": "Invalid status value","status": 400}`) @@ -2698,7 +2698,7 @@ func TestDeactivateAuthorizationHandlerWithAccount(t *testing.T) { _, _, body = signer.byKeyID(1, nil, "http://localhost/1", payload) request = makePostRequestWithPath("1", body) - wfe.AuthorizationHandler(ctx, newRequestEvent(), responseWriter, request) + wfe.DeprecatedAuthorizationHandler(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{ @@ -3682,7 +3682,7 @@ func TestPrepAuthzForDisplay(t *testing.T) { } // This modifies the authz in-place. - wfe.prepAuthorizationForDisplay(authzPath, &http.Request{Host: "localhost"}, authz) + wfe.prepAuthorizationForDisplay(deprecatedAuthzPath, &http.Request{Host: "localhost"}, authz) // Ensure ID and RegID are omitted. authzJSON, err := json.Marshal(authz) @@ -3734,7 +3734,7 @@ func TestPrepRevokedAuthzForDisplay(t *testing.T) { } // This modifies the authz in-place. - wfe.prepAuthorizationForDisplay(authzPath, &http.Request{Host: "localhost"}, authz) + wfe.prepAuthorizationForDisplay(deprecatedAuthzPath, &http.Request{Host: "localhost"}, authz) // All of the challenges should be revoked as well. for _, chall := range authz.Challenges { @@ -3782,7 +3782,7 @@ func TestPrepWildcardAuthzForDisplay(t *testing.T) { } // This modifies the authz in-place. - wfe.prepAuthorizationForDisplay(authzPath, &http.Request{Host: "localhost"}, authz) + wfe.prepAuthorizationForDisplay(deprecatedAuthzPath, &http.Request{Host: "localhost"}, authz) // The identifier should not start with a star, but the authz should be marked // as a wildcard. @@ -3841,7 +3841,7 @@ func TestPrepAuthzForDisplayShuffle(t *testing.T) { // Prep the authz 100 times, and count where each challenge ended up each time. for range 100 { // This modifies the authz in place - wfe.prepAuthorizationForDisplay(challengePath, &http.Request{Host: "localhost"}, authz) + wfe.prepAuthorizationForDisplay(deprecatedChallengePath, &http.Request{Host: "localhost"}, authz) for i, chall := range authz.Challenges { counts[chall.Type][i] += 1 } @@ -3952,7 +3952,7 @@ func TestGETAPIAuthorizationHandler(t *testing.T) { for _, tc := range testCases { responseWriter := httptest.NewRecorder() req, logEvent := makeGet(tc.path, getAuthzPath) - wfe.AuthorizationHandler(context.Background(), logEvent, responseWriter, req) + wfe.DeprecatedAuthorizationHandler(context.Background(), logEvent, responseWriter, req) if responseWriter.Code == http.StatusOK && tc.expectTooFreshErr { t.Errorf("expected too fresh error, got http.StatusOK") @@ -3991,7 +3991,7 @@ func TestGETAPIAuthorizationHandlerWitAccount(t *testing.T) { for _, tc := range testCases { responseWriter := httptest.NewRecorder() req, logEvent := makeGet(tc.path, getAuthzPath) - wfe.AuthorizationHandlerWithAccount(context.Background(), logEvent, responseWriter, req) + wfe.AuthorizationHandler(context.Background(), logEvent, responseWriter, req) if responseWriter.Code == http.StatusOK && tc.expectTooFreshErr { t.Errorf("expected too fresh error, got http.StatusOK") @@ -4030,7 +4030,7 @@ func TestGETAPIChallenge(t *testing.T) { for _, tc := range testCases { responseWriter := httptest.NewRecorder() req, logEvent := makeGet(tc.path, getAuthzPath) - wfe.ChallengeHandler(context.Background(), logEvent, responseWriter, req) + wfe.DeprecatedChallengeHandler(context.Background(), logEvent, responseWriter, req) if responseWriter.Code == http.StatusOK && tc.expectTooFreshErr { t.Errorf("expected too fresh error, got http.StatusOK") From 77912628153bba439fee9f5725652f3bab74e06c Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 21 Nov 2024 17:17:16 -0800 Subject: [PATCH 2/2] Rate limit deactivating pending authzs (#7835) When a client deactivates a pending authorization, count that towards their FailedAuthorizationsPerDomainPerAccount and FailedAuthorizationsForPausingPerDomainPerAccount rate limits. This should help curb the few clients which constantly create new orders and authzs, deactivate those pending authzs preventing reuse of them or their orders, and then rinse and repeat. Fixes https://github.com/letsencrypt/boulder/issues/7834 --- ra/ra.go | 13 +++++++- ra/ra_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 88 insertions(+), 9 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 7564a1395ae..0aee052816c 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -2425,7 +2425,7 @@ func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context, // DeactivateAuthorization deactivates a currently valid authorization func (ra *RegistrationAuthorityImpl) DeactivateAuthorization(ctx context.Context, req *corepb.Authorization) (*emptypb.Empty, error) { - if req == nil || req.Id == "" || req.Status == "" { + if core.IsAnyNilOrZero(req, req.Id, req.Status, req.RegistrationID) { return nil, errIncompleteGRPCRequest } authzID, err := strconv.ParseInt(req.Id, 10, 64) @@ -2435,6 +2435,17 @@ func (ra *RegistrationAuthorityImpl) DeactivateAuthorization(ctx context.Context if _, err := ra.SA.DeactivateAuthorization2(ctx, &sapb.AuthorizationID2{Id: authzID}); err != nil { return nil, err } + if req.Status == string(core.StatusPending) { + // Some clients deactivate pending authorizations without attempting them. + // We're not sure exactly when this happens but it's most likely due to + // internal errors in the client. From our perspective this uses storage + // resources similar to how failed authorizations do, so we increment the + // failed authorizations limit. + err = ra.countFailedValidations(ctx, req.RegistrationID, identifier.NewDNS(req.DnsName)) + if err != nil { + return nil, fmt.Errorf("failed to update rate limits: %w", err) + } + } return &emptypb.Empty{}, nil } diff --git a/ra/ra_test.go b/ra/ra_test.go index d1c98177bc1..594edc17ec4 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -865,13 +865,13 @@ func (msa *mockSAPaused) FinalizeAuthorization2(ctx context.Context, req *sapb.F } func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t *testing.T) { - if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { - t.Skip() - } - va, sa, ra, redisSrc, fc, cleanUp := initAuthorities(t) defer cleanUp() + if ra.limiter == nil { + t.Skip("no redis limiter configured") + } + features.Set(features.Config{AutomaticallyPauseZombieClients: true}) defer features.Reset() @@ -988,13 +988,13 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * } func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersRatelimit(t *testing.T) { - if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { - t.Skip() - } - va, sa, ra, redisSrc, fc, cleanUp := initAuthorities(t) defer cleanUp() + if ra.limiter == nil { + t.Skip("no redis limiter configured") + } + features.Set(features.Config{AutomaticallyPauseZombieClients: true}) defer features.Reset() @@ -1778,6 +1778,74 @@ func TestDeactivateAuthorization(t *testing.T) { test.AssertEquals(t, deact.Status, string(core.StatusDeactivated)) } +type mockSARecordingPauses struct { + sapb.StorageAuthorityClient + recv *sapb.PauseRequest +} + +func (sa *mockSARecordingPauses) PauseIdentifiers(ctx context.Context, req *sapb.PauseRequest, _ ...grpc.CallOption) (*sapb.PauseIdentifiersResponse, error) { + sa.recv = req + return &sapb.PauseIdentifiersResponse{Paused: int64(len(req.Identifiers))}, nil +} + +func (sa *mockSARecordingPauses) DeactivateAuthorization2(_ context.Context, _ *sapb.AuthorizationID2, _ ...grpc.CallOption) (*emptypb.Empty, error) { + return nil, nil +} + +func TestDeactivateAuthorization_Pausing(t *testing.T) { + _, _, ra, _, _, cleanUp := initAuthorities(t) + defer cleanUp() + + if ra.limiter == nil { + t.Skip("no redis limiter configured") + } + + msa := mockSARecordingPauses{} + ra.SA = &msa + + features.Set(features.Config{AutomaticallyPauseZombieClients: true}) + defer features.Reset() + + // Override the default ratelimits to only allow one failed validation. + txnBuilder, err := ratelimits.NewTransactionBuilder("testdata/one-failed-validation-before-pausing.yml", "") + test.AssertNotError(t, err, "making transaction composer") + ra.txnBuilder = txnBuilder + + // The first deactivation of a pending authz should work and nothing should + // get paused. + _, err = ra.DeactivateAuthorization(ctx, &corepb.Authorization{ + Id: "1", + RegistrationID: 1, + DnsName: "example.com", + Status: string(core.StatusPending), + }) + test.AssertNotError(t, err, "mock deactivation should work") + test.AssertBoxedNil(t, msa.recv, "shouldn't be a pause request yet") + + // Deactivating a valid authz shouldn't increment any limits or pause anything. + _, err = ra.DeactivateAuthorization(ctx, &corepb.Authorization{ + Id: "2", + RegistrationID: 1, + DnsName: "example.com", + Status: string(core.StatusValid), + }) + test.AssertNotError(t, err, "mock deactivation should work") + test.AssertBoxedNil(t, msa.recv, "deactivating valid authz should never pause") + + // Deactivating a second pending authz should surpass the limit and result + // in a pause request. + _, err = ra.DeactivateAuthorization(ctx, &corepb.Authorization{ + Id: "3", + RegistrationID: 1, + DnsName: "example.com", + Status: string(core.StatusPending), + }) + test.AssertNotError(t, err, "mock deactivation should work") + test.AssertNotNil(t, msa.recv, "should have recorded a pause request") + test.AssertEquals(t, msa.recv.RegistrationID, int64(1)) + test.AssertEquals(t, msa.recv.Identifiers[0].Value, "example.com") +} + func TestDeactivateRegistration(t *testing.T) { _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp()