-
Notifications
You must be signed in to change notification settings - Fork 283
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
Switch JWT library implementations from cxf to nimbus #3421
Switch JWT library implementations from cxf to nimbus #3421
Conversation
Looking into an alternative library implementation for the JWT provider Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]>
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Outdated
Show resolved
Hide resolved
src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Maciej Mierzwa <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #3421 +/- ##
============================================
- Coverage 64.97% 64.90% -0.07%
+ Complexity 3638 3637 -1
============================================
Files 281 284 +3
Lines 20581 20618 +37
Branches 3398 3390 -8
============================================
+ Hits 13372 13383 +11
- Misses 5520 5553 +33
+ Partials 1689 1682 -7
|
src/main/java/com/amazon/dlic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/jwt/keybyoidc/JwtVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Maciej Mierzwa <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MaciejMierzwa, thanks for working on this. I know the pr is still in draft, and I just left some comment for now.
src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]> # Conflicts: # src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
919b8b0
to
0ee2de6
Compare
# Conflicts: # src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM except some comments I left.
I think we need use LocalDateTime
or DateTime
or TimeUnit
or ChronoUnit
. To calculate expiration time for tokens.
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/jwt/keybyoidc/JwtVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]>
…ding import Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
Everything has been fixed. |
src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]>
src/test/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticatorTest.java
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
|
Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
Show resolved
Hide resolved
src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ryan Liang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the answers, looks great @RyanL1997 & @MaciejMierzwa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
4f89b4a
into
opensearch-project:main
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3421-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4f89b4a782aee54a1c69460a30903b0d0d6b7565
# Push it to GitHub
git push --set-upstream origin backport/backport-3421-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x Then, create a pull request where the |
…ject#3421) Switch from org.apache.cxf.rs.security.jose to com.nimbusds.jose.jwk. Signed-off-by: Peter Nied <[email protected]> Signed-off-by: Maciej Mierzwa <[email protected]> Signed-off-by: Ryan Liang <[email protected]> Co-authored-by: Peter Nied <[email protected]> Co-authored-by: Ryan Liang <[email protected]>
Manual backport has been created: #3595 |
Description
Switch from org.apache.cxf.rs.security.jose to com.nimbusds.jose.jwk.
Issues Resolved
#3267
Is this a backport? If so, please add backport PR # and/or commits #
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.