Enable un-attended bootstrapping for pk agent start and pk bootstrap with PK_RECOVERY_CODE and PK_PASSWORD
#202
Comments
CMCDragonkai
changed the title
CMCDragonkai
changed the title
|
The BIP39 spec is here: https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki The implementation of the function is currently: The The "mnemonic phrase" is converted to a binary seed using PBKDF2. This binary seed is what is used to generate the RSA root keypair. As long as the same seed can be generated from the same mnemonic phrase, then the same keypair will come out. Notice that the In the BIP39 spec, the user can specify a passphrase to protect the the mnemonic seed. To do so, they must provide a "passphrase" that is appended to the end of We are already randomly generating the root keypair. Switching over to BIP39 simply means we use:
|
|
Some options here for the
The semantics of these environment variables would be specific to the To ensure un-attended |
|
SHould the Bip recovery function separate from the |
|
I believe that the deterministic key generation should be done all the time. That means when calling When
This means recovery code is always generated. |
CMCDragonkai
changed the title
pk agent start and pk bootstrap with PK_RECOVERY_CODE and PK_PASSWORD
|
I've renamed this issue to better represent the intention. The usage of BIP39 is an implementation detail of the Recent discussions with @tegefaulkes indicates that That means Same behaviour then with |
|
BIP recovery code can be in |
CMCDragonkai
changed the title
pk agent start and pk bootstrap with PK_RECOVERY_CODE and PK_PASSWORDpk agent start and pk bootstrap with PK_RECOVERY_CODE and PK_PASSWORD
|
Separated duties regarding port and host bindings to #286. This issue is about "un-attended bootstrapping" of the node state and keys specifically. |
|
I just tested in we can check that a mnemonic matches generates the correct keypair for the node.
both methods check the generated nodeId matches as well. using method 2 is simpler as we use the proceedure.
After confirming that the keypair generated from the mnemonic is correct for the node we can 'recover' they node by writing a new password with |
|
I think there's 2 usecases of the You're talking about the second case right? That means the You have a root certificate that you can use to check if your root key pair's public key matches the expected node id. The node id information should be available in the root certificate, and perhaps available in other files too. I remember we're supposed to have a lock file for singleton agent instances. @joshuakarp can you incorporate this into the spec. |
|
Yes, I'm working on the 2nd right now. |
|
So as a summation of this:
From discussions with @tegefaulkes, it seems like both these environment variables will need to be available during:
Does this mean that we need to read the environment variables in multiple places in |
|
Yes, in each bin script. Each bin script will read their own env
variables (they have to resolve it against their command line
parameters). However they should share utility functions for parsing in
`src/bin/utils.ts`.
…On 11/2/21 2:12 PM, Josh wrote:
So as a summation of this:
* |PK_RECOVERY_CODE|: the environment variable used to both
1. Deterministically generate a root keypair during unattended
bootstrapping
2. Recover a root keypair (e.g. a lost root key password)
o presumably, we'll need another command to invoke a root
keypair recovery (e.g. |pk agent recover <nodepath> <24 word
phrase> <new password>|)
* |PK_PASSWORD|: the environment variable used for "unlocking" the
root keypair such that we authenticate the execution of PK commands
From discussions with @tegefaulkes <https://github.com/tegefaulkes>,
it seems like both these environment variables will need to be
available during:
1. |pk agent start|
2. |pk bootstrap|
3. The start of a session
Does this mean that we need to read the environment variables in
multiple places in |src/bin|?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#202 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHKX52LYSX6DQNXSPWTUJ5JLVANCNFSM472VZXHQ>.
|
|
Initial draft of me trying to map out the bootstrap process: Discussed this with Roger, and there's some things that will need to change in this process.
|
|
Some changes to the lockfile mechanism will occur once I merge in the CLI auto retry. There's some stuff that's important regarding the creation of the the status file. Remember the agent process is meant to hold the lock while it's alive, so |
|
@tegefaulkes I've updated #202 (comment) with comments from discussion with Roger about some other things that should change with the bootstrap process. Please have a read. |
|
Based on #202 (comment) and #202 (comment), this is how I foresee usage of
Does this make sense from your perspective @CMCDragonkai? A question of my own about this:
|
|
The agent recovery CLI command needs to be made but doesn't quite fit this scope. I think it should be made a separate issue. I've already made the recovery function |
|
The pk bootstrap --fresh # bootstrap a new node state
# this we can do later
pk recover --recovery-code-file=./rcf --password-file=./pf # just recovers the key (but doesn't start the agent)
# FOR: empty node state
# deterministic node state generation
pk agent start --recovery-code-file=./rcf
# generate key pair, and then set the password
pk agent start --recovery-code-file=./rcf --password-file=./pf
# FOR: non-empty node state
# generate a keypair deterministically, check if the public key is correct, and if so, prompt for password (this is the new password)
pk agent start --recovery-code-file=./rcf
# recover the key pair, if the public key is correct, set the password
pk agent start --recovery-code-file=./rcf --password-file=./pf
# the --fresh option ignores existing state (but not any existing lock)
pk agent start --recovery-code-file=./rcf --password-file=./pf --fresh |
|
Ok, so just to confirm, the expected behaviour is that we want to update the password when starting a node with a recovery key, password and existing state? |
|
Yes. |
|
For I can simplify Lastly, is this something that really needs to be changed at this stage? |
|
That was brought up by @CMCDragonkai. For reference, this is currently the function: async function checkKeynodeState(nodePath: string): Promise<KeynodeState> {
try {
const files = await fs.promises.readdir(nodePath);
//Checking if directory structure matches keynode structure. Possibly check the private and public key and the level db for keys)
if (
files.includes('keys') &&
files.includes('db') &&
files.includes('versionFile')
) {
const keysPath = path.join(nodePath, 'keys');
const keysFiles = await fs.promises.readdir(keysPath);
if (
!keysFiles.includes('db.key') ||
!keysFiles.includes('root_certs') ||
!keysFiles.includes('root.crt') ||
!keysFiles.includes('root.key') ||
!keysFiles.includes('root.pub') ||
!keysFiles.includes('vault.key')
) {
return 'MALFORMED_KEYNODE';
}
return 'KEYNODE_EXISTS'; // Should be a good initilized keynode.
} else {
if (files.length !== 0) {
return 'OTHER_EXISTS'; // Bad structure, either malformed or not a keynode.
} else {
return 'EMPTY_DIRECTORY'; // Directy exists, but is empty, can make a keynode.
}
}
} catch (e) {
if (e.code === 'ENOENT') {
return 'NO_DIRECTORY'; // The directory does not exist, we can create a bootstrap a keynode.
} else {
throw e;
}
}
}@CMCDragonkai can likely weigh in here. For just ensuring that we can deploy, it's not completely necessary to change this when just considering deployment. As far as I can tell, it's just a minor optimisation by simplifying the process. |
|
When checking whether an agent is running or not, it's not necessary to check the PID at all. Only that whether we can acquire the lock or not. If you cannot acquire the lock, you just fail the command entirely. This is why I realised that |
|
This issue is mostly complete. Everything is done besides the bootstrap task. if its not strictly required then I suggest that we move it to a new issue for later. |
|
According to this comment #286 (comment) we can handle ENV parsing and defaults with commander. However in the case here we want password file passed via a flag while the password itself passed via an ENV variable. so the solution in the comment is not quite applicable here. I can update the the |
|
Is it really necessary to use commander in this case then, given that we don't have the consistency between const passwordFilePath = new commander.Option(
'-pf, --password-file <path>',
'Password Filepath'
);
// now just use the coalescing:
const password = getFromParams(passwordFilePath) ?? getFromEnv('PK_PASSWORD') ?? getFromStdin(); |
Shouldn't the bootstrap share similar behaviour to what agent start is doing? |
Oh yea, that's true. The |
|
Ok, then there is not much to change for that. I can update the |
|
There's no |
Simplifying
|
|
Shouldn't we be holding the lock just before destroying the file? I can see a situation here were we call |
|
If you stop, it means you're stopping the So the real question is whether If so, then that means the lifetime of Now for However they may want to start the status before they are starting But if you are doing If you do need for some reason to use the And remember, the |
|
I was thinking that The reason is because you want to ensure that the status file is destroyed when it's not live. As in there's no reason to leave the file around. When the agent is no longer running. It's not stateful pass process lifecycle like Note that when reading the status, you may get no information. That might mean it's not "ready" yet, which means it's just waiting to launch the server. This is because the status file is created first, and then filled with information once the ports are all bound to. That means your Another problem, the intermediate time between creating a new file and locking it: So imagine: If somehow another process acquired the lock, then you'll have to fail here. For other process, the the status file will be empty at this point. This could mean you have to fail at this point cause it was in the middle of another process launching. |
|
A prototype pseudocode: @CreateDestroy
class Status {
static async createStatus(statusPath) {
// create if not exist
const fd = open(statusPath, this.fs.constants.O_RDWR | this.fs.constants.O_CREAT);
// at this point it may be locked by another process
const locked = lock(fd);
if (!locked) {
throw new ErrorStatusLocked();
}
fd.write({ type: 'starting' });
const status = new Status(statsuPath, statusFd);
return status;
}
constructor(statusPath, statusFd) {
this.statusPath = statusPath;
// assume this is locked already
this.statusFd = statusFd;
}
async destroy () {
unlock(this.statusFd);
this.fs.rm(this.statusPath);
}
// remember this may not be "ready"
// because the PolykeyAgent has to call `writeStatus` to write the actual status information
async readStatus () {
}
async writeStatus () {
}
}Now all agents will do: If it fails, then an exception is thrown there. If there are 2 processes both doing this, then only 1 winner. Whichever gets to lock it. The other fails, and throws that the status is already locked. |
|
Possible situations:
|
|
One of the problem is that Clients need to read the status information. And right now I don't think the Reading the status might need to be a utility function instead. It shouldn't be trying to acquire a lock cause we don't have readlocks atm. It might work as long as the status is well formed, and if it is not well formed, then it should be discarded and converted to be Anyway, the reading of the status and writing of the status is interdependent on the locking of the status. Need a design that can handle all of these cases. |
|
Ok better prototype: import type { FileSystem } from './src/types';
import { CustomError } from 'ts-custom-error';
import Logger from '@matrixai/logger';
import { StartStop, ready } from '@matrixai/async-init/dist/StartStop';
import lock from 'fd-lock';
// pseudocode
class ErrorStatus extends CustomError { }
class ErrorStatusLocked extends ErrorStatus { }
class ErrorStatusParse extends ErrorStatus { }
/**
* When starting, you have to "wait".
* When stopping, there's no point in connecting.
*/
type StatusInfo = {
type: 'starting';
} | {
type: 'stopping';
} | {
type: 'running';
clientHost: string;
clientPort: number;
};
interface Status extends StartStop {}
@StartStop()
class Status {
public readonly statusPath: string;
protected fs: FileSystem;
protected logger: Logger;
protected statusFd: number;
constructor ({
statusPath,
fs = require('fd'),
logger = new Logger(Status.name),
}: {
statusPath: string;
fs: FileSystem;
logger: Logger;
}) {
this.logger = logger;
this.statusPath = statusPath;
this.fs = fs;
}
public async start() {
this.logger.info('Starting Status');
const statusFile = await this.fs.promises.open(
this.statusPath,
this.fs.constants.O_RDWR | this.fs.constants.O_CREAT
);
if (!lock(statusFile.fd)) {
throw new ErrorStatusLocked;
}
this.statusFd = statusFile.fd;
this.logger.info('Started Status');
}
public async stop() {
this.logger.info('Stopping Status');
lock.unlock(this.statusFd);
await this.fs.promises.rm(this.statusPath,
{ force: true }
);
this.logger.info('Stopped Status');
}
/**
* Can read status without locking
*/
public async readStatus(): Promise<StatusInfo | undefined> {
let statusData;
try {
statusData = await this.fs.promises.readFile(this.statusPath, 'utf-8');
} catch (e) {
if (e.code === 'ENOENT') {
return;
}
// do we expect other filesystem errors?
// like permission errors?
// io errors?
throw e;
}
if (statusData === '') {
return;
}
let statusInfo;
try {
statusInfo = JSON.parse(statusData);
} catch(e) {
throw new ErrorStatusParse;
}
// TODO: PARSE the statusInfo, use json schema
return statusInfo;
}
/**
* Can only write status when it is locked
*/
@ready()
public async writeStatus(statusInfo: StatusInfo): Promise<void> {
// do your write
}
}So now it's possible: Or const status = new Status(...);
await status.start();
await status.writeStatus(...);
await status.stop();We may want to have exceptions for dealing with reads and writes. Exceptions could be more general like |
|
There are 4 places where Status would be used:
|
|
@tegefaulkes just to confirm the recovery code you're using is a 12 or 24 word recovery key? Is it 12 or 24? If you haven't chosen yet, I suggest parameterising it, and then defaulting it to |
|
It's 24 and easy to change. |
|
The 15s time of pbkdf2 means we should move towards webcrypto and webassembly asap. But we will do this after teh testnet deployment. This is tracked in #270. |
CMCDragonkai
added
the
r&d:polykey:core activity 1
Block 1 from #194 (comment)
Aims to provide support for a Polykey instance to run unattended (i.e. without user prompt) when both the
PK_RECOVERY_CODEandPK_PASSWORDvariables are set.Requirements of this design
Additional context
pki.rsa.generateKeyPairdigitalbazaar/forge#865Note that the core function to generate a root key from a BIP39 seed is already available in our
keys/utils.ts, see the function:generateDeterministicKeyPair. However it is not currently used by KeyManager, and not well tested.Screenshot from Figma GUI design:
Specification
generateDeterministicKeyPairgenerateDeterministicKeyPairis well testedRegarding the root key acceptance, consider this idea:
CLI command variables
Variables for CLI commands can be retrieved from:
src/config.tsWe require functions to retrieve the variables from each of these locations (created in
src/bin/utils).Variable precedence should be as follows: file parameters > environment variables/
src/config.ts> default (STDIN prompt). We can use a sequence of coalescing operations to emulate this: for example, to retrieve the password,password = getFromParams() ?? getFromEnv ?? getFromStdin().getFromStdin()here is expected to be the default, where we assume that a value will always be provided. If a default cannot exist for a particular variable, then throw an exception if none of the sources of variables can produce one.See point 1 at #202 (comment) for further information.
Bootstrap process
Some components of the bootstrapping process need to be simplified:
bootstrapPolykeyState(see 3 Enable un-attended bootstrapping forpk agent startandpk bootstrapwithPK_RECOVERY_CODEandPK_PASSWORD#202 (comment))writeTokeninSession.ts- discuss with @CMCDragonkaicheckKeynodeState(see 4 Enable un-attended bootstrapping forpk agent startandpk bootstrapwithPK_RECOVERY_CODEandPK_PASSWORD#202 (comment))nodePath: if any state exists besides the lockfile, throw an exceptionSub-Issues & Sub-PRs created
#283
checklist
bin/utilsfunctions for variable retrievalgetFromParams(): get variable from file parameter. Usestr.trim()to remove trailing newlines/whitespace from the file (see 2 at Enable un-attended bootstrapping forpk agent startandpk bootstrapwithPK_RECOVERY_CODEandPK_PASSWORD#202 (comment) for further information) ~~getFromEnv(): get variable from environment variablegetFromConfig(): get variable fromsrc/config.tsgetFromStdin(): get variable from user promptbinUtils.getPasswordandbinUtils.getRecoveryCode. options for hosts and ports was created insrc/bin/options.tswith their own defaults and env.bootstrapprocessbootstrapPolykeyState2x pk bootstrap2x pk agent startpk bootstrap+pk agent startcheckKeynodeState--freshflag: forces new keynode state, even if it already existspk agent startpk bootstrapThe text was updated successfully, but these errors were encountered: