[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 <[email protected]>
elastic · Sep 13, 2021 · 0a6dbab · 0a6dbab
1 parent 94a74f5
commit 0a6dbab
Show file tree

Hide file tree

Showing 8 changed files with 170 additions and 28 deletions.
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
@@ -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<typeof createMockEndpointAppContext>;
+
     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
@@ -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
@@ -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([

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
@@ -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/...ity_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.test.ts b/...ity_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<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', () => {
@@ -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/...security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.ts b/...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<Manifest | null>} 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
@@ -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

diff --git 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<PluginSetup, PluginStart, SetupPlugins, S
               taskManager,
             });
           } else {
-            logger.debug('User artifacts task not available.');
+            logger.error(new Error('User artifacts task not available.'));
           }
         });
       });