System indices treated as restricted indices #74212
Conversation
System indices should be treated as a special set of indices and not be accessible by all users. The existing security codebase has the notion of restricted indices, which are currently a subset of system indices. This change unifies the two concepts by making system indices the set of restricted indices. This means that going forward, consumers of system indices will need access to restricted indices. Closes elastic#69298
This reverts commit b12116b.
|
@elasticmachine run elasticsearch-ci/part-1 |
jaymode
added
:Core/Infra/Core
elasticmachine
added
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Pinging @elastic/es-security (Team:Security) |
There was a problem hiding this comment.
This makes a lot of sense to me.
Much of the code has the goal of replacing a hard-coded list of restricted indices with one that's constructed at node startup. IndexNameExpressionResolver supplies the Security plugin with an automaton that matches all system names, and this automaton is passed to the CompositeRolesStore. The CompositeRolesStore, in turn, constructs every IndicesPermission.Group with a reference to this automaton.
When the application is running, the check for restricted indices happens in the IndicesPermission#checkResourcePrivileges method. (I'm glad that there seems to be one place that handles all of the privilege checking: less potential for bugs.)
Test coverage is good, although I don't see that we have extended test coverage beyond the previous group of restricted indices (security- and async-search-related). I'd like to look at whether we can test that other system indices are treated as restricted.
Since I'm taking this work over from Jay, I will work with Gordon to get this PR merged.
...n/core/src/main/java/org/elasticsearch/xpack/core/security/index/RestrictedIndicesNames.java
Show resolved
Hide resolved
client/rest-high-level/roles.yml
Outdated
| @@ -3,6 +3,7 @@ admin: | |||
| - all | |||
| indices: | |||
| - names: '*' | |||
| allow_restricted_indices: true | |||
There was a problem hiding this comment.
We should give the clients team a heads up about this before we merge; otherwise we might mysteriously break their tests.
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
Outdated
Show resolved
Hide resolved
...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java
Show resolved
Hide resolved
...ity/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java
Outdated
Show resolved
Hide resolved
...ity/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java
Show resolved
Hide resolved
|
@tvernum and @albertzaharovits - @williamrandolph is going to address some comments and get @gwbrown to review them. However, a broader security review would be appreciated. |
You know, the update here was part of the work that Jay did originally, and I'm not sure what breaks if we take it out. I'm going to back out some changes to the reserved roles to see where tests break and get you a better answer to this question.
That's a good idea, and I'll make an issue for it as a follow-up. |
|
I spoke to Jay the other day and he didn't have a reason for allowing restricted indices on all of those feature and application roles. We have plenty of test coverage and it all still passes, so I'm going to assume that the "allow restricted indices" change was made by mistake. I still need to make the issue for a deprecation warning on user roles that try to set access on system indices, but after that I think we should be ready to merge.
|
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.
…76845) * Add system index patterns to TestRestrictedIndices A missing piece in #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. * Preserve existing Kibana data telemetry privileges * Test that data telemetry can't access security and async indices
Since elastic#74212, all system indices are now treated as restricted indices, which includes the fleet system indices. As a result, the fleet-server server account needs privileges to access restricted indices under the fleet-* namespace. Relates: elastic#74212
System indices should be treated as a special set of indices and not be
accessible by all users. The existing security codebase has the notion
of restricted indices, which are currently a subset of system indices.
This change unifies the two concepts by making system indices the set
of restricted indices. This means that going forward, consumers of
system indices will need access to restricted indices.
Closes #69298