Repository: ipfs/boxo

Issue:
bitswap/httpnet: add WithDenylist option
We have ability to enable HTTP retrieval for specific hostnames (only do HTTP with hosts from allowlist) via `httpnet.WithAllowlist`

To prepare for the future where we enable this by default in places like Kubo, there should be a companion, opposite flag that allows us to pass explicit denylist (with known hosts that should NOT be used for HTTP retrieval) – `httpnet.WithDenylist`

The denylist should apply to both original URL and any HTTP redirects.

The main reason here is to avoid situation where  defunct gateways start returning redirects to `ipfs.io` and `dweb.link`, effectively acting as amplification vector (note: there may  be a better fix in https://github.com/ipfs/boxo/issues/862).

cc @hsanjuan 

Discussion:

Solution patch:
diff --git a/bitswap/network/http_multiaddr.go b/bitswap/network/http_multiaddr.go
index de1da36e..8cc8fc3c 100644
--- a/bitswap/network/http_multiaddr.go
+++ b/bitswap/network/http_multiaddr.go
@@ -12,8 +12,9 @@ import (
 // ParsedURL contains the result of parsing an "http" transport multiaddress.
 // SNI is set when the multiaddress specifies an SNI value.
 type ParsedURL struct {
-	URL *url.URL
-	SNI string
+	Multiaddress multiaddr.Multiaddr
+	URL          *url.URL
+	SNI          string
 }
 
 // ExtractHTTPAddress extracts the HTTP schema+host+port from a multiaddress
@@ -68,8 +69,9 @@ func ExtractHTTPAddress(ma multiaddr.Multiaddr) (ParsedURL, error) {
 	}
 
 	parsedURL := ParsedURL{
-		URL: pURL,
-		SNI: sni,
+		Multiaddress: ma,
+		URL:          pURL,
+		SNI:          sni,
 	}
 
 	// Error on addresses which are not https nor local
diff --git a/bitswap/network/httpnet/httpnet.go b/bitswap/network/httpnet/httpnet.go
index 7b16d605..fbfe9986 100644
--- a/bitswap/network/httpnet/httpnet.go
+++ b/bitswap/network/httpnet/httpnet.go
@@ -32,6 +32,7 @@ var log = logging.Logger("httpnet")
 
 var ErrNoHTTPAddresses = errors.New("AddrInfo does not contain any valid HTTP addresses")
 var ErrNoSuccess = errors.New("none of the peer HTTP endpoints responded successfully to request")
+var ErrNotConnected = errors.New("no HTTP connection has been setup to this peer")
 
 var _ network.BitSwapNetwork = (*Network)(nil)
 
@@ -121,6 +122,7 @@ func WithInsecureSkipVerify(b bool) Option {
 // these hosts.
 func WithAllowlist(hosts []string) Option {
 	return func(net *Network) {
+		log.Infof("HTTP retrieval allowlist: %s", strings.Join(hosts, ", "))
 		net.allowlist = make(map[string]struct{})
 		for _, h := range hosts {
 			net.allowlist[h] = struct{}{}
@@ -128,6 +130,16 @@ func WithAllowlist(hosts []string) Option {
 	}
 }
 
+func WithDenylist(hosts []string) Option {
+	return func(net *Network) {
+		log.Infof("HTTP retrieval denylist: %s", strings.Join(hosts, ", "))
+		net.denylist = make(map[string]struct{})
+		for _, h := range hosts {
+			net.denylist[h] = struct{}{}
+		}
+	}
+}
+
 // WithMaxHTTPAddressesPerPeer limits how many http addresses we attempt to
 // connect to per peer.
 func WithMaxHTTPAddressesPerPeer(max int) Option {
@@ -170,6 +182,7 @@ type Network struct {
 	maxHTTPAddressesPerPeer int
 	httpWorkers             int
 	allowlist               map[string]struct{}
+	denylist                map[string]struct{}
 
 	metrics      *metrics
 	httpRequests chan httpRequestInfo
@@ -210,6 +223,7 @@ func New(host host.Host, opts ...Option) network.BitSwapNetwork {
 		opt(htnet)
 	}
 
+	// TODO: take allowlist into account!
 	htnet.metrics = newMetrics(htnet.allowlist)
 
 	reqTracker := newRequestTracker()
@@ -290,11 +304,7 @@ func New(host host.Host, opts ...Option) network.BitSwapNetwork {
 // received. It also starts the connection event manager. Start must be called
 // before using the Network.
 func (ht *Network) Start(receivers ...network.Receiver) {
-	allowlist := make([]string, 0, len(ht.allowlist))
-	for k := range ht.allowlist {
-		allowlist = append(allowlist, k)
-	}
-	log.Infof("httpnet: HTTP retrieval system started with allowlist: %s", strings.Join(allowlist, ","))
+	log.Info("HTTP raw block retrieval system started")
 	ht.receivers = receivers
 	connectionListeners := make([]network.ConnectionListener, len(receivers))
 	for i, v := range receivers {
@@ -366,97 +376,100 @@ func (ht *Network) Self() peer.ID {
 // connection success and marks this peer as "connected", setting it up to
 // handle messages and make requests. The peer will be pinged regularly to
 // collect latency measurements until DisconnectFrom() is called.
-func (ht *Network) Connect(ctx context.Context, p peer.AddrInfo) error {
-	existingPeerAddrs := ht.host.Peerstore().Addrs(p.ID)
-	existingPeerURLs := network.ExtractURLsFromPeer(peer.AddrInfo{
-		ID:    p.ID,
-		Addrs: existingPeerAddrs,
-	})
-	if len(existingPeerURLs) > 0 {
-		// assume already connected FIXME: issues when different
-		// provider records contain different HTTP urls for the same
-		// peer. Hope no one does that.
+func (ht *Network) Connect(ctx context.Context, pi peer.AddrInfo) error {
+	// Connect is called when finding provider records. We should avoid
+	// reconnecting all the time. We should avoid re-testing broken
+	// addresses all the time as well. For this reason we assume that if
+	// we are connected to an HTTP, the working urls we added the first
+	// time are correct, and we will only re-do the effort when we are not
+	// connected.
+	p := pi.ID
+	connected := ht.pinger.isPinging(p)
+	if connected {
 		return nil
 	}
 
-	htaddrs, _ := network.SplitHTTPAddrs(p)
-	if len(htaddrs.Addrs) == 0 {
-		return ErrNoHTTPAddresses
-	}
+	urls := network.ExtractURLsFromPeer(pi)
 
-	// avoid funny things like someone adding 100 broken urls to a peer.
-	if len(htaddrs.Addrs) > ht.maxHTTPAddressesPerPeer {
-		htaddrs.Addrs = htaddrs.Addrs[0:ht.maxHTTPAddressesPerPeer]
-	}
+	// Filter addresses based on allow and denylists
+	var filteredURLs []network.ParsedURL
+	for _, u := range urls {
+		host, _, err := net.SplitHostPort(u.URL.Host)
+		if err != nil {
+			return err
+		}
 
-	urls := network.ExtractURLsFromPeer(htaddrs)
-	if len(ht.allowlist) > 0 {
-		var filteredURLs []network.ParsedURL
-		var filteredAddrs []multiaddr.Multiaddr
-		for i, u := range urls {
-			host, _, err := net.SplitHostPort(u.URL.Host)
-			if err != nil {
-				return err
-			}
-			if _, ok := ht.allowlist[host]; ok {
-				filteredURLs = append(filteredURLs, u)
-				filteredAddrs = append(filteredAddrs, htaddrs.Addrs[i])
-			}
+		// Filter out if allowlist is enabled and it is not allowed,
+		// OR if host is in denylist.
+		_, inAllowlist := ht.allowlist[host]
+		allowed := (len(ht.allowlist) == 0) || inAllowlist
+		_, denied := ht.denylist[host]
+		if allowed && !denied {
+			filteredURLs = append(filteredURLs, u)
 		}
-		urls = filteredURLs
-		htaddrs.Addrs = filteredAddrs
+	}
+	urls = filteredURLs
+	if len(urls) == 0 {
+		return ErrNoHTTPAddresses
+	}
+	if len(urls) > ht.maxHTTPAddressesPerPeer {
+		urls = urls[0:ht.maxHTTPAddressesPerPeer]
 	}
 
-	// if len(filteredURLs == 0) nothing will happen below and we will return
-	// an error below.
-
-	// We will know try to talk to this peer by making HTTP requests to its urls
-	// and recording which ones work.
-	// This allows re-using the connections that we are about to open next
-	// time with the client. We call peer.Connected()
-	// on success.
+	// Try to talk to the peer by making HTTP requests to its urls and
+	// recording which ones work. This allows re-using the connections
+	// that we are about to open next time with the client. We call
+	// peer.Connected() on success.
 	var workingAddrs []multiaddr.Multiaddr
 	supportsHead := true
-	for i, u := range urls {
-		err := ht.connect(ctx, p.ID, u, "GET")
+	for _, u := range urls {
+		// If head works we assume GET works too.
+		err := ht.connectToURL(ctx, pi.ID, u, "HEAD")
 		if err != nil {
 			// abort if context cancelled
-			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
-				return err
+			if ctxErr := ctx.Err(); ctxErr != nil {
+				return ctxErr
 			}
+		} else {
+			workingAddrs = append(workingAddrs, u.Multiaddress)
 			continue
 		}
-		// GET works. Does HEAD work though?
-		err = ht.connect(ctx, p.ID, u, "HEAD")
+
+		// HEAD did not work. Try GET.
+		supportsHead = false
+
+		err = ht.connectToURL(ctx, pi.ID, u, "GET")
 		if err != nil {
 			// abort if context cancelled
-			if ctxErr := ctx.Err(); ctxErr != nil {
-				return ctxErr
+			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+				return err
 			}
-			supportsHead = false
+			continue
 		}
-
-		workingAddrs = append(workingAddrs, htaddrs.Addrs[i])
+		workingAddrs = append(workingAddrs, u.Multiaddress)
 	}
 
-	if len(workingAddrs) > 0 {
-		ht.host.Peerstore().AddAddrs(p.ID, workingAddrs, peerstore.PermanentAddrTTL)
-		// ignoring error
-		_ = ht.host.Peerstore().Put(p.ID, peerstoreSupportsHeadKey, supportsHead)
-
-		ht.connEvtMgr.Connected(p.ID)
-		ht.pinger.startPinging(p.ID)
-		log.Debugf("connect success to %s (supports HEAD: %t)", p.ID, supportsHead)
-		// We "connected"
-		return nil
+	// Bail out if no working urls found.
+	if len(workingAddrs) == 0 {
+		return fmt.Errorf("connect failure to %s: %w", p, ErrNoSuccess)
 	}
 
-	err := fmt.Errorf("connect failure to %s: %w", p.ID, ErrNoSuccess)
-	log.Debug(err)
-	return err
+	// We have some working urls!
+
+	// Add the working addresses to the peerstore. Clean the others.
+	ht.host.Peerstore().ClearAddrs(p)
+	ht.host.Peerstore().AddAddrs(p, workingAddrs, peerstore.PermanentAddrTTL)
+	// Record whether HEAD test passed for all urls - ignoring error
+	_ = ht.host.Peerstore().Put(pi.ID, peerstoreSupportsHeadKey, supportsHead)
+
+	ht.connEvtMgr.Connected(p)
+	ht.pinger.startPinging(p)
+	log.Debugf("connect success to %s (supports HEAD: %t)", p, supportsHead)
+	// We "connected"
+	return nil
 }
 
-func (ht *Network) connect(ctx context.Context, p peer.ID, u network.ParsedURL, method string) error {
+func (ht *Network) connectToURL(ctx context.Context, p peer.ID, u network.ParsedURL, method string) error {
 	req, err := buildRequest(ctx, u, method, pingCid, ht.userAgent)
 	if err != nil {
 		log.Debug(err)
@@ -679,6 +692,16 @@ func (ht *Network) NewMessageSender(ctx context.Context, p peer.ID, opts *networ
 	//
 	// This way we minimize lock contention around the cooldown map, with
 	// one read access per message sender only.
+
+	// Error when we have not called Connect() for this peer. Use the
+	// pinger as proxy for this information, since we should be pinging
+	// peers that we have connected to and we stop pinging them on
+	// disconnect.
+	if !ht.pinger.isPinging(p) {
+		return nil, ErrNotConnected
+	}
+
+	// Check that we have HTTP urls.
 	urls := ht.senderURLs(p)
 	if len(urls) == 0 {
 		return nil, ErrNoHTTPAddresses
diff --git a/bitswap/network/httpnet/msg_sender.go b/bitswap/network/httpnet/msg_sender.go
index 7980c69a..f4457b64 100644
--- a/bitswap/network/httpnet/msg_sender.go
+++ b/bitswap/network/httpnet/msg_sender.go
@@ -181,7 +181,6 @@ func (err senderError) Error() string {
 // processed. tryURL returns an error so that it can be decided what to do next:
 // i.e. retry, or move to next item in wantlist, or abort completely.
 func (sender *httpMsgSender) tryURL(ctx context.Context, u *senderURL, entry bsmsg.Entry) (blocks.Block, *senderError) {
-	// sleep whatever needed
 	if dl := u.cooldown.Load().(time.Time); !dl.IsZero() {
 		return nil, &senderError{
 			Type: typeRetryLater,
@@ -519,7 +518,6 @@ WANTLIST_LOOP:
 					bsresp.AddHave(entry.Cid)
 				}
 			} else {
-
 				// error handling
 				switch result.err.Type {
 				case typeFatal:
diff --git a/bitswap/network/httpnet/pinger.go b/bitswap/network/httpnet/pinger.go
index 67ce832e..3932fcbe 100644
--- a/bitswap/network/httpnet/pinger.go
+++ b/bitswap/network/httpnet/pinger.go
@@ -19,7 +19,7 @@ type pinger struct {
 	latenciesLock sync.RWMutex
 	latencies     map[peer.ID]time.Duration
 
-	pingsLock sync.Mutex
+	pingsLock sync.RWMutex
 	pings     map[peer.ID]context.CancelFunc
 }
 
@@ -52,7 +52,7 @@ func (pngr *pinger) ping(ctx context.Context, p peer.ID) ping.Result {
 	for _, u := range urls {
 		go func(u network.ParsedURL) {
 			start := time.Now()
-			err := pngr.ht.connect(ctx, p, u, method)
+			err := pngr.ht.connectToURL(ctx, p, u, method)
 			if err != nil {
 				log.Debug(err)
 				results <- ping.Result{Error: err}
@@ -162,3 +162,11 @@ func (pngr *pinger) stopPinging(p peer.ID) {
 	pngr.latenciesLock.Unlock()
 
 }
+
+func (pngr *pinger) isPinging(p peer.ID) bool {
+	pngr.pingsLock.RLock()
+	defer pngr.pingsLock.RUnlock()
+
+	_, ok := pngr.pings[p]
+	return ok
+}
diff --git a/bitswap/network/router.go b/bitswap/network/router.go
index 55fe855f..529e8674 100644
--- a/bitswap/network/router.go
+++ b/bitswap/network/router.go
@@ -146,14 +146,19 @@ func (rt *router) Stats() Stats {
 // NewMessageSender returns a MessageSender using the HTTP network when HTTP
 // addresses are known, and bitswap otherwise.
 func (rt *router) NewMessageSender(ctx context.Context, p peer.ID, opts *MessageSenderOpts) (MessageSender, error) {
-	// IF we did not manage to connect to any HTTP address beforehand, we
-	// should not have them in the peerstore.
-	pi := rt.Peerstore.PeerInfo(p)
-	htaddrs, _ := SplitHTTPAddrs(pi)
-	if len(htaddrs.Addrs) > 0 {
-		return rt.HTTP.NewMessageSender(ctx, p, opts)
+	// There seem to be cases when we have HTTP addresses in the peerstore
+	// even though we have not connected to the peer via HTTP before. This
+	// can then bypass allowlists etc.  HTTP.NewMessageSender() checks if
+	// the peer is known to HTTP or errors.
+	// We switch to bitswap in that case.
+
+	// Note that NewMessageSender extracts Addrs from the peerstore so we
+	// don't have to do it here as well.
+	ms, err := rt.HTTP.NewMessageSender(ctx, p, opts)
+	if err != nil {
+		return rt.Bitswap.NewMessageSender(ctx, p, opts)
 	}
-	return rt.Bitswap.NewMessageSender(ctx, p, opts)
+	return ms, nil
 }
 
 func (rt *router) TagPeer(p peer.ID, tag string, w int) {



Test patch:
diff --git a/bitswap/network/httpnet/httpnet_test.go b/bitswap/network/httpnet/httpnet_test.go
index 30b4091a..d900b9e7 100644
--- a/bitswap/network/httpnet/httpnet_test.go
+++ b/bitswap/network/httpnet/httpnet_test.go
@@ -22,7 +22,6 @@ import (
 	ipld "github.com/ipfs/go-ipld-format"
 	"github.com/libp2p/go-libp2p/core/host"
 	"github.com/libp2p/go-libp2p/core/peer"
-	"github.com/libp2p/go-libp2p/core/peerstore"
 	mocknet "github.com/libp2p/go-libp2p/p2p/net/mock"
 	"github.com/multiformats/go-multiaddr"
 	manet "github.com/multiformats/go-multiaddr/net"
@@ -203,6 +202,7 @@ func (h *Handler) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
 	if c.Equals(backoffCid) {
 		rw.Header().Set("Retry-After", "5")
 		rw.WriteHeader(http.StatusTooManyRequests)
+		return
 	}
 
 	if c.Equals(slowCid) {
@@ -256,12 +256,22 @@ func srvMultiaddr(t *testing.T, srv *httptest.Server) multiaddr.Multiaddr {
 	return maddr.Encapsulate(httpma)
 }
 
-func associateServerToPeer(t *testing.T, srv *httptest.Server, h, remote host.Host) {
-	h.Peerstore().AddAddr(
-		remote.ID(),
-		srvMultiaddr(t, srv),
-		peerstore.PermanentAddrTTL,
+func connectToPeer(t *testing.T, ctx context.Context, htnet *Network, remote host.Host, srvs ...*httptest.Server) {
+	var addrs []multiaddr.Multiaddr
+	for _, srv := range srvs {
+		addrs = append(addrs, srvMultiaddr(t, srv))
+	}
+
+	err := htnet.Connect(
+		ctx,
+		peer.AddrInfo{
+			ID:    remote.ID(),
+			Addrs: addrs,
+		},
 	)
+	if err != nil {
+		t.Fatal(err)
+	}
 }
 
 func TestBestURL(t *testing.T) {
@@ -272,7 +282,7 @@ func TestBestURL(t *testing.T) {
 		t.Fatal(err)
 	}
 	msrv := makeServer(t, 0, 0)
-	associateServerToPeer(t, msrv, htnet.host, peer)
+	connectToPeer(t, ctx, htnet, peer, msrv)
 
 	nms, err := htnet.NewMessageSender(
 		ctx,
@@ -366,7 +376,7 @@ func TestSendMessage(t *testing.T) {
 		t.Fatal(err)
 	}
 	msrv := makeServer(t, 0, 10)
-	associateServerToPeer(t, msrv, htnet.host, peer)
+	connectToPeer(t, ctx, htnet, peer, msrv)
 
 	wl := makeCids(t, 0, 10)
 	msg := makeWantsMessage(wl)
@@ -395,8 +405,7 @@ func TestSendMessageWithFailingServer(t *testing.T) {
 	}
 	msrv := makeServer(t, 0, 0)
 	msrv2 := makeServer(t, 0, 10)
-	associateServerToPeer(t, msrv, htnet.host, peer)
-	associateServerToPeer(t, msrv2, htnet.host, peer)
+	connectToPeer(t, ctx, htnet, peer, msrv, msrv2)
 
 	wl := makeCids(t, 0, 10)
 	msg := makeWantsMessage(wl)
@@ -427,7 +436,7 @@ func TestSendMessageWithPartialResponse(t *testing.T) {
 		t.Fatal(err)
 	}
 	msrv := makeServer(t, 5, 10)
-	associateServerToPeer(t, msrv, htnet.host, peer)
+	connectToPeer(t, ctx, htnet, peer, msrv)
 
 	wl := makeCids(t, 0, 10)
 	msg := makeWantsMessage(wl)
@@ -462,7 +471,7 @@ func TestSendMessageSendHavesAndDontHaves(t *testing.T) {
 		t.Fatal(err)
 	}
 	msrv := makeServer(t, 0, 5)
-	associateServerToPeer(t, msrv, htnet.host, peer)
+	connectToPeer(t, ctx, htnet, peer, msrv)
 
 	wl := makeCids(t, 0, 10)
 	msg := makeHavesMessage(wl)
@@ -497,6 +506,7 @@ func TestBackOff(t *testing.T) {
 	// We trigger backoff using peer1
 	// and the backoff should happen when making a
 	// request on peer2.
+	// The backoff means the blocks are recorded as "don't have".
 
 	peer, err := mn.GenPeer()
 	if err != nil {
@@ -509,8 +519,8 @@ func TestBackOff(t *testing.T) {
 	}
 
 	msrv := makeServer(t, 0, 1)
-	associateServerToPeer(t, msrv, htnet.host, peer)
-	associateServerToPeer(t, msrv, htnet.host, peer2)
+	connectToPeer(t, ctx, htnet, peer, msrv)
+	connectToPeer(t, ctx, htnet, peer2, msrv)
 
 	nms, err := htnet.NewMessageSender(ctx, peer.ID(), nil)
 	if err != nil {
@@ -526,6 +536,13 @@ func TestBackOff(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	recv.wait(1)
+	if len(recv.donthaves) == 0 {
+		t.Fatal("back off should have counted as DONT_HAVE")
+	}
+
+	// should produce a dont_have as well even though we have this cid.
+	// (because we are in backoff for the url-host).
 	nms2, err := htnet.NewMessageSender(ctx, peer2.ID(), nil)
 	if err != nil {
 		t.Fatal(err)
@@ -535,11 +552,10 @@ func TestBackOff(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err != nil {
-		t.Fatal(err)
-	}
 
-	if len(recv.blocks) > 0 || len(recv.donthaves) > 0 {
+	recv.wait(1)
+
+	if len(recv.donthaves) != 2 || (len(recv.blocks)+len(recv.haves)) > 0 {
 		t.Error("no blocks should have been received while on backoff")
 	}
 }