Remove implicit index monitor privilege #37774
Conversation
albertzaharovits
added
>breaking
v7.0.0
:Security/Authorization
|
Pinging @elastic/es-security |
|
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-1/5678/consoleFull
@elasticmachine run elasticsearch-ci/1 |
There was a problem hiding this comment.
Overall the changes look good. One thing I'd like to see though is a integration test with monitoring APIs that relate to indices and ensuring that they work if the user doesn't specify a specific index and does not have allow restricted indices set to true. During wildcard resolution we should be filtering out the indices without permissions but I think there were some APIs where we couldn't do that.
|
@jaymode Hey Jay, I have added the Integ Test for the index monitoring priv, but I have not noticed anything fishy. Can you take a another look please? |
| .endObject()); | ||
| assertEquals(DocWriteResponse.Result.CREATED, indexResponse.getResult()); | ||
|
|
||
| refresh(); |
There was a problem hiding this comment.
I think we need to make sure the internal security indices are created here. Maybe we create a user and a role and validate that the security indices exist
There was a problem hiding this comment.
We'll also want to look at testing the cluster health api at the indices and shards levels. Same for the cluster stats api
There was a problem hiding this comment.
Cluster Health and Stats are authorized as cluster actions cluster:monitor/* and will see all indices irrespective of the change proposed here.
There was a problem hiding this comment.
I went back into the time machine and figured out the API that this leniency for monitoring came from. It is the _cat/indices API. I am pretty sure it will still fail based on the way it works so I think we should:
- add a test and validate cat indices will cause issues if the security index exists and the user is not allowed access
- find a fix
There was a problem hiding this comment.
We dug into this over slack.
Jay pointed me to the commit that added this leniency and we identified that it was no longer relevant as the cause got inadvertently removed by #18545 .
As discussed, I have added a test for _cat/indices/* to validate that no unauthorized exception is thrown (when the wildcard includes the unauthorized .security index).
| @@ -127,6 +141,64 @@ public void testSingleRole() throws Exception { | |||
| assertHitCount(searchResponse, 1); | |||
| } | |||
|
|
|||
| public void testMonitorRestrictedWildcards() throws Exception { | |||
|
|
|||
| assertSecurityIndexActive(); | |||
There was a problem hiding this comment.
unfortunately this does not do what you want it to do. This will pass if the index does not exist.
There was a problem hiding this comment.
Good catch!
|
@jaymode I got this in the state we talked about. May you take a look, please? |
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine run elasticsearch-ci/2 |
1 similar comment
|
@elasticmachine run elasticsearch-ci/2 |
|
🎲 @elasticmachine run elasticsearch-ci/1 |
|
🎲 @elasticmachine run elasticsearch-ci/2 |
Follow-up on #37577 (comment)
Restricted indices (currently only
.security-6and.security) are special internalindices that require setting the
allow_restricted_indicesflag on every indexpermission that covers them. If this flag is
false(default) the permissionwill not cover these and actions against them will not be authorized.
However, the monitoring APIs were the only exception to this rule.
This exception is herein forfeited and index monitoring privileges have to be
granted explicitly, using the
allow_restricted_indicesflag on the permission,as is the case for any other index privilege.