[Feature/Identity] Permissions check API #4516

cwperks · 2022-09-14T22:50:22Z

Description

Companion PR in opensearch-sdk-java that shows this in use: opensearch-project/opensearch-sdk-java#118

This PR introduces an AuthorizationRequest that is intended to be used to evaluate whether a user is permitted to perform an action. This is experimental and used for prototyping AuthN/AuthZ. The AuthorizationRequest is intended to encapsulate all parameters needed to perform authorization on any request in OpenSearch. Where authorization is performed is being determined and this PR proposes a simple interface that encapsulates the information needed for authorization.

The AuthenticationRequest is a minimal interface that contains information about the requester (PrincipalIdentifierToken introduced in: #4299), a permissionId and a map of parameters that can be checked to see if the request fits within a user's permitted actions and constraints. PermissionId is a new concept and will be expanded on in future PRs. PermissionId can be thought of as action name for this PR.

The AuthorizationResponse is also minimal and contains a message and an AuthorizationStatus of granted or denied.

I would welcome any feedback on the design of the interface to see if this is suitable for most requests.

Issues Resolved

opensearch-project/opensearch-sdk-java#40

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-09-14T23:09:21Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3075/
CommitID: 5c40870

github-actions · 2022-09-15T13:42:57Z

Gradle Check (Jenkins) Run Completed with:

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

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

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

peternied · 2022-09-15T21:30:53Z

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

+        this.extensionUniqueId = extensionUniqueId;
+        this.requestIssuerIdentity = requestIssuerIdentity;


These pieces of information have been added into the request body so the caller could alter they as they saw fit. What about changing the usage so this request is only allowed for the current Subject?

What's the current mechanism for sending PrincipalIdentifierToken from extension -> cluster? I added this in when creating my own extension to pass back the PrincipalIdentifierToken that was sent to the extension.

You're right though, this shouldn't be exposed to the caller so I will remove it.

This class encapsulates the information needed to perform authorization, but currently isn't used. I am thinking that most of the time authorization will happen at the point in time that an action is being performed and we will permit/deny the action just in time and may not use this construct. We will resolve to a subject based on a token embedded in the request/message as you pointed out here (opensearch-project/opensearch-sdk-java#40 (comment)).

In the extensions world, I think we will want to perform authorization in RestSendToExtensionAction.prepareRequest before the transport service sends the request to the extension node. Is there a central place we can move authorization to in core similar to how all requests go through the security plugin when security is installed today?

@peternied I removed the reference to the requestIssuerIdentity from this class.

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

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

peternied · 2022-09-15T21:38:43Z

@cwperks I moved this PR to target feature/identity where we have more flexiblity

cwperks · 2022-09-16T15:04:44Z

@cwperks I moved this PR to target feature/identity where we have more flexiblity

@peternied Its hard to see the changes this PR is introducing now. Would it be possible to update the feature/identity branch with the latest changes in feature/extensions?

…thorize any request Signed-off-by: Craig Perkins <[email protected]>

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2022-09-27T16:57:52Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3479/
CommitID: 46fb6f9

github-actions · 2022-09-27T17:00:09Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3480/
CommitID: 060fffd

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2022-09-27T18:04:10Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2022-09-27T18:10:59Z

Gradle Check (Jenkins) Run Completed with:

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

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2022-09-27T18:48:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3490/
CommitID: 7182afb

peternied

I'd like to work out the new package before merging, everything else is optional IMO

peternied · 2022-09-27T20:40:22Z

server/src/main/java/org/opensearch/authz/package-info.java

+ *
+ * @opensearch.experimental
+ */
+package org.opensearch.authz;


When should this package be used vs the identity package?

Never. You can't have authorization without identity. I didn't want to put these in the base level of the identity package, can I move this package into identity as a sub-package?

Yup. lets do it!

peternied · 2022-09-27T20:43:43Z

server/src/main/java/org/opensearch/identity/package-info.java

+/**
+ * Core classes responsible for handling identity in OpenSearch
+ *
+ * @opensearch.internal


peternied · 2022-09-27T20:57:44Z

server/src/main/java/org/opensearch/authz/AuthorizationRequest.java

+    private PrincipalIdentifierToken requestIssuerIdentity;
+    private String permissionId;
+    private Map<String, CheckableParameter> params;


It looks like with this design the resource id is contained within this map. This means that this API could be used for cluster level permission or specific resources. 👍

peternied · 2022-09-27T20:59:02Z

server/src/main/java/org/opensearch/authz/AuthorizationResponse.java

+ * @opensearch.experimental
+ */
+public class AuthorizationResponse extends TransportResponse {
+    private String response;


This object is a response, what about changing the name to message? I take this field is meant to represent a human readable message

peternied · 2022-09-27T21:00:33Z

server/src/main/java/org/opensearch/authz/CheckableParameter.java

+
+    @Override
+    public String toString() {
+        return "CheckableParameter{key=" + key + ", value=" + value + "}";


Should the typename also be included?

peternied · 2022-09-27T21:07:43Z

server/src/main/java/org/opensearch/authz/WriteableParameter.java

+    }
+
+    @SuppressWarnings("unchecked")
+    private CheckableParameter<?> createParameter(ParameterType type, String key, Object value) {


This doesn't make sure callers use the correct value with the correct ParameterType so they can get an ugly runtime exception. Instead what about could create a set of overloads like createParameter(String key, TimeValue value) and createParameter(String key, ByteSizeValue value) that create the background implementations.

I'm uncertain the worth going back and forth on this, but I think we might want to revisit after we've implemented more classes

peternied · 2022-09-27T21:08:59Z

server/src/main/java/org/opensearch/authz/CheckableParameter.java

+        String key = in.readString();
+        String className = in.readString();
+        Object value = in.readGenericValue();
+        return new CheckableParameter(key, value, Class.forName(className));


Potential performance issue https://stackoverflow.com/q/18231991/533057

Maybe just make a comment around this line and we can revisit, what do you think?

Sounds good, this class and WriteableParameter were inspired by similar work on extensions with settings here: https://github.com/opensearch-project/OpenSearch/blob/feature/extensions/server/src/main/java/org/opensearch/common/settings/WriteableSetting.java

The different between Setting and here is that setting can rely on the type of the default value when determining its data type.

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2022-09-27T22:20:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3502/
CommitID: 2bf5401

Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura · 2022-09-29T15:56:11Z

server/src/main/java/org/opensearch/identity/authz/CheckableParameter.java

+ * compatible open source license.
+ */
+
+/*


is this license correct?

Thanks for catching that.

DarshitChanpura

Looks good overall. Thanks Craig. One ask is to add more doc in CheckableParameter.java explaining why is this needed/how is this used?

DarshitChanpura · 2022-09-29T15:56:53Z

server/src/main/java/org/opensearch/identity/authz/CheckableParameter.java

+ *
+ * @opensearch.experimental
+ */
+public class CheckableParameter<T> {


More javadoc explaining how will this be used. Thoughts?

I expect to iterate on this idea a bit. The AuthorizationRequest is a class that encapsulates all of the information needed to perform authorization which includes the permissionId of the action being performed and any parameters needed to evaluate privileges.

When asking for permission such as: "Can I read documents on index X?", we need to know:

Who is making the request (current subject)

What action is being performed?

On what resource?

github-actions · 2022-09-29T16:05:13Z

Gradle Check (Jenkins) Run Completed with:

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

…heckableParameter Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura · 2022-09-29T16:06:31Z

server/src/main/java/org/opensearch/identity/authz/WriteableParameter.java

+                return new CheckableParameter<>(key, (TimeValue) value, TimeValue.class);
+            case ByteSizeValue:
+                return new CheckableParameter<>(key, (ByteSizeValue) value, ByteSizeValue.class);
+            default:


Are we planning on supporting custom user object types here? (i.e. have a case that supports a generic type)

No, I expect we will define a list of parameters that are checkable. We need to support index patterns to support existing index permissions. Other parameters that are needed to perform authorization could be resource identifiers.

github-actions · 2022-09-29T16:25:28Z

Gradle Check (Jenkins) Run Completed with:

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

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2022-09-29T16:58:45Z

Gradle Check (Jenkins) Run Completed with:

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

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2022-09-29T17:35:07Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3593/
CommitID: 686bd2a

cwperks · 2022-09-29T17:38:22Z

@peternied Any idea what's going on with the CI check here?

I see BUILD SUCCESSFUL in 22m 14s in jenkins, but this is saying UNSTABLE.

cwperks · 2022-09-29T17:41:40Z

I see 2 test failures:

Tests with failures:
 - org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests.testTaskResourceTrackingDuringTaskCancellation
 - org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testReplicaThreadedUpdateToShardLimitsAndRejections

Are these known flaky tests?

peternied · 2022-09-29T19:26:47Z

Any idea what's going on with the CI check here?

@cwperks Nope! But I'm not going to work about it, as they look supremely unrelated to what we are doing. I am going to merge without passing CI... and I'll fix it if this is actually busted instead of an intermittent failure

github-actions · 2022-09-29T19:56:57Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3599/
CommitID: 7bfc25d

cwperks requested review from a team and reta as code owners September 14, 2022 22:50

cwperks marked this pull request as draft September 14, 2022 22:50

cwperks mentioned this pull request Sep 14, 2022

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

Closed

DarshitChanpura reviewed Sep 15, 2022

View reviewed changes

server/src/main/java/org/opensearch/extensions/rest/RestActionsRequestHandler.java Outdated Show resolved Hide resolved

peternied reviewed Sep 15, 2022

View reviewed changes

peternied changed the base branch from feature/extensions to feature/identity September 15, 2022 21:38

Introduce AuthorizationRequest to encapsulate parameters needed to au…

46fb6f9

…thorize any request Signed-off-by: Craig Perkins <[email protected]>

cwperks force-pushed the extensions-authorize-request branch from edbfbc1 to 46fb6f9 Compare September 27, 2022 16:47

Update WriteableParameter and CheckableParameter to experimental

060fffd

Signed-off-by: Craig Perkins <[email protected]>

Add PrincipalIdentifierToken from PR 4299

cae5ac4

Signed-off-by: Craig Perkins <[email protected]>

cwperks changed the title ~~[Feature/Extensions] Permissions check API for extensions~~ [Feature/Identity] Permissions check API for extensions Sep 27, 2022

Merge branch 'feature/identity' into extensions-authorize-request

0bd0d99

cwperks marked this pull request as ready for review September 27, 2022 17:57

cwperks changed the title ~~[Feature/Identity] Permissions check API for extensions~~ [Feature/Identity] Permissions check API Sep 27, 2022

Apply spotless changes

7182afb

Signed-off-by: Craig Perkins <[email protected]>

peternied requested changes Sep 27, 2022

View reviewed changes

Move authz package into identity

2bf5401

Signed-off-by: Craig Perkins <[email protected]>

Remove requestIssuerIdentity from AuthorizationRequest

dbff561

Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura reviewed Sep 29, 2022

View reviewed changes

Correct license in AuthorizationRequest and add more to javadoc for C…

c288e54

…heckableParameter Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura reviewed Sep 29, 2022

View reviewed changes

Apply spotless checks

ebb452e

Signed-off-by: Craig Perkins <[email protected]>

a vs an

686bd2a

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'feature/identity' into extensions-authorize-request

7bfc25d

peternied merged commit 1b403cd into opensearch-project:feature/identity Sep 29, 2022

DarshitChanpura mentioned this pull request Oct 3, 2022

[FEATURE] Track User Access/Roles when creating a detector opensearch-project/opensearch-sdk-java#63

Closed

DarshitChanpura mentioned this pull request Jan 11, 2023

[META] Identity in OpenSearch #5834

Open

35 tasks

		this.extensionUniqueId = extensionUniqueId;
		this.requestIssuerIdentity = requestIssuerIdentity;

[Feature/Identity] Permissions check API #4516

[Feature/Identity] Permissions check API #4516

Conversation

cwperks commented Sep 14, 2022 • edited Loading

Description

Issues Resolved

Check List

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:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied commented Sep 15, 2022

cwperks commented Sep 16, 2022

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

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

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

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 29, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Sep 29, 2022

Gradle Check (Jenkins) Run Completed with:

cwperks commented Sep 29, 2022

cwperks commented Sep 29, 2022

peternied commented Sep 29, 2022 • edited Loading

github-actions bot commented Sep 29, 2022

Gradle Check (Jenkins) Run Completed with:

cwperks commented Sep 14, 2022 •

edited

Loading

peternied commented Sep 29, 2022 •

edited

Loading