[Telemetry] update crypto packages by Bamieh · Pull Request #62469 · elastic/kibana

Bamieh · 2020-04-03T15:45:24Z

Update crypto packages, no breaking changes just converted the libraries to TS for typings support.

Removed typings/elastic__node_crypto.d.ts since the package provides the types.

Removed @ts-ignore from @elastic/node-crypto imports since this package is now typed as well.

Closes #58023

…ate_crypto_packages

elasticmachine · 2020-04-03T15:45:38Z

Pinging @elastic/pulse (Team:Pulse)

…ate_crypto_packages

Bamieh · 2020-04-06T11:55:28Z

@elasticmachine merge upstream

afharo

LGTM!

legrego · 2020-04-06T12:33:49Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts

          attributeValue,
          encryptionAAD
-        );
+        )) as string;


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>;

@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.

Thanks for the explanation @afharo!

Bamieh · 2020-04-06T13:54:21Z

x-pack/legacy/plugins/reporting/export_types/common/execute_job/decrypt_job_headers.ts

  try {
-    const decryptedHeaders: Record<string, string> = await crypto.decrypt(job.headers);
+    if (typeof job.headers !== 'string') {
+      throw new Error('Job headers are missing');


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

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

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.

Bamieh · 2020-04-06T13:55:30Z

x-pack/legacy/plugins/reporting/server/lib/crypto.ts


-export function cryptoFactory(encryptionKey: string | undefined) {
+export function cryptoFactory(encryptionKey?: string) {
+  if (typeof encryptionKey !== 'string') {


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.

Bamieh · 2020-04-06T13:55:44Z

x-pack/legacy/plugins/reporting/types.d.ts

  basePath: string;
 }

-export interface CryptoFactory {


Not needed anymore since Crypto is typed

tsullivan · 2020-04-06T15:38:30Z

x-pack/legacy/plugins/reporting/export_types/csv_from_savedobject/server/execute_job.ts

      try {
-        decryptedHeaders = await crypto.decrypt(serializedEncryptedHeaders);
+        if (typeof serializedEncryptedHeaders !== 'string') {
+          throw new Error('Job headers are missing');


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

joelgriffith

LGTM, thanks for the typeof checks on the headers (though those might be handled in joi schema's elsewhere, probably not a bad thing to double check).

tsullivan · 2020-04-06T15:41:56Z

x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.ts

      try {
-        decryptedHeaders = await crypto.decrypt(headers);
+        if (typeof headers !== 'string') {
+          throw new Error('Job headers are missing');


wrap the error message in i18n ?

tsullivan

On the reporting side, I think the new error messages should be wrapped in i18n.translate, as other error strings are done in the files that have been changed.

I also want to check with you that the crypto import in x-pack/legacy/plugins/reporting/server/config/index.ts is still OK as it is.

Bamieh · 2020-04-06T16:50:30Z

@joelgriffith @tsullivan thanks for the review!

I've updated the PR based on your feedback. @tsullivan the crytpo is a native module in node so using it to generate the encryption key is still OK and preferred.

We actually do something similar in request-crypto which is used to encrypt telemetry payloads in kibana if you wish to grab that instead.

import { generatePassphrase } from '@elastic/request-crypto';
const encryptionKey = generatePassphrase.toString('hex');

Here we generate 32 random bytes instead of 16 since it is considered a better practice to do so. This way the method can be used to create salt keys as well (16 bytes will be used if only 16 are needed).

kibanamachine · 2020-04-06T18:13:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5ef32c8

History

💚 Build #38859 succeeded 74c17a3
💔 Build #38814 failed bed4eb4
💚 Build #38557 succeeded 2cd9562
💔 Build #38486 failed 5e44e78
💔 Build #38459 failed 4e9c932

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

tsullivan

LGTM, and thank you for the write-up about generating encryption keys. :)

* master: (36 commits) [data.search.aggs] Remove service getters from agg types (elastic#61628) fixing APM internationalization (elastic#62757) fix: 🐛 correctly create error on no_matching_indices (elastic#61257) [Lens] Remove all legacy imports (elastic#62596) Add label for ace editor (elastic#62588) [ML] Show better file structure finder explanations (elastic#62316) Fix old pathes in eslintrc (elastic#62580) [Uptime] Improve Telemetry test (elastic#62428) [SIEM] Adds sort rules Cypress test (elastic#62700) [Uptime]Abstracted 'access:uptime-read' tag into a wrapper for… (elastic#62576) fixing bug (elastic#62577) [Maps] Allow updating requestType for ESGeoGridSource (elastic#62365) [Maps] do not show circle border when symbol size is zero (elastic#62644) [Maps] Always show current zoom level (elastic#62684) bc5 siem rules merge (elastic#62679) Revert "[Monitoring] Cluster state watch to Kibana alerting (elastic#61685)" Fix visual tests (elastic#62660) [Telemetry] update crypto packages (elastic#62469) [DOCS] Removed references to left (elastic#60807) [Maps] Move layers to np maps (elastic#61877) ...

kibanamachine · 2020-04-08T21:38:45Z