From 145e10b52e45818a84b7afaa6c627faba9e82769 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com> Date: Thu, 21 Mar 2019 15:55:19 +1100 Subject: [PATCH] Fix so non super users can also create API keys (#40028) (#40288) When creating API keys we check for if API key with the same key name already exists and fail the request if it does. The check should have been performed with XPackSecurityUser instead of the authenticated user. This caused the request to fail in case of the non-super user trying to create an API key. This commit fixes by executing search action with SECURITY_ORIGIN so it can be executed with XPackSecurityUser. Also fixed the Rest test to avoid using a user with `super_user` role. Closes #40029 --- .../xpack/security/authc/ApiKeyService.java | 183 +++++++++++------- .../rest-api-spec/test/api_key/10_basic.yml | 40 +++- 2 files changed, 139 insertions(+), 84 deletions(-) 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 9488e92b285d4..0222fbe97c859 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 @@ -22,6 +22,7 @@ import org.elasticsearch.action.get.GetResponse; import org.elasticsearch.action.index.IndexAction; import org.elasticsearch.action.index.IndexRequest; +import org.elasticsearch.action.search.SearchAction; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.support.WriteRequest.RefreshPolicy; import org.elasticsearch.action.update.UpdateRequest; @@ -94,9 +95,11 @@ import javax.crypto.SecretKeyFactory; +import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME; import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING; import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN; import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin; +import static org.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_INDEX_NAME; public class ApiKeyService { @@ -190,85 +193,115 @@ public void createApiKey(Authentication authentication, CreateApiKeyRequest requ * this check is best effort as there could be two nodes executing search and * then index concurrently allowing a duplicate name. */ - findApiKeyForApiKeyName(request.getName(), true, true, ActionListener.wrap(apiKeyIds -> { - if (apiKeyIds.isEmpty()) { - final Instant created = clock.instant(); - final Instant expiration = getApiKeyExpiration(created, request); - final SecureString apiKey = UUIDs.randomBase64UUIDSecureString(); - final Version version = clusterService.state().nodes().getMinNodeVersion(); - if (version.before(Version.V_6_7_0)) { - logger.warn( - "nodes prior to the minimum supported version for api keys {} exist in the cluster;" - + " these nodes will not be able to use api keys", - Version.V_6_7_0); - } - - final char[] keyHash = hasher.hash(apiKey); - try (XContentBuilder builder = XContentFactory.jsonBuilder()) { - builder.startObject() - .field("doc_type", "api_key") - .field("creation_time", created.toEpochMilli()) - .field("expiration_time", expiration == null ? null : expiration.toEpochMilli()) - .field("api_key_invalidated", false); - - byte[] utf8Bytes = null; - try { - utf8Bytes = CharArrays.toUtf8Bytes(keyHash); - builder.field("api_key_hash").utf8Value(utf8Bytes, 0, utf8Bytes.length); - } finally { - if (utf8Bytes != null) { - Arrays.fill(utf8Bytes, (byte) 0); - } - } + checkDuplicateApiKeyNameAndCreateApiKey(authentication, request, roleDescriptorSet, listener); + } + } - // Save role_descriptors - builder.startObject("role_descriptors"); - if (request.getRoleDescriptors() != null && request.getRoleDescriptors().isEmpty() == false) { - for (RoleDescriptor descriptor : request.getRoleDescriptors()) { - builder.field(descriptor.getName(), - (contentBuilder, params) -> descriptor.toXContent(contentBuilder, params, true)); + private void checkDuplicateApiKeyNameAndCreateApiKey(Authentication authentication, CreateApiKeyRequest request, + Set roleDescriptorSet, + ActionListener listener) { + final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery() + .filter(QueryBuilders.termQuery("doc_type", "api_key")) + .filter(QueryBuilders.termQuery("name", request.getName())) + .filter(QueryBuilders.termQuery("api_key_invalidated", false)); + final BoolQueryBuilder expiredQuery = QueryBuilders.boolQuery() + .should(QueryBuilders.rangeQuery("expiration_time").lte(Instant.now().toEpochMilli())) + .should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("expiration_time"))); + boolQuery.filter(expiredQuery); + + final SearchRequest searchRequest = client.prepareSearch(SECURITY_INDEX_NAME) + .setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings)) + .setQuery(boolQuery) + .setVersion(false) + .setSize(1) + .request(); + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> + executeAsyncWithOrigin(client, SECURITY_ORIGIN, SearchAction.INSTANCE, searchRequest, + ActionListener.wrap( + indexResponse -> { + if (indexResponse.getHits().getTotalHits() > 0) { + listener.onFailure(traceLog("create api key", new ElasticsearchSecurityException( + "Error creating api key as api key with name [{}] already exists", request.getName()))); + } else { + createApiKeyAndIndexIt(authentication, request, roleDescriptorSet, listener); } - } - builder.endObject(); + }, + listener::onFailure))); + } + + private void createApiKeyAndIndexIt(Authentication authentication, CreateApiKeyRequest request, Set roleDescriptorSet, + ActionListener listener) { + final Instant created = clock.instant(); + final Instant expiration = getApiKeyExpiration(created, request); + final SecureString apiKey = UUIDs.randomBase64UUIDSecureString(); + final Version version = clusterService.state().nodes().getMinNodeVersion(); + if (version.before(Version.V_6_7_0)) { + logger.warn( + "nodes prior to the minimum supported version for api keys {} exist in the cluster;" + + " these nodes will not be able to use api keys", + Version.V_6_7_0); + } + + final char[] keyHash = hasher.hash(apiKey); + try (XContentBuilder builder = XContentFactory.jsonBuilder()) { + builder.startObject() + .field("doc_type", "api_key") + .field("creation_time", created.toEpochMilli()) + .field("expiration_time", expiration == null ? null : expiration.toEpochMilli()) + .field("api_key_invalidated", false); + + byte[] utf8Bytes = null; + try { + utf8Bytes = CharArrays.toUtf8Bytes(keyHash); + builder.field("api_key_hash").utf8Value(utf8Bytes, 0, utf8Bytes.length); + } finally { + if (utf8Bytes != null) { + Arrays.fill(utf8Bytes, (byte) 0); + } + } - // Save limited_by_role_descriptors - builder.startObject("limited_by_role_descriptors"); - for (RoleDescriptor descriptor : roleDescriptorSet) { - builder.field(descriptor.getName(), - (contentBuilder, params) -> descriptor.toXContent(contentBuilder, params, true)); - } - builder.endObject(); - - builder.field("name", request.getName()) - .field("version", version.id) - .startObject("creator") - .field("principal", authentication.getUser().principal()) - .field("metadata", authentication.getUser().metadata()) - .field("realm", authentication.getLookedUpBy() == null ? - authentication.getAuthenticatedBy().getName() : authentication.getLookedUpBy().getName()) - .endObject() - .endObject(); - final IndexRequest indexRequest = - client.prepareIndex(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE) - .setSource(builder) - .setRefreshPolicy(request.getRefreshPolicy()) - .request(); - securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> - executeAsyncWithOrigin(client, SECURITY_ORIGIN, IndexAction.INSTANCE, indexRequest, - ActionListener.wrap( - indexResponse -> listener.onResponse( - new CreateApiKeyResponse(request.getName(), indexResponse.getId(), apiKey, expiration)), - listener::onFailure))); - } catch (IOException e) { - listener.onFailure(e); - } finally { - Arrays.fill(keyHash, (char) 0); - } - } else { - listener.onFailure(traceLog("create api key", new ElasticsearchSecurityException( - "Error creating api key as api key with name [{}] already exists", request.getName()))); + // Save role_descriptors + builder.startObject("role_descriptors"); + if (request.getRoleDescriptors() != null && request.getRoleDescriptors().isEmpty() == false) { + for (RoleDescriptor descriptor : request.getRoleDescriptors()) { + builder.field(descriptor.getName(), + (contentBuilder, params) -> descriptor.toXContent(contentBuilder, params, true)); } - }, listener::onFailure)); + } + builder.endObject(); + + // Save limited_by_role_descriptors + builder.startObject("limited_by_role_descriptors"); + for (RoleDescriptor descriptor : roleDescriptorSet) { + builder.field(descriptor.getName(), + (contentBuilder, params) -> descriptor.toXContent(contentBuilder, params, true)); + } + builder.endObject(); + + builder.field("name", request.getName()) + .field("version", version.id) + .startObject("creator") + .field("principal", authentication.getUser().principal()) + .field("metadata", authentication.getUser().metadata()) + .field("realm", authentication.getLookedUpBy() == null ? + authentication.getAuthenticatedBy().getName() : authentication.getLookedUpBy().getName()) + .endObject() + .endObject(); + final IndexRequest indexRequest = + client.prepareIndex(SECURITY_INDEX_NAME, SINGLE_MAPPING_NAME) + .setSource(builder) + .setRefreshPolicy(request.getRefreshPolicy()) + .request(); + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> + executeAsyncWithOrigin(client, SECURITY_ORIGIN, IndexAction.INSTANCE, indexRequest, + ActionListener.wrap( + indexResponse -> listener.onResponse( + new CreateApiKeyResponse(request.getName(), indexResponse.getId(), apiKey, expiration)), + listener::onFailure))); + } catch (IOException e) { + listener.onFailure(e); + } finally { + Arrays.fill(keyHash, (char) 0); } } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/10_basic.yml index 10b4622fccd9c..e61a296d80560 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/10_basic.yml @@ -7,13 +7,34 @@ setup: cluster.health: wait_for_status: yellow + - do: + xpack.security.put_role: + name: "admin_role" + body: > + { + "cluster": ["all"], + "indices": [ + { + "names": "*", + "privileges": ["all"] + } + ], + "applications": [ + { + "application": "myapp", + "privileges": ["*"], + "resources": ["*"] + } + ] + } + - do: xpack.security.put_user: username: "api_key_user" body: > { "password" : "x-pack-test-password", - "roles" : [ "superuser" ], + "roles" : [ "admin_role" ], "full_name" : "API key user" } @@ -38,6 +59,11 @@ setup: --- teardown: + - do: + xpack.security.delete_role: + name: "admin_role" + ignore: 404 + - do: xpack.security.delete_user: username: "api_key_user" @@ -54,7 +80,7 @@ teardown: - do: headers: - Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" + Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_user security.create_api_key: body: > { @@ -105,7 +131,7 @@ teardown: - do: headers: - Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" + Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_user security.create_api_key: body: > { @@ -140,8 +166,6 @@ teardown: - set: { name: api_key_name } - do: - headers: - Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" security.get_api_key: id: "$api_key_id" - match: { "api_keys.0.id": "$api_key_id" } @@ -157,7 +181,7 @@ teardown: - do: headers: - Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" + Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_user security.create_api_key: body: > { @@ -174,8 +198,6 @@ teardown: - transform_and_set: { login_creds: "#base64EncodeCredentials(id,api_key)" } - do: - headers: - Authorization: Apikey ${login_creds} security.invalidate_api_key: body: > { @@ -193,7 +215,7 @@ teardown: - do: headers: - Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" + Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_user security.create_api_key: body: > {