chore: ensure blstrs scalar Copy types are zeroized

[gilcu3]

Closes #2359

Notice during the implementation the issue was modified accordingly, as the original finding was not fully accurate

[claude]

Pull request overview

Addresses audit finding RG-F1209 / #2359: in the CKD protocols the signing share was wrapped in Zeroizing after being copied out of a KeygenOutput that was only borrowed, so the wrapper cleared a copy while the original stayed live in the caller for the whole protocol run (including across the network round of the coordinator). The internal CKD helpers now take KeygenOutput by value and drop it as soon as the share and public key have been copied out, and the share is held in the volatile-write ScalarWrapper of this crate rather than relying on the Zeroize impl of SigningShare. The Copy-semantics caveat is now documented at module level, matching the third acceptance criterion of the issue.

Changes:

• do_ckd_participant / do_ckd_coordinator / compute_signature_share in both CKD variants take key_pair: KeygenOutput by value; the public ckd / ckd_pv signatures are unchanged, so no call sites move.
• compute_signature_share copies the share into Zeroizing<ScalarWrapper>, then drop(key_pair) before any further work — for the coordinator this means the share no longer lives in the async frame across recv_from_others.
• protocol_pv::do_ckd_coordinator copies public_key out before the move, since it is needed again in aggregated_output_check after the await.
• scalar_wrapper gains a module doc stating that zeroization is best-effort under Copy, plus a zeroize() unit test.

Reviewed changes

Per-file summary

File	Description
crates/threshold-signatures/src/confidential_key_derivation/protocol.rs	key_pair by value through the participant/coordinator/share paths; share wrapped in ScalarWrapper; hash_point computed before an explicit drop(key_pair).
crates/threshold-signatures/src/confidential_key_derivation/protocol_pv.rs	Same by-value + early-drop change; public_key copied out for the post-await aggregated_output_check.
crates/threshold-signatures/src/confidential_key_derivation/scalar_wrapper.rs	Module doc on best-effort zeroization; module-level allow(non_snake_case) on the test module; new zeroize() test.

I checked that the RNG draw order is unchanged (hash_app_id_with_pk consumes no randomness), so the reordering in protocol.rs:173-186 cannot shift the test_ckd snapshot — consistent with no .snap churn in the diff. drop(key_pair) lands before the await of the coordinator in both variants, which is the part of this change that actually shortens the lifetime of the share.

Findings

Blocking (must fix before merge):

• crates/threshold-signatures/src/confidential_key_derivation/scalar_wrapper.rs:137 — the only new test exercises ScalarWrapper::zeroize, which is pre-existing behavior: revert every protocol-side change in this PR and scalar_wrapper__should_be_zero_after_zeroize still passes, while docs/engineering-standards.md §Add tests requires a test that would fail if the change were reverted. More importantly, nothing verifies the assumption that the new drop(key_pair) rests on — that zeroization of SigningShare<BLS12381SHA256> actually clears a blstrs::Scalar, a type that does not implement Zeroize and is exactly the class of silent no-op that Zeroizing wrapper on Copy type only zeroizes one copy in CKD protocol #2359 is about. SigningShare<BLS12381SHA256>: Zeroize holds (the pre-PR Zeroizing::new(key_pair.private_share) proves it), so this is directly assertable — as a test in the same module:

  fn signing_share__should_be_zero_after_zeroize() {
      // Given
      let mut rng = MockCryptoRng::seed_from_u64(42);
      let mut share = SigningShare::new(blstrs::Scalar::random(&mut rng));
      assert_ne!(share.to_scalar(), blstrs::Scalar::ZERO);
  
      // When
      share.zeroize();
  
      // Then
      assert_eq!(share.to_scalar(), blstrs::Scalar::ZERO);
  }

  (SigningShare is already re-exported as crate::confidential_key_derivation::SigningShare.) Without it, drop(key_pair) is an unverified claim about a third-party generic impl over a Copy scalar.

Non-blocking (nits, follow-ups, suggestions):

• crates/threshold-signatures/src/confidential_key_derivation/protocol.rs:178 and protocol_pv.rs:194 — the comment says that taking the pair by value "is what lets its ZeroizeOnDrop run instead of leaving the share live in the caller". That is not accurate: before this PR the caller (run_ckd_protocol) already owned key_pair by value, so ZeroizeOnDrop did run — just at the end of the future rather than before the network round. The comment also spends three lines partly restating drop() and pointing at the caller, which docs/engineering-standards.md §Write helpful code comments items 1 and 4 flag. Suggest one line stating the actual reason, e.g. // Dropped before the network round so the share does not sit in the frame of the future.
• crates/threshold-signatures/src/confidential_key_derivation/scalar_wrapper.rs:5 — "secrets are wrapped here to get a volatile overwrite on drop" only holds when the wrapper is itself inside Zeroizing: ScalarWrapper implements Zeroize, not ZeroizeOnDrop, and ciphersuite.rs:263 builds a bare [ScalarWrapper; 1] that is dropped without any overwrite. Either scope the sentence to Zeroizing, or add impl ZeroizeOnDrop for ScalarWrapper (with the corresponding Drop) so the claim holds for every use — no current usage destructures the wrapper, only .0 copy-reads, so the Drop impl would be non-breaking.
• crates/threshold-signatures/src/confidential_key_derivation/protocol_pv.rs:49 — "Copied out because key_pair is moved into compute_signature_share" restates what the borrow checker already enforces; the non-obvious part is that public_key is needed after the await by aggregated_output_check. Either say that or drop the comment.
• crates/threshold-signatures/src/confidential_key_derivation/protocol.rs:163 — "Zeroization of the secrets below is best-effort" reads as a comment about the following statements rather than about the item, and is duplicated verbatim in protocol_pv.rs:182; a // comment at the top of the body would fit better.

⚠️ Issues found

[claude]

PR title type suggestion: This PR adds new security-critical functionality to ensure cryptographic scalar types are properly zeroized. Since the changes add new source code with significant implementation logic (not routine maintenance), the type should probably be fix: (if addressing a security gap) or feat: (if adding new functionality) rather than chore:.

Suggested title: fix: ensure blstrs scalar Copy types are zeroized or feat: add zeroization wrapper for blstrs scalar types