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 support for the OIDC at_hash claim #296

Closed
wants to merge 2 commits into from

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Oct 2, 2017

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:

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

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage decreased (-0.2%) to 98.305% when pulling 3c08972 on sirosen:feature/at-hash into 72bb76c on jpadilla:master.

@sirosen
Copy link
Contributor Author

sirosen commented Oct 2, 2017

I amended my commit to fix a silly division error (/ vs //).

It looks to me like some Travis builds are failing for unrelated reasons (failure to find certain interpreter versions).

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage increased (+1.2%) to 99.718% when pulling b652c07 on sirosen:feature/at-hash into 72bb76c on jpadilla:master.

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage increased (+1.2%) to 99.718% when pulling d4bd7f2 on sirosen:feature/at-hash into 72bb76c on jpadilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 99.718% when pulling d4bd7f2 on sirosen:feature/at-hash into 72bb76c on jpadilla:master.

@sirosen
Copy link
Contributor Author

sirosen commented Oct 4, 2017

After discussing this change with a coworker, and we agreed that ideally tokens containing at_hash which are decoded with access_token=None would error.
However, that would be a breaking change for callers already using PyJWT on OIDC ID Tokens.

I'm going to add verify_at_hash as a config option defaulting to False, however, so that this looks more like other claims and can change to verify_at_hash: True whenever PyJWT 2.0 is being done.

@coveralls
Copy link

coveralls commented Oct 4, 2017

Coverage Status

Coverage increased (+1.2%) to 99.719% when pulling 0052f44 on sirosen:feature/at-hash into 72bb76c on jpadilla:master.

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
Copy link
Contributor Author

sirosen commented Oct 19, 2017

Rebased to trigger a fresh run after #301 merged. All tests should now be passing.

@coveralls
Copy link

coveralls commented Oct 19, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5709301 on sirosen:feature/at-hash into e1e4d02 on jpadilla:master.

@mark-adams
Copy link
Contributor

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 at_hash was a part of the JWT spec, this would be a no brainer. I don't want us to get in the habit of adding non-JWT things to the core library.

However, you are right on with your thinking that the at_hash value might not necessarily be perfectly at home in application business logic. It seems like this might be the kind of thing where it would make sense for PyJWT to have pluggable verifiers where someone could import PyJWT and then add verifier functions in an ad-hoc manner.

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.

@mark-adams mark-adams closed this Nov 25, 2017
@sirosen
Copy link
Contributor Author

sirosen commented Nov 27, 2017

I don't want us to get in the habit of adding non-JWT things to the core library.

Yeah, this seems like a really reasonable concern.
I'll open a follow-up issue which discusses the various things which a verifier needs to be able to do/have access to satisfy this use-case.

sirosen added a commit to sirosen/pyjwt that referenced this pull request Jun 29, 2022
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.
sirosen added a commit to sirosen/pyjwt that referenced this pull request Jun 29, 2022
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.
sirosen added a commit to sirosen/pyjwt that referenced this pull request Jun 29, 2022
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_`.
jpadilla pushed a commit that referenced this pull request Jul 3, 2022
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants