-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add pubkey importing / seed generation #172
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment thus far, but awesome to see this @mriise! We should also have this PR use the setting upon swarm startup, right?
Codecov Report
@@ Coverage Diff @@
## main #172 +/- ##
==========================================
- Coverage 47.97% 47.62% -0.35%
==========================================
Files 50 50
Lines 2193 2236 +43
Branches 539 557 +18
==========================================
+ Hits 1052 1065 +13
- Misses 817 834 +17
- Partials 324 337 +13
|
To the CI errors, @mriise if you install the pre-commit hook and such, you'll get the cargo fmt stuff for free too. LMK if you want to chat through that or the nix flake. |
homestar-runtime/src/settings.rs
Outdated
}; | ||
// TODO convert err | ||
let pem = pem::parse(pem_file).unwrap(); | ||
// we only take ed25519 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must we only take ed25519
btw? We could support others, i.e. https://libp2p.github.io/rust-libp2p/libp2p_identity/index.html? Maybe this is part of the configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can support Secp256k1 keys as it will likely be need for blockchains, but I would prefer to stay away from RSA. Am neutral on the random variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Good with that/those choice(es)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added support for secp256k1, however importing is a bit weird. for the moment i am just importing a raw key bytes in PEM format, but it should probably be pkcs#8 or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some super minor comments @mriise, but this is shaping up really nicely.
[node] | ||
|
||
[node.network.keypair_config] | ||
# `openssl ecparam -genkey -name secp256k1 -outform DER -o secp256k1_key.der` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so great to have the docs/comments here!
homestar-runtime/src/settings.rs
Outdated
match key_type { | ||
KeyType::Ed25519 => { | ||
identity::Keypair::ed25519_from_bytes(new_key).map_err(|e| { | ||
anyhow!("Failed to generate ed25519 key from random: {:?}", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: {:#?}
instead of {:?}
will pretty-print debug logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we can pretty print these. though if we are going to be showing these errors to a lib that displays errors better in general like https://docs.rs/color-eyre/latest/color_eyre/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mriise yep, we'll prob bring in color_eyre for local dev. Nonetheless, this is more so for logs that will be picked up by grafana/other-simialr tool (or logging service), especially as we use logfmt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @mriise. I had a super minor comment on the tests below, i.e. assert vs unwrap.
With that, this is mergeable! => First PR in the books!!!!
homestar-runtime/src/settings.rs
Outdated
fn import_secp256k1_key() { | ||
let settings = Settings::build("fixtures/settings-import-secp256k1.toml".into()).unwrap(); | ||
|
||
settings.node.network.keypair_config.keypair().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor:
settings.node.network.keypair_config.keypair().unwrap(); | |
assert!(settings.node.network.keypair_config.keypair().is_ok()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic on unwrap arguably give better info on failure than an assert of is_ok
here since the unwrap will print out the actual error and is_ok
will just print the bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mriise thinking about it, then expect
is what we'd want (as it will also print the actual error, and the context for the error given).
homestar-runtime/src/settings.rs
Outdated
fn seeded_secp256k1_key() { | ||
let settings = Settings::build("fixtures/settings-random-secp256k1.toml".into()).unwrap(); | ||
|
||
settings.node.network.keypair_config.keypair().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
|
||
match key_type { | ||
KeyType::Ed25519 => { | ||
const PEM_HEADER: &str = "PRIVATE KEY"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: I tend to put consts up top, though doesn't matter much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't want it conflated when we have the secp PEM headers, so would pref to keep as is.
06313f3
to
20a80a9
Compare
… not specified in config
…r function & warn about filesystem access.
Description
Add support for importing ed25519 keypairs and generating a new keypair from seed bytes.
Link to issue
#163
Type of change
Test plan (required)
TODO
Demonstrate the code is solid. Which commands did you test with and what are the expected results?
Which tests have you added or updated? Do the tests cover all of the changes included in this PR?
Screenshots/Screencaps
Please add previews of any UI Changes.