Allow exporting all environment variables by default for secrets env

[aryanjassal]

Description

This is a child PR of #305 which only implements the required changes to allow the secrets env to use the vault name only without needing to specify the secrets.

Currently, we do this: polykey secrets env vault:. to export all secrets from a vault.
After this, we can do polykey secrets env vault to automatically export all the secrets in the vault.

This PR will also allow using dots in vault names. More robust checks will be implemented as a part of the parent PR.

Issues Fixed

• Allows Polykey secrets env integration #289 to move forward
• Relates to Vault names can have . character when creating them, but they aren't valid paths for secrets commands #251
• REF ENG-379
• Partially resolves Specify only the vault path and infer the secret path to point to root #304
• REF ENG-425

Tasks

• 1. Allow dots to be included in the vault name
• 2. Allow secrets env to export all secrets if no secret path is specified

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[aryanjassal]

We have run into a roadblock with the implementation of secrets env. We are running the following command in a test, which runs secrets env and prints out the environment variables to stdout.

command = [
  'secrets',
  'env',
  '-np',
  dataDir,
  '--env-format',
  'unix',
  '--',
  `${vaultName}:dir1=SECRET_NEW`,
  'node',
  '-e',
  'console.log(JSON.stringify(process.env))',
];

The parser for this command first tries to parse each argument as a secret path. The previous implementation would disallow node to be a valid secret path, so it would start tracking all the args after that as the actual command. However, now that empty secret path does refer to a valid secret path (the root of the vault) instead of throwing a parsing error, the current parser just parses each argument as a valid vault name.

This doesn't allow for the additional commands to be accumulated, as we can no longer differentiate between the command and the vault paths. Until a solution for handling this is discussed, this PR is essentially blocked.

(Brian's commentary on this)

Its a common problem in parsing byte streams. If the data can be any byte then how do you split up the stream, any delimiter could actually be a value or vice versa. There's not a lot of options here that look nice.

1. There's length delimiting where you specify the length of each message at the beginning. Not great for args.
2. There's a delimter that you exclude from the values altogether.
3. Or you could use a delimiter and escape it in the values.

None of these are great but the original method was basically option 2.

We might be able to add a separate option which might take variadic parameters for entering the command, or maybe takes a string as the command. This needs some discussion and inputs before we can move ahead with this PR.

@CMCDragonkai @tegefaulkes

[linear]

ENG-379 Vault names can have `.` character when creating them, but they aren't valid paths for secrets commands

[linear]

ENG-425 Specify only the vault path and infer the secret path to point to root

[aryanjassal]

We can use an option with variadic arguments as such:

class CommandEnv extends CommandPolykey {
  constructor(...args: ConstructorParameters<typeof CommandPolykey>) {
    super(...args);
    this.name('env');
    this.argument(
      '<args...>',
      'arguments formatted as [envPaths...]',
      binParsers.parseEnvArgs,
    );
    this.option(
      '--command <args...>
      'command formatted as [cmd][cmdArgs...]',
    );
    this.action(async (args: Array<string>, options) => {
      console.log(options.command); // Example: ['node', '-e', 'console.log(JSON.stringify(process.env))']
...

So, the new command invocation can look like this:

[aryanj@matrix-34xx:~]$ polykey secrets env vault --command node -e console.log("hello world")

Should this be implemented in this PR, or is there another approach to this?

@CMCDragonkai @tegefaulkes

[CMCDragonkai]

We can use an option with variadic arguments as such:

class CommandEnv extends CommandPolykey {
  constructor(...args: ConstructorParameters<typeof CommandPolykey>) {
    super(...args);
    this.name('env');
    this.argument(
      '<args...>',
      'arguments formatted as [envPaths...]',
      binParsers.parseEnvArgs,
    );
    this.option(
      '--command <args...>
      'command formatted as [cmd][cmdArgs...]',
    );
    this.action(async (args: Array<string>, options) => {
      console.log(options.command); // Example: ['node', '-e', 'console.log(JSON.stringify(process.env))']
...

So, the new command invocation can look like this:

[aryanj@matrix-34xx:~]$ polykey secrets env vault --command node -e console.log("hello world")

Should this be implemented in this PR, or is there another approach to this?

@CMCDragonkai @tegefaulkes

I want it to work like env in the default case so --command shouldn't be used.

[aryanjassal]

command = [
  'secrets',
  'env',
  '-np',
  dataDir,
  '--env-format',
  'unix',
  '--',
  `${vaultName}:dir1=SECRET_NEW`,
  'node',
  '-e',
  'console.log(JSON.stringify(process.env))',
];

The -- can go after the vault name, and then that would interpret the following arguments as the command, and everything before that would be interpreted as the vault names.

[aryanjassal]

const { Command } = require('commander');
const program = new Command();

program
  .command('secrets env [vaults...]')
  .description('Set environment with vaults and execute a command')
  .allowUnknownOption(true) // Allows capturing anything after `--` as unknown options
  .action((vaults, options) => {
    // Find the `--` separator in the arguments to capture everything after it.
    const indexOfDoubleDash = process.argv.indexOf('--');
    let command = [];

    // If `--` is found, get all arguments after it.
    if (indexOfDoubleDash !== -1) {
      command = process.argv.slice(indexOfDoubleDash + 1);
    }

    console.log('vaults:', vaults);
    console.log('command:', command);
  });

program.parse(process.argv);

This is what ChatGPT gave me as a possible implementation. I will use this for reference when implementing this.

It also gave me this regex to match vault names:

^[\p{Print}&&[^/\p{Cc}]]+$

This will match all printable UTF-8 characters (from the {Print} group) while disallowing control characters (from the {Cc}group) and additionally the/`.

As we are not currently dealing with local file systems, this can be kept simple while retaining possibility of updates.

[aryanjassal]

const { Command } = require('commander');
const program = new Command();

program
  .command('secrets env [vaults...]')
  .description('Set environment with vaults and execute a command')
  .allowUnknownOption(true) // Allows capturing anything after `--` as unknown options
  .action((vaults, options) => {
    // Find the `--` separator in the arguments to capture everything after it.
    const indexOfDoubleDash = process.argv.indexOf('--');
    let command = [];

    // If `--` is found, get all arguments after it.
    if (indexOfDoubleDash !== -1) {
      command = process.argv.slice(indexOfDoubleDash + 1);
    }

    console.log('vaults:', vaults);
    console.log('command:', command);
  });

program.parse(process.argv);

This is what ChatGPT gave me as a possible implementation. I will use this for reference when implementing this.

I have realised that commander doesn't provide the location of -- or differentiate according to that. If we need to identify the location of --, then we need to do so with process.argv, which I believe completely undermines the purpose of commander.

And if we are already requiring the users to put the command after --, might as well make it --run -- or something.

The env command also relies on the syntax for the environment variables to be <KEY>=<VALUE>, and if the syntax doesn't follow this, then it would be classified as a command. However, if we want to just export all secrets in a vault, and the vault name can resemble a command and vice-versa, then implicitly detecting the beginning of the commands isn't viable anymore.

[aryanjassal]

Nevermind, I got it working.

Commander by default parses options positionlessly. Meaning, polykey secrets list -np 'path' would have been parsed the same as polykey secrets -np 'path' list. However, we can set Command.passThroughOptions(), which will force commander to parse everything after starting a variadic argument as part of it. Then, it will also parse -- as a part of the variadic argument. This can then be detected by the parser, and it can then signify the switch to parsing the rest as commands.

For example, polykey secrets env vault1 vault2 -- command arg1 arg2 will have the following passed into its parser: ['vault1', 'vault2', '--', 'command', 'arg1', 'arg2'].

Some more testing on this is needed, but from what I can tell, it seems to work pretty well.

[aryanjassal]

I have noticed that for all the tests, we are checking for the exact expected error code (like 64 for usage). For my secrets commands, I have just been testing if the exit code is not zero. Does this need to change? If so, I can track this change in some issue somewhere.

[aryanjassal]

The required change for allowing dots in vault names and defaulting the path to vault root has been implemented. This change isn't meant to be a complete and bulletproof change, but meant to be enough to allow env migration work to be done.

This PR can now be merged. The other features are being tracked in #305