feat(crypto): add secp256r1 variant to EcdsaCurve - #11573
Open
MRmarioruci wants to merge 13 commits into
Open
MRmarioruci wants to merge 13 commits into
MRmarioruci wants to merge 13 commits into
Conversation
### Why * The threshold crypto layer already implements ECDSA on NIST P-256. `EccCurveType` has a `P256` variant, `AlgorithmId` has `ThresholdEcdsaSecp256r1`, and the IDkg code paths are curve-generic, with no `todo!` or `unimplemented!` left on a P256 arm. * The public `EcdsaCurve` enum has a single variant, so a `secp256r1` key id cannot be named in the management canister API or represented in the registry. The curve is unreachable from outside the crypto layer. * P-256 is the curve the web platform standardised on: WebAuthn, TLS, VAPID web push (ES256) and OAuth DPoP. A canister cannot produce any of those signatures today, so each one needs a key held off-chain. ### What * `EcdsaCurve` gains `Secp256r1` in `rs/types/management_canister_types` and in the published `packages/ic-management-canister-types`, together with the Candid doc comment, the `u32` conversions that decode the `ic0.cost_sign_with_ecdsa` argument, and `FromStr`, which is what NNS proposals parse. * `From<EcdsaCurve> for AlgorithmId` in `rs/types/types/src/crypto.rs` maps the new variant to `AlgorithmId::ThresholdEcdsaSecp256r1`. This is the single conversion that hands a key id to the crypto layer. * `types.proto` gains `ECDSA_CURVE_SECP256R1 = 2`, so an `EcdsaKeyId` round-trips through the registry, plus the regenerated files. * Both `tests/ic.did` fixtures gain the variant. That is what the Candid equality tests compare the Rust enum against. Two values need confirming before this merges: `u32` = 1 and protobuf = 2. Each is the next free value in its own scheme, but the `u32` is normative because it is the System API ABI in the interface spec. The variant has to land in one commit. Adding it to either the proto or a types crate on its own breaks the other's exhaustive match, so no split compiles. This generates no key and makes no key id available. Enabling a key is a separate registry and governance step. `rs/tests/consensus/tecdsa` is deliberately not extended. `cast_ecdsa_key_id` casts into the published `ic-management-canister-types` through cdk-rs, so r1 system tests need that crate released first.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Exhaustive matches and hand-maintained Candid interfaces remain stale, causing compilation and interface-equality test failures.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds public secp256r1/P-256 support to ECDSA key identifiers and connects it to threshold crypto.
Changes:
- Adds
Secp256r1to public/private management types and Candid fixtures. - Maps the curve to its System API, protobuf, and crypto identifiers.
- Regenerates protobuf bindings and updates orchestrator test utilities.
File summaries
| File | Description |
|---|---|
rs/types/types/src/crypto.rs |
Maps r1 to its threshold algorithm. |
rs/types/management_canister_types/tests/ic.did |
Updates private Candid fixture. |
rs/types/management_canister_types/src/lib.rs |
Defines conversions and parsing. |
rs/protobuf/def/types/v1/types.proto |
Adds protobuf enum value. |
rs/protobuf/src/gen/types/types.v1.rs |
Regenerates types binding. |
rs/protobuf/src/gen/state/types.v1.rs |
Regenerates state binding. |
rs/protobuf/src/gen/registry/types.v1.rs |
Regenerates registry binding. |
rs/protobuf/src/gen/messaging/types.v1.rs |
Regenerates messaging binding. |
rs/protobuf/src/gen/crypto/types.v1.rs |
Regenerates crypto binding. |
rs/orchestrator/src/upgrade.rs |
Generates r1 test transcripts. |
packages/ic-management-canister-types/tests/ic.did |
Updates published Candid fixture. |
packages/ic-management-canister-types/src/lib.rs |
Exposes r1 and its ABI value. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The variant commit missed every consumer outside the type crates, because the sweep that scoped it excluded test files and searched only the lowercase `ecdsa_curve` spelling. `verify_signature` and `cast_ecdsa_key_id` in the tecdsa system-test utils match on `ic_management_canister_types_private::EcdsaCurve`, so both became non-exhaustive and that library stopped compiling. `verify_signature` gains a P-256 verifier mirroring the k1 one. `cast_ecdsa_key_id` cannot be completed: it casts into `ic_cdk_management_canister::EcdsaCurve`, which re-exports `ic-management-canister-types` 0.7.1 from crates.io and is secp256k1-only, so the arm is `unimplemented!` naming that blocker. `UpdateSubnetPayload` carries `Option<Vec<MasterPublicKeyId>>`, so the new variant reaches the Candid of the registry canister and of the engine controller, which forwards that payload. Their hand-maintained `.did` files are checked by `service_equal`, which fails with "Left side is missing secp256r1" until they declare it. `registry_test.did` too. `should_fail_if_unkown_ecdsa_curve` used protobuf value 2 as its unknown curve. That is now `ECDSA_CURVE_SECP256R1`, so the conversion succeeds and the test no longer panics. The sentinel moves to 3, which is how the Schnorr sibling already picks the first value past the last valid one. The published crate's changelog records the addition under `Unreleased`. `signer.did` and the management-canister bench `candid.did` are deliberately unchanged. Both canisters type their arguments with the crates.io `ic_cdk_management_canister`, so their interfaces stay secp256k1-only until that crate is released.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The public ABI value still requires external specification confirmation, and a newly added runtime path intentionally panics until a dependency release is available.
Review details
- Files reviewed: 20/21 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
`cup_compatibility_test` failed on the variant commit, in two independent ways. Both are fixed here, and the second is the interesting one. `derive(Hash)` omits the discriminant while an enum has a single variant and starts writing it as soon as a second one exists. `EcdsaCurve` reaches `payload_hash` through `IDkgPayload`, so adding the variant silently changed the hash of every block payload that names an ECDSA key, even though the serialized payload was byte-identical. A replica on the old version recomputes the old hash and rejects the block, which is a divergence during a rollout and not only a rollback problem. `EcdsaCurve` is therefore hashed by hand, with `Secp256k1` contributing nothing exactly as it did before. Separately, `ExhaustiveSet for EcdsaCurve` iterated the variants, so the generated CUP artifacts named `secp256r1` and the old version could not decode them at all. The variant is withheld until the rollout that generates an r1 key. Both were needed. Withholding the variant alone still failed the integrity check 5 times out of 5, and preserving the hash alone failed to decode with `Unable to convert 2 to an EcdsaCurve`. Verified against the parent commit's `types_test` binary: artifacts from this tree are byte-identical to the parent's under a pinned seed, and the parent binary deserializes them in 5 of 5 random-seed runs. The hand-written `Hash` needs the crypto team's sign-off, since `crypto_hash` asks for that on anything it hashes. It is also easy to undo by accident: a later `derive(Hash)` here would move consensus hashes with nothing to catch it except this test.
Until now the only thing catching a `derive(Hash)` put back on `EcdsaCurve` was `cup_compatibility_test`, a Bazel system test that needs the mainnet binaries and takes minutes. This pins the property directly: hashing `Secp256k1` writes no bytes, hashing `Secp256r1` writes some. Against the regression it fails immediately with `left: 8, right: 0`, the eight bytes being the derived discriminant.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to 50b8986. Security OverviewDetected Code Changes
|
|
✅ No security or compliance issues detected. Reviewed everything up to 50b8986. Security OverviewDetected Code Changes
|
ci/scripts/rust-lint.sh denies clippy::unseparated_literal_suffix.
eichhorl
reviewed
Sep 16, 2026
The previous marker byte was forgeable. `1_u8` for `Secp256r1` is
indistinguishable from a `name` whose first byte is `0x01`, so these two key
ids fed the hasher the identical stream `[01, 6b, 65, 79, 5f, 31, ff]`:
EcdsaKeyId { curve: Secp256r1, name: "key_1" }
EcdsaKeyId { curve: Secp256k1, name: "\u{1}key_1" }
That is a collision inside a type reaching `payload_hash`, so it goes.
No standalone `impl` on `EcdsaCurve` can fix this, because whatever byte it
picks the enclosing struct's next field can reproduce. Only the enclosing type
knows what follows the curve, so `EcdsaKeyId` hashes it and `EcdsaCurve`
implements no `Hash` at all, with the reasoning recorded on both. The marker is
now `0xfe`, which never begins a UTF-8 sequence and so cannot come out of a
`str`. The impl destructures `Self` exhaustively and binds `name` as `&str`, so
a new field or a `name` that stops being text fails to compile rather than
quietly weakening that argument.
`Secp256k1` still writes nothing, so existing hashes are unchanged: artifacts
are byte-identical to the pre-variant binary under a pinned seed, 48 of 48, and
round-trip through it on 5 of 5 random seeds.
Deriving `Hash` over both variants remains the right end state and needs its own
staged rollout.
`get_ecdsa_public_key_with_retries` validated every returned key as secp256k1, so an r1 key would have failed the check on a point that is perfectly valid on P-256. It already takes the `EcdsaKeyId`, so it now branches on the curve. Both verifiers are also named after their curve now, since `verify_ecdsa_signature` sitting next to `verify_ecdsa_secp256r1_signature` read as if it covered both.
`fake_master_public_key_ids_for_all_idkg_algorithms` skipped `ThresholdEcdsaSecp256r1`, so every consensus test built on it exercised one ECDSA curve only. Add the arm, with a `_with_curve` variant of each fake key id helper behind it; the existing no-argument helpers keep their signature and their secp256k1 result, so their 56 callers are untouched. The fourth key id is a second ECDSA one, and an ECDSA pre-signature costs two transcripts of payload capacity against a Schnorr one's single transcript. A round of the priority queue therefore went from 4 capacity to 6, and `test_pre_signatures_are_started_up_to_payload_capacity` asserted an even split across keys at a hardcoded capacity of 20. That is 5 whole rounds for three keys but 3 rounds and a remainder for four, and the remainder lands on one arbitrary key. Derive the capacity from the key set so the even split is a property of the allocator rather than of the constant.
Same facts, fewer words. 26 lines of comment become 15.
eichhorl
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
EccCurveTypehas aP256variant,AlgorithmIdhasThresholdEcdsaSecp256r1, and the IDkg code paths are curve-generic, with notodo!orunimplemented!left on a P256 arm.EcdsaCurveenum has a single variant, so asecp256r1key id cannot be named in the management canister API or represented in the registry. The curve is unreachable from outside the crypto layer.What
EcdsaCurvegainsSecp256r1inrs/types/management_canister_typesand in the publishedpackages/ic-management-canister-types, together with the Candid doc comment, theu32conversions that decode theic0.cost_sign_with_ecdsaargument, andFromStr, which is what NNS proposals parse.From<EcdsaCurve> for AlgorithmIdmaps the new variant toAlgorithmId::ThresholdEcdsaSecp256r1; that is the single conversion handing a key id to the crypto layer.types.protogainsECDSA_CURVE_SECP256R1 = 2so anEcdsaKeyIdround-trips through the registry, plus the regenerated files. Bothtests/ic.didfixtures gain the variant, which is what the Candid equality tests compare the Rust enum against.Two values need confirming before this merges:
u32= 1 and protobuf = 2. Each is the next free value in its own scheme, but theu32is normative because it is the System API ABI in the interface spec. The matching spec change is dfinity/developer-docs#394.The variant has to land in one commit. Adding it to either the proto or a types crate on its own breaks the other's exhaustive match, so no split compiles.
This generates no key and makes no key id available. Enabling a key is a separate registry and governance step.
rs/tests/consensus/tecdsais deliberately not extended.cast_ecdsa_key_idcasts into the publishedic-management-canister-typesthrough cdk-rs, so r1 system tests need that crate released first, andverify_signatureneeds a P-256 verifier. Happy to follow up with that once the crate is out.Tests
cargo check --tests, the 39 unit tests and bothcandid_equality_tests pass onic-management-canister-types,ic-management-canister-types-private,ic-typesandic-protobuf.cargo run -p ic-protobuf-generatorreproduces the five generated files exactly.Two gaps CI needs to cover, since neither is reachable on macOS:
rs/orchestratordoes not build there (libc::gettid,SYS_futex,libcryptsetup-rs), so its test arm is unverified, and a match in any other Linux-only crate would not have shown up locally.Unrelated, but worth flagging: running the protobuf generator also rewrites doc-comment escaping in six generated files it does not otherwise touch (
\<=becomes<=, list renumbering). That looks like prost-build 0.14.3 disagreeing with whatever version produced the committed files. Reverted here, but the next regen will hit it.