Skip to content
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.

Sign up for GitHub

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

Throw a more informative error on invalid keystore #4022

Merged
merged 5 commits into from
May 19, 2022
Merged

Throw a more informative error on invalid keystore #4022

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
072e28a
Throw a more informative error on invalid keystore
wemeetagain May 16, 2022
3caf052
Make error more descriptive
wemeetagain May 16, 2022
761b00e
Use template string
wemeetagain May 17, 2022
76fef1c
Update keys.ts
dapplion May 18, 2022
3af0e72
Update keys.ts
dapplion May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions packages/cli/src/cmds/validator/keys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,15 @@ export async function getLocalSecretKeys(
}

const secretKeys = await Promise.all(
keystorePaths.map(async (keystorePath) =>
SecretKey.fromBytes(await Keystore.parse(fs.readFileSync(keystorePath, "utf8")).decrypt(passphrase))
)
keystorePaths.map(async (keystorePath) => {
let keystore;
try {
keystore = Keystore.parse(fs.readFileSync(keystorePath, "utf8"));
} catch (e) {
throw new Error("Error parsing keystore at " + keystorePath + ": " + (e as Error).message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess since the key store file does not contain any decrypted secrets, it should be fine to log the location (we recently had a PR that removed similar logging here

Suggestion: consider using template literals rather than concatenation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We removed the path of the passphrase from the logs since its more sensitive?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We removed the path of the passphrase from the logs since its more sensitive?

Yeah. The advisory that it triggered can be seen here

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wemeetagain changed to only wrap the .parse step, not the file read. Also extended error to preserve stack trace

return SecretKey.fromBytes(await keystore.decrypt(passphrase));
})
);

return {
Expand Down