diff --git a/internal/engine/geolocate/geolocate.go b/internal/engine/geolocate/geolocate.go index d1371ae..15378cb 100644 --- a/internal/engine/geolocate/geolocate.go +++ b/internal/engine/geolocate/geolocate.go @@ -5,9 +5,9 @@ import ( "context" "errors" "fmt" - "net/http" "github.com/ooni/probe-cli/v3/internal/engine/model" + "github.com/ooni/probe-cli/v3/internal/engine/netx" "github.com/ooni/probe-cli/v3/internal/engine/runtimex" "github.com/ooni/probe-cli/v3/internal/version" ) @@ -51,7 +51,12 @@ var ( // Logger is the definition of Logger used by this package. type Logger interface { + Debug(msg string) Debugf(format string, v ...interface{}) + Info(msg string) + Infof(format string, v ...interface{}) + Warn(msg string) + Warnf(format string, v ...interface{}) } // Results contains geolocate results @@ -115,15 +120,23 @@ type ResourcesManager interface { MaybeUpdateResources(ctx context.Context) error } +// Resolver is a DNS resolver. +type Resolver interface { + LookupHost(ctx context.Context, domain string) ([]string, error) + Network() string + Address() string +} + // Config contains configuration for a geolocate Task. type Config struct { // EnableResolverLookup indicates whether we want to // perform the optional resolver lookup. EnableResolverLookup bool - // HTTPClient is the HTTP client to use. If not set, then - // we will use the http.DefaultClient. - HTTPClient *http.Client + // Resolver is the resolver we should use when + // making requests for discovering the IP. When + // this field is not set, we use the stdlib. + Resolver Resolver // Logger is the logger to use. If not set, then we will // use a logger that discards all messages. @@ -146,9 +159,6 @@ func Must(task *Task, err error) *Task { // NewTask creates a new instance of Task from config. func NewTask(config Config) (*Task, error) { - if config.HTTPClient == nil { - config.HTTPClient = http.DefaultClient - } if config.Logger == nil { config.Logger = model.DiscardLogger } @@ -158,13 +168,17 @@ func NewTask(config Config) (*Task, error) { if config.UserAgent == "" { config.UserAgent = fmt.Sprintf("ooniprobe-engine/%s", version.Version) } + if config.Resolver == nil { + config.Resolver = netx.NewResolver( + netx.Config{Logger: config.Logger}) + } return &Task{ countryLookupper: mmdbLookupper{}, enableResolverLookup: config.EnableResolverLookup, probeIPLookupper: ipLookupClient{ - HTTPClient: config.HTTPClient, - Logger: config.Logger, - UserAgent: config.UserAgent, + Resolver: config.Resolver, + Logger: config.Logger, + UserAgent: config.UserAgent, }, probeASNLookupper: mmdbLookupper{}, resolverASNLookupper: mmdbLookupper{}, diff --git a/internal/engine/geolocate/geolocate_test.go b/internal/engine/geolocate/geolocate_test.go index 99091ee..cabf2e2 100644 --- a/internal/engine/geolocate/geolocate_test.go +++ b/internal/engine/geolocate/geolocate_test.go @@ -393,3 +393,10 @@ func TestNewTaskWithNoResourcesManager(t *testing.T) { t.Fatal("expected nil task here") } } + +func TestASNStringWorks(t *testing.T) { + r := Results{ASN: 1234} + if r.ASNString() != "AS1234" { + t.Fatal("unexpected result") + } +} diff --git a/internal/engine/geolocate/iplookup.go b/internal/engine/geolocate/iplookup.go index 7bb6a92..acf0222 100644 --- a/internal/engine/geolocate/iplookup.go +++ b/internal/engine/geolocate/iplookup.go @@ -11,6 +11,7 @@ import ( "time" "github.com/ooni/probe-cli/v3/internal/engine/internal/multierror" + "github.com/ooni/probe-cli/v3/internal/engine/netx" ) var ( @@ -65,8 +66,8 @@ var ( ) type ipLookupClient struct { - // HTTPClient is the HTTP client to use - HTTPClient *http.Client + // Resolver is the resolver to use for HTTP. + Resolver Resolver // Logger is the logger to use Logger Logger @@ -88,7 +89,15 @@ func makeSlice() []method { func (c ipLookupClient) doWithCustomFunc( ctx context.Context, fn lookupFunc, ) (string, error) { - ip, err := fn(ctx, c.HTTPClient, c.Logger, c.UserAgent) + // Implementation note: we MUST use an HTTP client that we're + // sure IS NOT using any proxy. To this end, we construct a + // client ourself that we know is not proxied. + clnt := &http.Client{Transport: netx.NewHTTPTransport(netx.Config{ + Logger: c.Logger, + FullResolver: c.Resolver, + })} + defer clnt.CloseIdleConnections() + ip, err := fn(ctx, clnt, c.Logger, c.UserAgent) if err != nil { return DefaultProbeIP, err } @@ -102,7 +111,7 @@ func (c ipLookupClient) doWithCustomFunc( func (c ipLookupClient) LookupProbeIP(ctx context.Context) (string, error) { union := multierror.New(ErrAllIPLookuppersFailed) for _, method := range makeSlice() { - c.Logger.Debugf("iplookup: using %s", method.name) + c.Logger.Infof("iplookup: using %s", method.name) ip, err := c.doWithCustomFunc(ctx, method.fn) if err == nil { return ip, nil diff --git a/internal/engine/geolocate/iplookup_test.go b/internal/engine/geolocate/iplookup_test.go index 6d53a9c..b26ba9e 100644 --- a/internal/engine/geolocate/iplookup_test.go +++ b/internal/engine/geolocate/iplookup_test.go @@ -4,7 +4,6 @@ import ( "context" "errors" "net" - "net/http" "testing" "github.com/apex/log" @@ -12,9 +11,8 @@ import ( func TestIPLookupGood(t *testing.T) { ip, err := (ipLookupClient{ - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", + Logger: log.Log, + UserAgent: "ooniprobe-engine/0.1.0", }).LookupProbeIP(context.Background()) if err != nil { t.Fatal(err) @@ -28,9 +26,8 @@ func TestIPLookupAllFailed(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // immediately cancel to cause Do() to fail ip, err := (ipLookupClient{ - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", + Logger: log.Log, + UserAgent: "ooniprobe-engine/0.1.0", }).LookupProbeIP(ctx) if !errors.Is(err, context.Canceled) { t.Fatal("expected an error here") @@ -43,9 +40,8 @@ func TestIPLookupAllFailed(t *testing.T) { func TestIPLookupInvalidIP(t *testing.T) { ctx := context.Background() ip, err := (ipLookupClient{ - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", + Logger: log.Log, + UserAgent: "ooniprobe-engine/0.1.0", }).doWithCustomFunc(ctx, invalidIPLookup) if !errors.Is(err, ErrInvalidIPAddress) { t.Fatal("expected an error here") diff --git a/internal/engine/geolocate/stun.go b/internal/engine/geolocate/stun.go index 34e9abe..90332db 100644 --- a/internal/engine/geolocate/stun.go +++ b/internal/engine/geolocate/stun.go @@ -7,6 +7,11 @@ import ( "github.com/pion/stun" ) +// TODO(bassosimone): we should modify the stun code to use +// the session resolver rather than using its own. +// +// See https://github.com/ooni/probe/issues/1383. + type stunClient interface { Close() error Start(m *stun.Message, h stun.Handler) error diff --git a/internal/engine/session.go b/internal/engine/session.go index ccd200a..e80d99d 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -491,8 +491,8 @@ func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results // when we are using a proxy because that might leak information. task := geolocate.Must(geolocate.NewTask(geolocate.Config{ EnableResolverLookup: s.proxyURL == nil, - HTTPClient: s.DefaultHTTPClient(), Logger: s.Logger(), + Resolver: s.resolver, ResourcesManager: s, UserAgent: s.UserAgent(), }))