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

Introduce authorization for enrich in ESQL #99646

Merged
merged 5 commits into from
Sep 27, 2023

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Sep 19, 2023

This change introduces a new privilege monitor_enrich. Users are required to have this privilege in order to use the enrich functionality in ESQL. Additionally, it eliminates the need to use the enrich_origin when executing enrich lookups. The enrich_origin will only be used when resolving enrich policies to prevent warnings when accessing system indices directly.

Closes #98482

@dnhatn dnhatn changed the title Remove enrich origin in enrich lookup Introduce authorization for enrich in ESQL Sep 19, 2023
@dnhatn dnhatn requested a review from ywangd September 19, 2023 18:02
@dnhatn dnhatn marked this pull request as ready for review September 19, 2023 18:02
@elasticsearchmachine elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Sep 19, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

@dnhatn dnhatn added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC and removed Team:QL (Deprecated) Meta label for query languages team labels Sep 19, 2023
@elasticsearchmachine elasticsearchmachine added Team:Security Meta label for security team Team:QL (Deprecated) Meta label for query languages team labels Sep 19, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@ywangd
Copy link
Member

ywangd commented Sep 20, 2023

@dnhatn I am no longer on the security team. So I'll ping @jakelandis for a formal review.

Briefly looking through the changes, I think the general approach makes sense to me. We might need to tweak exactly what privilege is required for resolving the enrich policy. Right now it is monitor which feels a bit off to me. Existing manage_enrich cluster privilege does not seem to fit either. We might need add a new and more specific read level cluster privilege for this purpose. That said, I'll defer to Jake for concrete suggestions. Thanks!

@dnhatn dnhatn requested review from jakelandis and removed request for ywangd September 20, 2023 03:22
@dnhatn
Copy link
Member Author

dnhatn commented Sep 20, 2023

@ywangd No problem. Thank you for taking a look. I've requested @jakelandis to take a look at the PR.

@jakelandis
Copy link
Contributor

Do I understand this correctly ?:

Before: any user could use the enrich key word which allows the "indices:data/read/esql/lookup" by using the ClientHelper.ENRICH_ORIGIN (which in turn resulted in executing as the (InternalUsers.XPACK_USER user)

With this PR: we no longer user the ClientHelper.ENRICH_ORIGIN which means the user must be explicitly granted permissions. Specifically the user now need index:read (or explicitly named: indices:data/read/esql/lookup) for the enrich policy name and cluster:monitor (or explicitly named: cluster:monitor/esql/resolve_enrich) to order use the use the 'enrich' keyword. The new cluster:monitor is needed since the enrich indices are system (or at least hidden) indices and the concrete index names are implementation details. So this is needed map the friendly name of the enrich policy to the underlying index name.

Assuming I understand the change correctly (I may not!) ... it feels odd that we are applying index level privileges on the enrich policy name. I worry that it violates our expectations w.r.t. applying privileges for indices/aliases/data streams. I am also kinda surprised this actually works since for index level permissions since we iterate through the known indices, datastreams, and aliases, compare them against what is in the role and then rewrite the indices that are requests omiting the ones you don't have access to. I am failing to see how that works since that security logic does not take the enrich resolution. However, I also am not familiar with how ESQL works and feels like I am missing a key piece of information to understand how this works (is that what the messageRecieved override is doing ?...then how is the comparison against the friendly name working ?). A better understanding here might help with these technical concerns. However, there is still a non-technical concern w.r.t. to placing non-index abstractions (alias, datastream, index name) in the indices name of the role. For example, I can not query "test-enrich" with the normal _search API and am concerned this could lead to confusion for what is 'indices.names'... how does this work with Kibana UIs ? how do users know to put the policy name in there ? and the eventual questions for why does it not also work with enrich via ingest node pipelines or policy execution.

I also have concerns w.r.t. normal users that just want to do some queries needing the cluster:monitor privilege. That cluster privilege is intended to be a (lower) admin level set of privileges. There probably isn't too much of pure security concern with giving normal users that access in the scope that it does not leak credentials or the like... but doing so IMO it violates least privilege. It doesn't seem right that to use enrich queries I also have to give users permission to see things like autoscaling stats or ml jobs or ccr stats, etc. Being overly restrictive can have the opposite effect in that some admins will just give wider permissions (lowering security) to users that don't need it just to accomplish the goal without fully understanding the implications. Also, I don't believe that any normal users in serverless will ever have this permission.

Ideally we would have a permission model for enrich that is applicable to both ESQL, ingest nodes, and policy execution that does not require users to know to how to configure indices permissions for things that are not index abstractions (as defined by being able to _search on those names). This probably looks like a new field in the role 'enrich.names' where the enrich names are treated like abstractions such as date math or aliases that get resolved to the allowed list of concrete index names in the security (and non-security) index resolution. That model would conform to the existing expectations be accessible to ingest node / policy execution workflows and IMO better progresses enrich as concept.

Ultimately you are hitting on our lack of resource specific permissions and there might be some less involved short term solutions. However, before proposing any alternatives I would want to double check my understanding and better understand the requirements. Those conversations probably extend beyond this PR.

@tvernum
Copy link
Contributor

tvernum commented Sep 21, 2023

@jakelandis If I understand your comment correctly, I think you might have been mislead by the name of the index in the tests.

test-enrich is a normal index, that is used as part of the testing of the enrichment command.
The enrich index and policy are called songs.

So the ES|QL command is

FROM test-enrich | ENRICH songs ON song_id | stats total_duration = sum(duration) by artist | sort artist

Which (more or less) translates to

  1. Read from the test-enrich index
  2. Apply the song enrich policy to it, joining on song_id
  3. sum duration by artist
  4. sort by artist

The security change is in how step 2 is applied.

  • Previously every user would be able to perform the enrich step (against any policy).
  • After this change, the ability to perform an ES|QL enrich is restricted by a cluster privilege (currently proposed to bemonitor, but I agree that it's not a great fit). No specific index level privilege is required.

The change to the security setup in roles.yml consists of

  1. Granting monitor to the existing user (user1) who should be allowed to perform enrich (and previously could because no privilege was required)
  2. Creating a read-only user (user4) who should be allowed to run the ES|QL command. (This is helpful because user1 has additional privileges and we'd like to know that the command works without write etc).
  3. Creating another read-only user (user5) who does not have monitor and, therefore, should not be allowed to run the ES|QL command.

None of those users have access to anything named songs (neither the source index, nor the enrich policy). This works because the action is cluster:monitor/esql/resolve_enrich, so it is authorized as a cluster monitor action.
It's not really an "action" though - it's all handled as a lightweight transport request handler inside EnrichPolicyResolver.

Interestingly, despite the PR description, the actual enrichment still happens using the enrich origin which explains why it works

As @dnhatn has indicated, the ideal outcome would be to have a way to limit which enrich polices a user may use within ES|QL. It looks like we could probably do that with a ConfigurableClusterPrivilege if we wanted. It would require a little bit of rework of this PR (e.g. extracting a EnrichPolicyRequest interface), but I can't see any technical reason why it couldn't be done.

@dnhatn
Copy link
Member Author

dnhatn commented Sep 21, 2023

@jakelandis @tvernum Thank you so much for looking.

Tim has summarized this perfectly. In the enrich lookup in ESQL, there are two actions:

  1. Resolve enrich policy:
    Action (cluster:monitor/esql/resolve_enrich): This action doesn't access its Lucene index but only the enrich policies in the cluster state and its index metadata via the field-caps API. Previously, this action didn't require any access permission, meaning any user could access the enrich policies. This PR now requires users to have a 'monitor' permission to perform this step. We can consider introducing a new privilege for enrich-policy-read. Another question is how to integrate this privilege with the enrich ingest without causing any breaking changes. We will still perform the resolution of enrich policies under the 'enrich_origin' to avoid deprecation warnings when accessing the system indices directly.

  2. Lookup enrich data:
    Action (indices:data/read/esql/lookup): This action accesses the Lucene index and performs the actual enrich. This action requires the user to have the index-read permission for the enrich index. This action will no longer execute under the 'enrich_origin' after this PR.

I believe we don't require any extra privilege for ingest-enrich.

@jakelandis
Copy link
Contributor

Do I understand this correctly ?

...no I did not. Thanks Tim and Nhat for the clarifications. One last clarification:

This action (indices:data/read/esql/lookup) requires the user to have the index-read permission for the enrich index.

You mean the index being enriched... right ? The enrich index is defined here as "A special system index tied to a specific enrich policy". Sorry to be pedantic, but confusing the security requirements of two was the source of my original confusion.

Assuming we are all on the same page... The only relevant part of the original comment are :

Ideally we would have a permission model for enrich that is applicable to both ESQL, ingest nodes, and policy execution

While ideal that would be quite involved. An implementation with ConfigurableClusterPrivilege could set us on that path but we would probably want to flesh out the requirements more and consider other options. However, I do think the simpler cluster wide approach is viable and (with a correct understanding of the change) is in line with the existing model even it is not terribly flexible/expressive.

I also have concerns w.r.t. normal users that just want to do some queries needing the cluster:monitor privilege

While it possible to configure a role with the string cluster:monitor/esql/resolve_enrich we generally discourage this (except for tightly controlled internal usecases) since we consider those names implementation details. In practice we would advocate to use the named privileges such as 'monitor' .. as already mentioned that is likely too wide for this usecase. Can we use an ESQL specific named privilege... or change the action name to 'cluster:monitor/xpack/enrich/esql/resolve_enrichand introduce amonitor_enrichwith a pattern ofcluster:monitor/xpack/enrich/*` which would also include the ability to read enrich stats. There is always a friction between grouping raw privileges into named privileges lower vs. least privileges. I would advocate for use an EQSL name if you think there will be more than just 1 in the near future, else I would advocate to group in with a new 'monitor_enrich'.

@dnhatn dnhatn removed the request for review from ChrisHegarty September 26, 2023 18:01
@dnhatn
Copy link
Member Author

dnhatn commented Sep 26, 2023

Can we use an ESQL specific named privilege... or change the action name to 'cluster:monitor/xpack/enrich/esql/resolve_enrichand introduce amonitor_enrichwith a pattern ofcluster:monitor/xpack/enrich/*` which would also include the ability to read enrich stats.

@jakelandis Thanks so much for your suggestion. It makes sense to me. I will update this PR to introduce a new privilege.

@dnhatn
Copy link
Member Author

dnhatn commented Sep 26, 2023

@jakelandis I've introduced the monitor_enrich privilege as you suggested in 216d267. I should have also introduced enrich_user but we already have it for manage_enrich. Would you please take another look? Thank you!

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

LGTM - however, can you update the doc here either as part of this PR or as a followup.

Thanks again for clarifications and apologies for the noise.

@dnhatn
Copy link
Member Author

dnhatn commented Sep 27, 2023

@jakelandis Thank you for the review + suggestion :).

however, can you update the doc here either as part of this PR or as a followup.

Sure, I will do this in a follow-up.

@dnhatn dnhatn merged commit ae17505 into elastic:main Sep 27, 2023
15 checks passed
@dnhatn dnhatn deleted the remove-enrich-origin branch September 27, 2023 19:45
piergm pushed a commit to piergm/elasticsearch that referenced this pull request Oct 2, 2023
This change introduces a new privilege monitor_enrich. Users are 
required to have this privilege in order to use the enrich functionality
in ESQL. Additionally, it eliminates the need to use the enrich_origin
when executing enrich lookups. The enrich_origin will only be used when
resolving enrich policies to prevent warnings when accessing system
indices directly.

Closes elastic#98482
jakelandis added a commit that referenced this pull request Oct 13, 2023
manage_enrich is a cluster privilege, not a built in role. 
manage_enrich is already documented as a cluster privilege.
This commit remove manage_enrich from the role documentation.
This commit also makes mention of the monitor_enrich introduced in #99646.

related: #85877
jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Oct 13, 2023
manage_enrich is a cluster privilege, not a built in role.
manage_enrich is already documented as a cluster privilege.
This commit remove manage_enrich from the role documentation.
This commit also makes mention of the monitor_enrich introduced in elastic#99646.

related: elastic#85877
(cherry picked from commit 1eaa907)
elasticsearchmachine pushed a commit that referenced this pull request Oct 13, 2023
manage_enrich is a cluster privilege, not a built in role.
manage_enrich is already documented as a cluster privilege.
This commit remove manage_enrich from the role documentation.
This commit also makes mention of the monitor_enrich introduced in #99646.

related: #85877
(cherry picked from commit 1eaa907)
dnhatn added a commit that referenced this pull request Oct 18, 2023
Today, we have a hierarchy of tasks in ESQL designed to leverage the 
task framework for reporting status and cancellation.

```mermaid
flowchart
 RESTLayer -->|  EsqlQueryRequest indices:data/read/esql  | ComputeService
 ComputeService -->| DriverRequest indices:data/read/esql/compute | Driver
 ComputeService -->| DataNodeRequest indices:data/read/esql/data | DataNode
 DataNode -->| DriverRequest indices:data/read/esql/compute | Driver
 Driver -->| LookupRequest indices:data/read/esql/lookup | EnrichLookupService
```

The primary issue here is that `DriverRequest` is neither 
`IndicesRequest` nor `CompositeIndicesRequest`. Consequently, the Driver
is executed within the context of the system user, leading to access
indices with the system user.

To address this issue, this PR makes `DriverRequest` a 
`CompositeIndicesRequest` and ensures that the Driver executes within
the user's context. With this fix we can now properly capture the
response headers when a Driver is yielded and rescheduled.

Relates #100707
Relates #99646

Relates #99926
Closes #100164
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Oct 18, 2023
Today, we have a hierarchy of tasks in ESQL designed to leverage the 
task framework for reporting status and cancellation.

```mermaid
flowchart
 RESTLayer -->|  EsqlQueryRequest indices:data/read/esql  | ComputeService
 ComputeService -->| DriverRequest indices:data/read/esql/compute | Driver
 ComputeService -->| DataNodeRequest indices:data/read/esql/data | DataNode
 DataNode -->| DriverRequest indices:data/read/esql/compute | Driver
 Driver -->| LookupRequest indices:data/read/esql/lookup | EnrichLookupService
```

The primary issue here is that `DriverRequest` is neither 
`IndicesRequest` nor `CompositeIndicesRequest`. Consequently, the Driver
is executed within the context of the system user, leading to access
indices with the system user.

To address this issue, this PR makes `DriverRequest` a 
`CompositeIndicesRequest` and ensures that the Driver executes within
the user's context. With this fix we can now properly capture the
response headers when a Driver is yielded and rescheduled.

Relates elastic#100707
Relates elastic#99646

Relates elastic#99926
Closes elastic#100164
elasticsearchmachine pushed a commit that referenced this pull request Oct 18, 2023
Today, we have a hierarchy of tasks in ESQL designed to leverage the 
task framework for reporting status and cancellation.

```mermaid
flowchart
 RESTLayer -->|  EsqlQueryRequest indices:data/read/esql  | ComputeService
 ComputeService -->| DriverRequest indices:data/read/esql/compute | Driver
 ComputeService -->| DataNodeRequest indices:data/read/esql/data | DataNode
 DataNode -->| DriverRequest indices:data/read/esql/compute | Driver
 Driver -->| LookupRequest indices:data/read/esql/lookup | EnrichLookupService
```

The primary issue here is that `DriverRequest` is neither 
`IndicesRequest` nor `CompositeIndicesRequest`. Consequently, the Driver
is executed within the context of the system user, leading to access
indices with the system user.

To address this issue, this PR makes `DriverRequest` a 
`CompositeIndicesRequest` and ensures that the Driver executes within
the user's context. With this fix we can now properly capture the
response headers when a Driver is yielded and rescheduled.

Relates #100707
Relates #99646

Relates #99926
Closes #100164
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:QL (Deprecated) Meta label for query languages team Team:Security Meta label for security team v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security privileges for ESQL Enrich
5 participants