Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Allow mnemonic recovery via file #365

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brad-u410
Copy link

The only way to recover was to specify the mnemonic as an input via STDIN. This carries a number of risks, as that mnemonic could be exposed via process monitoring, shell history, or over the shoulder observation.

I believe allowing the user to specify a file to recover from is a generally more secure path to recovery from a mnemonic.

The only way to `recover` was to specify the mnemonic as an input via
STDIN. This carries a number of risks, as that mnemonic could be exposed
via process monitoring, shell history, or over the shoulder observation.

I believe allowing the user to specify a file to recover from is a generally more
secure path to recovery from a mnemonic.
@jclapis
Copy link
Member

jclapis commented May 17, 2023

@jshufro this is going to obviate your one-word-at-a-time verifier, want to give it a look?

@brad-u410
Copy link
Author

My ideal usage is something like:

api wallet recover --mnemonic-file <(gpg --decrypt /path/to/encrypted-backup)

Otherwise, I'll decrypt the backup, and write the mnemonic to a tmpfs file, recover, and then delete the plaintext.

But ideally, I never have to actually see the full mnemonic.

@brad-u410
Copy link
Author

For example, if any sort of process monitoring is enabled (i.e. datadog, metrics agents on GCP/AWS), using the CLI to recover will expose that mnemonic in plaintext at least twice (once via the CLI call, which then makes a docker call to the API container).

At the very least, I think the API commands should optionally support recovering from a file. If the change is deemed worthwhile, I can also modify the other recovery commands to support the same sort of logic.

@jshufro
Copy link
Contributor

jshufro commented May 17, 2023

@jshufro this is going to obviate your one-word-at-a-time verifier, want to give it a look?

This changes the api container, which is downstream. The word-at-a-time flow passes the full mnemonic to the mnemonic flag, so it isn't obviated.

That said, I think this is just in the wrong place. Accessing the filesystem from the API is tricky, you need to mount the file you want into the container. Is there a reason why rocketpool-cli doesn't just support an additional flag, and pass the mnemonic to rocketpool_api via its normal docker run semantics?

@jshufro
Copy link
Contributor

jshufro commented May 17, 2023

Ah I see, you don't want the mnemonic exposed when the cli forks off the api call.

Well, this seems fine, though now I wonder if it has any value given that the master node is stored on disk, as is the key, and anyone in the docker group can trivially decrypt it.

@brad-u410
Copy link
Author

yea, this was more motivated by potential exposure to monitoring agents, which could expose the mnemonic outside the instance itself.

@jshufro
Copy link
Contributor

jshufro commented May 17, 2023

Yep LGTM

@brad-u410
Copy link
Author

I've updated the MR to make this backwards compatible, given the interaction between the node and api containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants