From cde4b9c90f58a013a431c8b4880b15b57ca14b9c Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <jsha+github@letsencrypt.org>
Date: Tue, 20 Jun 2023 12:42:21 -0700
Subject: [PATCH] wfe: return proper error for goodkey timeout (#6938)

In WFE, we do a goodkey check when validating a self-authenticated POST
(i.e. when creating an account). For a while, that was a purely local
check, looking at a list of bad keys or bad moduluses, or checking for
factorability. At some point we also added a backend check, querying the
SA to see if a key was blocked. However, we did not update this one code
path to distinguish "bad key" from "timeout querying SA." That meant
that sometimes we would give a badPublicKey Problem Document when we
should have given an internalServerError.

Related:
https://github.com/letsencrypt/boulder/issues/6795#issuecomment-1574217398
---
 wfe2/verify.go      |  8 ++++++--
 wfe2/verify_test.go | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/wfe2/verify.go b/wfe2/verify.go
index 2d1d82ae2..3b53bc2bc 100644
--- a/wfe2/verify.go
+++ b/wfe2/verify.go
@@ -18,6 +18,7 @@ import (
 
 	"github.com/letsencrypt/boulder/core"
 	berrors "github.com/letsencrypt/boulder/errors"
+	"github.com/letsencrypt/boulder/goodkey"
 	"github.com/letsencrypt/boulder/grpc"
 	"github.com/letsencrypt/boulder/nonce"
 	noncepb "github.com/letsencrypt/boulder/nonce/proto"
@@ -682,8 +683,11 @@ func (wfe *WebFrontEndImpl) validSelfAuthenticatedPOST(
 	// If the key doesn't meet the GoodKey policy return a problem
 	err := wfe.keyPolicy.GoodKey(ctx, pubKey.Key)
 	if err != nil {
-		wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "JWKRejectedByGoodKey"}).Inc()
-		return nil, nil, probs.BadPublicKey(err.Error())
+		if errors.Is(err, goodkey.ErrBadKey) {
+			wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "JWKRejectedByGoodKey"}).Inc()
+			return nil, nil, probs.BadPublicKey(err.Error())
+		}
+		return nil, nil, probs.ServerInternal("error checking key quality")
 	}
 
 	return payload, pubKey, nil
diff --git a/wfe2/verify_test.go b/wfe2/verify_test.go
index 962f3abc2..3d893fe96 100644
--- a/wfe2/verify_test.go
+++ b/wfe2/verify_test.go
@@ -14,6 +14,7 @@ import (
 
 	"github.com/letsencrypt/boulder/core"
 	corepb "github.com/letsencrypt/boulder/core/proto"
+	"github.com/letsencrypt/boulder/goodkey"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
 	"github.com/letsencrypt/boulder/mocks"
 	"github.com/letsencrypt/boulder/probs"
@@ -1495,6 +1496,40 @@ func TestValidPOSTForAccountSwappedKey(t *testing.T) {
 	test.AssertEquals(t, prob.Detail, "JWS verification error")
 }
 
+func TestValidSelfAuthenticatedPOSTGoodKeyErrors(t *testing.T) {
+	wfe, _, signer := setupWFE(t)
+
+	timeoutErrCheckFunc := func(ctx context.Context, keyHash []byte) (bool, error) {
+		return false, context.DeadlineExceeded
+	}
+
+	kp, err := goodkey.NewKeyPolicy(&goodkey.Config{}, timeoutErrCheckFunc)
+	test.AssertNotError(t, err, "making key policy")
+
+	wfe.keyPolicy = kp
+
+	_, _, validJWSBody := signer.embeddedJWK(nil, "http://localhost/test", `{"test":"passed"}`)
+	request := makePostRequestWithPath("test", validJWSBody)
+
+	_, _, prob := wfe.validSelfAuthenticatedPOST(context.Background(), request)
+	test.AssertEquals(t, prob.Type, probs.ServerInternalProblem)
+
+	badKeyCheckFunc := func(ctx context.Context, keyHash []byte) (bool, error) {
+		return false, fmt.Errorf("oh no: %w", goodkey.ErrBadKey)
+	}
+
+	kp, err = goodkey.NewKeyPolicy(&goodkey.Config{}, badKeyCheckFunc)
+	test.AssertNotError(t, err, "making key policy")
+
+	wfe.keyPolicy = kp
+
+	_, _, validJWSBody = signer.embeddedJWK(nil, "http://localhost/test", `{"test":"passed"}`)
+	request = makePostRequestWithPath("test", validJWSBody)
+
+	_, _, prob = wfe.validSelfAuthenticatedPOST(context.Background(), request)
+	test.AssertEquals(t, prob.Type, probs.BadPublicKeyProblem)
+}
+
 func TestValidSelfAuthenticatedPOST(t *testing.T) {
 	wfe, _, signer := setupWFE(t)