Skip to content
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

_cat/indices with Security, hide names when wildcard #38824

Merged

Conversation

albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented Feb 12, 2019

This changes the output of the _cat/indices API with Security enabled.

Previously, it was It is possible to only display the index name (and possibly the index health, depending on the options) but not its stats (doc count, merges, size, etc). However, under usual circumstances, it was not possible to actually observe this, maybe just under a transient state (the index name should exist, but with no shards assigned). This is the case for closed indices which have index metadata in the cluster state but no associated shards, hence no shard stats. However, when Security is enabled, and the request contains wildcards, open indices without stats are a common occurrence. This is because the index names in the response table are picked up directly from the cluster state which is not filtered by Security's indexNameExpressionResolver, unlike the
stats data which is populated by the indices stats API which does go through the index name resolver.
This is a bug, because it is circumventing Security's function to hide unauthorized indices.

This has been fixed by displaying the index names as they are resolved by the indices stats API. The outputs of these two APIs is now very similar: same index names , similar data but different format.

Closes #37190

Even though the main code changes went into the server code, they impact the Security plugin, so I have labeled it in consequence.

cc @elastic/es-core-features

@albertzaharovits albertzaharovits added >bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.2.0 labels Feb 12, 2019
@albertzaharovits albertzaharovits self-assigned this Feb 12, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

assert concreteIndices.length == state.metaData().getIndices().size();

final ClusterState clusterState = clusterStateResponse.getState();
final IndexMetaData[] indicesMetaData = getOrderedIndexMetaData(indices, clusterState, strictExpandIndicesOptions);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the code suggests, indexNameExpressionResolver.concreteIndices should probably return the resolved indices expression in the order that they were requested. It does not appear to be the case now, but I nevertheless kept the logic based on this assumption in the spirit of introducing a single change in a PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we put indices in a set I think, then ordering is not deterministic

@@ -170,4 +188,19 @@ private IndicesStatsResponse randomIndicesStatsResponse(final Index[] indices) {
shardStats.toArray(new ShardStats[shardStats.size()]), shardStats.size(), shardStats.size(), 0, emptyList()
);
}

private <T> T[] removeRandomElement(T[] array) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you can use a List and avoid all the ceremony need to remove an element from an array?

assert concreteIndices.length == state.metaData().getIndices().size();

final ClusterState clusterState = clusterStateResponse.getState();
final IndexMetaData[] indicesMetaData = getOrderedIndexMetaData(indices, clusterState, strictExpandIndicesOptions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we put indices in a set I think, then ordering is not deterministic

@albertzaharovits
Copy link
Contributor Author

@javanna FYI after checking on the test failures I realized we need a special case for closed indices.
These have metadata but no shards, hence no shard stats, so they were falling in the "unauthorized" indices case, but they have to be displayed. I have pushed 137ad57 .

@javanna
Copy link
Member

javanna commented Feb 14, 2019

sounds good @albertzaharovits thanks for pinging

@albertzaharovits albertzaharovits merged commit 7b907c1 into elastic:master Feb 14, 2019
@albertzaharovits albertzaharovits deleted the fix-cat-indices-with-security branch February 14, 2019 10:50
albertzaharovits added a commit that referenced this pull request Feb 14, 2019
This changes the output of the `_cat/indices` API with `Security` enabled.

It is possible to only display the index name (and possibly the index
health, depending on the request options) but not its stats (doc count, merges,
size, etc). This is the case for closed indices which have index metadata in the
cluster state but no associated shards, hence no shard stats.
However, when `Security` is enabled, and the request contains wildcards,
**open** indices without stats are a common occurrence. This is because the
index names in the response table are picked up directly from the cluster state
which is not filtered by `Security`'s _indexNameExpressionResolver_, unlike the
stats data which is populated by the indices stats API which does go through the
index name resolver.
This is a bug, because it is circumventing `Security`'s function to hide
unauthorized indices.

This has been fixed by displaying the index names as they are resolved by the indices
stats API. The outputs of these two APIs is now very similar: same index names,
similar data but different format.

Closes #37190
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 14, 2019
* elastic/master:
  Remove immediate operation retry after mapping update (elastic#38873)
  Remove mentioning of types from bulk API docs (elastic#38896)
  SQL: change JDBC setup URL in the documentation (elastic#38564)
  Skip BWC tests in checkPart1 and checkPart2 (elastic#38730)
  Enable silent FollowersCheckerTest (elastic#38851)
  Update TESTING.asciidoc with platform specific instructions (elastic#38802)
  Use consistent view of realms for authentication (elastic#38815)
  Stabilize RareClusterState (elastic#38671)
  Increase Timeout in UnicastZenPingTests (elastic#38893)
  Do not recommend installing vagrant-winrm elastic#38887
  _cat/indices with Security, hide names when wildcard (elastic#38824)
  SQL: fall back to using the field name for column label (elastic#38842)
  Fix LocalIndexFollowingIT#testRemoveRemoteConnection() test (elastic#38709)
  Remove joda time mentions in documentation (elastic#38720)
  Add enabled status for token and api key service (elastic#38687)
tlrx added a commit that referenced this pull request Jun 25, 2019
After two recent changes (#38824 and #33888), the _cat/indices API
no longer report information for active recovering indices and 
non-replicated closed indices. It also misreport replicated closed 
indices that are potentially not authorized for the user.

This commit changes how the cat action works by first using the 
Get Settings API in order to resolve authorized indices. It then uses 
the Cluster State, Cluster Health and Indices Stats APIs to retrieve
 information about the indices.

Closes #39933
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Jun 25, 2019
After two recent changes (elastic#38824 and elastic#33888), the _cat/indices API
no longer report information for active recovering indices and
non-replicated closed indices. It also misreport replicated closed
indices that are potentially not authorized for the user.

This commit changes how the cat action works by first using the
Get Settings API in order to resolve authorized indices. It then uses
the Cluster State, Cluster Health and Indices Stats APIs to retrieve
 information about the indices.

Closes elastic#39933
tlrx added a commit that referenced this pull request Jun 25, 2019
After two recent changes (#38824 and #33888), the _cat/indices API
no longer report information for active recovering indices and
non-replicated closed indices. It also misreport replicated closed
indices that are potentially not authorized for the user.

This commit changes how the cat action works by first using the
Get Settings API in order to resolve authorized indices. It then uses
the Cluster State, Cluster Health and Indices Stats APIs to retrieve
 information about the indices.

Closes #39933
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Jun 26, 2019
After two recent changes (elastic#38824 and elastic#33888), the _cat/indices API
no longer report information for active recovering indices and
non-replicated closed indices. It also misreport replicated closed
indices that are potentially not authorized for the user.

This commit changes how the cat action works by first using the
Get Settings API in order to resolve authorized indices. It then uses
the Cluster State, Cluster Health and Indices Stats APIs to retrieve
information about the indices.

Closes elastic#39933
tlrx added a commit that referenced this pull request Jun 26, 2019
After two recent changes (#38824 and #33888), the _cat/indices API
no longer report information for active recovering indices and
non-replicated closed indices. It also misreport replicated closed
indices that are potentially not authorized for the user.

This commit changes how the cat action works by first using the
Get Settings API in order to resolve authorized indices. It then uses
the Cluster State, Cluster Health and Indices Stats APIs to retrieve
information about the indices.

Closes #39933
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

403 for no index permission from _cat/indices call
4 participants