Resolving wildcard application names without prefix query #96479
Conversation
For application privileges, today we use prefix query to resolve application names with trailing wildcard. However, prefix query is considered to be expensive and can be disabled if the cluster setting search.allow_expensive_queries is set to false. When that happens it breaks authorization in a surprising way. This PR adds conditional logic to fallback to in-memory filtering for application names when expensive queries are disabled. It is not less expensive. But it avoids the surprising breakage. Resolves: elastic#96465
ywangd
added
>bug
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
|
Hi @ywangd, I've created a changelog YAML for you. |
| return new Tuple<>(boolQuery, null); | ||
| } else { | ||
| logger.trace("expensive queries are not allowed, switching to filtering application names in memory"); | ||
| return new Tuple<>(QueryBuilders.existsQuery(APPLICATION.getPreferredName()), StringMatcher.of(applications)); |
There was a problem hiding this comment.
In the current implementation , I am pretty sure this is equivalent to a match all query since (for this doc type) that field will always exist. I think it would be more efficient to just do match all here and rely on the filter to provide all application privileges.
However, the in-memory post filtering (regardless of exists or match all query) is not ideal. Instead of post filtering it should be possible to configure the mappings to keep the keyword , but also add the index_prefix mapping or use edge ngrams. I think if we introduce this now we can handle the limitations passively.
For example, we could introduce index_prefix mapping and (when there is a trailing *) if the prefix is less than 20 we always query against the index_prefix else if greater than 20 (max size for index_prefix) we fallback to the prefix query...unless expensive queries are disabled , in that case (greater than 20 with trailing * and no expensive queries) we fail with a message stating they need to make the application name shorter or enable expensive queries - that is passive since that failure condition (this bug) already exists today. I think it would be exceptionally rare to hit the conditions to trigger the bug... so it would only be a partial fix to the bug that only fixes the bug when the application name is less than 20 characters. I think that is good enough, and this change could mostly remove the need to do expensive queries and introduce a meaningful and actionable error message.
(alternatively we could do the above but instead of falling back to the prefix query we could fallback as you do here if chars > 20 which should always work but may be silently slow)
There was a problem hiding this comment.
In the current implementation , I am pretty sure this is equivalent to a match all query since (for this doc type) that field will always exist. I think it would be more efficient to just do match all here and rely on the filter to provide all application privileges.
Yes, a match-all query should do the job. This was a copy/paste from existing code without too much thinking. Now if we take one step further, we can just return null here since no further limiting query is needed since the caller already has the filter query (by doc type).
we could introduce index_prefix mapping and (when there is a trailing *) if the prefix is less than 20 we always query against the index_prefix else if greater than 20
This could be a good solution for a new index. But it does not help existing deployments with existing data. So I think it is not really viable here.
There was a problem hiding this comment.
But it does not help existing deployments with existing data. So I think it is not really viable here.
Hmm... this highlights a bigger problem that we don't have a strategy for mappings updates (even passive additions) outside of a major versions bumps. Technically what i describe is possible (with inplace mapping updates + fallback queries or a new index), but without that strategy/code support I agree it is not really viable. I think we should work towards defining that strategy but that is well beyond the scope of this PR.
we can just return null here since no further limiting query is needed
++
There was a problem hiding this comment.
Thanks for the comment. I wrote my previous comment in a hurry. So please let me explain myself a bit more.
We have limited support for mappings update. It is possible to add new fields to the mappings. These new fields will have null values for existing documents, i.e. there is no automatic process to populate the new fields for existing documents. We also cannot update existing fields.
For the case here, we cannot update existing application field because (1) it is just not updatable (2) it is a keyword field while _index_prefixes requires text. So we will have to add a new field which would incur rather big overhead including both indexing and querying side changes. Also because this new field will not be populated for existing documents, we need to handle it in code to conditionally query the new field, if no result (this check could be tricky on its own), then fallback. It is not impossible. But the complexity does not feel right for this particular issue. In addition, being able to use expensive queries internally is a recurring ask. So a simpler solution seems to be more suitable because once internal expensive query usage is no longer an issue we can easily remove the workaround.
I think we should work towards defining that strategy
I agree. It would be great if we could update existing field's mapping. Or if not, at least being able to populate fields (new or existing) for existing documents. Lack of this process has been causing frictions for a while. A recent occurrence is about the API key's type field which is an existing field but was never used till the recent cross-cluster API key work. It is now a bit of pain to support query this field because we want to fetch API keys with both null and rest types when user asks for rest. You can refer to ES-5990 if interested in details.
For application privileges, today we use prefix query to resolve application names with trailing wildcard. However, prefix query is considered to be expensive and can be disabled if the cluster setting search.allow_expensive_queries is set to false. When that happens it breaks authorization in a surprising way.
This PR adds conditional logic to fallback to in-memory filtering for application names when expensive queries are disabled. It is not less expensive. But it avoids the surprising breakage.
Resolves: #96465