Adding end to end endpoint with various changes #1

peternied · 2023-06-12T17:13:18Z

Description

I've done a pass over the feature branch and made several updates, so it is more testable and cleaned up some smaller style issues that were nagging at me.

Testing

Testing of the new API was done via ./gradlew test --tests org.opensearch.security.dlic.rest.api.AccountApiTest.testGetAccount --info however, this was done to very quickly access the endpoint and should be properly moved/rearranged

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.

Signed-off-by: Peter Nied <petern@amazon.com>

… feature/extensions Signed-off-by: Peter Nied <petern@amazon.com>

Signed-off-by: Peter Nied <petern@amazon.com>

peternied · 2023-06-12T17:13:38Z

build.gradle

    runtimeOnly 'org.apache.ws.xmlschema:xmlschema-core:2.2.5'
    runtimeOnly 'org.apache.santuario:xmlsec:2.2.3'
-    runtimeOnly 'com.github.luben:zstd-jni:1.5.2-1'
+    runtimeOnly 'com.github.luben:zstd-jni:1.5.5-3'


Fixed the build break

peternied · 2023-06-12T17:16:36Z

src/test/java/org/opensearch/security/dlic/rest/api/AccountApiTest.java

        assertNotNull(body.getAsSettings("tenants"));
        assertNotNull(body.getAsList("roles"));
+
+        response = rh.executePostRequest(getEndpointPrefix() + "/api/user/onbehalfof", "{\"reason\":\"Test generation\"}", encodeBasicHeader(testUser, testPass));


This test only invokes the new endpoint and this should be deleted and replaced with changes in the integration tests that involve:

Happy path flow, get token then use to call WhoAmI endpoint

Basic error checking of the API

peternied · 2023-06-12T17:18:13Z

src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfToken.java

+
+import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
+
+public class CreateOnBehalfOfToken extends BaseRestHandler {


This class was hastily created, it should be re-written and compacted significately. I was also betting that we could use the AuthZ on REST requests so transport actions weren't generated. Might make sense to make this follow the transport actions more formally until that has been merged/backported

peternied · 2023-06-12T17:20:16Z

src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfToken.java

+                        user.getName(),
+                        source,
+                        tokenDuration,
+                        user.getSecurityRoles().stream().collect(Collectors.toList()));


Need more research into how these roles are populated, I'm not sure if this is correct

For extensions the token will contain the mapped roles

peternied · 2023-06-12T17:21:45Z

src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java

 import org.opensearch.security.user.AuthCredentials;

-public class HTTPOnBehalfOfJwtAuthenticator implements HTTPAuthenticator {
+public class OnBehalfOfAuthenticator implements HTTPAuthenticator {


Still has many opportunities to reuse the JwtVendor within this class rather than duplicate logic, but might be good fast followups rather than block the critical path

RyanL1997 · 2023-06-12T20:44:16Z

I will merge this one into my fork and fix this:

/home/runner/work/security/security/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthorizationHeaderFactory.java:49: error: incompatible types: LongSupplier cannot be converted to Optional<LongSupplier>
		JwtVendor jwtVendor = new JwtVendor(settings, currentTime);

cwperks · 2023-06-13T15:24:39Z

src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfToken.java

+                        .map(value -> Math.min(value, 72)) // Max duration is 72 hours
+                        .orElse(24); // Fallback to default;
+
+                    final String source = "self-issued";


If this is cluster name then this should be able to be obtained from the ClusterState which you can get in createComponents.

source is a bit confusing of a name here. This parameter is the audience claim in the JWT which is intended to be a unique identifier of the service/extension that would receive this token. For an endpoint to request the creation of a token, it may make sense to have this as a param.

In this REST API's case, the token was created by self-issued by user. If this was generated by a plugin or extension, then it should be marked with the identifier for traceability. This is good feedback around if we have the correct information in the token for the audit log or other use cases

cwperks · 2023-06-13T15:27:34Z

src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfToken.java

+                        .map(value -> (String)value)
+                        .map(Integer::parseInt)
+                        .map(value -> Math.min(value, 72)) // Max duration is 72 hours
+                        .orElse(24); // Fallback to default;


IMO 24 hours is too wide of a window. This value should match any currently configured REST Handler timeouts.

Good called, we can change the defaults as needed

cwperks · 2023-06-13T15:50:19Z

If we expand on this PR to also implement token revocation and richer management of these tokens then it may make sense to create a separate security index for token management. You can look at this PR which creates an index to store security information that is not supposed to be stored in the security configuration index.

RyanL1997 · 2023-06-13T16:57:09Z

If we expand on this PR to also implement token revocation and richer management of these tokens then it may make sense to create a separate security index for token management. You can look at this opensearch-project#2773 which creates an index to store security information that is not supposed to be stored in the security configuration index.

Hi @cwperks, thanks for the info. I would like ask some questions about the index (maybe called tokenManagementIndex?). I will leave some comments on the PR you linked.

Signed-off-by: Ryan Liang <jiallian@amazon.com>

peternied added 11 commits June 8, 2023 21:32

Fix build break

1d24b45

Signed-off-by: Peter Nied <petern@amazon.com>

Make authenticator name more concise

da94b82

Signed-off-by: Peter Nied <petern@amazon.com>

Clean up how roles are (de)encrypted

5913b57

Single constructor

437d315

Signed-off-by: Peter Nied <petern@amazon.com>

More JwtVendor cleanup - but not sure if this works?

4eda3db

Signed-off-by: Peter Nied <petern@amazon.com>

Merge remote-tracking branch 'ryan/add-oboauthcbackend-registry' into…

d4ab332

… feature/extensions Signed-off-by: Peter Nied <petern@amazon.com>

Misc cleanup

64732bd

Signed-off-by: Peter Nied <petern@amazon.com>

Remove bad import

861b588

Signed-off-by: Peter Nied <petern@amazon.com>

Add endpoint and test that interactions with JwtVendor

86b4fd9

Signed-off-by: Peter Nied <petern@amazon.com>

AlFix to do both decrypt and encrypt tokens

d15044b

Signed-off-by: Peter Nied <petern@amazon.com>

Add basic parameters and update test to use them

7f3e40e

Signed-off-by: Peter Nied <petern@amazon.com>

peternied requested a review from RyanL1997 as a code owner June 12, 2023 17:13

peternied commented Jun 12, 2023

View reviewed changes

RyanL1997 merged commit eb5552b into RyanL1997:add-oboauthcbackend-registry Jun 12, 2023

peternied deleted the feature/extensions branch June 12, 2023 20:47

cwperks reviewed Jun 13, 2023

View reviewed changes

RyanL1997 mentioned this pull request Jun 13, 2023

Store Scheduled Job User Information in an index owned by the Security plugin opensearch-project/security#2773

Closed

3 tasks

RyanL1997 added a commit that referenced this pull request Jun 22, 2023

Testing #1

e5c8ec5

Signed-off-by: Ryan Liang <jiallian@amazon.com>


		import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;

		public class CreateOnBehalfOfToken extends BaseRestHandler {

Adding end to end endpoint with various changes #1

Adding end to end endpoint with various changes #1

Uh oh!

Conversation

peternied commented Jun 12, 2023

Description

Testing

Check List

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!

RyanL1997 commented Jun 12, 2023

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!

cwperks commented Jun 13, 2023

Uh oh!

RyanL1997 commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants