From 69fc715bc35764509e03a525d81ffa2fb355c29f Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Mon, 21 Oct 2019 13:15:05 +0300 Subject: [PATCH] Fix security origin for TokenService#findActiveTokensFor... (#47418) (#48280) 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 815cd718af8..620268c9ec2 100644 --- a/client/rest-high-level/build.gradle +++ b/client/rest-high-level/build.gradle @@ -126,8 +126,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 00000000000..d3d0630f430 --- /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 2093e14d7c8..2edf4d5c4e0 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; @@ -121,6 +123,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; @@ -138,6 +142,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"}; @@ -737,7 +749,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 21494eb3e3f..5bf1569a9a2 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; @@ -91,6 +92,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 javax.crypto.SecretKeyFactory; @@ -666,28 +668,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 3a41b1b1ccf..0d5ca1fb275 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 00000000000..b1bb2762f84 --- /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 3400b4f83f6..ca3de688932 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: