Keypair::to_protobuf_encoding

[rubdos]

Keypair was able to decode protobuf-encoded keys, but not to encode them yet afaik. This adds a method to encode identity::Keypairs.

[rubdos]

I now also notice that PublicKey has into_protobuf_encoding, taking self (and not &self), however, no fields of PublicKey are ever moved out; every encode step is by reference. I think it makes sense to unify those, for which I'd vote to_ and not into_

[thomaseizinger]

LGTM!

Agree with PublicKey::into_protobuf_encoding -> PublicKey::to_protobuf_encoding

[rubdos]

Agree with PublicKey::into_protobuf_encoding -> PublicKey::to_protobuf_encoding

Do I put that in here, or shall I make a separate PR?

EDIT: and while we're at it, into_peer_id() is probably the same story...

[thomaseizinger]

Do I put that in here, or shall I make a separate PR?

Can we do it in separate without having dependencies to this code? I think so right?
Then a separate PR would be better because it will be an API breaking change. Personally, I'd actually favor if we can make more use of deprecated instead of breaking APIs right away but I know that @mxinden doesn't mind as much as long as the transition is easy (which it should be if it is just a rename).

Speaking of changes, can you add a changelog entry please?

EDIT: and while we're at it, into_peer_id() is probably the same story...

I think that was because PeerId used to not be Copy so it made sense to use into. But now that it is Copy, it might as well be named to_peer_id although I think clippy would still want you to take self then and not &self.

[rubdos]

Can we do it in separate without having dependencies to this code? I think so right?

Definitely.

Then a separate PR would be better because it will be an API breaking change. Personally, I'd actually favor if we can make more use of deprecated instead of breaking APIs right away but I know that @mxinden doesn't mind as much as long as the transition is easy (which it should be if it is just a rename).

I mean, we can leave both in, but it's really only deleting two characters :-)

Speaking of changes, can you add a changelog entry please?

Done

EDIT: and while we're at it, into_peer_id() is probably the same story...

I think that was because PeerId used to not be Copy so it made sense to use into. But now that it is Copy, it might as well be named to_peer_id although I think clippy would still want you to take self then and not &self.

Hmm, into vs to are related to the source type (PublicKey), not the destination type (PeerId), right?

[thomaseizinger]

Hmm, into vs to are related to the source type (PublicKey), not the destination type (PeerId), right?

They are, but if a type is not Copy, it is more flexible to offer into_ because it allows the user to potentially avoid an allocation (if the implementation allows it) whereas to_ can always be worked around with .clone().into_.

For encoding to protobuf, we need to allocate anyway so there is no point in offering into_.

[mxinden]

Can you expand on your use-case for encoding private keys?

[thomaseizinger]

Here is a usecase: https://github.com/comit-network/rendezvous-server/blob/c5dedb5119569ff523da981f5bb3b02d3e3cb903/src/main.rs#L143-L151

We want the server to always have the same identity, which is why we store the secret key in a file. Using normal unix user ACLs, we can control which parts of the machine have access to this private key.

Our design was inspired by: https://smallstep.com/blog/command-line-secrets/

[rubdos]

Same case here: a bootstrap server (set of bootstrap servers) with a fixed identity.

[mxinden]

It might be a bit paranoid, but I would really like to keep the extra safety measure of not being able to extract private key material once moved to a Keypar.

Why not do something along the lines of GenerateNodeKeyCmd and InspectNodeKeyCmd?

I would even suggest that we alter our peer ID generator, enabling it to generate a random private key and saving it to a file, preferably in the IPFS format. The rendezvous-server or your bootstrap server can then read that file and load it as a Keypair.

I am taking a similar approach with rust-libp2p-server and ipfs init.

[rubdos]

Makes sense. I was confused that Keypair::generate_*() was a thing, with seemingly no way to persist the private key. Maybe this is a documentation issue after all, if it's indeed possible to construct the private information, persist it, and then yield a key.

Would you agree if I say that it's strange to generate a Ed25519 keypair, persist the secret (somehow) and then read that as a Keypair? I wouldn't expect a struct to serialize, then deserialize to a different type.

It's not too paranoid IMO.

I would even suggest that we alter our peer ID generator, enabling it to generate a random private key and saving it to a file, preferably in the IPFS format. The rendezvous-server or your bootstrap server can then read that file and load it as a Keypair.

Note that saving to files is not a general solution; I imagine some people want to persist to a database, either in a file or in memory, etc.

[rubdos]

A patch to the docs would be very much appreciated, so others don't get confused too.

Making a lot of promises against rust-libp2p lately, but since I'll have to revise my own stuff now (it's currently using this branch), I suppose it's not a lot of work to create some docs and example code with doctests on the Keypair struct.

[thomaseizinger]

It might be a bit paranoid, but I would really like to keep the extra safety measure of not being able to extract private key material once moved to a Keypar.

But that is already possible or am I missing something?

• Fields in enums are public, hence one can just match on the variant and get for example an ed25519::Keypair
• ed25519::Keypair can be converted .into() an ed25519::SecretKey.
• ed25519::SecretKey implements AsRef<[u8]>

[rubdos]

Having a look at how to apply the approach you take in GenerateNodeKeyCmd and InspectNodeKeyCmd, but those seem not to use the protobuf encoding (which is what IPFS seems to use).

If the PeerID generator is altered to allow writing IPFS-style secret keys, I don't currently see how it can do without a patch like this one. Am I missing something here?

[mxinden]

🤦‍♂️ my bad, you are absolutely right @thomaseizinger! Sorry for the confusion.

If the PeerID generator is altered to allow writing IPFS-style secret keys, I don't currently see how it can do without a patch like this one. Am I missing something here?

Again, 100% correct.

[rubdos]

No worries, better to triple check and make sure! Thanks for having this :-)

[mxinden]

Thanks for bearing with me! Looks good to me. Also thanks for changelog entry and test.

[rubdos]

Current failure seems to be due to some incompatibility between #2142 and some new commit.

This should fix it:

diff --git a/core/src/identity.rs b/core/src/identity.rs                  
index 1569d4db..298797cd 100644                                           
--- a/core/src/identity.rs                                                
+++ b/core/src/identity.rs                                                
@@ -285,12 +285,12 @@ mod tests {                                         
     #[test]                                                              
     fn keypair_protobuf_roundtrip() {                                    
         let expected_keypair = Keypair::generate_ed25519();              
-        let expected_peer_id = expected_keypair.public().into_peer_id(); 
+        let expected_peer_id = expected_keypair.public().to_peer_id();   
                                                                          
         let encoded = expected_keypair.to_protobuf_encoding().unwrap();  
                                                                          
         let keypair = Keypair::from_protobuf_encoding(&encoded).unwrap();
-        let peer_id = keypair.public().into_peer_id();                   
+        let peer_id = keypair.public().to_peer_id();                     
                                                                          
         assert_eq!(expected_peer_id, peer_id);                           
     }                                                                    

[thomaseizinger]

Current failure seems to be due to some incompatibility between #2142 and some new commit.

That is because CI runs on pull_request and not push which is always a merge-commit between HEAD and target branch. Merging in latest master / rebasing onto master should allow you to reproduce the issue locally :)

[rubdos]

Right, I didn't realize it's code from this PR that messed that up. Too many things in parallel. Should work now :'-)