From aebc442f8bdfc81363744c2eedc54b2914d56bdc Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 9 Aug 2021 23:19:29 +1000 Subject: [PATCH] Fix bugs for unexpired API keys and id filtering (#76208) (#76236) This PR fixed an old bug and a new bug introduced #75335. Interestingly, the two bugs somewhat cancelled each other in tests. In addition, the test setup also contributed to the overall issue. The old bug is about filtering out expired API keys, but the relationship was wrong in the search query. The new bug is that _id field should be allowed in the index level for the new API key search API. Because of the old bug, the query always gets rewritten because the tests do not have any API keys that are expired before the query time. The query rewriting effectively bypasses the _id field check. Hence the new bug is not triggered. --- .../xpack/security/QueryApiKeyIT.java | 12 +++-- .../authc/apikey/ApiKeySingleNodeTests.java | 48 +++++++++++++++++++ .../xpack/security/authc/ApiKeyService.java | 2 +- .../support/ApiKeyBoolQueryBuilder.java | 2 +- 4 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/apikey/ApiKeySingleNodeTests.java diff --git a/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/QueryApiKeyIT.java b/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/QueryApiKeyIT.java index f7f2882c31e4f..741d539d5f277 100644 --- a/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/QueryApiKeyIT.java +++ b/x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/QueryApiKeyIT.java @@ -279,9 +279,15 @@ private Tuple createApiKey(String name, roleDescriptors == null ? org.elasticsearch.core.Map.of() : roleDescriptors, XContentType.JSON).utf8ToString(); final String metadataString = XContentTestUtils.convertToXContent( metadata == null ? org.elasticsearch.core.Map.of() : metadata, XContentType.JSON).utf8ToString(); - request.setJsonEntity("{\"name\":\"" + name - + "\", \"role_descriptors\":" + roleDescriptorsString - + ", \"metadata\":" + metadataString + "}"); + if (randomBoolean()) { + request.setJsonEntity("{\"name\":\"" + name + + "\", \"role_descriptors\":" + roleDescriptorsString + + ", \"metadata\":" + metadataString + "}"); + } else { + request.setJsonEntity("{\"name\":\"" + name + + "\", \"expiration\": \"10d\", \"role_descriptors\":" + roleDescriptorsString + + ", \"metadata\":" + metadataString + "}"); + } request.setOptions(request.getOptions().toBuilder().addHeader(HttpHeaders.AUTHORIZATION, authHeader)); final Response response = client().performRequest(request); assertOK(response); diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/apikey/ApiKeySingleNodeTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/apikey/ApiKeySingleNodeTests.java new file mode 100644 index 0000000000000..c4db969082772 --- /dev/null +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/apikey/ApiKeySingleNodeTests.java @@ -0,0 +1,48 @@ +/* + * 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.authc.apikey; + +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.TimeValue; +import org.elasticsearch.index.query.QueryBuilders; +import org.elasticsearch.test.SecuritySingleNodeTestCase; +import org.elasticsearch.xpack.core.XPackSettings; +import org.elasticsearch.xpack.core.security.action.CreateApiKeyAction; +import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest; +import org.elasticsearch.xpack.core.security.action.apikey.QueryApiKeyAction; +import org.elasticsearch.xpack.core.security.action.apikey.QueryApiKeyRequest; +import org.elasticsearch.xpack.core.security.action.apikey.QueryApiKeyResponse; + +import static org.hamcrest.Matchers.equalTo; + +public class ApiKeySingleNodeTests extends SecuritySingleNodeTestCase { + + @Override + protected Settings nodeSettings() { + Settings.Builder builder = Settings.builder().put(super.nodeSettings()); + builder.put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true); + return builder.build(); + } + + public void testQueryWithExpiredKeys() throws InterruptedException { + final String id1 = client().execute(CreateApiKeyAction.INSTANCE, + new CreateApiKeyRequest("expired-shortly", null, TimeValue.timeValueMillis(1), null)) + .actionGet() + .getId(); + final String id2 = client().execute(CreateApiKeyAction.INSTANCE, + new CreateApiKeyRequest("long-lived", null, TimeValue.timeValueDays(1), null)) + .actionGet() + .getId(); + Thread.sleep(10); // just to be 100% sure that the 1st key is expired when we search for it + + final QueryApiKeyRequest queryApiKeyRequest = new QueryApiKeyRequest(QueryBuilders.idsQuery().addIds(id1, id2)); + final QueryApiKeyResponse queryApiKeyResponse = client().execute(QueryApiKeyAction.INSTANCE, queryApiKeyRequest).actionGet(); + assertThat(queryApiKeyResponse.getApiKeyInfos().length, equalTo(1)); + assertThat(queryApiKeyResponse.getApiKeyInfos()[0].getId(), equalTo(id2)); + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java index 786c64bc3bee5..9eaf9e5f4832c 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java @@ -928,7 +928,7 @@ private void findApiKeys(final BoolQueryBuilder boolQuery, boolean filterOutInva } if (filterOutExpiredKeys) { final BoolQueryBuilder expiredQuery = QueryBuilders.boolQuery(); - expiredQuery.should(QueryBuilders.rangeQuery("expiration_time").lte(Instant.now().toEpochMilli())); + expiredQuery.should(QueryBuilders.rangeQuery("expiration_time").gt(Instant.now().toEpochMilli())); expiredQuery.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("expiration_time"))); boolQuery.filter(expiredQuery); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/ApiKeyBoolQueryBuilder.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/ApiKeyBoolQueryBuilder.java index f946f4d1bb3f2..0d83b7ed83200 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/ApiKeyBoolQueryBuilder.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/ApiKeyBoolQueryBuilder.java @@ -33,7 +33,7 @@ public class ApiKeyBoolQueryBuilder extends BoolQueryBuilder { // Field names allowed at the index level private static final Set ALLOWED_EXACT_INDEX_FIELD_NAMES = - org.elasticsearch.core.Set.of("doc_type", "name", "api_key_invalidated", "creation_time", "expiration_time"); + org.elasticsearch.core.Set.of("_id", "doc_type", "name", "api_key_invalidated", "creation_time", "expiration_time"); private ApiKeyBoolQueryBuilder() {}