Optimize UX loading for keystores #5043
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected Full benchmark results
|
nazarhussain
force-pushed
the
nh/4179-key-cache
branch
from
f70f720
to
8ed2b04
Compare
nflaig
reviewed
Feb 7, 2023
| const password = passwords.join(""); | ||
| // We can't use Keystore.parse as it validates the `encrypted message` to be only 32 bytes. | ||
| const keystore = new Keystore(JSON.parse(fs.readFileSync(cacheFilepath, "utf8"))); | ||
| const secretKeyConcatenatedBytes = await keystore.decrypt(password); |
There was a problem hiding this comment.
What's the point of encrypting the keystore at all if we store the passwords in the same place? I assume the decrypting still takes a long time if the keystore is big but probably still better then decryption every single key separately as it was done before
It does not seem ideal to me that we store passwords in plain text at all which is currently done by Lodestar as we have a keystore and a secrets (with passwords) file. Those files are stored on the same device (and even the same folder) so it pointless in terms of security.
In my opinion, we should never store the password, only the first time the user imports a encrypted keystore the password should be interactively provided through the cli or keymanager API. After keystores are imported they should be stored unecrypted which makes subsequently loading them much faster and we never store passwords in plain text.
There was a problem hiding this comment.
- Local keystores is a feature, which is consistent across all CL clients. As you mentioned it's not the best way in terms of security.
- To overcome security issue, there is a feature of remote key manager, which does not involve storing passing in local file.
It's upto user to use one feature or other. In regard to decryption, for cache decryption it will take time upto one keystore. So if we have 100 validators keys we can save upto 99% of time with the cache.
There was a problem hiding this comment.
That's what I figured, looks like all CLs implement it like this. We also quickly discussed this topic in the standup, I will create a separate issue for this to get some ideas how we could improve the current security model but it might just be the case that a remoter signer/key manager is the only solution.
So if we have 100 validators keys we can save upto 99% of time with the cache.
that's a great performance improvement, I was assuming it will be much faster but having constant time is really a big deal if you have a lot of validators
This reverts commit a5c78e5.
Co-authored-by: Nico Flaig <[email protected]>
nazarhussain
force-pushed
the
nh/4179-key-cache
branch
from
66e45db
to
86706bb
Compare
nflaig
previously approved these changes
Feb 7, 2023
| const password = passwords.join(""); | ||
| // We can't use Keystore.parse as it validates the `encrypted message` to be only 32 bytes. | ||
| const keystore = new Keystore(JSON.parse(fs.readFileSync(cacheFilepath, "utf8"))); | ||
| const secretKeyConcatenatedBytes = await keystore.decrypt(password); |
There was a problem hiding this comment.
That's what I figured, looks like all CLs implement it like this. We also quickly discussed this topic in the standup, I will create a separate issue for this to get some ideas how we could improve the current security model but it might just be the case that a remoter signer/key manager is the only solution.
So if we have 100 validators keys we can save upto 99% of time with the cache.
that's a great performance improvement, I was assuming it will be much faster but having constant time is really a big deal if you have a lot of validators
packages/cli/src/util/progress.ts
Outdated
| @@ -16,6 +16,7 @@ export function showProgress({ | |||
| let current = 0; | |||
| let last = 0; | |||
| let lastProcessTime: number = Date.now(); | |||
| let internalId: NodeJS.Timeout; | |||
There was a problem hiding this comment.
could use a more descriptive name for the interval, I think it will also not be just an ID but the whole Timeout object
There was a problem hiding this comment.
It's common convention among JS community to suffix output of setTimeout or setInterval as Ids because these are just the pointers to v8 internal timers.
There was a problem hiding this comment.
I changed the name to be more descriptive.
There was a problem hiding this comment.
It's common convention among JS community to suffix output of setTimeout or setInterval as Ids because these are just the pointers to v8 internal timers
gotcha, thanks for pointing that out, I didn't know about that
packages/cli/test/unit/cmds/validator/keymanager/keystoreCache.test.ts
Outdated
Show resolved
Hide resolved
| describe("writeKeystoreCache", () => { | ||
| it("should write a valid keystore cache file", async () => { | ||
| await expect(writeKeystoreCache(keystoreCacheFile, signers, passwords)).to.fulfilled; | ||
| expect(fs.existsSync(keystoreCacheFile)).to.be.true; |
There was a problem hiding this comment.
is there an easy way to do some basic sanitiy checks on the written file? I guess the proper check happens in the others test were the file is written and then loaded, so maybe should not bother too much here
There was a problem hiding this comment.
The authenticity of files could better be checked in e2e tests.
nflaig
approved these changes
Feb 7, 2023
wemeetagain
approved these changes
Feb 7, 2023
nflaig
reviewed
Feb 12, 2023
| return decryptKeystoreDefinitions(keystoreDefinitions, { | ||
| ...args, | ||
| onDecrypt: needle, | ||
| cacheFilePath: `${args.importKeystores[0]}.cache`, |
There was a problem hiding this comment.
@nazarhussain I noticed that the validator_keys.cache is stored in the same directory as my validator_keys folder which I set via --importKeystores. This is a not ideal in a containerized environment as the cache file is likely not mounted to the container or persistent in a volume. Maybe it would be better to store the cache file in the data directory (dataDir).
In our docker-compose.validator.yml file we also only mount the /keystores which would not include the cache file.
lodestar/docker-compose.validator.yml
Lines 4 to 11 in 2042c3b
This was referenced Feb 13, 2023
maschad
added a commit
to maschad/lodestar
that referenced
this pull request
Mar 16, 2023
…importing of validator keys (ChainSafe#5043)
|
🎉 This PR is included in v1.6.0 🎉 |
maschad
added a commit
to maschad/lodestar
that referenced
this pull request
Mar 26, 2023
…importing of validator keys (ChainSafe#5043)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Improve the UX for the keystores loading.
Description
Add key caching to the keystore loading logic.
Closes #4179
Steps to test or reproduce