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

Add a way to verify token signature #101

Closed
Daniel-Houston opened this issue Apr 3, 2018 · 4 comments
Closed

Add a way to verify token signature #101

Daniel-Houston opened this issue Apr 3, 2018 · 4 comments
Assignees

Comments

@Daniel-Houston
Copy link

Daniel-Houston commented Apr 3, 2018

In lib/token.js on line 231 you are calling verifyToken as follows.

return verifyToken(sdk, idToken, oauthParams.nonce, true)

That last value of true sets the variable 'ignoreSignature' to true, thereby skipping the validation of the jwt signature in the verifyToken function, which parseFromUrl uses indirectly (parseFromUrl -> handleOAuthResponse -> verifyToken) My first question is what is the reasoning for always ignoring the signature of the idToken? And my second question is would you be willing to make that configurable or open to a PR that would make it configurable?

Thanks!

@Daniel-Houston
Copy link
Author

Anyone have any comment on this?

@Daniel-Houston
Copy link
Author

@jmelberg-okta Sorry to pester you, but to me this issue seems more pressing than my other issue, since the id token signature is never validated. Do you have any thoughts on this one?

@jmelberg-okta
Copy link
Contributor

I'm not sure of the historical reason as to why we're ignoring signature validation when receiving the idToken from the /token endpoint. IMO we should be defaulting this to false to ensure it gets validated.

Our team is auditing this library + making changes to ensure we have all the necessary ID Token Validation steps.

Secondly, calling token.verifyIdToken() does validate the signature by retrieving the JWKS. You can see how it is implemented here. If you're using this for Implicit flows, please use that method instead of relying on the default behavior.

I'd be happy to take a look at a PR with these changes if you have time. Please make sure to review/sign our CLA beforehand.

Thanks!

@jmelberg-okta
Copy link
Contributor

Resolved with 2.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants