Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.x] Adds On-Behalf-Of authentication mechanism and service account capability #3416

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented Sep 27, 2023

Description

Backport On-behalf-of Authentication and Service account into the 2.x branch.

Category: New Feature

Issues Resolved

Testing

Integration testing

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.

stephen-crawford and others added 9 commits September 27, 2023 14:16
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@RyanL1997
Copy link
Collaborator

The manual testing with the build of opensearch-project/OpenSearch#10258 succeed:

➜  ~ curl -XPOST "https://localhost:9200/_plugins/_security/api/generateonbehalfoftoken" --insecure -u 'admin:admin' --data '
{
  "reason":"Testing",
  "service":"extension123",
  "durationSeconds":"600"
}' -H 'Content-Type: application/json' | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   462  100   382  100    80    758    158 --:--:-- --:--:-- --:--:--   920
{
  "user": "admin",
  "authenticationToken": "eyJhbGciOiJIUzUxMiJ9.eyJpc3MiOiJvcGVuc2VhcmNoIiwiaWF0IjoxNjk1OTIzNzU1LCJzdWIiOiJhZG1pbiIsImF1ZCI6ImV4dGVuc2lvbjEyMyIsIm5iZiI6MTY5NTkyMzc1NSwiZXhwIjoxNjk1OTI0MzU1LCJlciI6IjczU1JBZjlueStqR3AxczFLSjk4d1VOeXJDckY0TWcydW1pSU4vN0c5YkE9In0.op-l1zhpwz93zvYYzLEuGXFLH5ZGf_5wvMbd4SNtAtWClTmDrP7tepfC7X5yxuymnirZwHtYNk0eams_NQOJew",
  "durationSeconds": 600
}

@stephen-crawford
Copy link
Contributor

I would recommend splitting this into two PRs: one that does OBO changes and one that does service account changes.

I am not as familiar with the OBO changes since I did not write them originally but here is what is required for service accounts:

  1. Implement IdentityPlugin in OpenSearchSecurityPlugin
  2. Create an SecurityPluginTokenManager or something similar that implements the TokenManager interface.
    This should be able to issue a service account token and an on behalf of token
  3. Connect the IdentityPlugin method to the Token Manager so that when a new extension is registered and the transport action registering its security settings is called, we also issue it a new service account token.
  4. Inside the token manager, the service account issuance process should register a new internal user account for the extension if one does not already exist.
  5. When an extension attempts to use a service account token it will be using it as the authentication header for its request so all the changes relating to using it should go through the normal auth process of an internal user account.
    The only differences are that a enabled equals false service account should fail to perform any requests.
    As part of the threat model mitigation we also need to block any actions which are not. being explicitly performed on the system index associated with the service account being used. This requires issuance of system indices etc. for extensions which is not currently being done. I think we can wait on this but I would ask @peternied and @cwperks. Since they should know the decision there.
  6. Test service accounts are issued properly by using the fake rest handler with a service account token generated in a mocked scenario. We don't need to test the transport deliverance inside the security plugin because that is outside of the scope.

@RyanL1997
Copy link
Collaborator

RyanL1997 commented Sep 28, 2023

Thanks @scrawfor99! Alright then, I will do the backport of OBO separately.

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comments on the original changes to give some direction.

@@ -1110,6 +1144,15 @@ public Settings additionalSettings() {
return builder.build();
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the way Craig set this up, the idea is to register a set of extension security settings when an extension is installed/registered as part of the core initialization process. To make this work with service accounts the general idea would be to make sure that a service account is only able to operate on the indices listed in the reserved_indices list and affiliated with themselves--this should use a mapping or some other mechanism.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed BACKEND_ROLES setting for now. In SecurityIndexAccessEvaluator we check by looking up extension settings by username (should match extension name), and then matching the requested index with extension's reserved index to determine whether access should be granted

channel.sendResponse(
new BytesRestResponse(
RestStatus.SERVICE_UNAVAILABLE,
"The OnBehalfOf token generating API has been disabled, see {link to doc} for more information on this feature." /* TODO: Update the link to the documentation website */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @RyanL1997 is doing this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link will only go live after it has been published. I would recommend just redirecting them to website. @RyanL1997 thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine to redirect that to the homepage of our documentation website, at least for now. So it will be https://opensearch.org/docs/latest/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we wait until the doc team publish the work so that we can use aa more accurate url?

@DarshitChanpura DarshitChanpura self-assigned this Sep 29, 2023
DarshitChanpura and others added 4 commits September 29, 2023 16:05
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
@RyanL1997 RyanL1997 mentioned this pull request Oct 16, 2023
35 tasks
@DarshitChanpura
Copy link
Member Author

@RyanL1997 @scrawfor99 Should this be closed as there is a separate effort on-going for this?

@DarshitChanpura
Copy link
Member Author

I'll be closing this out in lieu of ongoing efforts to merge Service accounts and OBO related changes separately. Please re-open if this is picked back up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants