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.

Sign up for GitHub

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

[Telemetry] update crypto packages #62469

Merged
merged 10 commits into from
Apr 6, 2020
Merged

[Telemetry] update crypto packages #62469

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fba71d0
update crypto packages
Bamieh Apr 3, 2020
4e9c932
Merge branch 'master' of github.com:elastic/kibana into telemetry/upd…
Bamieh Apr 3, 2020
5e44e78
as type for return value
Bamieh Apr 3, 2020
b80e13b
Merge branch 'master' of github.com:elastic/kibana into telemetry/upd…
Bamieh Apr 3, 2020
19ab24a
Merge branch 'master' of github.com:elastic/kibana into telemetry/upd…
Bamieh Apr 3, 2020
2cd9562
get default export
Bamieh Apr 3, 2020
4d41db1
Merge branch 'master' into telemetry/update_crypto_packages
Bamieh Apr 6, 2020
bed4eb4
Merge branch 'master' into telemetry/update_crypto_packages
elasticmachine Apr 6, 2020
74c17a3
add if checks
Bamieh Apr 6, 2020
5ef32c8
wrap errors in i18n
Bamieh Apr 6, 2020
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
"@elastic/filesaver": "1.1.2",
"@elastic/good": "8.1.1-kibana2",
"@elastic/numeral": "2.4.0",
"@elastic/request-crypto": "^1.0.2",
"@elastic/request-crypto": "1.1.2",
"@elastic/ui-ace": "0.2.3",
"@hapi/good-squeeze": "5.2.1",
"@hapi/wreck": "^15.0.2",
Expand Down
20 changes: 0 additions & 20 deletions typings/elastic__node_crypto.d.ts
View file
Open in desktop

This file was deleted.

9 changes: 6 additions & 3 deletions x-pack/legacy/plugins/reporting/export_types/common/execute_job/decrypt_job_headers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { i18n } from '@kbn/i18n';
import { cryptoFactory } from '../../../server/lib/crypto';
import { CryptoFactory, Logger } from '../../../types';
import { Logger } from '../../../types';

interface HasEncryptedHeaders {
headers?: string;
Expand All @@ -25,9 +25,12 @@ export const decryptJobHeaders = async <
job: JobDocPayloadType;
logger: Logger;
}): Promise<Record<string, string>> => {
const crypto: CryptoFactory = cryptoFactory(encryptionKey);
try {
const decryptedHeaders: Record<string, string> = await crypto.decrypt(job.headers);
if (typeof job.headers !== 'string') {
throw new Error('Job headers are missing');
Copy link
Member Author

Choose a reason for hiding this comment

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

nodeCrypto will throw an error when calling Buffer.from if it was not passed a string or a buffer https://github.com/elastic/node-crypto/blob/master/src/crypto.ts#L133

This will throw a more readable error and safe-guard the type

Copy link
Member

@tsullivan tsullivan Apr 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe this error message should be wrapped in i18n.translate ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I havent wrapped them in i18n.translate because this is the actual error payload and not the error message we are showing to the user.

The error message to the user is already i18n wrapped:
'Failed to decrypt report job data. Please ensure that {encryptionKey} is set and re-generate this report. {err}'.

I've wrapped them with i18n.translation; just explaining my thought process about that one.

}
const crypto = cryptoFactory(encryptionKey);
const decryptedHeaders = (await crypto.decrypt(job.headers)) as Record<string, string>;
return decryptedHeaders;
} catch (err) {
logger.error(err);
Expand Down
7 changes: 4 additions & 3 deletions x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
} = job;

const decryptHeaders = async () => {
let decryptedHeaders;
try {
decryptedHeaders = await crypto.decrypt(headers);
if (typeof headers !== 'string') {
throw new Error('Job headers are missing');
Copy link
Member

Choose a reason for hiding this comment

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

wrap the error message in i18n ?

}
return await crypto.decrypt(headers);
} catch (err) {
logger.error(err);
throw new Error(
Expand All @@ -58,7 +60,6 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
)
); // prettier-ignore
}
return decryptedHeaders;
};

const fakeRequest = KibanaRequest.from({
Expand Down
8 changes: 7 additions & 1 deletion x-pack/legacy/plugins/reporting/export_types/csv_from_savedobject/server/execute_job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,13 @@ export const executeJobFactory: ExecuteJobFactory<ImmediateExecuteFn<
let decryptedHeaders: Record<string, unknown>;
const serializedEncryptedHeaders = job.headers;
try {
decryptedHeaders = await crypto.decrypt(serializedEncryptedHeaders);
if (typeof serializedEncryptedHeaders !== 'string') {
throw new Error('Job headers are missing');
Copy link
Member

Choose a reason for hiding this comment

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

looks like we'd want this wrapped in i18n.translate too

}
decryptedHeaders = (await crypto.decrypt(serializedEncryptedHeaders)) as Record<
string,
unknown
>;
} catch (err) {
jobLogger.error(err);
throw new Error(
Expand Down
6 changes: 5 additions & 1 deletion x-pack/legacy/plugins/reporting/server/lib/crypto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

import nodeCrypto from '@elastic/node-crypto';

export function cryptoFactory(encryptionKey: string | undefined) {
export function cryptoFactory(encryptionKey?: string) {
if (typeof encryptionKey !== 'string') {
Copy link
Member Author

Choose a reason for hiding this comment

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

nodeCrypto will throw an error if encryptionKey is undefined: https://github.com/elastic/node-crypto/blob/master/src/crypto.ts#L37

This will type guard the parameter.

throw new Error('Encryption Key required.');
}

return nodeCrypto({ encryptionKey });
}
4 changes: 0 additions & 4 deletions x-pack/legacy/plugins/reporting/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,6 @@ export interface ConditionalHeadersConditions {
basePath: string;
}

export interface CryptoFactory {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed anymore since Crypto is typed

decrypt: (headers?: string) => any;
}

export interface IndexPatternSavedObject {
attributes: {
fieldFormatMap: string;
Expand Down
2 changes: 1 addition & 1 deletion x-pack/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@
"@elastic/eui": "21.0.1",
"@elastic/filesaver": "1.1.2",
"@elastic/maki": "6.2.0",
"@elastic/node-crypto": "^1.0.0",
"@elastic/node-crypto": "1.1.1",
"@elastic/numeral": "2.4.0",
"@kbn/babel-preset": "1.0.0",
"@kbn/config-schema": "1.0.0",
Expand Down
7 changes: 4 additions & 3 deletions x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ beforeEach(() => {
mockAuditLogger = encryptedSavedObjectsAuditLoggerMock.create();

// Call actual `@elastic/node-crypto` by default, but allow to override implementation in tests.
jest
.requireMock('@elastic/node-crypto')
.mockImplementation((...args: any[]) => jest.requireActual('@elastic/node-crypto')(...args));
jest.requireMock('@elastic/node-crypto').mockImplementation((...args: any[]) => {
const { default: nodeCrypto } = jest.requireActual('@elastic/node-crypto');
return nodeCrypto(...args);
});

service = new EncryptedSavedObjectsService(
'encryption-key-abc',
Expand Down
12 changes: 4 additions & 8 deletions x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

// @ts-ignore
import nodeCrypto from '@elastic/node-crypto';
import nodeCrypto, { Crypto } from '@elastic/node-crypto';
import stringify from 'json-stable-stringify';
import typeDetect from 'type-detect';
import { Logger } from 'src/core/server';
Expand Down Expand Up @@ -49,10 +48,7 @@ export function descriptorToArray(descriptor: SavedObjectDescriptor) {
* attributes.
*/
export class EncryptedSavedObjectsService {
private readonly crypto: Readonly<{
encrypt<T>(valueToEncrypt: T, aad?: string): Promise<string>;
decrypt<T>(valueToDecrypt: string, aad?: string): Promise<T>;
}>;
private readonly crypto: Readonly<Crypto>;

/**
* Map of all registered saved object types where the `key` is saved object type and the `value`
Expand Down Expand Up @@ -229,10 +225,10 @@ export class EncryptedSavedObjectsService {
}

try {
decryptedAttributes[attributeName] = await this.crypto.decrypt(
decryptedAttributes[attributeName] = (await this.crypto.decrypt(
attributeValue,
encryptionAAD
);
)) as string;
Copy link
Member

Choose a reason for hiding this comment

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

nit: Any chance we can update the types for node-crypto to allow decrypt to accept a generic argument, like the old interface supported?

decrypt<T>(valueToDecrypt: string, aad?: string): Promise<T>;

Copy link
Member

Choose a reason for hiding this comment

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

@legrego we actually had a heated discussion about the same in the node-crypto repo but we decided not to do so for a reason:
The valueToDecrypt is not validated in any way before decryption (apart from the fact that it's a string when encrypted). It means that we have no control over the output of the decrypt method.
i.e.: You might be expecting the decrypted value to be a string, but that doesn't stop anyone to provide a valueToDecrypt that gets decrypted into anything else (string[], object, boolean, ...).
In my opinion, accepting a generic argument would allow (and encourage) devs to set the expected output, defeating the purpose of TS as a best practice advisor. With the current implementation (no generic argument), TS reminds you might get an unexpected output and you should implement type-guards. If you are sure about the exact output because you have full control over the input, you can cast it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation @afharo!

} catch (err) {
this.logger.error(`Failed to decrypt "${attributeName}" attribute: ${err.message || err}`);
this.audit.decryptAttributeFailure(attributeName, descriptor);
Expand Down
7 changes: 0 additions & 7 deletions x-pack/typings/elastic__node_crypto.d.ts
View file
Open in desktop

This file was deleted.

23 changes: 9 additions & 14 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,27 +1343,22 @@
resolved "https://registry.yarnpkg.com/@elastic/maki/-/maki-6.2.0.tgz#d0a85aa248bdc14dca44e1f9430c0b670f65e489"
integrity sha512-QkmRNpEY4Dy6eqwDimR5X9leMgdPFjdANmpEIwEW1XVUG2U4YtB2BXhDxsnMmNTUrJUjtnjnwgwBUyg0pU0FTg==

"@elastic/node-crypto@^0.1.2":
version "0.1.2"
resolved "https://registry.yarnpkg.com/@elastic/node-crypto/-/node-crypto-0.1.2.tgz#c18ac282f635e88f041cc1555d806e492ca8f3b1"
integrity sha1-wYrCgvY16I8EHMFVXYBuSSyo87E=

"@elastic/node-crypto@^1.0.0":
version "1.0.0"
resolved "https://registry.yarnpkg.com/@elastic/node-crypto/-/node-crypto-1.0.0.tgz#4d325df333fe1319556bb4d54214098ada1171d4"
integrity sha512-bbjbEyILPRTRt0xnda18OttLtlkJBPuXx3CjISUSn9jhWqHoFMzfOaZ73D5jxZE2SaFZUrJYfPpqXP6qqPufAQ==
"@elastic/[email protected]":
version "1.1.1"
resolved "https://registry.yarnpkg.com/@elastic/node-crypto/-/node-crypto-1.1.1.tgz#619b70322c9cce4a7ee5fbf8f678b1baa7f06095"
integrity sha512-F6tIk8Txdqjg8Siv60iAvXzO9ZdQI87K3sS/fh5xd2XaWK+T5ZfqeTvsT7srwG6fr6uCBfuQEJV1KBBl+JpLZA==

"@elastic/[email protected]":
version "2.4.0"
resolved "https://registry.yarnpkg.com/@elastic/numeral/-/numeral-2.4.0.tgz#883197b7f4bf3c2dd994f53b274769ddfa2bf79a"
integrity sha512-uGBKGCNghTgUZPHClji/00v+AKt5nidPTGOIbcT+lbTPVxNB6QPpPLGWtXyrg3QZAxobPM/LAZB1mAqtJeq44Q==

"@elastic/request-crypto@^1.0.2":
version "1.0.2"
resolved "https://registry.yarnpkg.com/@elastic/request-crypto/-/request-crypto-1.0.2.tgz#bf27bf009227166f3eeb2b5193a108752335ebd3"
integrity sha512-8FtGYl7LebhmJmEDWiGn3MorvNiGWSYPqhvgRlKXjNakEuLoPBBe0DHxbwLkj08CMLWczXcO2ixqBPY7fEhJpA==
"@elastic/request-crypto@1.1.2":
version "1.1.2"
resolved "https://registry.yarnpkg.com/@elastic/request-crypto/-/request-crypto-1.1.2.tgz#2e323550f546f6286994126d462a9ea480a3bfb1"
integrity sha512-i73wjj1Qi8dGJIy170Z8xyJ760mFNjTbdmcp/nEczqWD0miNW6I5wZ5MNrv7M6CXn2m1wMXiT6qzDYd93Hv1Dw==
dependencies:
"@elastic/node-crypto" "^0.1.2"
"@elastic/node-crypto" "1.1.1"
"@types/node-jose" "1.1.0"
node-jose "1.1.0"

Expand Down