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

Dynamically resolve secret key based on token #16

Closed
eventhough opened this issue Mar 10, 2015 · 9 comments · Fixed by #53
Closed

Dynamically resolve secret key based on token #16

eventhough opened this issue Mar 10, 2015 · 9 comments · Fixed by #53

Comments

@eventhough
Copy link
Contributor

It would be really nice if this library would support a dynamic secret key function for scenarios where we want to allow users to revoke all their valid tokens. If we assigned each user their own secret key in the database we can just update their secret key to invalidate existing tokens. This would force users to login again.

Here is the open issue on the other hapi-auth-jwt library:

ryanfitz/hapi-auth-jwt#9

@nelsonic
Copy link
Member

@eventhough I understand (and like) the theory behind this suggestion, but how would it work in practice...?
Would the key have to be looked up for every request?
(seems like a lot of over-head even if you're using Redis to cache the keys).

How would you know which key to use for a given incoming JWT?
If each user of the system had their own key the Plugin would have to know what datastore you are using in order to lookup the secret specific to the user ...

The way we are achieving this in our app is to treat the JWT signing secret as a rarely-changing key (shared across server nodes in a cluster) and have a Session ID (_sid_) inside the JWT which then gets looked up in the Database after the JWT has been verified. This still allows us to invalidate sessions on a per-user basis (e.g. if the person loses their device or wants to log-out remotely from a session). Each user has one valid sid per device/browser and the sid has the person's session info,
e.g:

// a session record
"dccacd30-8710-409e-85ce-06ef96416089": {
  "uid":"123",
  "start":"timestamp",
  "ended":false
}
// invalidated/terminated session:
"d2287fd1-6d85-420e-8cd0-b14b840f3ec1": {
  "uid":"123",
  "start":"timestamp",
  "ended":"timestamp"
}

then in the validateFunc we lookup the sid in the sessions table/collection and reject the request if the sid is no longer valid. pseudocode:

if(session.ended) { 
/* reject, send 401 */ 
} 
else { 
/* session is valid, continue */ 
}

The advantage of using the same JWT secret for all JWTs is the app has better stateless horizontal scaling (all nodes use the same secret to sign & verify tokens) and is more robust (harder to DDOS) because requests without a valid JWT are rejected much earlier in the request lifecycle (without flooding the database with lookups).

Also, under what circumstance would you want to invalidate all existing tokens at once?

@nelsonic
Copy link
Member

nelsonic commented Apr 9, 2015

@eventhough can you submit a PR for this (with tests ensuring we still have 100 test coverage and explanation in the documentation) ?

@eventhough
Copy link
Contributor Author

So I was reading your responses on auth0/hapi-auth-jwt and the way I was using this functionality was probably overkill. I had a secretKey assigned to every user which doesn't sound like a smart thing to do since it requires a db hit for every token verification. I like your idea of using something like redis to store a session and then use the jwt token to store a session id. Would you consider that a 'best practice'?

@nelsonic
Copy link
Member

nelsonic commented Apr 9, 2015

@eventhough "best practice" is to do as much verification at the earliest stage in the request lifecycle and reject bad requests as quickly as possible to avoid allowing bad people to consume your app's resources... While looking up one-key-per-user will be fast in Redis its still more than a mili-second (especially if the Redis Cache is not on the same instance as the node app...) which means that an attacker can flood your app with requests (which all have to hit Redis before being verified).

And, if the developer/app is choses to use a Cache that is slower than Redis (e.g. MongoDB or Postgres) to store the keys, the bottleneck will be even worse!
So, having one-key-per-user introduces an a DDOS attack vector (well-formed but invalid JWT tokens still require a DB lookup!)

However, given the _valid_ (multi-tenant) use-case described by @dschenkelman and in ryanfitz/hapi-auth-jwt#9

We could easily still do initial checks for validity and allow an optional keyLookup function to be passed in, which, if(typeof keyLookup === 'function') would lookup the key for each request before trying to decode the JWT...

I don't have time to write this (code, tests, example & docs) right now, but the people who need the feature can _send a PR_ and I'm happy to review, make suggestions and merge when ready.

@nelsonic
Copy link
Member

@eventhough Has this feature now been merged into ryanfitz/hapi-auth-jwt#9 ?

@eventhough
Copy link
Contributor Author

This feature has not been merged into ryanfitz/hapi-auth-jwt#9. I switched to hapi-auth-jwt2 because this project is much more active and well supported than the original.

When I get some time I will try to port over the multi-tenant use case but as I mentioned earlier, I decided not to use a secret key per user. I think storing a sessionId in the jwt token and revoking the session is a better use case for forcing a log out. (That's why I wanted one secret key per user.)

@nelsonic
Copy link
Member

Thanks for the endorsement! Please ⭐ the repo to help spread the word.
We look forward to a PR when you get time. 👍

@nelsonic
Copy link
Member

nelsonic commented Jun 7, 2015

@eventhough + @dschenkelman + @bbrown + @martinj + @ksck23
The PR: ryanfitz/hapi-auth-jwt#9 has still not been merged.

Do we want to support this use case...?

@eventhough
Copy link
Contributor Author

I was finally able to take a stab at this. I borrowed liberally from auth0/hapi-auth-jwt including the ability to pass extraInfo from the key lookup func to validateFunc. Not sure if this is the right approach. Everything is backwards compatible as well.

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

Successfully merging a pull request may close this issue.

2 participants