opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields

https://opensearch.org/docs/latest/security-plugin/index/

Apache License 2.0

180 stars 264 forks source link

Add default request labeling rules in security plugin #4403

Open ansjcy opened 4 weeks ago

ansjcy commented 4 weeks ago

Description

As part of the effort to introduce multi-tenancy as a construct in OpenSearch (https://github.com/opensearch-project/OpenSearch/issues/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: https://github.com/opensearch-project/OpenSearch/pull/13374#discussion_r1626477033

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.

cwperks commented 3 weeks ago

@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 commented 3 weeks ago

@cwperks good point! in https://github.com/opensearch-project/OpenSearch/issues/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 commented 3 weeks ago

We still need to merge https://github.com/opensearch-project/OpenSearch/pull/13374 before this PR, to resolve the build failures.

deshsidd commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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

ansjcy commented 3 weeks ago

Hey @cwperks and @DarshitChanpura, thanks for your comments! The overarching goal here is, we want to introduce the "multi-tenancy" concept in OpenSearch (https://github.com/opensearch-project/OpenSearch/issues/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: https://github.com/opensearch-project/OpenSearch/pull/13374#discussion_r1626477033). 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 https://github.com/opensearch-project/OpenSearch/pull/13374#discussion_r1580248441.

cwperks commented 3 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 1 week ago

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 https://github.com/opensearch-project/OpenSearch/issues/13341#issuecomment-2076192758. 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 commented 5 days ago

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