-
Notifications
You must be signed in to change notification settings - Fork 272
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
Fix issue with Get mappings on a Closed index #4685
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4685 +/- ##
==========================================
- Coverage 65.20% 65.20% -0.01%
==========================================
Files 318 318
Lines 22327 22328 +1
Branches 3591 3593 +2
==========================================
Hits 14558 14558
Misses 5971 5971
- Partials 1798 1799 +1
|
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@@ -829,12 +829,12 @@ private Set<String> getResolvedIndexPattern(User user, IndexNameExpressionResolv | |||
.toArray(String[]::new); | |||
|
|||
if (aliasesForPermittedPattern.length > 0) { | |||
resolved = resolver.concreteIndexNames(cs.state(), IndicesOptions.lenientExpandOpen(), aliasesForPermittedPattern); | |||
resolved = resolver.concreteIndexNames(cs.state(), IndicesOptions.lenientExpand(), aliasesForPermittedPattern); |
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.
Don't merge this PR yet, I am planning to push a commit to pass a boolean param in this method to includeClosed
with default value of false. I plan to pass this param as true
when computing the EvaluatedDlsFlsConfig.
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.
Pushed a change to pass includeClosed
as a param to ConfigV7.getResolvedIndexPattern
so that the caller can determine how they want the resolution to proceed.
It makes sense to include Closed when computing the EvaluatedFlsDlsConfig, but should not break other use-cases.
For instance if you have 3 indices:
- logs-1 - open
- logs-2 - open
- logs-3 - closed
When you search on logs-*
the current behavior (need to double check) will only resolve to logs-1
and logs-2
when ultimately performing the search so logs-3
is excluded. If this expansion mode is changed, then it might include logs-3
in the resolution which it currently does not. I will write an integ test to confirm this search scenario.
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.
Its a bit more complicated then this. The security plugin has a setting to preserve the indices options from the original request, but it is disabled by default. See this section of IndexResolverReplacer.
There's a few places in the security plugin that resolve patterns -> concrete indices:
- On the original request, if the request contains index patterns then these will get resolved to concrete indices for authorization
- When looping through roles, it will extract all patterns associated with index_permissions and resolve these to concrete indices
I think the logic I added into this PR is the right way to go. Consider the closed indices when computing the EvaluatedFlsDlsConfig, but keep the existing IndicesOptions.lenientExpandOpen()
otherwise.
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.
When you search on logs-* the current behavior (need to double check) will only resolve to logs-1 and logs-2 when ultimately performing the search so logs-3 is excluded. If this expansion mode is changed, then it includes logs-3 in the resolution which it currently does not. I will write an integ test to confirm this search scenario.
Thanks, good catch! This is an important scenario to test, DLS/FLS should not run search on closed indices.
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 security plugin does index pattern -> concrete resolution in 2 places.
- When expanding out the
index_permissions
from roles definitions. It expands to concrete indices to determine DLS/FLS/Field Masking rules per concrete index. See ConfigV7.getDlsFls - When determining the indices from a request.
For the second use-case, I actually found a new dynamic setting from the security plugin that I wasn't aware of yet. config.dynamic.respect_request_indices_options
which has a default value of false
. If this value is set to true
it will use the original request's options when expanding concrete indices. For example, if a search request includes expand_wildcards=all
then security would copy this. The default value for security is here.
IndicesOptions.fromOptions(false, true, true, false, true);
For reference, this is the default for a SearchRequest
See the full method here for determining IndicesOptions: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java#L810-L823
Signed-off-by: Craig Perkins <cwperx@amazon.com>
7de38e5
src/main/java/org/opensearch/security/identity/NoopPluginSubject.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
Outdated
Show resolved
Hide resolved
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.
You might need to remove the commits from a different rebase?
Done |
Description
This PR fixes an issue filtering out fields on a GET Mappings request on a closed index. This PR updates the indices options used when getting concrete indices.
Bug fix
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.