Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Changes for use in QUIC #7

Closed
wants to merge 2 commits into from

Conversation

Ichbinjoe
Copy link

@Ichbinjoe Ichbinjoe commented Dec 12, 2018

These two changes are required so that go-libp2p-quic-transport can use this package. Both changes are agnostic of quic, and should be required of basically any transport which will rely on this package in the future.

Use standard hostname for certs is required since otherwise mint will attempt to infer the hostname of the connected server. This is incorrect, so we choose a public hostname that all libp2p tls transports will share. This is the same behavior as quic-transport currently exhibits.

Return pubkeys which connect to us to caller is required to pass back the public key of the remote peer so an implementing connection can provide it to satisfy the ConnSecurity interface.

This is the majority of the work to allow this package to be used by QUIC. See #4

Copy link
Collaborator

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

We can't really fix #4 before releasing a new quic-go version, since we changed the length of the certificate chain.

This will also obsolete the hack that was needed to make mint accept the certificate, since quic-go now uses tls-tris instead of mint. I'd like to avoid polluting this repo with this kind of hack.

@Ichbinjoe
Copy link
Author

Regarding the hack, are you referring to the static servername? Regardless whether tls-tris fixes the hostname hack, the second commit (Return pubkeys which connect to us to caller) or some similar mechanism is required to pass the public key extracted from the TLS handshake verification back to the implementing transport, as noted above.

I'm being so verbose to be sure that I'm on the same page - obviously I'm pretty new here and just trying to get a handle on all of this (and to be honest, the pull requests were me learning more about libp2p).

@marten-seemann
Copy link
Collaborator

Yes, with the hack I was referring to setting a static server name.

You can't return the remote public key in ConfigForPeer, since ConfigForPeer returns before the handshake runs. At that point, we only know the remote peer ID, not its public key. We only learn about the public key once the tls.Config.VerifyPeerCertificate callback is executed, which happens during the TLS handshake.

@Ichbinjoe
Copy link
Author

Ichbinjoe commented Dec 13, 2018

If you notice, I'm not returning the public key directly but instead a pointer to the public key which is reset to the actual public key after the verification step in the handshake. I've validated this behavior via tests within quic-transport. This works with your statement above, in that we don't have the public key until after the handshake. Sorry if this code made that unclear - I'm open to suggestions on how do deal with this instance better.

https://github.com/libp2p/go-libp2p-quic-transport/blob/f0b88d6f6e25fbf4921840038f830091ae55b285/transport.go#L111

For reference, the remote public key is dereferenced on line 128 - we know it will not be nil at this point because there is no case where DialContext (line 113) would not error yet the verification not happen (and thus setting of the pointer).

@marten-seemann
Copy link
Collaborator

Oh right, that might work. Returning a nil pointer that is updated at some later moment sounds like a very brittle solution though.

I think the better way to solve this is to use KeyFromChain, as we already do it for the listener.

@marten-seemann
Copy link
Collaborator

Outdated due to recent changes. Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants