From 36a617a55b56e1ca7953b62ff7ea63af79089fc6 Mon Sep 17 00:00:00 2001 From: Samantha Frank Date: Wed, 31 Jul 2024 10:57:59 -0400 Subject: [PATCH 1/3] SFE: Improve UX when the JWT is malformed or expired (#7637) Currently, the SFE displays "An error occurred while unpausing your account" in scenarios where it's not correct or helpful. - Return a helpful message when a Subscriber attempts to access the unpause form but fails to copy the entire link - Return a helpful message when a Subscriber attempts to unpause using an expired JWT - Some small cleanups that make the code a little more mistake-proof. Part of https://github.com/letsencrypt/boulder/issues/7406 --- sfe/pages/unpause-expired.html | 19 ++++++++ sfe/pages/unpause-status.html | 2 +- sfe/sfe.go | 83 ++++++++++++++++++++-------------- sfe/sfe_test.go | 43 +++++++++++++++--- unpause/unpause.go | 15 ++++-- unpause/unpause_test.go | 14 +++--- 6 files changed, 124 insertions(+), 52 deletions(-) create mode 100644 sfe/pages/unpause-expired.html diff --git a/sfe/pages/unpause-expired.html b/sfe/pages/unpause-expired.html new file mode 100644 index 00000000000..69b8b81f8dd --- /dev/null +++ b/sfe/pages/unpause-expired.html @@ -0,0 +1,19 @@ +{{ template "header" }} + +
+

Expired unpause URL

+

+ If you got here by visiting a URL found in your ACME client logs, please + try an unpause URL from a more recent log entry. Each unpause URL is + only valid for a short period of time. If you cannot find a valid + unpause URL, you may need to re-run your ACME client to generate a new + one. +

+

+ If you continue to encounter difficulties, or if you need more help, our + community support forum + is a great resource for troubleshooting and advice. +

+
+ +{{template "footer"}} diff --git a/sfe/pages/unpause-status.html b/sfe/pages/unpause-status.html index efd1aa43566..59f0df5f93f 100644 --- a/sfe/pages/unpause-status.html +++ b/sfe/pages/unpause-status.html @@ -2,7 +2,7 @@
- {{ if eq .UnpauseSuccessful true }} + {{ if eq .Successful true }}

Account successfully unpaused

diff --git a/sfe/sfe.go b/sfe/sfe.go index ca089724a1b..19a6e6797c5 100644 --- a/sfe/sfe.go +++ b/sfe/sfe.go @@ -11,6 +11,7 @@ import ( "text/template" "time" + "github.com/go-jose/go-jose/v4/jwt" "github.com/jmhodges/clock" "github.com/prometheus/client_golang/prometheus" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" @@ -137,14 +138,21 @@ func (sfe *SelfServiceFrontEndImpl) BuildID(response http.ResponseWriter, reques // in this form. func (sfe *SelfServiceFrontEndImpl) UnpauseForm(response http.ResponseWriter, request *http.Request) { incomingJWT := request.URL.Query().Get("jwt") - if incomingJWT == "" { - sfe.unpauseInvalidRequest(response) - return - } regID, identifiers, err := sfe.parseUnpauseJWT(incomingJWT) if err != nil { - sfe.unpauseStatusHelper(response, false) + if errors.Is(err, jwt.ErrExpired) { + // JWT expired before the Subscriber visited the unpause page. + sfe.unpauseTokenExpired(response) + return + } + if errors.Is(err, unpause.ErrMalformedJWT) { + // JWT is malformed. This could happen if the Subscriber failed to + // copy the entire URL from their logs. + sfe.unpauseRequestMalformed(response) + return + } + sfe.unpauseFailed(response) return } @@ -160,20 +168,26 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseForm(response http.ResponseWriter, re sfe.renderTemplate(response, "unpause-form.html", tmplData{unpausePostForm, incomingJWT, regID, identifiers}) } -// UnpauseSubmit serves a page indicating if the unpause form submission -// succeeded or failed upon clicking the unpause button. We are explicitly -// choosing to not address CSRF at this time because we control creation and -// redemption of the JWT. +// UnpauseSubmit serves a page showing the result of the unpause form submission. +// CSRF is not addressed because a third party causing submission of an unpause +// form is not harmful. func (sfe *SelfServiceFrontEndImpl) UnpauseSubmit(response http.ResponseWriter, request *http.Request) { incomingJWT := request.URL.Query().Get("jwt") - if incomingJWT == "" { - sfe.unpauseInvalidRequest(response) - return - } _, _, err := sfe.parseUnpauseJWT(incomingJWT) if err != nil { - sfe.unpauseStatusHelper(response, false) + if errors.Is(err, jwt.ErrExpired) { + // JWT expired before the Subscriber could click the unpause button. + sfe.unpauseTokenExpired(response) + return + } + if errors.Is(err, unpause.ErrMalformedJWT) { + // JWT is malformed. This should never happen if the request came + // from our form. + sfe.unpauseRequestMalformed(response) + return + } + sfe.unpauseFailed(response) return } @@ -186,24 +200,24 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseSubmit(response http.ResponseWriter, http.Redirect(response, request, unpauseStatus, http.StatusFound) } -// unpauseInvalidRequest is a helper that displays a page indicating the -// Subscriber perform basic troubleshooting due to lack of JWT in the data -// object. -func (sfe *SelfServiceFrontEndImpl) unpauseInvalidRequest(response http.ResponseWriter) { +func (sfe *SelfServiceFrontEndImpl) unpauseRequestMalformed(response http.ResponseWriter) { sfe.renderTemplate(response, "unpause-invalid-request.html", nil) } -type unpauseStatusTemplateData struct { - UnpauseSuccessful bool +func (sfe *SelfServiceFrontEndImpl) unpauseTokenExpired(response http.ResponseWriter) { + sfe.renderTemplate(response, "unpause-expired.html", nil) +} + +type unpauseStatusTemplate struct { + Successful bool +} + +func (sfe *SelfServiceFrontEndImpl) unpauseFailed(response http.ResponseWriter) { + sfe.renderTemplate(response, "unpause-status.html", unpauseStatusTemplate{Successful: false}) } -// unpauseStatus is a helper that, by default, displays a failure message to the -// Subscriber indicating that their account has failed to unpause. For failure -// scenarios, only when the JWT validation should call this. Other types of -// failures should use unpauseInvalidRequest. For successes, call UnpauseStatus -// instead. -func (sfe *SelfServiceFrontEndImpl) unpauseStatusHelper(response http.ResponseWriter, status bool) { - sfe.renderTemplate(response, "unpause-status.html", unpauseStatusTemplateData{status}) +func (sfe *SelfServiceFrontEndImpl) unpauseSuccessful(response http.ResponseWriter) { + sfe.renderTemplate(response, "unpause-status.html", unpauseStatusTemplate{Successful: true}) } // UnpauseStatus displays a success message to the Subscriber indicating that @@ -218,19 +232,22 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseStatus(response http.ResponseWriter, // TODO(#7580) This should only be reachable after a client has clicked the // "Please unblock my account" button and that request succeeding. No one // should be able to access this page otherwise. - sfe.unpauseStatusHelper(response, true) + sfe.unpauseSuccessful(response) } // parseUnpauseJWT extracts and returns the subscriber's registration ID and a // slice of paused identifiers from the claims. If the JWT cannot be parsed or -// is otherwise invalid, an error is returned. +// is otherwise invalid, an error is returned. If the JWT is missing or +// malformed, unpause.ErrMalformedJWT is returned. func (sfe *SelfServiceFrontEndImpl) parseUnpauseJWT(incomingJWT string) (int64, []string, error) { - slug := strings.Split(unpause.APIPrefix, "/") - if len(slug) != 3 { - return 0, nil, errors.New("failed to parse API version") + if incomingJWT == "" || len(strings.Split(incomingJWT, ".")) != 3 { + // JWT is missing or malformed. This could happen if the Subscriber + // failed to copy the entire URL from their logs. This should never + // happen if the request came from our form. + return 0, nil, unpause.ErrMalformedJWT } - claims, err := unpause.RedeemJWT(incomingJWT, sfe.unpauseHMACKey, slug[2], sfe.clk) + claims, err := unpause.RedeemJWT(incomingJWT, sfe.unpauseHMACKey, unpause.APIVersion, sfe.clk) if err != nil { return 0, nil, err } diff --git a/sfe/sfe_test.go b/sfe/sfe_test.go index 513736cd360..9ee887b8506 100644 --- a/sfe/sfe_test.go +++ b/sfe/sfe_test.go @@ -94,6 +94,8 @@ func TestBuildIDPath(t *testing.T) { func TestUnpausePaths(t *testing.T) { t.Parallel() sfe, fc := setupSFE(t) + unpauseSigner, err := unpause.NewJWTSigner(cmd.HMACKeyConfig{KeyFile: "../test/secrets/sfe_unpause_key"}) + test.AssertNotError(t, err, "Should have been able to create JWT signer, but could not") // GET with no JWT responseWriter := httptest.NewRecorder() @@ -111,11 +113,22 @@ func TestUnpausePaths(t *testing.T) { URL: mustParseURL(fmt.Sprintf(unpause.GetForm + "?jwt=x")), }) test.AssertEquals(t, responseWriter.Code, http.StatusOK) - test.AssertContains(t, responseWriter.Body.String(), "An error occurred while unpausing your account") + test.AssertContains(t, responseWriter.Body.String(), "Invalid unpause URL") + + // GET with an expired JWT + expiredJWT, err := unpause.GenerateJWT(unpauseSigner, 1234567890, []string{"example.net"}, time.Hour, fc) + test.AssertNotError(t, err, "Should have been able to create JWT, but could not") + responseWriter = httptest.NewRecorder() + // Advance the clock by 337 hours to make the JWT expired. + fc.Add(time.Hour * 337) + sfe.UnpauseForm(responseWriter, &http.Request{ + Method: "GET", + URL: mustParseURL(fmt.Sprintf(unpause.GetForm + "?jwt=" + expiredJWT)), + }) + test.AssertEquals(t, responseWriter.Code, http.StatusOK) + test.AssertContains(t, responseWriter.Body.String(), "Expired unpause URL") // GET with a valid JWT - unpauseSigner, err := unpause.NewJWTSigner(cmd.HMACKeyConfig{KeyFile: "../test/secrets/sfe_unpause_key"}) - test.AssertNotError(t, err, "Should have been able to create JWT signer, but could not") validJWT, err := unpause.GenerateJWT(unpauseSigner, 1234567890, []string{"example.com"}, time.Hour, fc) test.AssertNotError(t, err, "Should have been able to create JWT, but could not") responseWriter = httptest.NewRecorder() @@ -126,6 +139,15 @@ func TestUnpausePaths(t *testing.T) { test.AssertEquals(t, responseWriter.Code, http.StatusOK) test.AssertContains(t, responseWriter.Body.String(), "Action required to unpause your account") + // POST with an expired JWT + responseWriter = httptest.NewRecorder() + sfe.UnpauseSubmit(responseWriter, &http.Request{ + Method: "POST", + URL: mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=" + expiredJWT)), + }) + test.AssertEquals(t, responseWriter.Code, http.StatusOK) + test.AssertContains(t, responseWriter.Body.String(), "Expired unpause URL") + // POST with no JWT responseWriter = httptest.NewRecorder() sfe.UnpauseSubmit(responseWriter, &http.Request{ @@ -135,14 +157,23 @@ func TestUnpausePaths(t *testing.T) { test.AssertEquals(t, responseWriter.Code, http.StatusOK) test.AssertContains(t, responseWriter.Body.String(), "Invalid unpause URL") - // POST with an invalid JWT + // POST with an invalid JWT, missing one of the three parts responseWriter = httptest.NewRecorder() sfe.UnpauseSubmit(responseWriter, &http.Request{ Method: "POST", - URL: mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=x")), + URL: mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=x.x")), }) test.AssertEquals(t, responseWriter.Code, http.StatusOK) - test.AssertContains(t, responseWriter.Body.String(), "An error occurred while unpausing your account") + test.AssertContains(t, responseWriter.Body.String(), "Invalid unpause URL") + + // POST with an invalid JWT, all parts present but missing some characters + responseWriter = httptest.NewRecorder() + sfe.UnpauseSubmit(responseWriter, &http.Request{ + Method: "POST", + URL: mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=x.x.x")), + }) + test.AssertEquals(t, responseWriter.Code, http.StatusOK) + test.AssertContains(t, responseWriter.Body.String(), "Invalid unpause URL") // POST with a valid JWT redirects to a success page responseWriter = httptest.NewRecorder() diff --git a/unpause/unpause.go b/unpause/unpause.go index 6b0a7179a91..eeb097a3f6f 100644 --- a/unpause/unpause.go +++ b/unpause/unpause.go @@ -17,8 +17,8 @@ const ( // API // Changing this value will invalidate all existing JWTs. - apiVersion = "v1" - APIPrefix = "/sfe/" + apiVersion + APIVersion = "v1" + APIPrefix = "/sfe/" + APIVersion GetForm = APIPrefix + "/unpause" // JWT @@ -67,7 +67,7 @@ func GenerateJWT(signer JWTSigner, regID int64, identifiers []string, lifetime t IssuedAt: jwt.NewNumericDate(clk.Now()), Expiry: jwt.NewNumericDate(clk.Now().Add(lifetime)), }, - V: apiVersion, + V: APIVersion, I: strings.Join(identifiers, ","), } @@ -79,6 +79,9 @@ func GenerateJWT(signer JWTSigner, regID int64, identifiers []string, lifetime t return serialized, nil } +// ErrMalformedJWT is returned when the JWT is malformed. +var ErrMalformedJWT = errors.New("malformed JWT") + // RedeemJWT deserializes an unpause JWT and returns the validated claims. The // key is used to validate the signature of the JWT. The version is the expected // API version of the JWT. This function validates that the JWT is: @@ -90,16 +93,18 @@ func GenerateJWT(signer JWTSigner, regID int64, identifiers []string, lifetime t // - subject can be parsed as a 64-bit integer, // - contains a set of paused identifiers as 'Identifiers', and // - contains the API the expected version as 'Version'. +// +// If the JWT is malformed or invalid in any way, ErrMalformedJWT is returned. func RedeemJWT(token string, key []byte, version string, clk clock.Clock) (JWTClaims, error) { parsedToken, err := jwt.ParseSigned(token, []jose.SignatureAlgorithm{jose.HS256}) if err != nil { - return JWTClaims{}, fmt.Errorf("parsing JWT: %s", err) + return JWTClaims{}, errors.Join(ErrMalformedJWT, err) } claims := JWTClaims{} err = parsedToken.Claims(key, &claims) if err != nil { - return JWTClaims{}, fmt.Errorf("verifying JWT: %s", err) + return JWTClaims{}, errors.Join(ErrMalformedJWT, err) } err = claims.Validate(jwt.Expected{ diff --git a/unpause/unpause_test.go b/unpause/unpause_test.go index 1346375e8b0..3d72ca58371 100644 --- a/unpause/unpause_test.go +++ b/unpause/unpause_test.go @@ -40,7 +40,7 @@ func TestUnpauseJWT(t *testing.T) { name: "valid one identifier", args: args{ key: hmacKey, - version: apiVersion, + version: APIVersion, account: 1234567890, identifiers: []string{"example.com"}, lifetime: time.Hour, @@ -53,7 +53,7 @@ func TestUnpauseJWT(t *testing.T) { Audience: jwt.Audience{defaultAudience}, Expiry: jwt.NewNumericDate(fc.Now().Add(time.Hour)), }, - V: apiVersion, + V: APIVersion, I: "example.com", }, wantGenerateJWTErr: false, @@ -63,7 +63,7 @@ func TestUnpauseJWT(t *testing.T) { name: "valid multiple identifiers", args: args{ key: hmacKey, - version: apiVersion, + version: APIVersion, account: 1234567890, identifiers: []string{"example.com", "example.org", "example.net"}, lifetime: time.Hour, @@ -76,7 +76,7 @@ func TestUnpauseJWT(t *testing.T) { Audience: jwt.Audience{defaultAudience}, Expiry: jwt.NewNumericDate(fc.Now().Add(time.Hour)), }, - V: apiVersion, + V: APIVersion, I: "example.com,example.org,example.net", }, wantGenerateJWTErr: false, @@ -86,7 +86,7 @@ func TestUnpauseJWT(t *testing.T) { name: "invalid no account", args: args{ key: hmacKey, - version: apiVersion, + version: APIVersion, account: 0, identifiers: []string{"example.com"}, lifetime: time.Hour, @@ -103,7 +103,7 @@ func TestUnpauseJWT(t *testing.T) { name: "invalid key too small", args: args{ key: []byte("key"), - version: apiVersion, + version: APIVersion, account: 1234567890, identifiers: []string{"example.com"}, lifetime: time.Hour, @@ -117,7 +117,7 @@ func TestUnpauseJWT(t *testing.T) { name: "invalid no identifiers", args: args{ key: hmacKey, - version: apiVersion, + version: APIVersion, account: 1234567890, identifiers: nil, lifetime: time.Hour, From c6c761785191456a1ac1ae8f9f6506701f3b89f5 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 31 Jul 2024 11:08:11 -0700 Subject: [PATCH 2/3] Profiles: allow for omission of KU, EKU, and SKID (#7622) Add three new keys to the CA's ProfileConfig: - OmitKeyEncipherment causes the keyEncipherment Key Usage to be omitted from certificates with RSA public keys. We currently include it for backwards compatibility with TLS 1.1 servers that don't support modern cipher suites, but this KU is completely useless as of TLS 1.3. - OmitClientAuth causes the tlsClientAuthentication Extended Key Usage to be omitted from all certificates. We currently include it to support any subscribers who may be relying on it, but Root Programs are moving towards single-purpose hierarchies and its inclusion is being discouraged. - OmitSKID causes the Subject Key Identifier extension to be omitted from all certificates. We currently include this extension because it is recommended by RFC 5280, but it serves little to no practical purpose and consumes a large number of bytes, so it is now NOT RECOMMENDED by the Baseline Requirements. Make substantive changes to issuer.requestValid and issuer.Prepare to implement the desired behavior for each of these options. Make a very slight change to ra.matchesCSR to generally allow for serverAuth-only EKUs. Improve the unit tests of both the //ca and //issuance packages to cover the new behavior. Part of https://github.com/letsencrypt/boulder/issues/7610 --- ca/ca.go | 32 +++----- ca/ca_test.go | 156 ++++++++++++++------------------------- issuance/cert.go | 57 ++++++++++---- issuance/cert_test.go | 50 +++++++++++-- ra/ra.go | 9 ++- test/config-next/ca.json | 13 +--- 6 files changed, 163 insertions(+), 154 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index b99fe702f4c..a1d2c62a57a 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -184,8 +184,8 @@ func makeCertificateProfilesMap(defaultName string, profiles map[string]issuance return certProfilesMaps{}, fmt.Errorf("defaultCertificateProfileName:\"%s\" was configured, but a profile object was not found for that name", defaultName) } - profileByName := make(map[string]*certProfileWithID, len(profiles)) - profileByHash := make(map[[32]byte]*certProfileWithID, len(profiles)) + profilesByName := make(map[string]*certProfileWithID, len(profiles)) + profilesByHash := make(map[[32]byte]*certProfileWithID, len(profiles)) for name, profileConfig := range profiles { profile, err := issuance.NewProfile(profileConfig, lints) @@ -208,30 +208,22 @@ func makeCertificateProfilesMap(defaultName string, profiles map[string]issuance } hash := sha256.Sum256(encodedProfile.Bytes()) - _, ok := profileByName[name] - if !ok { - profileByName[name] = &certProfileWithID{ - name: name, - hash: hash, - profile: profile, - } - } else { - return certProfilesMaps{}, fmt.Errorf("duplicate certificate profile name %s", name) + withID := certProfileWithID{ + name: name, + hash: hash, + profile: profile, } - _, ok = profileByHash[hash] - if !ok { - profileByHash[hash] = &certProfileWithID{ - name: name, - hash: hash, - profile: profile, - } - } else { + profilesByName[name] = &withID + + _, found := profilesByHash[hash] + if found { return certProfilesMaps{}, fmt.Errorf("duplicate certificate profile hash %d", hash) } + profilesByHash[hash] = &withID } - return certProfilesMaps{defaultName, profileByHash, profileByName}, nil + return certProfilesMaps{defaultName, profilesByHash, profilesByName}, nil } // NewCertificateAuthorityImpl creates a CA instance that can sign certificates diff --git a/ca/ca_test.go b/ca/ca_test.go index e1608b9b407..4f41bbe7a7e 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -158,19 +158,16 @@ func setup(t *testing.T) *testCtx { certProfiles := make(map[string]issuance.ProfileConfig, 0) certProfiles["legacy"] = issuance.ProfileConfig{ - AllowMustStaple: true, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, + AllowMustStaple: true, MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 90}, MaxValidityBackdate: config.Duration{Duration: time.Hour}, } certProfiles["modern"] = issuance.ProfileConfig{ - AllowMustStaple: true, - OmitCommonName: true, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, + AllowMustStaple: true, + OmitCommonName: true, + OmitKeyEncipherment: true, + OmitClientAuth: true, + OmitSKID: true, MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 6}, MaxValidityBackdate: config.Duration{Duration: time.Hour}, } @@ -556,157 +553,112 @@ func TestUnpredictableIssuance(t *testing.T) { test.Assert(t, seenR3, "Expected at least one issuance from active issuer") } -func TestProfiles(t *testing.T) { +func TestMakeCertificateProfilesMap(t *testing.T) { t.Parallel() testCtx := setup(t) test.AssertEquals(t, len(testCtx.certProfiles), 2) - sa := &mockSA{} - - // These profiles contain the same data which will produce an identical - // hash, even though the names are different. - duplicateProfiles := make(map[string]issuance.ProfileConfig, 0) - duplicateProfiles["legacy"] = issuance.ProfileConfig{ - AllowMustStaple: false, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, - MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 90}, - MaxValidityBackdate: config.Duration{Duration: time.Hour}, - } - duplicateProfiles["modern"] = issuance.ProfileConfig{ - AllowMustStaple: false, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, + testProfile := issuance.ProfileConfig{ + AllowMustStaple: false, MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 90}, MaxValidityBackdate: config.Duration{Duration: time.Hour}, } - test.AssertEquals(t, len(duplicateProfiles), 2) - - badProfiles := make(map[string]issuance.ProfileConfig, 0) - badProfiles["ruhroh"] = issuance.ProfileConfig{ - AllowMustStaple: false, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, - MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 90}, - MaxValidityBackdate: config.Duration{Duration: time.Hour}, - } - test.AssertEquals(t, len(badProfiles), 1) type nameToHash struct { name string hash [32]byte } - emptyMap := make(map[string]issuance.ProfileConfig, 0) testCases := []struct { name string - profileConfigs map[string]issuance.ProfileConfig defaultName string + profileConfigs map[string]issuance.ProfileConfig expectedErrSubstr string expectedProfiles []nameToHash }{ { - name: "no profiles", - profileConfigs: emptyMap, + name: "nil profile map", + profileConfigs: nil, expectedErrSubstr: "at least one certificate profile", }, { - name: "nil profile map", - profileConfigs: nil, + name: "no profiles", + profileConfigs: map[string]issuance.ProfileConfig{}, expectedErrSubstr: "at least one certificate profile", }, { - name: "duplicate hash", - profileConfigs: duplicateProfiles, + name: "no profile matching default name", + defaultName: "default", + profileConfigs: map[string]issuance.ProfileConfig{ + "notDefault": testProfile, + }, + expectedErrSubstr: "profile object was not found for that name", + }, + { + name: "duplicate hash", + defaultName: "default", + profileConfigs: map[string]issuance.ProfileConfig{ + "default": testProfile, + "default2": testProfile, + }, expectedErrSubstr: "duplicate certificate profile hash", }, { - name: "default profiles from setup func", - profileConfigs: testCtx.certProfiles, + name: "empty profile config", + defaultName: "empty", + profileConfigs: map[string]issuance.ProfileConfig{ + "empty": {}, + }, expectedProfiles: []nameToHash{ { - name: "legacy", - hash: [32]byte{0x41, 0xda, 0xe6, 0x97, 0xec, 0x2, 0x4c, 0x68, 0x7, 0x45, 0x57, 0xa2, 0x25, 0x86, 0xbb, 0xbe, 0x5, 0x8a, 0x40, 0x5, 0x72, 0xf5, 0x3f, 0x9f, 0x89, 0x2b, 0x1a, 0x24, 0x10, 0xab, 0xfc, 0x95}, - }, - { - name: "modern", - hash: [32]byte{0x85, 0xfb, 0x36, 0xdc, 0xef, 0x1b, 0x77, 0xc9, 0x50, 0x29, 0x36, 0xe7, 0xf2, 0xf8, 0xc, 0xed, 0xc, 0x14, 0xa8, 0x11, 0x18, 0xe9, 0x9f, 0xb3, 0xc1, 0xc7, 0x78, 0x81, 0xa2, 0x6, 0x2d, 0x12}, + name: "empty", + hash: [32]byte{0xec, 0xf4, 0x43, 0xe, 0x26, 0x1, 0x8c, 0x8b, 0x85, 0x83, 0x0, 0x6f, 0x55, 0xbd, 0x45, 0xd8, 0x66, 0x88, 0x95, 0x41, 0xd0, 0x9f, 0x8, 0x2e, 0x9b, 0x2b, 0xc9, 0x8a, 0xb4, 0x86, 0x69, 0x59}, }, }, }, { - name: "no profile matching default name", - profileConfigs: badProfiles, - expectedErrSubstr: "profile object was not found for that name", - }, - { - name: "certificate profile hash changed mid-issuance", - profileConfigs: badProfiles, - defaultName: "ruhroh", + name: "default profiles from setup func", + defaultName: testCtx.defaultCertProfileName, + profileConfigs: testCtx.certProfiles, expectedProfiles: []nameToHash{ { - // We'll change the mapped hash key under the hood during - // the test. - name: "ruhroh", - hash: [32]byte{0x12, 0x20, 0xb4, 0x5e, 0xf5, 0x9, 0x68, 0x37, 0x71, 0xb8, 0x2b, 0x2d, 0x1, 0xf6, 0xd5, 0x8c, 0xae, 0x9c, 0x6d, 0xc, 0x81, 0xb8, 0x60, 0xad, 0xfe, 0x31, 0x7, 0x60, 0x7e, 0x58, 0xed, 0xa4}, + name: "legacy", + hash: [32]byte{0x1e, 0x35, 0x19, 0x16, 0x69, 0xff, 0x4, 0x7b, 0xa5, 0xb5, 0xf, 0x2b, 0x59, 0x88, 0x7e, 0xe4, 0x22, 0x23, 0xb1, 0xad, 0xfc, 0x18, 0xe9, 0x9c, 0x57, 0x3f, 0x3, 0x95, 0xe3, 0xac, 0xff, 0x3c}, + }, + { + name: "modern", + hash: [32]byte{0xa7, 0x9b, 0xb1, 0x14, 0x8e, 0x12, 0x4, 0xb4, 0xb1, 0x14, 0xe9, 0x78, 0x2e, 0x4c, 0x20, 0x91, 0x90, 0xd0, 0x2d, 0x4e, 0x30, 0x96, 0xb2, 0xcf, 0xe0, 0xff, 0x70, 0x1f, 0x4e, 0x51, 0xf, 0x1e}, }, }, }, } for _, tc := range testCases { - // This is handled by boulder-ca, not the CA package. - if tc.defaultName == "" { - tc.defaultName = testCtx.defaultCertProfileName - } t.Run(tc.name, func(t *testing.T) { t.Parallel() - tCA, err := NewCertificateAuthorityImpl( - sa, - testCtx.pa, - testCtx.boulderIssuers, - tc.defaultName, - tc.profileConfigs, - testCtx.lints, - testCtx.serialPrefix, - testCtx.maxNames, - testCtx.keyPolicy, - testCtx.logger, - testCtx.metrics, - testCtx.fc, + profiles, err := makeCertificateProfilesMap( + tc.defaultName, tc.profileConfigs, lint.NewRegistry(), ) if tc.expectedErrSubstr != "" { + test.AssertError(t, err, "profile construction should have failed") test.AssertContains(t, err.Error(), tc.expectedErrSubstr) - test.AssertError(t, err, "No profile found during CA construction.") } else { - test.AssertNotError(t, err, "Profiles should exist, but were not found") + test.AssertNotError(t, err, "profile construction should have succeeded") } if tc.expectedProfiles != nil { - test.AssertEquals(t, len(tCA.certProfiles.profileByName), len(tc.expectedProfiles)) + test.AssertEquals(t, len(profiles.profileByName), len(tc.expectedProfiles)) } for _, expected := range tc.expectedProfiles { - cpwid, ok := tCA.certProfiles.profileByName[expected.name] - test.Assert(t, ok, "Profile name was not found, but should have been") + cpwid, ok := profiles.profileByName[expected.name] + test.Assert(t, ok, fmt.Sprintf("expected profile %q not found", expected.name)) test.AssertEquals(t, cpwid.hash, expected.hash) - if tc.name == "certificate profile hash changed mid-issuance" { - // This is an attempt to simulate the hash changing, but the - // name remaining the same on a CA node in the duration - // between CA1 sending capb.IssuePrecerticateResponse and - // before the RA calls - // capb.IssueCertificateForPrecertificate. We expect the - // receiving CA2 to error that the hash we expect could not - // be found in the map. - originalHash := cpwid.hash - cpwid.hash = [32]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 6, 6, 6} - test.AssertNotEquals(t, originalHash, cpwid.hash) - } + cpwid, ok = profiles.profileByHash[expected.hash] + test.Assert(t, ok, fmt.Sprintf("expected profile %q not found", expected.hash)) + test.AssertEquals(t, cpwid.name, expected.name) } }) } diff --git a/issuance/cert.go b/issuance/cert.go index 2ca8e25de28..cad4c001720 100644 --- a/issuance/cert.go +++ b/issuance/cert.go @@ -46,6 +46,14 @@ type ProfileConfig struct { // OmitCommonName causes the CN field to be excluded from the resulting // certificate, regardless of its inclusion in the IssuanceRequest. OmitCommonName bool + // OmitKeyEncipherment causes the keyEncipherment bit to be omitted from the + // Key Usage field of all certificates (instead of only from ECDSA certs). + OmitKeyEncipherment bool + // OmitClientAuth causes the id-kp-clientAuth OID (TLS Client Authentication) + // to be omitted from the EKU extension. + OmitClientAuth bool + // OmitSKID causes the Subject Key Identifier extension to be omitted. + OmitSKID bool MaxValidityPeriod config.Duration MaxValidityBackdate config.Duration @@ -61,8 +69,11 @@ type PolicyConfig struct { // Profile is the validated structure created by reading in ProfileConfigs and IssuerConfigs type Profile struct { - allowMustStaple bool - omitCommonName bool + allowMustStaple bool + omitCommonName bool + omitKeyEncipherment bool + omitClientAuth bool + omitSKID bool maxBackdate time.Duration maxValidity time.Duration @@ -85,11 +96,14 @@ func NewProfile(profileConfig ProfileConfig, lints lint.Registry) (*Profile, err } sp := &Profile{ - allowMustStaple: profileConfig.AllowMustStaple, - omitCommonName: profileConfig.OmitCommonName, - maxBackdate: profileConfig.MaxValidityBackdate.Duration, - maxValidity: profileConfig.MaxValidityPeriod.Duration, - lints: lints, + allowMustStaple: profileConfig.AllowMustStaple, + omitCommonName: profileConfig.OmitCommonName, + omitKeyEncipherment: profileConfig.OmitKeyEncipherment, + omitClientAuth: profileConfig.OmitClientAuth, + omitSKID: profileConfig.OmitSKID, + maxBackdate: profileConfig.MaxValidityBackdate.Duration, + maxValidity: profileConfig.MaxValidityPeriod.Duration, + lints: lints, } return sp, nil @@ -121,7 +135,7 @@ func (i *Issuer) requestValid(clk clock.Clock, prof *Profile, req *IssuanceReque return errors.New("inactive issuer cannot issue precert") } - if len(req.SubjectKeyId) != 20 { + if len(req.SubjectKeyId) != 0 && len(req.SubjectKeyId) != 20 { return errors.New("unexpected subject key ID length") } @@ -162,11 +176,7 @@ func (i *Issuer) requestValid(clk clock.Clock, prof *Profile, req *IssuanceReque func (i *Issuer) generateTemplate() *x509.Certificate { template := &x509.Certificate{ - SignatureAlgorithm: i.sigAlg, - ExtKeyUsage: []x509.ExtKeyUsage{ - x509.ExtKeyUsageServerAuth, - x509.ExtKeyUsageClientAuth, - }, + SignatureAlgorithm: i.sigAlg, OCSPServer: []string{i.ocspURL}, IssuingCertificateURL: []string{i.issuerURL}, BasicConstraintsValid: true, @@ -278,6 +288,17 @@ func (i *Issuer) Prepare(prof *Profile, req *IssuanceRequest) ([]byte, *issuance // generate template from the issuer's data template := i.generateTemplate() + ekus := []x509.ExtKeyUsage{ + x509.ExtKeyUsageServerAuth, + x509.ExtKeyUsageClientAuth, + } + if prof.omitClientAuth { + ekus = []x509.ExtKeyUsage{ + x509.ExtKeyUsageServerAuth, + } + } + template.ExtKeyUsage = ekus + // populate template from the issuance request template.NotBefore, template.NotAfter = req.NotBefore, req.NotAfter template.SerialNumber = big.NewInt(0).SetBytes(req.Serial) @@ -288,12 +309,18 @@ func (i *Issuer) Prepare(prof *Profile, req *IssuanceRequest) ([]byte, *issuance switch req.PublicKey.(type) { case *rsa.PublicKey: - template.KeyUsage = x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment + if prof.omitKeyEncipherment { + template.KeyUsage = x509.KeyUsageDigitalSignature + } else { + template.KeyUsage = x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment + } case *ecdsa.PublicKey: template.KeyUsage = x509.KeyUsageDigitalSignature } - template.SubjectKeyId = req.SubjectKeyId + if !prof.omitSKID { + template.SubjectKeyId = req.SubjectKeyId + } if req.IncludeCTPoison { template.ExtraExtensions = append(template.ExtraExtensions, ctPoisonExt) diff --git a/issuance/cert_test.go b/issuance/cert_test.go index c3364015e3a..de5875347fe 100644 --- a/issuance/cert_test.go +++ b/issuance/cert_test.go @@ -322,10 +322,6 @@ func TestGenerateTemplate(t *testing.T) { expected := &x509.Certificate{ BasicConstraintsValid: true, SignatureAlgorithm: x509.SHA256WithRSA, - ExtKeyUsage: []x509.ExtKeyUsage{ - x509.ExtKeyUsageServerAuth, - x509.ExtKeyUsageClientAuth, - }, IssuingCertificateURL: []string{"http://issuer"}, OCSPServer: []string{"http://ocsp"}, CRLDistributionPoints: nil, @@ -448,11 +444,55 @@ func TestIssueCommonName(t *testing.T) { test.AssertEquals(t, cert.Subject.CommonName, "") } -func TestIssueCTPoison(t *testing.T) { +func TestIssueOmissions(t *testing.T) { fc := clock.NewFake() fc.Set(time.Now()) + + lints, err := linter.NewRegistry([]string{ + "w_ext_subject_key_identifier_missing_sub_cert", + "w_ct_sct_policy_count_unsatisfied", + "e_scts_from_same_operator", + }) + test.AssertNotError(t, err, "building test lint registry") + pc := defaultProfileConfig() + pc.OmitCommonName = true + pc.OmitKeyEncipherment = true + pc.OmitClientAuth = true + pc.OmitSKID = true + prof, err := NewProfile(pc, lints) + test.AssertNotError(t, err, "building test profile") + signer, err := newIssuer(defaultIssuerConfig(), issuerCert, issuerSigner, fc) test.AssertNotError(t, err, "NewIssuer failed") + + pk, err := rsa.GenerateKey(rand.Reader, 2048) + test.AssertNotError(t, err, "failed to generate test key") + _, issuanceToken, err := signer.Prepare(prof, &IssuanceRequest{ + PublicKey: pk.Public(), + SubjectKeyId: goodSKID, + Serial: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9}, + DNSNames: []string{"example.com"}, + CommonName: "example.com", + IncludeCTPoison: true, + NotBefore: fc.Now(), + NotAfter: fc.Now().Add(time.Hour - time.Second), + }) + test.AssertNotError(t, err, "Prepare failed") + certBytes, err := signer.Issue(issuanceToken) + test.AssertNotError(t, err, "Issue failed") + cert, err := x509.ParseCertificate(certBytes) + test.AssertNotError(t, err, "failed to parse certificate") + + test.AssertEquals(t, cert.Subject.CommonName, "") + test.AssertEquals(t, cert.KeyUsage, x509.KeyUsageDigitalSignature) + test.AssertDeepEquals(t, cert.ExtKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}) + test.AssertEquals(t, len(cert.SubjectKeyId), 0) +} + +func TestIssueCTPoison(t *testing.T) { + fc := clock.NewFake() + fc.Set(time.Now()) + signer, err := newIssuer(defaultIssuerConfig(), issuerCert, issuerSigner, fc) test.AssertNotError(t, err, "NewIssuer failed") pk, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) test.AssertNotError(t, err, "failed to generate test key") diff --git a/ra/ra.go b/ra/ra.go index bfd2386efe9..651f6517242 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -764,8 +764,13 @@ func (ra *RegistrationAuthorityImpl) matchesCSR(parsedCertificate *x509.Certific if parsedCertificate.IsCA { return berrors.InternalServerError("generated certificate can sign other certificates") } - if !slices.Equal(parsedCertificate.ExtKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}) { - return berrors.InternalServerError("generated certificate doesn't have correct key usage extensions") + for _, eku := range parsedCertificate.ExtKeyUsage { + if eku != x509.ExtKeyUsageServerAuth && eku != x509.ExtKeyUsageClientAuth { + return berrors.InternalServerError("generated certificate has unacceptable EKU") + } + } + if !slices.Contains(parsedCertificate.ExtKeyUsage, x509.ExtKeyUsageServerAuth) { + return berrors.InternalServerError("generated certificate doesn't have serverAuth EKU") } return nil diff --git a/test/config-next/ca.json b/test/config-next/ca.json index 58df3a86645..a946b940b03 100644 --- a/test/config-next/ca.json +++ b/test/config-next/ca.json @@ -46,22 +46,15 @@ "certProfiles": { "legacy": { "allowMustStaple": true, - "policies": [ - { - "oid": "2.23.140.1.2.1" - } - ], "maxValidityPeriod": "7776000s", "maxValidityBackdate": "1h5m" }, "modern": { "allowMustStaple": true, "omitCommonName": true, - "policies": [ - { - "oid": "2.23.140.1.2.1" - } - ], + "omitKeyEncipherment": true, + "omitClientAuth": true, + "omitSKID": true, "maxValidityPeriod": "583200s", "maxValidityBackdate": "1h5m" } From c13591ab82870c48c12e614c077dbbaaa1b580c1 Mon Sep 17 00:00:00 2001 From: Samantha Frank Date: Wed, 31 Jul 2024 14:46:46 -0400 Subject: [PATCH 3/3] SFE: Call RA.UnpauseAccount and handle result (#7638) Call `RA.UnpauseAccount` for valid unpause form submissions. Determine and display the appropriate outcome to the Subscriber based on the count returned by `RA.UnpauseAccount`: - If the count is zero, display the "Account already unpaused" message. - If the count equals the max number of identifiers allowed in a single request, display a page explaining the need to visit the unpause URL again. - Otherwise, display the "Successfully unpaused all N identifiers" message. Apply per-request timeout from the SFE configuration. Part of https://github.com/letsencrypt/boulder/issues/7406 --- sa/sa.go | 8 +-- sfe/pages/unpause-form.html | 2 +- sfe/pages/unpause-status.html | 34 ++++++++++--- sfe/sfe.go | 85 +++++++++++++++++++++----------- sfe/sfe_test.go | 26 ++++++++-- test/integration/pausing_test.go | 10 +++- unpause/unpause.go | 13 +++++ 7 files changed, 133 insertions(+), 45 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index a565139c77b..747029d6d39 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -25,6 +25,7 @@ import ( blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/revocation" sapb "github.com/letsencrypt/boulder/sa/proto" + "github.com/letsencrypt/boulder/unpause" ) var ( @@ -1411,10 +1412,9 @@ func (ssa *SQLStorageAuthority) UnpauseAccount(ctx context.Context, req *sapb.Re return nil, errIncompleteRequest } - const batchSize = 10000 total := &sapb.Count{} - for i := 0; i < 5; i++ { + for i := 0; i < unpause.MaxBatches; i++ { result, err := ssa.dbMap.ExecContext(ctx, ` UPDATE paused SET unpausedAt = ? @@ -1424,7 +1424,7 @@ func (ssa *SQLStorageAuthority) UnpauseAccount(ctx context.Context, req *sapb.Re LIMIT ?`, ssa.clk.Now(), req.Id, - batchSize, + unpause.BatchSize, ) if err != nil { return nil, err @@ -1436,7 +1436,7 @@ func (ssa *SQLStorageAuthority) UnpauseAccount(ctx context.Context, req *sapb.Re } total.Count += rowsAffected - if rowsAffected < batchSize { + if rowsAffected < unpause.BatchSize { // Fewer than batchSize rows were updated, so we're done. break } diff --git a/sfe/pages/unpause-form.html b/sfe/pages/unpause-form.html index 3d396c0eff2..4870abc8da7 100644 --- a/sfe/pages/unpause-form.html +++ b/sfe/pages/unpause-form.html @@ -56,7 +56,7 @@

Ready to unpause?

resume requesting certificates for all affected identifiers associated with your account, not just those listed above.

-
+
diff --git a/sfe/pages/unpause-status.html b/sfe/pages/unpause-status.html index 59f0df5f93f..3f1c7b5b6ad 100644 --- a/sfe/pages/unpause-status.html +++ b/sfe/pages/unpause-status.html @@ -2,26 +2,46 @@
- {{ if eq .Successful true }} - -

Account successfully unpaused

+ {{ if and .Successful (gt .Count 0) (lt .Count .Limit) }} +

Successfully unpaused all {{ .Count }} identifier(s)

To obtain a new certificate, re-attempt issuance with your ACME client. Future repeated validation failures with no successes will result in identifiers being paused again.

- {{ else }} + {{ else if and .Successful (eq .Count .Limit)}} +

Some identifiers were unpaused

+

+ We can only unpause a limited number of identifiers for each request ({{ + .Limit }}). There are potentially more identifiers paused for your + account. +

+

+ To attempt to unpause more identifiers, visit the unpause URL from + your logs again and click the "Please Unpause My Account" button. +

-

An error occurred while unpausing your account

+ {{ else if and .Successful (eq .Count 0) }} +

Account already unpaused

- Please try again later. If you face continued difficulties, please visit our community support forum for troubleshooting and advice.

+ {{ else }} +

An error occurred while unpausing your account

+

+ Please try again later. If you face continued difficulties, please visit + our community support + forum + for troubleshooting and advice. +

+ {{ end }}
-{{template "footer"}} +{{ template "footer" }} diff --git a/sfe/sfe.go b/sfe/sfe.go index 19a6e6797c5..01385c2eae8 100644 --- a/sfe/sfe.go +++ b/sfe/sfe.go @@ -6,6 +6,7 @@ import ( "fmt" "io/fs" "net/http" + "net/url" "strconv" "strings" "text/template" @@ -84,23 +85,35 @@ func NewSelfServiceFrontEndImpl( return sfe, nil } +// handleWithTimeout registers a handler with a timeout using an +// http.TimeoutHandler. +func (sfe *SelfServiceFrontEndImpl) handleWithTimeout(mux *http.ServeMux, path string, handler http.HandlerFunc) { + timeout := sfe.requestTimeout + if timeout <= 0 { + // Default to 5 minutes if no timeout is set. + timeout = 5 * time.Minute + } + timeoutHandler := http.TimeoutHandler(handler, timeout, "Request timed out") + mux.Handle(path, timeoutHandler) +} + // Handler returns an http.Handler that uses various functions for various // non-ACME-specified paths. Each endpoint should have a corresponding HTML // page that shares the same name as the endpoint. func (sfe *SelfServiceFrontEndImpl) Handler(stats prometheus.Registerer, oTelHTTPOptions ...otelhttp.Option) http.Handler { - m := http.NewServeMux() + mux := http.NewServeMux() sfs, _ := fs.Sub(staticFS, "static") staticAssetsHandler := http.StripPrefix("/static/", http.FileServerFS(sfs)) + mux.Handle("GET /static/", staticAssetsHandler) - m.Handle("GET /static/", staticAssetsHandler) - m.HandleFunc("/", sfe.Index) - m.HandleFunc("GET /build", sfe.BuildID) - m.HandleFunc("GET "+unpause.GetForm, sfe.UnpauseForm) - m.HandleFunc("POST "+unpausePostForm, sfe.UnpauseSubmit) - m.HandleFunc("GET "+unpauseStatus, sfe.UnpauseStatus) + sfe.handleWithTimeout(mux, "/", sfe.Index) + sfe.handleWithTimeout(mux, "GET /build", sfe.BuildID) + sfe.handleWithTimeout(mux, "GET "+unpause.GetForm, sfe.UnpauseForm) + sfe.handleWithTimeout(mux, "POST "+unpausePostForm, sfe.UnpauseSubmit) + sfe.handleWithTimeout(mux, "GET "+unpauseStatus, sfe.UnpauseStatus) - return measured_http.New(m, sfe.clk, stats, oTelHTTPOptions...) + return measured_http.New(mux, sfe.clk, stats, oTelHTTPOptions...) } // renderTemplate takes the name of an HTML template and optional dynamicData @@ -139,7 +152,7 @@ func (sfe *SelfServiceFrontEndImpl) BuildID(response http.ResponseWriter, reques func (sfe *SelfServiceFrontEndImpl) UnpauseForm(response http.ResponseWriter, request *http.Request) { incomingJWT := request.URL.Query().Get("jwt") - regID, identifiers, err := sfe.parseUnpauseJWT(incomingJWT) + accountID, identifiers, err := sfe.parseUnpauseJWT(incomingJWT) if err != nil { if errors.Is(err, jwt.ErrExpired) { // JWT expired before the Subscriber visited the unpause page. @@ -157,15 +170,14 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseForm(response http.ResponseWriter, re } type tmplData struct { - UnpauseFormRedirectionPath string - JWT string - AccountID int64 - Identifiers []string + PostPath string + JWT string + AccountID int64 + Identifiers []string } - // Serve the actual unpause page given to a Subscriber. Populates the - // unpause form with the JWT from the URL. - sfe.renderTemplate(response, "unpause-form.html", tmplData{unpausePostForm, incomingJWT, regID, identifiers}) + // Present the unpause form to the Subscriber. + sfe.renderTemplate(response, "unpause-form.html", tmplData{unpausePostForm, incomingJWT, accountID, identifiers}) } // UnpauseSubmit serves a page showing the result of the unpause form submission. @@ -174,7 +186,7 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseForm(response http.ResponseWriter, re func (sfe *SelfServiceFrontEndImpl) UnpauseSubmit(response http.ResponseWriter, request *http.Request) { incomingJWT := request.URL.Query().Get("jwt") - _, _, err := sfe.parseUnpauseJWT(incomingJWT) + accountID, _, err := sfe.parseUnpauseJWT(incomingJWT) if err != nil { if errors.Is(err, jwt.ErrExpired) { // JWT expired before the Subscriber could click the unpause button. @@ -191,13 +203,19 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseSubmit(response http.ResponseWriter, return } - // TODO(#7536) Send gRPC request to the RA informing it to unpause - // the account specified in the claim. At this point we should wait - // for the RA to process the request before returning to the client, - // just in case the request fails. + unpaused, err := sfe.ra.UnpauseAccount(request.Context(), &rapb.UnpauseAccountRequest{ + RegistrationID: accountID, + }) + if err != nil { + sfe.unpauseFailed(response) + return + } - // Success, the account has been unpaused. - http.Redirect(response, request, unpauseStatus, http.StatusFound) + // Redirect to the unpause status page with the count of unpaused + // identifiers. + params := url.Values{} + params.Add("count", fmt.Sprintf("%d", unpaused.Count)) + http.Redirect(response, request, unpauseStatus+"?"+params.Encode(), http.StatusFound) } func (sfe *SelfServiceFrontEndImpl) unpauseRequestMalformed(response http.ResponseWriter) { @@ -210,14 +228,20 @@ func (sfe *SelfServiceFrontEndImpl) unpauseTokenExpired(response http.ResponseWr type unpauseStatusTemplate struct { Successful bool + Limit int64 + Count int64 } func (sfe *SelfServiceFrontEndImpl) unpauseFailed(response http.ResponseWriter) { sfe.renderTemplate(response, "unpause-status.html", unpauseStatusTemplate{Successful: false}) } -func (sfe *SelfServiceFrontEndImpl) unpauseSuccessful(response http.ResponseWriter) { - sfe.renderTemplate(response, "unpause-status.html", unpauseStatusTemplate{Successful: true}) +func (sfe *SelfServiceFrontEndImpl) unpauseSuccessful(response http.ResponseWriter, count int64) { + sfe.renderTemplate(response, "unpause-status.html", unpauseStatusTemplate{ + Successful: true, + Limit: unpause.RequestLimit, + Count: count}, + ) } // UnpauseStatus displays a success message to the Subscriber indicating that @@ -229,10 +253,13 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseStatus(response http.ResponseWriter, return } - // TODO(#7580) This should only be reachable after a client has clicked the - // "Please unblock my account" button and that request succeeding. No one - // should be able to access this page otherwise. - sfe.unpauseSuccessful(response) + count, err := strconv.ParseInt(request.URL.Query().Get("count"), 10, 64) + if err != nil || count < 0 { + sfe.unpauseFailed(response) + return + } + + sfe.unpauseSuccessful(response, count) } // parseUnpauseJWT extracts and returns the subscriber's registration ID and a diff --git a/sfe/sfe_test.go b/sfe/sfe_test.go index 9ee887b8506..f6b851f5821 100644 --- a/sfe/sfe_test.go +++ b/sfe/sfe_test.go @@ -182,14 +182,34 @@ func TestUnpausePaths(t *testing.T) { URL: mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=" + validJWT)), }) test.AssertEquals(t, responseWriter.Code, http.StatusFound) - test.AssertEquals(t, unpauseStatus, responseWriter.Result().Header.Get("Location")) + test.AssertEquals(t, unpauseStatus+"?count=0", responseWriter.Result().Header.Get("Location")) // Redirecting after a successful unpause POST displays the success page. responseWriter = httptest.NewRecorder() sfe.UnpauseStatus(responseWriter, &http.Request{ Method: "GET", - URL: mustParseURL(unpauseStatus), + URL: mustParseURL(unpauseStatus + "?count=1"), }) test.AssertEquals(t, responseWriter.Code, http.StatusOK) - test.AssertContains(t, responseWriter.Body.String(), "Account successfully unpaused") + test.AssertContains(t, responseWriter.Body.String(), "Successfully unpaused all 1 identifier(s)") + + // Redirecting after a successful unpause POST with a count of 0 displays + // the already unpaused page. + responseWriter = httptest.NewRecorder() + sfe.UnpauseStatus(responseWriter, &http.Request{ + Method: "GET", + URL: mustParseURL(unpauseStatus + "?count=0"), + }) + test.AssertEquals(t, responseWriter.Code, http.StatusOK) + test.AssertContains(t, responseWriter.Body.String(), "Account already unpaused") + + // Redirecting after a successful unpause POST with a count equal to the + // maximum number of identifiers displays the success with caveat page. + responseWriter = httptest.NewRecorder() + sfe.UnpauseStatus(responseWriter, &http.Request{ + Method: "GET", + URL: mustParseURL(unpauseStatus + "?count=" + fmt.Sprintf("%d", unpause.RequestLimit)), + }) + test.AssertEquals(t, responseWriter.Code, http.StatusOK) + test.AssertContains(t, responseWriter.Body.String(), "Some identifiers were unpaused") } diff --git a/test/integration/pausing_test.go b/test/integration/pausing_test.go index 8eb194ae09f..c4e7c461a24 100644 --- a/test/integration/pausing_test.go +++ b/test/integration/pausing_test.go @@ -20,7 +20,7 @@ import ( "github.com/letsencrypt/boulder/test" ) -func TestPausedOrderFails(t *testing.T) { +func TestIdentifiersPausedForAccount(t *testing.T) { t.Parallel() if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { @@ -72,4 +72,12 @@ func TestPausedOrderFails(t *testing.T) { test.AssertError(t, err, "Should not be able to issue a certificate for a paused domain") test.AssertContains(t, err.Error(), "Your account is temporarily prevented from requesting certificates for") test.AssertContains(t, err.Error(), "https://boulder.service.consul:4003/sfe/v1/unpause?jwt=") + + _, err = saClient.UnpauseAccount(context.Background(), &sapb.RegistrationID{ + Id: regID, + }) + test.AssertNotError(t, err, "Failed to unpause domain") + + _, err = authAndIssue(c, nil, []string{domain}, true) + test.AssertNotError(t, err, "Should be able to issue a certificate for an unpaused domain") } diff --git a/unpause/unpause.go b/unpause/unpause.go index eeb097a3f6f..f88b589f158 100644 --- a/unpause/unpause.go +++ b/unpause/unpause.go @@ -21,6 +21,19 @@ const ( APIPrefix = "/sfe/" + APIVersion GetForm = APIPrefix + "/unpause" + // BatchSize is the maximum number of identifiers that the SA will unpause + // in a single batch. + BatchSize = 10000 + + // MaxBatches is the maximum number of batches that the SA will unpause in a + // single request. + MaxBatches = 5 + + // RequestLimit is the maximum number of identifiers that the SA will + // unpause in a single request. This is used by the SFE to infer whether + // there are more identifiers to unpause. + RequestLimit = BatchSize * MaxBatches + // JWT defaultIssuer = "WFE" defaultAudience = "SFE Unpause"