Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow predefined ids for encrypted saved objects #83482

Merged
merged 8 commits into from
Nov 23, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,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).toBeFalsy();
thomheymann marked this conversation as resolved.
Show resolved Hide resolved

const typeDefinitionWithPredefinedIDAllowed = new EncryptedSavedObjectAttributesDefinition({
type: 'so-type',
attributesToEncrypt: new Set(['attr#1', 'attr#2']),
allowPredefinedID: true,
});
expect(typeDefinitionWithPredefinedIDAllowed.allowPredefinedID).toBe(true);
});
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export class EncryptedSavedObjectAttributesDefinition {
public readonly attributesToEncrypt: ReadonlySet<string>;
private readonly attributesToExcludeFromAAD: ReadonlySet<string> | undefined;
private readonly attributesToStrip: ReadonlySet<string>;
public readonly allowPredefinedID?: boolean;
thomheymann marked this conversation as resolved.
Show resolved Hide resolved

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

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
function createEncryptedSavedObjectsServiceMock() {
return ({
isRegistered: jest.fn(),
allowPredefinedID: jest.fn(),
stripOrDecryptAttributes: jest.fn(),
encryptAttributes: jest.fn(),
decryptAttributes: jest.fn(),
Expand Down Expand Up @@ -52,6 +53,13 @@ export const encryptedSavedObjectsServiceMock = {
mock.isRegistered.mockImplementation(
(type) => registrations.findIndex((r) => r.type === type) >= 0
);
mock.allowPredefinedID.mockImplementation((type) => {
const registration = registrations.find((r) => r.type === type);
if (!registration) {
return true;
}
return registration.allowPredefinedID === true;
});
mock.encryptAttributes.mockImplementation(async (descriptor, attrs) =>
processAttributes(
descriptor,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,35 @@ describe('#isRegistered', () => {
});
});

describe('#allowPredefinedID', () => {
it('returns true for unknown types', () => {
expect(service.allowPredefinedID('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.allowPredefinedID('known-type-1')).toBe(true);
});

it('returns false for types registered without setting allowPredefinedID', () => {
service.registerType({ type: 'known-type-1', attributesToEncrypt: new Set(['attr-1']) });
expect(service.allowPredefinedID('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.allowPredefinedID('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
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface EncryptedSavedObjectTypeRegistration {
readonly type: string;
readonly attributesToEncrypt: ReadonlySet<string | AttributeToEncrypt>;
readonly attributesToExcludeFromAAD?: ReadonlySet<string>;
readonly allowPredefinedID?: boolean;
}

/**
Expand Down Expand Up @@ -144,6 +145,19 @@ export class EncryptedSavedObjectsService {
return this.typeDefinitions.has(type);
}

/**
* Checks whether specified saved object type supports predefined IDs. If the type isn't registered
* as an encrypted saved object type, this will return "true".
* @param type Saved object type.
*/
public allowPredefinedID(type: string) {
thomheymann marked this conversation as resolved.
Show resolved Hide resolved
const typeDefinitions = this.typeDefinitions.get(type);
thomheymann marked this conversation as resolved.
Show resolved Hide resolved
if (typeDefinitions === undefined) {
return true;
}
return typeDefinitions.allowPredefinedID === true;
thomheymann marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* 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
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ beforeEach(() => {
{ key: 'attrNotSoSecret', dangerouslyExposeValue: true },
]),
},
{
type: 'known-type-predefined-id',
attributesToEncrypt: new Set(['attrSecret']),
allowPredefinedID: true,
},
]);

wrapper = new EncryptedSavedObjectsClientWrapper({
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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 = [
Expand All @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,16 +68,17 @@ 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.
// 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.

// 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;
const canSpecifyID =
(options.overwrite && options.version) || this.options.service.allowPredefinedID(type);
if (options.id && !canSpecifyID) {
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}".`
);
}

Expand Down Expand Up @@ -118,10 +119,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;
const canSpecifyID =
(options?.overwrite && object.version) ||
this.options.service.allowPredefinedID(object.type);
if (object.id && !canSpecifyID) {
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}".`
);
}

Expand Down