Implements token handlers and tests #2787

stephen-crawford · 2023-05-23T15:50:41Z

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

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

RyanL1997

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.

RyanL1997 · 2023-05-25T19:04:03Z

src/test/java/org/opensearch/security/auth/InternalUserTokenHandlerTests.java

+
+    @Test
+    public void testIssueTokenWithValidAccount() throws IOException {
+        when(userService.generateAuthToken("test")).thenReturn("Basic dGVzdDp0ZTpzdA=="); // test:te:st


Just for my understanding what is meaning of this comment? Is this like the ${username}:${password}?

Oh nvm I think I saw that at the below lines. :D

RyanL1997 · 2023-05-25T19:11:15Z

src/test/java/org/opensearch/security/http/HTTPOnBehalfOfJwtAuthenticatorTest.java

+
+        String jwsToken = Jwts.builder().setSubject("Leonard McCoy").signWith(secretKey, SignatureAlgorithm.HS512).compact();
+
+        HTTPJwtAuthenticator jwtAuth = new HTTPJwtAuthenticator(settings, null);


This will be fixed on the next sync up with main branch. It should be change to the testing case of HTTPOnBehalfOfAuthenticator() for testing.

RyanL1997 · 2023-05-25T19:11:41Z

src/test/java/org/opensearch/security/http/HTTPOnBehalfOfJwtAuthenticatorTest.java

+    @Test
+    public void testBasicAuthHeader() throws Exception {
+        Settings settings = Settings.builder().put("signing_key", BaseEncoding.base64().encode(secretKeyBytes)).build();
+        HTTPJwtAuthenticator jwtAuth = new HTTPJwtAuthenticator(settings, null);


Same as the above.

RyanL1997 · 2023-05-25T19:13:26Z

src/test/java/org/opensearch/security/http/HTTPOnBehalfOfJwtAuthenticatorTest.java

+import org.junit.Assert;
+import org.junit.Test;
+
+import com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator;


This was a mistake. And it will be fixed at the next sync with main branch.

RyanL1997 · 2023-05-25T19:15:30Z

src/integrationTest/java/org/opensearch/security/SecurityAdminLauncher.java

@@ -38,4 +38,17 @@ public int updateRoleMappings(File roleMappingsConfigurationFile) throws Excepti

 		return SecurityAdmin.execute(commandLineArguments);
 	}
+
+	public int updateConfig(File configFile) throws Exception {


Just for my knowledge, why we need to modify the AdminLauncher?

DarshitChanpura · 2023-05-30T16:30:01Z

...st/java/org/opensearch/test/framework/matcher/GetSettingsResponseContainsIndicesMatcher.java

@@ -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());


qq, why this change?

DarshitChanpura · 2023-05-30T16:31:47Z

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

@@ -1187,6 +1199,16 @@ private static String handleKeyword(final String field) {
        return field;
    }

+    @Override
+    public Subject getSubject() {
+        return null;


should we return some form of NoOp subject here instead of null?

DarshitChanpura · 2023-05-30T16:40:56Z

src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java

@@ -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) {


why remove "IndexResponse" here?

DarshitChanpura · 2023-05-30T16:47:55Z

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

@@ -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.


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

DarshitChanpura · 2023-05-30T16:59:26Z

src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java

@@ -319,5 +323,32 @@ public String toString() {


    }
-
+
+    public static class OnBehalfOf {


Do we need the class to be public?

DarshitChanpura · 2023-05-30T18:26:20Z

src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java

+
+    }
+
+    public static class OnBehalfOf {


same as above, do we need the class to be public?

DarshitChanpura · 2023-05-30T18:31:13Z

src/main/java/org/opensearch/security/user/UserService.java

@@ -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) {


why remove static non-access modifier here?

DarshitChanpura · 2023-05-30T18:34:58Z

src/main/java/org/opensearch/security/user/InternalUserTokenHandler.java

Are using the term service-account and internal user interchangeably?

DarshitChanpura · 2023-05-30T18:36:01Z

src/main/java/org/opensearch/security/user/UserTokenHandler.java

+        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()));


You can remove this sys outs and replace them with log.info if/as necessary.

peternied

Doh! Wrong pull request

Accidentally requested changes on this PR

stephen-crawford · 2023-06-22T17:53:39Z

This is waiting for the merge to main from the feature branch.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com>

stephen-crawford · 2023-07-26T16:20:42Z

@davidlago should I make this mergable and go forward with this or a similar approach or is someone else taking care of these efforts?

stephen-crawford · 2023-07-26T17:13:38Z

Closing this. Can be added to work @RyanL1997 is doing on the feature branch.

stephen-crawford requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997 and reta as code owners May 23, 2023 15:50

stephen-crawford mentioned this pull request May 24, 2023

TokenManager Interface opensearch-project/OpenSearch#7452

Merged

6 tasks

RyanL1997 reviewed May 25, 2023

View reviewed changes

DarshitChanpura reviewed May 30, 2023

View reviewed changes

peternied previously requested changes Jun 5, 2023

View reviewed changes

Rebase

cacb0bb

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

stephen-crawford force-pushed the extensions branch from a3af495 to cacb0bb Compare July 5, 2023 14:38

stephen-crawford requested a review from willyborankin as a code owner July 5, 2023 14:38

stephen-crawford mentioned this pull request Jul 5, 2023

[FEATURE] Add token manager to security plugin #2939

Closed

Update Manager

72d2bc1

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

This was referenced Jul 7, 2023

#2616 bwcPlugin extension setting and getTokenManager implementation #2938

Closed

Implement on behalf of token passing for extensions opensearch-project/OpenSearch#8679

Merged

Merge branch 'main' into extensions

e1d4fb0

Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com>

davidlago assigned stephen-crawford Jul 26, 2023

stephen-crawford closed this Jul 26, 2023

RyanL1997 mentioned this pull request Jul 27, 2023

[Feature/Extension] Extension token handler #3034

Closed

3 tasks

stephen-crawford deleted the extensions branch September 14, 2023 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements token handlers and tests #2787

Implements token handlers and tests #2787

stephen-crawford commented May 23, 2023 •

edited

Loading

RyanL1997 left a comment

RyanL1997 May 25, 2023

RyanL1997 May 25, 2023

RyanL1997 May 25, 2023

RyanL1997 May 25, 2023

RyanL1997 May 25, 2023

RyanL1997 May 25, 2023

DarshitChanpura May 30, 2023

DarshitChanpura May 30, 2023

DarshitChanpura May 30, 2023

DarshitChanpura May 30, 2023

DarshitChanpura May 30, 2023

DarshitChanpura May 30, 2023

DarshitChanpura May 30, 2023

DarshitChanpura May 30, 2023

DarshitChanpura May 30, 2023

peternied left a comment •

edited

Loading

stephen-crawford commented Jun 22, 2023

stephen-crawford commented Jul 26, 2023

stephen-crawford commented Jul 26, 2023


		String jwsToken = Jwts.builder().setSubject("Leonard McCoy").signWith(secretKey, SignatureAlgorithm.HS512).compact();

		HTTPJwtAuthenticator jwtAuth = new HTTPJwtAuthenticator(settings, null);

		@@ -319,5 +323,32 @@ public String toString() {


		}


		public static class OnBehalfOf {

Implements token handlers and tests #2787

Implements token handlers and tests #2787

Conversation

stephen-crawford commented May 23, 2023 • edited Loading

Description

Testing

Check List

RyanL1997 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied left a comment • edited Loading

Choose a reason for hiding this comment

stephen-crawford commented Jun 22, 2023

stephen-crawford commented Jul 26, 2023

stephen-crawford commented Jul 26, 2023

stephen-crawford commented May 23, 2023 •

edited

Loading

peternied left a comment •

edited

Loading