Empty GetAliases authorization fix #34444
Conversation
albertzaharovits
added
>bug
v7.0.0
:Security/Authorization
|
Pinging @elastic/es-security |
| // _all | ||
| if (aliasesRequest.aliases().length == 0) { | ||
| aliasesRequest.replaceAliases(NO_INDICES_OR_ALIASES_ARRAY); | ||
| } |
There was a problem hiding this comment.
This is the essence.
There was a problem hiding this comment.
@jasontedor @tvernum I believe this is the root issue!
It should be:
if (indicesRequest instanceof GetAliasesRequest && aliasesRequest.aliases().length == 0) {
aliasesRequest.replaceAliases(NO_INDICES_OR_ALIASES_ARRAY);
}
For the remove index alias action type the alias parameter is not used and replacing it triggers the bug.
| ResolvedIndices resolvedIndices = resolveIndices(request, buildAuthorizedIndices(userNoIndices, GetAliasesAction.NAME)); | ||
| assertThat(resolvedIndices.getLocal(), contains("non_existing")); | ||
| assertThat(request.aliases().length, equalTo(0)); | ||
| assertThat(Arrays.asList(request.indices()), contains("non_existing")); | ||
| assertThat(request.aliases(), arrayContaining(IndicesAndAliasesResolver.NO_INDICES_OR_ALIASES_ARRAY)); |
There was a problem hiding this comment.
Fixing these previous 3 tests : testResolveAliasesWildcardsIndicesAliasesRequestDeleteActionsNoAuthorizedIndices, testResolveAliasesWildcardsGetAliasesRequestNoAuthorizedIndices, testResolveAliasesExclusionWildcardsGetAliasesRequest
also test for the changes introduced herein, so no additional test is required.
| final String[] invalidAliasNames = new String[] { "-alias1", "+alias2", "_alias3", "a#lias", "al:ias", ".", ".." }; | ||
| setupRequestAlias(new Alias(randomFrom(invalidAliasNames))); | ||
| expectThrows(InvalidAliasNameException.class, this::executeTask); | ||
| } |
There was a problem hiding this comment.
This is testing that aliases with names with leading '-' cannot indeed be created so that -* can be safely used as an exclude wildcard (as it presently is).
There was a problem hiding this comment.
LGTM as long as CI is happy, thanks a lot @albertzaharovits for taking care of this!
| @@ -194,12 +192,11 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData | |||
| if (indicesRequest instanceof AliasesRequest) { | |||
| //special treatment for AliasesRequest since we need to replace wildcards among the specified aliases too. | |||
| //AliasesRequest extends IndicesRequest.Replaceable, hence its indices have already been properly replaced. | |||
| assert indicesRequest instanceof IndicesRequest.Replaceable; | |||
There was a problem hiding this comment.
is this new assert needed? after all the following cast will fail if the request is not a Replaceable...
There was a problem hiding this comment.
@javanna I am being extra careful for when AliasesRequest might not extend IndicesRequest.Replaceable . They are both interfaces (so the work is minimal) so it might happen, I guess (I haven't tested).
Are you ok with keeping the assertion?
There was a problem hiding this comment.
but doesn't AliasesRequest extend IndicesRequest.Replaceable ? I think I don't follow. Not a big deal though :)
There was a problem hiding this comment.
okay, I was being paranoid with this assert. I'll remove it.
|
run gradle build tests |
|
test this please |
|
test this please SUCCESS |
2 similar comments
|
test this please SUCCESS |
|
test this please SUCCESS |
|
test this please SUCCESS |
|
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/558/consoleFull passes locally. run gradle build tests |
|
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/633/console run gradle build tests |
* master: (24 commits) ingest: better support for conditionals with simulate?verbose (elastic#34155) [Rollup] Job deletion should be invoked on the allocated task (elastic#34574) [DOCS] .Security index is never auto created (elastic#34589) CCR: Requires soft-deletes on the follower (elastic#34725) re-enable bwc tests (elastic#34743) Empty GetAliases authorization fix (elastic#34444) INGEST: Document Processor Conditional (elastic#33388) [CCR] Add total fetch time leader stat (elastic#34577) SQL: Support pattern against compatible indices (elastic#34718) [CCR] Auto follow pattern APIs adjustments (elastic#34518) [Test] Remove dead code from ExceptionSerializationTests (elastic#34713) A small typo in migration-assistance doc (elastic#34704) ingest: processor stats (elastic#34724) SQL: Implement IN(value1, value2, ...) expression. (elastic#34581) Tests: Add checks to GeoDistanceQueryBuilderTests (elastic#34273) INGEST: Rename Pipeline Processor Param. (elastic#34733) Core: Move IndexNameExpressionResolver to java time (elastic#34507) [DOCS] Force Merge: clarify execution and storage requirements (elastic#33882) TESTING.asciidoc fix examples using forbidden annotation (elastic#34515) SQL: Implement `CONVERT`, an alternative to `CAST` (elastic#34660) ...
This fixes a bug about aliases authorization. That is, a user might see aliases which he is not authorized to see. This manifests when the user is not authorized to see any aliases and the `GetAlias` request is empty which normally is a marking that all aliases are requested. In this case, no aliases should be returned, but due to this bug, all aliases will have been returned.
The bug that this is fixing was introduced in #31952 and was discussed here #31952 (comment) .
The bug is about aliases authorization. A user might see aliases which he is not authorized to see.
This happens when the user is not authorized to see any alias and the
GetAliasrequest is empty which is a marking that all aliases are requested. No aliases should be returned in the response but, due to this bug, all aliases are woefully returned.@javanna When this is merged I will investigate removing
originalAliasesfrom theGetAliasrequest.