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

[ESO] Add flag to allow ESO consumers to opt-out of highly random UIDs #198287

Merged
merged 15 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 9 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 @@ -105,10 +105,14 @@ export class CommonHelper {
if (!id) {
return SavedObjectsUtils.generateId();
}
// 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 = (overwrite && version) || SavedObjectsUtils.isRandomId(id);

const shouldEnforceRandomId = this.encryptionExtension?.shouldEnforceRandomId(type);

// Allow specified ID if:
// 1. we're overwriting an existing ESO with a Version (this helps us ensure that the document really was previously created using ESO)
// 2. enforceRandomId is explicitly set to false
const canSpecifyID =
!shouldEnforceRandomId || (overwrite && version) || SavedObjectsUtils.isRandomId(id);
if (!canSpecifyID) {
throw SavedObjectsErrorHelpers.createBadRequestError(
'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.'
Expand Down
jeramysoucy marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ describe('SavedObjectsRepository Encryption Extension', () => {

it(`fails if non-UUID ID is specified for encrypted type`, async () => {
mockEncryptionExt.isEncryptableType.mockReturnValue(true);
mockEncryptionExt.shouldEnforceRandomId.mockReturnValue(true);
mockEncryptionExt.decryptOrStripResponseAttributes.mockResolvedValue({
...encryptedSO,
...decryptedStrippedAttributes,
Expand Down Expand Up @@ -483,6 +484,7 @@ describe('SavedObjectsRepository Encryption Extension', () => {

it(`fails if non-UUID ID is specified for encrypted type`, async () => {
mockEncryptionExt.isEncryptableType.mockReturnValue(true);
mockEncryptionExt.shouldEnforceRandomId.mockReturnValue(true);
const result = await bulkCreateSuccess(client, repository, [
encryptedSO, // Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const createEncryptionExtension = (): jest.Mocked<ISavedObjectsEncryptionExtensi
isEncryptableType: jest.fn(),
decryptOrStripResponseAttributes: jest.fn(),
encryptAttributes: jest.fn(),
shouldEnforceRandomId: jest.fn(),
});

const createSecurityExtension = (): jest.Mocked<ISavedObjectsSecurityExtension> => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const createEncryptionExtension = (): jest.Mocked<ISavedObjectsEncryptionExtensi
isEncryptableType: jest.fn(),
decryptOrStripResponseAttributes: jest.fn(),
encryptAttributes: jest.fn(),
shouldEnforceRandomId: jest.fn(),
});

const createSecurityExtension = (): jest.Mocked<ISavedObjectsSecurityExtension> => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ export interface ISavedObjectsEncryptionExtension {
*/
isEncryptableType: (type: string) => boolean;

/**
* Returns false if ESO explicilty opts out of highly random UID
SiddharthMantri marked this conversation as resolved.
Show resolved Hide resolved
*
* @param type the string name of the object type
* @returns boolean, true by default unless explicitly set to false
*/
shouldEnforceRandomId: (type: string) => boolean;

/**
* Given a saved object, will return a decrypted saved object or will strip
* attributes from the returned object if decryption fails.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export class EncryptedSavedObjectAttributesDefinition {
public readonly attributesToEncrypt: ReadonlySet<string>;
private readonly attributesToIncludeInAAD: ReadonlySet<string> | undefined;
private readonly attributesToStrip: ReadonlySet<string>;
public readonly enforceRandomId: boolean;

constructor(typeRegistration: EncryptedSavedObjectTypeRegistration) {
if (typeRegistration.attributesToIncludeInAAD) {
Expand Down Expand Up @@ -49,6 +50,8 @@ export class EncryptedSavedObjectAttributesDefinition {
}
}

this.enforceRandomId = typeRegistration.enforceRandomId !== false;

this.attributesToEncrypt = attributesToEncrypt;
this.attributesToStrip = attributesToStrip;
this.attributesToIncludeInAAD = typeRegistration.attributesToIncludeInAAD;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export interface EncryptedSavedObjectTypeRegistration {
readonly type: string;
readonly attributesToEncrypt: ReadonlySet<string | AttributeToEncrypt>;
readonly attributesToIncludeInAAD?: ReadonlySet<string>;
readonly enforceRandomId?: boolean;
}

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

/**
* Check whether the ESO has explicitly opted out of enforcing random IDs for the specified type.
SiddharthMantri marked this conversation as resolved.
Show resolved Hide resolved
* @param type Saved object type.
* @returns boolean - true unless explicitly opted out by setting enforceRandomId to false
*/
public shouldEnforceRandomId(type: string) {
jeramysoucy marked this conversation as resolved.
Show resolved Hide resolved
const typeDefinition = this.typeDefinitions.get(type);
return typeDefinition?.enforceRandomId !== false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Optional and not that important, but I’m wondering if we should throw an error here if type isn’t registered (isEncryptableType(type) !== true)? It’s unlikely to happen in practice since we expect consumers of this API to call isEncryptableType first, but it still feels a bit odd that this function would return true for non-ESOs if someone mistakenly uses it in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@azasypkin That's a good point! I think i'll address it in a follow-up PR.

I was also thinking about the scenario where a type explicitly opts out but no ID is provided during creation. In it's current implementation, a random ID will be created but I was wondering if it should behave differently?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@azasypkin That's a good point! I think i'll address it in a follow-up PR.

Thanks!

I was also thinking about the scenario where a type explicitly opts out but no ID is provided during creation. In it's current implementation, a random ID will be created but I was wondering if it should behave differently?

Hmm, I think the current behavior is correct and works the same for both ESO and non-ESO saved objects. I interpret enforceRandomId as "prevent consumers from explicitly specifying non-random IDs, but keep the default behavior if they don’t specify IDs".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ Yeah, if an type has opted out of enforcing random IDs but an ID is not provided during creation, a random ID should be generated.

}

/**
* 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 @@ -40,6 +40,10 @@ export class SavedObjectsEncryptionExtension implements ISavedObjectsEncryptionE
return this._service.isRegistered(type);
}

shouldEnforceRandomId(type: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should augment the unit tests in saved_objects_encryption_extension.test.ts to cover this function as well.

Trivial nit:

Suggested change
shouldEnforceRandomId(type: string) {
typeEnforcesRandomId(type: string) {

return this._service.shouldEnforceRandomId(type);
}

async decryptOrStripResponseAttributes<T, R extends SavedObject<T>>(
response: R,
originalAttributes?: T
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ const SAVED_OBJECT_WITH_MIGRATION_TYPE = 'saved-object-with-migration';

const SAVED_OBJECT_MV_TYPE = 'saved-object-mv';

const TYPE_WITH_PREDICTABLE_ID = 'type-with-predictable-ids';

interface MigratedTypePre790 {
nonEncryptedAttribute: string;
encryptedAttribute: string;
Expand Down Expand Up @@ -83,6 +85,30 @@ export const plugin: PluginInitializer<void, void, PluginsSetup, PluginsStart> =
});
}

core.savedObjects.registerType({
name: TYPE_WITH_PREDICTABLE_ID,
hidden: false,
namespaceType: 'single',
mappings: deepFreeze({
properties: {
publicProperty: { type: 'keyword' },
publicPropertyExcludedFromAAD: { type: 'keyword' },
publicPropertyStoredEncrypted: { type: 'binary' },
privateProperty: { type: 'binary' },
},
}),
});

deps.encryptedSavedObjects.registerType({
type: TYPE_WITH_PREDICTABLE_ID,
attributesToEncrypt: new Set([
'privateProperty',
{ key: 'publicPropertyStoredEncrypted', dangerouslyExposeValue: true },
]),
attributesToIncludeInAAD: new Set(['publicProperty']),
enforceRandomId: false,
});

core.savedObjects.registerType({
name: SAVED_OBJECT_WITHOUT_SECRET_TYPE,
hidden: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ export default function ({ getService }: FtrProviderContext) {
'saved-object-with-secret-and-multiple-spaces';
const SAVED_OBJECT_WITHOUT_SECRET_TYPE = 'saved-object-without-secret';

const TYPE_WITH_PREDICTABLE_ID = 'type-with-predictable-ids';

function runTests(
encryptedSavedObjectType: string,
getURLAPIBaseURL: () => string,
Expand Down Expand Up @@ -900,5 +902,79 @@ export default function ({ getService }: FtrProviderContext) {
}
});
});

describe('enforceRandomId', () => {
SiddharthMantri marked this conversation as resolved.
Show resolved Hide resolved
it('#create ESO which has opted out of random IDs allows setting ID', async () => {
const id = 'my_predictable_id';

const savedObjectOriginalAttributes = {
publicProperty: randomness.string(),
publicPropertyStoredEncrypted: randomness.string(),
privateProperty: randomness.string(),
publicPropertyExcludedFromAAD: randomness.string(),
};

const { body: response } = await supertest
.post(`/api/saved_objects/${TYPE_WITH_PREDICTABLE_ID}/${id}`)
.set('kbn-xsrf', 'xxx')
.send({ attributes: savedObjectOriginalAttributes })
.expect(200);

expect(response.id).to.be(id);
});

it('#create setting a predictable id on ESOs that have not opted out throws an error', async () => {
const id = 'my_predictable_id';

const savedObjectOriginalAttributes = {
publicProperty: randomness.string(),
publicPropertyStoredEncrypted: randomness.string(),
privateProperty: randomness.string(),
publicPropertyExcludedFromAAD: randomness.string(),
};

const { body: response } = await supertest
.post(`/api/saved_objects/saved-object-with-secret/${id}`)
.set('kbn-xsrf', 'xxx')
.send({ attributes: savedObjectOriginalAttributes })
.expect(400);

expect(response.message).to.contain(
'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.'
);
});

it('#bulkCreate not enforcing random ID allows to specify ID', async () => {
SiddharthMantri marked this conversation as resolved.
Show resolved Hide resolved
const bulkCreateParams = [
{
type: TYPE_WITH_PREDICTABLE_ID,
id: 'my_predictable_id',
attributes: {
publicProperty: randomness.string(),
publicPropertyExcludedFromAAD: randomness.string(),
publicPropertyStoredEncrypted: randomness.string(),
privateProperty: randomness.string(),
},
},
{
type: SAVED_OBJECT_WITHOUT_SECRET_TYPE,
attributes: {
publicProperty: randomness.string(),
},
},
];

const {
body: { saved_objects: savedObjects },
} = await supertest
.post('/api/saved_objects/_bulk_create')
.set('kbn-xsrf', 'xxx')
.send(bulkCreateParams)
.expect(200);

expect(savedObjects).to.have.length(bulkCreateParams.length);
expect(savedObjects[0].id).to.be('my_predictable_id');
});
});
});
}