-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution][Endpoint] Policy creation callback fixes + Improved error handling in user manifest loop #71269
Changes from all commits
6764d2c
53303ba
d856cf9
7519ee7
62b1a2c
cbe39ec
76939ee
a399e5e
c55ec69
127178a
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 |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { NewPackageConfig, PackageConfig } from './types/models/package_config'; | ||
|
||
export const createNewPackageConfigMock = () => { | ||
return { | ||
name: 'endpoint-1', | ||
description: '', | ||
namespace: 'default', | ||
enabled: true, | ||
config_id: '93c46720-c217-11ea-9906-b5b8a21b268e', | ||
output_id: '', | ||
package: { | ||
name: 'endpoint', | ||
title: 'Elastic Endpoint', | ||
version: '0.9.0', | ||
}, | ||
inputs: [], | ||
} as NewPackageConfig; | ||
}; | ||
|
||
export const createPackageConfigMock = () => { | ||
const newPackageConfig = createNewPackageConfigMock(); | ||
return { | ||
...newPackageConfig, | ||
id: 'c6d16e42-c32d-4dce-8a88-113cfe276ad1', | ||
version: 'abcd', | ||
revision: 1, | ||
updated_at: '2020-06-25T16:03:38.159292', | ||
updated_by: 'kibana', | ||
created_at: '2020-06-25T16:03:38.159292', | ||
created_by: 'kibana', | ||
inputs: [ | ||
{ | ||
config: {}, | ||
}, | ||
], | ||
} as PackageConfig; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
// eslint-disable-next-line @kbn/eslint/no-restricted-paths | ||
import { loggerMock } from 'src/core/server/logging/logger.mock'; | ||
import { createNewPackageConfigMock } from '../../../ingest_manager/common/mocks'; | ||
import { factory as policyConfigFactory } from '../../common/endpoint/models/policy_config'; | ||
import { getManifestManagerMock } from './services/artifacts/manifest_manager/manifest_manager.mock'; | ||
import { getPackageConfigCreateCallback } from './ingest_integration'; | ||
|
||
describe('ingest_integration tests ', () => { | ||
describe('ingest_integration sanity checks', () => { | ||
test('policy is updated with manifest', async () => { | ||
const logger = loggerMock.create(); | ||
const manifestManager = getManifestManagerMock(); | ||
const callback = getPackageConfigCreateCallback(logger, manifestManager); | ||
const policyConfig = createNewPackageConfigMock(); | ||
const newPolicyConfig = await callback(policyConfig); | ||
expect(newPolicyConfig.inputs[0]!.type).toEqual('endpoint'); | ||
expect(newPolicyConfig.inputs[0]!.config!.policy.value).toEqual(policyConfigFactory()); | ||
expect(newPolicyConfig.inputs[0]!.config!.artifact_manifest.value).toEqual({ | ||
artifacts: { | ||
'endpoint-exceptionlist-linux-v1': { | ||
compression_algorithm: 'zlib', | ||
decoded_sha256: '1a8295e6ccb93022c6f5ceb8997b29f2912389b3b38f52a8f5a2ff7b0154b1bc', | ||
decoded_size: 287, | ||
encoded_sha256: 'c3dec543df1177561ab2aa74a37997ea3c1d748d532a597884f5a5c16670d56c', | ||
encoded_size: 133, | ||
encryption_algorithm: 'none', | ||
relative_url: | ||
'/api/endpoint/artifacts/download/endpoint-exceptionlist-linux-v1/1a8295e6ccb93022c6f5ceb8997b29f2912389b3b38f52a8f5a2ff7b0154b1bc', | ||
}, | ||
}, | ||
manifest_version: 'WzAsMF0=', | ||
schema_version: 'v1', | ||
}); | ||
}); | ||
|
||
test('policy is returned even if error is encountered during artifact sync', async () => { | ||
const logger = loggerMock.create(); | ||
const manifestManager = getManifestManagerMock(); | ||
manifestManager.syncArtifacts = jest.fn().mockRejectedValue([new Error('error updating')]); | ||
const lastDispatched = await manifestManager.getLastDispatchedManifest(); | ||
const callback = getPackageConfigCreateCallback(logger, manifestManager); | ||
const policyConfig = createNewPackageConfigMock(); | ||
const newPolicyConfig = await callback(policyConfig); | ||
expect(newPolicyConfig.inputs[0]!.type).toEqual('endpoint'); | ||
expect(newPolicyConfig.inputs[0]!.config!.policy.value).toEqual(policyConfigFactory()); | ||
expect(newPolicyConfig.inputs[0]!.config!.artifact_manifest.value).toEqual( | ||
lastDispatched.toEndpointFormat() | ||
); | ||
}); | ||
|
||
test('initial policy creation succeeds if snapshot retrieval fails', async () => { | ||
const logger = loggerMock.create(); | ||
const manifestManager = getManifestManagerMock(); | ||
const lastDispatched = await manifestManager.getLastDispatchedManifest(); | ||
manifestManager.getSnapshot = jest.fn().mockResolvedValue(null); | ||
const callback = getPackageConfigCreateCallback(logger, manifestManager); | ||
const policyConfig = createNewPackageConfigMock(); | ||
const newPolicyConfig = await callback(policyConfig); | ||
expect(newPolicyConfig.inputs[0]!.type).toEqual('endpoint'); | ||
expect(newPolicyConfig.inputs[0]!.config!.policy.value).toEqual(policyConfigFactory()); | ||
expect(newPolicyConfig.inputs[0]!.config!.artifact_manifest.value).toEqual( | ||
lastDispatched.toEndpointFormat() | ||
); | ||
}); | ||
|
||
test('subsequent policy creations succeed', async () => { | ||
const logger = loggerMock.create(); | ||
const manifestManager = getManifestManagerMock(); | ||
const snapshot = await manifestManager.getSnapshot(); | ||
manifestManager.getLastDispatchedManifest = jest.fn().mockResolvedValue(snapshot!.manifest); | ||
manifestManager.getSnapshot = jest.fn().mockResolvedValue({ | ||
manifest: snapshot!.manifest, | ||
diffs: [], | ||
}); | ||
const callback = getPackageConfigCreateCallback(logger, manifestManager); | ||
const policyConfig = createNewPackageConfigMock(); | ||
const newPolicyConfig = await callback(policyConfig); | ||
expect(newPolicyConfig.inputs[0]!.type).toEqual('endpoint'); | ||
expect(newPolicyConfig.inputs[0]!.config!.policy.value).toEqual(policyConfigFactory()); | ||
expect(newPolicyConfig.inputs[0]!.config!.artifact_manifest.value).toEqual( | ||
snapshot!.manifest.toEndpointFormat() | ||
); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,9 @@ import { Logger } from '../../../../../src/core/server'; | |
import { NewPackageConfig } from '../../../ingest_manager/common/types/models'; | ||
import { factory as policyConfigFactory } from '../../common/endpoint/models/policy_config'; | ||
import { NewPolicyData } from '../../common/endpoint/types'; | ||
import { ManifestManager } from './services/artifacts'; | ||
import { ManifestManager, ManifestSnapshot } from './services/artifacts'; | ||
import { reportErrors, ManifestConstants } from './lib/artifacts/common'; | ||
import { ManifestSchemaVersion } from '../../common/endpoint/schema/common'; | ||
|
||
/** | ||
* Callback to handle creation of PackageConfigs in Ingest Manager | ||
|
@@ -29,58 +31,83 @@ export const getPackageConfigCreateCallback = ( | |
// follow the types/schema expected | ||
let updatedPackageConfig = newPackageConfig as NewPolicyData; | ||
|
||
// get snapshot based on exception-list-agnostic SOs | ||
// with diffs from last dispatched manifest, if it exists | ||
const snapshot = await manifestManager.getSnapshot({ initialize: true }); | ||
// get current manifest from SO (last dispatched) | ||
const manifest = ( | ||
await manifestManager.getLastDispatchedManifest(ManifestConstants.SCHEMA_VERSION) | ||
)?.toEndpointFormat() ?? { | ||
manifest_version: 'default', | ||
schema_version: ManifestConstants.SCHEMA_VERSION as ManifestSchemaVersion, | ||
artifacts: {}, | ||
}; | ||
|
||
if (snapshot === null) { | ||
logger.warn('No manifest snapshot available.'); | ||
return updatedPackageConfig; | ||
// Until we get the Default Policy Configuration in the Endpoint package, | ||
// we will add it here manually at creation time. | ||
if (newPackageConfig.inputs.length === 0) { | ||
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. Personally - I would remove this History:The 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. @paul-tavares Will inputs ever have length > 0 here? 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. @madirey - For endpoint Package Configs, no. At the moment we only expect 1 input at most that includes all of our data. |
||
updatedPackageConfig = { | ||
...newPackageConfig, | ||
inputs: [ | ||
{ | ||
type: 'endpoint', | ||
enabled: true, | ||
streams: [], | ||
config: { | ||
artifact_manifest: { | ||
value: manifest, | ||
}, | ||
policy: { | ||
value: policyConfigFactory(), | ||
}, | ||
}, | ||
}, | ||
], | ||
}; | ||
} | ||
|
||
if (snapshot.diffs.length > 0) { | ||
// create new artifacts | ||
await manifestManager.syncArtifacts(snapshot, 'add'); | ||
let snapshot: ManifestSnapshot | null = null; | ||
let success = true; | ||
try { | ||
// Try to get most up-to-date manifest data. | ||
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. Is what's returned here different from what you retrieved above in 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. @paul-tavares Possibly. Perhaps could look at renaming this function. The above builds a Manifest from the SO record of the last manifest dispatched. This function builds a manifest dynamically from the latest exception lists... there could be new items, so we try to get the most up-to-date data possible to avoid having to immediately re-dispatch. 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. @madirey thanks for explaining that. I'm not too familiar with the design around Lists, thus the questions 😃 . I think there is a process in place that periodically (on a timer) goes through and picks up new versions of lists and re-generates the Manifest (which pushes updates to the Package configs), right? If thats the case, I would suggest that we avoid the additional delays here in explicitly triggering that process, so that the overall Package Config creation is not incur a longer delay. |
||
|
||
// Until we get the Default Policy Configuration in the Endpoint package, | ||
// we will add it here manually at creation time. | ||
// @ts-ignore | ||
if (newPackageConfig.inputs.length === 0) { | ||
updatedPackageConfig = { | ||
...newPackageConfig, | ||
inputs: [ | ||
{ | ||
type: 'endpoint', | ||
enabled: true, | ||
streams: [], | ||
config: { | ||
artifact_manifest: { | ||
value: snapshot.manifest.toEndpointFormat(), | ||
}, | ||
policy: { | ||
value: policyConfigFactory(), | ||
}, | ||
}, | ||
}, | ||
], | ||
// get snapshot based on exception-list-agnostic SOs | ||
// with diffs from last dispatched manifest, if it exists | ||
snapshot = await manifestManager.getSnapshot({ initialize: true }); | ||
|
||
if (snapshot && snapshot.diffs.length) { | ||
// create new artifacts | ||
const errors = await manifestManager.syncArtifacts(snapshot, 'add'); | ||
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.
Sorry I see the method is handlePackageConfigCreate. There is a few condition blocks that may need to be tested to make sure all branches functions as expected. Is it possible to break this up so it may be more testable and probably maintainable. 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. @nnamdifrankie If new exception list entries were created, we may need to create new artifacts before sending the updated manifest. That's what 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. @XavierM and I have been talking about how to improve the way this callback happens... I think we will need some changes to make this more robust. Happy to discuss it @nnamdifrankie 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 think a test around what branches are supposed to exist together will suffice for now. Not sure what is the relationship between the snapshot and new package config. 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. @nnamdifrankie Just pushed some of the tests, working on more. 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. @nnamdifrankie @paul-tavares I've added several unit tests and have run through some manual tests. I think this fix will ensure that policy creation is not interrupted by any of the manifest processing code. |
||
if (errors.length) { | ||
reportErrors(logger, errors); | ||
throw new Error('Error writing new artifacts.'); | ||
} | ||
} | ||
|
||
if (snapshot) { | ||
updatedPackageConfig.inputs[0].config.artifact_manifest = { | ||
value: snapshot.manifest.toEndpointFormat(), | ||
}; | ||
} | ||
} | ||
|
||
try { | ||
return updatedPackageConfig; | ||
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 may be just be a personal choice 😄 I would propose (for clarify) that the 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. @paul-tavares It was an attempt to be sure that we return the data before doing other things... but we still don't have a good way to verify that the policy was actually created. We probably need to work together to figure out a better way to do this. Can we address in a follow-up to this PR? There are some important fixes here that will help us in the meantime. 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. @XavierM ^^ 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. Agreed @madirey Also - I'm tempted to suggest that if we refactor this, that we split up the concerns into 2 callbacks: one for handling only adding the policy data and a second to handle only adding the manifest information, an we register both callbacks with Ingest. We can work on this post 7.9 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. @paul-tavares As written, that should not happen, but I agree with the concerns. It's ugly. I can remove the section that pulls the snapshot and tries to get the most up-to-date information. That comes with a few caveats though:
In both of the cases above, we'll have to re-send the policy a minute later when the updates are detected. This is probably fine for now, but it may not be ideal when we have large deployments (could result in 2 back-to-back large rollouts if I'm not mistaken). |
||
} catch (err) { | ||
success = false; | ||
logger.error(err); | ||
return updatedPackageConfig; | ||
} finally { | ||
if (snapshot.diffs.length > 0) { | ||
// TODO: let's revisit the way this callback happens... use promises? | ||
// only commit when we know the package config was created | ||
if (success && snapshot !== null) { | ||
try { | ||
await manifestManager.commit(snapshot.manifest); | ||
if (snapshot.diffs.length > 0) { | ||
// TODO: let's revisit the way this callback happens... use promises? | ||
// only commit when we know the package config was created | ||
await manifestManager.commit(snapshot.manifest); | ||
|
||
// clean up old artifacts | ||
await manifestManager.syncArtifacts(snapshot, 'delete'); | ||
// clean up old artifacts | ||
await manifestManager.syncArtifacts(snapshot, 'delete'); | ||
} | ||
} catch (err) { | ||
logger.error(err); | ||
} | ||
} else if (snapshot === null) { | ||
logger.error('No manifest snapshot available.'); | ||
} | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,8 @@ | |
|
||
import { ILegacyScopedClusterClient, SavedObjectsClientContract } from 'kibana/server'; | ||
import { loggingSystemMock, savedObjectsServiceMock } from 'src/core/server/mocks'; | ||
// eslint-disable-next-line @kbn/eslint/no-restricted-paths | ||
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. We may want to ask the Kibana Platform team to include this in 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. @paul-tavares I see you approved, thank you! But let's follow up and figure out a better way to do this. +++ |
||
import { loggerMock } from 'src/core/server/logging/logger.mock'; | ||
import { xpackMocks } from '../../../../mocks'; | ||
import { | ||
AgentService, | ||
|
@@ -63,8 +65,8 @@ export const createMockEndpointAppContextServiceStartContract = (): jest.Mocked< | |
> => { | ||
return { | ||
agentService: createMockAgentService(), | ||
logger: loggerMock.create(), | ||
savedObjectsStart: savedObjectsServiceMock.createStartContract(), | ||
// @ts-ignore | ||
manifestManager: getManifestManagerMock(), | ||
registerIngestCallback: jest.fn< | ||
ReturnType<IngestManagerStartContract['registerExternalCallback']>, | ||
|
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.
Suggestion: add the return type to the function definition instead of casting. Same below