From 3df0c4b111a0ab2d60eae30b13181a7543941160 Mon Sep 17 00:00:00 2001 From: Samantha Date: Fri, 26 Jul 2024 17:07:09 -0400 Subject: [PATCH 1/2] SFE: Handle various unpause failure scenarios --- sfe/pages/unpause-expired.html | 19 ++++++++ sfe/pages/unpause-status.html | 2 +- sfe/sfe.go | 79 +++++++++++++++++++++------------- sfe/sfe_test.go | 43 +++++++++++++++--- unpause/unpause.go | 15 ++++--- unpause/unpause_test.go | 14 +++--- 6 files changed, 123 insertions(+), 49 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..6ba6cabae8e 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,27 @@ 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) + 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. + sfe.unpauseRequestMalformed(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 +174,30 @@ 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 as we control JWT creation and redemption. func (sfe *SelfServiceFrontEndImpl) UnpauseSubmit(response http.ResponseWriter, request *http.Request) { incomingJWT := request.URL.Query().Get("jwt") - if incomingJWT == "" { - sfe.unpauseInvalidRequest(response) + if incomingJWT == "" || len(strings.Split(incomingJWT, ".")) != 3 { + // This should never happen if the request came from our form. + sfe.unpauseRequestMalformed(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 +210,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) } -// 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}) +type unpauseStatusTemplate struct { + Successful bool +} + +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}) } // UnpauseStatus displays a success message to the Subscriber indicating that @@ -218,19 +242,14 @@ 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. 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") - } - - 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 3bca74fa41450f7154ed9fc9abf46c583851cca4 Mon Sep 17 00:00:00 2001 From: Samantha Date: Mon, 29 Jul 2024 17:36:15 -0400 Subject: [PATCH 2/2] Address comments. --- sfe/sfe.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/sfe/sfe.go b/sfe/sfe.go index 6ba6cabae8e..19a6e6797c5 100644 --- a/sfe/sfe.go +++ b/sfe/sfe.go @@ -138,12 +138,6 @@ 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 == "" || 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. - sfe.unpauseRequestMalformed(response) - return - } regID, identifiers, err := sfe.parseUnpauseJWT(incomingJWT) if err != nil { @@ -175,14 +169,10 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseForm(response http.ResponseWriter, re } // UnpauseSubmit serves a page showing the result of the unpause form submission. -// CSRF is not addressed as we control JWT creation and redemption. +// 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 == "" || len(strings.Split(incomingJWT, ".")) != 3 { - // This should never happen if the request came from our form. - sfe.unpauseRequestMalformed(response) - return - } _, _, err := sfe.parseUnpauseJWT(incomingJWT) if err != nil { @@ -247,8 +237,16 @@ func (sfe *SelfServiceFrontEndImpl) UnpauseStatus(response http.ResponseWriter, // 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) { + 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, unpause.APIVersion, sfe.clk) if err != nil { return 0, nil, err