Skip to content

Commit

Permalink
[Security Solution] Detect and fix corrupt artifacts (elastic#111853) (
Browse files Browse the repository at this point in the history
…elastic#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 <[email protected]>
  • Loading branch information
kibanamachine and paul-tavares authored Sep 13, 2021
1 parent 94a74f5 commit 0a6dbab
Show file tree
Hide file tree
Showing 8 changed files with 170 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -78,8 +80,9 @@ describe('task', () => {
});

describe('Artifacts generation flow tests', () => {
let mockContext: ReturnType<typeof createMockEndpointAppContext>;

const runTask = async (manifestManager: ManifestManager) => {
const mockContext = createMockEndpointAppContext();
const mockTaskManager = taskManagerMock.createSetup();

const manifestTaskInstance = new ManifestTask({
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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));
}
};

Expand All @@ -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;
}

Expand Down Expand Up @@ -159,7 +175,7 @@ export class ManifestTask {
reportErrors(this.logger, deleteErrors);
}
} catch (err) {
this.logger.error(err);
this.logger.error(wrapErrorIfNeeded(err));
}
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof internalManifestSchema>;

export const internalManifestCreateSchema = t.intersection([
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {}
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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')
);
});

Expand Down Expand Up @@ -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<EndpointArtifactClientInterface>).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', () => {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<Manifest | null>} The last computed manifest, or null if does not exist.
* @throws Throws/rejects if there is an unexpected error retrieving the manifest.
Expand All @@ -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({
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 };

Expand All @@ -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)
);
}
}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <E extends EndpointError = EndpointError>(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
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security_solution/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S
taskManager,
});
} else {
logger.debug('User artifacts task not available.');
logger.error(new Error('User artifacts task not available.'));
}
});
});
Expand Down

0 comments on commit 0a6dbab

Please sign in to comment.