From 6b6e5e9b5df3d7d7bd7da92e0e7386505635c741 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Mon, 10 Apr 2023 11:39:01 -0600 Subject: [PATCH 1/9] fix: support stronger ciphers --- src/crypto/crypto.ts | 79 ++++++++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/src/crypto/crypto.ts b/src/crypto/crypto.ts index 6ed30bbe48..7fea403a04 100644 --- a/src/crypto/crypto.ts +++ b/src/crypto/crypto.ts @@ -9,9 +9,10 @@ import * as crypto from 'crypto'; import * as os from 'os'; import { join as pathJoin } from 'path'; -import { ensure, Nullable, Optional } from '@salesforce/ts-types'; +import { ensure, isString, Nullable, Optional } from '@salesforce/ts-types'; import { AsyncOptionalCreatable, env } from '@salesforce/kit'; import { Logger } from '../logger'; +import { SfError } from '../sfError'; import { Messages } from '../messages'; import { Cache } from '../util/cache'; import { Global } from '../global'; @@ -20,7 +21,7 @@ import { KeyChain } from './keyChainImpl'; import { SecureBuffer } from './secureBuffer'; const TAG_DELIMITER = ':'; -const BYTE_COUNT_FOR_IV = 6; +const BYTE_COUNT_FOR_IV = 12; const ALGO = 'aes-256-gcm'; const AUTH_TAG_LENGTH = 32; @@ -39,7 +40,7 @@ interface CredType { const makeSecureBuffer = (password: string | undefined): SecureBuffer => { const newSb = new SecureBuffer(); - newSb.consume(Buffer.from(ensure(password), 'utf8')); + newSb.consume(Buffer.from(ensure(password), 'hex')); return newSb; }; @@ -66,7 +67,7 @@ const keychainPromises = { }) ); } else { - const pw = sb.value((buffer) => buffer.toString('utf8')); + const pw = sb.value((buffer) => buffer.toString('hex')); Cache.set(cacheKey, makeSecureBuffer(pw)); return new Promise((resolve): void => resolve({ username: account, password: ensure(pw) })); } @@ -107,6 +108,9 @@ export class Crypto extends AsyncOptionalCreatable { private noResetOnClose!: boolean; + // true when the key is 32 hex chars + private legacy_key_mode = false; + /** * Constructor * **Do not directly construct instances of this class -- use {@link Crypto.create} instead.** @@ -134,16 +138,17 @@ export class Crypto extends AsyncOptionalCreatable { throw messages.createError('keychainPasswordCreationError'); } - const iv = crypto.randomBytes(BYTE_COUNT_FOR_IV).toString('hex'); + const iv = crypto.randomBytes(BYTE_COUNT_FOR_IV); return this.key.value((buffer: Buffer): string => { - const cipher = crypto.createCipheriv(ALGO, buffer.toString('utf8'), iv); + const cipher = crypto.createCipheriv(ALGO, buffer, iv); + const ivHex = iv.toString('hex'); let encrypted = cipher.update(text, 'utf8', 'hex'); encrypted += cipher.final('hex'); const tag = cipher.getAuthTag().toString('hex'); - return `${iv}${encrypted}${TAG_DELIMITER}${tag}`; + return `${ivHex}${encrypted}${TAG_DELIMITER}${tag}`; }); } @@ -169,21 +174,41 @@ export class Crypto extends AsyncOptionalCreatable { const secret = tokens[0].substring(BYTE_COUNT_FOR_IV * 2, tokens[0].length); return this.key.value((buffer: Buffer) => { - const decipher = crypto.createDecipheriv(ALGO, buffer.toString('utf8'), iv); + let decipher = crypto.createDecipheriv(ALGO, buffer, Buffer.from(iv, 'hex')); - let dec; + let dec!: string; try { decipher.setAuthTag(Buffer.from(tag, 'hex')); dec = decipher.update(secret, 'hex', 'utf8'); dec += decipher.final('utf8'); - } catch (err) { - const error = messages.createError('authDecryptError', [(err as Error).message], [], err as Error); - const useGenericUnixKeychain = - env.getBoolean('SFDX_USE_GENERIC_UNIX_KEYCHAIN') || env.getBoolean('USE_GENERIC_UNIX_KEYCHAIN'); - if (os.platform() === 'darwin' && !useGenericUnixKeychain) { - error.actions = [messages.getMessage('macKeychainOutOfSync')]; + } catch (_err: unknown) { + const err = (isString(_err) ? SfError.wrap(_err) : _err) as Error; + const handleDecryptError = (decryptErr: Error) => { + const error = messages.createError('authDecryptError', [decryptErr.message], [], decryptErr); + const useGenericUnixKeychain = + env.getBoolean('SFDX_USE_GENERIC_UNIX_KEYCHAIN') || env.getBoolean('USE_GENERIC_UNIX_KEYCHAIN'); + if (os.platform() === 'darwin' && !useGenericUnixKeychain) { + error.actions = [messages.getMessage('macKeychainOutOfSync')]; + } + throw error; + }; + + if (this.legacy_key_mode && err?.message === 'Unsupported state or unable to authenticate data') { + try { + const iv_legacy = tokens[0].substring(0, BYTE_COUNT_FOR_IV); + const secret_legacy = tokens[0].substring(BYTE_COUNT_FOR_IV, tokens[0].length); + // legacy encryption used a utf8 encoded string from the buffer + decipher = crypto.createDecipheriv(ALGO, buffer.toString('utf8'), iv_legacy); + decipher.setAuthTag(Buffer.from(tag, 'hex')); + dec = decipher.update(secret_legacy, 'hex', 'utf8'); + dec += decipher.final('utf8'); + } catch (_err2: unknown) { + const err2 = (isString(_err2) ? SfError.wrap(_err2) : _err2) as Error; + handleDecryptError(err2); + } + } else { + handleDecryptError(err); } - throw error; } return dec; }); @@ -242,13 +267,19 @@ export class Crypto extends AsyncOptionalCreatable { this.noResetOnClose = !!this.options.noResetOnClose; try { - this.key.consume( - Buffer.from( - (await keychainPromises.getPassword(await this.getKeyChain(this.options.platform), KEY_NAME, ACCOUNT)) - .password, - 'utf8' - ) - ); + const keyChain = await this.getKeyChain(this.options.platform); + const pwdHex = (await keychainPromises.getPassword(keyChain, KEY_NAME, ACCOUNT)).password; + // This supports the legacy key size (32 hex chars) and the new key size (64 hex chars). + let encoding: 'hex' | 'utf8'; + if (pwdHex.length === 64) { + encoding = 'hex'; + logger.debug('Detected new key size'); + } else { + encoding = 'utf8'; + logger.debug('Detected legacy key size'); + this.legacy_key_mode = true; + } + this.key.consume(Buffer.from(pwdHex, encoding)); } catch (err) { // No password found if ((err as Error).name === 'PasswordNotFoundError') { @@ -260,7 +291,7 @@ export class Crypto extends AsyncOptionalCreatable { logger.debug('password not found in keychain attempting to created one and re-init.'); } - const key = crypto.randomBytes(Math.ceil(16)).toString('hex'); + const key = crypto.randomBytes(32).toString('hex'); // Create a new password in the KeyChain. await keychainPromises.setPassword(ensure(this.options.keychain), KEY_NAME, ACCOUNT, key); From aca00d9d3effff75a7044944d8b7f15a9a3d1214 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Mon, 29 Jan 2024 16:49:19 -0700 Subject: [PATCH 2/9] fix: update tests for Logger --- src/crypto/crypto.ts | 266 ++++++++++++++++++++++--------- test/unit/lifecycleEventsTest.ts | 3 +- 2 files changed, 189 insertions(+), 80 deletions(-) diff --git a/src/crypto/crypto.ts b/src/crypto/crypto.ts index 7161fd79e5..2de71df782 100644 --- a/src/crypto/crypto.ts +++ b/src/crypto/crypto.ts @@ -12,6 +12,7 @@ import { join as pathJoin } from 'node:path'; import { ensure, isString, Nullable, Optional } from '@salesforce/ts-types'; import { AsyncOptionalCreatable, env } from '@salesforce/kit'; import { Logger } from '../logger/logger'; +import { Lifecycle } from '../lifecycleEvents'; import { Messages } from '../messages'; import { Cache } from '../util/cache'; import { Global } from '../global'; @@ -21,7 +22,18 @@ import { KeyChain } from './keyChainImpl'; import { SecureBuffer } from './secureBuffer'; const TAG_DELIMITER = ':'; -const BYTE_COUNT_FOR_IV = 12; +const IV_BYTES = { + v1: 6, + v2: 12, +}; +const ENCODING: { v1: 'utf8'; v2: 'hex' } = { + v1: 'utf8', + v2: 'hex', +}; +const KEY_SIZE = { + v1: 16, + v2: 32, +}; const ALGO = 'aes-256-gcm'; const AUTH_TAG_LENGTH = 32; @@ -30,6 +42,30 @@ const ENCRYPTED_CHARS = /[a-f0-9]/; const KEY_NAME = 'sfdx'; const ACCOUNT = 'local'; +let cryptoLogger: Logger; +const getCryptoLogger = (): Logger => { + if (!cryptoLogger) { + cryptoLogger = Logger.childFromRoot('crypto'); + } + return cryptoLogger; +}; + +type CryptoVersion = 'v1' | 'v2'; +let cryptoVersion: CryptoVersion; +const getCryptoVersion = (): CryptoVersion => { + if (!cryptoVersion) { + cryptoVersion = env.getBoolean('SF_CRYPTO_V2') ? 'v2' : 'v1'; + getCryptoLogger().debug(`Using ${cryptoVersion} Crypto`); + void Lifecycle.getInstance().emitTelemetry({ + eventName: 'crypto_version', + library: 'sfdx-core', + function: 'getCryptoVersion', + cryptoVersion, + }); + } + return cryptoVersion; +}; + Messages.importMessagesDirectory(pathJoin(__dirname)); const messages = Messages.loadMessages('@salesforce/core', 'encryption'); @@ -38,9 +74,16 @@ interface CredType { password: string; } -const makeSecureBuffer = (password: string | undefined): SecureBuffer => { +type DecryptConfigV1 = { + tag: string; + iv: string; + secret: string; +}; +type DecryptConfigV2 = DecryptConfigV1 & { tokens: string[] }; + +const makeSecureBuffer = (password: string | undefined, encoding: 'utf8' | 'hex'): SecureBuffer => { const newSb = new SecureBuffer(); - newSb.consume(Buffer.from(ensure(password), 'hex')); + newSb.consume(Buffer.from(ensure(password), encoding)); return newSb; }; @@ -55,20 +98,20 @@ const keychainPromises = { * @param service The keychain service name. * @param account The keychain account name. */ - getPassword(_keychain: KeyChain, service: string, account: string): Promise { + getPassword(_keychain: KeyChain, service: string, account: string, encoding: 'utf8' | 'hex'): Promise { const cacheKey = `${Global.DIR}:${service}:${account}`; const sb = Cache.get>(cacheKey); if (!sb) { return new Promise((resolve, reject): {} => _keychain.getPassword({ service, account }, (err: Nullable, password?: string) => { if (err) return reject(err); - Cache.set(cacheKey, makeSecureBuffer(password)); + Cache.set(cacheKey, makeSecureBuffer(password, encoding)); return resolve({ username: account, password: ensure(password) }); }) ); } else { - const pw = sb.value((buffer) => buffer.toString('hex')); - Cache.set(cacheKey, makeSecureBuffer(pw)); + const pw = sb.value((buffer) => buffer.toString(encoding)); + Cache.set(cacheKey, makeSecureBuffer(pw, encoding)); return new Promise((resolve): void => resolve({ username: account, password: ensure(pw) })); } }, @@ -108,8 +151,12 @@ export class Crypto extends AsyncOptionalCreatable { private noResetOnClose!: boolean; - // true when the key is 32 hex chars - private legacyKeyMode = false; + // `true` when the key is 32 hex chars, which is a key created by v1 crypto. + // This is used to determine encoding (utf8/hex), and for v2 crypto to retry + // using v1 crypto. + private v1KeyLength = false; + + private cryptoVersion: CryptoVersion; /** * Constructor @@ -121,6 +168,7 @@ export class Crypto extends AsyncOptionalCreatable { public constructor(options?: CryptoOptions) { super(options); this.options = options ?? {}; + this.cryptoVersion = this.getCryptoVersion(); } /** @@ -138,18 +186,12 @@ export class Crypto extends AsyncOptionalCreatable { throw messages.createError('keychainPasswordCreationError'); } - const iv = crypto.randomBytes(BYTE_COUNT_FOR_IV); - - return this.key.value((buffer: Buffer): string => { - const cipher = crypto.createCipheriv(ALGO, buffer, iv); - const ivHex = iv.toString('hex'); - - let encrypted = cipher.update(text, 'utf8', 'hex'); - encrypted += cipher.final('hex'); - - const tag = cipher.getAuthTag().toString('hex'); - return `${ivHex}${encrypted}${TAG_DELIMITER}${tag}`; - }); + // When everything is v2, we can remove the else + if (this.isV2Crypto()) { + return this.encryptV2(text); + } else { + return this.encryptV1(text); + } } /** @@ -170,48 +212,15 @@ export class Crypto extends AsyncOptionalCreatable { } const tag = tokens[1]; - const iv = tokens[0].substring(0, BYTE_COUNT_FOR_IV * 2); - const secret = tokens[0].substring(BYTE_COUNT_FOR_IV * 2, tokens[0].length); - - return this.key.value((buffer: Buffer) => { - let decipher = crypto.createDecipheriv(ALGO, buffer, Buffer.from(iv, 'hex')); - - let dec!: string; - try { - decipher.setAuthTag(Buffer.from(tag, 'hex')); - dec = decipher.update(secret, 'hex', 'utf8'); - dec += decipher.final('utf8'); - } catch (_err: unknown) { - const err = (isString(_err) ? SfError.wrap(_err) : _err) as Error; - const handleDecryptError = (decryptErr: Error): void => { - const error = messages.createError('authDecryptError', [decryptErr.message], [], decryptErr); - const useGenericUnixKeychain = - env.getBoolean('SF_USE_GENERIC_UNIX_KEYCHAIN') || env.getBoolean('USE_GENERIC_UNIX_KEYCHAIN'); - if (os.platform() === 'darwin' && !useGenericUnixKeychain) { - error.actions = [messages.getMessage('macKeychainOutOfSync')]; - } - throw error; - }; + const iv = tokens[0].substring(0, IV_BYTES[this.getCryptoVersion()] * 2); + const secret = tokens[0].substring(IV_BYTES[this.getCryptoVersion()] * 2, tokens[0].length); - if (this.legacyKeyMode && err?.message === 'Unsupported state or unable to authenticate data') { - try { - const ivLegacy = tokens[0].substring(0, BYTE_COUNT_FOR_IV); - const secretLegacy = tokens[0].substring(BYTE_COUNT_FOR_IV, tokens[0].length); - // legacy encryption used a utf8 encoded string from the buffer - decipher = crypto.createDecipheriv(ALGO, buffer.toString('utf8'), ivLegacy); - decipher.setAuthTag(Buffer.from(tag, 'hex')); - dec = decipher.update(secretLegacy, 'hex', 'utf8'); - dec += decipher.final('utf8'); - } catch (_err2: unknown) { - const err2 = (isString(_err2) ? SfError.wrap(_err2) : _err2) as Error; - handleDecryptError(err2); - } - } else { - handleDecryptError(err); - } - } - return dec; - }); + // When everything is v2, we can remove the else + if (this.isV2Crypto()) { + return this.decryptV2({ tag, iv, secret, tokens }); + } else { + return this.decryptV1({ tag, iv, secret }); + } } /** @@ -237,7 +246,7 @@ export class Crypto extends AsyncOptionalCreatable { const value = tokens[0]; return ( tag.length === AUTH_TAG_LENGTH && - value.length >= BYTE_COUNT_FOR_IV && + value.length >= IV_BYTES[this.getCryptoVersion()] && ENCRYPTED_CHARS.test(tag) && ENCRYPTED_CHARS.test(tokens[0]) ); @@ -252,46 +261,46 @@ export class Crypto extends AsyncOptionalCreatable { } } + public isV2Crypto(): boolean { + return this.getCryptoVersion() === 'v2'; + } + /** * Initialize async components. */ protected async init(): Promise { - const logger = await Logger.child('crypto'); - if (!this.options.platform) { this.options.platform = os.platform(); } - logger.debug(`retryStatus: ${this.options.retryStatus}`); + getCryptoLogger().debug(`retryStatus: ${this.options.retryStatus}`); this.noResetOnClose = !!this.options.noResetOnClose; try { const keyChain = await this.getKeyChain(this.options.platform); - const pwdHex = (await keychainPromises.getPassword(keyChain, KEY_NAME, ACCOUNT)).password; - // This supports the legacy key size (32 hex chars) and the new key size (64 hex chars). - let encoding: 'hex' | 'utf8'; - if (pwdHex.length === 64) { - encoding = 'hex'; - logger.debug('Detected new key size'); + const pwd = (await keychainPromises.getPassword(keyChain, KEY_NAME, ACCOUNT, ENCODING[this.getCryptoVersion()])) + .password; + // This supports the v1 key size (32 hex chars) and the v2 key size (64 hex chars). + if (pwd.length === KEY_SIZE.v2 * 2) { + getCryptoLogger().debug('Detected v2 key size'); } else { - encoding = 'utf8'; - logger.debug('Detected legacy key size'); - this.legacyKeyMode = true; + getCryptoLogger().debug('Detected v1 key size'); + this.v1KeyLength = true; } - this.key.consume(Buffer.from(pwdHex, encoding)); + this.key.consume(Buffer.from(pwd, this.v1KeyLength ? ENCODING.v1 : ENCODING.v2)); } catch (err) { // No password found if ((err as Error).name === 'PasswordNotFoundError') { // If we already tried to create a new key then bail. if (this.options.retryStatus === 'KEY_SET') { - logger.debug('a key was set but the retry to get the password failed.'); + getCryptoLogger().debug('a key was set but the retry to get the password failed.'); throw err; } else { - logger.debug('password not found in keychain attempting to created one and re-init.'); + getCryptoLogger().debug('password not found in keychain. attempting to create one and re-init.'); } - const key = crypto.randomBytes(32).toString('hex'); + const key = crypto.randomBytes(KEY_SIZE[this.getCryptoVersion()]).toString('hex'); // Create a new password in the KeyChain. await keychainPromises.setPassword(ensure(this.options.keychain), KEY_NAME, ACCOUNT, key); @@ -302,6 +311,105 @@ export class Crypto extends AsyncOptionalCreatable { } } + // enables overriding the version in tests + private getCryptoVersion(): CryptoVersion { + return this.cryptoVersion ?? getCryptoVersion(); + } + + private encryptV1(text: string): Optional { + const iv = crypto.randomBytes(IV_BYTES.v1).toString('hex'); + + return this.key.value((buffer: Buffer): string => { + const cipher = crypto.createCipheriv(ALGO, buffer.toString('utf8'), iv); + + let encrypted = cipher.update(text, 'utf8', 'hex'); + encrypted += cipher.final('hex'); + + const tag = cipher.getAuthTag().toString('hex'); + return `${iv}${encrypted}${TAG_DELIMITER}${tag}`; + }); + } + + private encryptV2(text: string): Optional { + const iv = crypto.randomBytes(IV_BYTES.v2); + + return this.key.value((buffer: Buffer): string => { + const cipher = crypto.createCipheriv(ALGO, buffer, iv); + const ivHex = iv.toString('hex'); + + let encrypted = cipher.update(text, 'utf8', 'hex'); + encrypted += cipher.final('hex'); + + const tag = cipher.getAuthTag().toString('hex'); + return `${ivHex}${encrypted}${TAG_DELIMITER}${tag}`; + }); + } + + private decryptV1({ tag, iv, secret }: DecryptConfigV1): Optional { + return this.key.value((buffer: Buffer) => { + const decipher = crypto.createDecipheriv(ALGO, buffer.toString('utf8'), iv); + + let dec; + try { + decipher.setAuthTag(Buffer.from(tag, 'hex')); + dec = decipher.update(secret, 'hex', 'utf8'); + dec += decipher.final('utf8'); + } catch (err) { + const error = messages.createError('authDecryptError', [(err as Error).message], [], err as Error); + const useGenericUnixKeychain = + env.getBoolean('SF_USE_GENERIC_UNIX_KEYCHAIN') || env.getBoolean('USE_GENERIC_UNIX_KEYCHAIN'); + if (os.platform() === 'darwin' && !useGenericUnixKeychain) { + error.actions = [messages.getMessage('macKeychainOutOfSync')]; + } + throw error; + } + return dec; + }); + } + + private decryptV2({ tag, iv, secret, tokens }: DecryptConfigV2): Optional { + return this.key.value((buffer: Buffer) => { + let decipher = crypto.createDecipheriv(ALGO, buffer, Buffer.from(iv, 'hex')); + + let dec!: string; + try { + decipher.setAuthTag(Buffer.from(tag, 'hex')); + dec = decipher.update(secret, 'hex', 'utf8'); + dec += decipher.final('utf8'); + } catch (_err: unknown) { + const err = (isString(_err) ? SfError.wrap(_err) : _err) as Error; + const handleDecryptError = (decryptErr: Error): void => { + const error = messages.createError('authDecryptError', [decryptErr.message], [], decryptErr); + const useGenericUnixKeychain = + env.getBoolean('SF_USE_GENERIC_UNIX_KEYCHAIN') || env.getBoolean('USE_GENERIC_UNIX_KEYCHAIN'); + if (os.platform() === 'darwin' && !useGenericUnixKeychain) { + error.actions = [messages.getMessage('macKeychainOutOfSync')]; + } + throw error; + }; + + if (this.v1KeyLength && err?.message === 'Unsupported state or unable to authenticate data') { + getCryptoLogger().debug('v2 decryption failed so trying v1 decryption'); + try { + const ivLegacy = tokens[0].substring(0, IV_BYTES.v2); + const secretLegacy = tokens[0].substring(IV_BYTES.v2, tokens[0].length); + // v1 encryption uses a utf8 encoded string from the buffer + decipher = crypto.createDecipheriv(ALGO, buffer.toString('utf8'), ivLegacy); + decipher.setAuthTag(Buffer.from(tag, 'hex')); + dec = decipher.update(secretLegacy, 'hex', 'utf8'); + dec += decipher.final('utf8'); + } catch (_err2: unknown) { + const err2 = (isString(_err2) ? SfError.wrap(_err2) : _err2) as Error; + handleDecryptError(err2); + } + } else { + handleDecryptError(err); + } + } + return dec; + }); + } + private async getKeyChain(platform: string): Promise { if (!this.options.keychain) { this.options.keychain = await retrieveKeychain(platform); diff --git a/test/unit/lifecycleEventsTest.ts b/test/unit/lifecycleEventsTest.ts index d84a4a9121..786a49fe9c 100644 --- a/test/unit/lifecycleEventsTest.ts +++ b/test/unit/lifecycleEventsTest.ts @@ -11,6 +11,7 @@ import { spyMethod } from '@salesforce/ts-sinon'; import * as chai from 'chai'; import { Lifecycle } from '../../src/lifecycleEvents'; import { TestContext } from '../../src/testSetup'; +import { Logger } from '../../src/logger/logger'; describe('lifecycleEvents', () => { const $$ = new TestContext(); @@ -26,7 +27,7 @@ describe('lifecycleEvents', () => { const fake = new Foo(); beforeEach(() => { - loggerSpy = spyMethod($$.SANDBOX, $$.TEST_LOGGER, 'debug'); + loggerSpy = spyMethod($$.SANDBOX, Logger.prototype, 'debug'); fakeSpy = spyMethod($$.SANDBOX, fake, 'bar'); }); From b9b947c93e924e97cd8bf21fa4dac8df3029f4c3 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Tue, 30 Jan 2024 14:33:36 -0700 Subject: [PATCH 3/9] fix: review updates --- src/crypto/crypto.ts | 51 +++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/src/crypto/crypto.ts b/src/crypto/crypto.ts index 2de71df782..30feea1cb1 100644 --- a/src/crypto/crypto.ts +++ b/src/crypto/crypto.ts @@ -26,10 +26,10 @@ const IV_BYTES = { v1: 6, v2: 12, }; -const ENCODING: { v1: 'utf8'; v2: 'hex' } = { +const ENCODING = { v1: 'utf8', v2: 'hex', -}; +} as const; const KEY_SIZE = { v1: 16, v2: 32, @@ -44,13 +44,12 @@ const ACCOUNT = 'local'; let cryptoLogger: Logger; const getCryptoLogger = (): Logger => { - if (!cryptoLogger) { - cryptoLogger = Logger.childFromRoot('crypto'); - } + cryptoLogger ??= Logger.childFromRoot('crypto'); return cryptoLogger; }; -type CryptoVersion = 'v1' | 'v2'; +type CryptoEncoding = 'utf8' | 'hex'; +type CryptoVersion = keyof typeof IV_BYTES; let cryptoVersion: CryptoVersion; const getCryptoVersion = (): CryptoVersion => { if (!cryptoVersion) { @@ -74,14 +73,7 @@ interface CredType { password: string; } -type DecryptConfigV1 = { - tag: string; - iv: string; - secret: string; -}; -type DecryptConfigV2 = DecryptConfigV1 & { tokens: string[] }; - -const makeSecureBuffer = (password: string | undefined, encoding: 'utf8' | 'hex'): SecureBuffer => { +const makeSecureBuffer = (password: string | undefined, encoding: CryptoEncoding): SecureBuffer => { const newSb = new SecureBuffer(); newSb.consume(Buffer.from(ensure(password), encoding)); return newSb; @@ -98,7 +90,7 @@ const keychainPromises = { * @param service The keychain service name. * @param account The keychain account name. */ - getPassword(_keychain: KeyChain, service: string, account: string, encoding: 'utf8' | 'hex'): Promise { + getPassword(_keychain: KeyChain, service: string, account: string, encoding: CryptoEncoding): Promise { const cacheKey = `${Global.DIR}:${service}:${account}`; const sb = Cache.get>(cacheKey); if (!sb) { @@ -211,15 +203,11 @@ export class Crypto extends AsyncOptionalCreatable { throw messages.createError('invalidEncryptedFormatError'); } - const tag = tokens[1]; - const iv = tokens[0].substring(0, IV_BYTES[this.getCryptoVersion()] * 2); - const secret = tokens[0].substring(IV_BYTES[this.getCryptoVersion()] * 2, tokens[0].length); - // When everything is v2, we can remove the else if (this.isV2Crypto()) { - return this.decryptV2({ tag, iv, secret, tokens }); + return this.decryptV2(tokens); } else { - return this.decryptV1({ tag, iv, secret }); + return this.decryptV1(tokens); } } @@ -345,15 +333,17 @@ export class Crypto extends AsyncOptionalCreatable { }); } - private decryptV1({ tag, iv, secret }: DecryptConfigV1): Optional { + private decryptV1(tokens: string[]): Optional { + const tag = tokens[1]; + const iv = tokens[0].substring(0, IV_BYTES.v1 * 2); + const secret = tokens[0].substring(IV_BYTES.v1 * 2, tokens[0].length); + return this.key.value((buffer: Buffer) => { const decipher = crypto.createDecipheriv(ALGO, buffer.toString('utf8'), iv); - let dec; try { decipher.setAuthTag(Buffer.from(tag, 'hex')); - dec = decipher.update(secret, 'hex', 'utf8'); - dec += decipher.final('utf8'); + return `${decipher.update(secret, 'hex', 'utf8')}${decipher.final('utf8')}`; } catch (err) { const error = messages.createError('authDecryptError', [(err as Error).message], [], err as Error); const useGenericUnixKeychain = @@ -363,11 +353,14 @@ export class Crypto extends AsyncOptionalCreatable { } throw error; } - return dec; }); } - private decryptV2({ tag, iv, secret, tokens }: DecryptConfigV2): Optional { + private decryptV2(tokens: string[]): Optional { + const tag = tokens[1]; + const iv = tokens[0].substring(0, IV_BYTES.v2 * 2); + const secret = tokens[0].substring(IV_BYTES.v2 * 2, tokens[0].length); + return this.key.value((buffer: Buffer) => { let decipher = crypto.createDecipheriv(ALGO, buffer, Buffer.from(iv, 'hex')); @@ -391,8 +384,8 @@ export class Crypto extends AsyncOptionalCreatable { if (this.v1KeyLength && err?.message === 'Unsupported state or unable to authenticate data') { getCryptoLogger().debug('v2 decryption failed so trying v1 decryption'); try { - const ivLegacy = tokens[0].substring(0, IV_BYTES.v2); - const secretLegacy = tokens[0].substring(IV_BYTES.v2, tokens[0].length); + const ivLegacy = tokens[0].substring(0, IV_BYTES.v1 * 2); + const secretLegacy = tokens[0].substring(IV_BYTES.v1 * 2, tokens[0].length); // v1 encryption uses a utf8 encoded string from the buffer decipher = crypto.createDecipheriv(ALGO, buffer.toString('utf8'), ivLegacy); decipher.setAuthTag(Buffer.from(tag, 'hex')); From aaa7aad4f98956e551e0ac4feeba545bdc5deab2 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Wed, 31 Jan 2024 11:34:45 -0700 Subject: [PATCH 4/9] chore: lint fixes --- test/unit/crypto/cryptoTest.ts | 352 +++++++++++++++++++-------------- 1 file changed, 209 insertions(+), 143 deletions(-) diff --git a/test/unit/crypto/cryptoTest.ts b/test/unit/crypto/cryptoTest.ts index 461c018e4f..2f2736fb3d 100644 --- a/test/unit/crypto/cryptoTest.ts +++ b/test/unit/crypto/cryptoTest.ts @@ -14,14 +14,32 @@ import { SfError } from '../../../src/sfError'; import { Messages } from '../../../src/messages'; import { TestContext, shouldThrowSync } from '../../../src/testSetup'; -const TEST_KEY = { +const TEST_KEY_V1 = { service: 'sfdx', account: 'local', key: '8e8fd1e6dc06a37bf420898dbc3ee35c', }; -describe('CryptoTest', function () { +const TEST_KEY_V2 = { + service: 'sfdx', + account: 'local', + key: 'f699321865468a386434e78b3a4a5cf3b151e09a4391741e06fdbe8272ac07c0', +}; + +const setCryptoVersionEnvVar = (envVarValue?: boolean) => { + if (envVarValue === true) { + process.env.SF_CRYPTO_V2 = 'true'; + } else if (envVarValue === false) { + process.env.SF_CRYPTO_V2 = 'false'; + } else { + delete process.env.SF_CRYPTO_V2; + } +}; + +describe('CryptoTests', function () { + // Save env var original state const disableEncryptionEnvVar = process.env.SFDX_DISABLE_ENCRYPTION; + const cryptoVersionEnvVar = process.env.SF_CRYPTO_V2; let crypto: Crypto; const $$ = new TestContext(); @@ -29,163 +47,211 @@ describe('CryptoTest', function () { beforeEach(() => { // Testing crypto functionality, so restore global stubs. $$.SANDBOXES.CRYPTO.restore(); - - stubMethod($$.SANDBOX, Crypto.prototype, 'getKeyChain').callsFake(() => - Promise.resolve({ - setPassword: () => Promise.resolve(), - getPassword: (data: unknown, cb: (arg1: undefined, arg2: string) => {}) => cb(undefined, TEST_KEY.key), - }) - ); }); afterEach(() => { crypto.close(); - process.env.SFDX_DISABLE_ENCRYPTION = disableEncryptionEnvVar ?? ''; }); - if (process.platform === 'darwin') { - this.timeout(3 * 60 * 1000); + after(() => { + // Reset env vars to original state + if (disableEncryptionEnvVar) { + process.env.SFDX_DISABLE_ENCRYPTION = disableEncryptionEnvVar; + } else { + delete process.env.SFDX_DISABLE_ENCRYPTION; + } + + if (cryptoVersionEnvVar) { + process.env.SF_CRYPTO_V2 = cryptoVersionEnvVar; + } else { + delete process.env.SF_CRYPTO_V2; + } + }); + + const runTests = ({ keyVersion, envVarValue }: { keyVersion: 'v1' | 'v2'; envVarValue?: boolean }) => { + if (process.platform === 'darwin') { + this.timeout(3 * 60 * 1000); - const text = 'Unencrypted text'; - let secret: string | undefined; + const text = 'Unencrypted text'; + let secret: string | undefined; - it('Should have encrypted the string.', async () => { - process.env.SFDX_DISABLE_ENCRYPTION = 'false'; + const key = keyVersion === 'v2' ? TEST_KEY_V2.key : TEST_KEY_V1.key; - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - secret = crypto.encrypt(text); - expect(secret).to.not.equal(text); - }); + beforeEach(() => { + stubMethod($$.SANDBOX, Crypto.prototype, 'getKeyChain').callsFake(() => + Promise.resolve({ + setPassword: () => Promise.resolve(), + getPassword: (data: unknown, cb: (arg1: undefined, arg2: string) => {}) => cb(undefined, key), + }) + ); + stubMethod($$.SANDBOX, Crypto.prototype, 'getCryptoVersion').returns(envVarValue ? 'v2' : 'v1'); + }); - it('Should have decrypted the string', async () => { - process.env.SFDX_DISABLE_ENCRYPTION = 'false'; + it('Should have encrypted the string.', async () => { + setCryptoVersionEnvVar(envVarValue); + process.env.SFDX_DISABLE_ENCRYPTION = 'false'; - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - if (!secret) throw new Error('secret is undefined'); - const decrypted = crypto.decrypt(secret); - expect(decrypted).to.equal(text); - }); + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + secret = crypto.encrypt(text); + expect(secret).to.not.equal(text); + }); - it('Should have encrypted the string even if SFDX_DISABLE_ENCRYPTION is true.', async () => { - process.env.SFDX_DISABLE_ENCRYPTION = 'true'; + it('Should have decrypted the string', async () => { + setCryptoVersionEnvVar(envVarValue); + process.env.SFDX_DISABLE_ENCRYPTION = 'false'; - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - secret = crypto.encrypt(text); - expect(secret).to.not.equal(text); - }); + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + if (!secret) throw new Error('secret is undefined'); + const decrypted = crypto.decrypt(secret); + expect(decrypted).to.equal(text); + }); - it('Should have encrypted the string because SFDX_DISABLE_ENCRYPTION is not defined.', async () => { - delete process.env.SFDX_DISABLE_ENCRYPTION; + it('Should have encrypted the string even if SFDX_DISABLE_ENCRYPTION is true.', async () => { + setCryptoVersionEnvVar(envVarValue); + process.env.SFDX_DISABLE_ENCRYPTION = 'true'; - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - secret = crypto.encrypt(text); - expect(secret).to.not.equal(text); - }); - - it('Should have decrypted the string even if SFDX_DISABLE_ENCRYPTION is "true"', async () => { - process.env.SFDX_DISABLE_ENCRYPTION = 'true'; - - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - const str = '123456'; - const encrypted = crypto.encrypt(str); - const decrypted = crypto.decrypt(encrypted); - expect(encrypted).to.not.equal(str); - expect(decrypted).to.equal(str); - }); - - it('InvalidEncryptedFormatError action', async () => { - process.env.SFDX_DISABLE_ENCRYPTION = 'false'; - - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - expect(Crypto.prototype.decrypt.bind(crypto, 'foo')).to.throw(Error).and.have.property('actions'); - }); - - it('InvalidEncryptedFormatError name', async () => { - process.env.SFDX_DISABLE_ENCRYPTION = 'false'; - - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - expect(Crypto.prototype.decrypt.bind(crypto, '')) - .to.throw(Error) - .and.have.property('name', 'InvalidEncryptedFormatError'); - }); - - it('Should return null if text is null.', async () => { - delete process.env.SFDX_DISABLE_ENCRYPTION; + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + secret = crypto.encrypt(text); + expect(secret).to.not.equal(text); + }); - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - // @ts-expect-error -> null cannot be assigned to string - secret = crypto.encrypt(null); - expect(secret).to.equal(undefined); - }); + it('Should have encrypted the string because SFDX_DISABLE_ENCRYPTION is not defined.', async () => { + setCryptoVersionEnvVar(envVarValue); + delete process.env.SFDX_DISABLE_ENCRYPTION; - it('Should return null if text is undefined.', async () => { - delete process.env.SFDX_DISABLE_ENCRYPTION; + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + secret = crypto.encrypt(text); + expect(secret).to.not.equal(text); + }); - crypto = new Crypto(); - // @ts-expect-error: access protected method - await crypto.init(); - // @ts-expect-error: falsy value - secret = crypto.encrypt(undefined); - expect(secret).to.equal(undefined); - }); - - it('Decrypt should fail without env var, and add extra message', async () => { - const message = Messages.loadMessages('@salesforce/core', 'encryption').getMessage('macKeychainOutOfSync'); - const err = Error('Failed to decipher auth data. reason: Unsupported state or unable to authenticate data.'); - const sfdxErr = SfError.wrap(err); - sfdxErr.actions = []; - sfdxErr.actions[0] = message; - stubMethod($$.SANDBOX, os, 'platform').returns('darwin'); - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - $$.SANDBOX.stub(crypto, 'decrypt').throws(sfdxErr); - expect(() => crypto.decrypt('abcdefghijklmnopqrstuvwxyz:123456789')).to.throw( - 'Failed to decipher auth data. reason: Unsupported state or unable to authenticate data.' - ); - try { - shouldThrowSync(() => crypto.decrypt('abcdefghijklmnopqrstuvwxyz:123456789')); - } catch (error) { - const sfError = error as SfError; - if (!sfError.actions) throw new Error('sfError.actions is undefined'); - expect(sfError.actions[0]).to.equal(message); - } - }); - - it('Decrypt should fail but not add extra message with env var', async () => { - process.env.SF_USE_GENERIC_UNIX_KEYCHAIN = 'false'; - const message: string = Messages.loadMessages('@salesforce/core', 'encryption').getMessage('authDecryptError'); - const errorMessage: object = SfError.wrap(new Error(message)); - stubMethod($$.SANDBOX, os, 'platform').returns('darwin'); - stubMethod($$.SANDBOX, crypto, 'decrypt').callsFake(() => ({ - setAuthTag: () => { - throw errorMessage; - }, - update: () => {}, - final: () => {}, - })); - crypto = new Crypto(); - // @ts-expect-error -> init is protected - await crypto.init(); - // @ts-expect-error: secret is not a string - expect(() => crypto.decrypt(secret)).to.not.throw(message); - delete process.env.SF_USE_GENERIC_UNIX_KEYCHAIN; - }); - } + it('Should have decrypted the string even if SFDX_DISABLE_ENCRYPTION is "true"', async () => { + setCryptoVersionEnvVar(envVarValue); + process.env.SFDX_DISABLE_ENCRYPTION = 'true'; + + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + const str = '123456'; + const encrypted = crypto.encrypt(str); + const decrypted = crypto.decrypt(encrypted); + expect(encrypted).to.not.equal(str); + expect(decrypted).to.equal(str); + }); + + it('InvalidEncryptedFormatError action', async () => { + setCryptoVersionEnvVar(envVarValue); + process.env.SFDX_DISABLE_ENCRYPTION = 'false'; + + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + expect(Crypto.prototype.decrypt.bind(crypto, 'foo')).to.throw(Error).and.have.property('actions'); + }); + + it('InvalidEncryptedFormatError name', async () => { + setCryptoVersionEnvVar(envVarValue); + process.env.SFDX_DISABLE_ENCRYPTION = 'false'; + + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + expect(Crypto.prototype.decrypt.bind(crypto, '')) + .to.throw(Error) + .and.have.property('name', 'InvalidEncryptedFormatError'); + }); + + it('Should return null if text is null.', async () => { + setCryptoVersionEnvVar(envVarValue); + delete process.env.SFDX_DISABLE_ENCRYPTION; + + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + // @ts-expect-error -> null cannot be assigned to string + secret = crypto.encrypt(null); + expect(secret).to.equal(undefined); + }); + + it('Should return null if text is undefined.', async () => { + setCryptoVersionEnvVar(envVarValue); + delete process.env.SFDX_DISABLE_ENCRYPTION; + + crypto = new Crypto(); + // @ts-expect-error: access protected method + await crypto.init(); + // @ts-expect-error: falsy value + secret = crypto.encrypt(undefined); + expect(secret).to.equal(undefined); + }); + + it('Decrypt should fail without env var, and add extra message', async () => { + setCryptoVersionEnvVar(envVarValue); + const message = Messages.loadMessages('@salesforce/core', 'encryption').getMessage('macKeychainOutOfSync'); + const err = Error('Failed to decipher auth data. reason: Unsupported state or unable to authenticate data.'); + const sfdxErr = SfError.wrap(err); + sfdxErr.actions = []; + sfdxErr.actions[0] = message; + stubMethod($$.SANDBOX, os, 'platform').returns('darwin'); + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + $$.SANDBOX.stub(crypto, 'decrypt').throws(sfdxErr); + expect(() => crypto.decrypt('abcdefghijklmnopqrstuvwxyz:123456789')).to.throw( + 'Failed to decipher auth data. reason: Unsupported state or unable to authenticate data.' + ); + try { + shouldThrowSync(() => crypto.decrypt('abcdefghijklmnopqrstuvwxyz:123456789')); + } catch (error) { + const sfError = error as SfError; + if (!sfError.actions) throw new Error('sfError.actions is undefined'); + expect(sfError.actions[0]).to.equal(message); + } + }); + + it('Decrypt should fail but not add extra message with env var', async () => { + setCryptoVersionEnvVar(envVarValue); + process.env.SF_USE_GENERIC_UNIX_KEYCHAIN = 'false'; + const message: string = Messages.loadMessages('@salesforce/core', 'encryption').getMessage('authDecryptError'); + const errorMessage: object = SfError.wrap(new Error(message)); + stubMethod($$.SANDBOX, os, 'platform').returns('darwin'); + stubMethod($$.SANDBOX, crypto, 'decrypt').callsFake(() => ({ + setAuthTag: () => { + throw errorMessage; + }, + update: () => {}, + final: () => {}, + })); + crypto = new Crypto(); + // @ts-expect-error -> init is protected + await crypto.init(); + // @ts-expect-error: secret is not a string + expect(() => crypto.decrypt(secret)).to.not.throw(message); + delete process.env.SF_USE_GENERIC_UNIX_KEYCHAIN; + }); + } + }; + + describe('v1 crypto with v1 key, without env var', () => { + runTests({ keyVersion: 'v1' }); + }); + + describe('v1 crypto with v1 key, with env var', () => { + runTests({ keyVersion: 'v1', envVarValue: false }); + }); + + describe('v2 crypto with v1 key', () => { + runTests({ keyVersion: 'v1', envVarValue: true }); + }); + + describe('v2 crypto with v2 key', () => { + runTests({ keyVersion: 'v2', envVarValue: true }); + }); }); From 6bdb8c0906ddebd79c73f26a9809155e1e865a88 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Wed, 31 Jan 2024 15:14:01 -0700 Subject: [PATCH 5/9] feat: add some comments --- src/crypto/crypto.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/crypto/crypto.ts b/src/crypto/crypto.ts index 30feea1cb1..9e3141f44f 100644 --- a/src/crypto/crypto.ts +++ b/src/crypto/crypto.ts @@ -358,6 +358,7 @@ export class Crypto extends AsyncOptionalCreatable { private decryptV2(tokens: string[]): Optional { const tag = tokens[1]; + // assume the iv is v2 length (12 bytes) when parsing. const iv = tokens[0].substring(0, IV_BYTES.v2 * 2); const secret = tokens[0].substring(IV_BYTES.v2 * 2, tokens[0].length); @@ -381,6 +382,8 @@ export class Crypto extends AsyncOptionalCreatable { throw error; }; + // This happens when the iv is v1 length (6 bytes), and the key is v1 length. + // We re-parse the tokens for iv and secret based on that length. if (this.v1KeyLength && err?.message === 'Unsupported state or unable to authenticate data') { getCryptoLogger().debug('v2 decryption failed so trying v1 decryption'); try { From 032ce663e53e8c9f5ad4d764ef3fa5c6ee540c5f Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Tue, 6 Feb 2024 17:07:51 -0700 Subject: [PATCH 6/9] fix: detect crypto version from key length --- messages/encryption.md | 13 +++ src/crypto/crypto.ts | 152 ++++++++++++++++++--------------- test/unit/crypto/cryptoTest.ts | 19 +++-- 3 files changed, 108 insertions(+), 76 deletions(-) diff --git a/messages/encryption.md b/messages/encryption.md index e35967262f..5bfce3ddbc 100644 --- a/messages/encryption.md +++ b/messages/encryption.md @@ -83,3 +83,16 @@ Command failed with response: # macKeychainOutOfSync We’ve encountered an error with the Mac keychain being out of sync with your `sfdx` credentials. To fix the problem, sync your credentials by authenticating into your org again using the auth commands. + +# v1CryptoWithV2KeyWarning + +The SF_CRYPTO_V2 environment variable was set to "false" but a v2 crypto key was detected. v1 crypto can only be used with a v1 key. Unset the SF_CRYPTO_V2 environment variable. + +# v2CryptoWithV1KeyWarning + +SF_CRYPTO_V2 was set to "true" but a v1 crypto key was detected. v2 crypto can only be used with a v2 key. To generate a v2 key: + +1. Logout of all orgs: `sf org logout --all` +2. Delete the sfdx keychain entry (account: local, service: sfdx). If `SF_USE_GENERIC_UNIX_KEYCHAIN=true` env var is set, you can delete the `key.json` file. +3. Set `SF_CRYPTO_V2=true` env var. +4. Re-Authenticate with your orgs using the CLI org login commands. diff --git a/src/crypto/crypto.ts b/src/crypto/crypto.ts index 9e3141f44f..c33eb3e75c 100644 --- a/src/crypto/crypto.ts +++ b/src/crypto/crypto.ts @@ -48,21 +48,63 @@ const getCryptoLogger = (): Logger => { return cryptoLogger; }; +type CryptoV2Value = 'true' | 'false' | undefined; +const getCryptoV2EnvVar = (): CryptoV2Value => { + let sfCryptoV2 = process.env.SF_CRYPTO_V2?.toLowerCase(); + if (sfCryptoV2 !== undefined) { + getCryptoLogger().debug(`SF_CRYPTO_V2=${sfCryptoV2}`); + + // normalize all values that aren't "true" to be "false" + if (sfCryptoV2 !== 'true') { + sfCryptoV2 = 'false'; + } + } + return sfCryptoV2 as CryptoV2Value; +}; + type CryptoEncoding = 'utf8' | 'hex'; type CryptoVersion = keyof typeof IV_BYTES; let cryptoVersion: CryptoVersion; const getCryptoVersion = (): CryptoVersion => { if (!cryptoVersion) { - cryptoVersion = env.getBoolean('SF_CRYPTO_V2') ? 'v2' : 'v1'; - getCryptoLogger().debug(`Using ${cryptoVersion} Crypto`); + // This only happens when generating a new key, so use the env var + // and (for now) default to 'v1'. + cryptoVersion = getCryptoV2EnvVar() === 'true' ? 'v2' : 'v1'; + } + return cryptoVersion; +}; + +// Detect the crypto version based on the password (key) length. +// This happens once per process. +const detectCryptoVersion = (pwd?: string): void => { + if (!cryptoVersion) { + // check the env var to see if it's set + const sfCryptoV2 = getCryptoV2EnvVar(); + + // Password length of 64 is v2 crypto and uses hex encoding. + // Password length of 32 is v1 crypto and uses utf8 encoding. + if (pwd?.length === KEY_SIZE.v2 * 2) { + cryptoVersion = 'v2'; + getCryptoLogger().debug('Using v2 crypto'); + if (sfCryptoV2 === 'false') { + getCryptoLogger().warn(messages.getMessage('v1CryptoWithV2KeyWarning')); + } + } else if (pwd?.length === KEY_SIZE.v1 * 2) { + cryptoVersion = 'v1'; + getCryptoLogger().debug('Using v1 crypto'); + if (sfCryptoV2 === 'true') { + getCryptoLogger().warn(messages.getMessage('v2CryptoWithV1KeyWarning')); + } + } + void Lifecycle.getInstance().emitTelemetry({ eventName: 'crypto_version', library: 'sfdx-core', - function: 'getCryptoVersion', - cryptoVersion, + function: 'detectCryptoVersion', + cryptoVersion, // 'v1' or 'v2' + cryptoEnvVar: sfCryptoV2, // 'true' or 'false' or 'undefined' }); } - return cryptoVersion; }; Messages.importMessagesDirectory(pathJoin(__dirname)); @@ -90,18 +132,22 @@ const keychainPromises = { * @param service The keychain service name. * @param account The keychain account name. */ - getPassword(_keychain: KeyChain, service: string, account: string, encoding: CryptoEncoding): Promise { + getPassword(_keychain: KeyChain, service: string, account: string): Promise { const cacheKey = `${Global.DIR}:${service}:${account}`; const sb = Cache.get>(cacheKey); if (!sb) { return new Promise((resolve, reject): {} => _keychain.getPassword({ service, account }, (err: Nullable, password?: string) => { if (err) return reject(err); - Cache.set(cacheKey, makeSecureBuffer(password, encoding)); + detectCryptoVersion(password); + Cache.set(cacheKey, makeSecureBuffer(password, ENCODING[getCryptoVersion()])); return resolve({ username: account, password: ensure(password) }); }) ); } else { + // If the password is cached, we know the crypto version and encoding because it was + // detected by the non-cache code path just above this. + const encoding = ENCODING[getCryptoVersion()]; const pw = sb.value((buffer) => buffer.toString(encoding)); Cache.set(cacheKey, makeSecureBuffer(pw, encoding)); return new Promise((resolve): void => resolve({ username: account, password: ensure(pw) })); @@ -143,13 +189,6 @@ export class Crypto extends AsyncOptionalCreatable { private noResetOnClose!: boolean; - // `true` when the key is 32 hex chars, which is a key created by v1 crypto. - // This is used to determine encoding (utf8/hex), and for v2 crypto to retry - // using v1 crypto. - private v1KeyLength = false; - - private cryptoVersion: CryptoVersion; - /** * Constructor * **Do not directly construct instances of this class -- use {@link Crypto.create} instead.** @@ -160,7 +199,13 @@ export class Crypto extends AsyncOptionalCreatable { public constructor(options?: CryptoOptions) { super(options); this.options = options ?? {}; - this.cryptoVersion = this.getCryptoVersion(); + } + + // @ts-expect-error only for test access + // eslint-disable-next-line class-methods-use-this + private static unsetCryptoVersion(): void { + // @ts-expect-error only for test access + cryptoVersion = undefined; } /** @@ -234,7 +279,7 @@ export class Crypto extends AsyncOptionalCreatable { const value = tokens[0]; return ( tag.length === AUTH_TAG_LENGTH && - value.length >= IV_BYTES[this.getCryptoVersion()] && + value.length >= IV_BYTES[getCryptoVersion()] && ENCRYPTED_CHARS.test(tag) && ENCRYPTED_CHARS.test(tokens[0]) ); @@ -249,8 +294,9 @@ export class Crypto extends AsyncOptionalCreatable { } } + // eslint-disable-next-line class-methods-use-this public isV2Crypto(): boolean { - return this.getCryptoVersion() === 'v2'; + return getCryptoVersion() === 'v2'; } /** @@ -261,22 +307,13 @@ export class Crypto extends AsyncOptionalCreatable { this.options.platform = os.platform(); } - getCryptoLogger().debug(`retryStatus: ${this.options.retryStatus}`); - this.noResetOnClose = !!this.options.noResetOnClose; try { const keyChain = await this.getKeyChain(this.options.platform); - const pwd = (await keychainPromises.getPassword(keyChain, KEY_NAME, ACCOUNT, ENCODING[this.getCryptoVersion()])) - .password; - // This supports the v1 key size (32 hex chars) and the v2 key size (64 hex chars). - if (pwd.length === KEY_SIZE.v2 * 2) { - getCryptoLogger().debug('Detected v2 key size'); - } else { - getCryptoLogger().debug('Detected v1 key size'); - this.v1KeyLength = true; - } - this.key.consume(Buffer.from(pwd, this.v1KeyLength ? ENCODING.v1 : ENCODING.v2)); + const pwd = (await keychainPromises.getPassword(keyChain, KEY_NAME, ACCOUNT)).password; + // The above line ensures the crypto version is detected and set so we can rely on it now. + this.key.consume(Buffer.from(pwd, ENCODING[getCryptoVersion()])); } catch (err) { // No password found if ((err as Error).name === 'PasswordNotFoundError') { @@ -285,11 +322,15 @@ export class Crypto extends AsyncOptionalCreatable { getCryptoLogger().debug('a key was set but the retry to get the password failed.'); throw err; } else { - getCryptoLogger().debug('password not found in keychain. attempting to create one and re-init.'); + getCryptoLogger().debug( + `password not found in keychain. Creating new one (Crypto ${getCryptoVersion()}) and re-init.` + ); } - const key = crypto.randomBytes(KEY_SIZE[this.getCryptoVersion()]).toString('hex'); - // Create a new password in the KeyChain. + // 2/6/2024: This generates a new key using the crypto version based on the SF_CRYPTO_V2 env var. + // Sometime in the future we could hardcode this to be `KEY_SIZE.v2` so that it becomes the default. + const key = crypto.randomBytes(KEY_SIZE[getCryptoVersion()]).toString('hex'); + // Set the new password in the KeyChain. await keychainPromises.setPassword(ensure(this.options.keychain), KEY_NAME, ACCOUNT, key); return this.init(); @@ -299,11 +340,6 @@ export class Crypto extends AsyncOptionalCreatable { } } - // enables overriding the version in tests - private getCryptoVersion(): CryptoVersion { - return this.cryptoVersion ?? getCryptoVersion(); - } - private encryptV1(text: string): Optional { const iv = crypto.randomBytes(IV_BYTES.v1).toString('hex'); @@ -358,51 +394,25 @@ export class Crypto extends AsyncOptionalCreatable { private decryptV2(tokens: string[]): Optional { const tag = tokens[1]; - // assume the iv is v2 length (12 bytes) when parsing. const iv = tokens[0].substring(0, IV_BYTES.v2 * 2); const secret = tokens[0].substring(IV_BYTES.v2 * 2, tokens[0].length); return this.key.value((buffer: Buffer) => { - let decipher = crypto.createDecipheriv(ALGO, buffer, Buffer.from(iv, 'hex')); + const decipher = crypto.createDecipheriv(ALGO, buffer, Buffer.from(iv, 'hex')); - let dec!: string; try { decipher.setAuthTag(Buffer.from(tag, 'hex')); - dec = decipher.update(secret, 'hex', 'utf8'); - dec += decipher.final('utf8'); + return `${decipher.update(secret, 'hex', 'utf8')}${decipher.final('utf8')}`; } catch (_err: unknown) { const err = (isString(_err) ? SfError.wrap(_err) : _err) as Error; - const handleDecryptError = (decryptErr: Error): void => { - const error = messages.createError('authDecryptError', [decryptErr.message], [], decryptErr); - const useGenericUnixKeychain = - env.getBoolean('SF_USE_GENERIC_UNIX_KEYCHAIN') || env.getBoolean('USE_GENERIC_UNIX_KEYCHAIN'); - if (os.platform() === 'darwin' && !useGenericUnixKeychain) { - error.actions = [messages.getMessage('macKeychainOutOfSync')]; - } - throw error; - }; - - // This happens when the iv is v1 length (6 bytes), and the key is v1 length. - // We re-parse the tokens for iv and secret based on that length. - if (this.v1KeyLength && err?.message === 'Unsupported state or unable to authenticate data') { - getCryptoLogger().debug('v2 decryption failed so trying v1 decryption'); - try { - const ivLegacy = tokens[0].substring(0, IV_BYTES.v1 * 2); - const secretLegacy = tokens[0].substring(IV_BYTES.v1 * 2, tokens[0].length); - // v1 encryption uses a utf8 encoded string from the buffer - decipher = crypto.createDecipheriv(ALGO, buffer.toString('utf8'), ivLegacy); - decipher.setAuthTag(Buffer.from(tag, 'hex')); - dec = decipher.update(secretLegacy, 'hex', 'utf8'); - dec += decipher.final('utf8'); - } catch (_err2: unknown) { - const err2 = (isString(_err2) ? SfError.wrap(_err2) : _err2) as Error; - handleDecryptError(err2); - } - } else { - handleDecryptError(err); + const error = messages.createError('authDecryptError', [err.message], [], err); + const useGenericUnixKeychain = + env.getBoolean('SF_USE_GENERIC_UNIX_KEYCHAIN') || env.getBoolean('USE_GENERIC_UNIX_KEYCHAIN'); + if (os.platform() === 'darwin' && !useGenericUnixKeychain) { + error.actions = [messages.getMessage('macKeychainOutOfSync')]; } + throw error; } - return dec; }); } diff --git a/test/unit/crypto/cryptoTest.ts b/test/unit/crypto/cryptoTest.ts index 2f2736fb3d..3ad00a93bf 100644 --- a/test/unit/crypto/cryptoTest.ts +++ b/test/unit/crypto/cryptoTest.ts @@ -84,7 +84,8 @@ describe('CryptoTests', function () { getPassword: (data: unknown, cb: (arg1: undefined, arg2: string) => {}) => cb(undefined, key), }) ); - stubMethod($$.SANDBOX, Crypto.prototype, 'getCryptoVersion').returns(envVarValue ? 'v2' : 'v1'); + // @ts-expect-error Using a private static method for testing + Crypto.unsetCryptoVersion(); }); it('Should have encrypted the string.', async () => { @@ -239,19 +240,27 @@ describe('CryptoTests', function () { } }; - describe('v1 crypto with v1 key, without env var', () => { + describe('crypto with v1 key, no env var, does v1 crypto', () => { runTests({ keyVersion: 'v1' }); }); - describe('v1 crypto with v1 key, with env var', () => { + describe('crypto with v1 key, SF_CRYPTO_V2=false, does v1 crypto', () => { runTests({ keyVersion: 'v1', envVarValue: false }); }); - describe('v2 crypto with v1 key', () => { + describe('crypto with v1 key, SF_CRYPTO_V2=true, does v1 crypto', () => { runTests({ keyVersion: 'v1', envVarValue: true }); }); - describe('v2 crypto with v2 key', () => { + describe('crypto with v2 key, no env var, does v2 crypto', () => { + runTests({ keyVersion: 'v2' }); + }); + + describe('crypto with v2 key, SF_CRYPTO_V2=true, does v2 crypto', () => { runTests({ keyVersion: 'v2', envVarValue: true }); }); + + describe('crypto with v2 key, SF_CRYPTO_V2=false, does v2 crypto', () => { + runTests({ keyVersion: 'v2', envVarValue: false }); + }); }); From 6774418628e411e2ee48cd12e96bd8aceca6c6d7 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Tue, 6 Feb 2024 17:19:35 -0700 Subject: [PATCH 7/9] fix: use env var when key doesn't match v1 or v2 length --- src/crypto/crypto.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/crypto/crypto.ts b/src/crypto/crypto.ts index c33eb3e75c..59d222df7a 100644 --- a/src/crypto/crypto.ts +++ b/src/crypto/crypto.ts @@ -95,6 +95,9 @@ const detectCryptoVersion = (pwd?: string): void => { if (sfCryptoV2 === 'true') { getCryptoLogger().warn(messages.getMessage('v2CryptoWithV1KeyWarning')); } + } else { + getCryptoLogger().debug("crypto key doesn't match v1 or v2. using SF_CRYPTO_V2."); + getCryptoVersion(); } void Lifecycle.getInstance().emitTelemetry({ From 58761355f4fc175241e49edbf6fb8466c8e129f8 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Thu, 8 Feb 2024 10:36:47 -0700 Subject: [PATCH 8/9] chore: allow undefined for internal cryptoVersion --- src/crypto/crypto.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/crypto/crypto.ts b/src/crypto/crypto.ts index 59d222df7a..6f9ea14069 100644 --- a/src/crypto/crypto.ts +++ b/src/crypto/crypto.ts @@ -64,7 +64,7 @@ const getCryptoV2EnvVar = (): CryptoV2Value => { type CryptoEncoding = 'utf8' | 'hex'; type CryptoVersion = keyof typeof IV_BYTES; -let cryptoVersion: CryptoVersion; +let cryptoVersion: CryptoVersion | undefined; const getCryptoVersion = (): CryptoVersion => { if (!cryptoVersion) { // This only happens when generating a new key, so use the env var @@ -207,7 +207,6 @@ export class Crypto extends AsyncOptionalCreatable { // @ts-expect-error only for test access // eslint-disable-next-line class-methods-use-this private static unsetCryptoVersion(): void { - // @ts-expect-error only for test access cryptoVersion = undefined; } From 7b9d5931252367afaccab66069d953ef11a13100 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Wed, 21 Feb 2024 15:24:34 -0700 Subject: [PATCH 9/9] fix: better typing for makeSecureBuffer --- src/crypto/crypto.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/crypto/crypto.ts b/src/crypto/crypto.ts index 6f9ea14069..7cdb8774e4 100644 --- a/src/crypto/crypto.ts +++ b/src/crypto/crypto.ts @@ -118,9 +118,9 @@ interface CredType { password: string; } -const makeSecureBuffer = (password: string | undefined, encoding: CryptoEncoding): SecureBuffer => { +const makeSecureBuffer = (password: string, encoding: CryptoEncoding): SecureBuffer => { const newSb = new SecureBuffer(); - newSb.consume(Buffer.from(ensure(password), encoding)); + newSb.consume(Buffer.from(password, encoding)); return newSb; }; @@ -142,18 +142,22 @@ const keychainPromises = { return new Promise((resolve, reject): {} => _keychain.getPassword({ service, account }, (err: Nullable, password?: string) => { if (err) return reject(err); - detectCryptoVersion(password); - Cache.set(cacheKey, makeSecureBuffer(password, ENCODING[getCryptoVersion()])); - return resolve({ username: account, password: ensure(password) }); + const pwd = ensure(password, 'Expected the keychain password to be set'); + detectCryptoVersion(pwd); + Cache.set(cacheKey, makeSecureBuffer(pwd, ENCODING[getCryptoVersion()])); + return resolve({ username: account, password: pwd }); }) ); } else { // If the password is cached, we know the crypto version and encoding because it was // detected by the non-cache code path just above this. const encoding = ENCODING[getCryptoVersion()]; - const pw = sb.value((buffer) => buffer.toString(encoding)); - Cache.set(cacheKey, makeSecureBuffer(pw, encoding)); - return new Promise((resolve): void => resolve({ username: account, password: ensure(pw) })); + const pwd = ensure( + sb.value((buffer) => buffer.toString(encoding)), + 'Expected the keychain password to be set' + ); + Cache.set(cacheKey, makeSecureBuffer(pwd, encoding)); + return new Promise((resolve): void => resolve({ username: account, password: pwd })); } },