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

[Fleet] Tighten policy permissions, take II #97366

Merged

Conversation

afgomez
Copy link
Contributor

@afgomez afgomez commented Apr 16, 2021

Summary

Closes #64634.

This PR refines the work done in #94591. It narrows down the API key permissions that each agent needs based on the integrations that the policy uses.

To do so we look into the policy's PacakgePolicy and the enabled inputs. We link that with the package DataStream to determine which indices it uses and which permissions it needs. The permissions will be specified in the package's data_streams/*/manifest.yml (pending proposal).

At the moment of writing no packages specify what permissions they need, so I'm using the permissions that are necessary right now as defaults.

Testing

To test this PR:

  1. Go to Fleet > Policies
  2. Click on any policy
  3. Click on Actions > View policy

The policy should have an output_permissions block, with several permissions per package instead of the generic logs-*, metrics-*, etc.

Checklist

Delete any items that are not applicable to this PR.

@afgomez afgomez added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Fleet Fleet team's agent central management project Team:Fleet Team label for Observability Data Collection Fleet team v7.13.0 labels Apr 16, 2021
@afgomez afgomez force-pushed the 64634-policy-permissions-per-package-policy branch from 3e552e0 to c17f064 Compare April 16, 2021 10:49
@afgomez afgomez mentioned this pull request Apr 16, 2021
2 tasks
@ruflin ruflin added v7.14.0 and removed v7.13.0 labels Apr 18, 2021
@afgomez afgomez force-pushed the 64634-policy-permissions-per-package-policy branch 3 times, most recently from 368748d to 8b1426f Compare May 10, 2021 13:59
Alejandro Fernández Gómez added 8 commits May 25, 2021 15:22
Prepare code to provide tighth permissions as well.
We can read the package information from two places
- The EPM registry (method `getRegistryPackage()`)
- The local ES cache (method `getEsPackage()`)

The package contains a property called `data_streams`. The contents of
this property varied depending on from where the package was being
loaded. The code in `getEsPackage` was cherry-picking what properties to
load to do validation.

We have changed the code so we cherry-pick only the properties that need
some sort of validation, and pass the others in bulk.
@afgomez afgomez force-pushed the 64634-policy-permissions-per-package-policy branch from 8b1426f to dcc4c47 Compare May 25, 2021 13:22
@afgomez afgomez marked this pull request as ready for review May 25, 2021 13:22
@afgomez afgomez requested a review from a team as a code owner May 25, 2021 13:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Feature:Fleet)

@ruflin
Copy link
Member

ruflin commented May 25, 2021

My assumption is that as long as a package does not specify any more fine grained permissions the permissions granted would be to append data for the data streams specified in the package as a default. Is this part of this PR?

@afgomez
Copy link
Contributor Author

afgomez commented May 25, 2021

My assumption is that as long as a package does not specify any more fine grained permissions the permissions granted would be to append data for the data streams specified in the package as a default. Is this part of this PR?

@ruflin yes, and only for the enabled data streams.

For example, if a user adds the apache integration with only logs (no metrics), it will only give permissions to that agent to logs-apache-{namespace}.

@jen-huang
Copy link
Contributor

Hi @afgomez, in addition to the edge cases we discussed (Custom Logs, Endpoint, APM, etc), I remembered another set of permissions we need to add. If agent monitoring is enabled, we need to add permissions for those indices: elastic/beats#25350 (comment).

monitoring_enabled on agent policy can contain logs and/or metrics or both, so we need to add permissions for whatever is enabled, i.e. logs-elastic_agent.*-{AGENT POLICY NAMESPACE} and metrics-elastic_agent.*-{AGENT POLICY NAMESPACE} (I think those are the right patterns, but you might want to double check with agent team).

@afgomez afgomez requested a review from jen-huang June 3, 2021 08:01
@afgomez
Copy link
Contributor Author

afgomez commented Jun 3, 2021

One more thought: as it currently stands, users won't get these new agent policies with permissions until a "real" change is triggered on their policies (from any user update on the policy, or a global Settings change)

I'm assuming the agent releases are tied to the rest of the stack. How does it work right now when the user upgrades the stack?

  1. A user upgrades kibana to 7.14
  2. Then it upgrades the agents to 7.14

What happens in the second step? Will the agent fetch its policy then?

@afgomez
Copy link
Contributor Author

afgomez commented Jun 3, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
fleet 981 985 +4

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 425.1KB 425.2KB +49.0B
Unknown metric groups

API count

id before after diff
fleet 1071 1075 +4

History

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

cc @afgomez

@jen-huang jen-huang self-requested a review June 8, 2021 16:58
Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Thank you for making all the changes! I tested locally, Kibana only didn't test with agent usage yet, but the agent YAML LGTM 🚀

cluster: DEFAULT_PERMISSIONS.cluster,
};

// TODO fetch this from the elastic agent package
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you file an issue for this and link elastic/integrations#953 as a blocker?

@jen-huang jen-huang added release_note:enhancement auto-backport Deprecated - use backport:version if exact versions are needed and removed release_note:skip Skip the PR/issue when compiling release notes labels Jun 8, 2021
@ruflin
Copy link
Member

ruflin commented Jun 9, 2021

One thing that would be nice to do on this PR would be to run the e2e-tests once before merging, it should show if most permissions we need are still in place. If I remember correctly this is possible. @mdelapenya should be able to help here.

@mdelapenya
Copy link
Contributor

/run-fleet-e2e-tests

@mdelapenya
Copy link
Contributor

mdelapenya commented Jun 9, 2021

One thing that would be nice to do on this PR would be to run the e2e-tests once before merging, it should show if most permissions we need are still in place. If I remember correctly this is possible. @mdelapenya should be able to help here.

I started the tests with the above command. The result is:

@mdelapenya
Copy link
Contributor

  • Once built, tests will use docker.elastic.co/observability-ci/kibana:pr97366 as image (will update this comment with the link to the job, as we did not decide how to contribute back the CI notifications)

The job finished with errors: https://beats-ci.elastic.co/blue/organizations/jenkins/e2e-tests%2Fe2e-testing-mbp/detail/master/965/tests

  • ARM errors: do not pay attention to them, as we discovered the kibana image for PRs is not built on ARM (work in progress)
  • There are 3 potential errors:

All three fail because of timeouts: not reaching a desired state during a time window defined by max timeout (5-10 minutes!). The first one passes for Centos but not for Debian.

@afgomez afgomez merged commit cd8cf46 into elastic:master Jun 9, 2021
@afgomez afgomez deleted the 64634-policy-permissions-per-package-policy branch June 9, 2021 14:11
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 9, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 9, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Alejandro Fernández Gómez <alejandro.fernandez@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 9, 2021
* master:
  clarify which parts of TM are experimental (elastic#101757)
  Add sh scripts with _bulk_action route usage examples (elastic#101736)
  [Uptime] Only register route in side nav if uptime show capability is true (elastic#101709)
  Use KIBANA_DOCS in doc link service (elastic#101667)
  [Alerting][Event log] Persisting duration information for active alerts in event log (elastic#101387)
  Address design issues in Discover/Graph (elastic#101584)
  Optimize performance for document table (elastic#101715)
  Change file data visualizer links to point to new location in home application (elastic#101393)
  [Fleet] Tighten policy permissions, take II (elastic#97366)
  [ML] Add debounce to the severity control update  (elastic#101581)
  [Fleet] Fix routing issues with `getPath` and `history.push` (elastic#101658)
  [APM] Add link-to/transaction route (elastic#101731)
  [Index Patterns] Runtime fields CRUD REST API  (elastic#101164)
  [ILM] Refactor types and fix missing aria labels (elastic#101518)
  [Lens] New summary row feature for datatable (elastic#101075)
  Blocks save event filter with a white space name (elastic#101599)
  Improve security server types (elastic#101661)
  [APM] Replace side nav with tabs on Settings page (elastic#101460)
  [APM] Only register items in side nav if user has permissions to see app (elastic#101707)
  [Security solution][Endpoint] Add back button when to the event filters list (elastic#101280)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Fleet Fleet team's agent central management project release_note:enhancement Team:Fleet Team label for Observability Data Collection Fleet team v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Extract indices from configs
6 participants