Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibility of making the jwt signature verification configurable? #151

Closed
TomCaserta opened this issue Oct 20, 2021 · 9 comments
Closed
Labels
question Further information is requested

Comments

@TomCaserta
Copy link
Contributor

TomCaserta commented Oct 20, 2021

By far the largest part of this library right now is `jsrsassign' but I don't think it really needs to be. The problem with jsrsassign is that it doesn't lend itself to treeshaking.

For example, angular oidc client is utilising:

https://www.npmjs.com/package/jsrsasign-reduced

Which contains only the methods needed to actually verify the token. It's still a large 140kb bundle but it's way better than the 200-300kb we have now.

Auth0 went in a slightly different direction with https://www.npmjs.com/package/idtoken-verifier which only supports RS256 tokens however it is 14kb minified + gzipped which is a massive improvement.

I presume you can't really dictate that in this library as it needs to be somewhat generic so my suggestion is to effectively allow the verify method to be implemented by the library consumer and perhaps include the current functionality in a separate entry point so it can be tree-shaken if a user decides not to use it.

@pamapa
Copy link
Member

pamapa commented Oct 20, 2021

Lets try to keep it simple and only support one crypto library. I am open to change jsrsassign with something smaller or something that supports treeshaking. The crypto library must be actively supported by multiple maintainers and used by a lot of other projects...

I guess we cannot use idtoken-verifier, as we can not control which algorithm the different authorities will use.

According to this site
https://www.npmtrends.com/crypto-js-vs-jsrsasign-vs-jsrsasign-reduced

crypto-js would be the winner, however its does not directly support JWT. On the other hand we do not need much, mainly parsing and validating if the signature is good.

@pamapa
Copy link
Member

pamapa commented Oct 20, 2021

This library looks also promising:
https://www.npmjs.com/package/jose

Its getting used quite a lot nowadays
https://www.npmtrends.com/jose-vs-jsrsasign-vs-jsrsasign-reduced

Or maybe native with window.crypto.subtle? Drawback its undefined in insecure environments -> we could skip validation likewise.

@brockallen
Copy link
Contributor

I'd argue that you should remove implicit flow support all together, as it's a deprecated flow in OAuth. This would then allow you to remove all JWT signature checks and it would allow for the library to be much smaller.

@TomCaserta
Copy link
Contributor Author

TomCaserta commented Oct 20, 2021

Those other packages suggested look decent and anything more lightweight than jsrsasign would be great, but actually as @brokallen says if it can be removed entirely that will definitely be the best option.

I was reading through the spec yesterday (fairly new to openid) and did notice that implicit flow was the only implementation enforcing verification of the signature however was not something I wanted to suggest straight away without knowing the intended use of this library :P

@brockallen
Copy link
Contributor

The first version of this library implemented implicit flow because at the time it was the only flow that allowed client-side browser-based JS apps to obtain tokens. Code flow was added later.

Now with CORS being widely available, the code flow can be safely used, and in that workflow the id_token can be returned from the token endpoint which is exempt from signature validation.

@pamapa
Copy link
Member

pamapa commented Oct 21, 2021

@brockallen good to have you on board :-) I fully agree with you. Lets drop the implicit flow and keep the library lightweight and best practise. Specifically the upcoming OAuth2.1 will drop the implicit flow too (https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-04).

@pamapa pamapa mentioned this issue Oct 21, 2021
4 tasks
@pamapa pamapa added the question Further information is requested label Oct 21, 2021
@pamapa
Copy link
Member

pamapa commented Oct 25, 2021

configuration wont be needed, as we will remove that code completely, see #152...

@pamapa pamapa closed this as completed Oct 25, 2021
@brockallen
Copy link
Contributor

I didn't look at the PR, but I'd imagine all the response_mode stuff could also get removed if implicit is no longer supported.

@pamapa
Copy link
Member

pamapa commented Oct 26, 2021

I didn't look at the PR, but I'd imagine all the response_mode stuff could also get removed if implicit is no longer supported.

i added a separate issue for this, see #162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants