Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add elastic/enterprise-search-server service account #83325

Merged
merged 9 commits into from
Feb 11, 2022

Conversation

ioanatia
Copy link
Contributor

Add an elastic/enterprise-search-server service account similar to elastic/kibana and elastic/fleet-server.

Still WIP because I might need to adapt some tests and also need to test it a bit more with Enterprise Search.

@elasticsearchmachine elasticsearchmachine added v8.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jan 31, 2022
@ioanatia ioanatia force-pushed the enterprise_search_service_account branch from aca19ed to 4a2a37a Compare January 31, 2022 14:08
"enterprise-search-server",
new RoleDescriptor(
NAMESPACE + "/enterprise-search-server",
new String[] { "manage", "manage_ilm", "manage_index_templates", "manage_security", "monitor" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manage_security is a very powerful privilege which basically means the account has unbounded privilege. Is it necessary for enterprise search's use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enterprise Search needs to be able to create/modify native users and Elasticsearch roles.
Is there a different privilege that could be used for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification. In this case, manage_security is the privilege to go. We don't have separate privileges for working with users and roles only.

I understand this must be how Enterprise Search always works. But it would be great if we could learn about its details and potentially work out something that allows us moving away from manage_security in future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another point is about the manage privilege, it covers all manage_ilm, manage_index_templates and monitor (but not manage_security). It is also a powerful privilege. What actions do Enterprise Search need it for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manage_ilm - Enterprise Search creates several data streams for logging events, for example analytics events or API logs. These data streams have an ILM policy that is created by Enterprise Search.

manage_index_templates - we use composable index templates for these data streams to ensure that they are ECS compatible

monitor - Enterprise Search needs cluster information, such as the cluster health and node information. For example, when Enterprise Search starts, it waits for the cluster to be healthy. Node information is useful to determine what plugins are available, for example we use analysis-icu if it is available.

I understand this must be how Enterprise Search always works. But it would be great if we could learn about its details and potentially work out something that allows us moving away from manage_security in future.

I agree. Is there an issue if we remove this cluster privilege for the service account some time in future when Enterprise Search no longer needs it? I don't think it is, but just asking to be sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Is there an issue if we remove this cluster privilege for the service account some time in future when Enterprise Search no longer needs it? I don't think it is, but just asking to be sure.

If we don't have Enterprise Search on older versions talking to ES on new versions with reduced privieges, it should not be an issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ioanatia Thanks for clarifing the intended usage for manage_ilm, manage_index_templates and monitor. Do they already cover all that is necessary for Enterprise Search? Sorry if it wasn't clear, but my original question was whether we can drop manage if there is nothing else required outside of the three more fine-grained privileges. Examples of extra privileges granted by manage are manage_ml, manage_watcher, manage_slm, manage_pipeline etc. Do Enterprise Searh need any of these extra privileges?

Comment on lines 44 to 56
.privileges(
"create",
"create_index",
"delete",
"delete_index",
"index",
"manage",
"manage_ilm",
"monitor",
"read",
"view_index_metadata",
"write"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manage privilege covers create_index, delete_index, manage_ilm, monitor and view_index_metadata. The write privilege covers create, delete and index. So this list can be simplied down to ["read", "write", "manage"].

Above privileges combined are very close to the all privilege. The only actions that are not granted are proxy CCS related ones, e.g. internal:transport/proxy/indices:data/read/*. Is CCS something enterprise search needs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is CCS something enterprise search needs?

I don't think we need it at this stage, so we can just change this to ["read", "write", "manage"] as you suggested.

@ioanatia ioanatia changed the title [WIP] Add elastic/enterprise-search-server service account Add elastic/enterprise-search-server service account Feb 2, 2022
@mark-vieira mark-vieira added v8.2.0 and removed v8.1.0 labels Feb 2, 2022
@ioanatia ioanatia marked this pull request as ready for review February 4, 2022 13:15
@ioanatia ioanatia added the Team:Security Meta label for security team label Feb 4, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@ywangd ywangd added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Feb 6, 2022
@ywangd
Copy link
Member

ywangd commented Feb 6, 2022

The v7 REST compat test fails because there are now 3 service accounts instead of 2. This particular yaml test was not well written to be friendly for REST compat test. For now, I suggest that we skip it. I raised #83564 and will fix it with a separate PR. You can skip the test by apply the following patch. Thanks!

diff --git a/x-pack/plugin/build.gradle b/x-pack/plugin/build.gradle
index ce010eae6b3..1ec97158200 100644
--- a/x-pack/plugin/build.gradle
+++ b/x-pack/plugin/build.gradle
@@ -113,6 +113,7 @@ tasks.named("yamlRestTestV7CompatTransform").configure{ task ->
   task.skipTest("indices.freeze/20_stats/Translog stats on frozen indices", "#70192 -- the freeze index API is removed from 8.0")
   task.skipTest("indices.freeze/10_basic/Basic", "#70192 -- the freeze index API is removed from 8.0")
   task.skipTest("indices.freeze/10_basic/Test index options", "#70192 -- the freeze index API is removed from 8.0")
+  task.skipTest("service_accounts/10_basic/Test get service accounts", "new service accounts are added")
 
   task.replaceValueInMatch("_type", "_doc")
   task.addAllowedWarningRegex("\\[types removal\\].*")

@ioanatia ioanatia force-pushed the enterprise_search_service_account branch from a1d262e to c54ada6 Compare February 7, 2022 13:51
@ioanatia ioanatia requested a review from ywangd February 7, 2022 15:14
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Also answer my own question: I noticed that the test asserts privilege for updating cluster setting, which requires the manage cluster privilege.

Comment on lines 313 to 314
// manage
assertThat(role.cluster().check(ClusterHealthAction.NAME, request, authentication), is(true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: cluster health is covered by the monitor privilege.

@elasticsearchmachine
Copy link
Collaborator

Hi @ioanatia, I've created a changelog YAML for you.

@ioanatia ioanatia merged commit 8487b03 into elastic:master Feb 11, 2022
@ioanatia ioanatia deleted the enterprise_search_service_account branch February 11, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants