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

Introduce reserved ml privilege for the apm_user role #72266

Merged
merged 12 commits into from
Jul 28, 2020

Conversation

legrego
Copy link
Member

@legrego legrego commented Jul 17, 2020

Adds a new Kibana "reserved" privilege to the Machine Learning feature, in order to grant access to a subset of ML functionality within Kibana. This new privilege will be granted to the builtin apm_user role via elastic/elasticsearch#59854.

Previously, the apm_user role was authorized to retrieve ML jobs by virtue of its privileges against the .ml-anomalies* set of indices. This access is no longer sufficient, now that the ML feature is taking advantage of the Kibana privilege model.

In order to maintain BWC for existing users of the apm_user role, it's necessary to create a reserved privilege which mimics the previous access that this role used to have. Eventually, the Kibana privilege model will be updated to support access to ML jobs in a more holistic manner, but that is a longer term initiative.

Corresponding Elasticsearch PR: elastic/elasticsearch#59854


If you were pinged for CODEOWNERS review, and you're not on the Security or ML teams:

This new reserved privilege is the first of its kind which needs to grant access to a feature's underlying functionality, without granting access to the UI application itself. As a result, this PR also takes additional steps to transition away from the navLinkId property of the feature, in favor of the app property (see #66217).

In short, this means that every feature which declares a navLinkId must also include that id as one of their app entries in the feature.


To fully test this end-to-end, you must run Elasticsearch from my branch: https://github.com/legrego/elasticsearch/tree/fc/reserved_ml_apm_user

Resolves: #72260

@legrego legrego changed the title introduce reserved ml privilege for the apm_user role Introduce reserved ml privilege for the apm_user role Jul 20, 2020
@legrego legrego added :ml release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.10.0 v7.9.0 v8.0.0 labels Jul 20, 2020
@legrego legrego marked this pull request as ready for review July 20, 2020 18:51
@legrego legrego requested review from a team as code owners July 20, 2020 18:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@@ -14,11 +14,11 @@ export const METRICS_FEATURE = {
order: 700,
icon: 'metricsApp',
navLinkId: 'metrics',
app: ['infra', 'kibana'],
app: ['infra', 'metrics', 'kibana'],
catalogue: ['infraops'],
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a silly question: why is this PR updating the infra app and not the apm app?

Copy link
Member Author

Choose a reason for hiding this comment

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

APM is already "compliant" with the new requirement that your navLinkId value must be included in your app array. Infra was one of the few that weren't already configured this way:

navLinkId: 'apm',
app: ['apm', 'kibana'],

@legrego legrego requested a review from jgowdyelastic July 21, 2020 09:41
@legrego legrego requested a review from azasypkin July 21, 2020 16:45
@legrego
Copy link
Member Author

legrego commented Jul 22, 2020

@elasticmachine merge upstream

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Code changes LGTM 👍

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

The integration between infra and ml seems to continue to work 👍

@legrego
Copy link
Member Author

legrego commented Jul 28, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
ml 4.3MB +383.0B 4.3MB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@legrego
Copy link
Member Author

legrego commented Jul 28, 2020

Thanks all for your reviews and testing, I appreciate it!

@legrego legrego merged commit 09b11b6 into elastic:master Jul 28, 2020
@legrego legrego deleted the ml/apm-user-fix branch July 28, 2020 11:44
legrego added a commit to legrego/kibana that referenced this pull request Jul 28, 2020
legrego added a commit to legrego/kibana that referenced this pull request Jul 28, 2020
legrego added a commit that referenced this pull request Jul 28, 2020
legrego added a commit that referenced this pull request Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.9.0 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Application privileges for ML to apm_user
7 participants