diff --git a/README.md b/README.md index e41e715..9621ff8 100644 --- a/README.md +++ b/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 diff --git a/internal/server/handlers.go b/internal/server/handlers.go index c07527c..2132c0c 100644 --- a/internal/server/handlers.go +++ b/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 } diff --git a/internal/server/handlers_internal_test.go b/internal/server/handlers_internal_test.go index de5972f..a734953 100644 --- a/internal/server/handlers_internal_test.go +++ b/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 { diff --git a/internal/server/server.go b/internal/server/server.go index a6cf203..cd60c3f 100644 --- a/internal/server/server.go +++ b/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()