Skip to content

Commit e5eb8f8

Browse files
rolandshoemakerDaniel McCarney
rolandshoemaker
authored and
Daniel McCarney
committed
wfe/wfe2: make JWS signature alg error msgs match reality (letsencrypt#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.
1 parent e448e81 commit e5eb8f8

File tree

5 files changed

+104
-79
lines changed

5 files changed

+104
-79
lines changed

‎test/test-tools.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ func AssertDeepEquals(t *testing.T, one interface{}, two interface{}) {
8383
// AssertMarshaledEquals marshals one and two to JSON, and then uses
8484
// the equality operator to measure them
8585
func AssertMarshaledEquals(t *testing.T, one interface{}, two interface{}) {
86+
t.Helper()
8687
oneJSON, err := json.Marshal(one)
8788
AssertNotError(t, err, "Could not marshal 1st argument")
8889
twoJSON, err := json.Marshal(two)

‎wfe/jose.go‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ func algorithmForKey(key *jose.JSONWebKey) (string, error) {
2222
return string(jose.ES512), nil
2323
}
2424
}
25-
return "", fmt.Errorf("no signature algorithms suitable for given key type")
25+
return "", fmt.Errorf("JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521")
2626
}
2727

2828
const (
@@ -31,26 +31,35 @@ const (
3131
invalidAlgorithmOnKey = "WFE.Errors.InvalidAlgorithmOnKey"
3232
)
3333

34+
var supportedAlgs = map[string]bool{
35+
string(jose.RS256): true,
36+
string(jose.ES256): true,
37+
string(jose.ES384): true,
38+
string(jose.ES512): true,
39+
}
40+
3441
// Check that (1) there is a suitable algorithm for the provided key based on its
3542
// Golang type, (2) the Algorithm field on the JWK is either absent, or matches
3643
// that algorithm, and (3) the Algorithm field on the JWK is present and matches
37-
// that algorithm. Precondition: parsedJws must have exactly one signature on
44+
// that algorithm. Precondition: parsedJWS must have exactly one signature on
3845
// it. Returns stat name to increment if err is non-nil.
39-
func checkAlgorithm(key *jose.JSONWebKey, parsedJws *jose.JSONWebSignature) (string, error) {
40-
algorithm, err := algorithmForKey(key)
46+
func checkAlgorithm(key *jose.JSONWebKey, parsedJWS *jose.JSONWebSignature) (string, error) {
47+
sigHeaderAlg := parsedJWS.Signatures[0].Header.Algorithm
48+
if !supportedAlgs[sigHeaderAlg] {
49+
return invalidJWSAlgorithm, fmt.Errorf(
50+
"JWS signature header contains unsupported algorithm %q, expected one of RS256, ES256, ES384 or ES512",
51+
parsedJWS.Signatures[0].Header.Algorithm,
52+
)
53+
}
54+
expectedAlg, err := algorithmForKey(key)
4155
if err != nil {
4256
return noAlgorithmForKey, err
4357
}
44-
jwsAlgorithm := parsedJws.Signatures[0].Header.Algorithm
45-
if jwsAlgorithm != algorithm {
46-
return invalidJWSAlgorithm, fmt.Errorf(
47-
"signature type '%s' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
48-
jwsAlgorithm,
49-
)
58+
if sigHeaderAlg != string(expectedAlg) {
59+
return invalidJWSAlgorithm, fmt.Errorf("JWS signature header algorithm %q does not match expected algorithm %q for JWK", sigHeaderAlg, string(expectedAlg))
5060
}
51-
if key.Algorithm != "" && key.Algorithm != algorithm {
52-
return invalidAlgorithmOnKey, fmt.Errorf("algorithm '%s' on JWK is unacceptable",
53-
key.Algorithm)
61+
if key.Algorithm != "" && key.Algorithm != string(expectedAlg) {
62+
return invalidAlgorithmOnKey, fmt.Errorf("JWK key header algorithm %q does not match expected algorithm %q for JWK", key.Algorithm, string(expectedAlg))
5463
}
5564
return "", nil
5665
}

‎wfe/jose_test.go‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package wfe
22

33
import (
4+
"crypto/dsa"
45
"crypto/ecdsa"
56
"crypto/elliptic"
67
"crypto/rsa"
@@ -28,7 +29,7 @@ func TestRejectsNone(t *testing.T) {
2829
if prob == nil {
2930
t.Fatalf("verifyPOST did not reject JWS with alg: 'none'")
3031
}
31-
if prob.Detail != "signature type 'none' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512" {
32+
if prob.Detail != "JWS signature header contains unsupported algorithm \"none\", expected one of RS256, ES256, ES384 or ES512" {
3233
t.Fatalf("verifyPOST rejected JWS with alg: 'none', but for wrong reason: %#v", prob)
3334
}
3435
}
@@ -52,7 +53,7 @@ func TestRejectsHS256(t *testing.T) {
5253
if prob == nil {
5354
t.Fatalf("verifyPOST did not reject JWS with alg: 'HS256'")
5455
}
55-
expected := "signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512"
56+
expected := "JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512"
5657
if prob.Detail != expected {
5758
t.Fatalf("verifyPOST rejected JWS with alg: 'none', but for wrong reason: got %q, wanted %q", prob, expected)
5859
}
@@ -66,45 +67,51 @@ func TestCheckAlgorithm(t *testing.T) {
6667
expectedStat string
6768
}{
6869
{
69-
jose.JSONWebKey{
70-
Algorithm: "HS256",
70+
jose.JSONWebKey{},
71+
jose.JSONWebSignature{
72+
Signatures: []jose.Signature{
73+
{
74+
Header: jose.Header{
75+
Algorithm: "HS256",
76+
},
77+
},
78+
},
7179
},
72-
jose.JSONWebSignature{},
73-
"no signature algorithms suitable for given key type",
74-
"WFE.Errors.NoAlgorithmForKey",
80+
"JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512",
81+
invalidJWSAlgorithm,
7582
},
7683
{
7784
jose.JSONWebKey{
78-
Key: &rsa.PublicKey{},
85+
Key: &dsa.PublicKey{},
7986
},
8087
jose.JSONWebSignature{
8188
Signatures: []jose.Signature{
8289
{
8390
Header: jose.Header{
84-
Algorithm: "HS256",
91+
Algorithm: "ES512",
8592
},
8693
},
8794
},
8895
},
89-
"signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
90-
"WFE.Errors.InvalidJWSAlgorithm",
96+
"JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521",
97+
noAlgorithmForKey,
9198
},
9299
{
93100
jose.JSONWebKey{
94-
Algorithm: "HS256",
101+
Algorithm: "RS256",
95102
Key: &rsa.PublicKey{},
96103
},
97104
jose.JSONWebSignature{
98105
Signatures: []jose.Signature{
99106
{
100107
Header: jose.Header{
101-
Algorithm: "HS256",
108+
Algorithm: "ES512",
102109
},
103110
},
104111
},
105112
},
106-
"signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
107-
"WFE.Errors.InvalidJWSAlgorithm",
113+
"JWS signature header algorithm \"ES512\" does not match expected algorithm \"RS256\" for JWK",
114+
invalidJWSAlgorithm,
108115
},
109116
{
110117
jose.JSONWebKey{
@@ -120,8 +127,8 @@ func TestCheckAlgorithm(t *testing.T) {
120127
},
121128
},
122129
},
123-
"algorithm 'HS256' on JWK is unacceptable",
124-
"WFE.Errors.InvalidAlgorithmOnKey",
130+
"JWK key header algorithm \"HS256\" does not match expected algorithm \"RS256\" for JWK",
131+
invalidAlgorithmOnKey,
125132
},
126133
}
127134
for i, tc := range testCases {

‎wfe2/verify.go‎

Lines changed: 30 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package wfe2
22

33
import (
44
"context"
5-
"crypto"
65
"crypto/ecdsa"
76
"crypto/rsa"
87
"encoding/json"
@@ -27,29 +26,28 @@ import (
2726
// POST requests with a JWS body must have the following Content-Type header
2827
const expectedJWSContentType = "application/jose+json"
2928

30-
var sigAlgErr = errors.New("no signature algorithms suitable for given key type")
31-
32-
func sigAlgorithmForECDSAKey(key *ecdsa.PublicKey) (jose.SignatureAlgorithm, error) {
33-
params := key.Params()
34-
switch params.Name {
35-
case "P-256":
36-
return jose.ES256, nil
37-
case "P-384":
38-
return jose.ES384, nil
39-
case "P-521":
40-
return jose.ES512, nil
41-
}
42-
return "", sigAlgErr
43-
}
44-
45-
func sigAlgorithmForKey(key crypto.PublicKey) (jose.SignatureAlgorithm, error) {
46-
switch k := key.(type) {
29+
func sigAlgorithmForKey(key *jose.JSONWebKey) (jose.SignatureAlgorithm, error) {
30+
switch k := key.Key.(type) {
4731
case *rsa.PublicKey:
4832
return jose.RS256, nil
4933
case *ecdsa.PublicKey:
50-
return sigAlgorithmForECDSAKey(k)
34+
switch k.Params().Name {
35+
case "P-256":
36+
return jose.ES256, nil
37+
case "P-384":
38+
return jose.ES384, nil
39+
case "P-521":
40+
return jose.ES512, nil
41+
}
5142
}
52-
return "", sigAlgErr
43+
return "", errors.New("JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521")
44+
}
45+
46+
var supportedAlgs = map[string]bool{
47+
string(jose.RS256): true,
48+
string(jose.ES256): true,
49+
string(jose.ES384): true,
50+
string(jose.ES512): true,
5351
}
5452

5553
// 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) {
5856
// that algorithm. Precondition: parsedJws must have exactly one signature on
5957
// it.
6058
func checkAlgorithm(key *jose.JSONWebKey, parsedJWS *jose.JSONWebSignature) error {
61-
algorithm, err := sigAlgorithmForKey(key.Key)
59+
sigHeaderAlg := parsedJWS.Signatures[0].Header.Algorithm
60+
if !supportedAlgs[sigHeaderAlg] {
61+
return fmt.Errorf(
62+
"JWS signature header contains unsupported algorithm %q, expected one of RS256, ES256, ES384 or ES512",
63+
parsedJWS.Signatures[0].Header.Algorithm,
64+
)
65+
}
66+
expectedAlg, err := sigAlgorithmForKey(key)
6267
if err != nil {
6368
return err
6469
}
65-
jwsAlgorithm := parsedJWS.Signatures[0].Header.Algorithm
66-
if jwsAlgorithm != string(algorithm) {
67-
return fmt.Errorf(
68-
"signature type '%s' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
69-
jwsAlgorithm,
70-
)
70+
if sigHeaderAlg != string(expectedAlg) {
71+
return fmt.Errorf("JWS signature header algorithm %q does not match expected algorithm %q for JWK", sigHeaderAlg, string(expectedAlg))
7172
}
72-
if key.Algorithm != "" && key.Algorithm != string(algorithm) {
73-
return fmt.Errorf("algorithm '%s' on JWK is unacceptable", key.Algorithm)
73+
if key.Algorithm != "" && key.Algorithm != string(expectedAlg) {
74+
return fmt.Errorf("JWK key header algorithm %q does not match expected algorithm %q for JWK", key.Algorithm, string(expectedAlg))
7475
}
7576
return nil
7677
}

‎wfe2/verify_test.go‎

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package wfe2
33
import (
44
"context"
55
"crypto"
6+
"crypto/dsa"
67
"crypto/ecdsa"
78
"crypto/elliptic"
89
"crypto/rsa"
@@ -26,15 +27,15 @@ func sigAlgForKey(t *testing.T, key interface{}) jose.SignatureAlgorithm {
2627
var err error
2728
// Gracefully handle the case where a non-pointer public key is given where
2829
// sigAlgorithmForKey always wants a pointer. It may be tempting to try and do
29-
// `sigAlgorithmForKey(&key)` without a type switch but this produces
30+
// `sigAlgorithmForKey(&jose.JSONWebKey{Key: &key})` without a type switch but this produces
3031
// `*interface {}` and not the desired `*rsa.PublicKey` or `*ecdsa.PublicKey`.
3132
switch k := key.(type) {
3233
case rsa.PublicKey:
33-
sigAlg, err = sigAlgorithmForKey(&k)
34+
sigAlg, err = sigAlgorithmForKey(&jose.JSONWebKey{Key: &k})
3435
case ecdsa.PublicKey:
35-
sigAlg, err = sigAlgorithmForKey(&k)
36+
sigAlg, err = sigAlgorithmForKey(&jose.JSONWebKey{Key: &k})
3637
default:
37-
sigAlg, err = sigAlgorithmForKey(k)
38+
sigAlg, err = sigAlgorithmForKey(&jose.JSONWebKey{Key: k})
3839
}
3940
test.Assert(t, err == nil, fmt.Sprintf("Error getting signature algorithm for key %#v", key))
4041
return sigAlg
@@ -185,7 +186,7 @@ func TestRejectsNone(t *testing.T) {
185186
if err == nil {
186187
t.Fatalf("checkAlgorithm did not reject JWS with alg: 'none'")
187188
}
188-
if err.Error() != "signature type 'none' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512" {
189+
if err.Error() != "JWS signature header contains unsupported algorithm \"none\", expected one of RS256, ES256, ES384 or ES512" {
189190
t.Fatalf("checkAlgorithm rejected JWS with alg: 'none', but for wrong reason: %#v", err)
190191
}
191192
}
@@ -216,9 +217,9 @@ func TestRejectsHS256(t *testing.T) {
216217
if err == nil {
217218
t.Fatalf("checkAlgorithm did not reject JWS with alg: 'HS256'")
218219
}
219-
expected := "signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512"
220+
expected := "JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512"
220221
if err.Error() != expected {
221-
t.Fatalf("checkAlgorithm rejected JWS with alg: 'none', but for wrong reason: got '%s', wanted %s", err.Error(), expected)
222+
t.Fatalf("checkAlgorithm rejected JWS with alg: 'none', but for wrong reason: got %q, wanted %q", err.Error(), expected)
222223
}
223224
}
224225

@@ -229,42 +230,48 @@ func TestCheckAlgorithm(t *testing.T) {
229230
expectedErr string
230231
}{
231232
{
232-
jose.JSONWebKey{
233-
Algorithm: "HS256",
233+
jose.JSONWebKey{},
234+
jose.JSONWebSignature{
235+
Signatures: []jose.Signature{
236+
{
237+
Header: jose.Header{
238+
Algorithm: "HS256",
239+
},
240+
},
241+
},
234242
},
235-
jose.JSONWebSignature{},
236-
"no signature algorithms suitable for given key type",
243+
"JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512",
237244
},
238245
{
239246
jose.JSONWebKey{
240-
Key: &rsa.PublicKey{},
247+
Key: &dsa.PublicKey{},
241248
},
242249
jose.JSONWebSignature{
243250
Signatures: []jose.Signature{
244251
{
245252
Header: jose.Header{
246-
Algorithm: "HS256",
253+
Algorithm: "ES512",
247254
},
248255
},
249256
},
250257
},
251-
"signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
258+
"JWK contains unsupported key type (expected RSA, or ECDSA P-256, P-384, or P-521",
252259
},
253260
{
254261
jose.JSONWebKey{
255-
Algorithm: "HS256",
262+
Algorithm: "RS256",
256263
Key: &rsa.PublicKey{},
257264
},
258265
jose.JSONWebSignature{
259266
Signatures: []jose.Signature{
260267
{
261268
Header: jose.Header{
262-
Algorithm: "HS256",
269+
Algorithm: "ES512",
263270
},
264271
},
265272
},
266273
},
267-
"signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
274+
"JWS signature header algorithm \"ES512\" does not match expected algorithm \"RS256\" for JWK",
268275
},
269276
{
270277
jose.JSONWebKey{
@@ -280,13 +287,13 @@ func TestCheckAlgorithm(t *testing.T) {
280287
},
281288
},
282289
},
283-
"algorithm 'HS256' on JWK is unacceptable",
290+
"JWK key header algorithm \"HS256\" does not match expected algorithm \"RS256\" for JWK",
284291
},
285292
}
286293
for i, tc := range testCases {
287294
err := checkAlgorithm(&tc.key, &tc.jws)
288295
if tc.expectedErr != "" && err.Error() != tc.expectedErr {
289-
t.Errorf("TestCheckAlgorithm %d: Expected '%s', got '%s'", i, tc.expectedErr, err)
296+
t.Errorf("TestCheckAlgorithm %d: Expected %q, got %q", i, tc.expectedErr, err)
290297
}
291298
}
292299
}
@@ -1162,7 +1169,7 @@ func TestValidJWSForKey(t *testing.T) {
11621169
JWK: goodJWK,
11631170
ExpectedProblem: &probs.ProblemDetails{
11641171
Type: probs.BadSignatureAlgorithmProblem,
1165-
Detail: "signature type 'HS256' in JWS header is not supported, expected one of RS256, ES256, ES384 or ES512",
1172+
Detail: "JWS signature header contains unsupported algorithm \"HS256\", expected one of RS256, ES256, ES384 or ES512",
11661173
HTTPStatus: http.StatusBadRequest,
11671174
},
11681175
ErrorStatType: "JWSAlgorithmCheckFailed",

0 commit comments

Comments
 (0)