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.

Sign up for GitHub

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

Jump to bottom

Scaffolding for POST/DELETE/GET api tokens calls #4921

Merged
merged 26 commits into from
Dec 16, 2024
Merged

Scaffolding for POST/DELETE/GET api tokens calls #4921

merged 26 commits into from
Dec 16, 2024

Conversation

derek-ho
Copy link
Collaborator

@derek-ho derek-ho commented Nov 19, 2024 •
edited
Loading

Description

This PR puts the high level scaffolding in place to handle GET/POST/DELETE api tokens api calls.
Notably:

  • function to create an index to hold api tokens if it doesn't exist
  • function to create a document representing api tokens
  • function to delete a document corresponding to a given name
  • function to get all documents representing api tokens

This PR is not a finalized/api token structure may be subject to change up until merge into main branch (this is designated for feature branch), and does not perform any authc/authz checks using these newly introduced API tokens.

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • 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.

@derek-ho derek-ho changed the base branch from main to feature/api-tokens November 19, 2024 22:04
@derek-ho derek-ho mentioned this pull request Nov 19, 2024
Closed
5 tasks
@derek-ho derek-ho changed the title Handle POST api tokens calls Handle POST/DELETE/GET api tokens calls Nov 20, 2024
@cwperks
Copy link
Member

cwperks commented Nov 20, 2024

A couple of high-level comments:

  1. This PR currently looks like scaffolding to build on, what is the goal with this PR?

  2. Can you add tests for different scenarios that this PR handles?

  3. IMO Tokens should have permissions/scopes directly associated with them - not roles. We could consider allowing the API to request issuing a token to accept a role and copy the permissions from the role.

@derek-ho
Copy link
Collaborator Author

A couple of high-level comments:

  1. This PR currently looks like scaffolding to build on, what is the goal with this PR?

Updated the description. The goal of this PR is to mainly have a working POC/high level scaffolding in place to interact with the underlying opensearch cluster, such as creating/getting/deleting documents in an opensearch index representing API tokens. This PR does not finalize/use those documents to perform any authc/z checks yet.

  1. Can you add tests for different scenarios that this PR handles?

Yes, I am working on that now, and I will add those before this PR is merged.

  1. IMO Tokens should have permissions/scopes directly associated with them - not roles. We could consider allowing the API to request issuing a token to accept a role and copy the permissions from the role.

Yup - I am pushing a commit to address this. My high level approach will be to ask for user input for cluster permissions and index permissions (the same input when creating a role in the first place) and then performing authz checks on the ephemeral role, which will be created from these cluster/index permissions but not persisted to the index. Let me know if this high level approach makes sense to you CC @DarshitChanpura

@derek-ho derek-ho changed the title Handle POST/DELETE/GET api tokens calls Scaffolding for POST/DELETE/GET api tokens calls Nov 20, 2024
cwperks
cwperks reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

This is a good base to continue working off of! Good start to this much requested feature :). It would be great to see an end-to-end integ test where a user can request a token is created and then use it to make requests against the cluster.

DarshitChanpura
DarshitChanpura reviewed Dec 13, 2024
View reviewed changes
src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/security/action/apitokens/ApiToken.java Show resolved Hide resolved
src/main/java/org/opensearch/security/compliance/ComplianceConfig.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java Outdated
}

public void deleteApiToken(String name) throws ApiTokenException {
apiTokenIndexHandler.createApiTokenIndexIfAbsent();
Copy link
Member

Choose a reason for hiding this comment

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

why do we delete token if Index doesn't exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have logic handling the delete of the api token if it doesn't exist. My thinking around creating the index here is that if users are trying to call on the delete of this endpoint in the first place they may be interested in this feature, so we create the index for them (even if the token doesn't exist). Again this can be changed in future/if index is created on bootstrap, but we can re visit this when we decide on the correct approach for index creation.

Copy link
Member

Choose a reason for hiding this comment

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

We should not be creating an index. Instead we can modify the method definition to throw an exception when Index is not found.

Copy link
Member

Choose a reason for hiding this comment

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

Same for below.

src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java Show resolved Hide resolved
src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java Show resolved Hide resolved
DarshitChanpura
DarshitChanpura reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

Final few comments. Looks ready to be merged otherwsie. Thanks @derek-ho !

src/main/java/org/opensearch/security/action/apitokens/ApiToken.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/security/action/apitokens/ApiTokenIndexHandler.java Show resolved Hide resolved
src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java Outdated
}

public void deleteApiToken(String name) throws ApiTokenException {
apiTokenIndexHandler.createApiTokenIndexIfAbsent();
Copy link
Member

Choose a reason for hiding this comment

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

We should not be creating an index. Instead we can modify the method definition to throw an exception when Index is not found.

src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java Outdated
}

public void deleteApiToken(String name) throws ApiTokenException {
apiTokenIndexHandler.createApiTokenIndexIfAbsent();
Copy link
Member

Choose a reason for hiding this comment

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

Same for below.

@derek-ho derek-ho merged commit 3177c34 into opensearch-project:feature/api-tokens Dec 16, 2024
42 checks passed
@derek-ho derek-ho deleted the manager branch December 16, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwperks cwperks cwperks left review comments

@DarshitChanpura DarshitChanpura DarshitChanpura approved these changes

@nibix nibix Awaiting requested review from nibix nibix is a code owner

@peternied peternied Awaiting requested review from peternied peternied is a code owner

@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 is a code owner

@reta reta Awaiting requested review from reta reta is a code owner

@willyborankin willyborankin Awaiting requested review from willyborankin willyborankin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@derek-ho @cwperks @DarshitChanpura