-
Notifications
You must be signed in to change notification settings - Fork 274
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
Implements token handlers and tests #2787
Implements token handlers and tests #2787
Conversation
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 @scrawfor99, thank you for putting this together. I just left some general questions and also some notes related to the part I built for future reference.
|
||
@Test | ||
public void testIssueTokenWithValidAccount() throws IOException { | ||
when(userService.generateAuthToken("test")).thenReturn("Basic dGVzdDp0ZTpzdA=="); // test:te:st |
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.
Just for my understanding what is meaning of this comment? Is this like the ${username}:${password}?
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.
Oh nvm I think I saw that at the below lines. :D
|
||
String jwsToken = Jwts.builder().setSubject("Leonard McCoy").signWith(secretKey, SignatureAlgorithm.HS512).compact(); | ||
|
||
HTTPJwtAuthenticator jwtAuth = new HTTPJwtAuthenticator(settings, null); |
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.
This will be fixed on the next sync up with main branch. It should be change to the testing case of HTTPOnBehalfOfAuthenticator()
for testing.
@Test | ||
public void testBasicAuthHeader() throws Exception { | ||
Settings settings = Settings.builder().put("signing_key", BaseEncoding.base64().encode(secretKeyBytes)).build(); | ||
HTTPJwtAuthenticator jwtAuth = new HTTPJwtAuthenticator(settings, null); |
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.
Same as the above.
import org.junit.Assert; | ||
import org.junit.Test; | ||
|
||
import com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator; |
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.
This was a mistake. And it will be fixed at the next sync with main branch.
@@ -38,4 +38,17 @@ public int updateRoleMappings(File roleMappingsConfigurationFile) throws Excepti | |||
|
|||
return SecurityAdmin.execute(commandLineArguments); | |||
} | |||
|
|||
public int updateConfig(File configFile) throws Exception { |
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.
Just for my knowledge, why we need to modify the AdminLauncher?
@@ -38,7 +38,7 @@ protected boolean matchesSafely(GetSettingsResponse response, Description mismat | |||
if (!indexToSettings.containsKey(index)) { | |||
mismatchDescription | |||
.appendText("Response contains settings of indices: ") | |||
.appendValue(indexToSettings.keySet()); | |||
.appendValue(indexToSettings.keySet().toArray()); |
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.
qq, why this change?
@@ -1187,6 +1199,16 @@ private static String handleKeyword(final String field) { | |||
return field; | |||
} | |||
|
|||
@Override | |||
public Subject getSubject() { | |||
return null; |
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.
should we return some form of NoOp subject here instead of null?
@@ -167,7 +171,7 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C | |||
internalUsersConfiguration.putCObject(username, userData); | |||
|
|||
|
|||
saveAndUpdateConfigs(this.securityIndexName,client, CType.INTERNALUSERS, internalUsersConfiguration, new OnSucessActionListener<IndexResponse>(channel) { | |||
saveAndUpdateConfigs(this.securityIndexName,client, CType.INTERNALUSERS, internalUsersConfiguration, new OnSucessActionListener<>(channel) { |
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.
why remove "IndexResponse" here?
@@ -124,6 +124,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c | |||
if (!checkAndAuthenticateRequest(request, channel, client)) { | |||
User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); | |||
if (userIsSuperAdmin(user, adminDNs) || (whitelistingSettings.checkRequestIsAllowed(request, channel, client) && allowlistingSettings.checkRequestIsAllowed(request, channel, client))) { | |||
//TODO: If the request is going to the ext, issue a JWT for authenticated user. |
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.
if it helps, you can create a stub method like this:
if (original instanceOf RestSendToExtensionAction) {
stubbedMethodToIssueJwt();
}
src/main/java/org/opensearch/security/identity/SecurityTokenManager.java
Outdated
Show resolved
Hide resolved
@@ -319,5 +323,32 @@ public String toString() { | |||
|
|||
|
|||
} | |||
|
|||
|
|||
public static class OnBehalfOf { |
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.
Do we need the class to be public?
|
||
} | ||
|
||
public static class OnBehalfOf { |
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.
same as above, do we need the class to be public?
@@ -96,7 +99,7 @@ public UserService( | |||
* @param config CType whose data is to be loaded in-memory | |||
* @return configuration loaded with given CType data | |||
*/ | |||
protected static final SecurityDynamicConfiguration<?> load(final CType config, boolean logComplianceEvent) { | |||
protected final SecurityDynamicConfiguration<?> load(final CType config, boolean logComplianceEvent) { |
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.
why remove static
non-access modifier here?
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.
Are using the term service-account
and internal user
interchangeably?
System.out.println("IAT is : " + iat); | ||
System.out.println("EXP is : " + exp); | ||
System.out.println("Current time is : " + System.currentTimeMillis()); | ||
System.out.println("Exists in revoked is: " + revokedTokens.exists(bearerAuthToken.getCompleteToken())); |
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.
You can remove this sys outs and replace them with log.info if/as necessary.
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.
Doh! Wrong pull request
This is waiting for the merge to main from the feature branch. |
a3af495
to
cacb0bb
Compare
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com>
@davidlago should I make this mergable and go forward with this or a similar approach or is someone else taking care of these efforts? |
Closing this. Can be added to work @RyanL1997 is doing on the feature branch. |
Description
[Describe what this change achieves]
First introduces the feature branch into the main branch.
This PR introduces both internalUser and Security User (JWT) token handlers inside the security plugin. These handlers are managed by an overlying SecurityTokenManager class which sorts the token interaction requests coming from core.
These three managers work in concert to provide the required functionality of the Identity Plugin in core for Extensions to be live.
NOTE: This requires opensearch-project/OpenSearch#7452 to pass.
Testing
Adds three new test files with mocked tests for all methods.
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.