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

Add default request labeling rules in security plugin #4403

Closed
wants to merge 2 commits into from

Conversation

ansjcy
Copy link
Member

@ansjcy ansjcy commented Jun 5, 2024

Description

As part of the effort to introduce multi-tenancy as a construct in OpenSearch (opensearch-project/OpenSearch#13341), we are introducing a labeling service to attach tenancy labels to search requests. Plugins can define their rules to compute labels based on the given request and thread context. Adding this rule to resolve concerns brought here: opensearch-project/OpenSearch#13374 (comment)

Issues Resolved

#4402

Is this a backport? If so, please add backport PR # and/or commits #

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
  • 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.

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.

Thank you for adding this PR @ansjcy. Left a clarifying question.

public static final String USER_NAME = "user_name";
public static final String USER_SECURITY_ROLES = "user_backend_roles";
public static final String USER_ROLES = "user_roles";
public static final String USER_TENANT = "user_tenant";
Copy link
Member

Choose a reason for hiding this comment

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

Are all these fields needed? From the linked issue, it seems like only tenant information is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @DarshitChanpura ! The tenancy we want to obtain is different from the current tenancy concept (a dashboard concept ?). We want to provide the ability for users to identify "who send what" queries. A request can be sent from a human user (identified by username, roles etc), it can also be sent by some application used by multiple users (identified by username, remote address etc), Thus we want to populate all information that can correctly identify a "source of a request".

@cwperks
Copy link
Member

cwperks commented Jun 5, 2024

@ansjcy The security plugin has an existing concept of "Tenancy" for dashboards to separate saved objects by tenant.

It may get confusing to have 2 separate notions of tenancy that mean different things. How do you think we can better differentiate between the 2 concepts?

@ansjcy
Copy link
Member Author

ansjcy commented Jun 5, 2024

@cwperks good point! in opensearch-project/OpenSearch#13341 we mentioned introducing multi-tenancy as a first-citizen construct in OpenSearch. Probably we should rename the current tenancy to "dashboard tenant" ? what do you think?

@ansjcy
Copy link
Member Author

ansjcy commented Jun 5, 2024

We still need to merge opensearch-project/OpenSearch#13374 before this PR, to resolve the build failures.

@deshsidd
Copy link

deshsidd commented Jun 6, 2024

Looks good to me overall. Only issue seems : It may get confusing to have 2 separate notions of tenancy that mean different things. @cwperks are we not planning to deprecate the concept in the security plugin?

@cwperks
Copy link
Member

cwperks commented Jun 6, 2024

@ansjcy @deshsidd There is another library called common-utils that has methods to get user info from the ThreadContext: link

See example of usage in anomaly-detection.

Its also possible to get the TransportAddress like this.

When populating the remote_address header, the security plugin does XFF resolution in case OpenSearch is behind a proxy.

Would the common-utils library work for your use-case? Are you planning to have an implementation dependency on the security plugin?

@DarshitChanpura
Copy link
Member

Is the idea to fetch user information from requests only? OR as Craig mentioned, can common-utils be leveraged?

@ansjcy
Copy link
Member Author

ansjcy commented Jun 11, 2024

Hey @cwperks and @DarshitChanpura, thanks for your comments! The overarching goal here is, we want to introduce the "multi-tenancy" concept in OpenSearch (opensearch-project/OpenSearch#13341), as mentioned in the RFC, the approaches we want to take are "let the user attach the tenancy labels as part of the search request", as well as having an internal rule based system to attach labels based on certain internal rules.

So the idea of this rule based labeling service is, we provide the core labeling logic in OpenSearch core, and plugins (especially authz/authn related plugins) should define their own rules to attach labels to a search request based on the current auth info (as discussed here: opensearch-project/OpenSearch#13374 (comment)). OpenSearch core shouldn't know the existence of security plugin, it should only take in all the rules and compute the final labels.

Also, using common-utils means we need to add it as a OpenSearch core dependency, which is not possible as we already discussed in opensearch-project/OpenSearch#13374 (comment).

@cwperks
Copy link
Member

cwperks commented Jun 11, 2024

@ansjcy How does core receive the info from this PR? Which extension point is it using? When is core trying to obtain the data? Is it only during API handling after a request has been authenticated?

Can you demonstrate how the changes in this PR will be used with a test?

@ansjcy
Copy link
Member Author

ansjcy commented Jun 12, 2024

@cwperks In the current POC work, core opensearch will have a labeling service like (https://github.com/ansjcy/OpenSearch/blob/3b6fd30dbb70452368cee3501e6889b4d9cc347a/server/src/main/java/org/opensearch/node/Node.java#L971) to read the rules from other plugins. Once a search query is authenticated, the rule service will apply all rules like this: https://github.com/ansjcy/OpenSearch/blob/3b6fd30dbb70452368cee3501e6889b4d9cc347a/server/src/main/java/org/opensearch/search/labels/RequestLabelingService.java#L43

Is it only during API handling after a request has been authenticated?

Yes

Again, the above code is still in POC, we planned to ship it in 2.15 but decided to take further reviews on the rule-based labeling service. I will continue working on this PR and add related tests (maybe in core?) once I restart the progress on multi-tenancy support.

@DarshitChanpura
Copy link
Member

let the user attach the tenancy labels as part of the search request

Does it mean that not all requests will contain this info and is left onto user to send their identifying information? What kind of information do we expect to be received/handled by security plugin?

A video of a working PoC would be nice to have.

@cwperks
Copy link
Member

cwperks commented Jun 17, 2024

@ansjcy Curious about the extension point. Why iterate through the components that were created to look for an instance of a Rule instead of creating an extension point to get the rules explicitly? i.e. ActionPlugin.getRules() or something like that? Possible a new extension point instead of ActionPlugin if it doesn't make sense to include in the ActionPlugin interface.

@ansjcy
Copy link
Member Author

ansjcy commented Jun 25, 2024

Does it mean that not all requests will contain this info and is left onto user to send their identifying information? What kind of information do we expect to be received/handled by security plugin?

Hi @DarshitChanpura!
We are not changing anything related to the authn/authz workflow and the information received by security plugin will remain the same. We are simply introducing a new "label" concept in a request and we want to add a rule for security plugin to attach labels based on identify. typical labeling workflows are:

  • Users can send their own labels, the consumer of those labels (like query grouping) need to write their own logic to validate labels if needed.
  • Or, the user doesn't provide anything, plugins injects labels if necessary. In the security plugin case, we are injecting labels related to the user's identity, this is similar to what we are currently doing to inject identify info in thread context.

A video of a working PoC would be nice to have.

Yes! we actually have a video explaining the client side labeling use case in opensearch-project/OpenSearch#13341 (comment). I would also recommend going through the whole RFC, it would clear a lot of doubts around why we are doing this for labeling.

ActionPlugin.getRules() or something like that?

@cwperks This is actually a very good point. We are also exploring this approach with this PR: https://github.com/opensearch-project/OpenSearch/pull/14388/files . We introduced a new plugin interface instead of "iterate through the components that were created to look for an instance of a Rule", let me know what do you think!

@DarshitChanpura
Copy link
Member

Thank you @ansjcy for addressing my questions. I did go through the RFC and the idea is more clearer to me.

@DarshitChanpura
Copy link
Member

@ansjcy Are you still working on this PR?

@DarshitChanpura
Copy link
Member

@ansjcy Closing this PR due to lack of activity. Please open a new PR if you plan on continuing this work.

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.

4 participants