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

Allow invocation of single-share SLIP-39 backup #3636

Closed
andrewkozlik opened this issue Mar 21, 2024 · 3 comments · Fixed by #3710
Closed

Allow invocation of single-share SLIP-39 backup #3636

andrewkozlik opened this issue Mar 21, 2024 · 3 comments · Fixed by #3710
Assignees
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user protobuf Structure of messages exchanged between Trezor and the host

Comments

@andrewkozlik
Copy link
Contributor

andrewkozlik commented Mar 21, 2024

The Shamir backup workflow in Trezor currently requires the user to select the number of shares and the threshold on Trezor. We want to make it possible for Suite to bypass this process and invoke 1-of-1 SLIP-39 backup on Trezor. I see a simple solution and a more generic solution.

Solution 1

Add a new backup type Slip39_Single:

enum BackupType {
    Bip39 = 0;              // also called "Single Backup", see BIP-0039
    Slip39_Basic = 1;       // also called "Shamir Backup", see SLIP-0039
    Slip39_Advanced = 2;    // also called "Super Shamir" or "Shamir with Groups", see SLIP-0039#two-level-scheme
    Slip39_Single = 3;      // also called "Single Shamir", see SLIP-0039
}

If Slip39_Single, then the message handlers for BackupDevice and ResetDevice with skip_backup==false will go directly to the seed backup:

async def backup_seed(backup_type: BackupType, mnemonic_secret: bytes) -> None:
    ...
    elif backup_type == BAK_T_SLIP39_SINGLE:
        await _backup_slip39_single(mnemonic_secret)
    ...

async def _backup_slip39_single(encrypted_master_secret: bytes) -> None:
    identifier = storage_device.get_slip39_identifier()
    iteration_exponent = storage_device.get_slip39_iteration_exponent()
    if identifier is None or iteration_exponent is None:
        raise ValueError

    mnemonics = slip39.split_ems(1, [(1, 1)], identifier, iteration_exponent, encrypted_master_secret)[0]
    await layout.slip39_basic_show_and_confirm_shares(mnemonics)

Solution 2

Make it possible for Suite to specify the exact parameters of the Shamir backup. Extend BackupDevice with the following parameters:

message BackupDevice {
   optional uint32 group_threshold = 1;
   repeated uint32 member_thresholds = 2;
   repeated uint32 member_counts = 3;
}

The firmware will check that the following conditions are met:

  • All three fields must be present or all three must be absent.
  • The fileds must not be present if backup type is Bip39.
  • member_thresholds must be the same length as member_counts.

There is no need to do other sanity checks like group_threshold being at most the number of groups etc., because that is done by split_ems().

As an alternative:

message BackupDevice {
   optional uint32 group_threshold = 1;
   message Slip39Group {
       optional uint32 member_threshold = 1;
       optional uint32 member_count = 2;
   }
   repeated Slip39Group groups = 2;
}

Similar conditions as in the previous case need to be checked by the firmware.

The BackupDevice message handler goes directly to the backup process if the parameters are specified:

async def backup_seed(backup_type: BackupType, mnemonic_secret: bytes, group_threshold: int, groups: list[tuple[int, int]]) -> None:
    if group_threshold:
        await _backup_slip39(mnemonic_secret, group_threshold, groups)
        return;
    ...

async def _backup_slip39(encrypted_master_secret: bytes, group_threshold: int, groups: list[tuple[int, int]]):
    identifier = storage_device.get_slip39_identifier()
    iteration_exponent = storage_device.get_slip39_iteration_exponent()
    if identifier is None or iteration_exponent is None:
        raise ValueError

    # generate the mnemonics
    mnemonics = slip39.split_ems(
        group_threshold,
        groups,
        identifier,
        iteration_exponent,
        encrypted_master_secret,
    )

    # show and confirm individual shares
    if len(groups) == 1:
        await layout.slip39_basic_show_and_confirm_shares(mnemonics)
    else
        await layout.slip39_advanced_show_and_confirm_shares(mnemonics)
  • Should we go with the three-field representation or the Slip39Group sub-message?
  • Should we also put the parameters in ResetDevice? I feel we shouldn't, because if skip_backup==true, then it doesn't really make sense or we'd have to store the parameters in storage. Suite defers backup anyway.
  • Should we be pedantic about not allowing multiple groups when backup type was selected as Slip39_Basic? I don't think we should.
  • Should there be a confirmation screen on Trezor repeating the numbers of shares and thresholds before backup commences? I think it's not needed.

Further notes

  • When the Shamir backup is 1-of-1, we shouldn't be showing the Recovery share #1 and Check share #1, just Recovery seed and Check seed like we do in BIP-39 backup. Ideally even if the user manually configures 1-of-1, i.e. special handling in slip39_basic_show_and_confirm_shares() when len(shares) == 1, just show_share_words(share) and await _share_words_confirmed(None, share_words).
@andrewkozlik andrewkozlik self-assigned this Mar 21, 2024
@andrewkozlik andrewkozlik added core Trezor Core firmware. Runs on Trezor Model T and T2B1. protobuf Structure of messages exchanged between Trezor and the host feature Product related issue visible for end user labels Mar 21, 2024
@obrusvit
Copy link
Contributor

I think Solution 2 allows for more flexible backup usecases triggered by suite in the future.

@andrewkozlik
Copy link
Contributor Author

Meeting notes from 2024-03-26 with @matejcik and @Hannsek.

We will go with solution 2, because we want to allow the user to be able to configure their Shamir backup in Suite. However, we will maintain the option of configuring the Shamir backup parameters in Trezor as well. Thus users who consider the parameters of their backup to be sensitive information will not have to expose them to Suite.

  • Should we go with the three-field representation or the Slip39Group sub-message?

The Slip39Group sub-message

  • Should we also put the parameters in ResetDevice?

No.

  • Should we be pedantic about not allowing multiple groups when backup type was selected as Slip39_Basic?

No. If backup type was set to Slip39_Basic and Suite sends an advanced Shamir backup configuration, then accept the advanced configuration.

  • Should there be a confirmation screen on Trezor repeating the numbers of shares and thresholds before backup commences?

Yes. We need to let the user know the selected threshold anyway. A confirmation screen for advanced backup should also be implemented, but doesn't need to be neatly designed, because Suite does not support it anyway at the moment.

@Hannsek
Copy link
Contributor

Hannsek commented Apr 17, 2024

When a host request this single-share SLIP-39 backup. The flow should be different then standard (multi-share) SLIP-39 backup. The flow should be the same as during BIP-39 backup. See here. So no shares, no groups, no thresholds, different copy, …

cc @obrusvit @ibz

@ibz ibz closed this as completed in #3710 May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user protobuf Structure of messages exchanged between Trezor and the host
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants