From 7d929fe9030b67a6dd8017e8d61e8521f3fd74f8 Mon Sep 17 00:00:00 2001 From: Thom Heymann <190132+thomheymann@users.noreply.github.com> Date: Mon, 23 Nov 2020 14:53:35 +0000 Subject: [PATCH] Allow predefined ids for encrypted saved objects (#83482) * Allow predefined ids for encrypted saved objects * Fix mock * fix tests * Added suggestions from code review * added jsdocs params * Fixed jsdocs --- ...ypted_saved_object_type_definition.test.ts | 15 ++++ .../encrypted_saved_object_type_definition.ts | 2 + .../encrypted_saved_objects_service.mocks.ts | 7 ++ .../encrypted_saved_objects_service.test.ts | 39 +++++++++++ .../crypto/encrypted_saved_objects_service.ts | 20 ++++++ ...ypted_saved_objects_client_wrapper.test.ts | 69 +++++++++++++++++-- .../encrypted_saved_objects_client_wrapper.ts | 24 +++---- 7 files changed, 160 insertions(+), 16 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts index f528843cf9ea..f1e06a0cec03 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts @@ -113,3 +113,18 @@ it('correctly determines attribute properties', () => { } } }); + +it('it correctly sets allowPredefinedID', () => { + const defaultTypeDefinition = new EncryptedSavedObjectAttributesDefinition({ + type: 'so-type', + attributesToEncrypt: new Set(['attr#1', 'attr#2']), + }); + expect(defaultTypeDefinition.allowPredefinedID).toBe(false); + + const typeDefinitionWithPredefinedIDAllowed = new EncryptedSavedObjectAttributesDefinition({ + type: 'so-type', + attributesToEncrypt: new Set(['attr#1', 'attr#2']), + allowPredefinedID: true, + }); + expect(typeDefinitionWithPredefinedIDAllowed.allowPredefinedID).toBe(true); +}); diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts index 849a2888b6e1..398a64585411 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts @@ -15,6 +15,7 @@ export class EncryptedSavedObjectAttributesDefinition { public readonly attributesToEncrypt: ReadonlySet; private readonly attributesToExcludeFromAAD: ReadonlySet | undefined; private readonly attributesToStrip: ReadonlySet; + public readonly allowPredefinedID: boolean; constructor(typeRegistration: EncryptedSavedObjectTypeRegistration) { const attributesToEncrypt = new Set(); @@ -34,6 +35,7 @@ export class EncryptedSavedObjectAttributesDefinition { this.attributesToEncrypt = attributesToEncrypt; this.attributesToStrip = attributesToStrip; this.attributesToExcludeFromAAD = typeRegistration.attributesToExcludeFromAAD; + this.allowPredefinedID = !!typeRegistration.allowPredefinedID; } /** diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts index c692d8698771..0138e929ca1c 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts @@ -13,6 +13,7 @@ import { function createEncryptedSavedObjectsServiceMock() { return ({ isRegistered: jest.fn(), + canSpecifyID: jest.fn(), stripOrDecryptAttributes: jest.fn(), encryptAttributes: jest.fn(), decryptAttributes: jest.fn(), @@ -52,6 +53,12 @@ export const encryptedSavedObjectsServiceMock = { mock.isRegistered.mockImplementation( (type) => registrations.findIndex((r) => r.type === type) >= 0 ); + mock.canSpecifyID.mockImplementation((type, version, overwrite) => { + const registration = registrations.find((r) => r.type === type); + return ( + registration === undefined || registration.allowPredefinedID || !!(version && overwrite) + ); + }); mock.encryptAttributes.mockImplementation(async (descriptor, attrs) => processAttributes( descriptor, diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts index 88d57072697f..6bc4a392064e 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts @@ -89,6 +89,45 @@ describe('#isRegistered', () => { }); }); +describe('#canSpecifyID', () => { + it('returns true for unknown types', () => { + expect(service.canSpecifyID('unknown-type')).toBe(true); + }); + + it('returns true for types registered setting allowPredefinedID to true', () => { + service.registerType({ + type: 'known-type-1', + attributesToEncrypt: new Set(['attr-1']), + allowPredefinedID: true, + }); + expect(service.canSpecifyID('known-type-1')).toBe(true); + }); + + it('returns true when overwriting a saved object with a version specified even when allowPredefinedID is not set', () => { + service.registerType({ + type: 'known-type-1', + attributesToEncrypt: new Set(['attr-1']), + }); + expect(service.canSpecifyID('known-type-1', '2', true)).toBe(true); + expect(service.canSpecifyID('known-type-1', '2', false)).toBe(false); + expect(service.canSpecifyID('known-type-1', undefined, true)).toBe(false); + }); + + it('returns false for types registered without setting allowPredefinedID', () => { + service.registerType({ type: 'known-type-1', attributesToEncrypt: new Set(['attr-1']) }); + expect(service.canSpecifyID('known-type-1')).toBe(false); + }); + + it('returns false for types registered setting allowPredefinedID to false', () => { + service.registerType({ + type: 'known-type-1', + attributesToEncrypt: new Set(['attr-1']), + allowPredefinedID: false, + }); + expect(service.canSpecifyID('known-type-1')).toBe(false); + }); +}); + describe('#stripOrDecryptAttributes', () => { it('does not strip attributes from unknown types', async () => { const attributes = { attrOne: 'one', attrTwo: 'two', attrThree: 'three' }; 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 index 1f1093a17953..8d2ebb575c35 100644 --- 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 @@ -31,6 +31,7 @@ export interface EncryptedSavedObjectTypeRegistration { readonly type: string; readonly attributesToEncrypt: ReadonlySet; readonly attributesToExcludeFromAAD?: ReadonlySet; + readonly allowPredefinedID?: boolean; } /** @@ -144,6 +145,25 @@ export class EncryptedSavedObjectsService { return this.typeDefinitions.has(type); } + /** + * Checks whether ID can be specified for the provided saved object. + * + * If the type isn't registered as an encrypted saved object, or when overwriting an existing + * saved object with a version specified, this will return "true". + * + * @param type Saved object type. + * @param version Saved object version number which changes on each successful write operation. + * Can be used in conjunction with `overwrite` for implementing optimistic concurrency + * control. + * @param overwrite Overwrite existing documents. + */ + public canSpecifyID(type: string, version?: string, overwrite?: boolean) { + const typeDefinition = this.typeDefinitions.get(type); + return ( + typeDefinition === undefined || typeDefinition.allowPredefinedID || !!(version && overwrite) + ); + } + /** * 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 diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 6346e73e6ba5..3c722ccfabae 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -30,6 +30,11 @@ beforeEach(() => { { key: 'attrNotSoSecret', dangerouslyExposeValue: true }, ]), }, + { + type: 'known-type-predefined-id', + attributesToEncrypt: new Set(['attrSecret']), + allowPredefinedID: true, + }, ]); wrapper = new EncryptedSavedObjectsClientWrapper({ @@ -72,16 +77,36 @@ describe('#create', () => { expect(mockBaseClient.create).toHaveBeenCalledWith('unknown-type', attributes, options); }); - it('fails if type is registered and ID is specified', async () => { + it('fails if type is registered without allowPredefinedID and ID is specified', async () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; await expect(wrapper.create('known-type', attributes, { id: 'some-id' })).rejects.toThrowError( - 'Predefined IDs are not allowed for saved objects with encrypted attributes.' + 'Predefined IDs are not allowed for encrypted saved objects of type "known-type".' ); expect(mockBaseClient.create).not.toHaveBeenCalled(); }); + it('succeeds if type is registered with allowPredefinedID and ID is specified', async () => { + const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; + const mockedResponse = { + id: 'some-id', + type: 'known-type-predefined-id', + attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, + references: [], + }; + + mockBaseClient.create.mockResolvedValue(mockedResponse); + await expect( + wrapper.create('known-type-predefined-id', attributes, { id: 'some-id' }) + ).resolves.toEqual({ + ...mockedResponse, + attributes: { attrOne: 'one', attrThree: 'three' }, + }); + + expect(mockBaseClient.create).toHaveBeenCalled(); + }); + it('allows a specified ID when overwriting an existing object', async () => { const attributes = { attrOne: 'one', @@ -299,7 +324,7 @@ describe('#bulkCreate', () => { ); }); - it('fails if ID is specified for registered type', async () => { + it('fails if ID is specified for registered type without allowPredefinedID', async () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const bulkCreateParams = [ @@ -308,12 +333,48 @@ describe('#bulkCreate', () => { ]; await expect(wrapper.bulkCreate(bulkCreateParams)).rejects.toThrowError( - 'Predefined IDs are not allowed for saved objects with encrypted attributes.' + 'Predefined IDs are not allowed for encrypted saved objects of type "known-type".' ); expect(mockBaseClient.bulkCreate).not.toHaveBeenCalled(); }); + it('succeeds if ID is specified for registered type with allowPredefinedID', async () => { + const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; + const options = { namespace: 'some-namespace' }; + const mockedResponse = { + saved_objects: [ + { + id: 'some-id', + type: 'known-type-predefined-id', + attributes, + references: [], + }, + { + id: 'some-id', + type: 'unknown-type', + attributes, + references: [], + }, + ], + }; + mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); + + const bulkCreateParams = [ + { id: 'some-id', type: 'known-type-predefined-id', attributes }, + { type: 'unknown-type', attributes }, + ]; + + await expect(wrapper.bulkCreate(bulkCreateParams, options)).resolves.toEqual({ + saved_objects: [ + { ...mockedResponse.saved_objects[0], attributes: { attrOne: 'one', attrThree: 'three' } }, + mockedResponse.saved_objects[1], + ], + }); + + expect(mockBaseClient.bulkCreate).toHaveBeenCalled(); + }); + it('allows a specified ID when overwriting an existing object', async () => { const attributes = { attrOne: 'one', diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 59309ab67e77..ddef9f477433 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -68,16 +68,14 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon } // Saved objects with encrypted attributes should have IDs that are hard to guess especially - // since IDs are part of the AAD used during encryption, that's why we control them within this - // wrapper and don't allow consumers to specify their own IDs directly. - - // only allow a specified ID if we're overwriting an existing ESO with a Version - // this helps us ensure that the document really was previously created using ESO - // and not being used to get around the specified ID limitation - const canSpecifyID = options.overwrite && options.version; - if (options.id && !canSpecifyID) { + // since IDs are part of the AAD used during encryption. Types can opt-out of this restriction, + // when necessary, but it's much safer for this wrapper to generate them. + if ( + options.id && + !this.options.service.canSpecifyID(type, options.version, options.overwrite) + ) { throw new Error( - 'Predefined IDs are not allowed for saved objects with encrypted attributes.' + `Predefined IDs are not allowed for encrypted saved objects of type "${type}".` ); } @@ -118,10 +116,12 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document. - const canSpecifyID = options?.overwrite && object.version; - if (object.id && !canSpecifyID) { + if ( + object.id && + !this.options.service.canSpecifyID(object.type, object.version, options?.overwrite) + ) { throw new Error( - 'Predefined IDs are not allowed for saved objects with encrypted attributes.' + `Predefined IDs are not allowed for encrypted saved objects of type "${object.type}".` ); }