search

MatrixAI / Polykey-CLI

Polykey CLI - Open Source Decentralized Secret Sharing System for Zero Trust Workflows
https://polykey.com
GNU General Public License v3.0
6 stars 3 forks source link

Implement `secrets edit` command #266

Closed aryanjassal closed 3 weeks ago

aryanjassal commented 1 month ago

Specification

If a secret doesn't exist when the edit command is invoked, then currently, the command just fails. Instead, the user needs to run polykey secrets create to copy a file from their file system and make that a secret, then they are able to use polykey secrets edit command. This is not great for UI/UX.

Moreover, each platform has their own preferences for file editor. As such, identify and launch the corresponding file editor to edit the secret. Windows would have notepad.exe, and MacOS and Linux editors would require to be inferred from environment. To ensure everything is working smoothly, integration tests need to be done on all the given platforms.

Generally speaking you rely on an environment variable: $VISUAL then if not set, rely on $EDITOR, and if not set, then you default onto something that would exist on most platforms.

https://unix.stackexchange.com/questions/4859/visual-vs-editor-what-s-the-difference

The "default" depends on some magic.

Here's our list depending on the OS.

  1. Linux - VISUAL, EDITOR, nano, ed - I wouldn't default to vi or vim because it's actually a more advanced editor.
  2. Windows - VISUAL, EDITOR, get the .txt file association first, then default to notepad.exe and wait for it to finish. Windows never had a default terminal text editor.
  3. MacOS - VISUAL, EDITOR, pico, nano

If the defaults cannot be found, you need to error out, indicating that no default editor could be found, and users can instead choose to set VISUAL, EDITOR or pass in the "editor" program with --ed or something.

Furthermore it is important that the temporary being created can only be edited by the same user as the user who ran polykey secrets edit, otherwise it can be intercepted. The file should have a limited umask like 700. Or equivalent for the Windows.

The behaviour of secrets edit command shouldn't be what is simplest. It should be what makes the most sense.

For example, instead of making a blank secrets file if it doesn't exist, ignore the part where the secret contents are fetched instead. Point the editor, say, vim to the temporary file, but don't make the file yet. If the editor exists without making the file, the action was cancelled. Do nothing. No need to make a file in the vault. If, on the other hand, a file which wasn't there before was created, then the user wants to make a new file with that contents. Then, make a new file with the contents that the user just entered. Otherwise, it should behave as it already has been.

The file should be in the temp directory under a random subdirectory. For example, a file I'm editing could look like /tmp/somedir-1234/file. The file name could be a constant, that doesn't matter. The directory it is under should be randomised. This appears to be the current default behaviour, so no changes are required for this.

Finally, consider concurrency. An agent can run multiple RPC commands concurrently. What happens when a file is edited at the same time another file is being edited? What if the same file is being edited in two separate agent commands? This would need to look into PCC and OCC for file locking. However, this has already been implemented in Polykey side, where it only allows a single operation to modify a file at a time.

The implementation should consider the changes to how the file creation is handled, and verify that the implemented behaviour for file randomisation and file locking is functional as intended.

Additional context

Tasks

  1. Create a new secret and edit it if a secret doesn't already exist.
  2. Rely on system editor to edit the secrets.
  3. Test for desired behaviour on all platforms.
linear[bot] commented 1 month ago

ENG-398 Implement `secrets edit` command

aryanjassal commented 1 month ago

I'm not sure how to handle checking if the file exists or not.

My first idea was to add a check to make an empty file if it doesn't already exist in all RPC handlers in Polykey, but that would break things and the behaviour would feel strange.

My second idea was to use the new ls command on Polykey CLI to see if the file exists, as on UNIX, if you ls a file, it would return only that file. However, simplicity and speed was prioritised, and this feature wasn't included in the current PR for the secrets ls command, so this can't be used either.

My third idea was to make an RPC handler on Polykey which checks if a file exists or not, but that seems like a lot of work for doing something seemingly simple.

All these ideas have their pros and cons, and I'm not sure which one to follow, or if I'm missing an easier solution.

tegefaulkes commented 1 month ago

Part of the process is getting the file data to edit. You have two options here.

  1. Stat the file first. If the file doesn't exist then you'd get an ENOENT error and work from there.
  2. We should expand the file content serialiser to serialise errors it encounters when trying to open the file. This would just be the ENOENT error for the most part. Permission related errors would be possible but shouldn't happen for our usage since we don't use the file permissions in the vaults.
aryanjassal commented 1 month ago

Stat the file first. If the file doesn't exist then you'd get an ENOENT error and work from there.

I thought error-driven workflow wasn't ideal as it takes a lot of resources to throw an error.

Perhaps alternatively, we can return something like null or undefined if the file doesn't exist at the time of vaultsSecretsGet? Or maybe extend its functionality to include a parameter which would create the file if it doesn't exist?

tegefaulkes commented 1 month ago

Usually yeah, but for FS related stuff it's just how the API works. So in this case we just have to deal with it.

I know it's a contradiction of what I said earlier. But we can't really force our best practice on a library that's following an API pattern based on exit codes in a terminal.

CMCDragonkai commented 1 month ago

You should copy over the editor detection and error if not found spec to the spec above, it's core to this issue, it shouldn't be just additional context.

Also some links for later cross platform testing:

Since they haven't been updated, it's likely the existing user accounts on those platforms is still the case. I have to update access for @brynblack.

How can we automate orchestration of the Mac and Windows machines? That's something to figure out later - new issue needed @brynblack. But for now, manual access for @brynblack acting as the Orchestrator is sufficient.

CMCDragonkai commented 1 month ago

@brynblack I noticed that there's no mention of remmina for the 2 platforms. They are required for it.

CMCDragonkai commented 1 month ago

@brynblack https://gitlab.com/MatrixAI/Employees/matrix-orientation/-/issues/37 - this is missing from the docs.

CMCDragonkai commented 1 month ago

Related cross platform testing for this: https://github.com/MatrixAI/Epsilon-Base/issues/3.

aryanjassal commented 1 month ago

The behaviour of secrets edit command shouldn't be what is simplest. It should be what makes the most sense.

For example, instead of making a blank secrets file if it doesn't exist, ignore the part where the secret contents are fetched instead. Point the editor, say, vim to the temporary file, but don't make the file yet. If the editor exists without making the file, the action was cancelled. Do nothing. No need to make a file in the vault. If, on the other hand, a file which wasn't there before was created, then the user wants to make a new file with that contents. Then, make a new file with the contents that the user just entered. Otherwise, it should behave as it already has been.

The file should be in the temp directory under a random subdirectory. For example, a file I'm editing could look like /tmp/somedir-1234/file. The file name could be a constant, that doesn't matter. The directory it is under should be randomised. This appears to be the current default behaviour, so no changes are required for this.

Finally, consider concurrency. An agent can run multiple RPC commands concurrently. What happens when a file is edited at the same time another file is being edited? What if the same file is being edited in two separate agent commands? This would need to look into PCC and OCC for file locking. However, this has already been implemented in Polykey side, where it only allows a single operation to modify a file at a time.

Implement the changes to how the file creation is handled, and verify that the implemented behaviour for file randomisation and file locking works.

CMCDragonkai commented 4 weeks ago

The behaviour of secrets edit command shouldn't be what is simplest. It should be what makes the most sense.

For example, instead of making a blank secrets file if it doesn't exist, ignore the part where the secret contents are fetched instead. Point the editor, say, vim to the temporary file, but don't make the file yet. If the editor exists without making the file, the action was cancelled. Do nothing. No need to make a file in the vault. If, on the other hand, a file which wasn't there before was created, then the user wants to make a new file with that contents. Then, make a new file with the contents that the user just entered. Otherwise, it should behave as it already has been.

The file should be in the temp directory under a random subdirectory. For example, a file I'm editing could look like /tmp/somedir-1234/file. The file name could be a constant, that doesn't matter. The directory it is under should be randomised. This appears to be the current default behaviour, so no changes are required for this.

Finally, consider concurrency. An agent can run multiple RPC commands concurrently. What happens when a file is edited at the same time another file is being edited? What if the same file is being edited in two separate agent commands? This would need to look into PCC and OCC for file locking. However, this has already been implemented in Polykey side, where it only allows a single operation to modify a file at a time.

Implement the changes to how the file creation is handled, and verify that the implemented behaviour for file randomisation and file locking works.

That should be part of the spec.

aryanjassal commented 3 weeks ago

I have implemented the creation of a file if it doesn't already exist in the vault. The remaining tasks for this issue will be tracked in https://github.com/MatrixAI/Polykey-CLI/issues/277 (ENG-405).

CMCDragonkai commented 3 weeks ago

You should copy over the editor detection and error if not found spec to the spec above, it's core to this issue, it shouldn't be just additional context.

Also some links for later cross platform testing:

Since they haven't been updated, it's likely the existing user accounts on those platforms is still the case. I have to update access for @brynblack.

How can we automate orchestration of the Mac and Windows machines? That's something to figure out later - new issue needed @brynblack. But for now, manual access for @brynblack acting as the Orchestrator is sufficient.

Has the cross platform testing been done?

PROBABLY not! Because you need access to the mac and windows machines, and I haven't delegated due to lack of store-and-forward convenience. That's for manual sanity checks.