Allow access to restricted system indices for reserved system roles #76845

williamrandolph · 2021-08-23T19:09:45Z

After we merged #74212 , the Kibana team saw permissions errors at startup.

The cause of this is that all system indices are now restricted indices from the perspective of security, when previously the only restricted indices were security and async search indices. In that PR, we didn't update the reserved system roles so that the ones that access system indices have allowRestrictedIndices = true.

We can find the roles that need this change by adding system index patterns to the TestRestrictedIndices class and looking for failures in ReservedRolesStoreTests.

The fixes for the particular kibana issue are the updates to the kibana_system role.

elasticmachine · 2021-08-23T19:09:48Z

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

elasticmachine · 2021-08-23T19:09:48Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

tylersmalley · 2021-08-24T03:46:03Z

I verified this resolved the startup issue we're seeing with the promotion of the nightly snapshot for Kibana.

ywangd · 2021-08-24T04:54:33Z

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java

                    .indices("*")
-                    .privileges("view_index_metadata", "monitor").build(),
+                    .privileges("view_index_metadata", "monitor")
+                    .allowRestrictedIndices(true).build(),


This change allows the kibana_system user to have some access to the .security index. I don't think this is what we want.

I went through the list of system indices and pared down where I was allowing restricted indices to just the places where we are dealing with system indices, so I hope I've addressed this concern.

Yes the change addressed my previous comment, thanks. But we still have another semantic change: before the #74212, this index privilege allow kibana to monitor and view_index_metadata for indices like .watches, .triggered_watches. These indices are now no longer accessible for kibana_system. Maybe this is the right thing to do. But I'd like to double check whether this is intentional.

tvernum · 2021-08-24T05:04:03Z

It looks to me like this issue slipped through because TestRestrictedIndices only contains security and async search indices.

Can we update it to include the complete set of out-of-the-box system indices?

A missing piece in elastic#74212 was system index patterns in the tests for the ReservedRolesStore. Without these patterns, the tests did not accurately check whether a role was incorrectly accessing a system index that was not previously a restricted index. This commit adds all of the current system index patterns to the test class and adds restricted index access to the system roles that need it for tests to pass.

williamrandolph · 2021-08-25T02:13:43Z

@tvernum

Thanks for the pointer. I think that is exactly the issue. I've manually added all the system index patterns to the TestRestrictedIndices class. I would really like to refactor this work so that we are referencing the actual string constants that the system index plugins use, but I think there's a good argument for pushing that to a follow-up and merging a fix for the Kibana team as soon as possible so that they can update their Elasticsearch snapshots.

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java

ywangd · 2021-08-25T07:01:11Z

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java

                    .indices("*")
-                    .privileges("view_index_metadata", "monitor").build(),
+                    .privileges("view_index_metadata", "monitor")
+                    .allowRestrictedIndices(true).build(),


Yes the change addressed my previous comment, thanks. But we still have another semantic change: before the #74212, this index privilege allow kibana to monitor and view_index_metadata for indices like .watches, .triggered_watches. These indices are now no longer accessible for kibana_system. Maybe this is the right thing to do. But I'd like to double check whether this is intentional.

ywangd

The way how privileges are granted to restricted indices feels brittle to me. It requires checking and matching names from different places carefully (as well as adding them to tests). But maybe it is because this PR is a one-time mass update. It may feel alright for future smaller changes. Also I don't have a good suggestion for alternatives.

I think we need a conclusion on kibana_system's monitor and view_index_metadata access for *. Should we strictly maintain the previous semantic or are we ok to change? I am ok either way as long as we are conscious on the decision. Thanks!

williamrandolph · 2021-08-25T20:45:37Z

@ywangd In the XPackUser class, we have a pattern that allows access to all indices except for ones that begin with .security or .async-search. I've borrowed that pattern for use here, in order to preserve existing behavior.

I think it could be argued that we should exclude other system indices from data telemetry, but I'm not sure about that, and I don't want it to hold up getting this merged.

williamrandolph · 2021-08-25T22:45:19Z

@elasticmachine test this please

ywangd

LGTM

I think it could be argued that we should exclude other system indices from data telemetry

I tend to agree especially this change is 8.0 only. But I am not familiar with all system indices and how they should be surfaced by Kibana. I think it needs input from people who are more familiar with its usage (maybe a round of emails?). For this PR, I think maintaining the exact same semantic is the safest.

Can I also suggest we add a few more assertions about kibana_system cannot view metadata or monitor .security* and .async-search*? The current assertions only ensure no read or write access to the indices, but not metadata level access. I think this is also why the test didn't tripped on the first version of this PR.

williamrandolph · 2021-08-26T03:29:43Z

@ywangd Thank you for the review and for the final suggestion about checking the metadata privileges. I've added the test for it.

@tylersmalley I'll be letting tests run overnight and merging this first thing tomorrow. Apologies for the delays.

tvernum · 2021-08-26T05:41:37Z

I think it could be argued that we should exclude other system indices from data telemetry

I tend to agree especially this change is 8.0 only. But I am not familiar with all system indices and how they should be surfaced by Kibana. I think it needs input from people who are more familiar with its usage (maybe a round of emails?). For this PR, I think maintaining the exact same semantic is the safest.

I think there is where we should end up.
If Kibana needs to monitor system indices, we can have that conversation, but I don't think we should start on that assumption.

Broadly allow access to restricted indices from the Kibana role

19a544c

williamrandolph added :Core/Infra/Core Core issues without another label :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 labels Aug 23, 2021

elasticmachine added Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team labels Aug 23, 2021

Merge branch 'master' into kibana-role-allow-restricted-indices

b91f824

ywangd reviewed Aug 24, 2021

View reviewed changes

williamrandolph added 3 commits August 24, 2021 17:15

Merge branch 'master' into kibana-role-allow-restricted-indices

a603dad

Roll back broad permissions

4b83054

williamrandolph changed the title ~~Broadly allow access to restricted indices from the Kibana role~~ Allow access to restricted system indices for reserved system roles Aug 25, 2021

williamrandolph requested a review from ywangd August 25, 2021 03:00

ywangd reviewed Aug 25, 2021

View reviewed changes

Perserve existing Kibana data telemetry privileges

9d2dca5

williamrandolph requested a review from ywangd August 25, 2021 20:42

Merge branch 'master' into kibana-role-allow-restricted-indices

c8d59d4

ywangd approved these changes Aug 26, 2021

View reviewed changes

Test that data telemetry can't access security and async indices

666b870

williamrandolph merged commit 8759c85 into elastic:master Aug 26, 2021

williamrandolph deleted the kibana-role-allow-restricted-indices branch August 26, 2021 12:56

williamrandolph mentioned this pull request Aug 26, 2021

Remove data telemetry access to restricted indices #76994

Merged

jakelandis added v8.0.0-alpha2 >bug and removed v8.0.0 labels Sep 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow access to restricted system indices for reserved system roles #76845

Allow access to restricted system indices for reserved system roles #76845

williamrandolph commented Aug 23, 2021 •

edited

Loading

elasticmachine commented Aug 23, 2021

elasticmachine commented Aug 23, 2021

tylersmalley commented Aug 24, 2021 •

edited

Loading

ywangd Aug 24, 2021

williamrandolph Aug 24, 2021

ywangd Aug 25, 2021

tvernum commented Aug 24, 2021

williamrandolph commented Aug 25, 2021

ywangd Aug 25, 2021

ywangd left a comment

williamrandolph commented Aug 25, 2021

williamrandolph commented Aug 25, 2021

ywangd left a comment

williamrandolph commented Aug 26, 2021

tvernum commented Aug 26, 2021

Allow access to restricted system indices for reserved system roles #76845

Allow access to restricted system indices for reserved system roles #76845

Conversation

williamrandolph commented Aug 23, 2021 • edited Loading

elasticmachine commented Aug 23, 2021

elasticmachine commented Aug 23, 2021

tylersmalley commented Aug 24, 2021 • edited Loading

ywangd Aug 24, 2021

Choose a reason for hiding this comment

williamrandolph Aug 24, 2021

Choose a reason for hiding this comment

ywangd Aug 25, 2021

Choose a reason for hiding this comment

tvernum commented Aug 24, 2021

williamrandolph commented Aug 25, 2021

ywangd Aug 25, 2021

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

williamrandolph commented Aug 25, 2021

williamrandolph commented Aug 25, 2021

ywangd left a comment

Choose a reason for hiding this comment

williamrandolph commented Aug 26, 2021

tvernum commented Aug 26, 2021

williamrandolph commented Aug 23, 2021 •

edited

Loading

tylersmalley commented Aug 24, 2021 •

edited

Loading