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

crash on absent key id during exception handling #209

Closed
Zil0-eo opened this issue Jun 3, 2021 · 3 comments · Fixed by #210
Closed

crash on absent key id during exception handling #209

Zil0-eo opened this issue Jun 3, 2021 · 3 comments · Fixed by #210

Comments

@Zil0-eo
Copy link

Zil0-eo commented Jun 3, 2021

This has been introduced by 0edf66d in 0.9:

--- a/jwcrypto/jwt.py
+++ b/jwcrypto/jwt.py
@@ -489,7 +489,7 @@ class JWT(object):
                         self.deserializelog.append("Success")
                         break
                     except Exception as e:  # pylint: disable=broad-except
-                        keyid = k.key_id
+                        keyid = k['kid']
                         if keyid is None:

The property key_id returned None if key had no key id, but this made 'kid' mandatory.

The bug happens whenever an exception is raised during deserialization with such key. For example, a call to JWT(jwt=jwt, key=key, algs=['RS256', 'ES256']) with an ES256 key will trigger it, as the first attempt to use key as a RS256 key raises an exception.

@simo5 simo5 added the bug label Jun 3, 2021
@simo5
Copy link
Member

simo5 commented Jun 3, 2021

Good catch!

@simo5
Copy link
Member

simo5 commented Jun 3, 2021

Do you have a reproducer?
I am trying to write a test, bu I cannot cause it to crash.

@simo5
Copy link
Member

simo5 commented Jun 3, 2021

Nevermind, got it.

simo5 added a commit to simo5/jwcrypto that referenced this issue Jun 3, 2021
This was causing the excpetion itself to crash on keys that do not have
a 'kid'.

Fixes latchset#209

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5 simo5 added the Priority label Jun 3, 2021
simo5 added a commit to simo5/jwcrypto that referenced this issue Jun 3, 2021
Direct use of k['kid'] was causing the excpetion itself to
crash on keys that do not have a 'kid' with a keyError.

Always use the .get accessor to pull 'kid' so that None is
returned when not availble.

Fixes latchset#209

Signed-off-by: Simo Sorce <simo@redhat.com>
simo5 added a commit to simo5/jwcrypto that referenced this issue Jun 4, 2021
This was causing the excpetion itself to crash on keys that do not have
a 'kid'.

Fixes latchset#209

Signed-off-by: Simo Sorce <simo@redhat.com>
simo5 added a commit to simo5/jwcrypto that referenced this issue Jun 7, 2021
Direct use of k['kid'] was causing the excpetion itself to
crash on keys that do not have a 'kid' with a keyError.

Always use the .get accessor to pull 'kid' so that None is
returned when not availble.

Fixes latchset#209

Signed-off-by: Simo Sorce <simo@redhat.com>
simo5 added a commit to simo5/jwcrypto that referenced this issue Jun 7, 2021
Direct use of k['kid'] was causing the excpetion itself to
crash on keys that do not have a 'kid' with a keyError.

Always use the .get accessor to pull 'kid' so that None is
returned when not availble.

Fixes latchset#209

Signed-off-by: Simo Sorce <simo@redhat.com>
simo5 added a commit to simo5/jwcrypto that referenced this issue Jun 7, 2021
Direct use of k['kid'] was causing the excpetion itself to
crash on keys that do not have a 'kid' with a keyError.

Always use the .get accessor to pull 'kid' so that None is
returned when not availble.

Fixes latchset#209

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5 simo5 closed this as completed in #210 Jun 9, 2021
simo5 added a commit that referenced this issue Jun 9, 2021
Direct use of k['kid'] was causing the excpetion itself to
crash on keys that do not have a 'kid' with a keyError.

Always use the .get accessor to pull 'kid' so that None is
returned when not availble.

Fixes #209

Signed-off-by: Simo Sorce <simo@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants