Repository navigation
fix: widen verify_broker_token except clause to jwt.PyJWTError - #4
Merged
Merged
Conversation
RSAAlgorithm.from_jwk raises jwt.exceptions.InvalidKeyError for a malformed or non-RSA JWKS entry (e.g. hit via _select_jwk's single-key fallback when the JWKS happens to contain exactly one key). InvalidKeyError is a PyJWTError but not an InvalidTokenError, so the previous except jwt.InvalidTokenError clause let it escape verify_broker_token uncaught, surfacing as an unhandled 500 with no audit log line instead of the usual audited 401. A broker JWKS key-rotation glitch or misconfiguration would silently produce unaudited 500s. Widen the except clause to jwt.PyJWTError, the common base class for both InvalidTokenError and InvalidKeyError, so any PyJWT-raised verification failure is classified as a normal, audited 401. Found and first fixed in servicex-token-service during code review of its near-identical identity.py; ported here since krb5-token-service shares the same pattern. Assisted-by: Claude (Anthropic)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
verify_broker_tokeninsrc/krb5_token_service/identity.pycatchesjwt.InvalidTokenError(andValueError/KeyError) around the JWKS-basedsignature verification, but
jwt.algorithms.RSAAlgorithm.from_jwk(key_data)(called just above, to build the public key from a JWKS entry) can raise
jwt.exceptions.InvalidKeyErrorfor a malformed or non-RSA JWKS entry --e.g. reachable via
_select_jwk's single-key fallback path whenever theJWKS happens to publish exactly one key and that key is malformed/non-RSA.
InvalidKeyErroris ajwt.PyJWTErrorsubclass but not a subclass ofInvalidTokenError, and isn'tValueError/KeyErroreither, so it escapedboth except clauses and propagated out of
verify_broker_tokenentirelyuncaught -- surfacing as a bare unhandled 500 with no audit log line at
all (the route handler's
except HTTPExceptionnever sees it). A brokerJWKS key-rotation glitch or misconfiguration (a non-RSA key appearing in
the published JWKS) would silently produce unaudited 500s instead of the
usual audited 401.
Fix
Widen the first
exceptclause fromjwt.InvalidTokenErrortojwt.PyJWTError-- the common base class for bothInvalidTokenErrorandInvalidKeyError-- so any PyJWT-raised verification failure is classifiedas a normal, audited 401.
This was found and first fixed in
servicex-token-serviceduring codereview while building that service (
identity.pyis shared, near-verbatim,across the voms/krb5/condor/servicex-token-service family). This PR ports
the same fix here since
krb5-token-service'sidentity.pyhas the exactsame bug.
Test plan
test_malformed_jwks_key_is_401intests/test_identity.py:builds a JWKS entry missing
n/e({"kid": "malformed-key", "kty": "RSA", "use": "sig"}), mints a token whosekidmatches it, and assertsverify_broker_tokenraisesHTTPExceptionwithstatus_code == 401.Reassigns
stub_jwks_fetch.keysto a new list rather than mutating inplace, since the underlying
jwksfixture is session-scoped.jwt.exceptions.InvalidKeyErrorinstead ofHTTPException).pixi run -e dev check(lint + format + mypy + full test suite):148 passed, 1 skipped, no regressions.
Generated with Claude Code