JWT signature error validations aren't passed through

[trombonekenny]

Describe the bug
The openid-client bug is that any errors in the client.js jose.compactVerify function are not made visible or passed up the chain. The client throws a "failed to validate JWT signature" signature but doesn't include anymore data on why. It's not clear how I would get that troubleshooting if not from inside openid-client.

I think this area needs to be refactored:
https://github.com/panva/node-openid-client/blob/10e3a37efe2635c4b21fba30f5646ef7cf2f4b95/lib/client.js#L1045

The error handling for that stanza should include data from the jose error, IMO. Maybe move the throw new RPError on L1055 up into that catch block?

To Reproduce
I was having a hard time troubleshooting why my authentication sessions were working with 4.9.1 and failing with anything newer. I realized that 5.0.0 updated the jose version, which had different tolerances for RSA key length. The dev suite I was working with had <2048 bit keys, and it was eventually resolved by updating the keys in the suite.

Expected behaviour
I worked around this by sneaking some logging into the library (at the catch line link above).

I'm not sure how else to get the troubleshooting data from jose (maybe it has a debug setting? Or somewhere outside of openid-client could somehow capture that data?). It would make more sense to report that back in the error, like other RPError statements in client.js do.

Environment:

• openid-client version: 5.3.2
• node version: 16.19.0

Additional context

• the bug is happening on latest openid-client too.
• I have searched the issues tracker on github for similar issues and couldn't find anything related.

[koalazak]

I recently had same problem with new key size validations on jose . Besides the wanted debug information, is there any way to allow 1024 size from node-openid-client usage? looks like the limit is hardcoded (I guess because NIST recomendations)

thanks

[panva]

I guess because NIST recommendations

This comes straight from the JOSE Algorithms definition, see https://www.rfc-editor.org/rfc/rfc7518

A key of size 2048 bits or larger MUST be used with these algorithms.

[koalazak]

I guess because NIST recommendations

This comes straight from the JOSE Algorithms definition, see https://www.rfc-editor.org/rfc/rfc7518

A key of size 2048 bits or larger MUST be used with these algorithms.

yes the RFC is quoting NIST SP 800-57 [NIST.800-57]

make sense to me, but is there any way to avoid the verification?

thanks

[panva]

no there isn't/

[TiddoLangerak]

@panva would you be open for PRs that allow this to be bypassed with a flag? Unfortunately when integrating with 3rd party APIs we don't have the option to change their keys, and currently running into this issue with a large company (intuit, quickbooks) that's unlikely to change their keys for us. This effectively means we can't upgrade openid-client beyond 4.9.1, because from 5.x onwards the validation will fail. Supporting a allowInsecureKeys flag of some sorts would be greatly helpful!

[panva]

Same as in panva/jose#516 (reply in thread)

[panva]

The reason it does not expose "the error" is that it is potentially looping over a set of applicable keys, not just one. So "the error" can be multiple errors and if so it's not so cut and dry as to which one to expose.

[trombonekenny]

The reason it does not expose "the error" is that it is potentially looping over a set of applicable keys, not just one. So "the error" can be multiple errors and if so it's not so cut and dry as to which one to expose.

I figured it was something like that, or places where it could throw an error but not a critical one?

Maybe a strategy of reporting the first error? Or is there a JS precedent for combining multiple errors into one aggregate one?

[TiddoLangerak]

JS has the AggregateError built-in that can be used to represent multiple errors, and this is supported since node 15.

So for this issue, the errors could be collected into an AggregateError, which could then be passed as the cause to the RPError that's eventually thrown.