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

Seed Import from json file #109

Merged
merged 16 commits into from
Jun 13, 2024
Merged

Seed Import from json file #109

merged 16 commits into from
Jun 13, 2024

Conversation

matthme
Copy link
Collaborator

@matthme matthme commented Jun 12, 2024

Adds the backend functionality to import seeds from a JSON file as defined in this PR.

@matthme matthme requested review from mrruby, zo-el and neonphog June 12, 2024 13:30
napi::Error::from_reason(format!("Failed to encrypt the device_bundle_seed: {}", e))
})?;

let src_tag = format!("imported#{initial_host_pub_key_b64_derived}");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this tag make sense? Or might there be a reason to pick another tag?

nonce,
cipher,
src_tag.clone().into(),
false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Might there be a reason for having the seed be exportable? And are there any trade-offs to having it exportable (e.g. security-wise)?

Choose a reason for hiding this comment

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

Malicious actors will have a more difficult time extracting the private key if it is not exportable. I can't think of a use-case for needing to export this. Users should go back to their root seeds. If there is a use case, it doesn't change the security landscape that much, so this can be changed to true.

/// Reads a json file containing the device bundle and the device derivation path,
/// then derives the device seed and from it a sub seed at index 1 whose base64 encoded
/// public part should equal the initial_host_pub_key field.
pub async fn derive_and_import_seed_from_json_file(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the relevant method. The zome_call_signing stuff is only part of this PR because the file got renamed.

.map_err(|e| {
napi::Error::from_reason(format!("Failed to get public key: {:?}", e))
})?,
secret: SecretKey::from_bytes(&*derived_seed.get_seed().read_lock()).map_err(

Choose a reason for hiding this comment

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

Here you are taking the secret out of protected memory, and placing it into unprotected memory. The protected memory struct can be passed directly into crypto_box_xsalsa_by_pub_key so it never has to be unprotected.

Choose a reason for hiding this comment

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

But, I guess if you need it for the dalek library anyway, you can keep this as-is. Another option, though, would be to use sodoken directly instead of dalek.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, thanks. I think I can do that, it's only the public key that I would ideally want to have in plain format to be able to compare with what's expected before importing it.

Choose a reason for hiding this comment

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

It looks like LairClient currently can't accept a sodoken buffer directly, so I guess you'll need to leave this as-is unless/until we update that api to be more flexible.

Copy link

@neonphog neonphog left a comment

Choose a reason for hiding this comment

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

In general, this looks good to me 👍

@matthme matthme merged commit a49bc04 into develop Jun 13, 2024
@matthme matthme deleted the feat/seed-import branch October 8, 2024 09:39
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