[Fleet] Tighten policy permissions #96063

afgomez · 2021-04-01T15:33:35Z

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

To do so we had to figure out what data_streams were added by each package, and a way to specify what permissions are required by each data_stream (pending issue with 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, add the following flag to your kibana.yml or kibana.dev.yml

xpack.fleet.agents.agentPolicyTightPermissions: true

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

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.

Returns a list of the necessary ES permissions based on the permissions specified in each data_stream. If no permissions are specified it returns a default set of permissions for each data_stream.

afgomez · 2021-04-01T15:42:26Z

x-pack/plugins/fleet/common/types/models/epm.ts

+ indices?: string[];
+}
+
+export interface PackagePermissions {


I'm not sure where to put this or how to call it. I'm open to suggestions

I would move this to models/agent_policy.ts right under FullAgentPolicyOutputPermissions

actually, what do you think of renaming like this?:
FullAgentPolicyOutputPermissions -> FullAgentPolicyOutputRoles
PackagePermissions -> FullAgentPolicyOutputRolePermissions

afgomez · 2021-04-01T15:42:51Z

x-pack/plugins/fleet/server/services/agent_policy.ts


 const SAVED_OBJECT_TYPE = AGENT_POLICY_SAVED_OBJECT_TYPE;

+const DEFAULT_PERMISSIONS: PackagePermissions = {


This is used as a fallback in case we cannot read the data_streams for whatever reason

afgomez · 2021-04-01T15:44:02Z

x-pack/plugins/fleet/server/services/agent_policy.ts

@@ -737,24 +749,41 @@ class AgentPolicyService {
 }),
 };

+ const permissions = Object.fromEntries(
+ await Promise.all(
+ // Original type is `string[] | PackagePolicy[]`, but TS doesn't allow to `map()` over that.


I'm not sure if string[] still applies, but it's on the type definition for agentPolicy.package_policies so I'm handling that case for now.

afgomez · 2021-04-01T15:46:20Z

x-pack/plugins/fleet/server/services/epm/archive/storage.ts

 dataset,
 streams: manifestStreams,
+ ...dataStreamManifestProps


See the first commit message

afgomez · 2021-04-01T15:49:35Z

x-pack/plugins/fleet/server/services/epm/packages/get.ts

+ }
+
+ let index = `${ds.type}-${ds.dataset}-${namespace}`;
+ if (ds.dataset_is_prefix) {


APM uses data_is_prefix. I'm not sure if the way I'm building the pattern is correct though. I'm assuming the prefix includes the namespace. So for a namespace default, the prefix should be something like logs-apm.thing-default-*

elasticmachine · 2021-04-01T15:50:04Z

Pinging @elastic/fleet (Feature:Fleet)

jfsiii

Only reviewed on the phone so far but it looks very good. I left one question about a removed property.

x-pack/plugins/fleet/server/services/epm/archive/storage.ts

ruflin · 2021-04-04T18:05:45Z

I suggest for Phase 1 we put this behind a configuration option. I'm worried that having this enabled might break unexpected things. Having it as a config option will allow us to get this in early and start testing it without impacting users. For example we could already enable it by default in all testing environments. And when we are confident it works, we can switch the default or even remove the config option.

ruflin · 2021-04-06T06:31:57Z

x-pack/plugins/fleet/common/types/models/agent_policy.ts

- privileges: string[];
- }>;
- };
+ [role: string]: PackagePermissions;


I wonder if the permissions will be per package or per integration?

aren't they the same?

A package can contain a list of integrations. Today it is only one but very soon it is more then one. That might explain it.

Is it documented anywhere how such a package will look like? Right now, packages and integrations are 100% interchangeable, at least as far EPM code is concerned.

@mtojek @jen-huang Can you link to the related issues / PR's?

Here is an example of a package that ships multiple integrations: elastic/integrations#767

In summary, this is done by a package shipping multiple policy_templates. Today all packages only ship one policy template. I am actively working on supporting multiple templates in #93315. You can imagine the end result being that the aws package ships integrations like AWS EC2, AWS DynamoDB, etc, which show up as separate tiles in our Integrations UI. However, multiple integrations doesn't change our concept of "package policies", a single aws package policy can be used to configure all of its available integrations.

Whether a package ships multiple integrations or not has no bearing on the resulting agent YAML, which is the code that is touched in this PR. The agent yaml is only concerned with inputs, input streams, and outputs. So I think it's more of a question of what to call this permissions data structure instead of PackagePermissions (and maybe rename getPackagePermissions too) to decouple it from the concept of packages/integrations. I'll leave suggestions in other review comments.

Are data_streams nested under policy_templates or under the package itself? If it's the latter then the code is valid in the sense that it returns the permissions for a package, but my understanding is that we don't necessarily want that, so we need to re-estate the #64634 issue.

Edit: Ok I see they're not https://epr.elastic.co/package/aws/0.5.0/

Let me give it a thought and see if we can link each data stream with the policy_templates somehow

x-pack/plugins/fleet/server/services/epm/packages/get.ts

ruflin

Great to see this behind a feature flag so we can get this in and start to play around with it.

You have quite a few tests on the fake dataset which is really useful. An additional thing I would like to see is running some tests against actual packages like apache-0.4.0 and then generate the permission output for it. The reason I assume this should be possible is because there are already today some integration tests against a package-registry distribution which is run as a docker container. @skh might be able to share some details here.

Update: I realised I need to add a bit more details on why I'm asking for the above. I find it hard to decide on the code if the outcome is exactly as expected. The mental model I have is an integration and the permission it needs. If I take an integration and see the related permissions it needs in the tests, it makes reviewing for me pretty simple. The other part here is that I think we will hit a lot of edge cases. We have an integration which does not work because a permission is missing or even there are too many. If we are able to just add an integration from the registry to our test suite, produce a golden file, we can directly see the permission our code creates and what we expected. Then we can iterate on the code to get it to the expected state. As we have already tests for many other integrations, we also assure that we didn't break any of these. Last, I expected we will add more functionality to this model. For example pre-creating indices, dynamic fields off based on certain flags in the policy. Having the actual integrations and golden files for the permissions for it, will make it easy to review again and expand.

ruflin · 2021-04-13T17:36:17Z

x-pack/plugins/fleet/common/types/models/agent_policy.ts

- privileges: string[];
- }>;
- };
+ [role: string]: PackagePermissions;


A package can contain a list of integrations. Today it is only one but very soon it is more then one. That might explain it.

kibanamachine · 2021-04-14T15:41:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: fb90525
Storybooks Preview
Pipeline Steps (look for red circles / failed steps)
Interpreting CI Failures

Failed CI Steps

Jest Unit Tests

Test Failures

Kibana Pipeline / jest / Jest Tests.x-pack/plugins/fleet/server/services/epm/packages.epm/permissions getPackagePermissions() Returns empty permissions if datasets are empty

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

Stack Trace

Error: expect(received).toMatchObject(expected)

- Expected  - 1
+ Received  + 1

  Object {
-   "cluster": Array [],
+   "cluster": undefined,
    "indices": Array [],
  }
    at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/fleet/server/services/epm/packages/permissions.test.ts:98:27)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at _callCircusTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:212:5)
    at _runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:149:3)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:63:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at run (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:176:21)
    at jestAdapter (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:109:19)
    at runTestInternal (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:380:16)
    at runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:472:34)
    at Object.worker (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/testWorker.js:133:12)

Kibana Pipeline / jest / Jest Tests.x-pack/plugins/fleet/server/services/epm/packages.epm/permissions getPackagePermissions() Returns default permissions

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

Stack Trace

Error: expect(received).toMatchObject(expected)

- Expected  - 3
+ Received  + 1

@@ -1,9 +1,7 @@
  Object {
-   "cluster": Array [
-     "monitor",
-   ],
+   "cluster": undefined,
    "indices": Array [
      Object {
        "names": Array [
          "logs-dataset-*",
        ],
    at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/fleet/server/services/epm/packages/permissions.test.ts:109:27)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at _callCircusTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:212:5)
    at _runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:149:3)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:63:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at run (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:176:21)
    at jestAdapter (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:109:19)
    at runTestInternal (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:380:16)
    at runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:472:34)
    at Object.worker (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/testWorker.js:133:12)

Kibana Pipeline / jest / Jest Tests.x-pack/plugins/fleet/server/services/epm/packages.epm/permissions getPackagePermissions() Returns default permissions for multiple datasets

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

Stack Trace

Error: expect(received).toMatchObject(expected)

- Expected  - 3
+ Received  + 1

@@ -1,9 +1,7 @@
  Object {
-   "cluster": Array [
-     "monitor",
-   ],
+   "cluster": undefined,
    "indices": Array [
      Object {
        "names": Array [
          "logs-dataset1-*",
        ],
    at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/fleet/server/services/epm/packages/permissions.test.ts:131:27)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at _callCircusTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:212:5)
    at _runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:149:3)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:63:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at run (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:176:21)
    at jestAdapter (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:109:19)
    at runTestInternal (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:380:16)
    at runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:472:34)
    at Object.worker (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/testWorker.js:133:12)

and 4 more failures, only showing the first 3.

Metrics [docs]

Page load bundle

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

id	before	after	diff
`fleet`	345.4KB	345.5KB	+49.0B

History

💚 Build #119259 succeeded b6b1337
💚 Build #119146 succeeded 2696b90
💔 Build #118667 failed 754d6f9
💚 Build #118699 succeeded c4b1137

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

cc @afgomez

jen-huang

What happens if an agent policy has more than one package policy for the same package? Will the permissions be duplicated for each package policy?

jen-huang · 2021-04-14T22:15:35Z

x-pack/plugins/fleet/common/types/models/agent_policy.ts

- privileges: string[];
- }>;
- };
+ [role: string]: PackagePermissions;


Here is an example of a package that ships multiple integrations: elastic/integrations#767

In summary, this is done by a package shipping multiple policy_templates. Today all packages only ship one policy template. I am actively working on supporting multiple templates in #93315. You can imagine the end result being that the aws package ships integrations like AWS EC2, AWS DynamoDB, etc, which show up as separate tiles in our Integrations UI. However, multiple integrations doesn't change our concept of "package policies", a single aws package policy can be used to configure all of its available integrations.

Whether a package ships multiple integrations or not has no bearing on the resulting agent YAML, which is the code that is touched in this PR. The agent yaml is only concerned with inputs, input streams, and outputs. So I think it's more of a question of what to call this permissions data structure instead of PackagePermissions (and maybe rename getPackagePermissions too) to decouple it from the concept of packages/integrations. I'll leave suggestions in other review comments.

jen-huang · 2021-04-14T22:17:36Z

x-pack/plugins/fleet/common/types/models/epm.ts

+ indices?: string[];
+}
+
+export interface PackagePermissions {


I would move this to models/agent_policy.ts right under FullAgentPolicyOutputPermissions

actually, what do you think of renaming like this?:
FullAgentPolicyOutputPermissions -> FullAgentPolicyOutputRoles
PackagePermissions -> FullAgentPolicyOutputRolePermissions

jen-huang · 2021-04-14T22:41:17Z

x-pack/plugins/fleet/server/services/epm/packages/permissions.ts

+
+import { getPackageInfo } from './get';
+
+export async function getPackagePermissions(


this could be renamed getRolePermissions to match with my other suggestion

afgomez · 2021-04-15T15:43:25Z

This PR is solving the wrong problem (getting package permissions) instead of getting the permissions that an agent needs.

I'm going to close it to begin with a clean slate in my head. I'll reuse some of the code in a new PR.

ruflin · 2021-04-15T18:58:02Z

@afgomez Lets try to find some time to sync up on this before you start on the next iteration. Would like to learn more :-)

afgomez · 2021-04-16T13:26:18Z

Superseeded by #97366

Alejandro Fernández Gómez added 3 commits April 1, 2021 14:43

Add getPackagePermissions method

8b90529

Returns a list of the necessary ES permissions based on the permissions specified in each data_stream. If no permissions are specified it returns a default set of permissions for each data_stream.

Expose tight permissions in the agent policy

a57160f

afgomez changed the title ~~[Fleet] Ensure packages' data_streams contain all metadata~~ [Fleet] Tighten policy permissions Apr 1, 2021

afgomez added Feature:Fleet Fleet team's agent central management project release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0 labels Apr 1, 2021

afgomez commented Apr 1, 2021

View reviewed changes

afgomez marked this pull request as ready for review April 1, 2021 15:50

afgomez requested a review from a team as a code owner April 1, 2021 15:50

jen-huang added the Team:Fleet Team label for Observability Data Collection Fleet team label Apr 1, 2021

jen-huang assigned afgomez Apr 1, 2021

jen-huang self-requested a review April 1, 2021 19:06

jfsiii reviewed Apr 3, 2021

View reviewed changes

x-pack/plugins/fleet/server/services/epm/archive/storage.ts Show resolved Hide resolved

ruflin reviewed Apr 6, 2021

View reviewed changes

Alejandro Fernández Gómez added 5 commits April 12, 2021 13:42

Restore missing package prop

754d6f9

Merge branch 'master' into 64634-tight-policy-permissions

c4b1137

Add config flag to enable/disable the permissions

579278c

Use config flag to send tight permissions with the agent

e7053c4

Merge branch 'master' into 64634-tight-policy-permissions

2696b90

afgomez requested a review from ruflin April 13, 2021 11:11

Alejandro Fernández Gómez added 2 commits April 13, 2021 16:48

Extract getPackagePermissions to its own file

66b693f

Add tests to getPackagePermissions

b6b1337

ruflin reviewed Apr 13, 2021

View reviewed changes

Move default cluster permissions out of getPackagePermissions

fb90525

jen-huang reviewed Apr 14, 2021

View reviewed changes

afgomez closed this Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] Tighten policy permissions #96063

[Fleet] Tighten policy permissions #96063

afgomez commented Apr 1, 2021 •

edited

Loading

afgomez Apr 1, 2021

jen-huang Apr 14, 2021

afgomez Apr 1, 2021

afgomez Apr 1, 2021

afgomez Apr 1, 2021

afgomez Apr 1, 2021

elasticmachine commented Apr 1, 2021

jfsiii left a comment

ruflin commented Apr 4, 2021

ruflin Apr 6, 2021

afgomez Apr 12, 2021

ruflin Apr 13, 2021

skh Apr 13, 2021

ruflin Apr 13, 2021

jen-huang Apr 14, 2021

afgomez Apr 15, 2021 •

edited

Loading

ruflin left a comment •

edited

Loading

ruflin Apr 13, 2021

kibanamachine commented Apr 14, 2021 •

edited

Loading

Standard Out

Stack Trace

Standard Out

Stack Trace

Standard Out

Stack Trace

jen-huang left a comment

jen-huang Apr 14, 2021

jen-huang Apr 14, 2021

jen-huang Apr 14, 2021

afgomez commented Apr 15, 2021

ruflin commented Apr 15, 2021

afgomez commented Apr 16, 2021


		const SAVED_OBJECT_TYPE = AGENT_POLICY_SAVED_OBJECT_TYPE;

		const DEFAULT_PERMISSIONS: PackagePermissions = {


		import { getPackageInfo } from './get';

		export async function getPackagePermissions(

[Fleet] Tighten policy permissions #96063

[Fleet] Tighten policy permissions #96063

Conversation

afgomez commented Apr 1, 2021 • edited Loading

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 1, 2021

jfsiii left a comment

Choose a reason for hiding this comment

ruflin commented Apr 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afgomez Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

ruflin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Apr 14, 2021 • edited Loading

💔 Build Failed

Failed CI Steps

Standard Out

Stack Trace

Standard Out

Stack Trace

Standard Out

Stack Trace

Metrics [docs]

Page load bundle

History

jen-huang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afgomez commented Apr 15, 2021

ruflin commented Apr 15, 2021

afgomez commented Apr 16, 2021

afgomez commented Apr 1, 2021 •

edited

Loading

afgomez Apr 15, 2021 •

edited

Loading

ruflin left a comment •

edited

Loading

kibanamachine commented Apr 14, 2021 •

edited

Loading