chore: migrate from lestrrat-go/jwx v2 (EOL) to v3#4391
Conversation
GetHTTPStatusCode only did a direct type assertion for
HTTPStatusCodeError, so any fmt.Errorf("...: %w", err) wrap between an
error's origin and the Echo error handler silently discarded a
deliberately-set status code, falling back to 500. This affected
RequestServiceAccessToken when the remote presentation definition
endpoint returns an OAuth2 error (e.g. invalid_scope), among other
paths, per #2943.
jwx v2 is deprecated (EOL as of v2.1.7); migrate all direct usage to v3. - Rewrite imports v2 -> v3 across the codebase. - jwa algorithm constants -> function calls (jwa.ES256 -> jwa.ES256()). - Adapt Get()/accessor call sites to v3 signatures ((T, bool) returns). - jwk.FromRaw -> jwk.Import, (jwk.Key).Raw -> jwk.Export. - Replace the removed raw jws.NewVerifier with jwsbb.Verify for detached signature verification (crypto, vcr JSON-LD proof). - didkey resolver: use crypto/ecdh for X25519 (jwx/x25519 removed in v3). - Faithfully replace token.AsMap/PrivateClaims with JSON-based extraction so null-valued claims are still tolerated (v3 per-claim Get errors on null). - azure keyvault: validate key type before jwk.Export for a clear error. - Point go-did at its jwx-v3 branch (pending a tagged go-did release). Assisted by AI
| key, _ := token.DPoP.Headers.JWK() | ||
| hash, _ := key.Thumbprint(crypto.SHA256) |
There was a problem hiding this comment.
let's not ignore these errors
| return ValidateDPoPProof200JSONResponse{Reason: &reason}, nil | ||
| } | ||
| // check if the jti is already used, if not add it to the store for the duration of the access token lifetime | ||
| jti, _ := dpopToken.Token.JwtID() |
There was a problem hiding this comment.
what if an error is returned?
Addresses review feedback on the jwx v3 migration: - Add crypto/jwx.AsMap(joseObject) helper that converts a jwt.Token's claims to a map via JSON (preserving null-valued claims, unlike a per-field Get loop which errors on null). Replaces the duplicated marshal/unmarshal blocks in auth/client/iam, auth/api/iam/jar, auth/services/oauth, and the vcr test helper, and the equivalent loop in spi.FromJWK. The helper checks both the marshal and unmarshal errors, so those are no longer ignored. - Keep the per-field Get loop for JOSE *headers* (DecryptJWE, ExtractProtectedHeaders): a JSON round-trip flattens rich header values (e.g. the x5c certificate chain) and breaks callers that rely on their concrete Go types. - Remaining discarded returns are single-value accessors that returned no error/bool in v2 (e.g. token.Issuer()), so ignoring the new bool is faithful. Assisted by AI
reinkrul
requested review from
Dirklectisch,
JorisHeadease,
gerardsn,
stevenvegt and
woutslakhorst
as code owners
Replaces the interim jwx-v3-migration branch pseudo-version with the tagged go-did v0.22.0 release (jwx v3). Clears the merge blocker for the jwx v3 migration. Assisted by AI
20 new issues
|
; Conflicts: ; auth/oauth/types.go ; go.mod ; go.sum
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (37) 🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
The jwx v3 migration extracts JAR request-object claims via jwx.AsMap
(json.Marshal + Unmarshal into a map), which decodes JSON arrays as
[]interface{} rather than the []string that jwx v2's AsMap produced.
oauthParameters.get only matched []string, so a single-value "aud" claim
resolved to "" and audience validation failed (surfaced by the e2e OAuth
flow: "invalid audience, expected: ..., was: ").
Handle []interface{} single-element arrays too, fulfilling get's documented
"array values with len == 1 are treated as single string values" contract
regardless of whether the value came from query params ([]string) or JSON
claims ([]interface{}). Add a unit test covering the accessor.
Assisted by AI
| import "github.com/lestrrat-go/jwx/v3/jwa" | ||
|
|
||
| func init() { | ||
| AddSupportedAlgorithm(jwa.ES256K) |
There was a problem hiding this comment.
➜ nuts-node git:(chore/jwx-v3-migration) go build -tags jwx_es256k ./crypto/...
# github.com/nuts-foundation/nuts-node/crypto/jwx
crypto/jwx/jwx_es256k.go:26:24: cannot use jwa.ES256K (value of type func() jwa.SignatureAlgorithm) as jwa.SignatureAlgorithm value in argument to AddSupportedAlgorithm
| AddSupportedAlgorithm(jwa.ES256K) | |
| AddSupportedAlgorithm(jwa.ES256K()) |
| return nil, oauth.OAuth2Error{Code: oauth.InvalidRequestObject, Description: "request signature validation failed", InternalError: err} | ||
| } | ||
| claimsAsMap, err := token.AsMap(ctx) | ||
| claimsAsMap, err := jwx.AsMap(token) |
There was a problem hiding this comment.
The audience check in handleAuthorizeRequestFromHolder now always fails, breaking the JAR authorization flow. This is what's causing the e2e failure on this PR (invalid audience, expected: https://nodeA/oauth2/subjectA, was: ).
jar.validate now uses the new jwx.AsMap() helper, which does a JSON round-trip. jwx v3 always marshals aud as a JSON array (FlattenAudience is not enabled anywhere in the codebase), so after json.Unmarshal the claim comes back as []interface{}{"..."}. The old v2 token.AsMap(ctx) returned the audience as []string.oauthParameters.get() in params.go only handles string and []string, so params.get(jwt.AudienceKey) returns "" and the check at openid4vp.go:99 rejects every request.
The existing unit test doesn't catch this because it puts aud into the oauthParameters map directly as []string, bypassing the JWT parse path.
| for _, k := range protectedHeaders.Keys() { | ||
| var v interface{} | ||
| if err = protectedHeaders.Get(k, &v); err != nil { |
There was a problem hiding this comment.
The v2 version protectedHeaders.AsMap(ctx) properly returned null-members as a nil in the map.
This new per-field Get loop instead fails on them (failed to assign value for field "...": source value is invalid (<nil>)), so a JWS/JWE with a null-valued custom protected header is now rejected. The doc comment on crypto/jwx/claims.go AsMap() describes exactly this pitfall:
v3's per-field Get returns an error for a null-valued member, whereas the JSON round-trip preserves null members as nil - matching v2's AsMap behaviour and avoiding rejection of otherwise-valid input.
We probably want to handle null-valued members separately in this loop, keeping the concrete Go types for rich headers like x5c.
| // such as the x5c certificate chain keep their concrete Go types for callers. | ||
| for _, k := range protectedHeaders.Keys() { | ||
| var v interface{} | ||
| if err := protectedHeaders.Get(k, &v); err != nil { |
There was a problem hiding this comment.
| } | ||
| challenge := fmt.Sprintf("%s.%s", splittedJws[0], tbv) | ||
| if err = jswVerifier.Verify([]byte(challenge), sig, key); err != nil { | ||
| if err = jwsbb.Verify(key, alg.String(), []byte(challenge), sig); err != nil { |
There was a problem hiding this comment.
Are you sure we want to use this over jws.VerifierFor(alg) here? The building block mentions the following:
// Package jwsbb provides the building blocks (hence the name "bb") for JWS operations.
// It should be thought of as a low-level API, almost akin to internal packages
// that should not be used directly by users of the jwx package. However, these exist
// to provide a more efficient way to perform JWS operations without the overhead of
// the higher-level jws package to power-users who know what they are doing.
//
// This package is currently considered EXPERIMENTAL, and the API may change
// without notice. It is not recommended to use this package unless you are
// fully aware of the implications of using it.
//
// All bb packages in jwx follow the same design principles:
// 1. Does minimal checking of input parameters (for performance); callers need to ensure that the parameters are valid.
// 2. All exported functions are strongly typed (i.e. they do not take `any` types unless they absolutely have to).
// 3. Does not rely on other public jwx packages (they are standalone, except for internal packages).
//
// This implementation uses github.com/lestrrat-go/dsig as the underlying signature provider.
There was a problem hiding this comment.
I missed the experimental flag... I'll see if we can use the usual jws.Verifier
| payload = append(payload, part...) | ||
| } | ||
| err = verifier.Verify(payload, signature.Signature(), key) | ||
| err = jwsbb.Verify(key, alg.String(), payload, signature.Signature()) |
There was a problem hiding this comment.
|
There are also a lot of resolved comments about not ignoring the errors, but the code still ignores them |
| t.Run("test ES256K", func(t *testing.T) { | ||
| ecKey := test.GenerateECKey() | ||
| token := jwt.New() | ||
| signature, _ := jwt.Sign(token, jwt.WithKey(jwa.ES256K, ecKey)) |
There was a problem hiding this comment.
| signature, _ := jwt.Sign(token, jwt.WithKey(jwa.ES256K(), ecKey)) |
I checked them on case-by-case basis, and where they're ignored, they were ignored before. The semantic didn't change, as previously the single call basically did what is now done in 2 calls (meaning nil/error-behavior will be the same). |
2 participants
Migrates all direct usage of
github.com/lestrrat-go/jwxfrom the EOL v2 series to v3. Closes part of #4383 (themastertarget).Dependency
Requires go-did v0.22.0 (jwx v3) — pinned in
go.mod. This was the last thing keeping jwx v2 in the tree (see nuts-foundation/go-did#155); the earlier merge blocker (branch pseudo-version) is now resolved.Why
jwx v2 is deprecated (EOL as of v2.1.7) — no further bug/security fixes. jwx is a core JWT/JWS/JWK/crypto dependency, so staying on it is a security liability. We target v3, not v4: v4 hard-depends on
encoding/json/v2behindGOEXPERIMENT=jsonv2, which we won't bake into a production build until that graduates from the Go experiment (tracked as a follow-up in #4383).Key changes
go.mod/go.sum.jwaalgorithm constants → function calls (jwa.ES256→jwa.ES256());jwavalues are now comparable structs.Get()/accessor call sites adapted to v3 signatures (single-value accessors now return(T, bool);Get(name, &dst) error).jwk.FromRaw→jwk.Import;(jwk.Key).Raw(&x)→jwk.Export(key, &x).didkeyresolver: X25519 now built withcrypto/ecdh(jwx dropped itsx25519package in v3).Notable non-mechanical fixes (root-cause, behavior-preserving)
jws.NewVerifier; migrated the detached-signature verification incrypto/jwx.goandvcr/signature/proof/jsonld.gotojwsbb.Verify(...).AsMap/PrivateClaimsremoval: replaced the interim per-claimGetloops with JSON-based extraction inauth/client/iam,auth/api/iam/jar.go, andauth/services/oauth/authz_server.go. v3's per-claimGeterrors on a null-valued claim, which would have rejected otherwise-valid external tokens; JSON extraction preserves v2'sAsMap/PrivateClaimstolerance.parseKeynow checks the key type/curve beforejwk.Export, so an unsupported (e.g. RSA) key returns the intendedonly ES256 keys are supportedrather than a leaky jwx-internal export error.Behavior changes surfaced by v3 (kept intentionally)
TestInsecure1024BitRS512JWTstill rejects the weak key.alg:none: v3 rejects an empty compact signature on any other alg at parse time. Production already handlesalg:none; affected test fixtures updated. Any external issuer emitting unsigned credentials must setalg:none.jku/ remote JWK fetching is introduced anywhere (v3 flipped the fetch whitelist default to allow-all — audited, we don't use it).Testing
go build ./...clean;go test ./...green (107 packages).Follow-up
v6.2andv5.4(Migrate from lestrrat-go/jwx/v2 (EOL) to v3 #4383).encoding/json/v2leavesGOEXPERIMENT.Assisted by AI