feat: add persistent secret storage #396
Merged
Conversation
This file contains 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
Terraform Feature Environment (dev-396)Terraform Initialization ⚙️
|
|
Just deployed this branch to multichain0.testnet and it seems to be working fine. Example: |
volovyks
approved these changes
Jan 9, 2024
| @@ -64,6 +64,7 @@ pub struct ResharingContractState { | |||
|
|
|||
| #[derive(BorshDeserialize, BorshSerialize, Serialize, Deserialize, Debug)] | |||
| pub enum ProtocolContractState { | |||
| NotInitialized, | |||
There was a problem hiding this comment.
Why do we need one more status?
There was a problem hiding this comment.
It's a stop-gap for now, otherwise it's very hard to deploy multichain to GCP.
There was a problem hiding this comment.
I will document this behaviour
ppca
reviewed
Jan 9, 2024
| @@ -32,7 +32,7 @@ serde_json = "1" | |||
| testcontainers = { version = "0.15", features = ["experimental"] } | |||
| tokio = { version = "1.28", features = ["full"] } | |||
| tracing-subscriber = { version = "0.3", features = ["env-filter"] } | |||
| near-workspaces = "0.8.0" | |||
| near-workspaces = { git = "https://github.com/near/near-workspaces-rs.git", branch = "main" } | |||
There was a problem hiding this comment.
workspaces 0.8.0/0.9.0 depends on NEAR SDK 4.x which is no longer compilable due to a yanked dependency. See near/near-sdk-rs#1119.
So we have to use the unreleased version of workspaces for now
| @@ -118,6 +122,13 @@ impl CryptographicProtocol for GeneratingState { | |||
| public_key = hex::encode(r.public_key.to_bytes()), | |||
| "successfully completed key generation" | |||
| ); | |||
| ctx.secret_storage() | |||
There was a problem hiding this comment.
So if this storage attempt fails, the progress() will fail even tho it actually completed key generation. Is there a problem with that?
Also want to check the purpose of this change? Is it to have the nodes generate keys and upload themselves instead of us giving them keys?
There was a problem hiding this comment.
Not ideal for sure, but I would say storage being unavailable is a fatal error. If we can't store the private key share, then we wouldn't be able to recover it after reboot.
Also want to check the purpose of this change? Is it to have the nodes generate keys and upload themselves instead of us giving them keys?
So the idea is to be able to restart the node without loosing crucial information - current epoch, pre-agreed root public key and private key share specific for this node.
There was a problem hiding this comment.
If we can't store the private key share, then we wouldn't be able to recover it after reboot. -- Agree.
so when this progress() fails what happens next? Will progress() be tried again to generate and store the key share?
There was a problem hiding this comment.
Hmm, that's a good point. It might just make sense to panic here instead to signify that this is a fatal error. Otherwise progress will try to finish the protocol and save the result indefinitely.
Comment on lines
+105
to
+109
| name = "MPC_RECOVERY_GCP_PROJECT_ID" | ||
| value = var.project | ||
| } | ||
| env { | ||
| name = "MPC_RECOVERY_SK_SHARE_SECRET_ID" |
There was a problem hiding this comment.
should we rename these to be multichain instead of mpc-recovery?
There was a problem hiding this comment.
Mmm, not sure, maybe? I don't have a strong opinion so I just kept it as is for now
There was a problem hiding this comment.
Made a ticket for this one too: #407
| } | ||
|
|
||
| impl CryptographicCtx for &Ctx { | ||
| impl CryptographicCtx for &mut Ctx { |
There was a problem hiding this comment.
nothing to change here, but now that I think about it, why weren't we just passing around a concrete Ctx object instead of requiring specific *Ctx interfaces? Doesn't seem like we're getting much out of having traits for this purpose but maybe I'm missing something
There was a problem hiding this comment.
The initial idea was to be able to "mock" specific methods that are necessary for that component without having to instantiate other stuff. Not sure if that makes sense anymore as most of the methods are used everywhere
There was a problem hiding this comment.
yeah, I kept having to add new interface members to each of the traits, so probably need to clean this up later. made a ticket: #406
|
Merging now to unblock @volovyks |
Terraform Feature Environment Destroy (dev-396)Terraform Initialization ⚙️
|
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.
Hacked up something to store the private key share in GCP Secret Manager. Not tested yet