-
-
Notifications
You must be signed in to change notification settings - Fork 696
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
Add flexible and complete verification options #131
Changes from 5 commits
3ada770
b08a827
143feed
e62df13
13d2420
0ab287b
6e6046c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
|
||
|
||
class PyJWT(object): | ||
def __init__(self, algorithms=None): | ||
def __init__(self, algorithms=None, options=None): | ||
self._algorithms = get_default_algorithms() | ||
self._valid_algs = set(algorithms) if algorithms is not None else set(self._algorithms) | ||
|
||
|
@@ -25,6 +25,19 @@ def __init__(self, algorithms=None): | |
if key not in self._valid_algs: | ||
del self._algorithms[key] | ||
|
||
if not options: | ||
options = {} | ||
|
||
self.default_options = { | ||
'verify_signature': True, | ||
'verify_exp': True, | ||
'verify_nbf': True, | ||
'verify_iat': True, | ||
'verify_aud': True, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to see these abbreviations expanded. The only reason they are abbreviated in the standard is to reduce the amount of data transmitted. Here it just serves to obfuscate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How many people will choose to solve, non-verification messages by figuring out they can set all of these to False rather than taking the time to understand what it all means? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only way to avoid consumers skipping verification is to completely disallow it, which ignores a huge number of use cases that need to turn off various checks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I support keeping these abbreviated so they are consistent with the claim names. This is similar to how the Ruby JWT library does things. I think we should keep them the way they are. Regarding @johnlockwood-wf's comment about setting everything to False; I'm not sure we can avoid people being stupid with their usage. Defaulting to secure (and to the standard) is the best behavior here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. they should at least be labeled with expansions, or have a "constants" class that labels the keys, such as:
|
||
} | ||
|
||
self.options = self._merge_options(self.default_options, options) | ||
|
||
def register_algorithm(self, alg_id, alg_obj): | ||
""" | ||
Registers a new Algorithm for use when creating and verifying tokens. | ||
|
@@ -110,14 +123,16 @@ def encode(self, payload, key, algorithm='HS256', headers=None, json_encoder=Non | |
|
||
return b'.'.join(segments) | ||
|
||
def decode(self, jwt, key='', verify=True, algorithms=None, **kwargs): | ||
def decode(self, jwt, key='', verify=True, algorithms=None, options=None, **kwargs): | ||
payload, signing_input, header, signature = self._load(jwt) | ||
|
||
if verify: | ||
self._verify_signature(payload, signing_input, header, signature, | ||
key, algorithms) | ||
merged_options = self._merge_options(override_options=options) | ||
if merged_options.get('verify_signature'): | ||
self._verify_signature(payload, signing_input, header, signature, | ||
key, algorithms) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think one argument that cancel's out the other is dangerous and adds a lot of complexity. It gives attackers an attack vector they could drive a truck through. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's make it complicated to setup a non-standard set of claims verifications. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see no need to over complicate non-standard claims verifications. I believe this would lead more people not understanding what they are doing, not fewer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see the argument about verify vs verify_options in the options dict though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the ruby library that the original issue was based on, it looks like they use the I can see taking it out of the dictionary completely to be the same as the ruby library or doing something like this: merged_options = self._merge_options(override_options=options)
if verify or merged_options.get('verify_signature'):
self._verify_signature(payload, signing_input, header, signature,
key, algorithms)
self._validate_claims(payload, options=merged_options, **kwargs) This would avoid the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about adding common non-standard claims verification to the api? This way you have no single highly complex interface. Adding an options keyword argument that can take five different keys is the same as adding those keys to the method interface:
The order of complexity is increased for each option. As experts in the use cases, you will give less experienced users better information about how they should do things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I disagree with you here entirely. The options object is much more readable than simple adding an argument to the method's arguments and that gives it an advantage over simply adding arguments. I think maintaining some level of parity with ruby-jwt's API is also valuable here. (even though I'm not saying we should lock ourselves down to just matching ruby-jwt either; its just that I happen to agree with their design) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can always iterate on this further. I think this design is a big improvement over what we have currently. I'm open to PRs suggesting further enhancements before we do a release. The nice thing about code is that we can change it (until a release; then we have to consider API changes and how they will affect consumers) |
||
|
||
self._validate_claims(payload, **kwargs) | ||
self._validate_claims(payload, options=merged_options, **kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the call that happens here is configurable? If you build your own claim verifier, you can configure jwt with it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The number one way people mess up security is by trying to write it themselves. Forcing developers to write their own entire claim verifier, instead of allowing them to turn off the specific functionality that they do not need is going to cause much larger issues, especially when missing edge cases. I would argue pretty strongly against going that route. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that giving people specific flags to turn on and off (like the PR does) is better than letting them pass in an entirely separate claims verifier. If they really want to do something highly custom, they can always inherit from PyJWT and override methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. People mess things up because they are hard and complicated and take a lot of knowledge to know if they are doing the right thing or not. If we can reduce the complication and give them an interface that better explains itself, they will be less likely to screw it up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if the individual claims verification functionality was broken up, it would only leave it to the more advanced user to compose them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with you that people screw things up because they are hard and complicated. However, I think that's why its simplest to present a secure set of defaults and make people aware of the fact that they can modify them at their own risk. @michaeldavis-wf You might want to add to the README that changing the defaults is a behavior done at their own risk and may make their application less secure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mark-adams I updated the README with a note outlining that. |
||
|
||
return payload | ||
|
||
|
@@ -177,8 +192,8 @@ def _verify_signature(self, payload, signing_input, header, signature, | |
except KeyError: | ||
raise InvalidAlgorithmError('Algorithm not supported') | ||
|
||
def _validate_claims(self, payload, verify_expiration=True, leeway=0, | ||
audience=None, issuer=None): | ||
def _validate_claims(self, payload, audience=None, issuer=None, leeway=0, | ||
options=None, **kwargs): | ||
if isinstance(leeway, timedelta): | ||
leeway = timedelta_total_seconds(leeway) | ||
|
||
|
@@ -187,7 +202,7 @@ def _validate_claims(self, payload, verify_expiration=True, leeway=0, | |
|
||
now = timegm(datetime.utcnow().utctimetuple()) | ||
|
||
if 'iat' in payload: | ||
if 'iat' in payload and options.get('verify_iat'): | ||
try: | ||
iat = int(payload['iat']) | ||
except ValueError: | ||
|
@@ -196,7 +211,7 @@ def _validate_claims(self, payload, verify_expiration=True, leeway=0, | |
if iat > (now + leeway): | ||
raise InvalidIssuedAtError('Issued At claim (iat) cannot be in the future.') | ||
|
||
if 'nbf' in payload and verify_expiration: | ||
if 'nbf' in payload and options.get('verify_nbf'): | ||
try: | ||
nbf = int(payload['nbf']) | ||
except ValueError: | ||
|
@@ -205,7 +220,7 @@ def _validate_claims(self, payload, verify_expiration=True, leeway=0, | |
if nbf > (now + leeway): | ||
raise ImmatureSignatureError('The token is not yet valid (nbf)') | ||
|
||
if 'exp' in payload and verify_expiration: | ||
if 'exp' in payload and options.get('verify_exp'): | ||
try: | ||
exp = int(payload['exp']) | ||
except ValueError: | ||
|
@@ -214,7 +229,7 @@ def _validate_claims(self, payload, verify_expiration=True, leeway=0, | |
if exp < (now - leeway): | ||
raise ExpiredSignatureError('Signature has expired') | ||
|
||
if 'aud' in payload: | ||
if 'aud' in payload and options.get('verify_aud'): | ||
audience_claims = payload['aud'] | ||
if isinstance(audience_claims, string_types): | ||
audience_claims = [audience_claims] | ||
|
@@ -233,6 +248,21 @@ def _validate_claims(self, payload, verify_expiration=True, leeway=0, | |
if payload.get('iss') != issuer: | ||
raise InvalidIssuerError('Invalid issuer') | ||
|
||
def _merge_options(self, default_options=None, override_options=None): | ||
if not default_options: | ||
default_options = {} | ||
|
||
if not override_options: | ||
override_options = {} | ||
|
||
try: | ||
merged_options = self.default_options.copy() | ||
merged_options.update(override_options) | ||
except (AttributeError, ValueError) as e: | ||
raise TypeError('options must be a dictionary: %s' % e) | ||
|
||
return merged_options | ||
|
||
|
||
_jwt_global_obj = PyJWT() | ||
encode = _jwt_global_obj.encode | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about the keyword being,
verification_options
, so they are well categorized.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason why we couldn't use
options=
for other options later. I feel likeverification_options
would restrict what we could do with this argument unnecessarily.In addition, all the current options start with
verify_
so I feel like it'd be a bit redundant to call itverification_options
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grouping arguments into a category is better than having a lot of uncategorized arguments. If there are more unrelated types of arguments in the future, they could be added to another group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and to reduce redundancy, the verification_options dict could leave out the
verify_
prefix of the keys.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As maintainers it is also easier to commit to a more restricted change to the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the long-run, I'd rather add some sort of custom extensibility point (read: callable) that you could pass for additional validations on top of those provided by the library.
I don't feel like we're at risk here of adding "too many unrelated arguments or options" even though I do understand your concern. We aren't going to add all sorts of crazy willy-nilly validations that will require new options; our main focus is on parity with the validations with the JWT spec and I can't see it requiring us to add too many more options here outside of what's already in the spec. A couple more may be added from some additional work we'll probably wind up doing with JWK, but I think the number of new options would be very limited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem discussing more ideas in future PRs but I believe this is better than what we currently have for sure.