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

Add flexible and complete verification options #131

Merged
merged 7 commits into from
Apr 9, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,5 @@ Patches and Suggestions
- Mark Adams <mark@markadams.me>

- Wouter Bolsterlee <uws@xs4all.nl>

- Michael Davis <mike.philip.davis@gmail.com> <mike.davis@workiva.com>
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
-------------------------------------------------------------------------
### Changed
- Added this CHANGELOG.md file
- Added flexible and complete verification options. #131

### Fixed
- Placeholder
Expand Down
28 changes: 28 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,34 @@ except jwt.InvalidTokenError:
pass # do something sensible here, e.g. return HTTP 403 status code
```

You may also override exception checking via an `options` dictionary. The default
options are as follows:

```python
options = {
'verify_signature': True,
'verify_exp': True,
'verify_nbf': True,
'verify_iat': True,
'verify_aud`: True
}
```

You can skip individual checks by passing an `options` dictionary with certain keys set to `False`.
For example, if you want to verify the signature of a JWT that has already expired.

```python
options = {
'verify_exp': True,
}

jwt.decode('someJWTstring', 'secret', options=options)
```

**NOTE**: *Changing the default behavior is done at your own risk, and almost certainly will make your
application less secure. Doing so should only be done with a very clear understanding of what you
are doing.*

## Tests

You can run tests from the project root after cloning with:
Expand Down
52 changes: 41 additions & 11 deletions jwt/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@


class PyJWT(object):
def __init__(self, algorithms=None):
def __init__(self, algorithms=None, options=None):

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.

Copy link
Contributor

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 like verification_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 it verification_options.

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.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify_claims = {'exp': False, 'aud': False}

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

self._algorithms = get_default_algorithms()
self._valid_algs = set(algorithms) if algorithms is not None else set(self._algorithms)

Expand All @@ -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,

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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:

class CLAIMS_KEYS(object):
    class VERIFY(object):
        EXPIRATION = 'verify_exp'
        ISSUED_AT = 'verify_iat'

}

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.
Expand Down Expand Up @@ -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)

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 verify kwarg instead of verify_signature in the options dictionary at all.

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 verify kwarg from overriding the other options and require that the developer double opt-in to not verifying the signature.

Choose a reason for hiding this comment

The 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:

decode(self, jwt, key='', verify=True, algorithms=None, verify_signature=False, verify_exp=False, verify_nbf=False, verify_iat=False, verify_aud=False, **kwargs)

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an options keyword argument that can take five different keys is the same as adding those keys to the method interface

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)

Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Expand Down Expand Up @@ -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)

Expand All @@ -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:
Expand All @@ -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:
Expand All @@ -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:
Expand All @@ -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]
Expand All @@ -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
Expand Down
71 changes: 69 additions & 2 deletions tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,27 @@ def test_algorithms_parameter_removes_alg_from_algorithms_list(self):
self.assertNotIn('none', self.jwt.get_algorithms())
self.assertIn('HS256', self.jwt.get_algorithms())

def test_default_options(self):
self.assertEqual(self.jwt.default_options, self.jwt.options)

def test_override_options(self):
self.jwt = PyJWT(options={'verify_exp': False, 'verify_nbf': False})
expected_options = self.jwt.default_options
expected_options['verify_exp'] = False
expected_options['verify_nbf'] = False
self.assertEqual(expected_options, self.jwt.options)

def test_non_default_options_persist(self):
self.jwt = PyJWT(options={'verify_iat': False, 'foobar': False})
expected_options = self.jwt.default_options
expected_options['verify_iat'] = False
expected_options['foobar'] = False
self.assertEqual(expected_options, self.jwt.options)

def test_options_must_be_dict(self):
self.assertRaises(TypeError, PyJWT, options=object())
self.assertRaises(TypeError, PyJWT, options=('something'))

def test_encode_decode(self):
secret = 'secret'
jwt_message = self.jwt.encode(self.payload, secret)
Expand Down Expand Up @@ -467,14 +488,14 @@ def test_decode_skip_expiration_verification(self):
secret = 'secret'
jwt_message = self.jwt.encode(self.payload, secret)

self.jwt.decode(jwt_message, secret, verify_expiration=False)
self.jwt.decode(jwt_message, secret, options={'verify_exp': False})

def test_decode_skip_notbefore_verification(self):
self.payload['nbf'] = time.time() + 10
secret = 'secret'
jwt_message = self.jwt.encode(self.payload, secret)

self.jwt.decode(jwt_message, secret, verify_expiration=False)
self.jwt.decode(jwt_message, secret, options={'verify_nbf': False})

def test_decode_with_expiration_with_leeway(self):
self.payload['exp'] = utc_timestamp() - 2
Expand Down Expand Up @@ -765,6 +786,52 @@ def test_raise_exception_token_without_issuer(self):
with self.assertRaises(InvalidIssuerError):
self.jwt.decode(token, 'secret', issuer=issuer)

def test_skip_check_audience(self):
payload = {
'some': 'payload',
'aud': 'urn:me',
}
token = self.jwt.encode(payload, 'secret')
self.jwt.decode(token, 'secret', options={'verify_aud': False})

def test_skip_check_exp(self):
payload = {
'some': 'payload',
'exp': datetime.utcnow() - timedelta(days=1)
}
token = self.jwt.encode(payload, 'secret')
self.jwt.decode(token, 'secret', options={'verify_exp': False})

def test_skip_check_signature(self):
token = ("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9"
".eyJzb21lIjoicGF5bG9hZCJ9"
".4twFt5NiznN84AWoo1d7KO1T_yoc0Z6XOpOVswacPZA")
self.jwt.decode(token, 'secret', options={'verify_signature': False})

def test_skip_check_iat(self):
payload = {
'some': 'payload',
'iat': datetime.utcnow() + timedelta(days=1)
}
token = self.jwt.encode(payload, 'secret')
self.jwt.decode(token, 'secret', options={'verify_iat': False})

def test_skip_check_nbf(self):
payload = {
'some': 'payload',
'nbf': datetime.utcnow() + timedelta(days=1)
}
token = self.jwt.encode(payload, 'secret')
self.jwt.decode(token, 'secret', options={'verify_nbf': False})

def test_decode_options_must_be_dict(self):
payload = {
'some': 'payload',
}
token = self.jwt.encode(payload, 'secret')
self.assertRaises(TypeError, self.jwt.decode, token, 'secret', options=object())
self.assertRaises(TypeError, self.jwt.decode, token, 'secret', options='something')

def test_custom_json_encoder(self):

class CustomJSONEncoder(json.JSONEncoder):
Expand Down