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

Use alg from JWT header if not specified on JWK #80

Merged
merged 12 commits into from
Oct 20, 2022
Merged

Use alg from JWT header if not specified on JWK #80

merged 12 commits into from
Oct 20, 2022

Conversation

hakanson
Copy link
Contributor

@hakanson hakanson commented Jul 22, 2022 •
edited by ottokruse
Loading

Issue #, if available: browser implementation regression of #6

Description of changes:

  • alg is optional per rfc7517, but required on RsaHashedImportParams used by SubtleCrypto.importKey. If the JWK does not have an alg we'll use the one from the JWT header instead. This is secure, as we only allow one of RS256, RS384 and RS512 anyway.
  • "Illegal invocation" error happening in SimplePenaltyBox.registerFailedAttempt in browser, likely caused by setTimeout binding - updated to window.setTimeout.bind(window)
  • added browser test case with example token and JWKS missing alg

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ottokruse ottokruse changed the title default browser importKey to RS256 Use alg from JWT header if not specified on JWK Oct 11, 2022
hakanson
hakanson commented Oct 18, 2022
View reviewed changes
Copy link
Contributor Author

@hakanson hakanson left a comment

Choose a reason for hiding this comment

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

Overall, looks good. have 2 comments

@ottokruse ottokruse merged commit 7887360 into awslabs:main Oct 20, 2022
@hakanson hakanson deleted the importkeyfix branch October 28, 2022 13:55
@hakanson hakanson mentioned this pull request Jan 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ottokruse ottokruse ottokruse approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@hakanson @ottokruse