Skip to content

Commit 03c40f4

Browse files
yaronfcursoragent
andcommitted
fix: harden JWS constructors and bump golangci-lint for Go 1.27
Reject key/alg mismatches at NewJWS* construction, document SetAllowedAlgs vs foreign JWS, and move lint to v2.13.1 so CI can target go.mod 1.27.0. Track upstream jwx ECDSA/ML-DSA AlgorithmsForKey gaps for later. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 7950328 commit 03c40f4

5 files changed

Lines changed: 153 additions & 32 deletions

File tree

.github/workflows/lint.yml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,5 @@ jobs:
1616
- name: golangci-lint
1717
uses: golangci/golangci-lint-action@v9
1818
with:
19-
version: v2.12.2
19+
# v2.13+ is built with Go 1.27 (v2.12.x was go1.26 and rejects go.mod 1.27.0)
20+
version: v2.13.1

config.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -184,9 +184,11 @@ func (v *VerifyConfig) SetRejectExpired(rejectExpired bool) *VerifyConfig {
184184
return v
185185
}
186186

187-
// SetAllowedAlgs defines the allowed values of the "alg" parameter.
188-
// This is useful if the actual algorithm used in verification is taken from the message - not a recommended practice.
189-
// Default: an empty list, signifying all values are accepted.
187+
// SetAllowedAlgs defines the allowed values of the HTTP Message Signatures "alg" parameter
188+
// (RFC 9421), not the JWS algorithm passed to NewJWSSigner/NewJWSVerifier.
189+
// This is useful if the algorithm used in verification is taken from the message — not a recommended practice.
190+
// With foreign JWS signers the library refuses to emit "alg", so this policy only applies when a peer
191+
// still includes that parameter. Default: an empty list, signifying all values are accepted.
190192
func (v *VerifyConfig) SetAllowedAlgs(allowedAlgs []string) *VerifyConfig {
191193
v.allowedAlgs = allowedAlgs
192194
return v

crypto.go

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"crypto/sha512"
1313
"crypto/subtle"
1414
"fmt"
15+
"slices"
1516

1617
"github.com/lestrrat-go/jwx/v4/jwa"
1718
"github.com/lestrrat-go/jwx/v4/jws"
@@ -128,14 +129,22 @@ func NewEd25519SignerFromSeed(seed []byte, config *SignConfig, fields Fields) (*
128129

129130
// NewJWSSigner creates a generic signer for JWS algorithms via github.com/lestrrat-go/jwx/v4.
130131
// The particular key type for each algorithm is documented in that package (including
131-
// crypto/mldsa keys for ML-DSA on Go 1.27+). Config may be nil for a default configuration.
132+
// crypto/mldsa keys for ML-DSA on Go 1.27+). HMAC keys must be []byte (not string).
133+
// Config may be nil for a default configuration.
134+
//
135+
// Note: foreign JWS signers do not emit the HTTP Message Signatures "alg" parameter
136+
// (see SignConfig.SignAlg). VerifyConfig.SetAllowedAlgs therefore only applies if a peer
137+
// still includes that parameter; it does not constrain the JWS algorithm passed here.
132138
func NewJWSSigner(alg jwa.SignatureAlgorithm, key interface{}, config *SignConfig, fields Fields) (*Signer, error) {
133139
if key == nil {
134140
return nil, fmt.Errorf("key must not be nil")
135141
}
136142
if alg == jwa.NoSignature() {
137143
return nil, fmt.Errorf("the NONE signing algorithm is expressly disallowed")
138144
}
145+
if err := validateJWSKeyAlg(alg, key); err != nil {
146+
return nil, err
147+
}
139148
if config == nil {
140149
config = NewSignConfig()
141150
}
@@ -152,6 +161,20 @@ func NewJWSSigner(alg jwa.SignatureAlgorithm, key interface{}, config *SignConfi
152161
}, nil
153162
}
154163

164+
// validateJWSKeyAlg rejects keys that jwx does not associate with alg (wrong key family).
165+
// It uses jws.AlgorithmsForKey; some families list multiple algs (e.g. all HS*, all ML-DSA*),
166+
// so finer mismatches may still fail later at Sign/Verify time.
167+
func validateJWSKeyAlg(alg jwa.SignatureAlgorithm, key interface{}) error {
168+
algs, err := jws.AlgorithmsForKey(key)
169+
if err != nil {
170+
return fmt.Errorf("key is not usable for JWS signing/verification: %w", err)
171+
}
172+
if !slices.Contains(algs, alg) {
173+
return fmt.Errorf("algorithm %s is not valid for key type %T", alg, key)
174+
}
175+
return nil
176+
}
177+
155178
func (s Signer) sign(buff []byte) ([]byte, error) {
156179
if s.foreignSigner != nil {
157180
signer, ok := s.foreignSigner.(jws.Signer)
@@ -325,18 +348,25 @@ func NewEd25519Verifier(key ed25519.PublicKey, config *VerifyConfig, fields Fiel
325348

326349
// NewJWSVerifier creates a generic verifier for JWS algorithms via github.com/lestrrat-go/jwx/v4.
327350
// The particular key type for each algorithm is documented in that package (including
328-
// crypto/mldsa keys for ML-DSA on Go 1.27+). Set config to nil for a default configuration.
351+
// crypto/mldsa keys for ML-DSA on Go 1.27+). HMAC keys must be []byte (not string).
352+
// Set config to nil for a default configuration.
329353
// Fields is the list of required headers and fields, which may be empty (but this is typically insecure).
354+
//
355+
// Note: SetAllowedAlgs constrains the HTTP Message Signatures "alg" parameter on the wire,
356+
// not the JWS algorithm passed here. Foreign JWS signers omit that parameter by design.
330357
func NewJWSVerifier(alg jwa.SignatureAlgorithm, key interface{}, config *VerifyConfig, fields Fields) (*Verifier, error) {
331358
if key == nil {
332359
return nil, fmt.Errorf("key must not be nil")
333360
}
334-
if config == nil {
335-
config = NewVerifyConfig()
336-
}
337361
if alg == jwa.NoSignature() {
338362
return nil, fmt.Errorf("the NONE signing algorithm is expressly disallowed")
339363
}
364+
if err := validateJWSKeyAlg(alg, key); err != nil {
365+
return nil, err
366+
}
367+
if config == nil {
368+
config = NewVerifyConfig()
369+
}
340370
verifier, err := jws.VerifierFor(alg)
341371
if err != nil {
342372
return nil, err

crypto_test.go

Lines changed: 93 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,45 @@ func TestNewRSASigner1(t *testing.T) {
248248
}
249249
}
250250

251+
func TestNewJWSSigner(t *testing.T) {
252+
hmacKey := []byte("1234")
253+
priv, err := rsa.GenerateKey(rand.Reader, 1024)
254+
require.NoError(t, err)
255+
256+
tests := []struct {
257+
name string
258+
alg jwa.SignatureAlgorithm
259+
key any
260+
wantErr bool
261+
}{
262+
{name: "happy path", alg: jwa.HS256(), key: hmacKey},
263+
{name: "none", alg: jwa.NoSignature(), key: hmacKey, wantErr: true},
264+
{name: "nil key", alg: jwa.HS256(), key: nil, wantErr: true},
265+
{name: "string hmac key", alg: jwa.HS256(), key: "1234", wantErr: true},
266+
{name: "key alg mismatch", alg: jwa.HS256(), key: priv, wantErr: true},
267+
}
268+
for _, tt := range tests {
269+
t.Run(tt.name, func(t *testing.T) {
270+
got, err := NewJWSSigner(tt.alg, tt.key, nil, *NewFields())
271+
if tt.wantErr {
272+
require.Error(t, err)
273+
require.Nil(t, got)
274+
return
275+
}
276+
require.NoError(t, err)
277+
require.NotNil(t, got)
278+
require.NotNil(t, got.foreignSigner)
279+
assert.Equal(t, hmacKey, got.key)
280+
assert.Empty(t, got.alg)
281+
})
282+
}
283+
}
284+
251285
func TestNewJWSVerifier(t *testing.T) {
286+
hmacKey := []byte("1234")
287+
priv, err := rsa.GenerateKey(rand.Reader, 1024)
288+
require.NoError(t, err)
289+
252290
type args struct {
253291
alg jwa.SignatureAlgorithm
254292
key any
@@ -265,12 +303,12 @@ func TestNewJWSVerifier(t *testing.T) {
265303
name: "happy path",
266304
args: args{
267305
alg: jwa.HS256(),
268-
key: "1234",
306+
key: hmacKey,
269307
config: nil,
270308
fields: *NewFields(),
271309
},
272310
want: &Verifier{
273-
key: "1234",
311+
key: hmacKey,
274312
alg: "",
275313
config: NewVerifyConfig(),
276314
fields: *NewFields(),
@@ -282,7 +320,7 @@ func TestNewJWSVerifier(t *testing.T) {
282320
name: "none",
283321
args: args{
284322
alg: jwa.NoSignature(),
285-
key: "1234",
323+
key: hmacKey,
286324
config: NewVerifyConfig(),
287325
fields: *NewFields(),
288326
},
@@ -300,6 +338,28 @@ func TestNewJWSVerifier(t *testing.T) {
300338
want: nil,
301339
wantErr: true,
302340
},
341+
{
342+
name: "string hmac key",
343+
args: args{
344+
alg: jwa.HS256(),
345+
key: "1234",
346+
config: NewVerifyConfig(),
347+
fields: *NewFields(),
348+
},
349+
want: nil,
350+
wantErr: true,
351+
},
352+
{
353+
name: "key alg mismatch",
354+
args: args{
355+
alg: jwa.HS256(),
356+
key: priv.Public(),
357+
config: NewVerifyConfig(),
358+
fields: *NewFields(),
359+
},
360+
want: nil,
361+
wantErr: true,
362+
},
303363
}
304364
for _, tt := range tests {
305365
t.Run(tt.name, func(t *testing.T) {
@@ -338,25 +398,38 @@ func TestVerify(t *testing.T) {
338398
assert.ErrorContains(t, err, "expected", "bad algorithm")
339399
}
340400

341-
func TestForeignSignerMLDSA65(t *testing.T) {
342-
priv, err := mldsa.GenerateKey(mldsa.MLDSA65())
343-
require.NoError(t, err)
344-
pub := priv.Public().(*mldsa.PublicKey)
401+
func TestForeignSignerMLDSA(t *testing.T) {
402+
cases := []struct {
403+
name string
404+
params mldsa.Parameters
405+
alg jwa.SignatureAlgorithm
406+
}{
407+
{"MLDSA44", mldsa.MLDSA44(), jwa.MLDSA44()},
408+
{"MLDSA65", mldsa.MLDSA65(), jwa.MLDSA65()},
409+
{"MLDSA87", mldsa.MLDSA87(), jwa.MLDSA87()},
410+
}
411+
for _, tc := range cases {
412+
t.Run(tc.name, func(t *testing.T) {
413+
priv, err := mldsa.GenerateKey(tc.params)
414+
require.NoError(t, err)
415+
pub := priv.Public().(*mldsa.PublicKey)
345416

346-
config := NewSignConfig().setFakeCreated(1618884475).SignAlg(false)
347-
signatureName := "sig1"
348-
fields := *NewFields().AddHeader("@method").AddHeader("date").AddHeader("content-type").AddQueryParam("pet")
349-
signer, err := NewJWSSigner(jwa.MLDSA65(), priv, config.SetKeyID("pq1"), fields)
350-
require.NoError(t, err)
417+
config := NewSignConfig().setFakeCreated(1618884475).SignAlg(false)
418+
signatureName := "sig1"
419+
fields := *NewFields().AddHeader("@method").AddHeader("date").AddHeader("content-type").AddQueryParam("pet")
420+
signer, err := NewJWSSigner(tc.alg, priv, config.SetKeyID("pq1"), fields)
421+
require.NoError(t, err)
351422

352-
req := readRequest(httpreq2)
353-
sigInput, sig, err := SignRequest(signatureName, *signer, req)
354-
require.NoError(t, err)
355-
req.Header.Add("Signature", sig)
356-
req.Header.Add("Signature-Input", sigInput)
423+
req := readRequest(httpreq2)
424+
sigInput, sig, err := SignRequest(signatureName, *signer, req)
425+
require.NoError(t, err)
426+
req.Header.Add("Signature", sig)
427+
req.Header.Add("Signature-Input", sigInput)
357428

358-
verifier, err := NewJWSVerifier(jwa.MLDSA65(), pub, NewVerifyConfig().SetVerifyCreated(false).SetKeyID("pq1"), fields)
359-
require.NoError(t, err)
360-
require.NoError(t, VerifyRequest(signatureName, *verifier, req))
429+
verifier, err := NewJWSVerifier(tc.alg, pub, NewVerifyConfig().SetVerifyCreated(false).SetKeyID("pq1"), fields)
430+
require.NoError(t, err)
431+
require.NoError(t, VerifyRequest(signatureName, *verifier, req))
432+
})
433+
}
361434
}
362435

internal-docs/JWX.md

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ Ship as **`v0.6.0`** when merged. Dual v2+v3 is removed on this branch.
4848
| `encoding/json/v2` in stdlib (no `GOEXPERIMENT=jsonv2`) | **Met** — see [Go 1.27 notes](https://go.dev/doc/go1.27) |
4949
| jwx v4 mature | **Met** — pinned **`v4.4.0`** |
5050
| Smoke / full tests under `GOTOOLCHAIN=go1.27.0` | **Met** on cutover branch |
51-
| Stdlib **`crypto/mldsa`** + jwx native ML-DSA (PQ goal) | **Met**`TestForeignSignerMLDSA65` |
51+
| Stdlib **`crypto/mldsa`** + jwx native ML-DSA (PQ goal) | **Met**`TestForeignSignerMLDSA` (44/65/87) |
5252

5353
### Why this was deferred (history)
5454

@@ -93,7 +93,8 @@ No change to key types or httpsign `SignConfig` / `VerifyConfig` / `Fields` for
9393

9494
| Item | Requirement |
9595
|------|-------------|
96-
| Go | **1.27.0+** in `go.mod` / CI (today CI is still 1.24) |
96+
| Go | **1.27.0+** in `go.mod` / CI |
97+
| golangci-lint | **≥ v2.13** (built with Go 1.27; v2.12.x fails with go.mod `1.27.0`) |
9798
| `GOEXPERIMENT=jsonv2` | Not required; do not set |
9899
| `GOEXPERIMENT=nojsonv2` | Avoid in CI |
99100

@@ -111,12 +112,26 @@ Scoped to httpsign’s use of **jwa** + **jws** only (no JWT/JWE/JWK fetch in li
111112
- [x] Rewrite imports `jwx/v2|v3``jwx/v4`; collapse constructors; update `sign()` / `verify()` dispatch for v4 `jws.Signer` / `jws.Verifier` (renamed from v3 `Signer2` / `Verifier2`; parameter order matches today’s V3 path: key before payload).
112113
- [x] Confirm factory APIs (`SignerFor` / `VerifierFor`) and `NoSignature` rejection still work.
113114
- [x] Drop v2↔v3 cross-compat tests; keep round-trip tests on the single v4 path.
114-
- [x] **PQ:** foreign-JWS round-trip with `crypto/mldsa` + `jwa.MLDSA65()` (and smoke 44/87 if cheap); document in README/release notes.
115+
- [x] **PQ:** foreign-JWS round-trips with `crypto/mldsa` + `jwa.MLDSA44/65/87()`; document in README/release notes.
115116
- [ ] Run `jwxmigrate --fix` if helpful; fix remaining compile/test failures by hand. *(done by hand; migrate tool optional)*
116117
- [x] CI (`test.yml`, `lint.yml`, CodeQL): Go **1.27**; do not set `jsonv2` / `nojsonv2`.
118+
- [x] Lint: bump **golangci-lint ≥ v2.13** (v2.12.2 is built with go1.26 → fails on go.mod 1.27.0).
117119
- [x] Docs: README / `CLAUDE.md` / this file — remove dual-version guidance; **`v0.6.0`** release notes with caller steps + PQ; link upstream Changes-v4 if relevant.
120+
- [x] Hardening: constructor `jws.AlgorithmsForKey` check; reject `NoSignature`; HMAC keys must be `[]byte`; document `SetAllowedAlgs` vs JWS alg.
118121
- [ ] Tag **`v0.6.0`** and publish. *(after merge)*
119122

123+
### Later action: upstream jwx / dsig (found 2026-08-26 while hardening)
124+
125+
Not blocking `v0.6.0`. File / track upstream issues; optionally tighten httpsign further if upstream stays loose.
126+
127+
| Finding | Severity | Notes |
128+
|---------|----------|-------|
129+
| **`jws.AlgorithmsForKey` ignores ECDSA curve** for raw `*ecdsa.{Private,Public}Key` | Correctness | Returns `[ES256, ES384, ES512]` for any EC key. Docs claim curve is inferred from the Go type, but `hasCrv` is never set for stdlib ECDSA; `RegisterAlgorithmForCurve` is only used for Ed25519 in init (no P-256→ES256 etc.). |
130+
| **ECDSA Sign/Verify do not enforce RFC 7518 curve↔alg** | Spec / footgun | `ES384` effectively means SHA-384 only; a **P-256** key can mint `"alg":"ES384"` and verify with the same P-256 pub (64-byte sig). Will not verify under a real P-384 key. Policy that allows only ES384 expecting P-384 strength can accept weaker P-256+SHA-384 if a P-256 key is registered. dsig examples treat cross-curve as intentional for custom algs. |
131+
| **`AlgorithmsForKey` lists all ML-DSA algs** for any ML-DSA key | Classifier only | Sign/Verify correctly reject parameter-set mismatch — crypto path is fine; helper is over-broad (same class of bug as ECDSA listing). |
132+
133+
**Suggested upstream asks (lestrrat-go/jwx + dsig):** (1) extract curve from raw ECDSA keys in `AlgorithmsForKey` and register P-256/P-384/P-521 → ES256/ES384/ES512; (2) optionally enforce curve↔alg in ECDSA Sign/Verify; (3) refine ML-DSA listing by parameter set. **httpsign follow-up:** if upstream does not tighten ECDSA, consider our own curve check in `validateJWSKeyAlg` (and ML-DSA `Parameters()` vs `jwa.MLDSA*`).
134+
120135
### Upstream items likely N/A or low priority
121136

122137
| Topic | httpsign |

0 commit comments

Comments
 (0)