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 an encrypted config option to pcli #4343

Merged
merged 13 commits into from
May 8, 2024
Merged

Conversation

cronokirby
Copy link
Contributor

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

  • 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.

@cratelyn cratelyn added this to the Sprint 6 milestone May 7, 2024
@cratelyn cratelyn self-requested a review May 7, 2024 14:44
@cratelyn cratelyn added A-client Area: Design and implementation for client functionality C-enhancement Category: an enhancement to the codebase labels May 7, 2024
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small details, this looks like excellent work, @cronokirby!

crates/custody/src/encrypted.rs Outdated Show resolved Hide resolved
crates/custody/src/encrypted.rs Outdated Show resolved Hide resolved
crates/custody/src/encrypted.rs Show resolved Hide resolved
crates/custody/src/encrypted.rs Show resolved Hide resolved
crates/custody/src/encrypted.rs Outdated Show resolved Hide resolved
crates/custody/src/encrypted.rs Outdated Show resolved Hide resolved
crates/bin/pcli/src/terminal.rs Outdated Show resolved Hide resolved
@cronokirby
Copy link
Contributor Author

Addressed these suggestions in the latest commit

Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks for handling the extra fixes!

@cratelyn
Copy link
Contributor

cratelyn commented May 7, 2024

i believe this can also be marked as addressing #4260, via this change to the user input logic.

@hdevalence
Copy link
Member

I think if we're going to change the user input logic we should just have it retry on parse failure rather than handle empty strings specially, so we can close out #4335

@conorsch
Copy link
Contributor

conorsch commented May 7, 2024

Given that we plan to point-release this fix, please make sure to squash it down into a single commit, to ensure that cherry-picking doesn't miss anything. Refs #4345.

cronokirby and others added 13 commits May 7, 2024 19:29
This needs to be done because the new encrypted custody backend will
also make use of the trait, so it needs to be moved outside of that
particular module.
This means that you don't need to enter passwords unless custody methods
are actually called
Co-authored-by: cratelyn <cratelyn@users.noreply.github.com>
This prevents them from changing without us realizing it, since we want
backwards compatability.
@hdevalence hdevalence force-pushed the 4293-pcli-encrypted-config branch from 1af06e9 to 0349055 Compare May 8, 2024 02:29
@hdevalence
Copy link
Member

I rebased this to resolve a spurious conflict but I'm a little unclear on the status -- is this good to merge?

@hdevalence
Copy link
Member

We should not try to solve #4335 as part of this, looking at the threshold code it turns out to be somewhat involved

@hdevalence hdevalence mentioned this pull request May 8, 2024
1 task
Copy link
Member

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@redshiftzero redshiftzero merged commit dff2442 into main May 8, 2024
13 checks passed
@redshiftzero redshiftzero deleted the 4293-pcli-encrypted-config branch May 8, 2024 15:07
@cronokirby
Copy link
Contributor Author

cronokirby commented May 8, 2024

This was all good to merge last evening, I just wanted to address the hard coded argon2 params, and to extend encryption to governance subkeys.

Those were all included, so.

hdevalence added a commit that referenced this pull request May 8, 2024
## Describe your changes

Improves UX for the DKG ceremony based on user feedback, so we can
actually tell people to use it.

This is a PR on top of #4343 to make it as easy as possible to merge.

## Issue ticket number and link

#4335

## 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:

  > client-side key generation logic only
hdevalence pushed a commit that referenced this pull request 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 <cratelyn@users.noreply.github.com>
hdevalence added a commit that referenced this pull request May 8, 2024
## Describe your changes

Improves UX for the DKG ceremony based on user feedback, so we can
actually tell people to use it.

This is a PR on top of #4343 to make it as easy as possible to merge.

## Issue ticket number and link

#4335

## 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:

  > client-side key generation logic only
@hdevalence hdevalence mentioned this pull request May 8, 2024
1 task
conorsch pushed a commit that referenced this pull request 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 <cratelyn@users.noreply.github.com>
conorsch pushed a commit that referenced this pull request May 8, 2024
## Describe your changes

Improves UX for the DKG ceremony based on user feedback, so we can
actually tell people to use it.

This is a PR on top of #4343 to make it as easy as possible to merge.

## Issue ticket number and link

#4335

## 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:

  > client-side key generation logic only
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 C-enhancement Category: an enhancement to the codebase
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add encrypted configs to pcli
5 participants