Secret callback #413

Richie765 · 2017-10-05T13:46:01Z

Added features

verify's secretOrToken parameter can now be a callback function, see updated README.md for details
hardended checks for unsigned jwt, must now explicitly add ['none'] in algorithms to pass
updated tests and readme

Performance improvements

removed need to clone options object
merged two decodes into one

See issue #406

Richie765 · 2017-10-05T14:01:16Z

I've also created typescript typings here:
DefinitelyTyped/DefinitelyTyped#20321

ziluvatar · 2017-10-09T12:13:08Z

verify.js

@@ -86,92 +51,134 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {

  var header = decodedToken.header;

-  if (!~options.algorithms.indexOf(header.alg)) {
-    return done(new JsonWebTokenError('invalid algorithm'));
+  if(header.alg ==='none' && decodedToken.signature) {


Let's try to do PRs as atomic as possible. This change is not related, if you are interested on it, please open a different PR.

I believe the change is related. Because the callback could unintentionally return undefined (for example it should return an error but instead it returns undefined) then suddenly all unsigned keys are accepted.

I see, but we should use all checks we currently use for that https://github.com/auth0/node-jsonwebtoken/blob/master/verify.js#L52-L91 Otherwise it would mean there is a bug in the current code.

Sorry I don't know what you mean to say. I don't think there is a bug in the current code but I do think the logic was off. And adding none to the algorithms within the decode function scares me. That isn't a problem in the current code, but with this particular PR it can easily compromise security.

One has to be very very strict about when to allow unsigned JWT's and consider any way how they could sneak in unintentionally. I've thought long about this and looked up the JWT / JWS standards and I think this is the proper way to do it.

We could discuss about if this should be in a separate PR or not, but let's not be too bureaucratic about it. If you agree with the code than why make a fuzz. We all have busy schedules, you know.

Let's take another approach to see if I explain better my thoughts:
What's the risk difference between passing a secret/key nowadays vs passing the secret/key using the new function?

Ah, I thought you already got that 😉. It's actually a security hole, not a risk. I made it more explicit now.

Before, if you passed an unsigned token AND a secret, it would be blocked. If you pass an unsigned token and no secret it would always pass.

Now with the callback code, when you pass an unsigned token, there will not be a secret and the token is valid. So to block it, it checks if unsigned tokens are allowed.

💡 or 🤔?

Better, now we have the rationale of the change.

Now with the callback code, when you pass an unsigned token, there will not be a secret

Why won't there be a secret? I can see in these use cases, let me know if you have others in mind:
(For all of them the input is an unsigned token with algorithm none)

Your verification requires a signed token, getSecret returns a secret => unsigned token + secret => verification fails.

Your verification requires a signed token, getSecret can't find a secret to use => error from getSecret => verification fails.

Your verification allows unsigned tokens => You don't pass getSecret function, if you do the function needs to return nothing as secret.

In your last commit in case of algorithm === none then getSecret returns undefined, in that case the door is just open to anyone to craft a JWT with same data as an expected signed JWT and none as algorithm, and since getSecret from consumer won't be called the token will pass.

I have a proposal for you, clean all the checks you have added or modified, leave just the call to getSecret and move the current checks where they should be in the new code. Starting from that point try to write tests which will make the token end up as "verified" when it shouldn't, or the other way around, then we can analyze them and build the fixes properly.

Sorry for the delay, I was kind of MIA.

Point 1: the getSecret function would normally use kid to determine which secret to return, but since algorithm is none and kid is absent, there's no secret to return. So then it would have to return a bogus key which I think doesn't make much sense.

Point 2: Theoretically it is a possibility but I think it is the wrong approach. The getSecret function should have a single, clear and intuitive purpose: getting the secret. It should therefor return a secret, and if that is not possible it should return an error. Nothing else. What you are proposing here is to give getSecret a second purpose, namely, to determine if an unsigned token is allowed or not. It is not intuitive and especially prone to disasters. For instance return keys[kid] is all it takes to leave the door wide open.

Point 3: By not supplying the getSecret function it is possible to allow ONLY unsigned tokens. In that case it would support only unsigned tokens, or only signed tokens but not both. If you want so support both by returning nothing (I think you mean 'undefined') then I think it's a bad idea, see point 2.

So I think there are two possibilities.

Allow only unsigned tokens OR only signed tokens, but not both. If the getSecret function is present, it will allow only signed tokens. If it is undefined it would allow only unsigned tokens, just like in the first part of your 3rd point.

Allow both unsigned and signed tokens at the same time. Allow unsigned tokens only based on a configuration setting, not based on the result of getSecret. Like in my PR, unsigned tokens are only allowed if the algorithms array contains none.

I think unsigned tokens only have use for testing and either way makes that possible but I think method 1 would be the least error prone.

From my previous comment:

In your last commit in case of algorithm === none then getSecret returns undefined, in that case the door is just open to anyone to craft a JWT with same data as an expected signed JWT and none as algorithm, and since getSecret from consumer won't be called the token will pass.

Re-reading all the code I've seen it would fail with invalid algorithm: none, therefore forget about that comment then.

We would need a major release for all these changes, since:

Tokens with none as algorithm will be accepted only when it is passed through options.algorithms.

Changes on errors.

I've seen a couple of things to mention. I'll do it on their lines today.

ziluvatar · 2017-10-09T12:16:32Z

README.md

@@ -191,6 +192,61 @@ jwt.verify(token, cert, { algorithms: ['RS256'] }, function (err, payload) {

 ```

+Example verify token with fetching public key from Keycloak server


I would keep this example as minimum as possible, like:

function getSecret(header, callback) { // fetch secret or public key const key = ...; callback(null, key); }

ziluvatar · 2017-10-09T12:17:17Z

README.md


 As mentioned in [this comment](https://github.com/auth0/node-jsonwebtoken/issues/208#issuecomment-231861138), there are other libraries that expect base64 encoded secrets (random bytes encoded using base64), if that is your case you can pass `Buffer.from(secret, 'base64')`, by doing this the secret will be decoded using base64 and the token verification will use the original random bytes.

 `options`

-* `algorithms`: List of strings with the names of the allowed algorithms. For instance, `["HS256", "HS384"]`.
+* `algorithms`: List of strings with the names of the allowed algorithms. For instance, `["HS256", "HS384"]`. If your `token` is unsigned, this option must include `["none"]`.


I said about in other comment, do this in a different PR.

ziluvatar · 2017-10-09T12:18:57Z

verify.js

+  }
+  else {
+    getSecret = function(header, callback) {
+      return callback(undefined, secretOrPublicKey);


by convention: null instead of undefined

ziluvatar · 2017-10-09T12:24:03Z

verify.js

-    return done(e);
+  if(typeof secretOrPublicKey === 'function') {
+    if(!callback) {
+      return done(new JsonWebTokenError('verify must be called asynchronous if secret or public key is provided as a callback'));


Not sure, maybe we shouldn't block this use case, the consumer should know about it (or say it in README).
I can imagine a system that loads the keys into an object on startup, even if their execution is sync they could take advantage of this function (they would have to call callback(null, key) but their execution is sync).
Thoughts?

How should the caller know whether the callback is going to be async or not?

It's their code, they should know it, having a callback it does not mean it is async, if you don't perform any async operation inside it would be just a normal function call, ie:

const keys = { kid1: 'a', kid2: 'b' } function getSecret(jwtHeaders, jwtClaims, callback) { callback(null, keys[jwtHeaders.kid]) }

That's a sync getSecret function. It's true that it could lead into issues if a future developer, seeing the signature with callback, makes it async (ie: calling an external resource). Not sure if we should block that use case for consumer safeness.

That's not what I meant. It's kind of hard to explain. Let me give it another shot. From the perspective of the decode function, it is not known if getSecret is async or not. That is important to know, because the flow will be different in both cases.

Therefor, if you're using the decode with a getSecret callback, it makes logical sense that decode also needs a callback. In that way it will work in both cases.

It is true that it doesn't imply it is async. If one is sure getSecret isn't async, one could also do this:

let payload; decode(token, getSecret, (err, result) => { payload = result; });

Not that I recommend it, but it is a possibility. I wouldn't do it that way, because if getSecret later becomes async it would fail. I hope I explained better this time.

🤔 I think we are kind of in the same page, so you as a user would feel weird if you pass a: getSecret(blah, callback) to a verify(token, getSecret). Is it? Probably you are right and we can add the constraint to only use the getSecret when the verify function is callback-ish. If we get people asking to open it for the non-callback-ish signature we can remove the error later, the other way around would be more difficult.
Fine, let's leave the error.

By the way I've been calling it getSecret function, but you can call it getKey, keying or whatever you think it's more meaningful.

ziluvatar · 2017-10-09T12:28:10Z

verify.js


-  var payload;
+    var isSigned = header.alg !== 'none';


Please, don't modify the code that is not needed to be modified for this change. If so be free to open another PR (can you revert your changes and apply only commits for the main change?)

ziluvatar · 2017-10-09T12:30:38Z

verify.js

  }

-  if (!valid)
-    return done(new JsonWebTokenError('invalid signature'));
+  return getSecret(header, function(err, secretOrPublicKey) {


I'd add the claims as well, they can be used by the function to use the issuer for JWKs: https://auth0.com/docs/jwks

I'd advise against that. The payload shouldn't be used before it token has been verified. Using the issuer is especially dangerous. The risk is that someone will use it to fetch the key from that issuer. A hacker can then easily make a token that will always pass. I don't see a use-case for it either as the client should already know which issuer to use for instance with a config setting.

Let's say I have an API, that API accepts tokens from 2 issuers, legacy authorization server and new authorization system, both of them hase a policy of certificate rotation, using JWKs to offer their public keys. The API, as a token verifier, needs to check their public keys in order to verify the signature, how would the API know that is the public key "now for this incoming JWT for this issuer"?

The API could use the getSecret function, asserting that the domain (+path?+protocol?...) for the issuer is in their list (like verifying the issuer) to prevent SSRF attacks, after the API could call the JWK endpoint to get the public key (probably with a cache), then it will filter by kid to select the right one.

The risk is that someone will use it to fetch the key from that issuer. A hacker can then easily make a token that will always pass
What you fetch from the issuer is the public key. What is used to sign is the private key, that is not offered by the issuer.

We are talking security here and imo that should be the priority. The payload has not been verified yet and is therefor not trustworthy. Using it anyway can open a host of security risks.

The use-case you describe can be solved in several other ways that won't compromise security:

the legacy system can use a fixed key, and it can be easily identified by the kid

if key rotation is required, one could try to fetch from both auth servers, only one will give a key. It can be cached so no performance is lost

one could use a different URL or endpoint that can be used to select the server

Imo the payload should be made only available when decode returns, and only if it has been verified. Using the payload before that for any purpose just isn't hygienic.

But in the end I'm not the boss of this package so it's just my opinion of course.

Using the header or the payload has the same effect: you are using fields without being verified. In both use cases (load key by kid and/or by issuer) you are relaying on the not-verified information and you will check if that makes sense (it's verified) afterwards.

If someone modifies the header to set a different kid, what would happen? The consumer code on getSecret won't find the key, and the verification will fail.
In case of issuer it's the same, consumer code on getSecret will try to get the key, if it can't get it the verification will fail.

On getSecret code the consumer must use only the claims/headers needed to figure out the key to use to verify the token.

Just adding my .02: When using ID Tokens from the OpenID Connect, you have the iss in payload. Fetching the keys from that exact issuer (via its well-known/openid-configuration) is prudent, because the thing you are validating is that the id_token is signed by that very same issuer, and nothing else. Since the issuer is part of the subjects identity, it does not open a security issue per se. The authentication is about sub@iss always, and we should allow for that use case.

ziluvatar · 2017-10-09T12:32:21Z

Thanks for the typescript typings 👏 (I added a comment that may change them)

Added features * verify's secretOrToken parameter can now be a callback function, see updated README.md for details * hardended checks for unsigned jwt, must now explicitly add ['none'] in algorithms to pass * updated tests and readme Performance improvements * removed need to clone options object * merged two decodes into one

fiddur

Mainly checked the debated verify-none-additions; looks good to me.

fiddur · 2017-10-19T08:24:50Z

verify.js

-    return done(e);
+  if(typeof secretOrPublicKey !== 'function') {
+    getSecret = function(header, callback) {
+      return callback(null, secretOrPublicKey);


Please don't shadow the callback variable from the outer scope in the closure, it makes it confusing to read.

fiddur · 2017-10-19T08:31:45Z

verify.js


-  var payload;
+  return getSecret(header, function(err, secretOrPublicKey) {


Again, don't shadow outer variable names. Something that was passed as a function is suddenly a string within the same function... (perhaps it is the upper variable that should be renamed in this case, but anyhow, don't reuse it for two different things)

ziluvatar · 2017-10-20T14:53:55Z

verify.js

-
-  if (parts.length !== 3){
-    return done(new JsonWebTokenError('jwt malformed'));
-  }


I'd leave this assertion here, the rest of the code should not be executed if this error happens.

No need for this as jws.decode already does the job.

Fine. Can you add a test for this case? (since they are not failing I assume we don't have for this scenario currently)

ziluvatar · 2017-11-17T16:37:13Z

@Richie765 not sure if you are waiting for us to review some part. I left a comment that was collapsed #413 (comment)

Let me know if you need anything from us.

ziluvatar

Although I do like your approach securing by default against none algorithm unless explicitly provided as option. This will force us to release this as a breaking change so a major version would be required (it may delay the merge of this feature).

As I commented in the past, you could develop this feature without forcing the breaking changes and create other PRs that can be delayed until next major version with: different errors + none changes.

If you want to apply extra security to the function (getSecret) usage, that's Fine, you could wrapped the function when passed by consumer and make some extra validation there (ie: don't allow empty secrets returned unless none it is explicitly set in options.algorithms). That way this is a new feature without breaking changes and we can release it right away.

I leave to you the decision. I added some comments below.

ziluvatar · 2017-12-11T14:57:55Z

verify.js

-  if (!~options.algorithms.indexOf(header.alg)) {
-    return done(new JsonWebTokenError('invalid algorithm'));
+  if(header.alg === 'none' && decodedToken.signature) {
+    return done(new JsonWebTokenError('invalid token: unsigned but signature is present'));


I think text is misleading kind of "unsigned but signed", what about invalid token: algorithm 'none' found but signature is present

Add test for this.

ziluvatar · 2017-12-12T10:09:44Z

verify.js

-
-  if (parts.length !== 3){
-    return done(new JsonWebTokenError('jwt malformed'));
-  }


Fine. Can you add a test for this case? (since they are not failing I assume we don't have for this scenario currently)

ziluvatar · 2017-12-12T10:15:39Z

verify.js

-    if (typeof payload.nbf !== 'number') {
-      return done(new JsonWebTokenError('invalid nbf value'));
+    if (!isSigned && secretOrPublicKey){
+      return done(new JsonWebTokenError('jwt must be signed if secret or public key is provided'));


Add test for this

ziluvatar · 2017-12-12T10:15:57Z

verify.js

-      return done(new NotBeforeError('jwt not active', new Date(payload.nbf * 1000)));
+
+    if (isSigned && !secretOrPublicKey) {
+      return done(new JsonWebTokenError('jwt is signed, secret or public key must be provided'));


Add test for this

ziluvatar · 2017-12-12T10:17:17Z

verify.js

-      return done(new TokenExpiredError('jwt expired', new Date(payload.exp * 1000)));
+
+    if (!algorithms || !~algorithms.indexOf(header.alg)) {
+      return done(new JsonWebTokenError('invalid algorithm: ' + header.alg));


modify current tests accordingly

JacoKoster · 2018-05-22T19:38:20Z

I have made the changes and made a pull request for the originating branch (Richie765#1)

With istanbul, i checked for the missing tests and noticed that the (isSigned && !secretOrPublicKey) are already covered by tests. Please advise how to continue further, as this is a feature i would really like to use.

ziluvatar · 2018-05-31T17:55:55Z

@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.

ziluvatar · 2018-06-11T15:50:53Z

Closed by: #480
Thanks @Richie765 for the initial push!

Richie765 added a commit to Richie765/DefinitelyTyped that referenced this pull request Oct 5, 2017

See PR auth0/node-jsonwebtoken#413

cdad572

Richie765 mentioned this pull request Oct 5, 2017

jsonwebtoken - Typings for PR413 DefinitelyTyped/DefinitelyTyped#20321

Closed

ziluvatar suggested changes Oct 9, 2017

View reviewed changes

Richie765 added 4 commits October 14, 2017 14:17

fix typo

f563a56

null instead of undefined

63cdbbb

removed example

603fd5a

Richie765 force-pushed the secret-callback branch from b5c9088 to 603fd5a Compare October 14, 2017 12:26

Don't call getSecret if jwt not signed

897f928

fiddur reviewed Oct 19, 2017

View reviewed changes

ziluvatar reviewed Oct 20, 2017

View reviewed changes

ziluvatar suggested changes Dec 12, 2017

View reviewed changes

ziluvatar mentioned this pull request Apr 5, 2018

Support for JWK #43

Closed

JacoKoster mentioned this pull request Jun 3, 2018

Secret callback revisited #480

Merged

ziluvatar closed this Jun 11, 2018

		@@ -191,6 +192,61 @@ jwt.verify(token, cert, { algorithms: ['RS256'] }, function (err, payload) {

		```

		Example verify token with fetching public key from Keycloak server


		var payload;
		return getSecret(header, function(err, secretOrPublicKey) {

Secret callback #413

Secret callback #413

Conversation

Richie765 commented Oct 5, 2017

Richie765 commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziluvatar commented Oct 9, 2017

fiddur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziluvatar commented Nov 17, 2017

ziluvatar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacoKoster commented May 22, 2018

ziluvatar commented May 31, 2018 • edited Loading

ziluvatar commented Jun 11, 2018

ziluvatar commented May 31, 2018 •

edited

Loading