Bound the IP lookup's address list and name lookups (closes #110)

An IP lookup took any number of addresses and host names, gave each PTR
lookup until the request's 30-second limit, and looked up and named
every address a host name resolved to. It now answers 400 for more than
10 entries, or more than 10 addresses once the names are resolved,
before reading the database. Each PTR lookup gets 2 seconds; one that
runs out leaves the address's result without PTR names. Of a host
name's addresses only public ones are kept: global unicast and not
private. A name with none is reported as having no public address. The
tests give the server a fake resolver.

Model: opus-5-5

README.md

@@ -64,11 +64,16 @@ DEBUG=routewatch ./routewatch
   collector, stream and handler queue statistics
 - `GET /api/v1/ip/{ip}` - Looks up each address: its most specific live route
   (`netblock`), that route's origin AS with its WHOIS details, and the address's
-  PTR names. `{ip}` is an IP address, a host name (each address it resolves to
+  PTR names. `{ip}` is an IP address, a host name, or a comma-separated list of
-  is looked up), or a comma-separated list of these. `results` has one entry per
+  at most 10 of these. Of the addresses a host name resolves to, only public
-  address with a live route and `errors` one per address or name that failed.
+  ones are looked up: a name with none, such as one that resolves to a private
-  The answer is 400 when nothing in `{ip}` is an IP address or a host name that
+  address, is reported as `host name has no public address`, without its
-  resolves, and 404 when none of the addresses has a live route.
+  addresses. `results` has one entry per address with a live route and `errors`
+  one per address or name that failed. The answer is 400 when `{ip}` lists more
+  than 10, when it comes to more than 10 addresses once its names are resolved,
+  or when nothing in it is an IP address or a host name with a public address;
+  and 404 when none of the addresses has a live route. A PTR lookup gets 2
+  seconds; an address whose lookup takes longer is answered without PTR names.
 - `GET /api/v1/as/{asn}` - The AS and the prefixes it originates, IPv4 and IPv6
   apart, each with when its newest route was updated
 - `GET /api/v1/prefix/{prefix}/{length}` - Every live route for the prefix, one

internal/server/handlers.go

@@ -31,6 +31,18 @@ const (
 	healthCheckTimeout = 2 * time.Second
 )
 
+// Limits of one IP lookup.
+const (
+	// maxLookupTargets is the most addresses and host names an IP lookup may
+	// list, and the most addresses it may have once the names are resolved:
+	// each address costs up to 113 database reads and a PTR lookup.
+	maxLookupTargets = 10
+
+	// ptrLookupTimeout is each PTR lookup's own time limit, so that ten
+	// addresses whose PTR lookups never answer fit in the 30-second limit.
+	ptrLookupTimeout = 2 * time.Second
+)
+
 // Keys of the JSON envelope the handlers and middleware write, and the status
 // value of an error answer.
 const (
@@ -41,10 +53,13 @@ const (
 	jsonStatusError = "error"
 )
 
-// Error messages an answer sends in place of the error that caused it.
+// Error messages an answer sends, in place of the error behind it where there
+// is one.
 const (
 	invalidPrefixMessage   = "Invalid prefix"
 	hostNotResolvedMessage = "could not resolve host name"
+	hostNotPublicMessage   = "host name has no public address"
+	tooManyTargetsMessage  = "Too many addresses or host names"
 )
 
 // IP versions as database.LiveRoute.IPVersion records them.
@@ -678,13 +693,19 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 		response := IPLookupResponse{Query: target, Results: make([]*database.IPInfo, 0)}
 
 		// Collect all IPs to look up
-		ipsToLookup, resolveErrors := s.resolveLookupTargets(ctx, target)
+		ipsToLookup, resolveErrors, err := s.resolveLookupTargets(ctx, target)
 		if deadlinePassed(ctx) {
 			s.writeJSONError(w, http.StatusGatewayTimeout, requestTimeoutMessage)
 
 			return
 		}
 
+		if err != nil {
+			s.writeJSONError(w, http.StatusBadRequest, tooManyTargetsMessage)
+
+			return
+		}
+
 		response.Errors = append(response.Errors, resolveErrors...)
 
 		if len(ipsToLookup) == 0 {
@@ -718,7 +739,7 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 			}
 
 			// Do PTR lookup for this IP
-			ptrs := lookupPTRNames(ctx, ip)
+			ptrs := s.lookupPTRNames(ctx, ip)
 			if deadlinePassed(ctx) {
 				s.writeJSONError(w, http.StatusGatewayTimeout, requestTimeoutMessage)
 
@@ -739,17 +760,24 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 		// Queue WHOIS refresh for stale ASNs (non-blocking)
 		s.queueWHOISRefresh(refreshASNs)
 
-		// Return response (even if no results, include errors)
+		s.writeIPLookupAnswer(w, response)
-		if len(response.Results) == 0 && len(response.Errors) > 0 {
+	}
-			s.writeJSONError(w, http.StatusNotFound, "No routes found: "+response.Errors[0])
+}
 
-			return
+// writeIPLookupAnswer sends response, or 404 with its first error when no
-		}
+// address had a route.
+func (s *Server) writeIPLookupAnswer(
+	w http.ResponseWriter, response IPLookupResponse,
+) {
+	if len(response.Results) == 0 && len(response.Errors) > 0 {
+		s.writeJSONError(w, http.StatusNotFound, "No routes found: "+response.Errors[0])
 
-		err := writeJSONSuccess(w, response)
+		return
-		if err != nil {
+	}
-			s.logger.Error("Failed to encode IP lookup response", "error", err)
+
-		}
+	err := writeJSONSuccess(w, response)
+	if err != nil {
+		s.logger.Error("Failed to encode IP lookup response", "error", err)
 	}
 }
 
@@ -781,15 +809,16 @@ func (s *Server) queueWHOISRefresh(asns map[int]bool) {
 	}
 }
 
-// resolveLookupTargets splits a comma-separated list of IP addresses and
+// errTooManyTargets is what resolveLookupTargets returns for a list of more
-// hostnames and returns the IP addresses to look up, resolving each hostname.
+// than maxLookupTargets addresses and host names, or of names that resolve to
-// A hostname that does not resolve is reported in the returned errors instead,
+// more than maxLookupTargets addresses.
-// with the resolver's own error only in the log, since it names the DNS server.
+var errTooManyTargets = errors.New("too many addresses or host names")
-// It stops once ctx's deadline has passed; the caller then answers 504.
+
-func (s *Server) resolveLookupTargets(
+// splitLookupTargets returns the addresses and host names in a comma-separated
-	ctx context.Context, target string,
+// list, leaving out blank entries, or errTooManyTargets when there are more
-) ([]string, []string) {
+// than maxLookupTargets.
-	var ipsToLookup, resolveErrors []string
+func splitLookupTargets(target string) ([]string, error) {
+	var targets []string
 
 	for t := range strings.SplitSeq(target, ",") {
 		t = strings.TrimSpace(t)
@@ -797,6 +826,36 @@ func (s *Server) resolveLookupTargets(
 			continue
 		}
 
+		if len(targets) == maxLookupTargets {
+			return nil, errTooManyTargets
+		}
+
+		targets = append(targets, t)
+	}
+
+	return targets, nil
+}
+
+// resolveLookupTargets splits a comma-separated list of IP addresses and
+// hostnames and returns the IP addresses to look up, resolving each hostname.
+// A hostname that does not resolve is reported in the returned errors instead,
+// with the resolver's own error only in the log, since it names the DNS server.
+// Only the public addresses a hostname resolves to are kept, so the answer
+// does not show the deployment's private addresses; a hostname with none is
+// reported in the errors. More than maxLookupTargets entries, or addresses once
+// resolved, return errTooManyTargets. It stops once ctx's deadline has passed;
+// the caller then answers 504.
+func (s *Server) resolveLookupTargets(
+	ctx context.Context, target string,
+) ([]string, []string, error) {
+	targets, err := splitLookupTargets(target)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	var ipsToLookup, resolveErrors []string
+
+	for _, t := range targets {
 		// Check if this target is an IP address
 		if net.ParseIP(t) != nil {
 			ipsToLookup = append(ipsToLookup, t)
@@ -805,9 +864,9 @@ func (s *Server) resolveLookupTargets(
 		}
 
 		// It's a hostname - resolve it
-		resolved, err := net.DefaultResolver.LookupHost(ctx, t)
+		resolved, err := s.resolver.LookupHost(ctx, t)
 		if deadlinePassed(ctx) {
-			return nil, nil
+			return nil, nil, nil
 		}
 
 		if err != nil {
@@ -817,16 +876,45 @@ func (s *Server) resolveLookupTargets(
 			continue
 		}
 
-		ipsToLookup = append(ipsToLookup, resolved...)
+		public := publicAddresses(resolved)
+		if len(public) == 0 {
+			resolveErrors = append(resolveErrors, t+": "+hostNotPublicMessage)
+
+			continue
+		}
+
+		ipsToLookup = append(ipsToLookup, public...)
 	}
 
-	return ipsToLookup, resolveErrors
+	if len(ipsToLookup) > maxLookupTargets {
+		return nil, nil, errTooManyTargets
+	}
+
+	return ipsToLookup, resolveErrors, nil
+}
+
+// publicAddresses returns the addresses in addrs that are global unicast and
+// not private; Go counts the private ranges as global unicast.
+func publicAddresses(addrs []string) []string {
+	var public []string
+
+	for _, a := range addrs {
+		ip, err := netip.ParseAddr(a)
+		if err == nil && ip.IsGlobalUnicast() && !ip.IsPrivate() {
+			public = append(public, a)
+		}
+	}
+
+	return public
 }
 
 // lookupPTRNames returns the PTR records of ip without their trailing dots, or
-// nil when the lookup fails.
+// nil when the lookup fails or runs past ptrLookupTimeout.
-func lookupPTRNames(ctx context.Context, ip string) []string {
+func (s *Server) lookupPTRNames(ctx context.Context, ip string) []string {
-	ptrs, err := net.DefaultResolver.LookupAddr(ctx, ip)
+	ctx, cancel := context.WithTimeout(ctx, ptrLookupTimeout)
+	defer cancel()
+
+	ptrs, err := s.resolver.LookupAddr(ctx, ip)
 	if err != nil {
 		return nil
 	}

internal/server/handlers_internal_test.go

@@ -9,7 +9,9 @@ import (
 	"net/http/httptest"
 	"runtime"
 	"slices"
+	"strconv"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -917,6 +919,224 @@ func TestIPLookupAnswers500WhenDatabaseReadFails(t *testing.T) {
 	}
 }
 
+// errNoSuchHost is what fakeResolver answers a lookup it has no answer for
+// with.
+var errNoSuchHost = errors.New("no such host")
+
+// fakeResolver answers a host name lookup with the addresses hosts lists for
+// the name, and a PTR lookup with no names or, when ptrNeverAnswers is set,
+// not at all: it waits until the lookup's context ends.
+type fakeResolver struct {
+	hosts           map[string][]string
+	ptrNeverAnswers bool
+}
+
+func (r fakeResolver) LookupHost(_ context.Context, host string) ([]string, error) {
+	addrs, ok := r.hosts[host]
+	if !ok {
+		return nil, errNoSuchHost
+	}
+
+	return addrs, nil
+}
+
+func (r fakeResolver) LookupAddr(ctx context.Context, _ string) ([]string, error) {
+	if r.ptrNeverAnswers {
+		<-ctx.Done()
+
+		return nil, ctx.Err()
+	}
+
+	return nil, errNoSuchHost
+}
+
+// countingIPLookupDB embeds database.Store (left nil) and counts its IP
+// lookups, answering each that the address has no route. Every other Store
+// method is unused and would panic if called.
+type countingIPLookupDB struct {
+	database.Store
+
+	lookups atomic.Int64
+}
+
+func (d *countingIPLookupDB) GetIPInfoContext(
+	_ context.Context, _ string,
+) (*database.IPInfo, error) {
+	d.lookups.Add(1)
+
+	return nil, database.ErrNoRoute
+}
+
+// TestIPLookupRefusesTooManyAddresses sends IP lookups of maxLookupTargets
+// addresses and of one more, listed or as a host name that resolves to them.
+// The longer lookups answer 400 without reading the database; the others read
+// it once per address. Before the fix every address was read, however many
+// there were (https://git.eeqj.de/sneak/routewatch/issues/110).
+func TestIPLookupRefusesTooManyAddresses(t *testing.T) {
+	t.Parallel()
+
+	addrs := make([]string, maxLookupTargets+1)
+	for i := range addrs {
+		addrs[i] = "192.0.2." + strconv.Itoa(i+1)
+	}
+
+	resolver := fakeResolver{hosts: map[string][]string{
+		"most.test":     addrs[:maxLookupTargets],
+		"too-many.test": addrs,
+	}}
+
+	tests := []struct {
+		target      string
+		wantCode    int
+		wantLookups int64
+	}{
+		{
+			strings.Join(addrs[:maxLookupTargets], ","),
+			http.StatusNotFound, maxLookupTargets,
+		},
+		{strings.Join(addrs, ","), http.StatusBadRequest, 0},
+		{"most.test", http.StatusNotFound, maxLookupTargets},
+		{"too-many.test", http.StatusBadRequest, 0},
+	}
+
+	for _, tt := range tests {
+		db := &countingIPLookupDB{}
+		s := New(db, streamer.New(logger.New(), metrics.New()), logger.New(),
+			&config.Config{})
+		s.resolver = resolver
+
+		rec := httptest.NewRecorder()
+		s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+			t.Context(), http.MethodGet, "/api/v1/ip/"+tt.target, nil))
+
+		if rec.Code != tt.wantCode {
+			t.Errorf("%s: status %d, want %d; body %s",
+				tt.target, rec.Code, tt.wantCode, rec.Body)
+		}
+
+		checkJSONErrorCode(t, tt.target, rec, tt.wantCode)
+
+		got := db.lookups.Load()
+		if got != tt.wantLookups {
+			t.Errorf("%s: %d database reads, want %d",
+				tt.target, got, tt.wantLookups)
+		}
+	}
+}
+
+// addrInTestPrefixV4 is an address in testPrefixV4, which
+// newServerWithTestRoutes stores a route for.
+const addrInTestPrefixV4 = "192.0.2.1"
+
+// TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer looks up a routed address
+// with a resolver that never answers its PTR lookup. Once the PTR lookup's own
+// limit passes, the lookup answers 200 with the route. Before the fix the PTR
+// lookup ran until the request's time limit, and the lookup answered 504
+// without the route (https://git.eeqj.de/sneak/routewatch/issues/110).
+func TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer(t *testing.T) {
+	t.Parallel()
+
+	s := newServerWithTestRoutes(t)
+	s.resolver = fakeResolver{ptrNeverAnswers: true}
+
+	// The request's time limit passes after the PTR lookup's.
+	ctx, cancel := context.WithTimeout(t.Context(), 2*ptrLookupTimeout)
+	defer cancel()
+
+	rec := httptest.NewRecorder()
+	s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+		ctx, http.MethodGet, "/api/v1/ip/"+addrInTestPrefixV4, nil))
+
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status %d, want %d; body %s", rec.Code, http.StatusOK, rec.Body)
+	}
+
+	var body struct {
+		Data IPLookupResponse `json:"data"`
+	}
+
+	err := json.Unmarshal(rec.Body.Bytes(), &body)
+	if err != nil {
+		t.Fatalf("decoding the answer %q: %v", rec.Body, err)
+	}
+
+	if len(body.Data.Results) != 1 || body.Data.Results[0].Netblock != testPrefixV4 {
+		t.Errorf("answer %s, want one result in %s", rec.Body, testPrefixV4)
+	}
+}
+
+// The host name TestIPLookupHidesPrivateAddresses resolves to a private
+// address, and that address.
+const (
+	internalHostName = "internal.test"
+	internalHostAddr = "10.0.0.5"
+)
+
+// TestIPLookupHidesPrivateAddresses looks up host names that resolve to a
+// private address, alone or beside a routed address. No answer carries the
+// private address, and a name with no other address is reported as having no
+// public address. Before the fix the private address was looked up and named
+// in the answer, "No routes found: 10.0.0.5: no route found for IP 10.0.0.5",
+// so the API showed what the deployment's own DNS knows
+// (https://git.eeqj.de/sneak/routewatch/issues/110).
+func TestIPLookupHidesPrivateAddresses(t *testing.T) {
+	t.Parallel()
+
+	s := newServerWithTestRoutes(t)
+	s.resolver = fakeResolver{hosts: map[string][]string{
+		internalHostName: {internalHostAddr},
+		"mixed.test":     {internalHostAddr, addrInTestPrefixV4},
+	}}
+
+	tests := []struct {
+		target     string
+		wantCode   int
+		wantErrors []string
+	}{
+		{internalHostName, http.StatusBadRequest, nil},
+		{
+			internalHostName + "," + addrInTestPrefixV4, http.StatusOK,
+			[]string{internalHostName + ": " + hostNotPublicMessage},
+		},
+		{"mixed.test", http.StatusOK, nil},
+	}
+
+	for _, tt := range tests {
+		rec := httptest.NewRecorder()
+		s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+			t.Context(), http.MethodGet, "/api/v1/ip/"+tt.target, nil))
+
+		if rec.Code != tt.wantCode {
+			t.Errorf("%s: status %d, want %d; body %s",
+				tt.target, rec.Code, tt.wantCode, rec.Body)
+		}
+
+		if strings.Contains(rec.Body.String(), internalHostAddr) {
+			t.Errorf("%s: body %s carries %s", tt.target, rec.Body, internalHostAddr)
+		}
+
+		if tt.wantCode != http.StatusOK {
+			continue
+		}
+
+		var body struct {
+			Data IPLookupResponse `json:"data"`
+		}
+
+		err := json.Unmarshal(rec.Body.Bytes(), &body)
+		if err != nil {
+			t.Errorf("%s: decoding the answer %q: %v", tt.target, rec.Body, err)
+
+			continue
+		}
+
+		if !slices.Equal(body.Data.Errors, tt.wantErrors) {
+			t.Errorf("%s: errors %q, want %q",
+				tt.target, body.Data.Errors, tt.wantErrors)
+		}
+	}
+}
+
 // settledGoroutineCount lets transient goroutines finish, then reports the
 // current count.
 func settledGoroutineCount() int {

internal/server/server.go

@@ -32,6 +32,13 @@ type ASNFetcher interface {
 	GetStats() ASNFetcherStats
 }
 
+// dnsResolver looks up the addresses of host names and the names of addresses
+// in DNS, as *net.Resolver does.
+type dnsResolver interface {
+	LookupHost(ctx context.Context, host string) ([]string, error)
+	LookupAddr(ctx context.Context, addr string) ([]string, error)
+}
+
 // Server provides HTTP endpoints for status monitoring
 type Server struct {
 	router     *chi.Mux
@@ -45,6 +52,10 @@ type Server struct {
 	// trustedProxies are the reverse proxies whose X-Forwarded-For header
 	// RealIPMiddleware believes
 	trustedProxies []netip.Prefix
+
+	// resolver makes the IP lookup's DNS lookups: net.DefaultResolver, or a
+	// fake in tests
+	resolver dnsResolver
 }
 
 // New creates a new HTTP server
@@ -60,6 +71,7 @@ func New(
 		logger:         logger,
 		port:           cfg.Port,
 		trustedProxies: cfg.TrustedProxies,
+		resolver:       net.DefaultResolver,
 	}
 
 	s.setupRoutes()