Implements token handlers and tests by stephen-crawford · Pull Request #2787 · opensearch-project/security

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

 		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

 				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


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



-        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

                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

    }
-
+
+    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

     * @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 DarshitChanpura, RyanL1997, cliu123, cwperks, davidlago, peternied 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


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

		HTTPJwtAuthenticator jwtAuth = new HTTPJwtAuthenticator(settings, null);



		saveAndUpdateConfigs(this.securityIndexName,client, CType.INTERNALUSERS, internalUsersConfiguration, new OnSucessActionListener<IndexResponse>(channel) {
		saveAndUpdateConfigs(this.securityIndexName,client, CType.INTERNALUSERS, internalUsersConfiguration, new OnSucessActionListener<>(channel) {

Conversation

stephen-crawford commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Check List

Uh oh!

RyanL1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternied left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephen-crawford commented Jun 22, 2023

Uh oh!

stephen-crawford commented Jul 26, 2023

Uh oh!

stephen-crawford commented Jul 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stephen-crawford commented May 23, 2023 •

edited

Loading

peternied left a comment •

edited

Loading