[Feature/Identity] Introduce Identity module #5583

cwperks · 2022-12-15T16:51:54Z

Description

Opening a PR to solicit feedback for introduction of Identity module which includes an implementation of Basic Auth.

This PR introduces a new sandbox module identity that will use some of the existing extension points that the security plug-in does to authenticate rest requests and pass a token around on the header of the threadcontext of a task that identifies the user and can subsequently be used for authorization.

This new identity module uses a few existing extension points from the ActionPlugin and the NetworkPlugin.

From the ActionPlugin this branch uses:

getRestHandlerWrapper to provide a wrapper that handles authentication. As of now, there is only a Basic auth mechanism that uses the internal IdP in this feature branch to authenticate the user and return a 403 if the request cannot be authenticated
getActionFilters - This branch introduces an AuthorizationFilter that is intended to be used to perform authorization. This is mostly pass-through at the moment and right now it verifies that a token is present and valid before the TransportRequest performs its doExecute

From the NetworkPlugin this uses:

getTransportInterceptors - The transport interceptor intercepts outgoing TransportRequests and can modify the request before its sent to another node. When testing this branch, I ran into problems with how the TransportMessageListener intercepted outgoing requests as the ThreadContext was not available to inspect to ensure that the node that received the RestRequest created a token before sending the transport request to other nodes. When running the test its clear to see that other nodes received the created token, but the message listener is unable to get it because of how its wrapped in an ActionListener in OutboundHandler:

void sendRequest(...) throws IOException, TransportException {
        Version version = Version.min(this.version, channelVersion);
        OutboundMessage.Request message = new OutboundMessage.Request(
            threadPool.getThreadContext(),
            features,
            request,
            version,
            action,
            requestId,
            isHandshake,
            compressRequest
        );
        ActionListener<Void> listener = ActionListener.wrap(() -> messageListener.onRequestSent(node, requestId, action, request, options));
        sendMessage(channel, message, listener);
    }

The transport interceptor has access to the ThreadContext and the tests will be updated to use the interceptor.

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.

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

github-actions · 2022-12-15T17:38:57Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/8152/
CommitID: 49bac5d

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

github-actions · 2022-12-15T19:27:22Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.indices.stats.IndexStatsIT.testFilterCacheStats

URL: https://build.ci.opensearch.org/job/gradle-check/8156/
CommitID: f18332b
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

DarshitChanpura

Thank you for moving identity project to a module @cwperks !

DarshitChanpura · 2022-12-15T18:24:47Z

plugins/discovery-azure-classic/licenses/commons-lang-2.6.jar.sha1

@@ -0,0 +1 @@
+0ce1edb914c94ebc388f086c6827e8bdeec71ac2


qq.. are the changes in other plugin folders outside authn due to running updateShas?

Yes, I removed the server dependency on the :sandbox:libs:opensearch-authn library and ran updateSHAs after removing the dependency.

sandbox/libs/authn/build.gradle

DarshitChanpura · 2022-12-15T18:31:58Z

...ox/libs/authn/src/main/java/org/opensearch/authn/internal/InternalAuthenticationManager.java

@@ -32,7 +32,11 @@ public class InternalAuthenticationManager implements AuthenticationManager {
     * and this instantiation uses the default security manager
     */
    public InternalAuthenticationManager() {
-        final SecurityManager securityManager = new DefaultSecurityManager(InternalRealm.INSTANCE);
+        // final SecurityManager securityManager = new DefaultSecurityManager(InternalRealm.INSTANCE);
+        // SecurityUtils.setSecurityManager(securityManager);


we can remove this now

DarshitChanpura · 2022-12-15T18:34:14Z

sandbox/libs/authn/src/main/java/org/opensearch/authn/internal/InternalSubject.java

@@ -65,6 +66,7 @@ public String toString() {
    public void login(AuthenticationToken authenticationToken) {
        org.apache.shiro.authc.AuthenticationToken authToken = AuthenticationTokenHandler.extractShiroAuthToken(authenticationToken);
        // Login via shiro realm.
-        shiroSubject.login(authToken);
+        SecurityUtils.getSecurityManager().authenticate(authToken);
+        // shiroSubject.login(authToken);


should we remove shiroSubject completely? Since we are not using login method it won't creating and setting a subject on successful login, and so I'm not sure if we need this at all.

This section needs to be revisited for performance to see if the entire ./gradlew check can run with identity enabled. I can remove the commented out line for now.

sandbox/libs/authn/docs/auth_flow.md

...tity/src/internalClusterTest/java/org/opensearch/identity/HttpSmokeTestCaseWithIdentity.java

sandbox/modules/identity/src/main/java/org/opensearch/identity/PrivilegesEvaluatorResponse.java

DarshitChanpura · 2022-12-15T19:18:18Z

sandbox/modules/identity/src/main/java/org/opensearch/identity/SecurityFilter.java

+                log.debug(pres.toString());
+            }
+
+            if (pres.isAllowed()) {


this check would always be true based on setting on line 99. Is that expected or should line 99 be changed to something conditional?

That is more boilerplate that is meant to be expanded on when authorization is tackled. When I started working on this branch one of the big ideas was to deprecate the usage of serializing the User object and transporting around the cluster on the thread context headers as a proxy for a request that has previously been authenticated on the rest layer. Instead of serializing the user object, this PR introduces tokens for authentication information and transports them around the cluster using the thread context headers. I originally wanted to terminate an internal transport request if it did not contain authentication information with it. The problem I faced is that this header would be populated for internal transport requests that originated from the Rest Layer, but it was not present for internal actions invoked from elsewhere.

In a future PR we need to investigate where internal actions are invoked from and choose whether we should send information indicating that its starting from an internal source.

sandbox/modules/identity/src/main/java/org/opensearch/identity/SecurityRestFilter.java

server/src/main/java/org/opensearch/rest/RestController.java

github-actions · 2022-12-15T19:35:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/8157/
CommitID: 134bcb0

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

github-actions · 2022-12-15T21:59:18Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/8161/
CommitID: 32d8e75

codecov-commenter · 2022-12-15T22:01:30Z

Codecov Report

Merging #5583 (ad2f483) into feature/identity (71281f1) will decrease coverage by 0.55%.
The diff coverage is 63.04%.

@@                  Coverage Diff                   @@
##             feature/identity    #5583      +/-   ##
======================================================
- Coverage               71.51%   70.95%   -0.56%     
+ Complexity              58645    58494     -151     
======================================================
  Files                    4727     4768      +41     
  Lines                  277738   278941    +1203     
  Branches                40176    40297     +121     
======================================================
- Hits                   198612   197911     -701     
- Misses                  63222    64871    +1649     
- Partials                15904    16159     +255

Impacted Files	Coverage Δ
...search/transport/Netty4NioServerSocketChannel.java	`0.00% <0.00%> (ø)`
.../java/org/opensearch/transport/NettyAllocator.java	`45.45% <0.00%> (ø)`
...rch/authn/internal/InternalAccessTokenManager.java	`0.00% <0.00%> (ø)`
.../opensearch/authn/noop/NoopAccessTokenManager.java	`0.00% <0.00%> (ø)`
...ensearch/authn/noop/NoopAuthenticationManager.java	`66.66% <ø> (ø)`
...in/java/org/opensearch/authn/noop/NoopSubject.java	`66.66% <ø> (ø)`
.../java/org/opensearch/authn/tokens/AccessToken.java	`0.00% <ø> (ø)`
.../java/org/opensearch/identity/ConfigConstants.java	`0.00% <0.00%> (ø)`
...rc/main/java/org/opensearch/identity/Identity.java	`100.00% <ø> (ø)`
...rg/opensearch/identity/ThreadContextConstants.java	`0.00% <0.00%> (ø)`
... and 557 more

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

github-actions · 2022-12-15T22:03:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/8162/
CommitID: 2c93d03

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

github-actions · 2022-12-15T22:38:54Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/8167/
CommitID: 2665e11

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

github-actions · 2022-12-19T15:46:33Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/8257/
CommitID: b39cd11
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

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

github-actions · 2022-12-19T17:09:12Z

Gradle Check (Jenkins) Run Completed with:

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

DarshitChanpura

Thank you @cwperks !

DarshitChanpura · 2022-12-19T17:15:08Z

@saratvemulapalli Can you please help merge this?

saratvemulapalli

Didnt want to block you work.
But i'd like to review the change sometime this week.
@cwperks I'll merge it and i'll drop comments if I have any.

Introduce Identity module

9048d05

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

cwperks requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners December 15, 2022 16:51

This was referenced Dec 15, 2022

[Feature/Identity] Identity Module and tokens for internal authentication #5471

Closed

[Feature/Identity] Merge remote-tracking branch 'origin/main' into feature/identity #5581

Merged

cwperks removed request for dblock, nknize, mch2, adnapibar, shwetathareja and VachaShah December 15, 2022 17:05

cwperks removed request for setiah, Rishikesh1159, CEHENKLE, tlfeng, dreamer-89, kartg and xuezhou25 December 15, 2022 17:05

Update CHANGELOG

49bac5d

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

cwperks added 2 commits December 15, 2022 13:56

Add auth_flow.md

f18332b

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

Remove changes in security.policy

134bcb0

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

DarshitChanpura reviewed Dec 15, 2022

View reviewed changes

cwperks added 2 commits December 15, 2022 16:29

Address review comments

32d8e75

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

Add new line

2c93d03

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

Remove commented out code

2665e11

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

Make log statements debug level

b39cd11

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

Add Locale.ROOT

ad2f483

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

DarshitChanpura approved these changes Dec 19, 2022

View reviewed changes

saratvemulapalli approved these changes Dec 20, 2022

View reviewed changes

saratvemulapalli merged commit ec7f9b6 into opensearch-project:feature/identity Dec 20, 2022

DarshitChanpura mentioned this pull request Jan 11, 2023

[META] Identity in OpenSearch #5834

Open

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/Identity] Introduce Identity module #5583

[Feature/Identity] Introduce Identity module #5583

cwperks commented Dec 15, 2022 •

edited

Loading

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

DarshitChanpura left a comment

DarshitChanpura Dec 15, 2022

cwperks Dec 15, 2022

DarshitChanpura Dec 15, 2022

DarshitChanpura Dec 15, 2022

cwperks Dec 15, 2022

DarshitChanpura Dec 15, 2022

cwperks Dec 15, 2022 •

edited

Loading

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

codecov-commenter commented Dec 15, 2022 •

edited

Loading

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

DarshitChanpura left a comment

DarshitChanpura commented Dec 19, 2022

saratvemulapalli left a comment

		@@ -0,0 +1 @@
		0ce1edb914c94ebc388f086c6827e8bdeec71ac2

[Feature/Identity] Introduce Identity module #5583

[Feature/Identity] Introduce Identity module #5583

Conversation

cwperks commented Dec 15, 2022 • edited Loading

Description

Check List

github-actions bot commented Dec 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 15, 2022

Gradle Check (Jenkins) Run Completed with:

DarshitChanpura left a comment

Choose a reason for hiding this comment

DarshitChanpura Dec 15, 2022

Choose a reason for hiding this comment

cwperks Dec 15, 2022

Choose a reason for hiding this comment

DarshitChanpura Dec 15, 2022

Choose a reason for hiding this comment

DarshitChanpura Dec 15, 2022

Choose a reason for hiding this comment

cwperks Dec 15, 2022

Choose a reason for hiding this comment

DarshitChanpura Dec 15, 2022

Choose a reason for hiding this comment

cwperks Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 15, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Dec 15, 2022 • edited Loading

Codecov Report

github-actions bot commented Dec 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 19, 2022

Gradle Check (Jenkins) Run Completed with:

DarshitChanpura left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Dec 19, 2022

saratvemulapalli left a comment

Choose a reason for hiding this comment

cwperks commented Dec 15, 2022 •

edited

Loading

cwperks Dec 15, 2022 •

edited

Loading

codecov-commenter commented Dec 15, 2022 •

edited

Loading