-
Notifications
You must be signed in to change notification settings - Fork 302
feat: opt-in v2 Argon2+HKDF encryption for multisig and MPC flows #8592
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
base: master
Are you sure you want to change the base?
Changes from all commits
b2082f7
799c04b
1beafcd
22bab61
7c1ef0e
7957f0d
36c78cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ export interface EncryptOptions { | |
| input: string; | ||
| password?: string; | ||
| adata?: string; | ||
| encryptionVersion?: 2; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Major — use a named alias. A bare literal export type EncryptionVersion = 1 | 2;
export interface EncryptOptions {
input: string;
password?: string;
adata?: string;
encryptionVersion?: EncryptionVersion;
}This prepares for v3 without a hunt-and-replace across the 8+ option types where the field appears ( |
||
| } | ||
|
|
||
| export interface GetSharingKeyOptions { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,12 @@ export interface BitGoBase { | |
| decryptKeys(params: DecryptKeysOptions): string[]; | ||
| del(url: string): BitGoRequest; | ||
| encrypt(params: EncryptOptions): string; | ||
| encryptAsync(params: EncryptOptions): Promise<string>; | ||
| createEncryptionSession(password: string): Promise<{ | ||
| encrypt(plaintext: string): Promise<string>; | ||
| decrypt(ciphertext: string): Promise<string>; | ||
| destroy(): void; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Major — extract a named Without this, no external SDK consumer can write |
||
| }>; | ||
| readonly env: EnvironmentName; | ||
| fetchConstants(): Promise<any>; | ||
| get(url: string): BitGoRequest; | ||
|
|
||
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.
Major —
params.adatais silently dropped on the v2 path. v1 forwardsadatatosjcl.encrypt; v2 callsencryptV2(password, input)with no AAD. Two consequences:adataget false confidence that AAD is bound — it is not.ecdsaMPCv2.ts, the v1 path usesadata = ${hashBuffer}:${derivationPath}to bind the encrypted round-session to the transaction context (validateAdataenforces the binding). The v2 path has no equivalent (see inline comment onecdsaMPCv2.tsline 1242).v2's AES-GCM is self-authenticating for the ciphertext — but doesn't replicate the transaction-context binding unless AAD is threaded through
encryptV2envelopes. Fix: add anadditionalData?: stringoption toencryptV2/aesGcmEncrypt, store it in the v2 envelope, and verify indecryptV2. Or, at minimum, throw ifparams.adatais set andencryptionVersion === 2so the silent drop becomes a loud error.