-
Notifications
You must be signed in to change notification settings - Fork 279
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
OnBehalfOf authenticator and token generator #3179
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3179 +/- ##
==========================================
Coverage 62.51% 62.51%
- Complexity 3353 3402 +49
==========================================
Files 254 259 +5
Lines 19732 20055 +323
Branches 3334 3370 +36
==========================================
+ Hits 12336 12538 +202
- Misses 5767 5866 +99
- Partials 1629 1651 +22
|
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.
Thank you @RyanL1997. I took a first pass at the review and left a few comments.
src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
9b57e1b
to
6cafc19
Compare
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.
Thank you for putting together this PR and adding this feature. Took a detailed look. One generic comment is to add javadoc to the methods that were introduced in this PR.
Also, I apologize if I missed some of these in your PRs to feature branch.
src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/OnBehalfOfConfig.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/OnBehalfOfConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/ssl/util/ExceptionUtils.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtilTest.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
c43a2e9
to
9ce36dc
Compare
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.
Looks really close. Just a few comments. Also there are some codecov comments that might need addressing
src/integrationTest/java/org/opensearch/test/framework/OnBehalfOfConfig.java
Outdated
Show resolved
Hide resolved
@DarshitChanpura, Yes, I have created an exception called |
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. Could you please look into Codecov warnings.
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.
Took another pass and left a few more comments. Thank you for adding tests that show the obo endpoint authorization!
src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/support/ConfigConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/user/AuthCredentials.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/http/OnBehalfOfAuthenticatorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/http/OnBehalfOfAuthenticatorTest.java
Outdated
Show resolved
Hide resolved
Thanks for putting this out @RyanL1997 - there might be debates such as around naming - I don't think we need perfect names, but lets try to be really solid on the customer facing interfaces and APIs. As long as we've got buy in from a couple folks lets try to iterate quickly. |
src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
Outdated
Show resolved
Hide resolved
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.
I left a few more comments, but this looks ready to me. All comments left were nits. Thank you for filing a follow up issue to abstract the logic to determine if the obo configuration is valid.
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Show resolved
Hide resolved
@RyanL1997 Looks like you've got a DCO failure, can you resolve that? |
@RyanL1997 Could you check over all the unresolved comments and make sure they are addressed and resolved? Once there are 0 unresolved comments (resolutions like won't fix, defer to an issue, or make a change), I'll be happy to approve. |
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.
Looks ready to me. Generic comment: There are multiple usages of Exception and RuntimeException being thrown. I would suggest using OpenSearchException in place of these.
None of these comments are blockers and can be followed up in a separate PR. Once all comments are resolved I'll approve the PR. Thank you @RyanL1997 for putting this feature together and for your patience working through the comments.
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/user/AuthCredentials.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
Outdated
Show resolved
Hide resolved
Hate to say, but I can't :(. First, according to the above log from DCO, instead of expecting my correct email, it expected my email to be |
@RyanL1997 IMO the fastest way to fix the DCO is like this, it will require a force-push to your branch, but its pretty straight forward
|
ed4c0ff
to
fa0199d
Compare
Signed-off-by: Ryan Liang <jiallian@amazon.com>
fa0199d
to
5e68d80
Compare
Thank you for the quick follow-up @RyanL1997 ! There's nothing left to address from me. Any follow-ups have already been captured in issues. |
src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ryan Liang <jiallian@amazon.com>
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. Thank you @RyanL1997 . All my outstanding comments have been addressed.
Description
Merge OBO Authentication into 'main' branch
New Feature
Major Components:
JwtVendor
: assemble the jwt based OBO token.OnBehalfOfAuthenticator
: the authentication backend for validate OBO TokenCreateOnBehalfOfTokenAction
: the endpoint of creation of OBO TokenMajor Testing Components:
JwtVendorTest
: Unit tests for JwtVendorOnBehalfOfAuthenticatorTest
: Unit tests for OBO authentication backendOnBehalfOfAuthenticationTest
: Integration test for OBO issuing endpointsIssues Resolved
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.