- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
Failure Store Access Authorization #123986
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
Failure Store Access Authorization #123986
Conversation
…g/elasticsearch into read-failure-store-privilege-authz
        
          
                docs/changelog/123986.yaml
              
                Outdated
          
        
      | @@ -0,0 +1,5 @@ | |||
| pr: 123986 | |||
| summary: Failure Store Access Authorization | |||
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.
Eh, actually I'll remove this and turn this into a non-issue. There are follow ups to complete this and besides the failure store itself isn't available yet so it's not really an enhancement.
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 This turned out pretty neat! Nice job on integration tests. 👍
I've left couple of optional suggestions and a few non-blocking questions. Two main questions that I think we should discuss (and optionally address in the followup) are:
- Should 
read_failure_storeinclude the same automaton asread? I think we can drop theindices:admin/resolve/clustersince CCS/CCR are not supported in the MVP. - Do we want to allow that having 
read_failure_storeover.fs*grants read access to failure store backing indices (just like read)? In theory we could support it. 
| // * Indices don't have failure components so the failure component is not visible | ||
| 
               | 
          ||
| assertThat(isIndexVisible("data-stream1", null), is(true)); | ||
| assertThat(isIndexVisible("data-stream1", "*"), is(true)); | 
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.
This seems to be leftover from the #121900 PR, right?
Looking closer, these tests seem to pass with any string value for the selector, except the failures.
Not something we should do in this PR, but I wonder if we should validate the selector inside the isIndexVisible method and only accept null, failures or data?
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.
Yup, I'll note this down to look into in a follow up.
        
          
                ...ugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java
          
            Show resolved
            Hide resolved
        
              
          
                .../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | // Remove any selectors from abstraction name. Discard them for this check as we do not have access control for them (yet) | ||
| // Remove any selectors from abstraction name. Access control is based on the `selector` field of the IndexResource | ||
| Tuple<String, String> expressionAndSelector = IndexNameExpressionResolver.splitSelectorExpression(indexOrAlias); | ||
| indexOrAlias = expressionAndSelector.v1(); | 
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.
Sounds good to me!
        
          
                ...in/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizedIndicesTests.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | assertThat(authorizedIndices.check(SecuritySystemIndices.SECURITY_MAIN_ALIAS, IndexComponentSelector.DATA), is(false)); | ||
| } | ||
| 
               | 
          ||
| public void testDataStreamsAreNotIncludedInAuthorizedIndicesWithFailuresSelector() { | 
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.
Some more unit tests we could consider adding (can be done in a followup PR):
- having 
read_failure_storeover.*andallow_restricted_indices=truedoes not grant access to system indices - having  
read_failure_storeover.fs*indices does not grant read access to failure store indices (depends on the decision if we actually want to allow this - see my other comment) - having 
read_failure_storeover*grants read access to datastreams with their failure store backing indices - having 
manage_failure_storeover*,.fs*or datastream name does not grant access forTransportSearchAction.TYPE.name()but it should forTransportDeleteIndexAction.TYPE.name() - in general,  more test coverage for 
resolveAuthorizedIndicesFromRoleand e.g.TransportDeleteIndexActionwith roles that grantmanage_failure_storeandmanage 
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.
Absolutely. Adding these in a follow up.
        
          
                ...in/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizedIndicesTests.java
          
            Show resolved
            Hide resolved
        
      | entry("cross_cluster_replication_internal", CROSS_CLUSTER_REPLICATION_INTERNAL), | ||
| DataStream.isFailureStoreFeatureFlagEnabled() ? entry("read_failure_store", READ_FAILURE_STORE) : null | ||
| ).filter(Objects::nonNull).collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, Map.Entry::getValue)) | ||
| private static final Map<String, IndexPrivilege> VALUES = combineSortedInOrder( | 
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.
Thanks for the explanation!
| ); | ||
| } | ||
| 
               | 
          ||
| // TODO remove me (this has >700 usages in tests which would make for a horrible diff; will remove this once the main PR is merged) | 
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.
+1
        
          
                ...core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      …g/elasticsearch into read-failure-store-privilege-authz
…g/elasticsearch into read-failure-store-privilege-authz
| 
           🥳  | 
    
          💚 All backports created successfully
 Questions ?Please refer to the Backport tool documentation  | 
    
This PR implements authorization logic for failure store access. It builds on elastic#122715. Access to the failure store is granted by two privileges: `read_failure_store` and `manage_failure_store`. Either of these privileges lets a user access a failure store via the `::failures` selector, as well as access its backing failure indices. `read_failure_store` grants read access (for example to search documents in a failure store), `manage_failure_store` grants access to write operations, such as rollover. Users with only `read` or `manage` on a data stream do not get failure store access. Vice versa, users with `read_failure_store` and `manage_failure_store` do not get access to regular data in a data stream. The PR implements this by making authorization logic selector-aware. It involves two main changes: 1. Index permission groups now compare the selector under which an index resource is accessed to the selector associated with the group. 2. The `AuthorizedIndices` interface likewise uses selectors to decide which indices to treat as authorized. This part of the change requires a sizable refactor and changes to the interface. The high-level behavior for selector-aware search is as follows: For a user with `read_failure_store` over data stream `logs`: - `POST /logs::failures/_search` returns the documents in the failure store. - `POST /logs/_search` returns a 403. - `POST /logs/_search?ignore_unavailable=true` and `POST /*/_search` return an empty result. Similarly, for a user with `read` over data stream `logs`: - `POST /logs::failures/_search` returns a 403. - `POST /logs/_search` returns documents in the data stream. - `POST /logs::failures/_search?ignore_unavailable=true` and `POST /*::failures/_search` return an empty result. A user with both `read` and `read_failure_store` over data stream `logs` gets access to both `POST /logs::failures/_search` and `POST /logs/_search`. The index privilege `all` automatically grants access to both data and the failures store, as well as all hypothetical future selectors. Resolves: ES-10873
This PR implements authorization logic for failure store access. It builds on elastic#122715. Access to the failure store is granted by two privileges: `read_failure_store` and `manage_failure_store`. Either of these privileges lets a user access a failure store via the `::failures` selector, as well as access its backing failure indices. `read_failure_store` grants read access (for example to search documents in a failure store), `manage_failure_store` grants access to write operations, such as rollover. Users with only `read` or `manage` on a data stream do not get failure store access. Vice versa, users with `read_failure_store` and `manage_failure_store` do not get access to regular data in a data stream. The PR implements this by making authorization logic selector-aware. It involves two main changes: 1. Index permission groups now compare the selector under which an index resource is accessed to the selector associated with the group. 2. The `AuthorizedIndices` interface likewise uses selectors to decide which indices to treat as authorized. This part of the change requires a sizable refactor and changes to the interface. The high-level behavior for selector-aware search is as follows: For a user with `read_failure_store` over data stream `logs`: - `POST /logs::failures/_search` returns the documents in the failure store. - `POST /logs/_search` returns a 403. - `POST /logs/_search?ignore_unavailable=true` and `POST /*/_search` return an empty result. Similarly, for a user with `read` over data stream `logs`: - `POST /logs::failures/_search` returns a 403. - `POST /logs/_search` returns documents in the data stream. - `POST /logs::failures/_search?ignore_unavailable=true` and `POST /*::failures/_search` return an empty result. A user with both `read` and `read_failure_store` over data stream `logs` gets access to both `POST /logs::failures/_search` and `POST /logs/_search`. The index privilege `all` automatically grants access to both data and the failures store, as well as all hypothetical future selectors. Resolves: ES-10873
This PR implements authorization logic for failure store access. It builds on elastic#122715. Access to the failure store is granted by two privileges: `read_failure_store` and `manage_failure_store`. Either of these privileges lets a user access a failure store via the `::failures` selector, as well as access its backing failure indices. `read_failure_store` grants read access (for example to search documents in a failure store), `manage_failure_store` grants access to write operations, such as rollover. Users with only `read` or `manage` on a data stream do not get failure store access. Vice versa, users with `read_failure_store` and `manage_failure_store` do not get access to regular data in a data stream. The PR implements this by making authorization logic selector-aware. It involves two main changes: 1. Index permission groups now compare the selector under which an index resource is accessed to the selector associated with the group. 2. The `AuthorizedIndices` interface likewise uses selectors to decide which indices to treat as authorized. This part of the change requires a sizable refactor and changes to the interface. The high-level behavior for selector-aware search is as follows: For a user with `read_failure_store` over data stream `logs`: - `POST /logs::failures/_search` returns the documents in the failure store. - `POST /logs/_search` returns a 403. - `POST /logs/_search?ignore_unavailable=true` and `POST /*/_search` return an empty result. Similarly, for a user with `read` over data stream `logs`: - `POST /logs::failures/_search` returns a 403. - `POST /logs/_search` returns documents in the data stream. - `POST /logs::failures/_search?ignore_unavailable=true` and `POST /*::failures/_search` return an empty result. A user with both `read` and `read_failure_store` over data stream `logs` gets access to both `POST /logs::failures/_search` and `POST /logs/_search`. The index privilege `all` automatically grants access to both data and the failures store, as well as all hypothetical future selectors. Resolves: ES-10873
This PR implements authorization logic for failure store access. It builds on #122715.
Access to the failure store is granted by two privileges:
read_failure_storeandmanage_failure_store. Either of these privileges lets a user access a failure store via the::failuresselector, as well as access its backing failure indices.read_failure_storegrants read access (for example to search documents in a failure store),manage_failure_storegrants access to write operations, such as rollover. Users with onlyreadormanageon a data stream do not get failure store access. Vice versa, users withread_failure_storeandmanage_failure_storedo not get access to regular data in a data stream.The PR implements this by making authorization logic selector-aware. It involves two main changes:
AuthorizedIndicesinterface likewise uses selectors to decide which indices to treat as authorized. This part of the change requires a sizable refactor and changes to the interface.The high-level behavior for selector-aware search is as follows:
For a user with
read_failure_storeover data streamlogs:POST /logs::failures/_searchreturns the documents in the failure store.POST /logs/_searchreturns a 403.POST /logs/_search?ignore_unavailable=trueandPOST /*/_searchreturn an empty result.Similarly, for a user with
readover data streamlogs:POST /logs::failures/_searchreturns a 403.POST /logs/_searchreturns documents in the data stream.POST /logs::failures/_search?ignore_unavailable=trueandPOST /*::failures/_searchreturn an empty result.A user with both
readandread_failure_storeover data streamlogsgets access to bothPOST /logs::failures/_searchandPOST /logs/_search.The index privilege
allautomatically grants access to both data and the failures store, as well as all hypothetical future selectors.Resolves: ES-10873