-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Encrypted keystore #6232
base: v-next
Are you sure you want to change the base?
Encrypted keystore #6232
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
hardhatTotal size of the bundle: List of dependencies (sorted by size)
|
@@ -706,25 +706,12 @@ Try using another mnemonic or deriving less keys.`, | |||
}, | |||
}, | |||
KEYSTORE: { | |||
INVALID_KEYSTORE_FILE_FORMAT: { |
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.
Removed because it is no longer needed, as user input is now handled via the requestSecretInput
hook.
@@ -39,7 +41,16 @@ export default async (): Promise<Partial<ConfigurationVariableHooks>> => { | |||
return next(context, variable); | |||
} | |||
|
|||
return keystore.readValue(variable.name); | |||
const password = await askPassword( | |||
context.interruptions.requestSecretInput.bind(context.interruptions), |
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.
bind
is needed because requestSecretInput
is a method of hre.interruptions
, and when passed as an argument, it loses its original this context. Since method calls rely on the object reference for this, passing it as a standalone function removes that association. To ensure it retains the correct context, we need to explicitly bind this using .bind(hre.interruptions)
.
This approach will be used also later in other files
password: string; | ||
salt: Uint8Array; | ||
}): Uint8Array { | ||
password = password.normalize(PASSWORD_NORMALIZATION_FORM); |
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.
I need a double check that the normalization
is only required here
@@ -0,0 +1,630 @@ | |||
import { siv } from "@noble/ciphers/aes"; |
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.
We need a decision on this comment
return this.#keystoreData; | ||
} | ||
|
||
public async listKeys(): Promise<string[]> { |
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.
When listing the keys, HMAC file validation
is not performed. Is this acceptable, or do we want to validate it? If we choose to validate, we will need to request the password. I suggest keeping it as is, without validation.
) => Promise<string>, | ||
consoleLog: (text: string) => void = console.log, | ||
): Promise<string> { | ||
const PASSWORD_REGEX = /^.{8,}$/; |
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.
Currently, we only require a minimum of 8 characters. Do we want to introduce additional rules?
): Promise<string> { | ||
const PASSWORD_REGEX = /^.{8,}$/; | ||
|
||
consoleLog(UserDisplayMessages.keystoreBannerMessage()); |
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.
} | ||
|
||
let confirmPassword: string | undefined; | ||
while (confirmPassword === undefined) { |
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.
This loop will continue indefinitely until the confirmation password matches the original password.
This check ensures that the user saves the correct password, preventing spelling mistakes.
@@ -43,23 +34,28 @@ export class KeystoreFileLoader implements KeystoreLoader { | |||
this.#keystoreFilePath, | |||
); | |||
|
|||
this.#throwIfInvalidKeystoreFormat(keystoreFile); |
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.
The keystore validation file has been removed since HMAC validation
now handles the validation process.
}); | ||
}); | ||
|
||
describe("password normalization", () => { |
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.
I added this additional test to validate that the normalization is working
For a list of follow-up tasks related to user experience improvements (not security improvements), see the parent issue here
QUESTIONS:
requestSecretInput
fromhre.interruptions
, but outputs are not currently managed byhre.interruptions
. I believe they should be. Should we address this in this PR, or should I add it to the follow-up tasks?