NetDevPack / Security.Jwt

Jwt Manager. Set of components to deal with Jwt Stuff. Automate your key rotating, add support for jwks_uri. Store your cryptography keys in a secure place.
MIT License
271 stars 38 forks source link

Clarify key rotation #39

Closed sherlock1982 closed 1 year ago

sherlock1982 commented 1 year ago

I'm checking a bit the code and don't understand one thing here.

Every 90 days a new key will be created and old key will be discarded. It means immediate invalidation of all of the tokens.

Therefore all of the clients need to refresh tokens? Even if they got them like 1 minute ago? Would it make more sense to still keep the old keys but validate the signature only if it was issued the moment the key was active? I think this is how pretty much DataProtection key rotation works in .NET Core.

P.S. I'm also using this key for refresh token. It means that all refresh tokens will be also immediately invalidated.

brunobritodev commented 1 year ago

Hi @sherlock1982, your observation is valid!

The process does not nullify the token you created earlier. Here's what actually happens in the background:

  1. Deactivate the old Key (Key 0), so it'll not be able to generate new tokens
  2. Generate a new Key (Key 1)
  3. Remove the private key from the old Key (Key 0)

When you access https://my-api/jwks_uri, it will present two public keys:

  1. The public key associated with your most recent key
  2. The public of old key

By design, JWT validation receives an array known as a JWK Set. Hence, it will continue to validate the older JWT, but you won't be able to generate new JWTs based on the old key.

Here is the point where it get all keys, not only those with private key https://github.com/NetDevPack/Security.Jwt/blob/22e3a744e0a08b0025881e4941b856d664ce5bab/src/NetDevPack.Security.Jwt.AspNetCore/JwtServiceDiscoveryMiddleware.cs#L22

To gain a deeper understanding of JWK Set behavior, please refer to the following link: https://www.rfc-editor.org/rfc/rfc7517#section-5

For more information on why we remove the private key from the old key, consult this NIST document: https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-57pt1r4.pdf

sherlock1982 commented 1 year ago

If you have a look at this code inside JwtServiceValidationHandler.cs:

    //Retrieve the corresponding Public Key from our data store
    var keyMaterialTask = jwtService.GetCurrentSecurityKey();
    Task.WaitAll(keyMaterialTask);
    validationParameters.IssuerSigningKey = keyMaterialTask.Result;

It looks like only the very latest key is used when validating. It looks like an issue to me. To provide more than 1 key you need to use IssuerSigningKeys or IssuerSigningKeyResolver or other fields

brunobritodev commented 1 year ago

You are right, JwtServiceValidationHandler should take the last JwtOptions.AlgorithmsToKeep keys created to validate a token.

Thank you for your efforts. We'll fix it asap

sherlock1982 commented 1 year ago

Also note that valid keys should somehow correspond to the date of the issue of the token.

For example if token was issued on the 1st of may it should be signed by one of the keys valid on that date. Otherwise stolen keys will be valid forever.

With the current issue you’re kinda protected but blindly trusting all the keys will make it worse.

Well of course if we’re talking about jwt access tokens valid for 1 hour this change doesn’t make sense. But then there’s no point to store old keys at all.

brunobritodev commented 1 year ago

We will apply the same protocols we do for jwks. The system will only accept the most recent N keys (set at JwtOptions.AlgorithmsToKeep, typically the last two. If a key has been issued earlier than that, it will not be accepted.

If a developer wishes to retain more than two keys, they may do so, but it is important to note that this comes with its own set of risks. We have established a set of commonly used options in the JwtOptions.cs file. While it is feasible to modify these settings, we believe that a developer would need a strong rationale for making such changes.

Well of course if we’re talking about jwt access tokens valid for 1 hour this change doesn’t make sense. But then there’s no point to store old keys at all.

We pretend to follow the rfc's guidelines of jwk and jwks design. So while 1 hour is the best practice, we not enforce it for developers who have enough reasons to not follow it. So instead fight against, we just follow the paper.