Add support for the OIDC at_hash claim #296
Conversation
sirosen
force-pushed
the
feature/at-hash
branch
from
b652c07
to
d4bd7f2
Compare
|
I amended my commit to fix a silly division error ( It looks to me like some Travis builds are failing for unrelated reasons (failure to find certain interpreter versions). |
1 similar comment
|
After discussing this change with a coworker, and we agreed that ideally tokens containing I'm going to add |
Use PyJWT to compute the at_hash value for OpenID Connect: http://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken This makes more sense in PyJWT than its client code because of the tight coupling between the chosen signing algorithm and the computation of the at_hash. Any client code would have to jump through hoops to get this to work nicely based on the algorithm being fed to PyJWT. Closes jpadilla#295 Primary changes: Add support for access_token=... as a param to PyJWT.encode and PyJWT.decode . On encode, the at_hash claim is computed and added to the payload. On decode, unpacks the at_hash value, raising a missing claim error if its missing, and compares it to a freshly computed at_hash. Raises a new error type if they don't match. Does not use the verification options dict, as it's redundant with the caller supplying access_token in this case. Supporting changes: - Add tests for the above - Let PyJWT and PyJWS get an algorithm object from a string as a method - Add a method, compute_at_hash, to PyJWT objects - PyJWT._validate_claims now takes the header as an arg (needed to get algo)
To make it easier to enforce verification of at_hash in future major
versions of PyJWT, add the verify_at_hash option to the implementation,
defaulting to False. It's now necessary for callers to set
`options={'verify_at_hash': True}` in addition to `access_token=...`,
but it keeps backwards compatibility for users who are acting on OIDC ID
Tokens with current versions of PyJWT.
Whenever PyJWT 2.0 is created, this can be changed to default to True
sirosen
force-pushed
the
feature/at-hash
branch
from
0052f44
to
5709301
Compare
|
Rebased to trigger a fresh run after #301 merged. All tests should now be passing. |
|
Thanks for the submission! Hmm... I'm a little concerned about adding extraneous functionality to the core library like this. OpenID Connect is it's own thing with it's own spec that happens to use JWT. If it were the other way around and OpenID Connect's However, you are right on with your thinking that the Either way though, I don't feel like this is probably the way we should go about this. If you'd like to push this forward in PyJWT, I suggest filing an issue where we can discuss what pluggable verifiers might look like. |
Yeah, this seems like a really reasonable concern. |
Looking up an algorithm by name is used internally for signature generation. This encapsulates that functionality in a dedicated method and adds it to the public API. No new tests are needed to exercise the functionality. Rationale: 1. Inside of PyJWS, this improves the code. The KeyError handler is better scoped and the signing code reads more directly. 2. This is part of the path to supporting OIDC at_hash validation as a use-case (see: jpadilla#295, jpadilla#296, jpadilla#314). This is arguably sufficient to consider that use-case supported and close it. However, it is an improvement and step in the right direction in either case.
Looking up an algorithm by name is used internally for signature generation. This encapsulates that functionality in a dedicated method and adds it to the public API. No new tests are needed to exercise the functionality. Rationale: 1. Inside of PyJWS, this improves the code. The KeyError handler is better scoped and the signing code reads more directly. 2. This is part of the path to supporting OIDC at_hash validation as a use-case (see: jpadilla#295, jpadilla#296, jpadilla#314). This is arguably sufficient to consider that use-case supported and close it. However, it is an improvement and step in the right direction in either case.
Looking up an algorithm by name is used internally for signature generation. This encapsulates that functionality in a dedicated method and adds it to the public API. No new tests are needed to exercise the functionality. Rationale: 1. Inside of PyJWS, this improves the code. The KeyError handler is better scoped and the signing code reads more directly. 2. This is part of the path to supporting OIDC at_hash validation as a use-case (see: jpadilla#295, jpadilla#296, jpadilla#314). This is arguably sufficient to consider that use-case supported and close it. However, it is an improvement and step in the right direction in either case. A minor change was needed to satisfy mypy, as a union-typed variable does not narrow its type based on assignments. The easiest resolution is to use a new name, in this case, simply `algorithm -> algorithm_`.
* Expose get_algorithm_by_name as new method Looking up an algorithm by name is used internally for signature generation. This encapsulates that functionality in a dedicated method and adds it to the public API. No new tests are needed to exercise the functionality. Rationale: 1. Inside of PyJWS, this improves the code. The KeyError handler is better scoped and the signing code reads more directly. 2. This is part of the path to supporting OIDC at_hash validation as a use-case (see: #295, #296, #314). This is arguably sufficient to consider that use-case supported and close it. However, it is an improvement and step in the right direction in either case. A minor change was needed to satisfy mypy, as a union-typed variable does not narrow its type based on assignments. The easiest resolution is to use a new name, in this case, simply `algorithm -> algorithm_`. * Use get_algorithm_by_name in _verify_signature Rather than catching the KeyError from a dict lookup, catch the NotImplementedError raised by get_algorithm_by_name. This changes the exception seen in the cause under exception chaining but otherwise has no public-facing impact.
Use PyJWT to compute the at_hash value for OpenID Connect:
http://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken
This makes more sense in PyJWT than its client code because of the tight coupling between the chosen signing algorithm and the computation of the at_hash. Any client code would have to jump through hoops to get this to work nicely based on the algorithm being fed to PyJWT.
Closes #295
Primary changes:
Add support for access_token=... as a param to PyJWT.encode and PyJWT.decode . On encode, the at_hash claim is computed and added to the payload. On decode, unpacks the at_hash value, raising a missing claim error if its missing, and compares it to a freshly computed at_hash. Raises a new error type if they don't match. Does not use the verification options dict, as it's redundant with the caller supplying access_token in this case.
Supporting changes: