From 596727f1f0bc7ad53b4c544f670cf4a00d728d87 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Wed, 24 Jan 2024 07:29:53 +0100
Subject: [PATCH] fix(oohelperd): make sure endpoints don't connect to
 127.0.0.1 (#1463)

This diff adds extra tests and logging showing that we're not connecting
to 127.0.0.1 in endpoints code. We still need to deal with making sure
we don't connect to 127.0.0.1 in HTTP code. In principle, this is
already true, but the QA framework constructs the oohelperd differently
than with the default constructor, and we'll need to improve in this
department to make sure the results obtained in the QA suite also give
us confidence on the general oohelperd behavior. For now, QA tests
attempting to use 127.0.0.1 do not produce the correct result because
oohelperd misbehaves.

Extracted from https://github.com/ooni/probe-cli/pull/1462.

Part of https://github.com/ooni/probe/issues/2652.

Part of https://github.com/ooni/probe/issues/1517.
---
 internal/oohelperd/ipinfo.go      |  3 ++-
 internal/oohelperd/ipinfo_test.go | 33 ++++++++++++++++++++++++++++++-
 internal/oohelperd/measure.go     |  2 +-
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/internal/oohelperd/ipinfo.go b/internal/oohelperd/ipinfo.go
index 23669f0af..c3fa0edb9 100644
--- a/internal/oohelperd/ipinfo.go
+++ b/internal/oohelperd/ipinfo.go
@@ -74,7 +74,7 @@ type endpointInfo struct {
 // whether an IP address is valid for a domain;
 //
 // 4. otherwise, we don't generate any endpoint to measure.
-func ipInfoToEndpoints(URL *url.URL, ipinfo map[string]*model.THIPInfo) []endpointInfo {
+func ipInfoToEndpoints(logger model.Logger, URL *url.URL, ipinfo map[string]*model.THIPInfo) []endpointInfo {
 	var ports []string
 
 	if port := URL.Port(); port != "" {
@@ -88,6 +88,7 @@ func ipInfoToEndpoints(URL *url.URL, ipinfo map[string]*model.THIPInfo) []endpoi
 	out := []endpointInfo{}
 	for addr, info := range ipinfo {
 		if (info.Flags & model.THIPInfoFlagIsBogon) != 0 {
+			logger.Infof("IPInfo: skip bogon IP address: %s", addr)
 			continue // as documented
 		}
 		for _, port := range ports {
diff --git a/internal/oohelperd/ipinfo_test.go b/internal/oohelperd/ipinfo_test.go
index 99aa4a6db..eeeeb659f 100644
--- a/internal/oohelperd/ipinfo_test.go
+++ b/internal/oohelperd/ipinfo_test.go
@@ -4,6 +4,7 @@ import (
 	"net/url"
 	"testing"
 
+	"github.com/apex/log"
 	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
@@ -84,6 +85,32 @@ func Test_newIPInfo(t *testing.T) {
 			addrs: []string{},
 		},
 		want: map[string]*model.THIPInfo{},
+	}, {
+		name: "with localhost",
+		args: args{
+			creq: &model.THRequest{
+				HTTPRequest:        "",
+				HTTPRequestHeaders: map[string][]string{},
+				TCPConnect: []string{
+					"127.0.0.1:443",
+					"[::1]:443",
+				},
+			},
+			addrs: []string{
+				"127.0.0.1",
+				"::1",
+			},
+		},
+		want: map[string]*model.THIPInfo{
+			"127.0.0.1": {
+				ASN:   0,
+				Flags: model.THIPInfoFlagIsBogon | model.THIPInfoFlagResolvedByProbe | model.THIPInfoFlagResolvedByTH,
+			},
+			"::1": {
+				ASN:   0,
+				Flags: model.THIPInfoFlagIsBogon | model.THIPInfoFlagResolvedByProbe | model.THIPInfoFlagResolvedByTH,
+			},
+		},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -137,6 +164,10 @@ func Test_ipInfoToEndpoints(t *testing.T) {
 					ASN:   15169,
 					Flags: model.THIPInfoFlagResolvedByTH,
 				},
+				"127.0.0,1": {
+					ASN:   0,
+					Flags: model.THIPInfoFlagIsBogon | model.THIPInfoFlagResolvedByProbe,
+				},
 			},
 		},
 		want: []endpointInfo{{
@@ -239,7 +270,7 @@ func Test_ipInfoToEndpoints(t *testing.T) {
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := ipInfoToEndpoints(tt.args.URL, tt.args.ipinfo)
+			got := ipInfoToEndpoints(log.Log, tt.args.URL, tt.args.ipinfo)
 			if diff := cmp.Diff(tt.want, got); diff != "" {
 				t.Fatal(diff)
 			}
diff --git a/internal/oohelperd/measure.go b/internal/oohelperd/measure.go
index 3a0cc2198..1d041f0e1 100644
--- a/internal/oohelperd/measure.go
+++ b/internal/oohelperd/measure.go
@@ -80,7 +80,7 @@ func measure(ctx context.Context, config *Handler, creq *ctrlRequest) (*ctrlResp
 
 	// obtain IP info and figure out the endpoints measurement plan
 	cresp.IPInfo = newIPInfo(creq, cresp.DNS.Addrs)
-	endpoints := ipInfoToEndpoints(URL, cresp.IPInfo)
+	endpoints := ipInfoToEndpoints(logger, URL, cresp.IPInfo)
 
 	// tcpconnect: start over all the endpoints
 	tcpconnch := make(chan *tcpResultPair, len(endpoints))