-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Mirror privileges over data streams to their backing indices #58381
Mirror privileges over data streams to their backing indices #58381
Conversation
…quested index is a backing index
Pinging @elastic/es-security (:Security/Authorization) |
Pinging @elastic/es-core-features (:Core/Features/Data streams) |
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core change looks good to me, but I'd like to sit on it a bit more. I'll get back to you tomorrow.
@danhermann and I had a productive brainstorm session yesterday. We worked on specifying the Security behaviour when the issued request (which might or might not "support data streams") contains wildcards (which Security must replace with concrete names from the authorised set, given the user's roles) and when the roles grant access to a data stream (or a data stream wildcard) compared to explicit backing indices. As a recap, we wish that when a role grants access to a namespace that covers an existing data stream it implicitly grants access to the backing indices of that data stream as well (but not the other way around, i.e. if the role grants access to some/all of the backing indices of a data stream it does NOT implicitly grant access to the data stream). This must work for all types of "index" privileges, i.e. indices:data* (read, index, delete, write, create, create_doc) and indices:admin* (view_index_metadata, manage, monitor, maintenance). It's worth noting that this mirroring of privileges from the data stream to the backing index only happens if the data stream and the backing index exists in the first place, i.e. if the permission to create an index (or delete, or manage_follower/leader_index) is granted over some namespace it is NOT mirrored over the equivalent backing indices namespace (.ds-). We went over the following scenarios:
We discussed implementation as well, but I was not able to get my head around all that enough to recommend an approach. I now believe we should change Line 503 in e33a0df
Line 216 in e33a0df
indexAbstraction s that although are not covered by the permission they are backing indices of data streams covered as such. Lastly, we should also modify isIndexVisible to hide concrete data streams when the request's wildcard covers one, but data streams are not supported for the particular request.
I only have one clarifying question: does |
Yes, if the index abstraction is of type data stream then it returns the corresponding backing indices. |
@elasticmachine update branch |
@albertzaharovits, I've implemented the security integration as you suggested above and added unit tests. Also, I confirmed that along with the changes from #57900, this will resolve #57712. I added a single override to If this matches what you wanted, I'll add some additional higher-level tests that validate all the new scenarios we discussed earlier. I did notice one oddity in some manual testing that I was doing. If I create a user with a role limited to index read privileges on the pattern |
@neptunian @ph As soon as this gets merged, we need to clean up the |
@albertzaharovits, let me know if you have any test suggestions here. I tried to include unit tests targeted narrowly at the behavioral changes in the three methods that were changed as well as top-level end-to-end tests. For the latter, I didn't see a good place to add those except for the REST tests. |
@elasticmachine update branch |
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think the tests you've added are minimal but good enough.
I've left a couple of test suggestions. I'm not insisting over them because the missing pieces are covered across separate test classes; the issue is we're really not methodical with tests here, and so the burden of structuring testing in this area should not befall on you.
IndexAbstraction indexAbstraction = metadata.getIndicesLookup().get(index); | ||
if (indexAbstraction == null) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make it an IllegalStateException
.
@@ -35,7 +37,7 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to test that RBACEngine.resolveAuthorizedIndicesFromRole:
- returns the list containing the backing indices if the role grants access to a data stream (granting as part of a wildcard or a simple name)
- indices which are not backing indices are not returned when some data stream is covered by the role permission
- granting access to backing indices (as a wildcard or simple name) does not include the data stream in the list of authorised names
We need to repeat the tests for when the request works with data streams or not.
final List<String> authorizedIndices = buildAuthorizedIndices(user, GetAliasesAction.NAME, request); | ||
ResolvedIndices resolvedIndices = defaultIndicesResolver.resolveIndicesAndAliases(request, metadata, authorizedIndices); | ||
assertThat(resolvedIndices.getLocal(), not(hasItem("logs-foo"))); | ||
assertThat(resolvedIndices.getLocal(), not(hasItem("logs-foobar"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no backing indices as well.
But regular indices and aliases are visible. Same with backing indices.
(I would expand the namespace granted to by the role to ensure those entities are OK - i.e. I'm not suggesting another test, just a more complete one).
But I would also test a simple (not wildcard) data stream name.
final List<String> authorizedIndices = buildAuthorizedIndices(user, SearchAction.NAME, request); | ||
ResolvedIndices resolvedIndices = defaultIndicesResolver.resolveIndicesAndAliases(request, metadata, authorizedIndices); | ||
assertThat(resolvedIndices.getLocal(), hasItem("logs-foo")); | ||
assertThat(resolvedIndices.getLocal(), hasItem("logs-foobar")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should include the backing indices as well, right?
Before merging, can you work on the description and the title as well? Eg "Mirror privileges over data stream to their backing indices" Also explain the wildcard behaviour for requests that don't work with |
Thank you, @albertzaharovits. I'll make the changes you requested. I'm also happy to work on additional tests in a follow-up if there are others that you would like to have. |
This PR comprises the core of data streams security integration. It changes the authorization code to extend any privileges granted on a data stream to all of its backing indices. It also introduces an
includeDataStreams()
flag on any requests implementingIndicesRequest
so that they can indicate how data streams should be considered during the authz process for each request. For requests whereincludeDataStreams()
isfalse
, authz will not include any data streams in the list of authorized indices for that request. For requests whereincludeDataStreams()
istrue
, authz will include any matching data streams along with their respective backing indices in the list of authorized indices.Note that @albertzaharovits's comment below (#58381 (comment)) includes another description of the way in which the authz code expands wildcards for requests that include data streams and those that do not.
Relates to #53100