From 0a6dbab310466fbf6568868141f04b5d091a3c45 Mon Sep 17 00:00:00 2001 From: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Date: Mon, 13 Sep 2021 10:55:51 -0400 Subject: [PATCH] [Security Solution] Detect and fix corrupt artifacts (#111853) (#111947) * Recover from artifact not found errors in `getLastComputedManifest()` + some changes to logger calls * Recover from errors while retrieving the internal artifact map * Use `EndpointError` in ManifestManager and capture more metadata around error * Convert some logger.debug calls to logger.error + use EndpointError Co-authored-by: Paul Tavares <56442535+paul-tavares@users.noreply.github.com> --- .../endpoint/lib/artifacts/task.test.ts | 28 +++++++- .../server/endpoint/lib/artifacts/task.ts | 32 ++++++--- .../schemas/artifacts/saved_objects.ts | 4 ++ .../endpoint/services/artifacts/errors.ts | 14 ++++ .../manifest_manager/manifest_manager.test.ts | 70 +++++++++++++++++-- .../manifest_manager/manifest_manager.ts | 43 +++++++++--- .../server/endpoint/utils/wrap_errors.ts | 5 +- .../security_solution/server/plugin.ts | 2 +- 8 files changed, 170 insertions(+), 28 deletions(-) create mode 100644 x-pack/plugins/security_solution/server/endpoint/services/artifacts/errors.ts diff --git a/x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.test.ts b/x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.test.ts index b6fd384cdd646..a9e30e64a003b 100644 --- a/x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.test.ts +++ b/x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.test.ts @@ -16,6 +16,8 @@ import { ManifestManager } from '../../services/artifacts/manifest_manager'; import { buildManifestManagerMock } from '../../services/artifacts/manifest_manager/manifest_manager.mock'; import { InternalArtifactCompleteSchema } from '../../schemas/artifacts'; import { getMockArtifacts } from './mocks'; +import { InvalidInternalManifestError } from '../../services/artifacts/errors'; +import { loggingSystemMock } from '../../../../../../../src/core/server/mocks'; describe('task', () => { const MOCK_TASK_INSTANCE = { @@ -78,8 +80,9 @@ describe('task', () => { }); describe('Artifacts generation flow tests', () => { + let mockContext: ReturnType; + const runTask = async (manifestManager: ManifestManager) => { - const mockContext = createMockEndpointAppContext(); const mockTaskManager = taskManagerMock.createSetup(); const manifestTaskInstance = new ManifestTask({ @@ -112,6 +115,10 @@ describe('task', () => { ARTIFACT_TRUSTED_APPS_MACOS = artifacts[2]; }); + beforeEach(() => { + mockContext = createMockEndpointAppContext(); + }); + test('Should not run the process when no current manifest manager', async () => { const manifestManager = buildManifestManagerMock(); @@ -144,6 +151,25 @@ describe('task', () => { expect(manifestManager.deleteArtifacts).not.toHaveBeenCalled(); }); + test('Should recover if last Computed Manifest threw an InvalidInternalManifestError error', async () => { + const manifestManager = buildManifestManagerMock(); + const logger = loggingSystemMock.createLogger(); + const newManifest = ManifestManager.createDefaultManifest(); + + manifestManager.buildNewManifest = jest.fn().mockRejectedValue(newManifest); + mockContext.logFactory.get = jest.fn().mockReturnValue(logger); + manifestManager.getLastComputedManifest = jest.fn(async () => { + throw new InvalidInternalManifestError( + 'Internal Manifest map SavedObject is missing version' + ); + }); + + await runTask(manifestManager); + + expect(logger.info).toHaveBeenCalledWith('recovering from invalid internal manifest'); + expect(logger.error).toHaveBeenNthCalledWith(1, expect.any(InvalidInternalManifestError)); + }); + test('Should not bump version and commit manifest when no diff in the manifest', async () => { const manifestManager = buildManifestManagerMock(); diff --git a/x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.ts b/x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.ts index 8588a30aceb89..6a89c50b86973 100644 --- a/x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.ts +++ b/x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.ts @@ -14,7 +14,11 @@ import { import { EndpointAppContext } from '../../types'; import { getArtifactId, reportErrors } from './common'; import { InternalArtifactCompleteSchema } from '../../schemas/artifacts'; -import { isEmptyManifestDiff } from './manifest'; +import { isEmptyManifestDiff, Manifest } from './manifest'; +import { InvalidInternalManifestError } from '../../services/artifacts/errors'; +import { ManifestManager } from '../../services'; +import { wrapErrorIfNeeded } from '../../utils'; +import { EndpointError } from '../../errors'; export const ManifestTaskConstants = { TIMEOUT: '1m', @@ -87,7 +91,7 @@ export class ManifestTask { params: { version: ManifestTaskConstants.VERSION }, }); } catch (e) { - this.logger.debug(`Error scheduling task, received ${e.message}`); + this.logger.error(new EndpointError(`Error scheduling task, received ${e.message}`, e)); } }; @@ -112,15 +116,27 @@ export class ManifestTask { const manifestManager = this.endpointAppContext.service.getManifestManager(); if (manifestManager === undefined) { - this.logger.debug('Manifest Manager not available.'); + this.logger.error('Manifest Manager not available.'); return; } try { - // Last manifest we computed, which was saved to ES - const oldManifest = await manifestManager.getLastComputedManifest(); - if (oldManifest == null) { - this.logger.debug('User manifest not available yet.'); + let oldManifest: Manifest | null; + + try { + // Last manifest we computed, which was saved to ES + oldManifest = await manifestManager.getLastComputedManifest(); + } catch (e) { + // Lets recover from a failure in getting the internal manifest map by creating an empty default manifest + if (e instanceof InvalidInternalManifestError) { + this.logger.error(e); + this.logger.info('recovering from invalid internal manifest'); + oldManifest = ManifestManager.createDefaultManifest(); + } + } + + if (oldManifest! == null) { + this.logger.debug('Last computed manifest not available yet'); return; } @@ -159,7 +175,7 @@ export class ManifestTask { reportErrors(this.logger, deleteErrors); } } catch (err) { - this.logger.error(err); + this.logger.error(wrapErrorIfNeeded(err)); } }; } diff --git a/x-pack/plugins/security_solution/server/endpoint/schemas/artifacts/saved_objects.ts b/x-pack/plugins/security_solution/server/endpoint/schemas/artifacts/saved_objects.ts index 675ed41e394aa..0bb32cc10e571 100644 --- a/x-pack/plugins/security_solution/server/endpoint/schemas/artifacts/saved_objects.ts +++ b/x-pack/plugins/security_solution/server/endpoint/schemas/artifacts/saved_objects.ts @@ -73,6 +73,10 @@ export const internalManifestSchema = t.exact( semanticVersion, }) ); + +/** + * The Internal map of all artifacts that the ManifestManager knows about and is managing + */ export type InternalManifestSchema = t.TypeOf; export const internalManifestCreateSchema = t.intersection([ diff --git a/x-pack/plugins/security_solution/server/endpoint/services/artifacts/errors.ts b/x-pack/plugins/security_solution/server/endpoint/services/artifacts/errors.ts new file mode 100644 index 0000000000000..1810c46ef4313 --- /dev/null +++ b/x-pack/plugins/security_solution/server/endpoint/services/artifacts/errors.ts @@ -0,0 +1,14 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { EndpointError } from '../../errors'; + +/** + * Indicates that the internal manifest that is managed by ManifestManager is invalid or contains + * invalid data + */ +export class InvalidInternalManifestError extends EndpointError {} diff --git a/x-pack/plugins/security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.test.ts b/x-pack/plugins/security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.test.ts index e3dc66c20bb67..340137a530e6d 100644 --- a/x-pack/plugins/security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.test.ts +++ b/x-pack/plugins/security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.test.ts @@ -37,6 +37,8 @@ import { import { ManifestManager } from './manifest_manager'; import { EndpointArtifactClientInterface } from '../artifact_client'; +import { EndpointError } from '../../../errors'; +import { InvalidInternalManifestError } from '../errors'; const getArtifactObject = (artifact: InternalArtifactSchema) => JSON.parse(Buffer.from(artifact.body!, 'base64').toString()); @@ -104,11 +106,13 @@ describe('ManifestManager', () => { const manifestManager = new ManifestManager( buildManifestManagerContextMock({ savedObjectsClient }) ); - const error = { output: { statusCode: 500 } }; + const error = { message: 'bad request', output: { statusCode: 500 } }; savedObjectsClient.get = jest.fn().mockRejectedValue(error); - await expect(manifestManager.getLastComputedManifest()).rejects.toStrictEqual(error); + await expect(manifestManager.getLastComputedManifest()).rejects.toThrow( + new EndpointError('bad request', error) + ); }); test('Throws error when no version on the manifest', async () => { @@ -120,7 +124,7 @@ describe('ManifestManager', () => { savedObjectsClient.get = jest.fn().mockResolvedValue({}); await expect(manifestManager.getLastComputedManifest()).rejects.toStrictEqual( - new Error('No version returned for manifest.') + new InvalidInternalManifestError('Internal Manifest map SavedObject is missing version') ); }); @@ -208,6 +212,59 @@ describe('ManifestManager', () => { new Set([TEST_POLICY_ID_1, TEST_POLICY_ID_2]) ); }); + + test("Retrieve non empty manifest and skips over artifacts that can't be found", async () => { + const savedObjectsClient = savedObjectsClientMock.create(); + const manifestManagerContext = buildManifestManagerContextMock({ savedObjectsClient }); + const manifestManager = new ManifestManager(manifestManagerContext); + + savedObjectsClient.get = jest + .fn() + .mockImplementation(async (objectType: string, id: string) => { + if (objectType === ManifestConstants.SAVED_OBJECT_TYPE) { + return { + attributes: { + created: '20-01-2020 10:00:00.000Z', + schemaVersion: 'v2', + semanticVersion: '1.0.0', + artifacts: [ + { artifactId: ARTIFACT_ID_EXCEPTIONS_MACOS, policyId: undefined }, + { artifactId: ARTIFACT_ID_EXCEPTIONS_WINDOWS, policyId: undefined }, + { artifactId: ARTIFACT_ID_EXCEPTIONS_LINUX, policyId: undefined }, + { artifactId: ARTIFACT_ID_EXCEPTIONS_WINDOWS, policyId: TEST_POLICY_ID_1 }, + { artifactId: ARTIFACT_ID_TRUSTED_APPS_MACOS, policyId: TEST_POLICY_ID_1 }, + { artifactId: ARTIFACT_ID_TRUSTED_APPS_WINDOWS, policyId: TEST_POLICY_ID_1 }, + { artifactId: ARTIFACT_ID_TRUSTED_APPS_WINDOWS, policyId: TEST_POLICY_ID_2 }, + ], + }, + version: '2.0.0', + }; + } else { + return null; + } + }); + + (manifestManagerContext.artifactClient as jest.Mocked).getArtifact.mockImplementation( + async (id) => { + // report the MACOS Exceptions artifact as not found + return id === ARTIFACT_ID_EXCEPTIONS_MACOS ? undefined : ARTIFACTS_BY_ID[id]; + } + ); + + const manifest = await manifestManager.getLastComputedManifest(); + + expect(manifest?.getAllArtifacts()).toStrictEqual(ARTIFACTS.slice(1, 5)); + + expect(manifestManagerContext.logger.error).toHaveBeenCalledWith( + new InvalidInternalManifestError( + `artifact id [${ARTIFACT_ID_EXCEPTIONS_MACOS}] not found!`, + { + entry: ARTIFACTS_BY_ID[ARTIFACT_ID_EXCEPTIONS_MACOS], + action: 'removed from internal ManifestManger tracking map', + } + ) + ); + }); }); describe('buildNewManifest', () => { @@ -565,7 +622,10 @@ describe('ManifestManager', () => { ) ).resolves.toStrictEqual([ error, - new Error(`Incomplete artifact: ${ARTIFACT_ID_TRUSTED_APPS_MACOS}`), + new EndpointError( + `Incomplete artifact: ${ARTIFACT_ID_TRUSTED_APPS_MACOS}`, + ARTIFACTS_BY_ID[ARTIFACT_ID_TRUSTED_APPS_MACOS] + ), ]); expect(artifactClient.createArtifact).toHaveBeenCalledTimes(2); @@ -720,7 +780,7 @@ describe('ManifestManager', () => { ]); await expect(manifestManager.tryDispatch(manifest)).resolves.toStrictEqual([ - new Error(`Package Policy ${TEST_POLICY_ID_1} has no config.`), + new EndpointError(`Package Policy ${TEST_POLICY_ID_1} has no 'inputs[0].config'`), ]); expect(context.packagePolicyService.update).toHaveBeenCalledTimes(0); diff --git a/x-pack/plugins/security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.ts b/x-pack/plugins/security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.ts index f2d1d3660d78e..4c69aa1dd0737 100644 --- a/x-pack/plugins/security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.ts +++ b/x-pack/plugins/security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.ts @@ -34,6 +34,9 @@ import { import { EndpointArtifactClientInterface } from '../artifact_client'; import { ManifestClient } from '../manifest_client'; import { ExperimentalFeatures } from '../../../../../common/experimental_features'; +import { InvalidInternalManifestError } from '../errors'; +import { wrapErrorIfNeeded } from '../../../utils'; +import { EndpointError } from '../../../errors'; interface ArtifactsBuildResult { defaultArtifacts: InternalArtifactCompleteSchema[]; @@ -282,7 +285,7 @@ export class ManifestManager { newManifest.replaceArtifact(fleetArtifact); } } else { - errors.push(new Error(`Incomplete artifact: ${getArtifactId(artifact)}`)); + errors.push(new EndpointError(`Incomplete artifact: ${getArtifactId(artifact)}`, artifact)); } } return errors; @@ -310,8 +313,8 @@ export class ManifestManager { } /** - * Returns the last computed manifest based on the state of the - * user-artifact-manifest SO. + * Returns the last computed manifest based on the state of the user-artifact-manifest SO. If no + * artifacts have been created yet (ex. no Endpoint policies are in use), then method return `null` * * @returns {Promise} The last computed manifest, or null if does not exist. * @throws Throws/rejects if there is an unexpected error retrieving the manifest. @@ -321,7 +324,10 @@ export class ManifestManager { const manifestSo = await this.getManifestClient().getManifest(); if (manifestSo.version === undefined) { - throw new Error('No version returned for manifest.'); + throw new InvalidInternalManifestError( + 'Internal Manifest map SavedObject is missing version', + manifestSo + ); } const manifest = new Manifest({ @@ -334,16 +340,21 @@ export class ManifestManager { const artifact = await this.artifactClient.getArtifact(entry.artifactId); if (!artifact) { - throw new Error(`artifact id [${entry.artifactId}] not found!`); + this.logger.error( + new InvalidInternalManifestError(`artifact id [${entry.artifactId}] not found!`, { + entry, + action: 'removed from internal ManifestManger tracking map', + }) + ); + } else { + manifest.addEntry(artifact, entry.policyId); } - - manifest.addEntry(artifact, entry.policyId); } return manifest; } catch (error) { if (!error.output || error.output.statusCode !== 404) { - throw error; + throw wrapErrorIfNeeded(error); } return null; } @@ -381,7 +392,10 @@ export class ManifestManager { await iterateArtifactsBuildResult(result, async (artifact, policyId) => { const artifactToAdd = baselineManifest.getArtifact(getArtifactId(artifact)) || artifact; if (!internalArtifactCompleteSchema.is(artifactToAdd)) { - throw new Error(`Incomplete artifact detected: ${getArtifactId(artifactToAdd)}`); + throw new EndpointError( + `Incomplete artifact detected: ${getArtifactId(artifactToAdd)}`, + artifactToAdd + ); } manifest.addEntry(artifactToAdd, policyId); @@ -416,7 +430,12 @@ export class ManifestManager { const serializedManifest = manifest.toPackagePolicyManifest(packagePolicy.id); if (!manifestDispatchSchema.is(serializedManifest)) { - errors.push(new Error(`Invalid manifest for policy ${packagePolicy.id}`)); + errors.push( + new EndpointError( + `Invalid manifest for policy ${packagePolicy.id}`, + serializedManifest + ) + ); } else if (!manifestsEqual(serializedManifest, oldManifest.value)) { newPackagePolicy.inputs[0].config.artifact_manifest = { value: serializedManifest }; @@ -443,7 +462,9 @@ export class ManifestManager { this.logger.debug(`No change in manifest version for package policy: ${id}`); } } else { - errors.push(new Error(`Package Policy ${id} has no config.`)); + errors.push( + new EndpointError(`Package Policy ${id} has no 'inputs[0].config'`, newPackagePolicy) + ); } } ); diff --git a/x-pack/plugins/security_solution/server/endpoint/utils/wrap_errors.ts b/x-pack/plugins/security_solution/server/endpoint/utils/wrap_errors.ts index 475cd42bc802c..5ff4307574fcd 100644 --- a/x-pack/plugins/security_solution/server/endpoint/utils/wrap_errors.ts +++ b/x-pack/plugins/security_solution/server/endpoint/utils/wrap_errors.ts @@ -11,8 +11,9 @@ import { EndpointError } from '../errors'; * Will wrap the given Error with `EndpointError`, which will help getting a good picture of where in * our code the error originated (better stack trace). */ -export const wrapErrorIfNeeded = (error: Error): EndpointError => - error instanceof EndpointError ? error : new EndpointError(error.message, error); +export const wrapErrorIfNeeded = (error: Error): E => { + return (error instanceof EndpointError ? error : new EndpointError(error.message, error)) as E; +}; /** * used as the callback to `Promise#catch()` to ensure errors diff --git a/x-pack/plugins/security_solution/server/plugin.ts b/x-pack/plugins/security_solution/server/plugin.ts index e1af47de8f152..6bb4940882bbc 100644 --- a/x-pack/plugins/security_solution/server/plugin.ts +++ b/x-pack/plugins/security_solution/server/plugin.ts @@ -379,7 +379,7 @@ export class Plugin implements IPlugin