-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 #96063
Changes from all commits
4176e8f
8b90529
a57160f
754d6f9
c4b1137
579278c
e7053c4
2696b90
66b693f
b6b1337
fb90525
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -256,6 +256,7 @@ export enum RegistryDataStreamKeys { | |
ingest_pipeline = 'ingest_pipeline', | ||
elasticsearch = 'elasticsearch', | ||
dataset_is_prefix = 'dataset_is_prefix', | ||
permissions = 'permissions', | ||
} | ||
|
||
export interface RegistryDataStream { | ||
|
@@ -271,13 +272,24 @@ export interface RegistryDataStream { | |
[RegistryDataStreamKeys.ingest_pipeline]: string; | ||
[RegistryDataStreamKeys.elasticsearch]?: RegistryElasticsearch; | ||
[RegistryDataStreamKeys.dataset_is_prefix]?: boolean; | ||
[RegistryDataStreamKeys.permissions]?: RegistryDataStreamPermissions; | ||
} | ||
|
||
export interface RegistryElasticsearch { | ||
'index_template.settings'?: object; | ||
'index_template.mappings'?: object; | ||
} | ||
|
||
export interface RegistryDataStreamPermissions { | ||
cluster?: string[]; | ||
indices?: string[]; | ||
} | ||
|
||
export interface PackagePermissions { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure where to put this or how to call it. I'm open to suggestions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would move this to actually, what do you think of renaming like this?: |
||
cluster?: string[]; | ||
indices?: Array<{ names: string[]; privileges: string[] }>; | ||
} | ||
|
||
export type RegistryVarType = 'integer' | 'bool' | 'password' | 'text' | 'yaml' | 'string'; | ||
export enum RegistryVarsEntryKeys { | ||
name = 'name', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ import { | |
AGENT_POLICY_INDEX, | ||
DEFAULT_FLEET_SERVER_AGENT_POLICY, | ||
} from '../../common'; | ||
import type { FullAgentPolicyOutputPermissions, PackagePermissions } from '../../common'; | ||
import type { | ||
DeleteAgentPolicyResponse, | ||
Settings, | ||
|
@@ -52,7 +53,7 @@ import { | |
} from '../errors'; | ||
import { getFullAgentPolicyKibanaConfig } from '../../common/services/full_agent_policy_kibana_config'; | ||
|
||
import { getPackageInfo } from './epm/packages'; | ||
import { getPackageInfo, getPackagePermissions } from './epm/packages'; | ||
import { createAgentPolicyAction, getAgentsByKuery } from './agents'; | ||
import { packagePolicyService } from './package_policy'; | ||
import { outputService } from './output'; | ||
|
@@ -64,6 +65,16 @@ import { appContextService } from './app_context'; | |
|
||
const SAVED_OBJECT_TYPE = AGENT_POLICY_SAVED_OBJECT_TYPE; | ||
|
||
const DEFAULT_PERMISSIONS: PackagePermissions = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is used as a fallback in case we cannot read the data_streams for whatever reason |
||
cluster: ['monitor'], | ||
indices: [ | ||
{ | ||
names: ['logs-*', 'metrics-*', 'traces-*', '.logs-endpoint.diagnostic.collection-*'], | ||
privileges: ['auto_configure', 'create_doc'], | ||
}, | ||
], | ||
}; | ||
|
||
class AgentPolicyService { | ||
private triggerAgentPolicyUpdatedEvent = async ( | ||
soClient: SavedObjectsClientContract, | ||
|
@@ -739,24 +750,53 @@ class AgentPolicyService { | |
}), | ||
}; | ||
|
||
const hasTightPermissions = appContextService.getConfig()?.agents.agentPolicyTightPermissions; | ||
let permissions: FullAgentPolicyOutputPermissions; | ||
|
||
if (hasTightPermissions) { | ||
permissions = Object.fromEntries( | ||
await Promise.all( | ||
// Original type is `string[] | PackagePolicy[]`, but TS doesn't allow to `map()` over that. | ||
(agentPolicy.package_policies as Array<string | PackagePolicy>).map( | ||
async (packagePolicy): Promise<[string, PackagePermissions]> => { | ||
if (typeof packagePolicy === 'string' || !packagePolicy.package) { | ||
return ['_fallback', DEFAULT_PERMISSIONS]; | ||
} | ||
|
||
const { name, version } = packagePolicy.package; | ||
|
||
const packagePermissions = await getPackagePermissions( | ||
soClient, | ||
name, | ||
version, | ||
packagePolicy.namespace | ||
); | ||
|
||
return packagePermissions | ||
? [packagePolicy.name, packagePermissions] | ||
: ['_fallback', DEFAULT_PERMISSIONS]; | ||
} | ||
) | ||
) | ||
); | ||
permissions._elastic_agent_checks = { | ||
cluster: DEFAULT_PERMISSIONS.cluster, | ||
}; | ||
} else { | ||
permissions = { | ||
_fallback: DEFAULT_PERMISSIONS, | ||
}; | ||
} | ||
|
||
// Only add permissions if output.type is "elasticsearch" | ||
fullAgentPolicy.output_permissions = Object.keys(fullAgentPolicy.outputs).reduce< | ||
NonNullable<FullAgentPolicy['output_permissions']> | ||
>((permissions, outputName) => { | ||
>((p, outputName) => { | ||
const output = fullAgentPolicy.outputs[outputName]; | ||
if (output && output.type === 'elasticsearch') { | ||
permissions[outputName] = {}; | ||
permissions[outputName]._fallback = { | ||
cluster: ['monitor'], | ||
indices: [ | ||
{ | ||
names: ['logs-*', 'metrics-*', 'traces-*', '.logs-endpoint.diagnostic.collection-*'], | ||
privileges: ['auto_configure', 'create_doc'], | ||
}, | ||
], | ||
}; | ||
p[outputName] = permissions; | ||
} | ||
return permissions; | ||
return p; | ||
}, {}); | ||
|
||
// only add settings if not in standalone | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -224,24 +224,20 @@ export const getEsPackage = async ( | |
); | ||
const dataStreamManifest = safeLoad(soResDataStreamManifest.attributes.data_utf8); | ||
const { | ||
title: dataStreamTitle, | ||
release, | ||
ingest_pipeline: ingestPipeline, | ||
type, | ||
dataset, | ||
streams: manifestStreams, | ||
...dataStreamManifestProps | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the first commit message |
||
} = dataStreamManifest; | ||
const streams = parseAndVerifyStreams(manifestStreams, dataStreamPath); | ||
|
||
dataStreams.push({ | ||
dataset: dataset || `${pkgName}.${dataStreamPath}`, | ||
title: dataStreamTitle, | ||
release, | ||
package: pkgName, | ||
afgomez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
dataset: dataset || `${pkgName}.${dataStreamPath}`, | ||
ingest_pipeline: ingestPipeline || 'default', | ||
path: dataStreamPath, | ||
type, | ||
streams, | ||
...dataStreamManifestProps, | ||
}); | ||
}) | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the permissions will be per package or per integration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't they the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtojek @jen-huang Can you link to the related issues / PR's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theaws
package ships integrations likeAWS 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 singleaws
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 renamegetPackagePermissions
too) to decouple it from the concept of packages/integrations. I'll leave suggestions in other review comments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are
data_stream
s nested underpolicy_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