SFE: Improve UX when the JWT is malformed or expired

sfe/pages/unpause-expired.html

@@ -0,0 +1,19 @@
+{{ template "header" }}
+
+<div class="section">
+    <h1>Expired unpause URL</h1>
+    <p>
+        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.
+    </p>
+    <p>
+        If you continue to encounter difficulties, or if you need more help, our
+        <a href='https://community.letsencrypt.org'>community support forum</a>
+        is a great resource for troubleshooting and advice.
+    </p>
+</div>
+
+{{template "footer"}}

sfe/pages/unpause-status.html

@@ -2,7 +2,7 @@
 
 <div class="section">
 
-    {{ if eq .UnpauseSuccessful true }}
+    {{ if eq .Successful true }}
 
     <h1>Account successfully unpaused</h1>
     <p>

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
 	}

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()

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{

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,