Address filtering is semi-broken

[Stebalien]

Due to multiformats/go-multiaddr-fmt#1, the TCP, UDP, and WS(S) multiaddr matchers now match DNS multiaddrs. This means we'll try to dial DNS multiaddrs and succeed, even if we've blocked the underlying address/subnet address via the address filter.

In the TCP & WS transports, this isn't terrible because we check again after establishing the connection. However, it looks like the QUIC transport uses the input address, not the final resolved address, so this second fallback check fails to work either.

TODO:

• Expose resolved addresses in QUIC.
• Fix the filtering logic, either in go-multiaddr-fmt, or in the individual transports.
• Add a test in libp2p.

[Stebalien]

Note: this is a blocker for the go-ipfs release.

[marten-seemann]

Not sure if I understand the issue correctly. Should this be an issue in go-libp2p-quic-transport?

Is it incorrect to use the raddr we dialed as a RemoteMultiaddr here? https://github.com/libp2p/go-libp2p-quic-transport/blob/c250617d5b6ef80e4641295d703bb7b514d34200/transport.go#L119-L172

[Stebalien]

I've filed an issue for the QUIC side of this libp2p/go-libp2p-quic-transport#123 However, that's only showing up because we're passing dns addresses directly to transports. Historically, we we resolved them in the routed host. We should either go back to doing that (might cause issues with some recent changes to the websocket transport?) or resolve then filter addresses inside transports. Tangentially related, we should filter inbound connections in QUIC before we even try to handshake: libp2p/go-libp2p-quic-transport#124.

[raulk]

From an architectural perspective, the transport should receive the raw multiaddr and decide what to do with it.

Rationale: future transports like Ethernet, Bluetooth, Tor, etc. have their own address format, and they may support custom name resolution schemes that aren't DNS based.

Putting the resolution logic on the side of go-multiaddr leaks such concerns, and favours monolithic, god-like "know-it-all" logic.

I'd support the go-multiaddr family of repos offering functions to facilitate commonplace resolutions for transports to reuse (e.g. DNS resolution), but ultimately it is the transport that knows which addressing schemes are relevant to it, and is responsible for resolving what needs to be resolved.

[Stebalien]

@raulk To make sure we're on the same page, the bug here is that:

1. Currently, it's the routed host's responsibility to resolve DNS addresses.
2. Previously, the swarm would never actually try to dial a DNS address.
3. The swarm would call transport.CanDial("/dns/...").
4. The transport would call mafmt.(TCP|UDP).Matches("/dns/...") which would return false.
5. Now, `mafmt.(TCP|UDP).Matches("/dns/...") returns true.

This issue is not about adding smarts to go-multiaddr.

From an architectural perspective, the transport should receive the raw multiaddr and decide what to do with it.

For DNS, that might make sense. In general, that's not always possible:

• /dnsaddr addresses must be resolved before we can know the underlying transport.
• I wouldn't want every transport to have to know about every name system we decide to use.

Rationale: future transports like Ethernet, Bluetooth, Tor, etc. have their own address format, and they may support custom name resolution schemes that aren't DNS based.

For transport-specific name systems, that totally makes sense.

Putting the resolution logic on the side of go-multiaddr leaks such concerns, and favours monolithic, god-like "know-it-all" logic.

?

[Stebalien]

Digging into this, if we resolve inside the transports, we have several issues:

1. We may resolve to multiple addresses. Usually, it's up to the swarm to figure out how to schedule dials.
2. We'll end up dialing the same underlying addresses multiple times because, again, we do the scheduling/deduping inside the swarm itself.

Our options are to resolve in the swarm or resolve in the host (what we do now). Given that the swarm holds the transports, IMO, it would make more sense if the swarm also held a set of pluggable resolvers.

[raulk]

I still regard address resolution as a transport concern, but not part of the transport dial. I’d enhance transports with an optional `ResolveAddr(Multiaddr) []Multiaddr` that the network can call to ask the transport to perform the resolution. In other words, we don’t need to add yet another component (pluggable resolvers): the transport can act as the resolver.

…

On Mon, Mar 23 2020 at 10:23 PM, Steven Allen < ***@***.*** > wrote: Digging into this, if we resolve inside the transports, we have several issues: * We may resolve to multiple addresses. Usually, it's up to the swarm to figure out how to schedule dials. * We'll end up dialing the same underlying addresses multiple times because, again, we do the scheduling/deduping inside the swarm itself. Our options are to resolve in the swarm or resolve in the host (what we do now). Given that the swarm holds the transports, IMO, it would make more sense if the swarm also held a set of pluggable resolvers. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( #841 (comment) ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AAIM3OU2YZCHIVAKYHYOCY3RI7OOLANCNFSM4LHRG2QA ).

[Stebalien]

My concern is that resolvers and transports are independent. We'll want one resolver per name system (dns, dnsaddr, eth, handshake, etc.). This is is independent from the transports.

[raulk]

Understood. Under that model, resolvers would enlist themselves in a registry, and transports would call multiaddr.Resolve(ma) []multiaddr.Multiaddr which would use the global registry to traverse the multiaddr and resolve all resolvable components?

[Stebalien]

Resolvers would be registered with the swarm/host and the swarm/host would resolve all addresses, deduplicate the results, sort the addresses based on priority, then invoke the transports.

The transports can't be the ones to resolve the addresses because of all the reasons I've mentioned above (we may not know the transport till we resolve an address and transports can't deduplicate addresses or plan dials with multiple addresses).

[jacobheun]

The latest patch release of go-libp2p, https://github.com/libp2p/go-libp2p/releases/tag/v0.7.4, says this issue is fixed.

This patch also fixes #841.

Can this be closed or are there remaining issues we need to track?

[Stebalien]

I've cut a release of quic-go that fixes this, so yes.