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

⬆️ upgrade pyjwt to latest; introduce leeway to jwt.decode #2335

Merged

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Jul 21, 2023

Latest release (2.8) introduces support for python 3.10+

Release notes: https://github.com/jpadilla/pyjwt/releases

Note: jwt.decode can fail spuriously due to breaking changes introduced in jpadilla/pyjwt#797, where an exception is raised if the 'issued at' time of the JWT is in the future when decoding, with default 0 leeway. This means that a miniscule mismatch in clock synchronization, between generation and decoding of JWT, can now cause a failure when decoding. This arose in the integration tests, and is resolved with a leeway of 1 second.

@ff137
Copy link
Contributor Author

ff137 commented Jul 21, 2023

Some work required here, I can have a look after the weekend

@ff137 ff137 changed the title ⬆️ upgrade pyjwt to latest ⬆️ upgrade pyjwt Jul 24, 2023
@ff137
Copy link
Contributor Author

ff137 commented Jul 24, 2023

Test failures with pyjwt~=2.6.0:

Failing scenarios:
  features/0160-connection.feature:19  establish a connection between two agents -- @1.4 
  features/0160-connection.feature:20  establish a connection between two agents -- @1.5 
  features/0453-issue-credential.feature:40  Issue a json-ld credential with the Issuer beginning with an offer -- @1.4 
  features/0453-issue-credential.feature:58  Issue a credential with revocation, with the Issuer beginning with an offer, and then revoking the 

First scenario fails with:

Generate mediation invite ...
INFO:aiohttp.access:172.17.0.1 [24/Jul/2023:17:40:54 +0000] "POST /webhooks/topic/connections/ HTTP/1.1" 200 149 "-" "Python/3.9 aiohttp/3.8.5"
Accept mediation invite ...
Acme.agent | 2023-07-24 17:40:54,854 aries_cloudagent.admin.server ERROR Handler error with exception: Unauthorized
Acme.agent | Traceback (most recent call last):
Acme.agent |   File "/home/indy/aries_cloudagent/admin/server.py", line 397, in setup_context
Acme.agent |     profile = await self.multitenant_manager.get_profile_for_token(
Acme.agent |   File "/home/indy/aries_cloudagent/multitenant/base.py", line 363, in get_profile_for_token
Acme.agent |     token_body = jwt.decode(token, jwt_secret, algorithms=["HS256"])
Acme.agent |   File "/home/indy/.local/lib/python3.9/site-packages/jwt/api_jwt.py", line 168, in decode
Acme.agent |     decoded = self.decode_complete(
Acme.agent |   File "/home/indy/.local/lib/python3.9/site-packages/jwt/api_jwt.py", line 136, in decode_complete
Acme.agent |     self._validate_claims(
Acme.agent |   File "/home/indy/.local/lib/python3.9/site-packages/jwt/api_jwt.py", line 193, in _validate_claims
Acme.agent |     self._validate_iat(payload, now, leeway)
Acme.agent |   File "/home/indy/.local/lib/python3.9/site-packages/jwt/api_jwt.py", line 219, in _validate_iat
Acme.agent |     raise ImmatureSignatureError("The token is not yet valid (iat)")
Acme.agent | jwt.exceptions.ImmatureSignatureError: The token is not yet valid (iat)
Acme.agent | 
Acme.agent | During handling of the above exception, another exception occurred:
Acme.agent | 
Acme.agent | Traceback (most recent call last):
Acme.agent |   File "/home/indy/aries_cloudagent/admin/server.py", line 173, in ready_middleware
Acme.agent |     return await handler(request)
Acme.agent |   File "/home/indy/aries_cloudagent/admin/server.py", line 210, in debug_middleware
Acme.agent |     return await handler(request)
Acme.agent |   File "/home/indy/.local/lib/python3.9/site-packages/aiohttp_apispec/middlewares.py", line 45, in validation_middleware
Acme.agent |     return await handler(request)
Acme.agent |   File "/home/indy/aries_cloudagent/admin/server.py", line 379, in check_multitenant_authorization
Acme.agent |     return await handler(request)
Acme.agent |   File "/home/indy/aries_cloudagent/admin/server.py", line 413, in setup_context
Acme.agent |     raise web.HTTPUnauthorized()
Acme.agent | aiohttp.web_exceptions.HTTPUnauthorized: Unauthorized

@ff137
Copy link
Contributor Author

ff137 commented Jul 24, 2023

Breaking change is from: jpadilla/pyjwt#797

This introduces stricter validation for the issued at claim in JWT tokens. If the value is in the future, even just a millisecond, PyJWT will raise an ImmatureSignatureError.

ff137 added 4 commits July 24, 2023 21:07
This addresses breaking change in `pyjwt` version 2.6 (jpadilla/pyjwt#797), where validation will now raise an `ImmatureSignatureError` if the 'issued at' time is in the future.

Integration tests fail with `pyjwt~=2.6`, potentially because of clock synchronization / network latency / time zone differences in `issued_at` time of the jwt, so a leeway of 5 seconds attempts to accommodate ant potential latency / clock sync issue

Signed-off-by: ff137 <[email protected]>
This addresses breaking change in `pyjwt` version 2.6 (jpadilla/pyjwt#797), where validation will now raise an `ImmatureSignatureError` if the 'issued at' time is in the future, with default 0 leeway.

Integration tests fail with `pyjwt~=2.6`, potentially because of clock synchronization / network latency between `issued_at` time at generation and decoding of the jwt; so, a leeway of 1 second accommodates any potential latency / clock sync issue

Signed-off-by: ff137 <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ff137 ff137 changed the title ⬆️ upgrade pyjwt ⬆️ upgrade pyjwt to latest Jul 24, 2023
@ff137 ff137 marked this pull request as ready for review July 24, 2023 20:34
@ff137 ff137 changed the title ⬆️ upgrade pyjwt to latest ⬆️ upgrade pyjwt to latest; introduce leeway to jwt.decode Jul 24, 2023
@ff137
Copy link
Contributor Author

ff137 commented Jul 24, 2023

@swcurran Any chance to include with 0.9.0 release / #2344 ?

@dbluhm
Copy link
Contributor

dbluhm commented Jul 24, 2023

To understand the fix, we're permitting leeway of 1 second? 1 ms?

@swcurran
Copy link
Contributor

I think we need to get 0.9.0 out. We’ve got several hot items to follow quickly on the tail of 0.9.0 so we’ll put this there (and we can do an rc0 early if this is needed). But I think we need to get 0.9.0 ahead of this one.

Sorry about that. We’ll be talking about the next release at the ACA-Pug meeting tomorrow.

@swcurran swcurran requested a review from dbluhm July 24, 2023 20:49
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

@swcurran I think this would be a low risk addition but happy to defer to your decision (I was a split second behind on my approval lol).

@swcurran swcurran merged commit 81f6281 into openwallet-foundation:main Jul 24, 2023
@ff137
Copy link
Contributor Author

ff137 commented Jul 24, 2023

All good! It just resolves a python dependency conflict on a local test environment, but not a breaking one

And the 1s leeway could be shorter, but it's a reiterative process to halve until it breaks again - with builds taking 35 minutes, I figured 1s is good enough :-)
Can't think of any exploit for a 1s 'issued at' leeway - seems it's more a new feature of pyjwt to reject future issued_at times, which was missing before. So could now lead to problems with time zone differences, if anything, but that should be fixed in any case ...

edit: Thanks!

@swcurran
Copy link
Contributor

@dbluhm convinced me :-). Merged and will be in 0.9.0.

@ff137 ff137 deleted the chore/upgrade-pyjwt-dependency branch July 4, 2024 22:09
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