Skip to content

Allow secretOrPrivateKey in verify to be function(kid) #406

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

Closed
Richie765 opened this issue Sep 27, 2017 · 8 comments
Closed

Allow secretOrPrivateKey in verify to be function(kid) #406

Richie765 opened this issue Sep 27, 2017 · 8 comments

Comments

@Richie765
Copy link

Richie765 commented Sep 27, 2017

I currently use decode to get the kid, then use verify with the correct key. This could be simplified if verify would accept secretOrPrivateKey to be a function(kid) so this could be handled in one step.

I could make a PR in the coming weeks, would this be accepted?

EDIT: I just realize that function(kid) would probably fetch the key asynchronously from the AP. In that case the function would return a promise. That can only work if verify is called asynchronously. I think it's possible to implement this backwards compatible. Perhaps in the future the whole library should be made with promises instead of callbacks? Any thoughts?

@ziluvatar
Copy link
Contributor

Good one, I've seen that in other libraries (like express-jwt).

I think it is better to pass a more generic function like: getSecretOrPrivateKey(jwtHeaders, jwtClaims, callback). In express-jwt they pass the req I think we can delay that until someone presents an use case where the req is needed.

That open the use case for jwks, where based on the issuer you could reach their public keys.

What do you think?

@Richie765
Copy link
Author

I think it's a bit risky. I'm not trusting the jwtClaims until the signature check has passed. The one to decide which is the issuer is the client, not the token. Otherwise anyone could put any issuer in the token and make the signature check pass. I would avoid that and just pass the jwtHeaders (or just kid).

I'm not very familiar with jwks, so what do you think?

@ziluvatar
Copy link
Contributor

I pasted the link in the PR and I gave you feedback there. (https://auth0.com/docs/jwks)

@riyan119
Copy link

i got this mistake :/ secretOrPrivateKey must have a value on jsonwebtoken :/

@JacoKoster
Copy link
Contributor

JacoKoster commented May 30, 2018

@ziluvatar @Richie765 I have asked this question in the PR as well, but what needs to be done to move this along? It has been open for quite some time, i have added the relevant tests and was hoping this could be part of a new major release. I'd be happy to do contribute, but i'd prefer not to be waiting for weeks on a response.

@ziluvatar
Copy link
Contributor

Good to hear that. I answered in the PR:

@JacoKoster we have two options:

  • If @Richie765 wants to continue with this we can work on this branch and your PR can be merged to it and we continue the review here.
  • You create your own PR based on this current work or on yours (in case you have different design in mind) + use your tests on it.

@JacoKoster
Copy link
Contributor

@ziluvatar I have opened a new PR for this, which has the functionality from the original PR, but without the breaking changes. I am eager to see if this is ok.

@ziluvatar
Copy link
Contributor

Fixed by: #480

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

No branches or pull requests

4 participants