-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat: support stronger ciphers #810
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments to get the thinking going.
src/crypto/crypto.ts
Outdated
@@ -54,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<CredType> { | |||
getPassword(_keychain: KeyChain, service: string, account: string, encoding: 'utf8' | 'hex'): Promise<CredType> { | |||
const cacheKey = `${Global.DIR}:${service}:${account}`; | |||
const sb = Cache.get<SecureBuffer<string>>(cacheKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache
here is an example of overengineering. We could just have a Map<string, SecureBuffer>
in this module.
But there's this whole other non-exported class only used by Crypto with methods that aren't being used and a whole test for it, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I can't tell if I'm wrong and it's necessary because of the weird returned promises structure here.
src/crypto/crypto.ts
Outdated
@@ -107,6 +151,13 @@ export class Crypto extends AsyncOptionalCreatable<CryptoOptions> { | |||
|
|||
private noResetOnClose!: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta: Crypto is never exported, so its only consumers would be here in core (only configStore).
I've not seen anyone create
it with the options. They're not even used in UT to inject stuff.
So I guess if we have to go through QAing this, it's probably the best time to remove all the unused bits?
QA notes: deleted the fresh start with v2 keybacked up mac (native keychain): ✅ auth to a hub with linux and windows (generic keychain) existing v1 key + auth files with crypto v2 enabled✅ sfdx-core users v1 crypto even with |
What does this PR do?
Supports stronger ciphers for encryption/decryption.
What issues does this PR fix or reference?
@W-12422652@
QA Notes
MAKE A BACKUP OF YOUR ~/.sfdx DIRECTORY BEFORE DOING ANYTHING. Use the backup to revert everything done during testing.
Link this library to plugin-auth and plugin-org.
Enable Crypto debug output.
export DEBUG=sf:crypto
andexport SF_LOG_LEVEL=trace
You'll probably want to set
SF_USE_GENERIC_UNIX_KEYCHAIN=true
for initial testing.Run commands from those plugins without SF_CRYPTO_V2 env var.
Run commands from those plugins with SF_CRYPTO_V2=false.
Run commands from those plugins with SF_CRYPTO_V2=true.
Move the ~/.sfdx dir so that a new key is generated, then:
Run commands from those plugins with SF_CRYPTO_V2=true.
See the doc in the WI for more usecases.