Add support for the `simple_query_string` to the Query API Key API #104132

albertzaharovits · 2024-01-09T14:10:23Z

This adds support for the simple_query_string query type to the Query API key Information API.
In addition, this also adds support for querying all the API Key metadata fields simultaneously,
rather than requiring each to be specified, such as metadata.x, metadata.y, etc.

Relates: #101691

elasticsearchmachine · 2024-01-12T14:13:32Z

Hi @albertzaharovits, I've created a changelog YAML for you.

elasticsearchmachine · 2024-01-12T14:17:03Z

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

jfreden

Great job on this! I think it looks great! Just have some minor comments/questions.

At a high level, as someone who has not seen this before, I had a hard time wrapping my head around why we don't allow simple match query on the metadata level. If I use your IT test data as an example, why should this return data:

{"query": {"simple_query_string": {"query": "prod 42 true", "fields": ["meta*"]}}}

but not:

{"query": {"simple_query_string": {"query": "prod 42 true", "fields": ["metadata.val*"]}}}

jfreden · 2024-01-15T10:05:26Z

.../security/src/main/java/org/elasticsearch/xpack/security/support/ApiKeyBoolQueryBuilder.java

+                }
+            }
+            if (simpleQueryStringBuilder.fields().isEmpty()) {
+                simpleQueryStringBuilder.field("match_nothing_place_holder_field_name_wildcard*");


Is this for testing purposes only?

No, it's not for test only.
I forgot to comment in the code...
If we let the SimpleQueryStringBuilder#fields() be empty here, it's eventually going to be interpreted as *

elasticsearch/server/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java

Line 414 in 293d030

List<String> defaultFields = context.defaultFields();

This means it's going to search on the other fields in the .security index, including the disallowed ones. This conspicuous pattern (a non-pattern should work too for this query type) is put in here in order to avoid this.

Ah yes, because the default is *, that makes sense. Thanks for clarifying!

Pushed b73a13d to add a comment explaining it.

Nifty! I'm wondering if we can be more explicit by returning a MatchNoneQueryBuilder here instead (it also avoids "pointless" match work on a field we know doesn't exist).

(Note: I may be overlooking something here!)

Good point! I'll try it, I think it should work! Thanks!

Pushed 0a7b76e , I think it will pass the tests.

jfreden · 2024-01-15T10:16:33Z

...urity/src/main/java/org/elasticsearch/xpack/security/support/ApiKeyFieldNameTranslators.java

+            }
+        }
+        if (indexFieldNames.isEmpty() && Regex.isSimpleMatchPattern(fieldNameOrPattern) == false) {
+            throw new IllegalArgumentException("Field [" + fieldNameOrPattern + "] is not allowed for API Key query");


Should this result in an error or just be ignored? Based on this it looks like it shouldn't even be checked for correct syntax.

Hmmm, now that you're mentioning it....
I verified that the simple_query_string for the regular _search endpoints just ignores unrecognized fields (if querying on a unrecognized field, no matches are returned) - and there's no parameter to change this behavior.
So, I believe that you're right that it would probably be more appropriate to also ignore unrecognized fields for simple_query_strings for the /_security/_query/api_key endpoint, even if all the other allowed query types complain about it.
@n1v0lg Do you have preference here?

Pushed f542fc5 to make the SimpleQueryString preprocessing ignore unknown field names, analogously to the the core's processing of the same query type.
The result of it is that a simple query string issued to the POST /_security/_query/api_key endpoint behaves identical to when issued to the regular _search endpoints (ignores unknown fields).
@n1v0lg If you're leaning towards the less lenient, original implementation, please pitch in, I can easily revert it.

Sorry for the delay! I'm ++ on the consistent behavior with regular _search. I'll review the rest of the PR shortly.

jfreden · 2024-01-15T11:06:55Z

...urity/src/main/java/org/elasticsearch/xpack/security/support/ApiKeyFieldNameTranslators.java

+            // moreover, it's not possible to iterate the concrete fields matching the prefix
+            if (Regex.isSimpleMatchPattern(fieldNamePrefix)) {
+                // this means `metadata.*` and `metadata.x*` are expanded to the empty list
+                // rather than be replaced with `metadata_flattened.*` and `metadata_flattened.x*`


I found this a little confusing. Sorry if my question doesn't make sense!

So, this means that we don't support wildcards for metadata attribute field selection, right? It has to be the exact name of the attribute to match? So we can't have metadata.something_a and metadata.something_b and select them for search with "fields": ["metadata.something*"]. Is it possible to document that part, so the part where the result of this return false is that we can't match on a simple match pattern when selecting metadata attributes, it's either all attributes or a list of specific attributes.

It's indeed confusing. I'll jot something in the docs to explain it.

So, this means that we don't support wildcards for metadata attribute field selection, right?

That's exactly right!

The root limitation actually comes from the way the flattened field type works (the API key metadata_flattened field is mapped as such).
As you say, it's not possible to access a subset of the metadata_flattened.* fields via wildcards. It's either all of them, via metadata_flattened, or specific (non-wildcard) ones in a list.

Because the limitation comes from the Simple Query String processing, we could drop the processing here, i.e. which prohibits the metadata.x* -> metadata_flattened.x* translation, and it would still work identically (because metadata_flattened.x* is going to be implicitly ignored). But I think it's clearer to keep it (and test for it).

Interestingly, if the sub-field is a wildcard, e.g. metadata_flattened.wild* exists as a field, then this field cannot be accessed in the field parameter of the simple_query_string.

Pushed 18e9993 to explain the above in the docs proper.

albertzaharovits · 2024-01-17T13:32:23Z

@elasticsearchmachine run elasticsearch-ci/part-4-fips

albertzaharovits · 2024-01-17T19:34:44Z

@elasticsearchmachine run elasticsearch-ci/part-2-fips

albertzaharovits · 2024-01-18T08:27:10Z

@elasticsearchmachine run elasticsearch-ci/part-2-fips

albertzaharovits · 2024-01-18T08:43:56Z

@elasticsearchmachine run elasticsearch-ci/part-2-fips

albertzaharovits · 2024-01-18T10:25:15Z

@elasticsearchmachine run elasticsearch-ci/part-2-fips

albertzaharovits · 2024-01-18T12:20:50Z

@elasticmachine run elasticsearch-ci/part-2-fips

albertzaharovits · 2024-01-18T12:54:39Z

@elasticmachine run elasticsearch-ci/part-2-fips

Simple query string

624c7b9

albertzaharovits self-assigned this Jan 9, 2024

elasticsearchmachine added the v8.13.0 label Jan 9, 2024

simplest test

4c62314

albertzaharovits force-pushed the query-string-support-for-api-key-querying branch from 347cdd7 to 4c62314 Compare January 10, 2024 11:22

albertzaharovits added 10 commits January 10, 2024 16:16

Merge branch 'main' into query-string-support-for-api-key-querying

34e27d5

Merge branch 'main' into query-string-support-for-api-key-querying

39d534b

Field name pattern that doesn't match

7586af2

Extend the IT tests

b547a42

even more ITs

3723c22

even more ITs

0e53356

More tests

d479d05

Merge branch 'main' into query-string-support-for-api-key-querying

3169f3a

ApiKeyBoolQueryBuilderTests

74a5098

Last tests, docs and nits

724c8da

albertzaharovits changed the title ~~Simple query string~~ Add support for the simple_query_string to the Query API Key API Jan 12, 2024

albertzaharovits added >enhancement :Security/Security Security issues without another label labels Jan 12, 2024

albertzaharovits requested a review from n1v0lg January 12, 2024 14:13

Update docs/changelog/104132.yaml

3d7c1e3

albertzaharovits marked this pull request as ready for review January 12, 2024 14:16

elasticsearchmachine added the Team:Security Meta label for security team label Jan 12, 2024

n1v0lg requested a review from jfreden January 12, 2024 15:36

jfreden approved these changes Jan 15, 2024

View reviewed changes

albertzaharovits added 4 commits January 15, 2024 16:22

Merge branch 'main' into query-string-support-for-api-key-querying

464c69f

Merge branch 'main' into query-string-support-for-api-key-querying

0cf9977

Merge branch 'main' into query-string-support-for-api-key-querying

b06b26a

Comment on the reason for the field name pattern placeholder

b73a13d

albertzaharovits removed the :Security/Security Security issues without another label label Jan 16, 2024

elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Security Meta label for security team labels Jan 16, 2024

Merge branch 'main' into query-string-support-for-api-key-querying

faee38a

albertzaharovits added :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) and removed needs:triage Requires assignment of a team area label labels Jan 16, 2024

elasticsearchmachine added the Team:Security Meta label for security team label Jan 16, 2024

albertzaharovits added 5 commits January 16, 2024 16:32

Shorten docs

554af9f

assertThat(apiKeys, is(empty()))

701e2c1

new MatchNoneQueryBuilder()

0a7b76e

Merge branch 'main' into query-string-support-for-api-key-querying

e14bcf9

Merge branch 'main' into query-string-support-for-api-key-querying

f50ff13

albertzaharovits added 2 commits January 17, 2024 17:24

Merge branch 'main' into query-string-support-for-api-key-querying

5afe0cc

Merge branch 'main' into query-string-support-for-api-key-querying

4e7c80a

Merge branch 'main' into query-string-support-for-api-key-querying

f25c173

albertzaharovits added 2 commits January 18, 2024 13:21

Merge branch 'main' into query-string-support-for-api-key-querying

780504c

Merge branch 'main' into query-string-support-for-api-key-querying

bfbc7fb

albertzaharovits added 3 commits January 18, 2024 17:16

Merge branch 'main' into query-string-support-for-api-key-querying

994a140

Merge branch 'main' into query-string-support-for-api-key-querying

14a2ab6

Merge branch 'main' into query-string-support-for-api-key-querying

63f397b

albertzaharovits merged commit aeb2b77 into elastic:main Jan 19, 2024
19 checks passed

albertzaharovits deleted the query-string-support-for-api-key-querying branch January 19, 2024 12:51

albertzaharovits mentioned this pull request Feb 19, 2024

Enhance Query API key information API #101691

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the `simple_query_string` to the Query API Key API #104132

Add support for the `simple_query_string` to the Query API Key API #104132

albertzaharovits commented Jan 9, 2024 •

edited

elasticsearchmachine commented Jan 12, 2024

elasticsearchmachine commented Jan 12, 2024

jfreden left a comment •

edited

jfreden Jan 15, 2024

albertzaharovits Jan 15, 2024

jfreden Jan 16, 2024

albertzaharovits Jan 16, 2024

n1v0lg Jan 16, 2024

albertzaharovits Jan 16, 2024

albertzaharovits Jan 16, 2024

jfreden Jan 15, 2024 •

edited

albertzaharovits Jan 15, 2024

albertzaharovits Jan 16, 2024

n1v0lg Jan 16, 2024

jfreden Jan 15, 2024

albertzaharovits Jan 15, 2024

albertzaharovits Jan 16, 2024

albertzaharovits commented Jan 17, 2024

albertzaharovits commented Jan 17, 2024

albertzaharovits commented Jan 18, 2024

albertzaharovits commented Jan 18, 2024

albertzaharovits commented Jan 18, 2024

albertzaharovits commented Jan 18, 2024

albertzaharovits commented Jan 18, 2024

Add support for the simple_query_string to the Query API Key API #104132

Add support for the simple_query_string to the Query API Key API #104132

Conversation

albertzaharovits commented Jan 9, 2024 • edited

elasticsearchmachine commented Jan 12, 2024

elasticsearchmachine commented Jan 12, 2024

jfreden left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfreden Jan 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jan 17, 2024

albertzaharovits commented Jan 17, 2024

albertzaharovits commented Jan 18, 2024

albertzaharovits commented Jan 18, 2024

albertzaharovits commented Jan 18, 2024

albertzaharovits commented Jan 18, 2024

albertzaharovits commented Jan 18, 2024

Add support for the `simple_query_string` to the Query API Key API #104132

Add support for the `simple_query_string` to the Query API Key API #104132

albertzaharovits commented Jan 9, 2024 •

edited

jfreden left a comment •

edited

jfreden Jan 15, 2024 •

edited