Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
Expand Down Expand Up @@ -47,6 +48,7 @@
import java.util.stream.Collectors;

import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.is;
Expand Down Expand Up @@ -1388,12 +1390,8 @@ public void testDlsFls() throws Exception {
Map.of(dataIndexName, Set.of("@timestamp", "age"))
);

// FLS sort of applies to failure store
// TODO this will change with FLS handling
assertSearchResponseContainsExpectedIndicesAndFields(
performRequest(user, new Search("test1::failures").toSearchRequest()),
Map.of(failureIndexName, Set.of("@timestamp"))
);
// FLS does not apply to failure store
expectFlsDlsError(() -> performRequest(user, new Search("test1::failures").toSearchRequest()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've hit the ready button too soon. I still want to test cover the case when users have direct access to .fs-* indices and accessing without ::failures selector.


upsertRole(Strings.format("""
{
Expand Down Expand Up @@ -1422,12 +1420,8 @@ public void testDlsFls() throws Exception {
Map.of(dataIndexName, Set.of("@timestamp", "age"))
);

// FLS sort of applies to failure store
// TODO this will change with FLS handling
assertSearchResponseContainsExpectedIndicesAndFields(
performRequest(user, new Search("test1::failures").toSearchRequest()),
Map.of(failureIndexName, Set.of("@timestamp"))
);
// FLS does not apply to failure store
expectFlsDlsError(() -> performRequest(user, new Search("test1::failures").toSearchRequest()));

upsertRole("""
{
Expand Down Expand Up @@ -1455,7 +1449,21 @@ public void testDlsFls() throws Exception {

assertSearchResponseContainsExpectedIndicesAndFields(
performRequest(user, new Search("test1::failures").toSearchRequest()),
Map.of(failureIndexName, Set.of("@timestamp", "document", "error"))
Map.of(
failureIndexName,
Set.of(
"@timestamp",
"document.id",
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, do you know why this changed? I.e., why are we getting flattened fields now, instead of top-level field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the test assertion to flatten the results in order to make sure that FLS is properly respected for nested fields. Previously we were only asserting the top level fields.
I had a test which granted document.source.name over .fs* indices that was succeeding. This was before the change to prevent direct access as well. I can revert that if you think it's not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, this was an old comment I forgot to delete -- plz ignore, this is a good improvement

"document.index",
"document.source.@timestamp",
"document.source.age",
"document.source.email",
"document.source.name",
"error.message",
"error.stack_trace",
"error.type"
)
)
);

// DLS
Expand All @@ -1473,7 +1481,8 @@ public void testDlsFls() throws Exception {
}""", role);
// DLS applies and no docs match the query
expectSearch(user, new Search(randomFrom("test1", "test1::data")));
expectSearch(user, new Search("test1::failures"));
// DLS is not applicable to failure store
expectFlsDlsError(() -> performRequest(user, new Search("test1::failures").toSearchRequest()));

upsertRole("""
{
Expand All @@ -1488,7 +1497,8 @@ public void testDlsFls() throws Exception {
}""", role);
// DLS applies and doc matches the query
expectSearch(user, new Search(randomFrom("test1", "test1::data")), dataIndexDocId);
expectSearch(user, new Search("test1::failures"));
// DLS is not applicable to failure store
expectFlsDlsError(() -> performRequest(user, new Search("test1::failures").toSearchRequest()));

upsertRole("""
{
Expand All @@ -1507,6 +1517,57 @@ public void testDlsFls() throws Exception {
}""", role);
// DLS does not apply because there is a section without DLS
expectSearch(user, new Search(randomFrom("test1", "test1::data")), dataIndexDocId);

// check that direct access to backing failure store indices is not allowed
upsertRole(Strings.format("""
{
"cluster": ["all"],
"indices": [
{
"names": ["%s"],
"privileges": ["read"],
"field_security": {
"grant": ["@timestamp", "age"]
}
},
{
"names": ["%s"],
"privileges": ["read"],
"field_security": {
"grant": ["@timestamp", "document.source.name"]
}
}
]
}""", dataIndexName, failureIndexName), role);

// FLS applies to backing data index
assertSearchResponseContainsExpectedIndicesAndFields(
performRequest(user, new Search(dataIndexName).toSearchRequest()),
Map.of(dataIndexName, Set.of("@timestamp", "age"))
);
assertSearchResponseContainsExpectedIndicesAndFields(
performRequest(user, new Search(".ds-*").toSearchRequest()),
Map.of(dataIndexName, Set.of("@timestamp", "age"))
);
// FLS is not applicable to backing failure store indices
expectFlsDlsError(() -> performRequest(user, new Search(failureIndexName).toSearchRequest()));
expectFlsDlsError(() -> performRequest(user, new Search(".fs-*").toSearchRequest()));

// DLS is not applicable to backing failure store, even when granted directly
upsertRole(Strings.format("""
{
"cluster": ["all"],
"indices": [
{
"names": ["%s"],
"privileges": ["read"],
"query":{"term":{"name":{"value":"jack"}}}
}
]
}""", failureIndexName), role);
expectFlsDlsError(() -> performRequest(user, new Search(failureIndexName).toSearchRequest()));
expectFlsDlsError(() -> performRequest(user, new Search(".fs-*").toSearchRequest()));

}

private static void expectThrows(ThrowingRunnable runnable, int statusCode) {
Expand Down Expand Up @@ -1797,7 +1858,7 @@ protected void assertSearchResponseContainsExpectedIndicesAndFields(
assertThat(searchResult.keySet(), equalTo(expectedIndicesAndFields.keySet()));
for (String index : expectedIndicesAndFields.keySet()) {
Set<String> expectedFields = expectedIndicesAndFields.get(index);
assertThat(searchResult.get(index).keySet(), equalTo(expectedFields));
assertThat(Maps.flatten(searchResult.get(index), false, true).keySet(), equalTo(expectedFields));
}
} finally {
response.decRef();
Expand Down Expand Up @@ -1827,4 +1888,14 @@ private Tuple<String, String> getSingleDataAndFailureIndices(String dataStreamNa
assertThat(indices.v2().size(), equalTo(1));
return new Tuple<>(indices.v1().get(0), indices.v2().get(0));
}

private static void expectFlsDlsError(ThrowingRunnable runnable) {
var exception = expectThrows(ResponseException.class, runnable);
assertThat(
exception.getMessage(),
containsString(
"Failure store access is not allowed for users who have field or document level security enabled on one of the indices"
)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.elasticsearch.bootstrap.BootstrapCheck;
import org.elasticsearch.client.internal.Client;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.DataStream;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexTemplateMetadata;
import org.elasticsearch.cluster.metadata.ProjectId;
Expand Down Expand Up @@ -328,6 +329,7 @@
import org.elasticsearch.xpack.security.authz.accesscontrol.OptOutQueryCache;
import org.elasticsearch.xpack.security.authz.interceptor.BulkShardRequestInterceptor;
import org.elasticsearch.xpack.security.authz.interceptor.DlsFlsLicenseRequestInterceptor;
import org.elasticsearch.xpack.security.authz.interceptor.FailureStoreRequestInterceptor;
import org.elasticsearch.xpack.security.authz.interceptor.IndicesAliasesRequestInterceptor;
import org.elasticsearch.xpack.security.authz.interceptor.RequestInterceptor;
import org.elasticsearch.xpack.security.authz.interceptor.ResizeRequestInterceptor;
Expand Down Expand Up @@ -1139,6 +1141,9 @@ Collection<Object> createComponents(
new ValidateRequestInterceptor(threadPool, getLicenseState())
)
);
if (DataStream.isFailureStoreFeatureFlagEnabled()) {
requestInterceptors.add(new FailureStoreRequestInterceptor(clusterService, projectResolver, threadPool, getLicenseState()));
}
}
requestInterceptors = Collections.unmodifiableSet(requestInterceptors);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.security.authz.interceptor;

import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.IndicesRequest;
import org.elasticsearch.action.support.IndexComponentSelector;
import org.elasticsearch.cluster.metadata.IndexAbstraction;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.project.ProjectResolver;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;

import java.util.Map;

public class FailureStoreRequestInterceptor extends FieldAndDocumentLevelSecurityRequestInterceptor {

private final ClusterService clusterService;
private final ProjectResolver projectResolver;

public FailureStoreRequestInterceptor(
ClusterService clusterService,
ProjectResolver projectResolver,
ThreadPool threadPool,
XPackLicenseState licenseState
) {
super(threadPool.getThreadContext(), licenseState);
this.clusterService = clusterService;
this.projectResolver = projectResolver;
}

@Override
void disableFeatures(
IndicesRequest indicesRequest,
Map<String, IndicesAccessControl.IndexAccessControl> indicesAccessControlByIndex,
ActionListener<Void> listener
) {
for (var indexAccessControl : indicesAccessControlByIndex.entrySet()) {
if ((hasFailuresSelectorSuffix(indexAccessControl.getKey()) || isBackingFailureStoreIndex(indexAccessControl.getKey()))
&& hasDlsFlsPermissions(indexAccessControl.getValue())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider flipping this since most roles won't have FLS/DLS so we'll short-circuit quicker

listener.onFailure(
new ElasticsearchSecurityException(
"Failure store access is not allowed for users who have "
+ "field or document level security enabled on one of the indices",
RestStatus.BAD_REQUEST
)
);
return;
}
}
listener.onResponse(null);
}

@Override
boolean supports(IndicesRequest request) {
if (request.indicesOptions().allowSelectors()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want request.indicesOptions().allowSelectors() here as there may be cases where you can access the failure index but selectors are not allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! This started with intention to prevent access with ::failures selector but turned into "let's prevent access to failure indices regardless if selectors are used or not". I'll adjust this.

for (String index : request.indices()) {
if (hasFailuresSelectorSuffix(index) || isBackingFailureStoreIndex(index)) {
return true;
}
}
}
return false;
}

private boolean hasFailuresSelectorSuffix(String name) {
return IndexNameExpressionResolver.hasSelectorSuffix(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use IndexNameExpressionResolver.hasSelector(name, IndexComponentSelector.FAILURES) here instead, to avoid splitting.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Fine to include the bigger check with splitting as an assertion since it does additional validation, but hasSelector should be faster so better suited for prod-code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Will make the change.

&& IndexComponentSelector.getByKey(
IndexNameExpressionResolver.splitSelectorExpression(name).v2()
) == IndexComponentSelector.FAILURES;
}

private boolean hasDlsFlsPermissions(IndicesAccessControl.IndexAccessControl indexAccessControl) {
return indexAccessControl.getDocumentPermissions().hasDocumentLevelPermissions()
|| indexAccessControl.getFieldPermissions().hasFieldLevelSecurity();
}

private boolean isBackingFailureStoreIndex(String index) {
final IndexAbstraction indexAbstraction = clusterService.state()
.metadata()
.getProject(projectResolver.getProjectId())
.getIndicesLookup()
.get(index);
if (indexAbstraction == null) {
return false;
}
return indexAbstraction.isFailureIndexOfDataStream();
}

}