Remove newlines by default when exporting contents using secrets env

[aryanjassal]

Description

The standard to end all files in Unix is adding a newline at the end of the file. All editors, including vim do it. However, adding a stray newline at the end of a secret, like a password, can have adverse effects.

As such, this PR will, by default, trim the single trailing newline, with an option to disable it if needed.

Issues Fixed

• Fixes Update secrets env to omit single trailing \n from files which automatically include it #296 (REF ENG-416)
• Related to secrets env needs -- to be passed twice to parse the command #325 (REF ENG-466)

Tasks

• 1. Remove newlines by default
• 2. Add a new option to allow preserving newlines
• 3. Add tests ensuring this behaviour

Final checklist

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

[linear]

ENG-416 Update `secrets env` to omit single trailing `\n` from files which automatically include it

[aryanjassal]

We are using spawnSync to spawn the env command. This will result in everything else being blocked, and in cases of a long-running command, will also lead to websocket timeouts. To avoid this, I will change it to use spawn much like what I did in secrets edit.

Also, there was a minor inaccuracy in the command, where the command would fail without any environment variables being provided. This is different than the Unix env, which just runs the command with all the system variables instead. was this intentional or should I fix it?

@tegefaulkes @CMCDragonkai

[CMCDragonkai]

In our case polykey secrets env doesn't make sense to run with no variables. But... What do you mean by runs with system variables?

[aryanjassal]

In our case polykey secrets env doesn't make sense to run with no variables. But... What do you mean by runs with system variables?

By default, when no variables are provided to env, it exports all the current shell variables into the command. This can be previewed by running env in the terminal, which would output all the current variables. That's what gets passed on to any command. Any other variables we define gets appended to that list.

In our case, it does not make much sense, but is a deviation from the standard Unix behaviour, which is why I wanted to ask and confirm this.

[aryanjassal]

We are using spawnSync to spawn the env command. This will result in everything else being blocked, and in cases of a long-running command, will also lead to websocket timeouts. To avoid this, I will change it to use spawn much like what I did in secrets edit.

After talking about this with Brian, I have realised that the Polykey client is actually stopped before the commands are even run. This means that, without a connection to Polykey, the websocket errors won't happen, and that is a non-issue in this case.

I did a manual sanity check by using a long-running command in secrets env, and it didn't throw any errors.

[aryanjassal]

The syntax to preserve newlines for a secret is as follows:

[aryanj@matrix-34xx:~]$ polykey secrets env vault:SECRET1 vault:SECRET2
SECRET1='testing123'
SECRET2='testing123'

[aryanj@matrix-34xx:~]$ polykey secrets env -pn vault:SECRET1 vault:SECRET2
SECRET1='testing123
'
SECRET2='testing123'

[aryanj@matrix-34xx:~]$ polykey secrets env --preserve-newline VAULT:SECRET3
SECRET3='newlines-after-this

'

[aryanj@matrix-34xx:~]$ polykey secrets env VAULT:SECRET3
SECRET3='newlines-after-this
'

If the newlines should be preserved, then the flag -pn --preserve-newline should be used before providing a secret.

[aryanjassal]

How should I handle providing the same secret in two different contexts? If SECRET1 has been provided with both a preserve newline and without, what should take precedence?

What about cases where we export entire vaults? Currently, the flag does preserve newlines of all the secrets exported from the vault, but what if we need one secret to have newlines, and the rest can be trimmed? Or vice-versa, where all secrets need to have preserved newlines, but one secret needs to be trimmed? How would be handle those cases?

With the way how the command is currently structured, is it not possible to preserve the order of the secrets, so they can't be used for prioritisation.

Currently, the preserving newline takes precedence over trimming them. For example, the following will preserve newlines for all secrets:

[aryanj@matrix-34xx:~]$ polykey secrets env -pn vault vault:SECRET1
SECRET1='testing123
'
SECRET2='testing123
'

Should this be the intentional behaviour? @CMCDragonkai

[aryanjassal]

I've run into a roadblock and need some inputs from @CMCDragonkai and @tegefaulkes to help me with some issues.

[linear]

ENG-466 `secrets env` needs `--` to be passed twice to parse the command

[CMCDragonkai]

You can have a global flag or the flags are applied per spec. I would do something like that.

[CMCDragonkai]

By default secret env is going to need to trim a single trailing newline.

Repeating the spec is an interesting situation I would argue that you maintain order in line with whatever the order of the specification is. In the case of env shells the last one takes priority because of how shell processes them.

[aryanjassal]

After a discussion with Brian, we have come to the following conclusion:

Instead of using a separate option like --preserve-newline, then merging the secrets passed in to that and the original arguments, we instead pass in all the secret paths as arguments. The flag can be used as a modifier to the argument list instead of acting as a separate argument list to keep track of.

This means that all secrets specified are, by default, trimmed. However, if a secret name is present in the list to preserve newlines, then the newlines will be preserved. This approach does cause duplication to the secret path, but that's not a huge concern, as most of the times, the newlines would be trimmed as usual.

This approach solves all the issues. The newline preservation takes precedence. If the newlines must be trimmed, then don't specify it to be preserved. As the arguments need to be populated anyways, they will never be empty, and the usual checks can be done from within the parser without need of manually creating an empty object or throwing an error from the action.

This also, to an extent, treats the symptoms of '--' being weird to use. That is an exceptionally weird issue, as they work perfectly in the tests, but break when used via the command line. I can start by using shell in pkExec, which is being used in the tests. This will let me find out if the shell is misbehaving or not.

[CMCDragonkai]

Can you also review MatrixAI/Polykey#222 (comment)? I want to make sure we're laying foundations to enable egress schema implementation.

[aryanjassal]

Can you also review MatrixAI/Polykey#222 (comment)? I want to make sure we're laying foundations to enable egress schema implementation.

Just to confirm, this egress schema refers to a schema which would allow formatting of the secrets in a particular way at the time of egress, which would be using the secrets env command in this case? For example, the schema would specify if a secret needs to preserve trailing newline or not. If so, then yes, foundation for supporting this is being laid in this PR.

[aryanjassal]

Key changes in this PR:

• Added a new error which is thrown whenever a subprocess fails. It is used in secrets env (if the command fails) and secrets edit (if the editor fails).
• Properly removed handlers from editor process in secrets edit
• Automatically trimming newlines when exporting secrets using secrets env
• Preserving newlines of any secrets listed in -pn option. (should there be a short form of this option?)
• Created type for parsed secret path and value ([string, string?, string?] like [vaultName, secretPath, value])
• Added tests for trimming and preserving newlines using fastcheck

Need input on:

• Is the addition and usage of ErrorPolykeyCLISubprocessFailure reasonable?
• Should -pn --preserve-newline have a short form of the option?

@tegefaulkes

[CMCDragonkai]

Key changes in this PR:

• Added a new error which is thrown whenever a subprocess fails. It is used in secrets env (if the command fails) and secrets edit (if the editor fails).
• Properly removed handlers from editor process in secrets edit
• Automatically trimming newlines when exporting secrets using secrets env
• Preserving newlines of any secrets listed in -pn option. (should there be a short form of this option?)
• Created type for parsed secret path and value ([string, string?, string?] like [vaultName, secretPath, value])
• Added tests for trimming and preserving newlines using fastcheck

Need input on:

• Is the addition and usage of ErrorPolykeyCLISubprocessFailure reasonable?
• Should -pn --preserve-newline have a short form of the option?

@tegefaulkes

If we are using exec, subprocess failure should be temporary on Unix systems, because of process replacement. It would be more accurate to say that you were not able to execute the subprocess. Also I prefer the name ChildProcess.

On windows however there is no process replacement. It's always a child process. Did you do cross platform sanity checks?

[aryanjassal]

Did you do cross platform sanity checks?

Oh, it completely slipped my mind. I will do this today before merging.

[aryanjassal]

On windows however there is no process replacement. It's always a child process. Did you do cross platform sanity checks?

#331

[aryanjassal]

The CI is passing, and the PR has been approved. I will be merging it now.

[CMCDragonkai]

This was a significant change to the UX of secrets env. Now it is actually usable.