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.

Sign up for GitHub

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

Jump to bottom

Fix crash in Keyset JWT decoding exception handler #210

Merged
merged 4 commits into from
Jun 9, 2021
Merged

Fix crash in Keyset JWT decoding exception handler #210

merged 4 commits into from
Jun 9, 2021

Conversation

simo5
Copy link
Member

@simo5 simo5 commented Jun 3, 2021

Fixes #209

@simo5
Copy link
Member Author

simo5 commented Jun 3, 2021

@Zil0-eo this should fix the issue

@Zil0-eo
Copy link

Zil0-eo commented Jun 3, 2021

It does, thanks :)

Note that there are two places where .key_id has been replaced by ['kid'], the second one is 0edf66d#diff-c30947f8244c914e06a5c138fd70c7a0759bb70b300c980d2744df399466cf01R1134. Maybe a check that 'kid' is in jwk should be added there too, just to be safe?

@simo5
Copy link
Member Author

simo5 commented Jun 3, 2021

Interestingly if I change this other occurrence to use get('kid') I get test failures in unexpected places ...

@simo5
Copy link
Member Author

simo5 commented Jun 3, 2021 •
edited

Welp ... this is a problem:

>>> k['kid']
'foo'
>>> k.get('kid')
>>>

@simo5
Copy link
Member Author

simo5 commented Jun 3, 2021

I pushed a patch that "fixes" the tests, but I am pretty sure @tiran will scold me for that :-D
I will discuss with Christian this area and find a decent solution tomorrow.

@simo5
Silence a test warning
cd018bd 
Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5 simo5 force-pushed the issue_209 branch 2 times, most recently from a26dd91 to 6290faa Compare June 7, 2021 20:25
simo5 added 3 commits June 7, 2021 18:43
@simo5
Remove the _params abstraction and simplify JWK
1c736e5 
Go one step further than previously done and actually remove the _params
dict and use the object as a real dict as intended.

This should fix multiple issues with the current implementation
including odd behaviors repr() and other functions that were not
overriden properly.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Fix crash in exception handler
9a57bc4 
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
Add test for Issue 209
39f31be 
Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Copy link
Member Author

simo5 commented Jun 8, 2021

Changed code to remove _param use completely, and therefore implementing .get() is now not necessary anymore and I removed that commit.

@simo5 simo5 added the Priority label Jun 8, 2021
@simo5 simo5 requested a review from tiran June 9, 2021 14:58
@simo5 simo5 merged commit 015cf17 into latchset:master Jun 9, 2021
@simo5 simo5 mentioned this pull request Jun 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiran tiran tiran approved these changes

Assignees
No one assigned
Labels
Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

crash on absent key id during exception handling
3 participants
@simo5 @Zil0-eo @tiran