[Feature/Extensions] Principal identity for Opensearch requests to/from extensions #4299

DarshitChanpura · 2022-08-25T05:44:04Z

Description

Adds a basic identifiers to determine owner for each request from OpenSearch to extensions.

Issues Resolved

Resolves Add support for using User Identity in extensions opensearch-sdk-java#37

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

github-actions · 2022-08-25T07:46:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2105/
CommitID: b4c3b34705e4b3a0efaac7597f6d67d340460e28

github-actions · 2022-08-25T17:28:10Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: null
CommitID: d15b1a10f1839ae0e163c264c69160cec421bffe

github-actions · 2022-08-25T19:27:48Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2115/
CommitID: 1671cacfbb9e4610a6631c9380bfe80db8cafd8b

dbwiddis · 2022-08-25T23:58:35Z

Hey @DarshitChanpura thanks for this draft PR. Some quick feedback that should hopefully help you iterate:

Don't use the RegisterRestActionsRequest. That's inbound to OpenSearch from an extension that just registers the action on the RestController.
Take a look at [Feature/extensions] Register REST API and forward REST requests to associated extension #4282. In the RestActionsRequestHandler class we are registering the RestController to respond to a User's REST request by forwarding it to the extension. This forwarding process is where I think you'll need to insert identity information
- The RestSendToExtensionAction directly handles the RestRequest. You may have to back up and see where that Request comes from and how you can attach a principal to it. Then we could send it along to the extension with the method and URI, just add a third String parameter which is a key to an internal map where the user is the principal. (Eventually we'll actually want to send a JWT containing an Access Token.)

dbwiddis · 2022-08-26T00:21:18Z

Continuing on the above comment, the RestRequest does include the headers which would have the user's authorization info in it. So I'd suggest using that class as a basis (it's not merged yet but you can copy it and start with it for now) and:

parse the RestRequest headers to get the user info (also see [Prototype] Security (AuthN / AuthZ) with Shiro #4247 where it looks like @peternied has done this for basic auth) and turn that into a Principal
store the Principal in a map with some unique key. For now I think a random UUID would work.
send the key as a String along with the Method and URI in the RestExecuteOnExtensionRequest class.

Later the extension will send this key back with a different request or transport, and you can look up the Principal with it.

dbwiddis · 2022-08-26T00:24:37Z

As far as storing in the map, if you go with Apache Shiro, it looks like the RememberMeManager is probably built for this purpose. Not sure if the token it produces is an "access token" but worth looking into.

dbwiddis · 2022-08-26T02:38:37Z

Finally comment for today: where to put the map? I'd suggest a brand new object somewhere. Look in Node.java where a lot of individual objects are instantiated, such as the RestController and ExtensionsOrchestrator and others. Find a nice home in org.opensearch.identity and build a new object to your liking and instantiate it in Node and pass it around where it's needed.

peternied

Thanks for getting this out there and forwarding the discussion around our identities!

peternied · 2022-08-29T21:41:39Z

server/src/main/java/org/opensearch/identity/Principal.java

+    /* A non-unique human readable identifier */
+    String getUserName();


Should an extension be able to know a human readable identifier?

If we are being privacy focused - no seems like the answer. Get access to user data such as there name should involve a permission that must be requested.

If this Principal is be used as a cache key, then we wouldn't want fields that could change - would the username as well as the other fields be consistent or do they store mutable data? I like the notion from Shiro of a principal collection (mutable) being composed of immutable principals.

If we want to simplify calling patterns, less back/forth for often used data can offer big gains in roundtrip impact.

For me, a separate set of APIs for getting identity information limits the amount of work we need to do in the moment and removes assumptions from extensions implementations.

@dbwiddis What do you think about the role of this object, be more user property bag focused or identity token? What scenarios are more important for extensions writers?

I'm following SCIM definition. However this implementation is open to suggestions and changes

peternied · 2022-08-29T21:48:51Z

server/src/main/java/org/opensearch/identity/Identity.java

+
+    protected final static class AnonymousIdentity extends Identity {
+        // TODO: Determine risk of collision when generating random UUID
+        private final static UUID ID = new UUID(0L, 0L);


I think this should be an non-zero constant value, new UUID(0x817a6e, 0x817a6e) would be a quirky entry. Inspired by @dbwiddis comment #4028 (comment)

peternied · 2022-08-29T21:49:44Z

server/src/main/java/org/opensearch/identity/Principal.java

+    /* Contains principal type definition */
+    List<String> getSchemas();


Do you have an example of what kind of data would be in here?

peternied · 2022-08-29T21:50:52Z

server/src/main/java/org/opensearch/identity/Principal.java

+    String getUserName();
+
+    /* Contains metadata relevant to entity*/
+    Map<String, String> getMetaData();


What kind of data would we store in here? I would prefer strong types when possible to avoid blind gets/casts, but maybe we need it?

Refer this comment: #4028 (comment)

peternied · 2022-08-29T22:05:42Z

server/src/main/java/org/opensearch/extensions/RegisterRestActionsRequest.java

@@ -24,49 +26,57 @@
 */
 public class RegisterRestActionsRequest extends TransportRequest {


I think we would want similar treatment on any request from OpenSearch -> Extension interactions - should a there be a base class for all interactions, or a common wrapper? @saratvemulapalli have you had thoughts about this space?

Making this more concrete, lets consider the IndicesModuleRequest.

IndicesModuleRequest is translated by ExtensionsOrchestrator to send the request to the Extension (ExtensionRunner). When the call is send off to the extension, the identity of the operation should be included. Attaching this information onto of the Request object seems most straight forward, but is there another way that would better integrate it?

This would also mean that before transiting the OpenSearch -> Extension boundary there would be some kind of 'get current identity and associate it as a Principle with outbound request' component.

@peternied Absolutely, as we are working through the security features I think we should start implementing generic message handler which mandates including node identity, security identity etc.
May be lets start opening up an issue ?

peternied · 2022-08-29T22:09:06Z

server/src/main/java/org/opensearch/identity/Principal.java

+// TODO: Determine how this principal can be shared between threads and
+//  the effects of extending NamedWriteable class


I'm not certain the implications of using extending NamedWritable - is there a good reference or implementation that I could follow?

I'm not sure about that yet.. but you can search for NamedWritable in the codebase. It is for arbitrary seralizable objects. See interface definition here

dblock · 2022-08-30T13:53:57Z

I believe it should be "Username" and "Metadata" vs. "UserName" and "MetaData".
https://english.stackexchange.com/questions/43436/username-user-name-or-user-name

github-actions · 2022-08-31T03:04:51Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2325/
CommitID: 0a96e31955d54567d96f8ab8ddb4ffe725885671

github-actions · 2022-08-31T06:10:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2327/
CommitID: 6eaa76f0d466b86e3a611de08815fd6a0f28deb1

github-actions · 2022-08-31T07:01:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2330/
CommitID: c28cfd3

github-actions · 2022-08-31T07:12:25Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2331/
CommitID: f0d6e646aec21ab33ca5855a33e75d19669a2b7a

github-actions · 2022-08-31T07:30:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2332/
CommitID: abce649

github-actions · 2022-08-31T16:11:26Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2345/
CommitID: da6ac4aac36bb5e50895a08f5bbae39b61f847bb

github-actions · 2022-08-31T16:15:35Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2346/
CommitID: e86cc10

github-actions · 2022-08-31T16:34:45Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2352/
CommitID: 00e2683

github-actions · 2022-08-31T16:37:24Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2349/
CommitID: f0b5755933f71660370e14810e85dacf7c65b380

server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java

server/src/main/java/org/opensearch/identity/ExtensionPrincipalsRegistry.java

github-actions · 2022-09-01T14:58:33Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2434/
CommitID: dd41e8a

github-actions · 2022-09-02T14:00:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2546/
CommitID: f24bb48

github-actions · 2022-09-02T14:16:51Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2547/
CommitID: c3f8ef7

Signed-off-by: Peter Nied <[email protected]>

Signed-off-by: Darshit Chanpura <[email protected]>

… to correct registry Signed-off-by: Darshit Chanpura <[email protected]>

…s a few PR comments Signed-off-by: Darshit Chanpura <[email protected]>

…ate token correctly Signed-off-by: Darshit Chanpura <[email protected]>

github-actions · 2022-09-14T20:09:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3070/
CommitID: a92488c

DarshitChanpura · 2022-09-14T20:16:45Z

Pre-commit task failed with unable to resolve netty dependencies. Unable to reproduce it locally.

Update: Resolved on its own

github-actions · 2022-09-14T20:35:22Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3071/
CommitID: 4a4c533

Signed-off-by: Darshit Chanpura <[email protected]>

github-actions · 2022-09-15T15:05:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3103/
CommitID: a0083ea

Signed-off-by: Darshit Chanpura <[email protected]>

github-actions · 2022-09-15T20:36:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3117/
CommitID: dd3ce31

DarshitChanpura · 2022-09-15T20:37:58Z

Build failed with this:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':distribution:bwc:staged:buildBwcLinuxTar'.
> Building 2.3.0 didn't generate expected file /var/jenkins/workspace/gradle-check/search/distribution/bwc/staged/build/bwc/checkout-2.3/distribution/archives/linux-tar/build/distributions/opensearch-min-2.3.0-SNAPSHOT-linux-x64.tar.gz

Probably need to re-trigger the check

github-actions · 2022-09-15T21:06:03Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3121/
CommitID: dd3ce31

github-actions · 2022-09-19T21:01:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3221/
CommitID: 803c044

codecov-commenter · 2022-09-19T21:06:23Z

Codecov Report

Merging #4299 (803c044) into feature/extensions (edde6a4) will increase coverage by 0.14%.
The diff coverage is 73.46%.

@@                   Coverage Diff                    @@
##             feature/extensions    #4299      +/-   ##
========================================================
+ Coverage                 70.57%   70.72%   +0.14%     
- Complexity                57370    57527     +157     
========================================================
  Files                      4642     4644       +2     
  Lines                    276302   276347      +45     
  Branches                  40389    40394       +5     
========================================================
+ Hits                     195012   195446     +434     
+ Misses                    64936    64557     -379     
+ Partials                  16354    16344      -10

Impacted Files	Coverage Δ
...rch/extensions/rest/RestSendToExtensionAction.java	`27.63% <25.00%> (-0.15%)`	⬇️
...extensions/rest/RestExecuteOnExtensionRequest.java	`63.33% <45.45%> (-1.89%)`	⬇️
.../opensearch/identity/PrincipalIdentifierToken.java	`86.66% <86.66%> (ø)`
...g/opensearch/identity/ExtensionTokenProcessor.java	`88.88% <88.88%> (ø)`
...a/org/opensearch/common/network/NetworkModule.java	`92.10% <100.00%> (+0.10%)`	⬆️
...cluster/coordination/PendingClusterStateStats.java	`20.00% <0.00%> (-48.00%)`	⬇️
...ain/java/org/opensearch/search/sort/MinAndMax.java	`63.15% <0.00%> (-36.85%)`	⬇️
...tions/InternalSingleBucketAggregationTestCase.java	`59.74% <0.00%> (-28.58%)`	⬇️
.../java/org/opensearch/index/reindex/RemoteInfo.java	`50.45% <0.00%> (-28.45%)`	⬇️
...opensearch/index/reindex/BulkByScrollResponse.java	`48.38% <0.00%> (-28.23%)`	⬇️
... and 514 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

saratvemulapalli

Overall the changes look good to me. @DarshitChanpura thanks for this start.

Dropped a couple of questions but it shouldn't be blocking the merge.

saratvemulapalli · 2022-09-19T22:51:38Z

CHANGELOG.md

@@ -16,6 +16,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Update previous release bwc version to 2.4.0 ([#4455](https://github.com/opensearch-project/OpenSearch/pull/4455))
 - 2.3.0 release notes ([#4457](https://github.com/opensearch-project/OpenSearch/pull/4457))
 - Warn future developers not to pass headers to extensions ([#4430](https://github.com/opensearch-project/OpenSearch/pull/4430))
+- Principal Identity for Opensearch requests to/from extensions ([#4299](https://github.com/opensearch-project/OpenSearch/pull/4299))


How do you guys plan to get feature/extensions and feature/identity to be in sync?
Its going to be pain because you might need changes in feature/extensions in feature/identity but they might not work.

saratvemulapalli · 2022-09-19T22:58:17Z

server/src/main/java/org/opensearch/identity/PrincipalIdentifierToken.java

+ * Requester Token for requests to/from an extension
+ * Each user will have different token for different extension
+ *
+ * @opensearch.internal


opensearch.internal is used for everything internal to opensearch which is not exposed to plugins/extensions.
@dbwiddis @owaiskazi19 how do you want to name this? (cc: @nknize)

Not a blocker for this PR but I think we need to think about it.

How about we create a new javadoc flag and call it as opensearch.extensions?

As we refactor, I think we would want to call out for both plugins and extensions.

I'll create a follow up.

saratvemulapalli · 2022-09-19T23:05:09Z

server/src/main/java/org/opensearch/identity/ExtensionTokenProcessor.java

+ * Token processor class to handle token encryption/decryption
+ * This processor is will be instantiated for every extension
+ */
+public class ExtensionTokenProcessor {


This is very much tied to an extension, do you want to do something more generic which we can use for non-extensions?

Again, not a blocker for this PR. Trying to understand how we could make it better for rest of the use-cases.

DarshitChanpura force-pushed the principal-identity-extensions branch from 1671cac to d15b1a1 Compare August 25, 2022 17:27

DarshitChanpura changed the title ~~Principal identity for Opensearch requests to extensions~~ [Feature/Extensions] Principal identity for Opensearch requests to extensions Aug 26, 2022

peternied requested changes Aug 29, 2022

View reviewed changes

DarshitChanpura force-pushed the principal-identity-extensions branch from 6eaa76f to c28cfd3 Compare August 31, 2022 06:53

DarshitChanpura force-pushed the principal-identity-extensions branch from f0d6e64 to abce649 Compare August 31, 2022 07:22

DarshitChanpura force-pushed the principal-identity-extensions branch from da6ac4a to e86cc10 Compare August 31, 2022 15:56

DarshitChanpura force-pushed the principal-identity-extensions branch from f0b5755 to 00e2683 Compare August 31, 2022 16:18

cwperks mentioned this pull request Aug 31, 2022

WIP on a prototype of authorization for extension actions opensearch-project/opensearch-sdk-java#118

Closed

peternied requested changes Aug 31, 2022

View reviewed changes

DarshitChanpura force-pushed the principal-identity-extensions branch from dd41e8a to f24bb48 Compare September 2, 2022 13:46

peternied and others added 6 commits September 14, 2022 16:05

Changes in flight

3f9255f

Signed-off-by: Peter Nied <[email protected]>

Fix test caases

5569fd2

Signed-off-by: Peter Nied <[email protected]>

Fixes Spotless error

9f3aba2

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes unknown namedwriteable category error by addint the token class…

b57584f

… to correct registry Signed-off-by: Darshit Chanpura <[email protected]>

Fixes incorrect javadoc, adds a validity check for token and addresse…

2c0207c

…s a few PR comments Signed-off-by: Darshit Chanpura <[email protected]>

Adds test for extension token processor and modfifies a test to gener…

4a4c533

…ate token correctly Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the principal-identity-extensions branch from a92488c to 4a4c533 Compare September 14, 2022 20:05

cwperks mentioned this pull request Sep 14, 2022

[Feature/Identity] Permissions check API #4516

Merged

6 tasks

Adds opensearch internal annotation to class desciption

a0083ea

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura requested review from dbwiddis and saratvemulapalli and removed request for reta and dbwiddis September 15, 2022 15:06

Fixes dynamic array reference to a final variable before returning

dd3ce31

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura mentioned this pull request Sep 16, 2022

[Feature/Extensions] Adds Principal-Identity Token to extensions rest request opensearch-project/opensearch-sdk-java#140

Merged

Merge branch 'feature/extensions' into principal-identity-extensions

803c044

dbwiddis requested a review from owaiskazi19 September 19, 2022 22:42

dbwiddis approved these changes Sep 19, 2022

View reviewed changes

saratvemulapalli approved these changes Sep 19, 2022

View reviewed changes

saratvemulapalli merged commit 3a73a44 into opensearch-project:feature/extensions Sep 20, 2022

DarshitChanpura mentioned this pull request Jan 11, 2023

[META] Identity in OpenSearch #5834

Open

35 tasks

		/* A non-unique human readable identifier */
		String getUserName();

		/* Contains principal type definition */
		List<String> getSchemas();

		@@ -24,49 +26,57 @@
		*/
		public class RegisterRestActionsRequest extends TransportRequest {

		// TODO: Determine how this principal can be shared between threads and
		// the effects of extending NamedWriteable class

[Feature/Extensions] Principal identity for Opensearch requests to/from extensions #4299

[Feature/Extensions] Principal identity for Opensearch requests to/from extensions #4299

Conversation

DarshitChanpura commented Aug 25, 2022

Description

Issues Resolved

Check List

github-actions bot commented Aug 25, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 25, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 25, 2022

Gradle Check (Jenkins) Run Completed with:

dbwiddis commented Aug 25, 2022

dbwiddis commented Aug 26, 2022

dbwiddis commented Aug 26, 2022

dbwiddis commented Aug 26, 2022

peternied 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

saratvemulapalli Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

dblock commented Aug 30, 2022

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 31, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 1, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 2, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 2, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 14, 2022

Gradle Check (Jenkins) Run Completed with:

DarshitChanpura commented Sep 14, 2022 • edited Loading

github-actions bot commented Sep 14, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 15, 2022

Gradle Check (Jenkins) Run Completed with:

DarshitChanpura commented Sep 15, 2022

github-actions bot commented Sep 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 19, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Sep 19, 2022

Codecov Report

saratvemulapalli 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

saratvemulapalli Sep 7, 2022 •

edited

Loading

DarshitChanpura Aug 30, 2022 •

edited

Loading

DarshitChanpura commented Sep 14, 2022 •

edited

Loading