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

add connection gating interfaces and types. #139

Merged
merged 11 commits into from
May 13, 2020
Merged

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Apr 7, 2020

For libp2p/go-libp2p#872.

We've decided to model the interface as a bunch of gating functions that are applied during the different life cycle states of the connection. The specific gating function that is applied depends on the lifecycle state of the connection.

// DenyAddrConnection returns true if a connection to/from a peer
// should be denied.
// The caller must supply the multi-address of the remote peer.
DenyAddrConnection(ma.Multiaddr) (deny bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth providing both (ma.Multiaddr, peer.ID) as arguments. Rather than re-parsing the peer out of the address, most dial attempts ought to be in the context of a specific peer already (so it shouldn't be much of an imposition on the caller), and that might make implementing some type of gaters simpler.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

We recently added the requirement that for the Connection Gater to be unifying abstraction, such that the Filters can be subsumed under a default connection gater implementation. I believe this PR attempts to implement that.


Out of band meta-suggestion: explain what the PR is about, the design choices you've made, etc.; it's hard to infer that from an issue link, especially when the consensus on the issue can evolve over time, and there could be many PRs implementing different parts of an issue; also, the indirection is mildly annoying ;-)


I think we'd be better served by the following interface definition:

type ConnectionGater interface {
    // AllowDial tests whether we're permitted to dial the specified multiaddr.
    // Insofar filter.Filters is concerned, this would map to its AddrBlock method, with the inverse condition.
    // This is to be called by the network/swarm when dialling.
    AllowDial(ma.Multiaddr) bool

    // AllowIncoming tests whether an incipient inbound connection is allowed. 
    // network.ConnMultiaddrs is what we pass to the upgrader.
    // This is intended to be called by the upgrader, or by the transport directly (e.g. QUIC, Bluetooth), straight after it's accepted a connection from its socket.
    AllowIncoming(network.ConnMultiaddrs) bool

    // AllowConnection tests whether a given connection, now authenticated, is allowed.
    // This is intended to be called by the upgrader, after it has negotiated crypto, and before it negotiates the muxer, or by the directly by the transport, at the exact same checkpoint.
    AllowConnection(network.Direction, peer.ID, network.ConnMultiaddrs) bool
}

@raulk
Copy link
Member

raulk commented Apr 8, 2020

I'm having doubts if AllowConnection should be tested after negotiating the muxer. In that case, it would be swarm that calls it (in the original spirit of #99), and it could return a disconnect reason, as you suggest, and we could send it given that we now have a muxer.

@aarshkshah1992
Copy link
Contributor Author

@raulk

In addition to having a muxed connection we can send the Disconnect message on, one more reason I'd consider putting AllowConnection in the Swarm is to gate outbound connections before they've had the opportunity to travel "downwards" from the Swarm.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Actually, there's an opportunity to unify with the FSM model that we've discussed before -- where we model connection establishment as a finite state machine.

Borrowing from that model, this interface would receive callbacks at each state of the connection state machine.

And, in fact, this really starts to look like a middleware pattern:

type ConnectionGater interface {
    // InterceptDial tests whether we're permitted to dial the specified multiaddr.
    // Insofar filter.Filters is concerned, this would map to its AddrBlock method, 
    // with the inverse condition.
    // This is to be called by the network/swarm when dialling.
    InterceptDial(ma.Multiaddr) (allow bool)

    // InterceptAccept tests whether an incipient inbound connection is allowed. 
    // network.ConnMultiaddrs is what we pass to the upgrader.
    // This is intended to be called by the upgrader, or by the transport 
    // directly (e.g. QUIC, Bluetooth), straight after it's accepted a connection 
    // from its socket.
    InterceptAccept(network.ConnMultiaddrs) (allow bool)

    // InterceptSecured tests whether a given connection, now authenticated, 
    // is allowed.
    // This is intended to be called by the upgrader, after it has negotiated crypto,
    // and before it negotiates the muxer, or by the directly by the transport, 
    // at the exact same checkpoint.
    InterceptSecured(network.Direction, peer.ID, network.ConnMultiaddrs) (allow bool)

    // InterceptUpgraded tests whether a fully capable connection is allowed.
    // At this point, we have a multiplexer, so the middleware can 
    // return a DisconnectReason. 
    // and the swarm would use the control stream to convey it to the peer.
    InterceptUpgraded(network.CapableConn) (allow bool, reason control.DisconnectReason)
}


// InterceptPeerDial tests whether we're permitted to Dial the specified peer.
// This is to be called by the network/swarm when dialling.
InterceptPeerDial(p peer.ID) (allow bool)
Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 Apr 9, 2020

Choose a reason for hiding this comment

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

@raulk I've added this function to gate outbound dials based on the peerId. Otherwise, users would have to wait till the dial is complete and InterceptSecured is called.

Also, gives this functionality to transports that do not use an upgrader.

Copy link
Member

Choose a reason for hiding this comment

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

Not really necessary. On an outbound dial, the multiaddr contains the peer ID.

connmgr/conngating.go Outdated Show resolved Hide resolved
connmgr/conngating.go Outdated Show resolved Hide resolved
connmgr/conngating.go Outdated Show resolved Hide resolved
connmgr/conngating.go Outdated Show resolved Hide resolved
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Reviewed and made changes:

  • revised all godocs; made them clearer, and rewrote them in a more imperative, less reflexive manner.
  • dropped the conn prefix from files in the connmgr package.
  • marked the DisconnectReason type/feature as experimental.
  • InterceptUpgraded(transport.CapableConn) => InterceptUpgraded(network.Conn).
  • Intercept{Peer => }AddrDial(peer.ID, ma.Multiaddr) (allow bool).

@raulk raulk changed the title Connection Gater interfaces add connection gating interfaces and types. May 13, 2020
@raulk raulk merged commit 909c774 into master May 13, 2020
@raulk raulk deleted the feat/connection-gating branch May 13, 2020 12:15
@raulk raulk mentioned this pull request May 14, 2020
2 tasks
@Stebalien Stebalien mentioned this pull request May 26, 2020
77 tasks
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.

4 participants