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.

Sign up for GitHub

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

Jump to bottom

Extend fleet-server service account privileges #82600

Merged
merged 4 commits into from
Jan 24, 2022
Merged

Extend fleet-server service account privileges #82600

merged 4 commits into from
Jan 24, 2022

Conversation

axw
Copy link
Member

@axw axw commented Jan 14, 2022

Allow elastic/fleet-server service account to additionally read, monitor, and refresh traces-apm.sampled-* data streams.

These data streams do not contain any sensitive information. Fleet-server itself does not need to perform these actions, but it creates API Keys for APM Server, which does need to.

Fixes elastic/fleet-server#1048

@elasticsearchmachine elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label Jan 14, 2022
@axw
Extend fleet-server service account privileges
21b0203 
Allow elastic/fleet-server service account to
additionally read, monitor, and refresh
traces-apm.sampled-* data streams.

These data streams do not contain any sensitive
information. Fleet-server itself does not need
to perform these actions, but it creates API
Keys for APM Server, which does need to.
@axw axw force-pushed the fleetserver-apmsampledtraces branch from df6d012 to 21b0203 Compare January 14, 2022 08:06
@axw axw added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Jan 14, 2022
@axw axw marked this pull request as ready for review January 14, 2022 10:35
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jan 14, 2022
@elasticmachine
Copy link
Collaborator

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

@axw axw added >bug and removed >enhancement labels Jan 14, 2022
@axw
Copy link
Member Author

axw commented Jan 14, 2022

Not really sure if this should be classified as a bug or not. It's expected that fleet-server should be able to issue API Keys to APM Server that allow it to read/monitor/manage traces-apm.sampled-*. It can't though, because the service account doesn't include the privileges.

@axw axw mentioned this pull request Jan 14, 2022
Closed
@ruflin
Copy link
Member

ruflin commented Jan 14, 2022

@axw I remember someone mentioned that this feature would land in 8.1? Is this already shipped?

ruflin
ruflin approved these changes Jan 19, 2022
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

This change LGTM. But I would like to also get @ph and @joshdover review on this before merging.

@mtojek @jsoriano FYI: The spec around what permissions would be allowed for package-spec get extended here.

@ruflin ruflin requested review from ph and joshdover January 19, 2022 09:02
@jsoriano
Copy link
Member

jsoriano commented Jan 19, 2022 •
edited
Loading

@mtojek @jsoriano FYI: The spec around what permissions would be allowed for package-spec get extended here.

I think that there is no limitation in the package-spec of the permissions that can be used 🤔

UPDATE: This is something requested, not done yet: elastic/package-spec#255

ywangd
ywangd approved these changes Jan 20, 2022
View reviewed changes
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

...ity/src/main/java/org/elasticsearch/xpack/security/authc/service/ElasticServiceAccounts.java Outdated Show resolved Hide resolved
@axw @ywangd
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
e4fe170 
…ecurity/authc/service/ElasticServiceAccounts.java

Co-authored-by: Yang Wang <ywangd@gmail.com>
axw
axw commented Jan 20, 2022
View reviewed changes
Copy link
Member Author

@axw axw left a comment

Choose a reason for hiding this comment

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

Update docs and tests to match privileges change

x-pack/docs/en/rest-api/security/get-service-accounts.asciidoc Outdated Show resolved Hide resolved
...t/src/javaRestTest/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountIT.java Outdated Show resolved Hide resolved
@axw
Copy link
Member Author

axw commented Jan 20, 2022

@elasticmachine update branch

@ywangd
Copy link
Member

ywangd commented Jan 20, 2022

Failure is genuine but not related to this PR. I raised #82840

@ywangd
Copy link
Member

ywangd commented Jan 20, 2022

@elasticmachine run elasticsearch-ci/part-2

@jsoriano jsoriano mentioned this pull request Jan 20, 2022
Open
@axw
Copy link
Member Author

axw commented Jan 24, 2022

In the interest of getting the issue in apm-server fixed, I'm going to merge this.

@ph @joshdover if you have concerns, please do still leave a review and I'll follow up.

@axw axw removed the v8.0.0 label Jan 24, 2022
@axw axw merged commit ef1f7a5 into elastic:master Jan 24, 2022
@axw axw deleted the fleetserver-apmsampledtraces branch January 24, 2022 02:21
@joshdover
Copy link
Contributor

Apologize, I meant to approve this PR before 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@ywangd ywangd ywangd approved these changes

@ph ph Awaiting requested review from ph

@joshdover joshdover Awaiting requested review from joshdover

Assignees
No one assigned
Labels
>bug 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.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Service tokens ignore privileges additionally defined by packages
7 participants
@axw @elasticmachine @ruflin @jsoriano @ywangd @joshdover @elasticsearchmachine