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

feat(crypto): CRP-2609 Introduce master key ID variant for vetKD #2108

Merged
merged 16 commits into from
Oct 29, 2024

Conversation

fspreiss
Copy link
Member

@fspreiss fspreiss commented Oct 17, 2024

Adds a variant for vetKD in the MasterPublicKeyId in the

  1. registry protobuf definitions,

    where it is used in the KeyConfig (which is used in the ChainKeyConfig, which is used in the SubnetRecord).

  2. management canister types,

    where it is used in the ComputeInitialIDkgDealingsArgs (which will be generalized in a later step, e.g., to ReshareChainKeyArgs).

  3. registry canister API (registry.did),

    where it is used in the KeyConfig (which is used in the KeyConfigRequest, which is used in the InitialChainKeyConfig, which is used in CreateSubnetPayload and RecoverSubnetPayload), and in the UpdateSubnetPayload's chain_key_signing_enable/chain_key_signing_disable.

This is the very first step for the vetKeys feature.

As discussed with the Consensus team, various consensus tests related to pre-signatures are adapted so that the tests panic if the vetKD variant is used, because vetKD doesn't have pre-signatures.

Registry API backwards-compatibility

The changes in the registry canister API should be safe/backwards-compatible based on the following argumentation (see Can I add a variant alternative?):

The MasterPublicKeyId variant in registry.did is extended with a new alternative as follows:

- type MasterPublicKeyId = variant { Schnorr : SchnorrKeyId; Ecdsa : EcdsaKeyId };
+ type MasterPublicKeyId = variant { Schnorr : SchnorrKeyId; Ecdsa : EcdsaKeyId; VetKd : VetKdKeyId };

This variant is used in two places:

  1. type KeyConfig = record {
       key_id : opt MasterPublicKeyId;
       [...]
    };
    
    Here, adding the alternative is safe, given that the variant field key_id is optional.
  2. type UpdateSubnetPayload = record {
       [...]
       chain_key_signing_enable : opt vec MasterPublicKeyId;
       chain_key_signing_disable : opt vec MasterPublicKeyId;
       [...]
    }
    
    Although chain_key_signing_enable and chain_key_signing_disable are not (directly) optional variant fields (because they are contained in a vec), adding the alternative is safe, given that the MasterPublicKeyId variant is only ever used (as part of UpdateSubnetPayload) in method arguments but never as method result.

This argumentation is also supported by the fact that the //rs/registry/canister:registry-canister_did_git_test test passes.

Registry protobuf backwards-compatibility

The changes in the registry protocol buffer definitions is backwards-compatible. On the one hand, adding a new field to a oneof is generally backward-compatible in Protocol Buffers (with some nuances that are not relevant here). On the other hand, the new field/variant in the MasterPublicKeyId is not yet written by any code, and this new variant won't be written for many weeks to come. In particular, it will only be written to the registry once subnets will actually hold master public keys for vetKD, which will only happen in 2025.

Crypto Algorithm ID

For the time being, we don't need a particular AlgorithmId for vetKD, so we use the AlgorithmId::Placeholder. In any case, the exact algorithm also doesn't matter for now because neither the intended Crypto APIs will use an algorithm ID nor will this algorithm be persisted anywhere (e.g., in the registry or in a key store).

@github-actions github-actions bot added the feat label Oct 17, 2024
@fspreiss fspreiss added the CI_COVERAGE Run bazel coverage and upload report to S3 label Oct 22, 2024
@fspreiss fspreiss removed the CI_COVERAGE Run bazel coverage and upload report to S3 label Oct 23, 2024
@fspreiss fspreiss marked this pull request as ready for review October 24, 2024 08:13
@fspreiss fspreiss requested review from a team as code owners October 24, 2024 08:13
rs/types/management_canister_types/src/lib.rs Outdated Show resolved Hide resolved
rs/registry/canister/tests/common/test_helpers.rs Outdated Show resolved Hide resolved
rs/registry/canister/canister/registry.did Show resolved Hide resolved
rs/consensus/src/idkg/signer.rs Outdated Show resolved Hide resolved
rs/consensus/src/idkg/test_utils.rs Outdated Show resolved Hide resolved
@fspreiss fspreiss requested a review from max-dfinity October 29, 2024 10:33
@fspreiss fspreiss added this pull request to the merge queue Oct 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 29, 2024
@fspreiss fspreiss added this pull request to the merge queue Oct 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 29, 2024
@fspreiss fspreiss added this pull request to the merge queue Oct 29, 2024
Merged via the queue into master with commit 39f67a6 Oct 29, 2024
24 checks passed
@fspreiss fspreiss deleted the franzstefan/CRP-2609-introduce-vetkd-master-key-id branch October 29, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants