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

Add encrypted configs to pcli #4293

Closed
Tracked by #4345
cronokirby opened this issue Apr 30, 2024 · 4 comments · Fixed by #4343
Closed
Tracked by #4345

Add encrypted configs to pcli #4293

cronokirby opened this issue Apr 30, 2024 · 4 comments · Fixed by #4343
Assignees
Labels
A-client Area: Design and implementation for client functionality _P-V1 Priority: slated for V1 release
Milestone

Comments

@cronokirby
Copy link
Contributor

In real-world custody setups, in can be useful to store the pcli config file—which contains the shares of the spend key—encrypted at rest. However, given the complexity of setting up an actual threshold custody, the additional barrier of also setting up a correct encrypted backup is an additional paper cut which we might want to avoid.

We could natively support an encrypted configuration in pcli.
We would have an encrypted configuration option in pcli, which would store an encrypted blob instead of the usual config, and then require a password to decrypt the configuration.
We could implement this by:

  • adding a --encrypted flag to pcli init,
  • adding a method to convert a normal config into an encrypted one.
@cronokirby cronokirby added the A-client Area: Design and implementation for client functionality label Apr 30, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Penumbra Apr 30, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label Apr 30, 2024
@hdevalence
Copy link
Member

hdevalence commented Apr 30, 2024

I think we should try to figure out a way to have only subfields of the config data be encrypted. This would allow the FVK to sit in the toml file and for pcli to be used for viewing operations without prompting for a password.

It would be useful to support encryption for single spend keys as well as threshold data, where it's even more important.

Is there a way that we could have the SoftKms custody implementation prompt for a passphrase only when it gets a request? That way we could avoid doing large changes to the structure of the pcli code.

@cronokirby
Copy link
Contributor Author

Another approach instead of encrypting the config itself, is to just encrypt the custody portion of the config. We could do this with a new custody backend which would wrap another backend. This would touch the relevant items for SoftKms and Threshold custody.

@hdevalence
Copy link
Member

Hmm, so the idea is something like

  • there's a new EncryptedCustody struct (name tbd) with its own config type that has an encrypted blob;
  • the EncryptedCustody has internal state that's an enum of the encrypted blob and optionally a boxed service;
  • when a request comes in:
    • if the boxed service is None, it prompts for a password on stdin, decrypts, parses the config, instantiates the inner service, boxes it and sets it, then sends the request in
    • if it's Some, just pass through the request

?

@cronokirby
Copy link
Contributor Author

Yeah, that's basically the idea; we might want to shift the decryption to initialization, but otherwise the wrapping is what I'd envisioned.

@aubrika aubrika added this to the Sprint 6 milestone May 1, 2024
@aubrika aubrika added _P-V1 Priority: slated for V1 release and removed needs-refinement unclear, incomplete, or stub issue that needs work labels May 1, 2024
@cronokirby cronokirby moved this from Backlog to In progress in Penumbra May 1, 2024
@cronokirby cronokirby self-assigned this May 1, 2024
TalDerei pushed a commit that referenced this issue May 3, 2024
Bumps the application version to v0.72.0, which is required
because the changes in #4293 were merged into a release branch,
and thus didn't land on main naturally. We'll backport that patch
shortly.

More relevantly, the chain-upgrade docs now reference the "current"
migration, which is 71 -> 72, with its own genesis time.

Finally, a small change is made to the recommended dir names, using
unique-to-this-migration dirnames for the export, to avoid confusion
for operators who have previously migrated and not cleaned up the state.
redshiftzero pushed a commit that referenced this issue May 8, 2024
## Describe your changes

This adds a new option to encrypt the `soft-kms` and `threshold` custody
backends with a password, so that spend-key related material is
encrypted at rest. This is implemented by:

1. Having a `pcli init --encrypted` flag that applies to both of these
backends, which prompts a user for a password (and confirmation) before
using that to encrypt the config.
2. Having a `pcli init re-encrypt` command to read an existing config
and encrypt its backend, if necessary, to allow importing existing
configs.

This is also implemented internally in a lazy way, so that a password is
only prompted when the custody services methods are actually called,
allowing us to not need a password for view only commands.

## Issue ticket number and link

Closes #4293.

## Checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > This is a client-only change.

---------

Co-authored-by: cratelyn <[email protected]>
@github-project-automation github-project-automation bot moved this from In progress to Done in Penumbra May 8, 2024
hdevalence pushed a commit that referenced this issue May 8, 2024
This adds a new option to encrypt the `soft-kms` and `threshold` custody
backends with a password, so that spend-key related material is
encrypted at rest. This is implemented by:

1. Having a `pcli init --encrypted` flag that applies to both of these
backends, which prompts a user for a password (and confirmation) before
using that to encrypt the config.
2. Having a `pcli init re-encrypt` command to read an existing config
and encrypt its backend, if necessary, to allow importing existing
configs.

This is also implemented internally in a lazy way, so that a password is
only prompted when the custody services methods are actually called,
allowing us to not need a password for view only commands.

Closes #4293.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > This is a client-only change.

---------

Co-authored-by: cratelyn <[email protected]>
conorsch pushed a commit that referenced this issue May 8, 2024
This adds a new option to encrypt the `soft-kms` and `threshold` custody
backends with a password, so that spend-key related material is
encrypted at rest. This is implemented by:

1. Having a `pcli init --encrypted` flag that applies to both of these
backends, which prompts a user for a password (and confirmation) before
using that to encrypt the config.
2. Having a `pcli init re-encrypt` command to read an existing config
and encrypt its backend, if necessary, to allow importing existing
configs.

This is also implemented internally in a lazy way, so that a password is
only prompted when the custody services methods are actually called,
allowing us to not need a password for view only commands.

Closes #4293.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > This is a client-only change.

---------

Co-authored-by: cratelyn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-client Area: Design and implementation for client functionality _P-V1 Priority: slated for V1 release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants