Skip to content

Commit

Permalink
Revert "Allow predefined ids for encrypted saved objects (#83482)"
Browse files Browse the repository at this point in the history 
This reverts commit 7d929fe.
  • Loading branch information
@thomheymann
thomheymann committed Nov 25, 2020
1 parent 337a902 commit 5f598d1
Show file tree
Hide file tree
Showing 7 changed files with 16 additions and 160 deletions.
15 changes: 0 additions & 15 deletions ...gins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,18 +113,3 @@ 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);
});
2 changes: 0 additions & 2 deletions ...k/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ export class EncryptedSavedObjectAttributesDefinition {
public readonly attributesToEncrypt: ReadonlySet<string>;
private readonly attributesToExcludeFromAAD: ReadonlySet<string> | undefined;
private readonly attributesToStrip: ReadonlySet<string>;
public readonly allowPredefinedID: boolean;

constructor(typeRegistration: EncryptedSavedObjectTypeRegistration) {
const attributesToEncrypt = new Set<string>();
Expand All @@ -35,7 +34,6 @@ export class EncryptedSavedObjectAttributesDefinition {
this.attributesToEncrypt = attributesToEncrypt;
this.attributesToStrip = attributesToStrip;
this.attributesToExcludeFromAAD = typeRegistration.attributesToExcludeFromAAD;
this.allowPredefinedID = !!typeRegistration.allowPredefinedID;
}

/**
Expand Down
7 changes: 0 additions & 7 deletions ...ck/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
function createEncryptedSavedObjectsServiceMock() {
return ({
isRegistered: jest.fn(),
canSpecifyID: jest.fn(),
stripOrDecryptAttributes: jest.fn(),
encryptAttributes: jest.fn(),
decryptAttributes: jest.fn(),
Expand Down Expand Up @@ -53,12 +52,6 @@ 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,
Expand Down
39 changes: 0 additions & 39 deletions x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,45 +89,6 @@ 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' };
Expand Down
20 changes: 0 additions & 20 deletions x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ export interface EncryptedSavedObjectTypeRegistration {
readonly type: string;
readonly attributesToEncrypt: ReadonlySet<string | AttributeToEncrypt>;
readonly attributesToExcludeFromAAD?: ReadonlySet<string>;
readonly allowPredefinedID?: boolean;
}

/**
Expand Down Expand Up @@ -145,25 +144,6 @@ 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
Expand Down
69 changes: 4 additions & 65 deletions ...crypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,6 @@ beforeEach(() => {
{ key: 'attrNotSoSecret', dangerouslyExposeValue: true },
]),
},
{
type: 'known-type-predefined-id',
attributesToEncrypt: new Set(['attrSecret']),
allowPredefinedID: true,
},
]);

wrapper = new EncryptedSavedObjectsClientWrapper({
Expand Down Expand Up @@ -79,36 +74,16 @@ describe('#create', () => {
expect(mockBaseClient.create).toHaveBeenCalledWith('unknown-type', attributes, options);
});

it('fails if type is registered without allowPredefinedID and ID is specified', async () => {
it('fails if type is registered 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 encrypted saved objects of type "known-type".'
'Predefined IDs are not allowed for saved objects with encrypted attributes.'
);

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',
Expand Down Expand Up @@ -326,7 +301,7 @@ describe('#bulkCreate', () => {
);
});

it('fails if ID is specified for registered type without allowPredefinedID', async () => {
it('fails if ID is specified for registered type', async () => {
const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' };

const bulkCreateParams = [
Expand All @@ -335,48 +310,12 @@ describe('#bulkCreate', () => {
];

await expect(wrapper.bulkCreate(bulkCreateParams)).rejects.toThrowError(
'Predefined IDs are not allowed for encrypted saved objects of type "known-type".'
'Predefined IDs are not allowed for saved objects with encrypted attributes.'
);

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',
Expand Down
24 changes: 12 additions & 12 deletions ...ns/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,16 @@ 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. 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)
) {
// 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) {
throw new Error(
`Predefined IDs are not allowed for encrypted saved objects of type "${type}".`
'Predefined IDs are not allowed for saved objects with encrypted attributes.'
);
}

Expand Down Expand Up @@ -108,12 +110,10 @@ 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.
if (
object.id &&
!this.options.service.canSpecifyID(object.type, object.version, options?.overwrite)
) {
const canSpecifyID = options?.overwrite && object.version;
if (object.id && !canSpecifyID) {
throw new Error(
`Predefined IDs are not allowed for encrypted saved objects of type "${object.type}".`
'Predefined IDs are not allowed for saved objects with encrypted attributes.'
);
}

Expand Down

0 comments on commit 5f598d1

Please sign in to comment.