wfe/wfe2: make JWS signature alg error msgs match reality (#4519)

Errors that were being returned in the checkAlgorithm methods of both wfe and
wfe2 didn't really match up to what was actually being checked. This change
attempts to bring the errors in line with what is actually being tested.

Fixes #4452.

test/test-tools.go

@@ -83,6 +83,7 @@ func AssertDeepEquals(t *testing.T, one interface{}, two interface{}) {
 // AssertMarshaledEquals marshals one and two to JSON, and then uses
 // the equality operator to measure them
 func AssertMarshaledEquals(t *testing.T, one interface{}, two interface{}) {
+	t.Helper()
 	oneJSON, err := json.Marshal(one)
 	AssertNotError(t, err, "Could not marshal 1st argument")
 	twoJSON, err := json.Marshal(two)

wfe/jose.go

@@ -22,7 +22,7 @@ func algorithmForKey(key *jose.JSONWebKey) (string, error) {
 			return string(jose.ES512), nil
 		}
 	}
-	return "", fmt.Errorf("no signature algorithms suitable for given key type")
+	return "", fmt.Errorf("JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521")
 }
 
 const (
@@ -31,26 +31,35 @@ const (
 	invalidAlgorithmOnKey = "WFE.Errors.InvalidAlgorithmOnKey"
 )
 
+var supportedAlgs = map[string]bool{
+	string(jose.RS256): true,
+	string(jose.ES256): true,
+	string(jose.ES384): true,
+	string(jose.ES512): true,
+}
+
 // Check that (1) there is a suitable algorithm for the provided key based on its
 // Golang type, (2) the Algorithm field on the JWK is either absent, or matches
 // that algorithm, and (3) the Algorithm field on the JWK is present and matches
-// that algorithm. Precondition: parsedJws must have exactly one signature on
+// that algorithm. Precondition: parsedJWS must have exactly one signature on
 // it. Returns stat name to increment if err is non-nil.
-func checkAlgorithm(key *jose.JSONWebKey, parsedJws *jose.JSONWebSignature) (string, error) {
-	algorithm, err := algorithmForKey(key)
+func checkAlgorithm(key *jose.JSONWebKey, parsedJWS *jose.JSONWebSignature) (string, error) {
+	sigHeaderAlg := parsedJWS.Signatures[0].Header.Algorithm
+	if !supportedAlgs[sigHeaderAlg] {
+		return invalidJWSAlgorithm, fmt.Errorf(
+			"JWS signature header contains unsupported algorithm %q, expected one of RS256, ES256, ES384 or ES512",
+			parsedJWS.Signatures[0].Header.Algorithm,
+		)
+	}
+	expectedAlg, err := algorithmForKey(key)
 	if err != nil {
 		return noAlgorithmForKey, err
 	}
-	jwsAlgorithm := parsedJws.Signatures[0].Header.Algorithm
-	if jwsAlgorithm != algorithm {
-		return invalidJWSAlgorithm, fmt.Errorf(
-			"signature type '%s' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
-			jwsAlgorithm,
-		)
+	if sigHeaderAlg != string(expectedAlg) {
+		return invalidJWSAlgorithm, fmt.Errorf("JWS signature header algorithm %q does not match expected algorithm %q for JWK", sigHeaderAlg, string(expectedAlg))
 	}
-	if key.Algorithm != "" && key.Algorithm != algorithm {
-		return invalidAlgorithmOnKey, fmt.Errorf("algorithm '%s' on JWK is unacceptable",
-			key.Algorithm)
+	if key.Algorithm != "" && key.Algorithm != string(expectedAlg) {
+		return invalidAlgorithmOnKey, fmt.Errorf("JWK key header algorithm %q does not match expected algorithm %q for JWK", key.Algorithm, string(expectedAlg))
 	}
 	return "", nil
 }

wfe/jose_test.go

@@ -1,6 +1,7 @@
 package wfe
 
 import (
+	"crypto/dsa"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rsa"
@@ -28,7 +29,7 @@ func TestRejectsNone(t *testing.T) {
 	if prob == nil {
 		t.Fatalf("verifyPOST did not reject JWS with alg: 'none'")
 	}
-	if prob.Detail != "signature type 'none' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512" {
+	if prob.Detail != "JWS signature header contains unsupported algorithm \"none\", expected one of RS256, ES256, ES384 or ES512" {
 		t.Fatalf("verifyPOST rejected JWS with alg: 'none', but for wrong reason: %#v", prob)
 	}
 }
@@ -52,7 +53,7 @@ func TestRejectsHS256(t *testing.T) {
 	if prob == nil {
 		t.Fatalf("verifyPOST did not reject JWS with alg: 'HS256'")
 	}
-	expected := "signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512"
+	expected := "JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512"
 	if prob.Detail != expected {
 		t.Fatalf("verifyPOST rejected JWS with alg: 'none', but for wrong reason: got %q, wanted %q", prob, expected)
 	}
@@ -66,17 +67,7 @@ func TestCheckAlgorithm(t *testing.T) {
 		expectedStat string
 	}{
 		{
-			jose.JSONWebKey{
-				Algorithm: "HS256",
-			},
-			jose.JSONWebSignature{},
-			"no signature algorithms suitable for given key type",
-			"WFE.Errors.NoAlgorithmForKey",
-		},
-		{
-			jose.JSONWebKey{
-				Key: &rsa.PublicKey{},
-			},
+			jose.JSONWebKey{},
 			jose.JSONWebSignature{
 				Signatures: []jose.Signature{
 					{
@@ -86,25 +77,41 @@ func TestCheckAlgorithm(t *testing.T) {
 					},
 				},
 			},
-			"signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
-			"WFE.Errors.InvalidJWSAlgorithm",
+			"JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512",
+			invalidJWSAlgorithm,
 		},
 		{
 			jose.JSONWebKey{
-				Algorithm: "HS256",
+				Key: &dsa.PublicKey{},
+			},
+			jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Header: jose.Header{
+							Algorithm: "ES512",
+						},
+					},
+				},
+			},
+			"JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521",
+			noAlgorithmForKey,
+		},
+		{
+			jose.JSONWebKey{
+				Algorithm: "RS256",
 				Key:       &rsa.PublicKey{},
 			},
 			jose.JSONWebSignature{
 				Signatures: []jose.Signature{
 					{
 						Header: jose.Header{
-							Algorithm: "HS256",
+							Algorithm: "ES512",
 						},
 					},
 				},
 			},
-			"signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
-			"WFE.Errors.InvalidJWSAlgorithm",
+			"JWS signature header algorithm \"ES512\" does not match expected algorithm \"RS256\" for JWK",
+			invalidJWSAlgorithm,
 		},
 		{
 			jose.JSONWebKey{
@@ -120,8 +127,8 @@ func TestCheckAlgorithm(t *testing.T) {
 					},
 				},
 			},
-			"algorithm 'HS256' on JWK is unacceptable",
-			"WFE.Errors.InvalidAlgorithmOnKey",
+			"JWK key header algorithm \"HS256\" does not match expected algorithm \"RS256\" for JWK",
+			invalidAlgorithmOnKey,
 		},
 	}
 	for i, tc := range testCases {

wfe2/verify.go

@@ -2,7 +2,6 @@ package wfe2
 
 import (
 	"context"
-	"crypto"
 	"crypto/ecdsa"
 	"crypto/rsa"
 	"encoding/json"
@@ -27,29 +26,28 @@ import (
 // POST requests with a JWS body must have the following Content-Type header
 const expectedJWSContentType = "application/jose+json"
 
-var sigAlgErr = errors.New("no signature algorithms suitable for given key type")
-
-func sigAlgorithmForECDSAKey(key *ecdsa.PublicKey) (jose.SignatureAlgorithm, error) {
-	params := key.Params()
-	switch params.Name {
-	case "P-256":
-		return jose.ES256, nil
-	case "P-384":
-		return jose.ES384, nil
-	case "P-521":
-		return jose.ES512, nil
-	}
-	return "", sigAlgErr
-}
-
-func sigAlgorithmForKey(key crypto.PublicKey) (jose.SignatureAlgorithm, error) {
-	switch k := key.(type) {
+func sigAlgorithmForKey(key *jose.JSONWebKey) (jose.SignatureAlgorithm, error) {
+	switch k := key.Key.(type) {
 	case *rsa.PublicKey:
 		return jose.RS256, nil
 	case *ecdsa.PublicKey:
-		return sigAlgorithmForECDSAKey(k)
+		switch k.Params().Name {
+		case "P-256":
+			return jose.ES256, nil
+		case "P-384":
+			return jose.ES384, nil
+		case "P-521":
+			return jose.ES512, nil
+		}
 	}
-	return "", sigAlgErr
+	return "", errors.New("JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521")
+}
+
+var supportedAlgs = map[string]bool{
+	string(jose.RS256): true,
+	string(jose.ES256): true,
+	string(jose.ES384): true,
+	string(jose.ES512): true,
 }
 
 // Check that (1) there is a suitable algorithm for the provided key based on its
@@ -58,19 +56,22 @@ func sigAlgorithmForKey(key crypto.PublicKey) (jose.SignatureAlgorithm, error) {
 // that algorithm. Precondition: parsedJws must have exactly one signature on
 // it.
 func checkAlgorithm(key *jose.JSONWebKey, parsedJWS *jose.JSONWebSignature) error {
-	algorithm, err := sigAlgorithmForKey(key.Key)
+	sigHeaderAlg := parsedJWS.Signatures[0].Header.Algorithm
+	if !supportedAlgs[sigHeaderAlg] {
+		return fmt.Errorf(
+			"JWS signature header contains unsupported algorithm %q, expected one of RS256, ES256, ES384 or ES512",
+			parsedJWS.Signatures[0].Header.Algorithm,
+		)
+	}
+	expectedAlg, err := sigAlgorithmForKey(key)
 	if err != nil {
 		return err
 	}
-	jwsAlgorithm := parsedJWS.Signatures[0].Header.Algorithm
-	if jwsAlgorithm != string(algorithm) {
-		return fmt.Errorf(
-			"signature type '%s' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
-			jwsAlgorithm,
-		)
+	if sigHeaderAlg != string(expectedAlg) {
+		return fmt.Errorf("JWS signature header algorithm %q does not match expected algorithm %q for JWK", sigHeaderAlg, string(expectedAlg))
 	}
-	if key.Algorithm != "" && key.Algorithm != string(algorithm) {
-		return fmt.Errorf("algorithm '%s' on JWK is unacceptable", key.Algorithm)
+	if key.Algorithm != "" && key.Algorithm != string(expectedAlg) {
+		return fmt.Errorf("JWK key header algorithm %q does not match expected algorithm %q for JWK", key.Algorithm, string(expectedAlg))
 	}
 	return nil
 }

wfe2/verify_test.go

@@ -3,6 +3,7 @@ package wfe2
 import (
 	"context"
 	"crypto"
+	"crypto/dsa"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rsa"
@@ -26,15 +27,15 @@ func sigAlgForKey(t *testing.T, key interface{}) jose.SignatureAlgorithm {
 	var err error
 	// Gracefully handle the case where a non-pointer public key is given where
 	// sigAlgorithmForKey always wants a pointer. It may be tempting to try and do
-	// `sigAlgorithmForKey(&key)` without a type switch but this produces
+	// `sigAlgorithmForKey(&jose.JSONWebKey{Key: &key})` without a type switch but this produces
 	// `*interface {}` and not the desired `*rsa.PublicKey` or `*ecdsa.PublicKey`.
 	switch k := key.(type) {
 	case rsa.PublicKey:
-		sigAlg, err = sigAlgorithmForKey(&k)
+		sigAlg, err = sigAlgorithmForKey(&jose.JSONWebKey{Key: &k})
 	case ecdsa.PublicKey:
-		sigAlg, err = sigAlgorithmForKey(&k)
+		sigAlg, err = sigAlgorithmForKey(&jose.JSONWebKey{Key: &k})
 	default:
-		sigAlg, err = sigAlgorithmForKey(k)
+		sigAlg, err = sigAlgorithmForKey(&jose.JSONWebKey{Key: k})
 	}
 	test.Assert(t, err == nil, fmt.Sprintf("Error getting signature algorithm for key %#v", key))
 	return sigAlg
@@ -185,7 +186,7 @@ func TestRejectsNone(t *testing.T) {
 	if err == nil {
 		t.Fatalf("checkAlgorithm did not reject JWS with alg: 'none'")
 	}
-	if err.Error() != "signature type 'none' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512" {
+	if err.Error() != "JWS signature header contains unsupported algorithm \"none\", expected one of RS256, ES256, ES384 or ES512" {
 		t.Fatalf("checkAlgorithm rejected JWS with alg: 'none', but for wrong reason: %#v", err)
 	}
 }
@@ -216,9 +217,9 @@ func TestRejectsHS256(t *testing.T) {
 	if err == nil {
 		t.Fatalf("checkAlgorithm did not reject JWS with alg: 'HS256'")
 	}
-	expected := "signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512"
+	expected := "JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512"
 	if err.Error() != expected {
-		t.Fatalf("checkAlgorithm rejected JWS with alg: 'none', but for wrong reason: got '%s', wanted %s", err.Error(), expected)
+		t.Fatalf("checkAlgorithm rejected JWS with alg: 'none', but for wrong reason: got %q, wanted %q", err.Error(), expected)
 	}
 }
 
@@ -229,16 +230,7 @@ func TestCheckAlgorithm(t *testing.T) {
 		expectedErr string
 	}{
 		{
-			jose.JSONWebKey{
-				Algorithm: "HS256",
-			},
-			jose.JSONWebSignature{},
-			"no signature algorithms suitable for given key type",
-		},
-		{
-			jose.JSONWebKey{
-				Key: &rsa.PublicKey{},
-			},
+			jose.JSONWebKey{},
 			jose.JSONWebSignature{
 				Signatures: []jose.Signature{
 					{
@@ -248,23 +240,38 @@ func TestCheckAlgorithm(t *testing.T) {
 					},
 				},
 			},
-			"signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
+			"JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512",
 		},
 		{
 			jose.JSONWebKey{
-				Algorithm: "HS256",
+				Key: &dsa.PublicKey{},
+			},
+			jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Header: jose.Header{
+							Algorithm: "ES512",
+						},
+					},
+				},
+			},
+			"JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521",
+		},
+		{
+			jose.JSONWebKey{
+				Algorithm: "RS256",
 				Key:       &rsa.PublicKey{},
 			},
 			jose.JSONWebSignature{
 				Signatures: []jose.Signature{
 					{
 						Header: jose.Header{
-							Algorithm: "HS256",
+							Algorithm: "ES512",
 						},
 					},
 				},
 			},
-			"signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
+			"JWS signature header algorithm \"ES512\" does not match expected algorithm \"RS256\" for JWK",
 		},
 		{
 			jose.JSONWebKey{
@@ -280,13 +287,13 @@ func TestCheckAlgorithm(t *testing.T) {
 					},
 				},
 			},
-			"algorithm 'HS256' on JWK is unacceptable",
+			"JWK key header algorithm \"HS256\" does not match expected algorithm \"RS256\" for JWK",
 		},
 	}
 	for i, tc := range testCases {
 		err := checkAlgorithm(&tc.key, &tc.jws)
 		if tc.expectedErr != "" && err.Error() != tc.expectedErr {
-			t.Errorf("TestCheckAlgorithm %d: Expected '%s', got '%s'", i, tc.expectedErr, err)
+			t.Errorf("TestCheckAlgorithm %d: Expected %q, got %q", i, tc.expectedErr, err)
 		}
 	}
 }
@@ -1162,7 +1169,7 @@ func TestValidJWSForKey(t *testing.T) {
 			JWK:  goodJWK,
 			ExpectedProblem: &probs.ProblemDetails{
 				Type:       probs.BadSignatureAlgorithmProblem,
-				Detail:     "signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
+				Detail:     "JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512",
 				HTTPStatus: http.StatusBadRequest,
 			},
 			ErrorStatType: "JWSAlgorithmCheckFailed",