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: encrypt user credentials in Datastore #144

Merged
merged 4 commits into from
Apr 26, 2023

Conversation

itegulov
Copy link
Contributor

Fixes #134

aes-gcm is a pretty poor choice for this, but I needed something high-level due to time constraints. Let me know if there is a good alternative.

Ignoring nonce for now, but we should probably use it to salt the results

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.

I'm not sure if we are treating the cipher properly here. LGTM otherwise.

integration-tests/tests/lib.rs Show resolved Hide resolved
mpc-recovery/src/main.rs Show resolved Hide resolved
mpc-recovery/src/main.rs Outdated Show resolved Hide resolved
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.

LGTM, we probably don't need the verification part of aes-gcm, but we'll survive the overhead

@@ -40,6 +40,14 @@ You also need a Ed25519 key pair that you can generate by running `cargo run --

Now save it to GCP Secret Manager under the name of your choosing (e.g. `mpc-recovery-key-prod`). This name will be referred to as `GCP_SM_KEY_NAME`.

You also need to grab the AES cipher key that was printed after `Cipher 0:`; it should like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both this and the JSON private key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, one is for encryption, and the other is for multi-party stuff

@itegulov itegulov merged commit 75e9382 into develop Apr 26, 2023
@itegulov itegulov deleted the daniyar/credentials-encryption 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.

Encrypt user credentials
3 participants