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

support RS384 and RS512 #44

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Conversation

motocodeltd
Copy link
Contributor

Problem Analysis (Technical)

Okta JWT only supports RS256 - this adds support for RS384 and RS512.

Solution (Technical)

Change Utils.isSupportedAlg to Utils.hashFunction that returns the relevant function or nil. Function is passed in to RSAPKCS1VerifierFactory for verification.

Affected Components

Steps to reproduce:

Try to validate an RS384 or a RS512 token.

Actual result:
Fails with unsupported algorithm

Expected result:

Tests

Didn't commit any RS384 or RS512 tokens but could do if requested

@oleggnidets-okta
Copy link
Contributor

@motocodeltd Many thanks for the PR!

It would be great to have some unit tests on that. Can you do that?

return true
default:
return false
open class func hashFunction(_ alg: String) -> SignatureAlgorithm.HashFunction? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has now been done

@IldarAbdullin-okta
Copy link
Contributor

@motocodeltd , thanks a lot for the contribution. Please sign CLA

@motocodeltd
Copy link
Contributor Author

@oleggnidets-okta couple of unit tests added

@IldarAbdullin-okta CLA emailed.

thanks for the code!

@arvindkrishnakumar-okta

@oleggnidets-okta couple of unit tests added

@IldarAbdullin-okta CLA emailed.

thanks for the code!

@motocodeltd We received the signed CLA. Thanks!

@oleggnidets-okta
Copy link
Contributor

@IldarAbdullin-okta @anastasiiaiurok-okta Please, approve or disapprove :)

Copy link
Contributor

@IldarAbdullin-okta IldarAbdullin-okta left a comment

Choose a reason for hiding this comment

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

LGTM!

@oleggnidets-okta oleggnidets-okta merged commit 6429323 into okta:master Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants