From 9964d89dd5d67cf72a85eb48d76347f09bd875f5 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Mon, 21 Oct 2019 12:18:49 +0300 Subject: [PATCH] Fix security origin for TokenService#findActiveTokensFor... (#47418) All internal searches (triggered by APIs) across the .security index must be performed while "under the security origin". Otherwise, the search is performed in the context of the caller which most likely does not have privileges to search .security (hopefully). This commit fixes this in the case of two methods in the TokenService and corrects an overly done such context switch in the ApiKeyService. In addition, this makes all tests from the client/rest-high-level module execute as an all mighty administrator, but not a literal superuser. Closes #47151 --- client/rest-high-level/build.gradle | 5 +- client/rest-high-level/roles.yml | 12 ++ .../SecurityDocumentationIT.java | 14 +- .../xpack/security/authc/ApiKeyService.java | 41 ++-- .../xpack/security/authc/TokenService.java | 43 ++-- .../test/api_key/11_invalidation.yml | 204 ++++++++++++++++++ .../rest-api-spec/test/token/10_basic.yml | 24 ++- 7 files changed, 300 insertions(+), 43 deletions(-) create mode 100644 client/rest-high-level/roles.yml create mode 100644 x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/11_invalidation.yml diff --git a/client/rest-high-level/build.gradle b/client/rest-high-level/build.gradle index e8c5f4d53f1a..fccad8cc0a11 100644 --- a/client/rest-high-level/build.gradle +++ b/client/rest-high-level/build.gradle @@ -129,8 +129,11 @@ testClusters.integTest { setting 'indices.lifecycle.poll_interval', '1000ms' keystore 'xpack.security.transport.ssl.truststore.secure_password', 'testnode' + extraConfigFile 'roles.yml', file('roles.yml') user username: System.getProperty('tests.rest.cluster.username', 'test_user'), - password: System.getProperty('tests.rest.cluster.password', 'test-password') + password: System.getProperty('tests.rest.cluster.password', 'test-password'), + role: System.getProperty('tests.rest.cluster.role', 'admin') + user username: 'admin_user', password: 'admin-password' extraConfigFile nodeCert.name, nodeCert extraConfigFile nodeTrustStore.name, nodeTrustStore diff --git a/client/rest-high-level/roles.yml b/client/rest-high-level/roles.yml new file mode 100644 index 000000000000..d3d0630f4305 --- /dev/null +++ b/client/rest-high-level/roles.yml @@ -0,0 +1,12 @@ +admin: + cluster: + - all + indices: + - names: '*' + privileges: + - all + run_as: [ '*' ] + applications: + - application: '*' + privileges: [ '*' ] + resources: [ '*' ] diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java index 7567e4314f7e..7964cb04ab24 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java @@ -95,7 +95,9 @@ import org.elasticsearch.client.security.user.privileges.Role.IndexPrivilegeName import org.elasticsearch.client.security.user.privileges.UserIndicesPrivileges; import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.util.set.Sets; import org.hamcrest.Matchers; @@ -122,6 +124,8 @@ import java.util.concurrent.TimeUnit; import javax.crypto.SecretKeyFactory; import javax.crypto.spec.PBEKeySpec; +import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; + import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; @@ -139,6 +143,14 @@ import static org.hamcrest.Matchers.nullValue; public class SecurityDocumentationIT extends ESRestHighLevelClientTestCase { + @Override + protected Settings restAdminSettings() { + String token = basicAuthHeaderValue("admin_user", new SecureString("admin-password".toCharArray())); + return Settings.builder() + .put(ThreadContext.PREFIX + ".Authorization", token) + .build(); + } + public void testGetUsers() throws Exception { final RestHighLevelClient client = highLevelClient(); String[] usernames = new String[] {"user1", "user2", "user3"}; @@ -739,7 +751,7 @@ public class SecurityDocumentationIT extends ESRestHighLevelClientTestCase { //end::authenticate-response assertThat(user.getUsername(), is("test_user")); - assertThat(user.getRoles(), contains(new String[]{"superuser"})); + assertThat(user.getRoles(), contains(new String[]{"admin"})); assertThat(user.getFullName(), nullValue()); assertThat(user.getEmail(), nullValue()); assertThat(user.getMetadata().isEmpty(), is(true)); 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 6ecd7cd20e5d..b96f99f60c71 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 @@ -23,6 +23,7 @@ import org.elasticsearch.action.get.GetResponse; import org.elasticsearch.action.index.IndexAction; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchRequest; +import org.elasticsearch.action.support.ContextPreservingActionListener; import org.elasticsearch.action.support.WriteRequest.RefreshPolicy; import org.elasticsearch.action.update.UpdateRequest; import org.elasticsearch.action.update.UpdateResponse; @@ -92,6 +93,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; +import java.util.function.Supplier; import java.util.stream.Collectors; import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME; @@ -659,28 +661,29 @@ public class ApiKeyService { expiredQuery.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("expiration_time"))); boolQuery.filter(expiredQuery); } + final Supplier supplier = client.threadPool().getThreadContext().newRestorableContext(false); try (ThreadContext.StoredContext ignore = client.threadPool().getThreadContext().stashWithOrigin(SECURITY_ORIGIN)) { final SearchRequest request = client.prepareSearch(SECURITY_MAIN_ALIAS) - .setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings)) - .setQuery(boolQuery) - .setVersion(false) - .setSize(1000) - .setFetchSource(true) - .request(); + .setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings)) + .setQuery(boolQuery) + .setVersion(false) + .setSize(1000) + .setFetchSource(true) + .request(); securityIndex.checkIndexVersionThenExecute(listener::onFailure, - () -> ScrollHelper.fetchAllByEntity(client, request, listener, - (SearchHit hit) -> { - Map source = hit.getSourceAsMap(); - String name = (String) source.get("name"); - String id = hit.getId(); - Long creation = (Long) source.get("creation_time"); - Long expiration = (Long) source.get("expiration_time"); - Boolean invalidated = (Boolean) source.get("api_key_invalidated"); - String username = (String) ((Map) source.get("creator")).get("principal"); - String realm = (String) ((Map) source.get("creator")).get("realm"); - return new ApiKey(name, id, Instant.ofEpochMilli(creation), - (expiration != null) ? Instant.ofEpochMilli(expiration) : null, invalidated, username, realm); - })); + () -> ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), + (SearchHit hit) -> { + Map source = hit.getSourceAsMap(); + String name = (String) source.get("name"); + String id = hit.getId(); + Long creation = (Long) source.get("creation_time"); + Long expiration = (Long) source.get("expiration_time"); + Boolean invalidated = (Boolean) source.get("api_key_invalidated"); + String username = (String) ((Map) source.get("creator")).get("principal"); + String realm = (String) ((Map) source.get("creator")).get("realm"); + return new ApiKey(name, id, Instant.ofEpochMilli(creation), + (expiration != null) ? Instant.ofEpochMilli(expiration) : null, invalidated, username, realm); + })); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java index a2082e3bd09c..c5888cc9b5b8 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java @@ -30,6 +30,7 @@ import org.elasticsearch.action.index.IndexAction; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.action.support.ContextPreservingActionListener; import org.elasticsearch.action.support.TransportActions; import org.elasticsearch.action.support.WriteRequest.RefreshPolicy; import org.elasticsearch.action.support.master.AcknowledgedRequest; @@ -135,6 +136,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; import java.util.function.Predicate; +import java.util.function.Supplier; import java.util.stream.Collectors; import static org.elasticsearch.action.support.TransportActions.isShardNotAvailableException; @@ -1240,17 +1242,20 @@ public final class TokenService { - TimeValue.timeValueHours(ExpiredTokenRemover.MAXIMUM_TOKEN_LIFETIME_HOURS).millis())) ) ); - final SearchRequest request = client.prepareSearch(indicesWithTokens.toArray(new String[0])) - .setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings)) - .setQuery(boolQuery) - .setVersion(false) - .setSize(1000) - .setFetchSource(true) - .request(); - ScrollHelper.fetchAllByEntity(client, request, listener, (SearchHit hit) -> filterAndParseHit(hit, filter)); + final Supplier supplier = client.threadPool().getThreadContext().newRestorableContext(false); + try (ThreadContext.StoredContext ignore = client.threadPool().getThreadContext().stashWithOrigin(SECURITY_ORIGIN)) { + final SearchRequest request = client.prepareSearch(indicesWithTokens.toArray(new String[0])) + .setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings)) + .setQuery(boolQuery) + .setVersion(false) + .setSize(1000) + .setFetchSource(true) + .request(); + ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), + (SearchHit hit) -> filterAndParseHit(hit, filter)); + } } }, listener::onFailure)); - } /** @@ -1284,14 +1289,18 @@ public final class TokenService { - TimeValue.timeValueHours(ExpiredTokenRemover.MAXIMUM_TOKEN_LIFETIME_HOURS).millis())) ) ); - final SearchRequest request = client.prepareSearch(indicesWithTokens.toArray(new String[0])) - .setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings)) - .setQuery(boolQuery) - .setVersion(false) - .setSize(1000) - .setFetchSource(true) - .request(); - ScrollHelper.fetchAllByEntity(client, request, listener, (SearchHit hit) -> filterAndParseHit(hit, isOfUser(username))); + final Supplier supplier = client.threadPool().getThreadContext().newRestorableContext(false); + try (ThreadContext.StoredContext ignore = client.threadPool().getThreadContext().stashWithOrigin(SECURITY_ORIGIN)) { + final SearchRequest request = client.prepareSearch(indicesWithTokens.toArray(new String[0])) + .setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings)) + .setQuery(boolQuery) + .setVersion(false) + .setSize(1000) + .setFetchSource(true) + .request(); + ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), + (SearchHit hit) -> filterAndParseHit(hit, isOfUser(username))); + } } }, listener::onFailure)); } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/11_invalidation.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/11_invalidation.yml new file mode 100644 index 000000000000..b1bb2762f843 --- /dev/null +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/11_invalidation.yml @@ -0,0 +1,204 @@ +--- +setup: + - skip: + features: headers + + - do: + cluster.health: + wait_for_status: yellow + + - do: + security.put_role: + name: "admin_role" + body: > + { + "cluster": ["manage_api_key"] + } + + - do: + security.put_role: + name: "user_role" + body: > + { + "cluster": ["manage_own_api_key"] + } + + - do: + security.put_user: + username: "api_key_manager" + body: > + { + "password" : "x-pack-test-password", + "roles" : [ "admin_role" ], + "full_name" : "API key manager" + } + + - do: + security.put_user: + username: "api_key_user_1" + body: > + { + "password" : "x-pack-test-password", + "roles" : [ "user_role" ], + "full_name" : "API key user" + } + +--- +teardown: + - do: + security.delete_role: + name: "admin_role" + ignore: 404 + + - do: + security.delete_role: + name: "use_role" + ignore: 404 + + - do: + security.delete_user: + username: "api_key_user_1" + ignore: 404 + + - do: + security.delete_user: + username: "api_key_manager" + ignore: 404 + +--- +"Test invalidate api key by username": + + # every user first gets its own API key + - do: + headers: + Authorization: "Basic YXBpX2tleV9tYW5hZ2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_manager + security.create_api_key: + body: > + { + "name": "manager-api-key", + "expiration": "1d", + "role_descriptors": { + } + } + - match: { name: "manager-api-key" } + - is_true: id + - is_true: api_key + - is_true: expiration + - set: { id: manager_key_id } + + - do: + headers: + Authorization: "Basic YXBpX2tleV91c2VyXzE6eC1wYWNrLXRlc3QtcGFzc3dvcmQ=" # api_key_user_1 + security.create_api_key: + body: > + { + "name": "user1-api-key", + "expiration": "1d", + "role_descriptors": { + } + } + - match: { name: "user1-api-key" } + - is_true: id + - is_true: api_key + - is_true: expiration + - set: { id: user1_key_id } + + - do: + headers: + Authorization: "Basic YXBpX2tleV9tYW5hZ2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_manager + security.invalidate_api_key: + body: > + { + "username": "api_key_user_1" + } + - length: { "invalidated_api_keys" : 1 } + - match: { "invalidated_api_keys.0" : "${user1_key_id}" } + - length: { "previously_invalidated_api_keys" : 0 } + - match: { "error_count" : 0 } + + - do: + catch: forbidden + headers: + Authorization: "Basic YXBpX2tleV91c2VyXzE6eC1wYWNrLXRlc3QtcGFzc3dvcmQ=" # api_key_user_1 + security.invalidate_api_key: + body: > + { + "username": "api_key_manager" + } + - match: { "error.type": "security_exception" } + - match: { "error.reason": "action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [api_key_user_1]" } + + - do: + headers: + Authorization: "Basic YXBpX2tleV9tYW5hZ2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_manager + security.invalidate_api_key: + body: > + { + "username": "api_key_manager" + } + - length: { "invalidated_api_keys" : 1 } + - match: { "invalidated_api_keys.0" : "${manager_key_id}" } + - length: { "previously_invalidated_api_keys" : 0 } + - match: { "error_count" : 0 } + +--- +"Test invalidate api key by realm name": + + # every user first gets its own API key + - do: + headers: + Authorization: "Basic YXBpX2tleV9tYW5hZ2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_manager + security.create_api_key: + body: > + { + "name": "manager-api-key", + "expiration": "1d", + "role_descriptors": { + } + } + - match: { name: "manager-api-key" } + - is_true: id + - is_true: api_key + - is_true: expiration + - set: { id: manager_key_id } + + - do: + headers: + Authorization: "Basic YXBpX2tleV91c2VyXzE6eC1wYWNrLXRlc3QtcGFzc3dvcmQ=" # api_key_user_1 + security.create_api_key: + body: > + { + "name": "user1-api-key", + "expiration": "1d", + "role_descriptors": { + } + } + - match: { name: "user1-api-key" } + - is_true: id + - is_true: api_key + - is_true: expiration + - set: { id: user1_key_id } + + - do: + catch: forbidden + headers: + Authorization: "Basic YXBpX2tleV91c2VyXzE6eC1wYWNrLXRlc3QtcGFzc3dvcmQ=" # api_key_user_1 + security.invalidate_api_key: + body: > + { + "realm_name": "default_native" + } + - match: { "error.type": "security_exception" } + - match: { "error.reason": "action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [api_key_user_1]" } + + - do: + headers: + Authorization: "Basic YXBpX2tleV9tYW5hZ2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_manager + security.invalidate_api_key: + body: > + { + "realm_name": "default_native" + } + - length: { "invalidated_api_keys" : 2 } + - length: { "previously_invalidated_api_keys" : 0 } + - match: { "error_count" : 0 } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/token/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/token/10_basic.yml index 3400b4f83f64..ca3de6889329 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/token/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/token/10_basic.yml @@ -7,18 +7,32 @@ setup: cluster.health: wait_for_status: yellow + - do: + security.put_role: + name: "admin_role" + body: > + { + "cluster": ["manage_security"] + } + - do: security.put_user: username: "token_user" body: > { "password" : "x-pack-test-password", - "roles" : [ "superuser" ], + "roles" : [ "admin_role" ], "full_name" : "Token User" } --- teardown: + + - do: + security.delete_role: + name: "admin_role" + ignore: 404 + - do: security.delete_user: username: "token_user" @@ -46,7 +60,7 @@ teardown: security.authenticate: {} - match: { username: "token_user" } - - match: { roles.0: "superuser" } + - match: { roles.0: "admin_role" } - match: { full_name: "Token User" } --- @@ -71,7 +85,7 @@ teardown: security.authenticate: {} - match: { username: "token_user" } - - match: { roles.0: "superuser" } + - match: { roles.0: "admin_role" } - match: { full_name: "Token User" } - do: @@ -111,7 +125,7 @@ teardown: security.authenticate: {} - match: { username: "token_user" } - - match: { roles.0: "superuser" } + - match: { roles.0: "admin_role" } - match: { full_name: "Token User" } - do: @@ -152,7 +166,7 @@ teardown: security.authenticate: {} - match: { username: "token_user" } - - match: { roles.0: "superuser" } + - match: { roles.0: "admin_role" } - match: { full_name: "Token User" } - do: