diff --git a/test/test-tools.go b/test/test-tools.go index eb95be229..f3d46a0df 100644 --- a/test/test-tools.go +++ b/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) diff --git a/wfe/jose.go b/wfe/jose.go index ec33f7a68..3f3aacdd9 100644 --- a/wfe/jose.go +++ b/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 } diff --git a/wfe/jose_test.go b/wfe/jose_test.go index 8a2fc9662..411e2df06 100644 --- a/wfe/jose_test.go +++ b/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 { diff --git a/wfe2/verify.go b/wfe2/verify.go index b6d1a38f3..9a4efdffc 100644 --- a/wfe2/verify.go +++ b/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 } diff --git a/wfe2/verify_test.go b/wfe2/verify_test.go index 0b620f687..822e1dc5a 100644 --- a/wfe2/verify_test.go +++ b/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",