PR fixes

elastic · Jul 22, 2021 · 1365d05 · 1365d05
1 parent 501b82b
commit 1365d05
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 98 deletions.
diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts
@@ -2353,7 +2353,7 @@ describe('Task Runner', () => {
       mockedTaskInstance,
       taskRunnerFactoryInitializerParams
     );
-    alertsClient.get.mockResolvedValue(mockedAlertTypeSavedObject);
+    rulesClient.get.mockResolvedValue(mockedAlertTypeSavedObject);
     encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce({
       id: '1',
       type: 'alert',

diff --git a/x-pack/plugins/encrypted_saved_objects/server/create_migration.test.ts b/x-pack/plugins/encrypted_saved_objects/server/create_migration.test.ts
@@ -291,7 +291,7 @@ describe('createMigration()', () => {
       const instantiateServiceWithLegacyType = jest.fn(() =>
         encryptedSavedObjectsServiceMock.create()
       );
-      const migrationFunc = jest.fn((doc) => {
+      const migrationFunc = jest.fn(() => {
         throw new Error('migration failed!');
       });
 
@@ -342,7 +342,7 @@ describe('createMigration()', () => {
       const instantiateServiceWithLegacyType = jest.fn(() =>
         encryptedSavedObjectsServiceMock.create()
       );
-      const migrationFunc = jest.fn((doc) => {
+      const migrationFunc = jest.fn(() => {
         throw new Error('migration failed!');
       });
 

diff --git a/x-pack/plugins/encrypted_saved_objects/server/create_migration.ts b/x-pack/plugins/encrypted_saved_objects/server/create_migration.ts
@@ -97,28 +97,20 @@ export const getCreateMigration = (
     // if we are continuing the migration, strip encrypted attributes from the document using stripOrDecryptAttributesSync
     const documentToMigrate = mapAttributes(encryptedDoc, (inputAttributes) => {
       try {
-        const decryptedAttributes = inputService.decryptAttributesSync<any>(
-          decryptDescriptor,
-          inputAttributes,
-          { convertToMultiNamespaceType }
-        );
-        return decryptedAttributes;
+        return inputService.decryptAttributesSync<any>(decryptDescriptor, inputAttributes, {
+          convertToMultiNamespaceType,
+        });
       } catch (err) {
         if (!shouldMigrateIfDecryptionFails || !(err instanceof EncryptionError)) {
           throw err;
         }
 
         context.log.warn(
-          `Decryption failed for encrypted Saved Object "${encryptedDoc.id}" of type "${encryptedDoc.type}" with error: ${err.message}. Migration will be applied to the original encrypted document but this may cause decryption errors later on.`
-        );
-        const { attributes: strippedAttributes } = inputService.stripOrDecryptAttributesSync<any>(
-          decryptDescriptor,
-          inputAttributes,
-          {
-            convertToMultiNamespaceType,
-          }
+          `Decryption failed for encrypted Saved Object "${encryptedDoc.id}" of type "${encryptedDoc.type}" with error: ${err.message}. Encrypted attributes have been stripped from the original document and migration will be applied but this may cause decryption errors later on.`
         );
-        return strippedAttributes;
+        return inputService.stripOrDecryptAttributesSync<any>(decryptDescriptor, inputAttributes, {
+          convertToMultiNamespaceType,
+        }).attributes;
       }
     });
 

diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts
@@ -168,82 +168,78 @@ export class EncryptedSavedObjectsService {
    */
   public async stripOrDecryptAttributes<T extends Record<string, unknown>>(
     descriptor: SavedObjectDescriptor,
-    attributes: T,
+    attributesToStripOrDecrypt: T,
     originalAttributes?: T,
     params?: DecryptParameters
   ) {
-    const typeDefinition = this.typeDefinitions.get(descriptor.type);
-    if (typeDefinition === undefined) {
-      return { attributes };
-    }
-
-    let decryptedAttributes: T | null = null;
-    let decryptionError: Error | undefined;
-    const clonedAttributes: Record<string, unknown> = {};
-    for (const [attributeName, attributeValue] of Object.entries(attributes)) {
-      // We should strip encrypted attribute if definition explicitly mandates that or decryption
-      // failed.
-      if (
-        typeDefinition.shouldBeStripped(attributeName) ||
-        (!!decryptionError && typeDefinition.shouldBeEncrypted(attributeName))
-      ) {
-        continue;
-      }
-
-      // If attribute isn't supposed to be encrypted, just copy it to the resulting attribute set.
-      if (!typeDefinition.shouldBeEncrypted(attributeName)) {
-        clonedAttributes[attributeName] = attributeValue;
-      } else if (originalAttributes) {
-        // If attribute should be decrypted, but we have original attributes used to create object
-        // we should get raw unencrypted value from there to avoid performance penalty.
-        clonedAttributes[attributeName] = originalAttributes[attributeName];
-      } else {
-        // Otherwise just try to decrypt attribute. We decrypt all attributes at once, cache it and
-        // reuse for any other attributes.
-        if (decryptedAttributes === null) {
-          try {
-            decryptedAttributes = await this.decryptAttributes(
-              descriptor,
-              // Decrypt only attributes that are supposed to be exposed.
-              Object.fromEntries(
-                Object.entries(attributes).filter(([key]) => !typeDefinition.shouldBeStripped(key))
-              ) as T,
-              params
-            );
-          } catch (err) {
-            decryptionError = err;
-            continue;
-          }
-        }
-
-        clonedAttributes[attributeName] = decryptedAttributes[attributeName];
-      }
+    const { attributes, attributesToDecrypt } = this.prepareAttributesForStripOrDecrypt(
+      descriptor,
+      attributesToStripOrDecrypt,
+      originalAttributes
+    );
+    try {
+      const decryptedAttributes = attributesToDecrypt
+        ? await this.decryptAttributes(descriptor, attributesToDecrypt, params)
+        : {};
+      return { attributes: { ...attributes, ...decryptedAttributes } };
+    } catch (error) {
+      return { attributes, error };
     }
-
-    return { attributes: clonedAttributes as T, error: decryptionError };
   }
 
+  /**
+   * Takes saved object attributes for the specified type and, depending on the type definition,
+   * either decrypts or strips encrypted attributes (e.g. in case AAD or encryption key has changed
+   * and decryption is no longer possible).
+   * @param descriptor Saved object descriptor (ID, type and optional namespace)
+   * @param attributesToStripOrDecrypt Object that includes a dictionary of __ALL__ saved object attributes stored
+   * in Elasticsearch.
+   * @param [originalAttributes] An optional dictionary of __ALL__ saved object original attributes
+   * that were used to create that saved object (i.e. values are NOT encrypted).
+   * @param [params] Parameters that control the way encrypted attributes are handled.
+   */
   public stripOrDecryptAttributesSync<T extends Record<string, unknown>>(
     descriptor: SavedObjectDescriptor,
-    attributes: T,
+    attributesToStripOrDecrypt: T,
     originalAttributes?: T,
     params?: DecryptParameters
+  ) {
+    const { attributes, attributesToDecrypt } = this.prepareAttributesForStripOrDecrypt(
+      descriptor,
+      attributesToStripOrDecrypt,
+      originalAttributes
+    );
+    try {
+      const decryptedAttributes = attributesToDecrypt
+        ? this.decryptAttributesSync(descriptor, attributesToDecrypt, params)
+        : {};
+      return { attributes: { ...attributes, ...decryptedAttributes } };
+    } catch (error) {
+      return { attributes, error };
+    }
+  }
+
+  /**
+   * Takes saved object attributes for the specified type and, depending on the type definition,
+   * either strips encrypted attributes, replaces with original decrypted value if available, or
+   * prepares them for decryption.
+   * @private
+   */
+  private prepareAttributesForStripOrDecrypt<T extends Record<string, unknown>>(
+    descriptor: SavedObjectDescriptor,
+    attributes: T,
+    originalAttributes?: T
   ) {
     const typeDefinition = this.typeDefinitions.get(descriptor.type);
     if (typeDefinition === undefined) {
-      return { attributes };
+      return { attributes, attributesToDecrypt: null };
     }
 
-    let decryptedAttributes: T | null = null;
-    let decryptionError: Error | undefined;
+    let attributesToDecrypt: T | undefined;
     const clonedAttributes: Record<string, unknown> = {};
     for (const [attributeName, attributeValue] of Object.entries(attributes)) {
-      // We should strip encrypted attribute if definition explicitly mandates that or decryption
-      // failed.
-      if (
-        typeDefinition.shouldBeStripped(attributeName) ||
-        (!!decryptionError && typeDefinition.shouldBeEncrypted(attributeName))
-      ) {
+      // We should strip encrypted attribute if definition explicitly mandates that.
+      if (typeDefinition.shouldBeStripped(attributeName)) {
         continue;
       }
 
@@ -254,30 +250,21 @@ export class EncryptedSavedObjectsService {
         // If attribute should be decrypted, but we have original attributes used to create object
         // we should get raw unencrypted value from there to avoid performance penalty.
         clonedAttributes[attributeName] = originalAttributes[attributeName];
-      } else {
-        // Otherwise just try to decrypt attribute. We decrypt all attributes at once, cache it and
-        // reuse for any other attributes.
-        if (decryptedAttributes === null) {
-          try {
-            decryptedAttributes = this.decryptAttributesSync(
-              descriptor,
-              // Decrypt only attributes that are supposed to be exposed.
-              Object.fromEntries(
-                Object.entries(attributes).filter(([key]) => !typeDefinition.shouldBeStripped(key))
-              ) as T,
-              params
-            );
-          } catch (err) {
-            decryptionError = err;
-            continue;
-          }
-        }
-
-        clonedAttributes[attributeName] = decryptedAttributes[attributeName];
+      } else if (!attributesToDecrypt) {
+        // Decrypt only attributes that are supposed to be exposed.
+        attributesToDecrypt = Object.fromEntries(
+          Object.entries(attributes).filter(([key]) => !typeDefinition.shouldBeStripped(key))
+        ) as T;
       }
     }
 
-    return { attributes: clonedAttributes as T, error: decryptionError };
+    return {
+      attributes: clonedAttributes as T,
+      attributesToDecrypt:
+        attributesToDecrypt && Object.keys(attributesToDecrypt).length > 0
+          ? attributesToDecrypt
+          : null,
+    };
   }
 
   private *attributesToEncryptIterator<T extends Record<string, unknown>>(