Skip to content

Conversation

@slobodanadamovic
Copy link
Contributor

@slobodanadamovic slobodanadamovic commented Mar 12, 2025

Failure store collects ingestion and mapping related failures when documents are written to a data stream. Indexing can fail and be captured in the failure store at any point in the ingest process.

The fields may not have been dropped or sanitized during ingestion processing, or the document may not be in the form expected by document/field-level security rules, either of which may lead to the document exposing sensitive information that would otherwise not be exposed if the document was successfully processed and ingested.

Since the DLS/FLS may not be applicable in the expected way, we here prevent access to the failure store for all users that have DLS/FLS restrictions and are accessing failure store using the ::failures selector (e.g. logs-*::failures). The direct access to the backing failure store indices (.fs-*) is also not allowed.

Failure store collects ingestion and mapping related failures when
documents are written to a data stream. Indexing can fail and be captured
in the failure store at any point in the ingest process.

The fields may not have been dropped or sanitized during ingestion
processing, or the document may not be in the form expected by
document/field-level security rules, either of which may lead to the
document exposing sensitive information that would otherwise not be
exposed if the document was successfully processed and ingested.

Since the DLS/FLS may not be applicable in the expected way,
we here prevent access to the failure store for all users that have
DLS/FLS restrictions.
@slobodanadamovic slobodanadamovic added >non-issue :Security/Security Security issues without another label Team:Security Meta label for security team labels Mar 12, 2025
@slobodanadamovic slobodanadamovic self-assigned this Mar 12, 2025
@slobodanadamovic slobodanadamovic added backport v8.19.0 auto-backport Automatically create backport pull requests when merged and removed backport labels Mar 18, 2025
@slobodanadamovic slobodanadamovic requested a review from n1v0lg March 24, 2025 10:24
@slobodanadamovic slobodanadamovic marked this pull request as ready for review March 24, 2025 10:24
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

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.

Copy link
Contributor

@n1v0lg n1v0lg left a comment

Choose a reason for hiding this comment

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

Leaving a few smaller comments, but as discussed let's sanity check that this is the direction we want to go -- as opposed to not treating DLS/FLS as special -- and get another pair of eyes on using cluster state here.

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


@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.

}

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.

) {
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged >non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants