Conversation
Consolidated Tests Results 2026-04-16 - 17:33:29Test Results
Details
Flaky Testsclient::tests::threshold::public_decryption_tests::test_decryption_threshold::case_1 test-reporter: Run #4006
🎉 All tests passed!TestsView All Tests
Flaky Tests
Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the custodian backup/recovery flow to carry an explicit MPC context identifier so recovery artifacts can be associated with the correct operator verification key when multiple signing keys exist. It also refactors S3 anonymous access helpers (including region inference) and updates tests/docs accordingly.
Changes:
- Add
mpc_context_idtoNewCustodianContextRequestand propagate it through recovery material creation and custodian recovery outputs. - Tighten public decryption response validation around server verification keys (uniqueness/matching) and adjust related tests.
- Move/centralize S3 region inference + update anonymous S3 client construction call sites (core + core-client).
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/guides/core_client.md | Updates manual test instructions; adds required --mpc_context_id and a key filename warning. |
| core/service/tests/integration_test.rs | Updates integration test setup for renamed custodian context field and new mpc_context. |
| core/service/tests/backward_compatibility_kms.rs | Updates backward-compat tests to include the new mpc_context/mpc_context_id fields. |
| core/service/src/vault/storage/s3.rs | Changes anonymous client builder signature; adds find_region_from_s3_url; adds tests (but currently contains unresolved conflict markers). |
| core/service/src/vault/keychain/secretsharing.rs | Updates tests for renamed custodian context field and new RecoveryValidationMaterial::new signature. |
| core/service/src/engine/validation_non_wasm.rs | Refactors public decrypt response validation to validate uniqueness by deserializing server keys; updates test helpers/calls. |
| core/service/src/engine/threshold/service/reshare_utils.rs | Uses S3 helpers from vault::storage::s3 and updates anonymous client construction. |
| core/service/src/engine/context_manager.rs | Requires mpc_context_id for new custodian contexts; threads it through validation material generation; updates tests. |
| core/service/src/engine/backup_operator.rs | Threads MPC context through recovery request generation by reading it from stored validation material. |
| core/service/src/client/tests/threshold/custodian_context_tests.rs | Updates client test helper to pass an MPC context id. |
| core/service/src/client/tests/threshold/custodian_backup_tests.rs | Updates reencryption verification calls to include mpc_context_id. |
| core/service/src/client/tests/centralized/custodian_context_tests.rs | Updates client test helper to pass an MPC context id. |
| core/service/src/client/tests/centralized/custodian_backup_tests.rs | Updates reencryption verification calls to include mpc_context_id. |
| core/service/src/client/custodian_context.rs | Updates request builder for renamed proto field and adds mpc_context_id. |
| core/service/src/bin/kms-custodian.rs | Adds CLI flag --mpc_context_id and passes it into custodian reencryption verification (currently uses panics on invalid input). |
| core/service/src/backup/tests.rs | Updates backup tests to pass mpc_context_id into reencryption + validation material creation. |
| core/service/src/backup/operator.rs | Extends RecoveryValidationMaterialPayload with mpc_context. |
| core/service/src/backup/custodian.rs | Extends InternalCustodianRecoveryOutput and grpc conversions with mpc_context_id; updates custodian context field name. |
| core/grpc/proto/kms.v1.proto | Renames custodian context id field; renames new_context field; adds mpc_context_id and recovery output field. |
| core-client/tests/integration/integration_test.rs | Updates core-client integration test to pass mpc_context_id when creating custodian context. |
| core-client/tests/integration/integration_test_isolated.rs | Same as above for isolated tests. |
| core-client/src/s3_operations.rs | Refactors key fetching to use S3Storage + shared S3 helper utilities. |
| core-client/src/lib.rs | Adds mpc_context_id CLI arg for the NewCustodianContext command and parses it as a ContextId. |
| core-client/src/backup.rs | Updates custodian context creation RPC to include mpc_context_id; enforces all recovery outputs share the same MPC context id. |
Comments suppressed due to low confidence (1)
core/service/src/engine/validation_non_wasm.rs:589
validate_public_decrypt_responsesno longer checkstrusted_ctx.extra_dataagainstcur_resp.extra_datawheneip712_domainisNone. This makesPublicDecTrustedValidationContext::extra_dataeffectively unenforced in the non-EIP712 path and can allow responses with unexpectedextra_datato be accepted. Ifextra_datais meant to be part of the request/response binding outside of EIP-712, reintroduce the explicit mismatch check (or include it in the signed message) for the non-EIP712 case.
// Validate that all the responses agree with the pivot on the static parts of the
// response
let eip712_params = trusted_ctx
.eip712_domain
.map(|domain| Eip712VerificationParams {
response_external_signature: &cur_resp.external_signature,
response_extra_data: &cur_resp.extra_data,
trusted_eip712_domain: domain,
});
if !validate_public_decrypt_meta_data(
&cur_verf_key,
trusted_ctx.ext_handles_bytes,
&pivot_payload,
cur_payload,
&cur_resp.signature,
eip712_params.as_ref(),
)? {
tracing::warn!("Some server did not provide the proper response!");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jot2re
changed the title
dd23
reviewed
Apr 9, 2026
dd23
reviewed
Apr 9, 2026
dd23
reviewed
Apr 9, 2026
Member
|
From a review that I did locally with Codex (and that I didn't have the time to check to the full extent):
I think the TL;DR is: It seems we're trying to verify an old backup with a new key, which seems like an issue indeed. We should check this. |
kc1212
reviewed
Apr 9, 2026
Contributor
kc1212
left a comment
kc1212
left a comment
There was a problem hiding this comment.
There seem to be something weird going on with the verification
Collaborator
Author
Contributor
you mean public decryption right? because the simplification is in validation_non_wasm |
dd23
reviewed
Apr 15, 2026
kc1212
reviewed
Apr 15, 2026
Contributor
kc1212
left a comment
kc1212
left a comment
There was a problem hiding this comment.
My main question is on the mpc_context_id, otherwise LGTM!
Member
|
Codex flagged the following:
I tried to make sense of if, but I'm not sure if it's an actual issue. Maybe you can have a look @jot2re |
Collaborator
Author
From how I understand Codex here, it looks like the issue is exactly the thing we talked about last night; that we now assume signing keys never change (at least not without making a new context that is deployed by a new conceptual party). For this reason a backup must be redone when we launch such a new party and old backups won't work, since they are conceptually done by another party. However, I find Codex comment a bit weird and partially untrue; we don't store the MPC context as part of any objects used in custodian backup. We only store the MPC context ID. I guess part of it's complaint is that we do not use it when recovering, but I cannot see why would need it? I guess if we want to be pedantic we could validate that the verification key fetched does indeed match the one of the context, after having recovered. But tbh I don't think we need to do these extra, restrictive checks, in code. We have always found that checks that aren't strictly needed, could hinder us when experimenting with new features. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 109 out of 120 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kc1212
approved these changes
Apr 17, 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.
Description of changes
This PR cherry picks changes from this PR (where we wanted to implement support for multiple signing keys). More specifically the following changes and enhancements and carried over into this PR:
SessionMakernow loads all MPC contexts on init, ensuring that the default context is properly loaded.Issue ticket number and link
PR Checklist
I attest that all checked items are satisfied. Any deviation is clearly justified above.
chore: ...).TODO(#issue).unwrap/expect/paniconly in tests or for invariant bugs (documented if present).devopslabel + infra notified + infra-team reviewer assigned.!and affected teams notified.Zeroize+ZeroizeOnDropimplemented.unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.Dependency Update Questionnaire (only if deps changed or added)
Answer in the
Cargo.tomlnext to the dependency (or here if updating):More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md