connection is not closed when rejected by InterceptSecured

[marten-seemann]

See #152 (review).

[aarshkshah1992]

@marten-seemann Would you be able to create the PR for this or do you want me to create one ?

[marten-seemann]

@aarshkshah1992 Wouldn't it be better to call this callback during the TLS handshake, not after it's finished?
We determine the peer's ID in https://github.com/libp2p/go-libp2p-tls/blob/8a8ad624a2911e6557746729a37f641580b3b16a/crypto.go#L79-L104, and we could fail the handshake right at that place, couldn't we?

[aarshkshah1992]

@marten-seemann I am not sure it would be easy to source the local and remote multi-addresses for the connection that we require for calling InterceptSecured in go-libp2p-tls.

However, I do NOT have an understanding of go-libp2p-tls and so will defer this question to @raulk .

[marten-seemann]

@marten-seemann I am not sure it would be easy to source the local and remote multi-addresses for the connection that we require for calling InterceptSecured in go-libp2p-tls.

Yes, we'd probably have to make a slight change to the interface expose by that package. Maybe GetConfigForPeer(remote peer.ID, laddr, raddr ma.Multiaddr)?

I'm not very familiar with the motivation behind the connection gater, is there any reason that InterceptSecured is specified as being called after the completion of the handshake?

[raulk]

@marten-seemann InterceptSecured should be called straight after the handshake has taken place -- as soon as we've authenticated the peer and we have a peer ID.

[raulk]

We can override the conf.VerifyPeerCertificate function returned by go-libp2p-tls with a function that calls the original + calls InterceptSecured. We'd introduce the change here: https://github.com/libp2p/go-libp2p-quic-transport/blob/master/transport.go#L156, where we have access to the multiaddrs, so we can pass in a network.ConnMultiaddrs object into the connection gater.

[marten-seemann]

@marten-seemann InterceptSecured should be called straight after the handshake has taken place -- as soon as we've authenticated the peer and we have a peer ID.

The tls.VerifyPeerCertificate is doing the authentication of the peer and determines its ID.

We can override the conf.VerifyPeerCertificate function returned by go-libp2p-tls with a function that calls the original + calls InterceptSecured.

The same logic applies if we use TLS as a handshake protocol, so we probably should make the change there at the same time.

Would it make sense to move the responsibility of calling InterceptSecured to the libp2p handshake layer?

[raulk]

@marten-seemann

The same logic applies if we use TLS as a handshake protocol, so we probably should make the change there at the same time.

It's not the handshake's responsibility to call InterceptSecured -- it's the Transport's responsibility, by either delegating to the Upgrader (which is implicitly a part of the transport), or calling it itself. Also, handshakes don't have access to the multiaddrs.

Take a look at #156 -- I think it solves it elegantly, although ideally quic-go would expose these hooks so we're not hijacking the hook that the crypto/tls package exposes.

[marten-seemann]

Take a look at #156 -- I think it solves it elegantly, although ideally quic-go would expose these hooks so we're not hijacking the hook that the crypto/tls package exposes.

I'm not sure how such a hook would look like. quic-go is using crypto/tls (or rather, a fork thereof) to perform the handshake.

My reasoning is the following: Since both go-libp2p-tls and go-libp2p-quic-transport use TLS 1.3 to perform the handshake, the behavior regarding the connection gater should be the same: Either we reject the handshake attempt during the handshake, as you've implemented in #156, or we do it after the handshake, as currently implemented in master (and just close the connection if that fails). I don't see any reason to treat TLS and QUIC different in that regard.

[raulk]

@marten-seemann Our solution needs to cover Noise, SecIO, etc., not just TLS 1.3. We don't want to run around injecting the connection gater into all handshakes.

Conceptually, it's the transport's responsibility to gate connections. It does so delegating to the upgrader (if the transport has no native security), or it does so explicitly.

If we add this to the TLS 1.3 handshake directly, it would run twice for non-QUIC cases: once in the handshake, once in the upgrader. We can't remove the call from the upgrader, because that means we need to push the connection gater to the handshakes too, and also modify SecIO and Noise, which we definitely won't do.

Handshakes are pretty lean, in the sense that they can operate purely on a net.Conn, they need nothing else.

[marten-seemann]

Fair enough. Then I suggest we go with #157, instead of making QUIC a special case here.

[Stebalien]

We don't want to run around injecting the connection gater into all handshakes.

Why not? This seems pretty reasonable to me. If double checking is expensive, something is probably wrong.

Basically, @marten-seemann's saying that we should either:

1. Always try to check as early as possible (e.g., in the TLS/SECIO/Noise transports).
2. Consistently check after the handshake.

Checking early in some cases and later in others is inconsistent and confusing.

[raulk]

@Stebalien I don't regard it as inconsistent. It's the transport's responsibility to gate accepted and secured connections. Some transports are entirely coupled with their handshake (because it's native), and can take advantage of the connection gater sooner.

The correct way to do this would've been for quic-go to expose hooks during the connection pipeline. With the right hook, we could complete the handshake and then intercept the connection, just like the Upgrader is doing. But quic-go does not support hooks/callbacks (I remember seeing an issue for it somewhere), so we have to intercept the handshake itself. Since the hooks don't exist and QUIC and TLS are coupled, why not take advantage and intercept at the lower level?

See the savings in #156 (comment).

---

Also, our handshakes are pretty lean; they don't care about higher level libp2p constructs. Plus, pushing the connection gater to all handshakes will mean we'd call the gater from three places:

1. Network.
2. Transports (for accept).
3. Handshakes (for secured).

This proliferation is not good.

---

Honestly, I think this discussion is outliving its purpose. In 2 days we won't even remember/look at this.

I'm planning to close #157 and merge #156 tomorrow (Friday), but I'd like to have your buy-in that we disagree and commit on the premises that I've posted above.

[marten-seemann]

Basically, @marten-seemann's saying that we should either:

1. Always try to check as early as possible (e.g., in the TLS/SECIO/Noise transports).
2. Consistently check after the handshake.

Checking early in some cases and later in others is inconsistent and confusing.

Thank you @Stebalien, that's an excellent summary of my argument.

The correct way to do this would've been for quic-go to expose hooks during the connection pipeline. With the right hook, we could complete the handshake and then intercept the connection, just like the Upgrader is doing. But quic-go does not support hooks/callbacks (I remember seeing an issue for it somewhere), so we have to intercept the handshake itself.

Can you elaborate what this hook would do? I don't remember any hook-related issue in quic-go. As far as my understanding goes, this hook wouldn't do anything other than exactly what #157 implements.

The same applies to InterceptAccept by the way. You could argue that quic-go could expose a hook to filter out connection attempts by IP address. The way I'd implement this in quic-go would be to look at every incoming (QUIC Long Header) packet and discard it if the IP address is on the blacklist, which is exactly what @aarshkshah1992 implemented in #152. As there's no benefit of doing this in quic-go itself, there's an argument to be made to keep the API as lean as possible.

[Stebalien]

I will let you two sort this out, I have no strong opinions either way. However, #157 fixes the bug while #156 is an optimization. I've merged it so we don't block a bug fix on a debate.

[raulk]

Re: the architecture question, the right interception point for InterceptSecured is as soon as the connection has been authenticated, and BEFORE it has been upgraded with further capabilities (right now: multiplexing; in the future: compression, etc.).

When the latter happens, InterceptUpgraded is called.

The right hooks in quic-go would allow us to:

1. intercept the connection as soon as it has been authenticated (the above).
2. intercept inbound connection attempts as soon as possible.
3. intercept reads/writes and allow to mutate them, to implement quic: private network support go-libp2p#1432 effectively

• For working around (1), I filed fix: call InterceptSecured earlier; drop conn. #156.
• For working around (2), we detect long frames, which leaks QUIC implementation details we should not know about.
• For (3), we don't have a solution yet.