-
Notifications
You must be signed in to change notification settings - Fork 19
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.
This sort of works but it's kind of messy and I'm pretty sure it's leaking connections. Instead of stashing the key in a global map, it may be simpler to use a one-off channel. That is:
- Implement a
Config() (*tls.Config, <-chan ic.PubKey)
function onIdentity
. This generates a one-off config object and a one-off channel that returns the peer's key on successful authentication. - Change
ConfigForPeer(peer.ID) *tls.Config
toConfigForPeer(peer.ID) (*tls.Config, <-chan ic.PubKey)
. - Make
t.handshake take a
<-chan ic.PublicKey` channel.
transport.go
Outdated
return t.handshake(ctx, cl) | ||
verifiedCallback := func(pubKey ic.PubKey) { | ||
t.activeMutex.Lock() | ||
t.active[insecure] = pubKey |
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.
Won't this leak the conn?
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.
It is, but only because I forgot the defer
statement that cleans up the map, as in SecureInbound
.
I agree that it's kind of messy, although maybe not messier than calculating the peer ID multiple times during the handshake.
I'm not sure how this should work for a |
@marten-seemann can you generate a new config here: https://github.com/libp2p/go-libp2p-tls/pull/17/files#diff-0fecf8d8a1c3b52ef05e62d6a1bf6effR72? |
Something like: #18 (needs testing). |
Thanks for the PR, @Stebalien. I don't think this needs additional tests, this should all be covered by the existing tests already. |
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.
LGTM modulo the duplicate import.
(note: I'm not married to the channel method, I'm just not a fan of mutable shared state)
transport.go
Outdated
"net" | ||
"os" | ||
|
||
cs "github.com/libp2p/go-conn-security" | ||
ci "github.com/libp2p/go-libp2p-crypto" | ||
ic "github.com/libp2p/go-libp2p-crypto" |
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.
Duplicate import.
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.
Why is this even allowed in Go... Fixed.
Instead of a |
On the other hand, the chan makes it immediately obvious that the |
Yeah... It should technically be safe but only technically. |
Fixes #15. Fixes #16.
This PR fixes two issues by
tls.Config.VerifyPeerCertificate
callbacknet.Conn
. This is necessary since in theVerifyPeerCertificate
callback the server has no idea which connection it is currently verifying.Note that this PR will make it harder to reuse the code with QUIC, since in QUIC, there's no such thing as a (unique)
net.Conn
(as a client, you could reuse the same UDP connection to run multiple dials, and a server only listens on a singlenet.Conn
). I suggest taking one step at a time and dealing with these issues later.