[Feature/Extension] Extension token handler by RyanL1997 · Pull Request #3034 · opensearch-project/security

RyanL1997 · 2023-07-19T16:36:54Z

Description

This PR is the refactor/modification based on @scrawfor99's previous PR(#2787) against main branch for Extension token handler. However, the scope of this PR is only for OBO related changes.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
New Feature

Issues Resolved

Pre-requisite Implement on behalf of token passing for extensions OpenSearch#8679
Relate [META] On-Behalf-Of Authentication #2573
Resolve Determine how auth tokens are forwarded to Extensions #2764

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.

codecov · 2023-07-19T22:12:37Z

Codecov Report

Merging #3034 (8e1f399) into feature/extensions (058f8ec) will decrease coverage by 56.92%.
The diff coverage is 0.00%.

❗ Current head 8e1f399 differs from pull request most recent head 03a7875. Consider uploading reports for the commit 03a7875 to get more accurate results

@@                   Coverage Diff                    @@
##             feature/extensions   #3034       +/-   ##
========================================================
- Coverage                 62.38%   5.47%   -56.92%     
+ Complexity                 3390     222     -3168     
========================================================
  Files                       259     275       +16     
  Lines                     20044   20198      +154     
  Branches                   3371    3392       +21     
========================================================
- Hits                      12505    1105    -11400     
- Misses                     5888   18913    +13025     
+ Partials                   1651     180     -1471

Files Changed	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`10.87% <0.00%> (-73.55%)`	⬇️
...y/action/onbehalf/CreateOnBehalfOfTokenAction.java	`0.00% <ø> (-28.82%)`	⬇️
...security/dlic/rest/api/InternalUsersApiAction.java	`0.00% <0.00%> (-64.08%)`	⬇️
...g/opensearch/security/dlic/rest/support/Utils.java	`0.00% <0.00%> (-54.66%)`	⬇️
...search/security/identity/SecurityTokenManager.java	`0.00% <0.00%> (ø)`
...g/opensearch/security/securityconf/impl/CType.java	`0.00% <0.00%> (-100.00%)`	⬇️
...search/security/user/InternalUserTokenHandler.java	`0.00% <0.00%> (ø)`
...java/org/opensearch/security/user/UserService.java	`0.00% <0.00%> (-50.87%)`	⬇️
...org/opensearch/security/user/UserTokenHandler.java	`0.00% <0.00%> (ø)`

... and 230 files with indirect coverage changes

RyanL1997 · 2023-07-27T23:08:15Z

Moving some of the communication between me and @scrawfor99 over here

The scope of this PR should only focus on the interface of issueOnBehalfOfToken(), reference to here.
For the changes that got carried over from Implements token handlers and tests #2787, if the team thinks it is not in the scope of this PR, it is ok to remove them. I'm leaving them here for now cuz I think it is easier to continue the service account work from here.

RyanL1997

Adding some additional context here:

RyanL1997 · 2023-07-27T23:15:07Z

src/main/java/org/opensearch/security/user/UserTokenHandler.java

+        return (exp > currentTime && !revokedTokens.exists(bearerAuthToken.getCompleteToken()));
+    }
+
+    public boolean validateJustInTimeToken(AuthToken authToken) {


The reason I make this validation is that I think we do have different expectations for obo tokens than the other bearer auth tokens. For obo tokens, according to its feature of just-in-time, we do expect the payload of iat and nbf have the same value.

RyanL1997 · 2023-07-27T23:20:04Z

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

        JwtClaims jwtClaims = new JwtClaims();
        JwtToken jwt = new JwtToken(jwtClaims);

+        jwtClaims.setProperty("typ", tokenIdentifier);


This is the idea that can be also applied for the potential approach of handling the edge cases of OBO endpoint (see comments). By having a token type as the identifier, it will be easier for us to know if it is a obo token or not so that we can have different expectations.

If we decide to go with this design, I will also update the threat model documentation

RyanL1997 · 2023-07-27T23:25:09Z

src/main/java/org/opensearch/security/user/UserTokenHandler.java

+        Long iat = (Long) jwt.getClaim("iat");
+        Long nbf = (Long) jwt.getClaim("nbf");
+        Long exp = (Long) jwt.getClaim("exp");
+        SecurityDynamicConfiguration revokedTokens = load(getRevokedTokensConfigName(), false);


If we choose to split the validations between obo tokens and other bearer auth tokens in this way, then I don't think we need this line here, since the revocation is not in the scope of obo token. (At least for now)

cwperks

@RyanL1997 There looks to be a lot more on this PR than an implementation of issueOnBehalfOfToken. Is everything in this PR needed for the issuance of obo tokens? Can we narrow the PR to the issuance of obo tokens and have a subsequent PR address the issuance of service account tokens?

cwperks · 2023-08-01T13:08:25Z

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

        List<String> roles,
        List<String> backendRoles
    ) throws Exception {
+        String tokenIdentifier = "obo";


Can this variable be called tokenType?

Sometimes JWTs include a claim called jti that is a unique identifier for a token. In my experience, systems use jti to invalidate tokens since this identifier uniquely identifies a token.

cwperks · 2023-08-01T13:10:22Z

src/main/java/org/opensearch/security/user/InternalUserTokenHandler.java

+
+import static org.opensearch.security.dlic.rest.support.Utils.universalHash;
+
+public class InternalUserTokenHandler implements TokenManager {


This class looks like its doing too much. In the PR description it mentions that this PR is for issueOnBehalfOfToken. Are all of these methods being utilized in this class?

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

DarshitChanpura · 2023-08-17T20:47:43Z

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

+    public static DiscoveryNode getLocalNode() {
+        return localNode;
+    }
+
+    public static void setLocalNode(DiscoveryNode node) {
+        localNode = node;
+    }


this should be removed. Check this PR for more details: https://github.com/opensearch-project/security/pull/3066/files

DarshitChanpura · 2023-08-17T20:48:00Z

src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java

            return;
        } catch (IOException ex) {
            throw new IOException(ex);
+        } catch (NoSuchAlgorithmException e) {


Why add this catch block to then throw RuntimeException?

DarshitChanpura · 2023-08-17T20:50:37Z

src/main/java/org/opensearch/security/identity/SecurityTokenManager.java

+    @Subscribe
+    public void onDynamicConfigModelChanged(DynamicConfigModel dcm) {
+        this.dcm = dcm;
+        if (dcm.getDynamicOnBehalfOfSettings().get("signing_key") != null


we should extract: dcm.getDynamicOnBehalfOfSettings() to a variable

DarshitChanpura · 2023-08-17T20:52:53Z

src/main/java/org/opensearch/security/user/UserService.java

-
-        return passwordGenerator.generatePassword(random.nextInt(8) + 8, rules);
+    private String generatePassword() {
+        return "superSecurePassword";


why make this change?

DarshitChanpura · 2023-08-17T20:55:06Z

src/main/java/org/opensearch/security/user/UserTokenHandler.java

+import java.io.IOException;
+import java.util.Collections;
+
+public class UserTokenHandler {


Is this class different that InternalUserTokenHandler in the sense that this class deals with actual user vs the other one deals with service accounts?
If so why not combine them into 1 class?

DarshitChanpura · 2023-08-17T20:55:40Z

src/test/java/org/opensearch/security/auth/SecurityTokenManagerTests.java

+        claims = new HashMap<String, Object>() {
+            {
+                put(JwtConstants.CLAIM_AUDIENCE, "ext_0");
+            }
+        };


can write this as: Map.of(JwtConstants.CLAIM_AUDIENCE, "ext_0");

RyanL1997 force-pushed the ext-token-handler branch from 358fe72 to d6c541e Compare July 27, 2023 16:08

RyanL1997 marked this pull request as ready for review July 27, 2023 23:09

RyanL1997 requested review from DarshitChanpura, cliu123, cwperks, davidlago, peternied, reta, stephen-crawford and willyborankin as code owners July 27, 2023 23:09

RyanL1997 changed the title ~~[Feature] Extension token handler~~ [Feature/Extension] Extension token handler Jul 27, 2023

RyanL1997 commented Jul 27, 2023

View reviewed changes

RyanL1997 self-assigned this Jul 28, 2023

cwperks reviewed Aug 1, 2023

View reviewed changes

RyanL1997 added 5 commits August 8, 2023 10:28

Implements token handler

09b51fd

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

Fix the NoSuchAlgo Exception

f6f368e

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

Add issueOBOToken in the Security Token Manager

b935c32

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

Add test cases and modify the checks of obo validation

1be2551

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

Add license header for security token manager tests

69dfb67

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

RyanL1997 force-pushed the ext-token-handler branch from ef2e39d to 69dfb67 Compare August 8, 2023 17:31

Add library fix

03a7875

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

DarshitChanpura reviewed Aug 17, 2023

View reviewed changes

RyanL1997 closed this Aug 22, 2023


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

		public class InternalUserTokenHandler implements TokenManager {

Conversation

RyanL1997 commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

codecov bot commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RyanL1997 commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyanL1997 left a comment

Choose a reason for hiding this comment

Uh oh!

RyanL1997 Jul 27, 2023 • 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!

RyanL1997 Jul 27, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

cwperks 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RyanL1997 commented Jul 19, 2023 •

edited

Loading

codecov bot commented Jul 19, 2023 •

edited

Loading

RyanL1997 commented Jul 27, 2023 •

edited

Loading

RyanL1997 Jul 27, 2023 •

edited

Loading

RyanL1997 Jul 27, 2023 •

edited

Loading