Tasks: Only require task permissions #35667

nik9000 · 2018-11-16T20:56:53Z

Right now using the GET /_tasks/<taskid> API and causing a task to opt
in to saving its result after being completed requires permissions on
the .tasks index. When we built this we thought that that was fine,
but we've since moved towards not leaking details like "persisting task
results after the task is completed is done by saving them into an index
named .tasks." A more modern way of doing this would be to save the
tasks into the index "under the hood" and to have APIs to manage the
saved tasks. This is the first step down that road: it drops the
requirement to have permissions to interact with the .tasks index when
fetching task statuses and when persisting statuses beyond the lifetime
of the task.

In particular, this moves the concept of the "origin" of an action into
a more prominent place in the Elasticsearch server. The origin of an
action is ignored by the server, but the security plugin uses the origin
to make requests on behalf of a user in such a way that the user need
not have permissions to perform these actions. It can be made to be
fairly precise. More specifically, we can create an internal user just
for the tasks API that just has permission to interact with the .tasks
index. This change doesn't do that, instead, it uses the ubiquitus
"xpack" user which has most permissions because it is simpler. Adding
the tasks user is something I'd like to get to in a follow up change.

Instead, the majority of this change is about moving the "origin"
concept from the security portion of x-pack into the server. This should
allow any code to use the origin. To keep the change managable I've also
opted to deprecate rather than remove the "origin" helpers in the
security code. Removing them is almost entirely mechanical and I'd like
to that in a follow up as well.

Relates to #35573

Right now using the `GET /_tasks/<taskid>` API and causing a task to opt in to saving its result after being completed requires permissions on the `.tasks` index. When we built this we thought that that was fine, but we've since moved towards not leaking details like "persisting task results after the task is completed is done by saving them into an index named `.tasks`." A more modern way of doing this would be to save the tasks into the index "under the hood" and to have APIs to manage the saved tasks. This is the first step down that road: it drops the requirement to have permissions to interact with the `.tasks` index when fetching task statuses and when persisting statuses beyond the lifetime of the task. In particular, this moves the concept of the "origin" of an action into a more prominent place in the Elasticsearch server. The origin of an action is ignored by the server, but the security plugin uses the origin to make requests on behalf of a user in such a way that the user need not have permissions to perform these actions. It *can* be made to be fairly precise. More specifically, we can create an internal user just for the tasks API that just has permission to interact with the `.tasks` index. This change doesn't do that, instead, it uses the ubiquitus "xpack" user which has most permissions because it is simpler. Adding the tasks user is something I'd like to get to in a follow up change. Instead, the majority of this change is about moving the "origin" concept from the security portion of x-pack into the server. This should allow any code to use the origin. To keep the change managable I've also opted to deprecate rather than remove the "origin" helpers in the security code. Removing them is almost entirely mechanical and I'd like to that in a follow up as well. Relates to elastic#35573

elasticmachine · 2018-11-16T20:56:54Z

Pinging @elastic/es-distributed

nik9000 · 2018-11-16T20:57:29Z

x-pack/qa/reindex-tests-with-security/src/test/resources/rest-api-spec/test/10_reindex.yml

@@ -316,3 +314,51 @@ setup:
            index: dest
          dest:
            index: source
+
+---


This is the first thing I made in this PR. It is kind of a shame I couldn't drag it to the front because it exposes the problem perfectly

nik9000 · 2018-11-16T20:58:54Z

...n/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationUtilsTests.java

@@ -90,60 +91,45 @@ public void testShouldSetUser() {
    }

    public void testSwitchAndExecuteXpackSecurityUser() throws Exception {
-        SecurityContext securityContext = new SecurityContext(Settings.EMPTY, threadContext);


I yanked all of this into a common test case method below now that I have an extra case. I made an extra case for the tasks API because I had anticipated making the tasks user but now I've decided it is a good thing for a follow up.

nik9000 · 2018-11-16T20:59:15Z

...src/test/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStoreTests.java

@@ -279,18 +277,6 @@ public void testKibanaSystemRole() {
        assertThat(kibanaRole.indices().allowedIndicesMatcher(MultiSearchAction.NAME).test(index), is(true));
        assertThat(kibanaRole.indices().allowedIndicesMatcher(GetAction.NAME).test(index), is(true));
        assertThat(kibanaRole.indices().allowedIndicesMatcher(READ_CROSS_CLUSTER_NAME).test(index), is(false));
-
-        // Tasks index
-        final String taskIndex = org.elasticsearch.tasks.TaskResultsService.TASK_INDEX;


These shouldn't be required any more.

nik9000 · 2018-11-16T20:59:31Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ClientHelperTests.java

@@ -42,32 +42,6 @@
 import static org.mockito.Mockito.when;

 public class ClientHelperTests extends ESTestCase {
-
-    public void testStashContext() {


Moved to ThreadContextTests.

nik9000 · 2018-11-16T20:59:46Z

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java

@@ -118,9 +118,7 @@
                                RoleDescriptor.IndicesPrivileges.builder()
                                        .indices(".monitoring-*").privileges("read", "read_cross_cluster").build(),
                                RoleDescriptor.IndicesPrivileges.builder()
-                                        .indices(".management-beats").privileges("create_index", "read", "write").build(),
-                                RoleDescriptor.IndicesPrivileges.builder()


Shouldn't be needed any more.

nik9000 · 2018-11-16T21:03:36Z

server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TasksIT.java

@@ -725,12 +724,6 @@ public void testTasksWaitForAllTask() throws Exception {
    }

    public void testTaskStoringSuccesfulResult() throws Exception {
-        // Randomly create an empty index to make sure the type is created automatically


I removed this because it made writing the assertions about setting the origin very difficult and it isn't really a supported configuration.

nik9000 · 2018-11-16T21:04:10Z

server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TasksIT.java

@@ -743,23 +736,20 @@ public void testTaskStoringSuccesfulResult() throws Exception {
        TaskInfo taskInfo = events.get(0);
        TaskId taskId = taskInfo.getTaskId();

-        GetResponse resultDoc = client()


I replaced these with the tasks API calls because it gives me a good opportunity to assert that they find the documents while setting the origin.

nik9000 · 2018-11-16T21:04:39Z

server/src/main/java/org/elasticsearch/tasks/TaskResultsService.java

@@ -70,7 +71,7 @@

    @Inject
    public TaskResultsService(Client client, ClusterService clusterService) {
-        this.client = client;
+        this.client = new OriginSettingClient(client, "tasks");


OriginSettingClient removes a lot of the ceremony around this!

Should this be this.client = new OriginSettingClient(client, TASKS_ORIGIN)?

nik9000 · 2018-11-16T21:05:55Z

server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java

@@ -47,15 +45,14 @@
 */
 public class PersistentTasksService extends AbstractComponent {

-    private static final String ACTION_ORIGIN_TRANSIENT_NAME = "action.origin";


Now that the server has ThreadContext.stashWithOrigin and OriginSettingClient we can use it. I believe this change is a noop, and the tests seem to agree, but I'd love a second set of eyes on it.

nik9000 · 2018-11-16T21:07:14Z

server/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/get/GetTaskAction.java

@@ -25,6 +25,7 @@
 * Action for retrieving a list of currently running tasks
 */
 public class GetTaskAction extends Action<GetTaskResponse> {
+    public static final String TASKS_ORIGIN = "tasks";


I stuck this here because it felt like a fairly right place at the time. Now that I look at it again maybe it should be on something a little more centrally located. But it is about actions so I stuck it on an action. I'm not sure of a better spot.

imotov

Looks reasonable to me from the task management perspective, but I would leave it to @jaymode to review the security aspects.

imotov · 2018-11-19T21:46:04Z

server/src/main/java/org/elasticsearch/tasks/TaskResultsService.java

@@ -70,7 +71,7 @@

    @Inject
    public TaskResultsService(Client client, ClusterService clusterService) {
-        this.client = client;
+        this.client = new OriginSettingClient(client, "tasks");


Should this be this.client = new OriginSettingClient(client, TASKS_ORIGIN)?

imotov · 2018-11-19T21:51:34Z

.../main/java/org/elasticsearch/action/admin/cluster/node/tasks/get/TransportGetTaskAction.java

@@ -210,7 +212,7 @@ void getFinishedTaskFromIndex(Task thisTask, GetTaskRequest request, ActionListe
        GetRequest get = new GetRequest(TaskResultsService.TASK_INDEX, TaskResultsService.TASK_TYPE,
                request.getTaskId().toString());
        get.setParentTask(clusterService.localNode().getId(), thisTask.getId());
-        client.get(get, new ActionListener<GetResponse>() {
+        new OriginSettingClient(client, TASKS_ORIGIN).get(get, new ActionListener<GetResponse>() {


Any reason why this is not done once in the TransportGetTaskAction's constructor?

I had a good reason for doing it this way when I wrote it at first but now it doesn't seem like a good reason. I'm switching it to what you suggest.

jaymode

Overall I like the changes. I left a few comments

server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TestTaskPlugin.java

jaymode · 2018-11-27T22:05:22Z

server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TestTaskPlugin.java

+                public <T extends TransportResponse> void sendRequest(
+                        Transport.Connection connection, String action, TransportRequest request,
+                        TransportRequestOptions options, TransportResponseHandler<T> handler) {
+                    if (action.startsWith("indices:data/write/bulk[s]")) {


I think this would be simpler if this check got folded into shouldHaveOrigin

...plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationUtils.java

jaymode

LGTM

nik9000 · 2018-11-28T14:34:18Z

Thanks for reviewing @jaymode and @imotov!

About the BWC stuff - I think doing this without BWC would break a mixed cluster in new and intriguing ways. Ways that I'm not 100% ready to think about right now but will poke apart in the follow up that adds the user.

Right now using the `GET /_tasks/<taskid>` API and causing a task to opt in to saving its result after being completed requires permissions on the `.tasks` index. When we built this we thought that that was fine, but we've since moved towards not leaking details like "persisting task results after the task is completed is done by saving them into an index named `.tasks`." A more modern way of doing this would be to save the tasks into the index "under the hood" and to have APIs to manage the saved tasks. This is the first step down that road: it drops the requirement to have permissions to interact with the `.tasks` index when fetching task statuses and when persisting statuses beyond the lifetime of the task. In particular, this moves the concept of the "origin" of an action into a more prominent place in the Elasticsearch server. The origin of an action is ignored by the server, but the security plugin uses the origin to make requests on behalf of a user in such a way that the user need not have permissions to perform these actions. It *can* be made to be fairly precise. More specifically, we can create an internal user just for the tasks API that just has permission to interact with the `.tasks` index. This change doesn't do that, instead, it uses the ubiquitus "xpack" user which has most permissions because it is simpler. Adding the tasks user is something I'd like to get to in a follow up change. Instead, the majority of this change is about moving the "origin" concept from the security portion of x-pack into the server. This should allow any code to use the origin. To keep the change managable I've also opted to deprecate rather than remove the "origin" helpers in the security code. Removing them is almost entirely mechanical and I'd like to that in a follow up as well. Relates to #35573

nik9000 added >enhancement :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. v7.0.0 v6.6.0 labels Nov 16, 2018

nik9000 requested review from imotov and jaymode November 16, 2018 20:56

nik9000 commented Nov 16, 2018

View reviewed changes

nik9000 added 2 commits November 19, 2018 09:46

Merge branch 'master' into tasks_no_need_index_permission

02b3746

Silly

701cb55

imotov reviewed Nov 19, 2018

View reviewed changes

Fixup

18c8bd5

jaymode requested a review from tvernum November 26, 2018 21:49

Merge branch 'master' into tasks_no_need_index_permission

0771769

jaymode requested changes Nov 27, 2018

View reviewed changes

jaymode approved these changes Nov 28, 2018

View reviewed changes

nik9000 merged commit 0588dad into elastic:master Nov 28, 2018

This was referenced Nov 29, 2018

Security: Remove system context in favor of origin #36071

Open

Tasks: Create Internal Tasks User #36282

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

tvernum mentioned this pull request Jun 16, 2020

Provide a mechanism for securely indexing internally irrespective of license #58117

Closed

Tasks: Only require task permissions #35667

Tasks: Only require task permissions #35667

Uh oh!

Conversation

nik9000 commented Nov 16, 2018

Uh oh!

elasticmachine commented Nov 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Nov 28, 2018

Uh oh!

Uh oh!