security: verify JWToken signatures when a key is provided#4093
security: verify JWToken signatures when a key is provided#4093Ersa-tech wants to merge 7 commits into
Conversation
…ignature verification Signed-off-by: Security Researcher <security@example.com>
…rovided When a key is provided, the code only disabled verify_exp but left verify_aud and verify_nbf at their PyJWT defaults of True. This caused DecodeError for any JWT containing an aud claim because no audience parameter was passed to jwt.decode(). Fix by adding verify_aud=False and verify_nbf=False to the options when key is provided, matching the behavior of the no-key path which used verify_signature=False to implicitly disable all claim verifications. Also add tests for JWT tokens with audience and nbf claims. Signed-off-by: Security Researcher <security@example.com>
- Use PEP 604 type hints (str | None, list[str] | None) - Require algorithms when key is provided (raise ValueError otherwise) - Remove verify_exp=False from both decode paths (per maintainer: don't disable expiration validation) - Keep verify_signature=False only in backward-compat key=None path - Add docstring describing behavior and backward-compat path - Update tests for old init, key+algorithms, algorithms-only, and missing-algorithms error - Add mock_identity_provider_unsafe fixture for backward-compat testing
- Add mock_identity_provider_legacy() for backward-compat testing - Add tests for exp=-1 sentinel with both verified and legacy init - Add tests for audience claims with both init styles - Addresses maintainer feedback on PR redis#4085
- Modify entraid_utils.py to support mock_idp='legacy' - Add TestLegacyMockIdentityProvider with coverage for unverified JWToken - Addresses maintainer feedback on PR redis#4085
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
Hey @Ersa-tech, I'll take a look at this later this week. |
| options={"verify_signature": False}, | ||
| algorithms=algorithms, | ||
| ) | ||
| options = {"verify_aud": False, "verify_nbf": False} |
There was a problem hiding this comment.
I think you should add here also verify_iat: False.
With the current code a correctly signed token with a slightly future iat can now fail construction due to clock skew, even though the legacy path accepts it.
If we want verified mode to preserve the existing redis-py token semantics, I think this options dict should include verify_iat: False too, plus a regression test with a future iat.
| algorithms=algorithms, | ||
| ) | ||
| options = {"verify_aud": False, "verify_nbf": False} | ||
| if unverified_claims.get("exp") == -1: |
There was a problem hiding this comment.
Sorry, I think my earlier comment about removing verify_exp=False was off. I dug further into the surrounding JWToken logic and noticed that expiration is still validated later by redis-py via is_expired() / ttl().
Given that, can we avoid the double decode here and set verify_exp=False on the verified decode directly?
The current flow first decodes without signature verification only to inspect exp == -1, then decodes again with verification. Since redis-py already owns the expiration semantics, including the exp == -1 sentinel, it seems cleaner and cheaper to verify the signature once and let the existing token methods handle expiration afterward.
| token = { | ||
| "exp": datetime.now(timezone.utc).timestamp() + 100, | ||
| "iat": datetime.now(timezone.utc).timestamp(), | ||
| "nbf": datetime.now(timezone.utc).timestamp() - 10, |
There was a problem hiding this comment.
In order to validate you actually skip the nbf validation, you need to set the nbf with time in the future, not in the past.
2 participants
Summary
This is a replacement/update for #4085 from a branch I can push to with the current GitHub account.
It keeps the backward-compatible
JWToken(encoded)path, but adds an opt-in verified path viaJWToken(encoded, key=..., algorithms=...). When a key is provided, PyJWT verifies the signature with the caller-provided algorithms. When no key is provided, the existing unverified initialization path remains available for compatibility.Maintainer feedback addressed from #4085
algorithmswithout a key, preserving compatibility.ValueErrorwhen key is provided without algorithms.redis_entraidintegration test path that was not needed for the focused coverage.exp=-1never-expiring sentinel while keeping normal PyJWT expiration validation for non-sentinel verified tokens.Verification
python -m pytest tests\test_auth\test_jwt_signature.py tests\test_auth\test_token.py -qpython -m ruff check redis\auth\token.py tests\test_auth\test_jwt_signature.py tests\test_auth\test_token.py tests\conftest.py tests\entraid_utils.py tests\test_credentials.pygit diff --checkAll passed locally.
Note
Medium Risk
Changes authentication token handling and verification behavior; legacy unverified usage remains, so risk depends on callers adopting the verified API.
Overview
JWTokennow accepts optionalkeyandalgorithmsso callers can opt into PyJWT signature verification;JWToken(token)without a key still decodes without verifying the signature (unchanged compatibility).When verifying, the implementation requires
algorithmsifkeyis set (otherwiseValueError), disablesaud/nbfchecks, and preserves theexp=-1never-expire sentinel by skipping expiration verification for that case. Tests andmock_identity_provideruse the verified constructor where appropriate; newtest_jwt_signature.pycovers forged tokens, legacy paths, and claim edge cases.Reviewed by Cursor Bugbot for commit 895642c. Bugbot is set up for automated code reviews on this repo. Configure here.