close the connection when it is refused by InterceptSecured #157
Conversation
| @@ -186,6 +186,7 @@ func (t *transport) Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID) (tp | |||
|
|
|||
| connaddrs := &connAddrs{lmAddr: localMultiaddr, rmAddr: remoteMultiaddr} | |||
| if t.gater != nil && !t.gater.InterceptSecured(n.DirOutbound, p, connaddrs) { | |||
| sess.CloseWithError(0, "") | |||
There was a problem hiding this comment.
We could use a different error code and / or a reason string here. Might help debugging, but then this would be something QUIC-specific as afaik we don't have any close reasons for TCP-based transports.
marten-seemann
force-pushed
the
close-when-gated
branch
from
9fbb5a0 to
9338905
Compare
|
Overhead in what sense? |
|
@marten-seemann creating state in QUIC that doesn't need to exist, which will be immediately available for GC? #156 drops connection the as soon as we authenticate the peer, and will likely perform better in terms of allocs and ns/op. We can benchmark it if you'd like. |
|
quic-go creates almost all of the state needed for a connection when accepting it (not when the handshake completes), so I don't expect a big difference here. For sure, aborting early is almost always more efficient, but since this is not the common case anyway, I don't think it's worth optimizing too much for it. I'd rather be consistent with other transports here. |
|
@marten-seemann no other transport does this interception itself. They delegate on the Upgrader, which does it ASAP (as soon as the crypto handshake is done, and before the multiplexer is negotiated). We push the gater down to the transport precisely so that each transport can gate in the most efficient way, i.e. the goal is to specialise, not to normalise. I think the other PR achieves this in the most efficient and cleanest way possible. |
|
I don't think this is the right way to do this, @Stebalien. Please look at #156 and discussion. |
|
Sorry, I looked at the code instead of reading the discussion. I agree with @raulk here. We should reject the connection as early as we can. |
|
Ok, I need to read the full discussion before commenting. Continuing in #155. |
|
Given that this PR is a strict bug fix while the other PR bundles an optimization, I'm going to merge this and let the debate continue. |
Minimally fixes #155. Closes #156.
This is the solution to #155 that's analogous to what we do for other transports: We first finish the cryptographic handshake (which verifies the peer's identity), and then afterwards close the connection when the connection gater doesn't like that peer's ID.