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: store user keys in GCP Datastore #103

Merged
merged 10 commits into from
Apr 24, 2023
Merged

Conversation

itegulov
Copy link
Contributor

This PR makes the leader node generate random private keys for users and store them in GCP Datastore. The code is quite hefty mainly because of the amount of boilerplate I had to write for protobuf (de)serialization.

  • Integration tests are using Google Cloud Emulator to emulate Datastore locally
  • Not tested in real world as we have not been given a real Datastore yet
  • Keys are being stored in plaintext for now but will be encrypted in a follow-up PR

@itegulov itegulov requested review from volovyks and DavidM-D April 23, 2023 06:43
Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks cool!

@@ -57,6 +54,12 @@ enum Cli {
default_value("https://api.kitwallet.app")
)]
account_lookup_url: String,
/// GCP project ID
#[arg(long, env("MPC_RECOVERY_GCP_PROJECT_ID"))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the same ID I have added in my PR? Let's delete it, if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's the GCP project id (currently we use pagoda-discovery-platform-dev). It's needed for integration tests where I am using an emulated GCP project with a different name.

mpc-recovery/src/key_recovery.rs Outdated Show resolved Hide resolved
@itegulov
Copy link
Contributor Author

Let's not merge this until Datastore is up on our GCP project so that we don't break the dev environment (assuming other teams still depend on it for testing their stuff).

Also, should probably come in after #97 as that is a more substantial change, and I want to minimize the number of conflicts to resolve.

@itegulov itegulov changed the title feat: store user keys in GCP Datastore feat: store user keys in GCP Datastore [DO NOT MERGE] Apr 23, 2023
@itegulov itegulov changed the title feat: store user keys in GCP Datastore [DO NOT MERGE] feat: store user keys in GCP Datastore Apr 24, 2023
@itegulov
Copy link
Contributor Author

@DavidM-D could you make sure that whatever I wrote in sign_node/mod.rs makes sense to you? Also, had to modify one of the integration tests you added, assuming you intended to aggregate public keys there.

Copy link
Contributor

@DavidM-D DavidM-D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sign node stuff looks good to me

Err(err) => {
tracing::error!(?err);
(
StatusCode::INTERNAL_SERVER_ERROR,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is probably a bad request, not an internal server error

mpc-recovery/src/sign_node/mod.rs Show resolved Hide resolved

let combined_pub = to_dalek_combined_public_key(ctx.pk_set).unwrap();
let account_id = get_test_claims().get_internal_account_id();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense, previously we were just checking against the node signatures, which were identical to the signing signatures

@DavidM-D DavidM-D merged commit aaf6953 into develop Apr 24, 2023
@itegulov itegulov deleted the daniyar/gcp-datastore branch July 20, 2023 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants