nigoroll commented 4 years ago

This PR consists of two patches:

The first adds support for accepting multiple keys
The second adds support for named keys (with key ids)

Please refer to the individual commit messages and the updated documentation.

A changelog will be added to the PR if/when otherwise accepted.

nigoroll commented 4 years ago

The first commit is #32

codecov-io commented 4 years ago

Codecov Report

Merging #33 into master will increase coverage by 0.22%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   97.28%   97.50%   +0.22%     
==========================================
  Files          19       19              
  Lines         442      481      +39     
  Branches       32       44      +12     
==========================================
+ Hits          430      469      +39     
  Misses          9        9              
  Partials        3        3

Flag	Coverage Δ
#codecov	`97.50% <100.00%> (+0.22%)`	:arrow_up:
#dj111	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#dj20	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#dj21	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#dj22	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#dj30	`97.50% <100.00%> (+0.22%)`	:arrow_up:
#drf310	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#drf311	`97.50% <100.00%> (+0.22%)`	:arrow_up:
#drf37	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#drf38	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#drf39	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#py27	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#py35	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#py36	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#py37	`97.02% <100.00%> (+0.26%)`	:arrow_up:
#py38	`96.88% <100.00%> (+0.27%)`	:arrow_up:

Impacted Files	Coverage Δ
src/rest_framework_jwt/settings.py	`100.00% <ø> (ø)`
src/rest_framework_jwt/utils.py	`99.20% <100.00%> (+0.35%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 612ba1e...4a4bf99. Read the comment docs.

nigoroll commented 4 years ago

If I read the coverage report correctly, the delta stems from https://github.com/Styria-Digital/django-rest-framework-jwt/pull/33/commits/2e124f60e10ea841be33060bbc560f71ace06092 which, as explained in the commit message, I this focuses on python 3.7 for a good reason.

ashokdelphia commented 4 years ago

Really happy to see support for multiple keys and key ids coming along.

It looks like someone could also use this to migrate between algorithms, which is also very welcome.

I'm a little unsure of the behaviour around a missing key id value. I would expect to only need to support that while migrating from an older scheme, to one with key ids.

I think there are three scenarios I would want to support:

someone is using this with an older configuration: no kid, a single key or secret and single algorithm
someone moving from an older configuration with no kid, to one with multiple, identified keys while still supporting the tokens issued with no kid; in this case I'd expect to identify in the configuration which key/secret and algorithm to use when the token has no kid
someone using multiple, identified keys; in this case I'd want to reject tokens that have no kid in the header

I don't think it is desirable to support a fourth case:

using multiple, identified keys, but allowing an attacker to make tokens that try all of the keys in sequence (this would be necessary to allow rotating unidentified keys, but I think that brings a lot of baggage with it)

Put another way: I think it should be possible to always pick a single algorithm and secret/key based on the header information supplied in the token, and the configuration.

nigoroll commented 4 years ago

Hi @fitodic,

thank you for your comprehensive review, I highly appreciate you taking the time to provide such helpful feedback.

I am going to respond to the overall comments first, then on the detailed comments on code

1. The proposed settings schema: If I understood this correctly, each setting accepts a `dict` and a `list`. During the signing process, the first element is used, but there are issued with `dict`s not being ordered in until Python 3.7. Why not split these into multiple settings and drop the use of dicts where it isn't necessary or where order needs to be preserved?

So this only affects JWT_SECRET_KEY, the symmetric signing secret setting. For asymmetric crypto, there already is the JWT_PRIVATE_KEY/JWT_PUBLIC_KEY separation.

As you suggest, one way to solve this would be to add something like a JWT_SECRET_SIGNING_KEY which would be required to contain either a scalar or a dict with exactly one member.

As you wrote later on, adding more settings seemed unattractive to me and as this only affected

users of python < 3.7
who want to use multiple (named) key ids
with symmetric signatures

I thought that keeping things simple was the best choice going forward, in particular as the old pythons will eventually become obsolete.

If you think that this is important, I guess our options are:

to use an OrderedDict
to use another setting like JWT_SECRET_SIGNING_KEY

Do you have a preference?

2. I suppose the `dict` settings are used to support named keys, but why is this necessary? Is it not enough to handle this via ordering or specifying only the specific keys? I believe adopting a more explicit settings schema would simplify the JWT decode process.

In general, key ids allow to specify which key is to be used, such that at most one verification attempt has to be made. To elaborate:

trying multiple keys is a waste of cycles, and in particular asymmetric crypto is relatively expensive. Maybe not that much in the realm of django, but, for example my machine does 15492.0 verifies/s for rsa 4096 (openssl speed rsa), which makes ~65us per RSA verify in the core openssl code alone. At least in my mind, this is something I would want to avoid
- this aspect is particularly true for verifying JWTs on other systems: Consider a CDN node which has to verify JWTs for every request. For a system which will normally process ~10k req/s on one CPU code, one RSA might cut that performance roughly in half, an additional attempt takes it down to about a third etc.
- But, true, for this scenario is is more important to produce JWTs with key ids than to verify with optimal efficiency in Django
naming keys removes guesswork, which I expect to be helpful for analyzing any potential issues. If a key id has been used for signing, that key id needs to verify ok.

As to the variable names (...)

Yeah, habits of a C developer. BTW, C symbol length does not matter performance wise either, this is more a style habit carried over.

nigoroll commented 4 years ago

@fitodic I have used separate commits in response to your comments, IMHO these should be squashed before merge

nigoroll commented 4 years ago

Hi @ashokdelphia ,

It looks like someone could also use this to migrate between algorithms, which is also very welcome.

Yes, the motivation is to support all kinds of migrations as well as regular key rollover.

If I read your comment correctly, you agree that, if a kid header is present, we should only try a known key by that id. But you are also saying that we should not always fall back to trying all the keys if there is no kid header, right? That would probably require changing the configuration or adding another flag like JWT_INSIST_ON_KID: Such a flag could be turned on after a transition period to refuse JWTs without a kid header.

What do you think?

ashokdelphia commented 4 years ago

But you are also saying that we should not always fall back to trying all the keys if there is no kid header, right? That would probably require changing the configuration or adding another flag like JWT_INSIST_ON_KID: Such a flag could be turned on after a transition period to refuse JWTs without a kid header.

What do you think?

I think you're right that this comes down to how to express it in the config.

I don't think a flag for 'insist on kid' is quite right; if I follow what you intend with that, it sounds like you'd still be looping over all of the keys when someone is mid-transition.

I would be tempted to allow fewer 'shapes' of configuration. Either you have a single key, which doesn't have a kid, or a set of keys each of which must have a kid. In the latter case, you could have an optional config variable for which kid should be used in the case of a token without a kid in the header.

The main case that would disallow would be rotating unidentified keys. I think that's naturally hazardous, and worth excluding so that by the time we're validating a token we always know which algorithm and key / secret should be used.

nigoroll commented 4 years ago

@ashokdelphia you got a valid point. Having less variants would be advantageous. Yet IIUC your proposal would make it impossible to transition from multiple, unidentified keys to named keys (with a kid). I agree that this setup should be used, but for someone who already finds herself in that situation, there should be a migration option towards the better setup. IOW, this migration option appears important to me, so being short of any better idea, I will go with JWT_INSIST_ON_KID now.

ashokdelphia commented 4 years ago

Yet IIUC your proposal would make it impossible to transition from multiple, unidentified keys to named keys (with a kid).

Perhaps I'm missing some essential point, but I don't think having multiple unidentified keys is possible at the moment.

I'm not sure it's good to support multiple unidentified keys at all, since I think it would be unsafe in general unless they were either all symmetric or all asymmetric keys. Even then, being able to try a list of keys sounds better for an attacker than for the defence.

If you implement it the way you're thinking will I be able to gracefully transition from a single unidentified key to multiple identified keys without having to allow trying multiple keys, potentially of different algorithms. (In my particular case, I want to move from HS256 to RS512, so I'm naturally concerned about the potential to confuse a public key and a symmetric secret.)

nigoroll commented 4 years ago

@ashokdelphia You are right, this code does not currently allow for multiple, unidentified keys. Yet it could be used in an environment where JWTs are generated outside this module, or even outside Django. In my world, rollover of unidentified keys is common practice, unfortunately, so while I would want to avoid it, I neither want to write code locking in people who find themselves in such environments. In general, the associated risk is the added validation overhead, as explained in the second half of this comment. This risk will be avoided specifically with JWT_INSIST_ON_KID and I do not see any other (security) related risks. In other words, if your policy is to not support multiple, unidentified keys, you will be able to enforce just that.

Let's go through your scenario (please forgive any syntax errors in this mockup code):

you start off with
```
"JWT_SECRET":"HMAC_KEY"
```
you add your RSA keypair and specify that you want to use RSA for signing, but still accept HMACs:

"JWT_SECRET":"HMAC_KEY",
"JWT_PRIVATE_KEY": {"kid": load_pem_private_key(...)},
"JWT_PUBLIC_KEY": {"kid": load_pem_public_key(...)},
"JWT_INSIST_ON_KID": True,
"JWT_ALGORITHM": ["RS256", "HS256"],

Wait until your HS256 JWTs expire, then remove HS* support:

"JWT_SECRET": None,
"JWT_PRIVATE_KEY": {"kid": load_pem_private_key(...)},
"JWT_PUBLIC_KEY": {"kid": load_pem_public_key(...)},
"JWT_INSIST_ON_KID": True,
"JWT_ALGORITHM": "RS256",

Roll over by signing with a new private key, still accepting the old one

"JWT_SECRET": None,
"JWT_PRIVATE_KEY": {"nukid": load_pem_private_key(...)},
"JWT_PUBLIC_KEY": {"nukid": load_pem_public_key(...), "kid": load_pem_public_key(...)},
"JWT_INSIST_ON_KID": True,
"JWT_ALGORITHM": "RS256",

ashokdelphia commented 4 years ago

@nigoroll: Thanks for the detailed example. I think I was misunderstanding how INSIST_ON_KID would work. In step 2 there, it sounds like INSIST_ON_KID would be set, but we'd still be accepting symmetric tokens with no kid, which would indeed allow what I was asking about.

I'll take a closer look at the implementation later; thanks.

nigoroll commented 4 years ago

@ashokdelphia I still have not written the INSIST_ON_KID code so there is nothing for you to look at more closely yet. I should be ready in an hour or so, please check back then.

nigoroll commented 4 years ago

I have force-pushed an update which addresses feedback:

To solve the dict ordering issue, we really do not need to do much except to advise users of python versions < 3.7 to use an OrderedDict instead of an ordinary dict
INSIST_ON_KID has been added, see previous notes for detailed discussion.

ashokdelphia commented 4 years ago

I have force-pushed an update which addresses feedback:

Thanks for being defensive about the potential for crossing symmetric and asymmetric algorithms.

I also appreciate how you've kept that separate, and also allowed INSIST_ON_KID to permit kid-less tokens as long as key ids are not defined in the relevant config. That will make this a good fit for a migration I need to do shortly.

nigoroll commented 4 years ago

force-pushed adressing @ashokdelphia 's feedback

nigoroll commented 4 years ago

@fitodic Apologies for the long delay on my side and thank you very much for your detailed feedback. I hope to have addressed all of it. At this point, here is a travis build error which seems unrelated, but I will try to fix or work around it to get ahead. Please note that I have kept some intermediate commits with the intent of making your review easier, but I would suggest we squash some before a possible merge. Again, thank you!

nigoroll commented 4 years ago

so with pytest, all tests succeed, but I can reproduce the pytest failures via tox locally as seen with travis. If anyone has any input? This happens equally on the master branch, so whatever changed seems to be unrelated to this PR.

(edit: clarify that the tox/pytest issue is unrelated to this PR)

nigoroll commented 4 years ago

wrt https://github.com/Styria-Digital/django-rest-framework-jwt/pull/33#issuecomment-616475204 I am totally lost. Will tend to something else any would appreciate help from a python wizard

fitodic commented 4 years ago

@fitodic Apologies for the long delay on my side and thank you very much for your detailed feedback. I hope to have addressed all of it. At this point, here is a travis build error which seems unrelated, but I will try to fix or work around it to get ahead. Please note that I have kept some intermediate commits with the intent of making your review easier, but I would suggest we squash some before a possible merge. Again, thank you!

There's really no need to apologize. These are stressful times for all of us and I really appreciate your continued involvement. These things take time, especially in these circumstances, so there's really no rush. Take care of yourself and your loved ones and we'll bring this to the finish line in no time. :slightly_smiling_face: Don't worry about the commits or waste time merging them. If that is what you wish, I'll squash them during the merge process.

so with pytest, all tests succeed, but I can reproduce the pytest failures via tox locally as seen with travis. If anyone has any input? This happens equally on the master branch, so whatever changed seems to be unrelated to this PR.

(edit: clarify that the tox/pytest issue is unrelated to this PR)

I'm a bit lost. You can or can't reproduce this issue? As you mentiond, this probably isn't related to this PR as it's failing in the master branch as well. I'll have a look and get back to you as soon as I have something concrete.

nigoroll commented 4 years ago

@fitodic thank you and I also wish you and really everybody to stay safe. Regarding the squashes: Yes, I would like to tidy up the commits when we are ready or you can do it also. If you just want to merge when you feel we are ready, please do whatever you find appropriate or let me know to do a final squash. Regarding the jenkins/tox failure:

running pytest locally succeeds
running tox locally fails for a failing pytest

This is is reproducible locally and not just happening with Jenkins.

nigoroll commented 4 years ago

After another minor change it seems we are getting a green light from travis now. I will squash commits.

nigoroll commented 4 years ago

squash done

nigoroll commented 4 years ago

@fitodic Sorry for having missed some conversations, gh hid them from me by default and I assumed that it would only do that for resolved conversations. Looking at them now

fitodic commented 4 years ago

@fitodic Sorry for having missed some conversations, gh hid them from me by default and I assumed that it would only do that for resolved conversations. Looking at them now

Don't worry about it, happens to all of us. That's why I started reviewing everything anew from the /files endpoint some time ago. At least GitHub has the Mark as viewed checkbox next to each file :laughing:

nigoroll commented 4 years ago

@fitodic I feel bad about wasting your time with these little details, thank you for your thorough work. I hope all comments are addressed now.

fitodic commented 4 years ago

@fitodic I feel bad about wasting your time with these little details, thank you for your thorough work. I hope all comments are addressed now.

After all your hard work, it's the least I can do. :slightly_smiling_face:

fitodic commented 4 years ago

@nigoroll I've squashed the commits as you've requested. This feature definitely improves the overall quality of this library so once again, thank you for your contribution and patience. :slightly_smiling_face:

nigoroll commented 4 years ago

@fitodic it has been a pleasure working with you

fitodic commented 4 years ago

@fitodic it has been a pleasure working with you

It has been a pleasure working with you too :slightly_smiling_face: The new release is available on PyPI, and the documentation has been updated.

nigoroll commented 4 years ago

thx. Seeing there html-render of the docs I noticed that we could look after some indentation fixes still and maybe give the settings own paragraphs. I might get back to that

Styria-Digital / django-rest-framework-jwt

Add multi key and key id support. #33

Codecov Report