Add multi key and key id support. #33
Conversation
|
The first commit is #32 |
nigoroll
force-pushed
the
multi_algo
branch
2 times, most recently
from
19d67e7
to
2e124f6
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
If I read the coverage report correctly, the delta stems from 2e124f6 which, as explained in the commit message, I this focuses on python 3.7 for a good reason. |
nigoroll
force-pushed
the
multi_algo
branch
2 times, most recently
from
9ba885d
to
fa59c52
Compare
There was a problem hiding this comment.
Thanks for the PR!
There are a couple of points I would like to clarify before proceeding:
- The proposed settings schema: If I understood this correctly, each setting accepts a
dictand alist. During the signing process, the first element is used, but there are issued withdicts 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? - I suppose the
dictsettings 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.
I appreciate your contribution and the time you've invested into this solution. You've mentioned that you are not primarily a Python developer, so I've added a couple of links for you to check out before proceeding.
My goal here is to assist you in making this solution as simple as possible, while preserving the functionality. I'm not a big fan of settings because each new setting increases the complexity exponentially when the entire code base is taken into account, but this library is structured in a way that they are unavoidable. That is why I have suggested splitting signing and verification keys and algorithms.
As to the variable names, there is no limit in the amount of characters you can use or a performance penalty, so feel free to use explanatory variables so other maintainers find it easy to read and maintain.
|
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:
I don't think it is desirable to support a fourth case:
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. |
|
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
So this only affects As you suggest, one way to solve this would be to add something like a As you wrote later on, adding more settings seemed unattractive to me and as this only affected
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:
Do you have a preference?
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:
Yeah, habits of a C developer. BTW, C symbol length does not matter performance wise either, this is more a style habit carried over. |
|
@fitodic I have used separate commits in response to your comments, IMHO these should be squashed before merge |
|
Hi @ashokdelphia ,
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 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. |
|
@ashokdelphia you got a valid point. Having less variants would be advantageous. |
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.) |
|
@ashokdelphia You are right, this code does not currently allow for multiple, unidentified keys. Let's go through your scenario (please forgive any syntax errors in this mockup code):
|
|
@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. |
|
@ashokdelphia I still have not written the |
|
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 |
|
force-pushed adressing @ashokdelphia 's feedback |
There was a problem hiding this comment.
@nigoroll Thank you for the time and effort you've put into this. I see you and @ashokdelphia really hashed this out so thank you both 🙂
I have nothing more to add apart from the comments requesting minor changes that would ease the future maintenance of this library.
I've also seen that some edge cases haven't been covered, primarily:
- The algorithm in the header is not found in the updated list of algorithms (InvalidAlgorithmError)
- The
InvalidKeyErroris raised when thekidis not found among the specified keys - The JWT couldn't be decoded
Given the sheer amount of configuration possibilities that are now available, I cannot be sure if any more pressing edge cases have been omitted from the test suite. If you or @ashokdelphia think of something, please feel free to mention them or add them.
as suggested by fitodic here: Styria-Digital#33 (comment)
as suggested by fitodic here: Styria-Digital#33 (comment)
Existing code made key rollovers or algorithm changes hard and basically required a breaking change: Once any of JWT_ALGORITHM, JWT_SECRET_KEY, or JWT_PRIVATE_KEY/JWT_PUBLIC_KEY were changed, existing tokens were rendered invalid. We now support JWT_ALGORITHM, JWT_SECRET_KEY, and JWT_PUBLIC_KEY optionally being a list, where all members are accepted as valid. When JWT_SECRET_KEY is a list, the first member is used for signing and all others are accepted for verification.
as suggested by fitodic here: Styria-Digital#33 (comment)
|
After another minor change it seems we are getting a green light from travis now. I will squash commits. |
The previous commit added support for multiple keys, which (despite
being useful as a fallback, if anything) implies multiple verification
attempts and thus is not computationally efficient.
We now support identifing keys by key id ("kid" header): When a JWT
carries a key id, we can identify immediately if it is known and only
need to make at most one verification attempt.
To configure keys with ids, JWT_SECRET_KEY, JWT_PRIVATE_KEY and
JWT_PUBLIC_KEY can now also be a dict in the form
{ "kid1": key1, "kid2": key2, ... }
When a JWT does not carry a key id ("kid" header), the default is to
fall back to trying all keys if keys are named (defined as a dict).
Setting JWT_INSIST_ON_KID: True avoids this fallback and requires any
JWT to be validated to carry a key id _if_ key IDs are used
Closes Styria-Digital#31
as suggested by fitodic here: Styria-Digital#33 (comment)
|
squash done |
|
@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 |
|
@fitodic I feel bad about wasting your time with these little details, thank you for your thorough work. |
After all your hard work, it's the least I can do. 🙂 |
|
@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. 🙂 |
|
@fitodic it has been a pleasure working with you |
It has been a pleasure working with you too 🙂 The new release is available on PyPI, and the documentation has been updated. |
|
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 |
This PR consists of two patches:
Please refer to the individual commit messages and the updated documentation.
A changelog will be added to the PR if/when otherwise accepted.