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

pcli threshold interface is unnecessarily brittle #4335

Closed
hdevalence opened this issue May 6, 2024 · 3 comments
Closed

pcli threshold interface is unnecessarily brittle #4335

hdevalence opened this issue May 6, 2024 · 3 comments
Labels
needs-refinement unclear, incomplete, or stub issue that needs work

Comments

@hdevalence
Copy link
Member

Describe the bug

Using the DKG through pcli involves copy-pasting JSON messages between the participants. We chose this design for two reasons:

  1. It's minimally opinionated about the method used to communicate the data between participants (slack/signal/magic-wormhole/etc)
  2. It avoids needing to write any intermediate state to disk, which could open up the possibility of replay attacks, where the same secret state is improperly reused between ceremonies.

However, the way this is currently implemented causes a terrible user experience:

we are struggling with the pcli dkg flow, we are on attempt 7 now I think.
Hitting enter twice accidentally, accidentally typing and it won't allow backspace, copied with incorrect quotes
With any of those it panics and you have to start the whole ceremony over

We do want to have the behavior of "abort on failure" causing a ceremony restart, without the possibility of improperly reusing any state. But, if we could distinguish between errors caused by copy paste problems and errors caused by a cryptographic adversary, we could substantially improve the UX here.

To do this, we should change the code that reads a single message to retry on any parse failure. This handles issues like #4260 as a side effect, and cannot leak any secrets (because parsing the message does not involve secret data).

@cratelyn
Copy link
Contributor

cratelyn commented May 7, 2024

cross-referencing #4343, which makes some improvements re: hitting enter twice.

@hdevalence
Copy link
Member Author

A common pattern in the threshold code seems to be code like

            for _ in 1..self.config.threshold() {
                let reply_str = self
                    .terminal
                    .next_response()
                    .await?
                    .ok_or(anyhow!("expected round1 reply"))?;
                let reply = from_json::<sign::FollowerRound1>(&reply_str)?;
                acc.push(reply);
            }

This would be cumbersome to add retry logic to in every place. I'm wondering if instead we could change next_response to take a generic type parameter, and bundle the from_json into it internally, so that it waits to successfully decode a message of the expected type and only then yields back to the caller.

hdevalence added a commit that referenced this issue May 8, 2024
I implemented the suggestion in #4335 and then tested running a ceremony in
three split-pane terminal windows for maximum confusion about which messages go
where.  The resulting code is robust to mistaken entry, ensuring:

- messages are validly parseable (so cannot mix round 1 and round 2, and resilient to typos)
- messages are not duplicated (so pasting twice does not kill the ceremony)
- messages are not self-broadcast (so pasting our own message is ignored)
@hdevalence hdevalence mentioned this issue May 8, 2024
1 task
hdevalence added a commit that referenced this issue May 8, 2024
I implemented the suggestion in #4335 and then tested running a ceremony in
three split-pane terminal windows for maximum confusion about which messages go
where.  The resulting code is robust to mistaken entry, ensuring:

- messages are validly parseable (so cannot mix round 1 and round 2, and resilient to typos)
- messages are not duplicated (so pasting twice does not kill the ceremony)
- messages are not self-broadcast (so pasting our own message is ignored)
hdevalence added a commit that referenced this issue 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
@cronokirby
Copy link
Contributor

Closed by #4352

@github-project-automation github-project-automation bot moved this from Backlog to Done in Penumbra May 8, 2024
hdevalence added a commit that referenced this issue 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
conorsch pushed a commit that referenced this issue 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
needs-refinement unclear, incomplete, or stub issue that needs work
Projects
Archived in project
Development

No branches or pull requests

3 participants