Skip to content

Commit

Permalink
wfe/wfe2: make JWS signature alg error msgs match reality (letsencryp…
Browse files Browse the repository at this point in the history 
…t#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 letsencrypt#4452.
  • Loading branch information
@rolandshoemaker
rolandshoemaker authored and Daniel McCarney committed Oct 31, 2019
1 parent e448e81 commit e5eb8f8
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 79 deletions.
1 change: 1 addition & 0 deletions test/test-tools.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
35 changes: 22 additions & 13 deletions wfe/jose.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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
}
41 changes: 24 additions & 17 deletions wfe/jose_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package wfe

import (
"crypto/dsa"
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rsa"
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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)
}
Expand All @@ -66,45 +67,51 @@ func TestCheckAlgorithm(t *testing.T) {
expectedStat string
}{
{
jose.JSONWebKey{
Algorithm: "HS256",
jose.JSONWebKey{},
jose.JSONWebSignature{
Signatures: []jose.Signature{
{
Header: jose.Header{
Algorithm: "HS256",
},
},
},
},
jose.JSONWebSignature{},
"no signature algorithms suitable for given key type",
"WFE.Errors.NoAlgorithmForKey",
"JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512",
invalidJWSAlgorithm,
},
{
jose.JSONWebKey{
Key: &rsa.PublicKey{},
Key: &dsa.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",
"JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521",
noAlgorithmForKey,
},
{
jose.JSONWebKey{
Algorithm: "HS256",
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{
Expand All @@ -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 {
Expand Down
59 changes: 30 additions & 29 deletions wfe2/verify.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package wfe2

import (
"context"
"crypto"
"crypto/ecdsa"
"crypto/rsa"
"encoding/json"
Expand All @@ -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
Expand All @@ -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
}
Expand Down
47 changes: 27 additions & 20 deletions wfe2/verify_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package wfe2
import (
"context"
"crypto"
"crypto/dsa"
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rsa"
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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)
}
}

Expand All @@ -229,42 +230,48 @@ func TestCheckAlgorithm(t *testing.T) {
expectedErr string
}{
{
jose.JSONWebKey{
Algorithm: "HS256",
jose.JSONWebKey{},
jose.JSONWebSignature{
Signatures: []jose.Signature{
{
Header: jose.Header{
Algorithm: "HS256",
},
},
},
},
jose.JSONWebSignature{},
"no signature algorithms suitable for given key type",
"JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512",
},
{
jose.JSONWebKey{
Key: &rsa.PublicKey{},
Key: &dsa.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",
"JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521",
},
{
jose.JSONWebKey{
Algorithm: "HS256",
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{
Expand All @@ -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)
}
}
}
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit e5eb8f8

Please sign in to comment.