From ddbfee00376353474b6b8e788184f5ff8bc899b4 Mon Sep 17 00:00:00 2001 From: sukun Date: Mon, 8 Jul 2024 20:25:36 +0530 Subject: [PATCH 1/3] config: fix AddrFactory for AutoNAT --- config/config.go | 40 +++++++++++++++++++++++----------------- libp2p_test.go | 5 ++++- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/config/config.go b/config/config.go index a99da1232d..623cc473fc 100644 --- a/config/config.go +++ b/config/config.go @@ -486,26 +486,33 @@ func (cfg *Config) NewNode() (host.Host, error) { ) } - // Note: h.AddrsFactory may be changed by relayFinder, but non-relay version is - // used by AutoNAT below. - if cfg.EnableAutoRelay { - if !cfg.DisableMetrics { - mt := autorelay.WithMetricsTracer( - autorelay.NewMetricsTracer(autorelay.WithRegisterer(cfg.PrometheusRegisterer))) - mtOpts := []autorelay.Option{mt} - cfg.AutoRelayOpts = append(mtOpts, cfg.AutoRelayOpts...) - } - fxopts = append(fxopts, - fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) (*autorelay.AutoRelay, error) { + // oldAddrFactory is the AddrFactory before it's modified by autorelay + // we need this for checking reachability via autonat + oldAddrFactory := func(addrs []ma.Multiaddr) []ma.Multiaddr { + return addrs + } + + fxopts = append(fxopts, + fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) (*autorelay.AutoRelay, error) { + oldAddrFactory = h.AddrsFactory + if cfg.EnableAutoRelay { + if !cfg.DisableMetrics { + mt := autorelay.WithMetricsTracer( + autorelay.NewMetricsTracer(autorelay.WithRegisterer(cfg.PrometheusRegisterer))) + mtOpts := []autorelay.Option{mt} + cfg.AutoRelayOpts = append(mtOpts, cfg.AutoRelayOpts...) + } + ar, err := autorelay.NewAutoRelay(h, cfg.AutoRelayOpts...) if err != nil { return nil, err } lifecycle.Append(fx.StartStopHook(ar.Start, ar.Close)) return ar, nil - }), - ) - } + } + return nil, nil + }), + ) var bh *bhost.BasicHost fxopts = append(fxopts, fx.Invoke(func(bho *bhost.BasicHost) { bh = bho })) @@ -523,7 +530,7 @@ func (cfg *Config) NewNode() (host.Host, error) { return nil, err } - if err := cfg.addAutoNAT(bh); err != nil { + if err := cfg.addAutoNAT(bh, oldAddrFactory); err != nil { app.Stop(context.Background()) if cfg.Routing != nil { rh.Close() @@ -539,8 +546,7 @@ func (cfg *Config) NewNode() (host.Host, error) { return &closableBasicHost{App: app, BasicHost: bh}, nil } -func (cfg *Config) addAutoNAT(h *bhost.BasicHost) error { - addrF := h.AddrsFactory +func (cfg *Config) addAutoNAT(h *bhost.BasicHost, addrF AddrsFactory) error { autonatOpts := []autonat.Option{ autonat.UsingAddresses(func() []ma.Multiaddr { return addrF(h.AllAddrs()) diff --git a/libp2p_test.go b/libp2p_test.go index ea52e56470..e8339705d8 100644 --- a/libp2p_test.go +++ b/libp2p_test.go @@ -377,7 +377,10 @@ func TestRoutedHost(t *testing.T) { } func TestAutoNATService(t *testing.T) { - h, err := New(EnableNATService()) + // h, err := New(EnableNATService()) + // require.NoError(t, err) + // h.Close() + h, err := New(EnableNATService(), EnableAutoRelayWithStaticRelays([]peer.AddrInfo{{ID: peer.ID("hello")}})) require.NoError(t, err) h.Close() } From ada53360ac9517822cc7fb9435c5c7ff4d43608c Mon Sep 17 00:00:00 2001 From: sukun Date: Sat, 20 Jul 2024 17:45:04 +0530 Subject: [PATCH 2/3] add certhashes to addresses provided by AddrsFactory --- config/config.go | 9 +-- libp2p_test.go | 28 +++++++-- p2p/host/basic/basic_host.go | 107 +++++++++++++++++++++-------------- 3 files changed, 93 insertions(+), 51 deletions(-) diff --git a/config/config.go b/config/config.go index 623cc473fc..07b3e12747 100644 --- a/config/config.go +++ b/config/config.go @@ -492,8 +492,9 @@ func (cfg *Config) NewNode() (host.Host, error) { return addrs } + // enable autorelay fxopts = append(fxopts, - fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) (*autorelay.AutoRelay, error) { + fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) error { oldAddrFactory = h.AddrsFactory if cfg.EnableAutoRelay { if !cfg.DisableMetrics { @@ -505,12 +506,12 @@ func (cfg *Config) NewNode() (host.Host, error) { ar, err := autorelay.NewAutoRelay(h, cfg.AutoRelayOpts...) if err != nil { - return nil, err + return err } lifecycle.Append(fx.StartStopHook(ar.Start, ar.Close)) - return ar, nil + return nil } - return nil, nil + return nil }), ) diff --git a/libp2p_test.go b/libp2p_test.go index e8339705d8..a5b4190d1e 100644 --- a/libp2p_test.go +++ b/libp2p_test.go @@ -377,10 +377,7 @@ func TestRoutedHost(t *testing.T) { } func TestAutoNATService(t *testing.T) { - // h, err := New(EnableNATService()) - // require.NoError(t, err) - // h.Close() - h, err := New(EnableNATService(), EnableAutoRelayWithStaticRelays([]peer.AddrInfo{{ID: peer.ID("hello")}})) + h, err := New(EnableNATService()) require.NoError(t, err) h.Close() } @@ -468,3 +465,26 @@ func TestDialCircuitAddrWithWrappedResourceManager(t *testing.T) { require.NoError(t, res.Error) defer cancel() } + +func TestHostAddrsFactoryAddsCerthashes(t *testing.T) { + addr := ma.StringCast("/ip4/1.2.3.4/udp/1/quic-v1/webtransport") + h, err := New( + AddrsFactory(func(m []ma.Multiaddr) []ma.Multiaddr { + return []ma.Multiaddr{addr} + }), + ) + require.NoError(t, err) + require.Eventually(t, func() bool { + addrs := h.Addrs() + for _, a := range addrs { + first, last := ma.SplitFunc(a, func(c ma.Component) bool { + return c.Protocol().Code == ma.P_CERTHASH + }) + if addr.Equal(first) && last != nil { + return true + } + } + return false + }, 5*time.Second, 50*time.Millisecond) + h.Close() +} diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index a5eba01d72..84118564f6 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -284,6 +284,20 @@ func NewHost(n network.Network, opts *HostOpts) (*BasicHost, error) { if opts.AddrsFactory != nil { h.AddrsFactory = opts.AddrsFactory } + // This is a terrible hack. + // We want to use this AddrsFactory for autonat. Wrapping AddrsFactory here ensures + // that autonat receives addresses with the correct certhashes. + // + // This logic cannot be in Addrs method as autonat cannot use the Addrs method directly. + // The autorelay package updates AddrsFactory to only provide p2p-circuit addresses when + // reachability is Private. + // + // Wrapping it here allows us to provide the wrapped AddrsFactory to autonat before + // autorelay updates it. + addrFactory := h.AddrsFactory + h.AddrsFactory = func(addrs []ma.Multiaddr) []ma.Multiaddr { + return h.addCertHashes(addrFactory(addrs)) + } if opts.NATManager != nil { h.natmgr = opts.NATManager(n) @@ -804,47 +818,13 @@ func (h *BasicHost) ConnManager() connmgr.ConnManager { // Addrs returns listening addresses that are safe to announce to the network. // The output is the same as AllAddrs, but processed by AddrsFactory. func (h *BasicHost) Addrs() []ma.Multiaddr { - // This is a temporary workaround/hack that fixes #2233. Once we have a - // proper address pipeline, rework this. See the issue for more context. - type transportForListeninger interface { - TransportForListening(a ma.Multiaddr) transport.Transport - } - - type addCertHasher interface { - AddCertHashes(m ma.Multiaddr) (ma.Multiaddr, bool) - } - + // We don't need to append certhashes here, the user provided addrsFactory was + // wrapped with addCertHashes in the constructor. addrs := h.AddrsFactory(h.AllAddrs()) - - s, ok := h.Network().(transportForListeninger) - if !ok { - return addrs - } - - // Copy addrs slice since we'll be modifying it. - addrsOld := addrs - addrs = make([]ma.Multiaddr, len(addrsOld)) - copy(addrs, addrsOld) - - for i, addr := range addrs { - wtOK, wtN := libp2pwebtransport.IsWebtransportMultiaddr(addr) - webrtcOK, webrtcN := libp2pwebrtc.IsWebRTCDirectMultiaddr(addr) - if (wtOK && wtN == 0) || (webrtcOK && webrtcN == 0) { - t := s.TransportForListening(addr) - tpt, ok := t.(addCertHasher) - if !ok { - continue - } - addrWithCerthash, added := tpt.AddCertHashes(addr) - if !added { - log.Debugf("Couldn't add certhashes to multiaddr: %s", addr) - continue - } - addrs[i] = addrWithCerthash - } - } - - return addrs + // Make a copy. Consumers can modify the slice elements + res := make([]ma.Multiaddr, len(addrs)) + copy(res, addrs) + return res } // NormalizeMultiaddr returns a multiaddr suitable for equality checks. @@ -864,8 +844,9 @@ func (h *BasicHost) NormalizeMultiaddr(addr ma.Multiaddr) ma.Multiaddr { return addr } -// AllAddrs returns all the addresses of BasicHost at this moment in time. -// It's ok to not include addresses if they're not available to be used now. +// AllAddrs returns all the addresses the host is listening on except circuit addresses. +// The output has webtransport addresses inferred from quic addresses. +// All the addresses have the correct func (h *BasicHost) AllAddrs() []ma.Multiaddr { listenAddrs := h.Network().ListenAddresses() if len(listenAddrs) == 0 { @@ -959,10 +940,50 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr { } finalAddrs = ma.Unique(finalAddrs) finalAddrs = inferWebtransportAddrsFromQuic(finalAddrs) - return finalAddrs } +func (h *BasicHost) addCertHashes(addrs []ma.Multiaddr) []ma.Multiaddr { + // This is a temporary workaround/hack that fixes #2233. Once we have a + // proper address pipeline, rework this. See the issue for more context. + type transportForListeninger interface { + TransportForListening(a ma.Multiaddr) transport.Transport + } + + type addCertHasher interface { + AddCertHashes(m ma.Multiaddr) (ma.Multiaddr, bool) + } + + s, ok := h.Network().(transportForListeninger) + if !ok { + return addrs + } + + // Copy addrs slice since we'll be modifying it. + addrsOld := addrs + addrs = make([]ma.Multiaddr, len(addrsOld)) + copy(addrs, addrsOld) + + for i, addr := range addrs { + wtOK, wtN := libp2pwebtransport.IsWebtransportMultiaddr(addr) + webrtcOK, webrtcN := libp2pwebrtc.IsWebRTCDirectMultiaddr(addr) + if (wtOK && wtN == 0) || (webrtcOK && webrtcN == 0) { + t := s.TransportForListening(addr) + tpt, ok := t.(addCertHasher) + if !ok { + continue + } + addrWithCerthash, added := tpt.AddCertHashes(addr) + if !added { + log.Debugf("Couldn't add certhashes to multiaddr: %s", addr) + continue + } + addrs[i] = addrWithCerthash + } + } + return addrs +} + var wtComponent = ma.StringCast("/webtransport") // inferWebtransportAddrsFromQuic infers more webtransport addresses from QUIC addresses. From 5f1723c8eb4b7bac7349ecce01d83f53a920bc5a Mon Sep 17 00:00:00 2001 From: Marco Munizaga Date: Tue, 30 Jul 2024 18:16:44 -0700 Subject: [PATCH 3/3] Nits --- config/config.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/config/config.go b/config/config.go index 07b3e12747..8a7b8455e1 100644 --- a/config/config.go +++ b/config/config.go @@ -407,9 +407,9 @@ func (cfg *Config) newBasicHost(swrm *swarm.Swarm, eventBus event.Bus) (*bhost.B // addresses by default. // // TODO: We shouldn't be doing this here. - oldFactory := h.AddrsFactory + originalAddrFactory := h.AddrsFactory h.AddrsFactory = func(addrs []ma.Multiaddr) []ma.Multiaddr { - return oldFactory(autorelay.Filter(addrs)) + return originalAddrFactory(autorelay.Filter(addrs)) } } return h, nil @@ -486,16 +486,18 @@ func (cfg *Config) NewNode() (host.Host, error) { ) } - // oldAddrFactory is the AddrFactory before it's modified by autorelay + // originalAddrFactory is the AddrFactory before it's modified by autorelay // we need this for checking reachability via autonat - oldAddrFactory := func(addrs []ma.Multiaddr) []ma.Multiaddr { + originalAddrFactory := func(addrs []ma.Multiaddr) []ma.Multiaddr { return addrs } // enable autorelay fxopts = append(fxopts, + fx.Invoke(func(h *bhost.BasicHost) { + originalAddrFactory = h.AddrsFactory + }), fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) error { - oldAddrFactory = h.AddrsFactory if cfg.EnableAutoRelay { if !cfg.DisableMetrics { mt := autorelay.WithMetricsTracer( @@ -531,7 +533,7 @@ func (cfg *Config) NewNode() (host.Host, error) { return nil, err } - if err := cfg.addAutoNAT(bh, oldAddrFactory); err != nil { + if err := cfg.addAutoNAT(bh, originalAddrFactory); err != nil { app.Stop(context.Background()) if cfg.Routing != nil { rh.Close()