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
-  is looked up), or a comma-separated list of these. `results` has one entry per
-  address with a live route and `errors` one per address or name that failed.
-  The answer is 400 when nothing in `{ip}` is an IP address or a host name that
-  resolves, and 404 when none of the addresses has a live route.
+  PTR names. `{ip}` is an IP address, a host name, or a comma-separated list of
+  at most 10 of these. Of the addresses a host name resolves to, only public
+  ones are looked up: a name with none, such as one that resolves to a private
+  address, is reported as `host name has no public address`, without its
+  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,17 +693,23 @@ 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 {
-			s.writeJSONError(w, http.StatusBadRequest, "No valid IPs or hostnames provided")
+			s.writeJSONError(w, http.StatusBadRequest, noTargetsMessage(resolveErrors))
 
 			return
 		}
@@ -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,37 @@ 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)
-		if len(response.Results) == 0 && len(response.Errors) > 0 {
-			s.writeJSONError(w, http.StatusNotFound, "No routes found: "+response.Errors[0])
+		s.writeIPLookupAnswer(w, response)
+	}
+}
 
-			return
-		}
+// noTargetsMessage returns the message of the 400 an IP lookup answers when
+// no address is left to look up, followed by the first of resolveErrors, which
+// says why, when there is one.
+func noTargetsMessage(resolveErrors []string) string {
+	const message = "No valid IPs or hostnames provided"
 
-		err := writeJSONSuccess(w, response)
-		if err != nil {
-			s.logger.Error("Failed to encode IP lookup response", "error", err)
-		}
+	if len(resolveErrors) == 0 {
+		return message
+	}
+
+	return message + ": " + resolveErrors[0]
+}
+
+// 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])
+
+		return
+	}
+
+	err := writeJSONSuccess(w, response)
+	if err != nil {
+		s.logger.Error("Failed to encode IP lookup response", "error", err)
 	}
 }
 
@@ -781,15 +822,16 @@ func (s *Server) queueWHOISRefresh(asns map[int]bool) {
 	}
 }
 
-// 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.
-// It stops once ctx's deadline has passed; the caller then answers 504.
-func (s *Server) resolveLookupTargets(
-	ctx context.Context, target string,
-) ([]string, []string) {
-	var ipsToLookup, resolveErrors []string
+// errTooManyTargets is what resolveLookupTargets returns for a list of more
+// than maxLookupTargets addresses and host names, or of names that resolve to
+// more than maxLookupTargets addresses.
+var errTooManyTargets = errors.New("too many addresses or host names")
+
+// splitLookupTargets returns the addresses and host names in a comma-separated
+// list, leaving out blank entries, or errTooManyTargets when there are more
+// than maxLookupTargets.
+func splitLookupTargets(target string) ([]string, error) {
+	var targets []string
 
 	for t := range strings.SplitSeq(target, ",") {
 		t = strings.TrimSpace(t)
@@ -797,6 +839,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 +877,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 +889,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.
-func lookupPTRNames(ctx context.Context, ip string) []string {
-	ptrs, err := net.DefaultResolver.LookupAddr(ctx, ip)
+// nil when the lookup fails or runs past ptrLookupTimeout.
+func (s *Server) lookupPTRNames(ctx context.Context, ip string) []string {
+	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,253 @@ 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. hostLookups counts the
+// host name lookups.
+type fakeResolver struct {
+	hosts           map[string][]string
+	ptrNeverAnswers bool
+
+	hostLookups atomic.Int64
+}
+
+func (r *fakeResolver) LookupHost(_ context.Context, host string) ([]string, error) {
+	r.hostLookups.Add(1)
+
+	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,
+// and a lookup listing maxLookupTargets+1 host names. The longer lookups answer
+// 400 without reading the database, and the list of host names without
+// resolving any of them; the others read the database once per address. Before
+// the fix every address was read and every name resolved, however many there
+// were (https://git.eeqj.de/sneak/routewatch/issues/110).
+func TestIPLookupRefusesTooManyAddresses(t *testing.T) {
+	t.Parallel()
+
+	addrs := make([]string, maxLookupTargets+1)
+	names := make([]string, maxLookupTargets+1)
+
+	for i := range addrs {
+		addrs[i] = "192.0.2." + strconv.Itoa(i+1)
+		names[i] = "host" + strconv.Itoa(i+1) + ".test"
+	}
+
+	hosts := map[string][]string{
+		"most.test":     addrs[:maxLookupTargets],
+		"too-many.test": addrs,
+	}
+
+	tests := []struct {
+		target          string
+		wantCode        int
+		wantLookups     int64
+		wantHostLookups int64
+	}{
+		{
+			strings.Join(addrs[:maxLookupTargets], ","),
+			http.StatusNotFound, maxLookupTargets, 0,
+		},
+		{strings.Join(addrs, ","), http.StatusBadRequest, 0, 0},
+		{"most.test", http.StatusNotFound, maxLookupTargets, 1},
+		{"too-many.test", http.StatusBadRequest, 0, 1},
+		{strings.Join(names, ","), http.StatusBadRequest, 0, 0},
+	}
+
+	for _, tt := range tests {
+		db := &countingIPLookupDB{}
+		s := New(db, streamer.New(logger.New(), metrics.New()), logger.New(),
+			&config.Config{})
+		resolver := &fakeResolver{hosts: hosts}
+		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)
+		}
+
+		gotHostLookups := resolver.hostLookups.Load()
+		if gotHostLookups != tt.wantHostLookups {
+			t.Errorf("%s: %d host name lookups, want %d",
+				tt.target, gotHostLookups, tt.wantHostLookups)
+		}
+	}
+}
+
+// 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, in the 400 when nothing else is left to look up. 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},
+	}}
+
+	notPublic := internalHostName + ": " + hostNotPublicMessage
+
+	tests := []struct {
+		target      string
+		wantCode    int
+		wantMessage string
+		wantErrors  []string
+	}{
+		{
+			internalHostName, http.StatusBadRequest,
+			"No valid IPs or hostnames provided: " + notPublic, nil,
+		},
+		{
+			internalHostName + "," + addrInTestPrefixV4, http.StatusOK, "",
+			[]string{notPublic},
+		},
+		{"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)
+		}
+
+		var body struct {
+			Data  IPLookupResponse `json:"data"`
+			Error struct {
+				Msg string `json:"msg"`
+			} `json:"error"`
+		}
+
+		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 body.Error.Msg != tt.wantMessage {
+			t.Errorf("%s: error message %q, want %q",
+				tt.target, body.Error.Msg, tt.wantMessage)
+		}
+
+		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()