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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Endpoint] Policy creation callback fixes + Improved error handling in user manifest loop #71269

Merged
merged 10 commits into from
Jul 10, 2020
Merged

[Security Solution][Endpoint] Policy creation callback fixes + Improved error handling in user manifest loop #71269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6764d2c
Clean up matcher types
madirey Jul 9, 2020
53303ba
Rework promise and error-handling in ManifestManager
madirey Jul 9, 2020
d856cf9
Write tests for ingest callback and ensure policy is returned when er…
madirey Jul 9, 2020
7519ee7
More tests for ingest callback
madirey Jul 9, 2020
62b1a2c
Merge master
madirey Jul 9, 2020
cbe39ec
Update tests
madirey Jul 9, 2020
76939ee
Merge branch 'master' into user-manifest-cleanup
elasticmachine Jul 10, 2020
a399e5e
Fix tests
madirey Jul 10, 2020
c55ec69
Merge branch 'master' of github.com:elastic/kibana into user-manifest…
madirey Jul 10, 2020
127178a
Merge branch 'user-manifest-cleanup' of github.com:madirey/kibana int…
madirey Jul 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions x-pack/plugins/ingest_manager/common/mocks.ts
View file
Open in desktop
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;
Copy link
Contributor

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

};

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;
};
91 changes: 91 additions & 0 deletions x-pack/plugins/security_solution/server/endpoint/ingest_integration.test.ts
View file
Open in desktop
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()
);
});
});
});
105 changes: 66 additions & 39 deletions x-pack/plugins/security_solution/server/endpoint/ingest_integration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally - I would remove this if() and always insert our input into the Package Config.

History:

The if() is only here because at one point we thought that we would get data included in the Package Config at creation time by defining it in the Endpoint Package, thus why we were doing a conditional check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paul-tavares Will inputs ever have length > 0 here?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is what's returned here different from what you retrieved above in manifestManager.getLastDispatchedManifest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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');
Copy link
Contributor

@nnamdifrankie nnamdifrankie Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

how does syncArtifacts relates getPackageConfigCreateCallback. That appears to update something, perhaps a side effect?

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 syncArtifacts does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nnamdifrankie Just pushed some of the tests, working on more.

Copy link
Contributor Author

@madirey madirey Jul 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

this may be just be a personal choice 😄
I find this return here as well as the one inside of catch() confusing because one (a future "us" ) add another return to finally {} and that would be the value that is returned out of the function.

I would propose (for clarify) that the return be consolidated to one under the finally {} block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XavierM ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed @madirey
My point really was that the finally {} block could actually change what the function returns (ex. it could overwrite the return from either the catch() or the regular try {}.

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

Copy link
Contributor Author

@madirey madirey Jul 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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:

  1. There's no guarantee that we will have already created the manifest when the callback runs, so we may have to send an empty artifact list... the policy returned will not be deterministic on the first policy creation (artifacts may or may not exist), so that will have unit testing implications as well,
  2. If there have been new exception items added within the last minute, we won't pick it up.

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.');
}
}
};
Expand Down
7 changes: 7 additions & 0 deletions x-pack/plugins/security_solution/server/endpoint/lib/artifacts/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { Logger } from 'src/core/server';

export const ArtifactConstants = {
GLOBAL_ALLOWLIST_NAME: 'endpoint-exceptionlist',
Expand All @@ -16,3 +17,9 @@ export const ManifestConstants = {
SCHEMA_VERSION: 'v1',
INITIAL_VERSION: 'WzAsMF0=',
};

export const reportErrors = (logger: Logger, errors: Error[]) => {
errors.forEach((err) => {
logger.error(err);
});
};
26 changes: 22 additions & 4 deletions x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
TaskManagerStartContract,
} from '../../../../../task_manager/server';
import { EndpointAppContext } from '../../types';
import { reportErrors } from './common';

export const ManifestTaskConstants = {
TIMEOUT: '1m',
Expand Down Expand Up @@ -88,19 +89,36 @@ export class ManifestTask {
return;
}

let errors: Error[] = [];
try {
// get snapshot based on exception-list-agnostic SOs
// with diffs from last dispatched manifest
const snapshot = await manifestManager.getSnapshot();
if (snapshot && snapshot.diffs.length > 0) {
// create new artifacts
await manifestManager.syncArtifacts(snapshot, 'add');
errors = await manifestManager.syncArtifacts(snapshot, 'add');
if (errors.length) {
reportErrors(this.logger, errors);
throw new Error('Error writing new artifacts.');
}
// write to ingest-manager package config
await manifestManager.dispatch(snapshot.manifest);
errors = await manifestManager.dispatch(snapshot.manifest);
if (errors.length) {
reportErrors(this.logger, errors);
throw new Error('Error dispatching manifest.');
}
// commit latest manifest state to user-artifact-manifest SO
await manifestManager.commit(snapshot.manifest);
const error = await manifestManager.commit(snapshot.manifest);
if (error) {
reportErrors(this.logger, [error]);
throw new Error('Error committing manifest.');
}
// clean up old artifacts
await manifestManager.syncArtifacts(snapshot, 'delete');
errors = await manifestManager.syncArtifacts(snapshot, 'delete');
if (errors.length) {
reportErrors(this.logger, errors);
throw new Error('Error cleaning up outdated artifacts.');
}
}
} catch (err) {
this.logger.error(err);
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/security_solution/server/endpoint/mocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 src/core/server/mocks

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -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']>,
Expand Down
Loading