gate QUIC connections via new ConnectionGater #152

aarshkshah1992 · 2020-05-18T06:36:44Z

This PR intercepts packets read from the UDP socket to determine if they should be passed upwards in the stack or gated right away.
It does so by calling the InterceptAccept method of the Connection Gater that is passed to the QUIC transport during initialization.

@raulk
I am not sure if we can actually call InterceptSecured here as the security handshake seems to be part of making a QUIC connection and is handled completely by the QUIC library we use.

aarshkshah1992 · 2020-05-18T06:41:23Z

conn_test.go


 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )

+type localhostMockGater struct {


@raulk I know this will make you cringe and add some entropy to thy life. But, I have added a TODO at libp2p/go-libp2p#930 to replace this with the "Functional Gater" that we plan to implement.

For all that you hold dear on this good Earth, rest assured, this shall be fixed.

raulk

We need to call InterceptSecured and InterceptUpgraded. Even if there are no explicit steps in the QUIC transport, those checkpoints still exist virtually. We should provide a normalised experience. The implementor of ConnectionGater should not be aware of the individual nuances in each transport.

Note: the InterceptUpgraded will be redundant with the swarm's call (we should remove it from there).

raulk · 2020-05-18T09:21:04Z

filtered_conn.go

 			return
 		}
 	}
-}
+}


go fmt missing.

transport.go

aarshkshah1992 · 2020-05-18T09:31:29Z

Note: the InterceptUpgraded will be redundant with the swarm's call (we should remove it from there).

I am not sure why we need to call InterceptUpgraded here. The Swarm will call InterceptUpgraded for ALL transports. There's no special casing there. We don't call InterceptUpgraded in any of the other transports(upgrader) as well.

raulk · 2020-05-18T09:38:54Z

@aarshkshah1992 It's a separation of concerns argument. The component that's responsible for returning an upgraded connection is the transport, therefore it should be the one intercepting, in my view. However, OTOH the swarm receives only upgraded connections from the transports, so it's safe to just leave the interception point there... No right or wrong here, disregard my comment about InterceptUpgraded and let's implement only InterceptSecured here.

raulk · 2020-05-18T09:39:21Z

@aarshkshah1992 We should update the godocs in go-libp2p-core to make it explicit what's expected to be implemented by transport writers and what isn't...

aarshkshah1992 · 2020-05-18T13:01:53Z

@raulk Have addressed your changes. Please take a look now.

conn_test.go

raulk · 2020-05-18T13:20:38Z

conn_test.go

@@ -192,7 +218,9 @@ var _ = Describe("Connection", func() {

 		// now allow the address and make sure the connection goes through
 		clientTransport.(*transport).clientConfig.HandshakeTimeout = 2 * time.Second
-		filters.AddFilter(ipNet, filter.ActionAccept)
+		cg.lk.Lock()
+		cg.allowAll = true


A test for InterceptSecured would be great (mostly for regression testing).

This is done.

listener.go

filtered_conn.go

Stebalien · 2020-05-19T05:33:40Z

Note: It's not actually possible to call InterceptUpgraded here as we have a transport.CapableConn, not a network.Conn.

aarshkshah1992 · 2020-05-19T07:45:11Z

@raulk Have added a test for InterceptSecured as well. Please approve and merge if happy.

raulk · 2020-05-19T11:39:17Z

Note: It's not actually possible to call InterceptUpgraded here as we have a transport.CapableConn, not a network.Conn.

Correct. I knew there was a reason I had originally planned InterceptUpgraded to take a transport.CapableConn. But that wouldn't have worked anyway, because the calling component will, in the future, need to return a disconnect reason over a control stream, for which we need a network.Conn that's working with multistream.

raulk · 2020-05-19T11:45:07Z

conn_test.go

+	allowAllAccepted   bool
+	disableSecuredPeer peer.ID


I've simplified these names @aarshkshah1992.

marten-seemann

Hi guys, just came back after a few days OOO.

The code here mostly lgtm, but I think we need to close the connection when InterceptSecured rejects it.

Furthermore, I have one question: I assume there's an InterceptAccept and an InterceptSecured to allow rejecting connection both from a certain multiaddr as well as from a certain peer ID. This much makes sense to. But what's the point of having an InterceptUpgraded on the connection gater? And why is it not called here?

Edit: Found the answer: #152 (comment).

marten-seemann · 2020-05-21T06:07:12Z

transport.go

@@ -178,6 +183,13 @@ func (t *transport) Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID) (tp
 		pconn.DecreaseCount()
 		return nil, err
 	}
+
+	connaddrs := &connAddrs{lmAddr: localMultiaddr, rmAddr: remoteMultiaddr}
+	if t.gater != nil && !t.gater.InterceptSecured(n.DirOutbound, p, connaddrs) {


We need to close the connection at this point. Otherwise the peer will never know that we closed the connection. As from quic-go's point of view the connection is fully established, there won't be any connection-level timeout here either.

gating in QUIC

bc2b6e0

aarshkshah1992 requested review from marten-seemann and raulk May 18, 2020 06:36

aarshkshah1992 commented May 18, 2020

View reviewed changes

raulk suggested changes May 18, 2020

View reviewed changes

filtered_conn.go Outdated

return

}

}

}

}

Copy link

Member

raulk May 18, 2020

Choose a reason for hiding this comment

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

go fmt missing.

transport.go Outdated Show resolved Hide resolved

changes as per review

21b5936

raulk reviewed May 18, 2020

View reviewed changes

conn_test.go Outdated Show resolved Hide resolved

raulk reviewed May 18, 2020

View reviewed changes

listener.go Outdated Show resolved Hide resolved

raulk reviewed May 18, 2020

View reviewed changes

filtered_conn.go Show resolved Hide resolved

fix import order

135bb88

Stebalien approved these changes May 19, 2020

View reviewed changes

add test for intercept secured

d960369

raulk added 2 commits May 19, 2020 12:40

go mod tidy.

cb8b875

naming nits; upgrade deps.

6326c75

raulk reviewed May 19, 2020

View reviewed changes

raulk approved these changes May 19, 2020

View reviewed changes

raulk changed the title ~~Add Connection Gating to the QUIC transport by intercepting packets read from the Socket~~ gate QUIC connections via new ConnectionGater May 19, 2020

raulk merged commit 541e8a3 into master May 19, 2020

raulk deleted the feat/conn-gating branch May 19, 2020 11:48

raulk mentioned this pull request May 19, 2020

Connection gating: adapt QUIC transport libp2p/go-libp2p#931

Closed

1 task

marten-seemann reviewed May 21, 2020

View reviewed changes

marten-seemann mentioned this pull request May 21, 2020

connection is not closed when rejected by InterceptSecured #155

Open

Stebalien mentioned this pull request May 26, 2020

Release 0.6.0 ipfs/kubo#7347

Closed

77 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gate QUIC connections via new ConnectionGater #152

gate QUIC connections via new ConnectionGater #152

aarshkshah1992 commented May 18, 2020 •

edited

aarshkshah1992 May 18, 2020 •

edited

raulk left a comment

raulk May 18, 2020

aarshkshah1992 commented May 18, 2020

raulk commented May 18, 2020

raulk commented May 18, 2020

aarshkshah1992 commented May 18, 2020

raulk May 18, 2020

aarshkshah1992 May 19, 2020

Stebalien commented May 19, 2020

aarshkshah1992 commented May 19, 2020

raulk commented May 19, 2020

raulk May 19, 2020

marten-seemann left a comment •

edited

marten-seemann May 21, 2020

gate QUIC connections via new ConnectionGater #152

gate QUIC connections via new ConnectionGater #152

Conversation

aarshkshah1992 commented May 18, 2020 • edited

aarshkshah1992 May 18, 2020 • edited

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

raulk May 18, 2020

Choose a reason for hiding this comment

aarshkshah1992 commented May 18, 2020

raulk commented May 18, 2020

raulk commented May 18, 2020

aarshkshah1992 commented May 18, 2020

raulk May 18, 2020

Choose a reason for hiding this comment

aarshkshah1992 May 19, 2020

Choose a reason for hiding this comment

Stebalien commented May 19, 2020

aarshkshah1992 commented May 19, 2020

raulk commented May 19, 2020

raulk May 19, 2020

Choose a reason for hiding this comment

marten-seemann left a comment • edited

Choose a reason for hiding this comment

marten-seemann May 21, 2020

Choose a reason for hiding this comment

aarshkshah1992 commented May 18, 2020 •

edited

aarshkshah1992 May 18, 2020 •

edited

marten-seemann left a comment •

edited