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

Add support for the encryption key rotation to the encrypted saved objects. #72420

Merged
merged 9 commits into from
Oct 2, 2020
37 changes: 27 additions & 10 deletions x-pack/plugins/encrypted_saved_objects/server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,33 @@
import crypto from 'crypto';
import { map } from 'rxjs/operators';
import { schema, TypeOf } from '@kbn/config-schema';
import { UnwrapObservable } from '@kbn/utility-types';
import { PluginInitializerContext } from 'src/core/server';

export const ConfigSchema = schema.object({
enabled: schema.boolean({ defaultValue: true }),
encryptionKey: schema.conditional(
schema.contextRef('dist'),
true,
schema.maybe(schema.string({ minLength: 32 })),
schema.string({ minLength: 32, defaultValue: 'a'.repeat(32) })
),
});
export type ConfigType = UnwrapObservable<ReturnType<typeof createConfig$>>;

export const ConfigSchema = schema.object(
{
enabled: schema.boolean({ defaultValue: true }),
encryptionKey: schema.conditional(
schema.contextRef('dist'),
true,
schema.maybe(schema.string({ minLength: 32 })),
schema.string({ minLength: 32, defaultValue: 'a'.repeat(32) })
),
keyRotation: schema.object({
decryptionOnlyKeys: schema.arrayOf(schema.string({ minLength: 32 }), { defaultValue: [] }),
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to add this to kibana_docker, and allow on ESS once this merges.

}),
},
{
validate(value) {
const decryptionOnlyKeys = value.keyRotation?.decryptionOnlyKeys ?? [];
if (value.encryptionKey && decryptionOnlyKeys.includes(value.encryptionKey)) {
return '`keyRotation.decryptionOnlyKeys` cannot contain primary encryption key specified in `encryptionKey`.';
legrego marked this conversation as resolved.
Show resolved Hide resolved
}
},
}
);

export function createConfig$(context: PluginInitializerContext) {
return context.config.create<TypeOf<typeof ConfigSchema>>().pipe(
Expand All @@ -37,7 +53,8 @@ export function createConfig$(context: PluginInitializerContext) {
}

return {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: the config object we return here is supposed to be a convenient wrapper around config related stuff used by plugin internally, so it's not required to reflect raw config shape exactly, so I reshaped it slightly.

config: { ...config, encryptionKey },
...config,
encryptionKey,
usingEphemeralEncryptionKey,
};
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,16 @@ interface CommonParameters {
user?: AuthenticatedUser;
}

/**
* Describes parameters for the decrypt methods.
*/
interface DecryptParameters extends CommonParameters {
/**
* Indicates whether decryption should only be performed using secondary decryption-only keys.
*/
useDecryptionOnlyKeys?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think reversing this would make it easier to understand:

Suggested change
useDecryptionOnlyKeys?: boolean;
includePrimaryEncryptionKey?: boolean;

or if you didn't want to reverse it, something like:

Suggested change
useDecryptionOnlyKeys?: boolean;
omitPrimaryEncryptionKey?: boolean;

or (my least favorite suggestion)

Suggested change
useDecryptionOnlyKeys?: boolean;
onlyUseDecryptionOnlyKeys?: boolean;

Copy link
Member Author

Choose a reason for hiding this comment

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

omitPrimaryEncryptionKey?: boolean;

Sounds good 👍

}

/**
* Utility function that gives array representation of the saved object descriptor respecting
* optional `namespace` property.
Expand Down Expand Up @@ -80,15 +90,21 @@ export class EncryptedSavedObjectsService {
> = new Map();

/**
* @param crypto nodeCrypto instance.
* @param cryptos nodeCrypto instances used for encryption and/or decryption. The first crypto
* in the list is considered as primary and it's the only one that is used for both encryption and
* decryption, the rest are decryption-only cryptos.
* @param logger Ordinary logger instance.
* @param audit Audit logger instance.
*/
constructor(
private readonly crypto: Readonly<Crypto>,
private readonly cryptos: Readonly<Crypto[]>,
private readonly logger: Logger,
private readonly audit: EncryptedSavedObjectsAuditLogger
) {}
) {
if (cryptos.length === 0 || !cryptos[0]) {
throw new Error('The list of cryptos should include at least one item.');
}
}

/**
* Registers saved object type as the one that contains attributes that should be encrypted.
Expand Down Expand Up @@ -136,7 +152,7 @@ export class EncryptedSavedObjectsService {
descriptor: SavedObjectDescriptor,
attributes: T,
originalAttributes?: T,
params?: CommonParameters
params?: DecryptParameters
) {
const typeDefinition = this.typeDefinitions.get(descriptor.type);
if (typeDefinition === undefined) {
Expand Down Expand Up @@ -174,7 +190,7 @@ export class EncryptedSavedObjectsService {
Object.fromEntries(
Object.entries(attributes).filter(([key]) => !typeDefinition.shouldBeStripped(key))
) as T,
{ user: params?.user }
params
);
} catch (err) {
decryptionError = err;
Expand Down Expand Up @@ -264,13 +280,14 @@ export class EncryptedSavedObjectsService {
attributes: T,
params?: CommonParameters
): Promise<T> {
const encrypter = this.getEncrypter();
const iterator = this.attributesToEncryptIterator<T>(descriptor, attributes, params);

let iteratorResult = iterator.next();
while (!iteratorResult.done) {
const [attributeValue, encryptionAAD] = iteratorResult.value;
try {
iteratorResult = iterator.next(await this.crypto.encrypt(attributeValue, encryptionAAD));
iteratorResult = iterator.next(await encrypter.encrypt(attributeValue, encryptionAAD));
} catch (err) {
iterator.throw!(err);
}
Expand All @@ -293,13 +310,14 @@ export class EncryptedSavedObjectsService {
attributes: T,
params?: CommonParameters
): T {
const encrypter = this.getEncrypter();
const iterator = this.attributesToEncryptIterator<T>(descriptor, attributes, params);

let iteratorResult = iterator.next();
while (!iteratorResult.done) {
const [attributeValue, encryptionAAD] = iteratorResult.value;
try {
iteratorResult = iterator.next(this.crypto.encryptSync(attributeValue, encryptionAAD));
iteratorResult = iterator.next(encrypter.encryptSync(attributeValue, encryptionAAD));
} catch (err) {
iterator.throw!(err);
}
Expand All @@ -321,19 +339,31 @@ export class EncryptedSavedObjectsService {
public async decryptAttributes<T extends Record<string, unknown>>(
descriptor: SavedObjectDescriptor,
attributes: T,
params?: CommonParameters
params?: DecryptParameters
): Promise<T> {
const decrypters = this.getDecrypters(params?.useDecryptionOnlyKeys);
const iterator = this.attributesToDecryptIterator<T>(descriptor, attributes, params);

let iteratorResult = iterator.next();
while (!iteratorResult.done) {
const [attributeValue, encryptionAAD] = iteratorResult.value;
try {
iteratorResult = iterator.next(
(await this.crypto.decrypt(attributeValue, encryptionAAD)) as string
);
} catch (err) {
iterator.throw!(err);

let decryptionError;
for (const decrypter of decrypters) {
try {
iteratorResult = iterator.next(await decrypter.decrypt(attributeValue, encryptionAAD));
decryptionError = undefined;
break;
} catch (err) {
// Remember the error thrown when we tried to decrypt with the primary key.
if (!decryptionError) {
decryptionError = err;
legrego marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

if (decryptionError) {
iterator.throw!(decryptionError);
}
}

Expand All @@ -353,17 +383,31 @@ export class EncryptedSavedObjectsService {
public decryptAttributesSync<T extends Record<string, unknown>>(
descriptor: SavedObjectDescriptor,
attributes: T,
params?: CommonParameters
params?: DecryptParameters
): T {
const decrypters = this.getDecrypters(params?.useDecryptionOnlyKeys);
const iterator = this.attributesToDecryptIterator<T>(descriptor, attributes, params);

let iteratorResult = iterator.next();
while (!iteratorResult.done) {
const [attributeValue, encryptionAAD] = iteratorResult.value;
try {
iteratorResult = iterator.next(this.crypto.decryptSync(attributeValue, encryptionAAD));
} catch (err) {
iterator.throw!(err);

let decryptionError;
for (const decrypter of decrypters) {
try {
iteratorResult = iterator.next(decrypter.decryptSync(attributeValue, encryptionAAD));
decryptionError = undefined;
break;
} catch (err) {
// Remember the error thrown when we tried to decrypt with the primary key.
if (!decryptionError) {
decryptionError = err;
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above: Should we check to make sure that this is an EncryptionError, and not some other unexpected failure condition?

}
}
}

if (decryptionError) {
iterator.throw!(decryptionError);
}
}

Expand Down Expand Up @@ -468,4 +512,30 @@ export class EncryptedSavedObjectsService {

return stringify([...descriptorToArray(descriptor), attributesAAD]);
}

/**
* Returns NodeCrypto instance used for encryption (the primary crypto).
*/
private getEncrypter() {
return this.cryptos[0];
}

/**
* Returns list of NodeCrypto instances used for decryption.
* @param useDecryptionOnlyKeys Specifies whether returned decrypters should include only those
* that are using decryption only keys (the secondary cryptos).
*/
private getDecrypters(useDecryptionOnlyKeys?: boolean) {
if (!useDecryptionOnlyKeys) {
return this.cryptos;
}

if (this.cryptos.length === 1) {
throw new Error(
`"useDecryptionOnlyKeys" cannot be set when decryption only keys aren't configured.`
);
}

return this.cryptos.slice(1);
}
}
Loading