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

use private network key instead of protector interface as a libp2p.Option #795

Closed
5 of 6 tasks
marten-seemann opened this issue Feb 19, 2020 · 8 comments
Closed
5 of 6 tasks
Labels
kind/enhancement A net-new feature or improvement to an existing feature

Comments

@marten-seemann
Copy link
Contributor

marten-seemann commented Feb 19, 2020

This issue summarizes a Slack discussion with @raulk, @Stebalien and @yusefnapora.

The current design of the private network feature requires users to pass a pnet.Protector as a libp2p.Option:

func PrivateNetwork(prot pnet.Protector) Option

go-libp2p-pnet is currently the only implementation of the protector. This implementation encrypts the underlying TCP connection using Salsa20.

The pnet.Protector interfaces looks like this:

// Protector interface is a way for private network implementation to be transparent in
// libp2p. It is created by implementation and use by libp2p-conn to secure connections
// so they can be only established with selected number of peers.
type Protector interface {
	// Wraps passed connection to protect it
	Protect(net.Conn) (net.Conn, error)

	// Returns key fingerprint that is safe to expose
	Fingerprint() []byte
}

This interface assumes that every connection is running on top of a net.Conn, which is not the case with QUIC.

By using the private key currently used to construct the pnet.Protector in the libp2p.Option, we can allow different transports to apply their own protection logic. This could be achieved by changing it ito:

type PSK []byte

func PrivateNetwork(PSK) Option

Tracking PRs

@marten-seemann
Copy link
Contributor Author

@raulk @Stebalien @Kubuxu @yusefnapora
This should be ready for review now. I implemented the changes suggested above.

Furthermore, I noticed that go-libp2p-pnet contained a lot of code for serializing / deserializing a PSK from a file. This doesn't work any more as ipnet.PSK now is a []byte. Arguably, serializing the PSK should not be libp2p's responsibilty, but the responsibility of the application using libp2p. I moved that code to go-ipfs.

The transport constructor in go-libp2p-quic-transport now already takes a PSK, but doesn't use it yet (it returns an error, since private networks are not yet supported). How exactly we protect QUIC connections is independent of this refactoring, and I'll track this in a different issue.

What do you think? Does that make sense?

@Stebalien
Copy link
Member

Per the discussion in ipfs/kubo#6914, let's leave the decoding logic in libp2p somewhere. We could leave it in go-libp2p-pnet or move it into go-libp2p itself. Really, it probably belongs in go-libp2p-pnet.


Unfortunately, I think you're on the right track here and doing this the right way is going to be unavoidably very API breaking. We can't accept both a key and a pre-built protector.

@marten-seemann
Copy link
Contributor Author

I restored the PSKv1 decoding logic in go-libp2p-pnet (see libp2p/go-libp2p-pnet#32).

To be honest, I'm not sure if we gain a lot from this - it's still a breaking API change, since we remove the func NewProtector(input io.Reader) (ipnet.Protector, error) constructor. As you point out, this is unavoidable, since we don't have any use for an ipnet.Protector any more.

Therefore, by making this change, we now force people to change their code to use the newly exposed (and immediately deprecated) DecodeV1PSK function in pnet, and some time in the future, when we remove this function, we will force them to change their code yet again to use the function provided in IPFS (or implement their own PSK serialization function).

@Stebalien
Copy link
Member

Therefore, by making this change, we now force people to change their code to use the newly exposed (and immediately deprecated) DecodeV1PSK function in pnet, and some time in the future, when we remove this function, we will force them to change their code yet again to use the function provided in IPFS (or implement their own PSK serialization function).

The issue isn't breaking things, the issue is that nobody wants to depend on go-ipfs itself to decode their existing private network keys. If we find that we need to keep it around, we can move it to go-libp2p or even go-libp2p-core (or we can just un-deprecate it).

@marten-seemann
Copy link
Contributor Author

That makes sense. Why don’t me move it to -core and not deprecate it now?

@Stebalien
Copy link
Member

Mumble punting decisions down the road.

You're right, we should just do that.

@marten-seemann
Copy link
Contributor Author

@Stebalien Great. Just move decoding to go-libp2p-core and updated all the PRs.

@Stebalien
Copy link
Member

This looks all ready but I'd like to merge, release go-libp2p-core, and update everything all at once. Unfortunately, releasing go-libp2p-core is currently blocked on some peerstore questions that should be resolved imminently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

No branches or pull requests

2 participants