Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
89 changes: 89 additions & 0 deletions packages/cli/src/cmds/validator/keymanager/keystoreCache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import fs from "node:fs";
import bls from "@chainsafe/bls";
import {Keystore} from "@chainsafe/bls-keystore";
import {SignerLocal, SignerType} from "@lodestar/validator";
import {fromHex, toHex} from "@lodestar/utils";
import {PointFormat} from "@chainsafe/bls/types";
import {lockFilepath, unlockFilepath} from "../../../util/lockfile.js";
import {LocalKeystoreDefinition} from "./interface.js";

export async function loadKeystoreCache(
cacheFilepath: string,
keystoreDefinitions: LocalKeystoreDefinition[]
): Promise<SignerLocal[]> {
const keystores: Keystore[] = [];
const passwords: string[] = [];
for (const {keystorePath, password} of keystoreDefinitions) {
keystores.push(Keystore.parse(fs.readFileSync(keystorePath, "utf8")));
passwords.push(password);
}

if (keystores.length !== passwords.length) {
throw new Error(
`Number of keystores and passwords must be equal. keystores=${keystores.length}, passwords=${passwords.length}`
);
}

if (!fs.existsSync(cacheFilepath)) {
throw new Error(`Cache file ${cacheFilepath} does not exists.`);
}

lockFilepath(cacheFilepath);

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);

@nflaig nflaig Feb 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. 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.
  2. 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.

@nflaig nflaig Feb 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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


const result: SignerLocal[] = [];
for (const [index, k] of keystores.entries()) {
const secretKeyBytes = Uint8Array.prototype.slice.call(secretKeyConcatenatedBytes, index * 32, (index + 1) * 32);
const secretKey = bls.SecretKey.fromBytes(secretKeyBytes);
const publicKey = secretKey.toPublicKey().toBytes(PointFormat.compressed);

if (toHex(publicKey) !== toHex(fromHex(k.pubkey))) {
throw new Error(
`Keystore ${k.uuid} does not match the expected pubkey. expected=${toHex(fromHex(k.pubkey))}, found=${toHex(
publicKey
)}`
);
}

result.push({
type: SignerType.Local,
secretKey,
});
}

unlockFilepath(cacheFilepath);

return result;
}

export async function writeKeystoreCache(
cacheFilepath: string,
signers: SignerLocal[],
passwords: string[]
): Promise<void> {
if (signers.length !== passwords.length) {
throw new Error(
`Number of signers and passwords must be equal. signers=${signers.length}, passwords=${passwords.length}`
);
}
const secretKeys = signers.map((s) => s.secretKey.toBytes());
const publicKeys = signers.map((s) => s.secretKey.toPublicKey().toBytes());
const password = passwords.join("");
const secretKeyConcatenatedBytes = Buffer.concat(secretKeys);
const publicConcatenatedBytes = Buffer.concat(publicKeys);
const keystore = await Keystore.create(password, secretKeyConcatenatedBytes, publicConcatenatedBytes, cacheFilepath);
lockFilepath(cacheFilepath);
fs.writeFileSync(cacheFilepath, keystore.stringify());
unlockFilepath(cacheFilepath);
}

export async function clearKeystoreCache(cacheFilepath: string): Promise<void> {
if (fs.existsSync(cacheFilepath)) {
unlockFilepath(cacheFilepath);
fs.unlinkSync(cacheFilepath);
}
}
35 changes: 31 additions & 4 deletions packages/cli/src/cmds/validator/keymanager/persistedKeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import fs from "node:fs";
import path from "node:path";
import bls from "@chainsafe/bls";
import {Keystore} from "@chainsafe/bls-keystore";
import {Signer, SignerType, ProposerConfig} from "@lodestar/validator";
import {Signer, SignerType, ProposerConfig, SignerLocal} from "@lodestar/validator";
import {DeletionStatus, ImportStatus, PubkeyHex, SignerDefinition} from "@lodestar/api/keymanager";
import {
getPubkeyHexFromKeystore,
Expand All @@ -14,6 +14,7 @@ import {
} from "../../../util/index.js";
import {lockFilepath} from "../../../util/lockfile.js";
import {IPersistedKeysBackend} from "./interface.js";
import {clearKeystoreCache, loadKeystoreCache, writeKeystoreCache} from "./keystoreCache.js";

export {ImportStatus, DeletionStatus};

Expand Down Expand Up @@ -246,11 +247,32 @@ export class PersistedKeysBackend implements IPersistedKeysBackend {
}
}

type KeystoreDecryptOptions = {
force?: boolean;
onDecrypt?: (index: number, signer: Signer) => void;
// Try to use the cache file if it exists
cacheFilePath?: string;
};

export async function decryptKeystoreDefinitions(
keystoreDefinitions: LocalKeystoreDefinition[],
opts: {force?: boolean; onDecrypt?: (index: number, signer: Signer) => void}
opts: KeystoreDecryptOptions
): Promise<Signer[]> {
const signers: Signer[] = [];
if (opts.cacheFilePath) {
try {
const signers = await loadKeystoreCache(opts.cacheFilePath, keystoreDefinitions);
if (opts?.onDecrypt) {
opts?.onDecrypt(signers.length - 1, signers[signers.length - 1]);
}
return signers;
} catch {
// Some error loading the cache, ignore and invalidate cache
await clearKeystoreCache(opts.cacheFilePath);
}
}

const signers: SignerLocal[] = [];
const passwords: string[] = [];

for (const [index, {keystorePath, password}] of keystoreDefinitions.entries()) {
try {
Expand Down Expand Up @@ -281,18 +303,23 @@ export async function decryptKeystoreDefinitions(
//
const secretKeyBytes = await keystore.decrypt(password);

const signer: Signer = {
const signer: SignerLocal = {
type: SignerType.Local,
secretKey: bls.SecretKey.fromBytes(secretKeyBytes),
};

signers.push(signer);
passwords.push(password);

if (opts?.onDecrypt) {
opts?.onDecrypt(index, signer);
}
}

if (opts.cacheFilePath) {
await writeKeystoreCache(opts.cacheFilePath, signers, passwords);
}

return signers;
}

Expand Down
12 changes: 10 additions & 2 deletions packages/cli/src/cmds/validator/signers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ export async function getSignersFromArgs(
);
},
});
return await decryptKeystoreDefinitions(keystoreDefinitions, {...args, onDecrypt: needle});
return decryptKeystoreDefinitions(keystoreDefinitions, {
...args,
onDecrypt: needle,
cacheFilePath: `${args.importKeystores[0]}.cache`,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

image: chainsafe/lodestar:next
restart: always
volumes:
- validator:/data
- logs:/logs
- ./keystores:/keystores
env_file: .env
command: validator --dataDir /data --importKeystores /keystores --importKeystoresPassword /keystores/password.txt --server http://beacon_node:9596 --logFile /logs/validator.log --logFileLevel debug --logFileDailyRotate 5

});
}

// Remote keys declared manually with --externalSignerPublicKeys
Expand Down Expand Up @@ -119,7 +123,11 @@ export async function getSignersFromArgs(
);
},
});
const keystoreSigners = await decryptKeystoreDefinitions(keystoreDefinitions, {...args, onDecrypt: needle});
const keystoreSigners = await decryptKeystoreDefinitions(keystoreDefinitions, {
...args,
onDecrypt: needle,
cacheFilePath: `${accountPaths.keystoresDir}.cache`,
});

// Read local remote keys, imported via keymanager api
const signerDefinitions = persistedKeysBackend.readAllRemoteKeys();
Expand Down
11 changes: 7 additions & 4 deletions packages/cli/src/util/progress.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export function showProgress({
let current = 0;
let last = 0;
let lastProcessTime: number = Date.now();
let progressIntervalId: NodeJS.Timeout;

const needle: NeedleFunc = (needle: number) => {
// zero is considered first index in the range
Expand All @@ -34,21 +35,23 @@ export function showProgress({
current,
total,
ratePerSec: processTime === 0 ? 0 : ((current - last) / processTime) * 1000,
percentage: (current / total) * 100,
percentage: current && total ? (current / total) * 100 : 100,
});

last = current;
lastProcessTime = currentTime;

if (current >= total) {
clearInterval(internalId);
clearInterval(progressIntervalId);
}
};

const internalId = setInterval(processProgress, frequencyMs);
if (total > 0) {
progressIntervalId = setInterval(processProgress, frequencyMs);
}

signal.addEventListener("abort", () => {
clearInterval(internalId);
clearInterval(progressIntervalId);
});

return needle;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {getKeymanagerTestRunner} from "../utils/keymanagerTestRunners.js";
import {getKeystoresStr} from "../utils/keystores.js";

describeCliTest("import keystores from api, test DefaultProposerConfig", function ({spawnCli}) {
const dataDir = path.join(testFilesDir, "import-keystores-test");
const dataDir = path.join(testFilesDir, "proposer-config-test");

const defaultOptions = {
suggestedFeeRecipient: "0x0000000000000000000000000000000000000000",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import fs from "node:fs";
import {randomBytes} from "node:crypto";
import tmp from "tmp";
import {expect} from "chai";
import chainAsPromised from "chai-as-promised";
import chai from "chai";
import {Keystore} from "@chainsafe/bls-keystore";
import {interopSecretKey} from "@lodestar/state-transition";
import bls from "@chainsafe/bls";
import {SignerLocal, SignerType} from "@lodestar/validator";
import {loadKeystoreCache, writeKeystoreCache} from "../../../../../src/cmds/validator/keymanager/keystoreCache.js";
import {LocalKeystoreDefinition} from "../../../../../src/cmds/validator/keymanager/interface.js";

chai.use(chainAsPromised);

const numberOfSigners = 10;

describe("keystoreCache", () => {
let definitions: LocalKeystoreDefinition[];
let signers: SignerLocal[];
let secretKeys: Uint8Array[];
let passwords: string[];
let keystoreCacheFile: string;

beforeEach(async function setup() {
this.timeout(50000);
definitions = [];
signers = [];
secretKeys = [];
passwords = [];
keystoreCacheFile = tmp.tmpNameSync({postfix: ".cache"});

for (let i = 0; i < numberOfSigners; i++) {
const secretKey = bls.SecretKey.fromBytes(interopSecretKey(i).toBytes());
const keystorePath = tmp.tmpNameSync({postfix: ".json"});
const password = secretKey.toHex();
const keystore = await Keystore.create(
password,
secretKey.toBytes(),
secretKey.toPublicKey().toBytes(),
keystorePath,
"test-keystore",
// To make the test efficient we use a low iteration count
{
function: "pbkdf2",
params: {dklen: 32, c: 10, prf: "hmac-sha256", salt: randomBytes(32).toString("hex")},
}
);
fs.writeFileSync(keystorePath, keystore.stringify());

signers.push({type: SignerType.Local, secretKey});

// Use secretkey hex as password
definitions.push({password: secretKey.toHex(), keystorePath});
passwords.push(password);
secretKeys.push(secretKey.toBytes());
}
});

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;

@nflaig nflaig Feb 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The authenticity of files could better be checked in e2e tests.

});

it("should throw error if password length are not same as signers", async () => {
await expect(writeKeystoreCache(keystoreCacheFile, signers, [passwords[0]])).to.rejectedWith(
`Number of signers and passwords must be equal. signers=${numberOfSigners}, passwords=1`
);
});
});

describe("loadKeystoreCache", () => {
it("should load the valid keystore cache", async () => {
await writeKeystoreCache(keystoreCacheFile, signers, passwords);
const result = await loadKeystoreCache(keystoreCacheFile, definitions);

expect(result.map((r) => r.secretKey.toBytes())).to.eql(secretKeys);
});

it("should raise error for mismatch public key", async () => {
await writeKeystoreCache(keystoreCacheFile, signers, passwords);
definitions[0].keystorePath = definitions[1].keystorePath;

await expect(loadKeystoreCache(keystoreCacheFile, definitions)).to.rejected;
});
});
});
20 changes: 20 additions & 0 deletions packages/cli/test/unit/util/progress.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,26 @@ describe("progress", () => {
expect(progress.secondCall.args[0]).to.eql({total, current: total, ratePerSec: 0, percentage: 100});
});

it("should call progress with correct values directly reaches to total", () => {
const progress = sandbox.spy();
const frequencyMs = 50;
const total = 8;
const needle = showProgress({total, signal: new AbortController().signal, frequencyMs, progress});
Comment thread
nazarhussain marked this conversation as resolved.
needle(7);

expect(progress).to.be.calledOnce;
expect(progress.firstCall.args[0]).to.eql({total, current: total, ratePerSec: 0, percentage: 100});
});

it("should not call progress when initiated with zero total", () => {
const progress = sandbox.spy();
const frequencyMs = 50;
const total = 0;
showProgress({total, signal: new AbortController().signal, frequencyMs, progress});

expect(progress).to.be.not.be.called;
});

it("should not call progress further when abort signal is called", () => {
const progress = sandbox.spy();
const frequencyMs = 50;
Expand Down