From 57a3919d2ae5610cc2264f9ec8e9c741ba7b42ed Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 12 Oct 2022 18:07:42 +0200 Subject: [PATCH] fix(geolocate): always use netxlite functionality (#976) This change ensures that, in turn, we're able to "remote" all the traffic generated by the `geolocate` package, rather than missing some bits of it that were still using the standard library and caused _some_ geolocations to geolocate as the local host rather than as the remote host. Extracted from https://github.com/ooni/probe-cli/pull/969, where we tested this functionality. Closes https://github.com/ooni/probe/issues/1383 (which was long overdue). Part of https://github.com/ooni/probe/issues/2340, because it allows us to make progress with that. --- internal/engine/geolocate/cloudflare.go | 1 + internal/engine/geolocate/cloudflare_test.go | 2 + internal/engine/geolocate/geolocate.go | 4 +- internal/engine/geolocate/invalid_test.go | 1 + internal/engine/geolocate/iplookup.go | 3 +- internal/engine/geolocate/resolverlookup.go | 9 ++-- .../engine/geolocate/resolverlookup_test.go | 16 +++++-- internal/engine/geolocate/stun.go | 40 +++++++++++------ internal/engine/geolocate/stun_test.go | 45 ++++++++++++++----- internal/engine/geolocate/ubuntu.go | 1 + internal/engine/geolocate/ubuntu_test.go | 3 ++ 11 files changed, 93 insertions(+), 32 deletions(-) diff --git a/internal/engine/geolocate/cloudflare.go b/internal/engine/geolocate/cloudflare.go index b98c1e6..3de61a5 100644 --- a/internal/engine/geolocate/cloudflare.go +++ b/internal/engine/geolocate/cloudflare.go @@ -15,6 +15,7 @@ func cloudflareIPLookup( httpClient *http.Client, logger model.Logger, userAgent string, + resolver model.Resolver, ) (string, error) { data, err := (&httpx.APIClientTemplate{ BaseURL: "https://www.cloudflare.com", diff --git a/internal/engine/geolocate/cloudflare_test.go b/internal/engine/geolocate/cloudflare_test.go index 49c50b7..4f0b44d 100644 --- a/internal/engine/geolocate/cloudflare_test.go +++ b/internal/engine/geolocate/cloudflare_test.go @@ -8,6 +8,7 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestIPLookupWorksUsingcloudlflare(t *testing.T) { @@ -16,6 +17,7 @@ func TestIPLookupWorksUsingcloudlflare(t *testing.T) { http.DefaultClient, log.Log, model.HTTPHeaderUserAgent, + netxlite.NewStdlibResolver(model.DiscardLogger), ) if err != nil { t.Fatal(err) diff --git a/internal/engine/geolocate/geolocate.go b/internal/engine/geolocate/geolocate.go index 175c7db..0b88bba 100644 --- a/internal/engine/geolocate/geolocate.go +++ b/internal/engine/geolocate/geolocate.go @@ -90,7 +90,9 @@ func NewTask(config Config) *Task { probeIPLookupper: ipLookupClient(config), probeASNLookupper: mmdbLookupper{}, resolverASNLookupper: mmdbLookupper{}, - resolverIPLookupper: resolverLookupClient{}, + resolverIPLookupper: resolverLookupClient{ + Resolver: config.Resolver, + }, } } diff --git a/internal/engine/geolocate/invalid_test.go b/internal/engine/geolocate/invalid_test.go index aade7c7..bb62404 100644 --- a/internal/engine/geolocate/invalid_test.go +++ b/internal/engine/geolocate/invalid_test.go @@ -12,6 +12,7 @@ func invalidIPLookup( httpClient *http.Client, logger model.Logger, userAgent string, + resolver model.Resolver, ) (string, error) { return "invalid IP", nil } diff --git a/internal/engine/geolocate/iplookup.go b/internal/engine/geolocate/iplookup.go index b5b40b0..3d9711f 100644 --- a/internal/engine/geolocate/iplookup.go +++ b/internal/engine/geolocate/iplookup.go @@ -27,6 +27,7 @@ var ( type lookupFunc func( ctx context.Context, client *http.Client, logger model.Logger, userAgent string, + resolver model.Resolver, ) (string, error) type method struct { @@ -89,7 +90,7 @@ func (c ipLookupClient) doWithCustomFunc( txp := netxlite.NewHTTPTransportWithResolver(c.Logger, c.Resolver) clnt := &http.Client{Transport: txp} defer clnt.CloseIdleConnections() - ip, err := fn(ctx, clnt, c.Logger, c.UserAgent) + ip, err := fn(ctx, clnt, c.Logger, c.UserAgent, c.Resolver) if err != nil { return model.DefaultProbeIP, err } diff --git a/internal/engine/geolocate/resolverlookup.go b/internal/engine/geolocate/resolverlookup.go index fb25eb1..ef6835b 100644 --- a/internal/engine/geolocate/resolverlookup.go +++ b/internal/engine/geolocate/resolverlookup.go @@ -3,7 +3,8 @@ package geolocate import ( "context" "errors" - "net" + + "github.com/ooni/probe-cli/v3/internal/model" ) var ( @@ -16,7 +17,9 @@ type dnsResolver interface { LookupHost(ctx context.Context, host string) (addrs []string, err error) } -type resolverLookupClient struct{} +type resolverLookupClient struct { + Resolver model.Resolver +} func (rlc resolverLookupClient) do(ctx context.Context, r dnsResolver) (string, error) { var ips []string @@ -31,5 +34,5 @@ func (rlc resolverLookupClient) do(ctx context.Context, r dnsResolver) (string, } func (rlc resolverLookupClient) LookupResolverIP(ctx context.Context) (ip string, err error) { - return rlc.do(ctx, &net.Resolver{}) + return rlc.do(ctx, rlc.Resolver) } diff --git a/internal/engine/geolocate/resolverlookup_test.go b/internal/engine/geolocate/resolverlookup_test.go index 0333161..3d34c3f 100644 --- a/internal/engine/geolocate/resolverlookup_test.go +++ b/internal/engine/geolocate/resolverlookup_test.go @@ -4,10 +4,16 @@ import ( "context" "errors" "testing" + + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestLookupResolverIP(t *testing.T) { - addr, err := (resolverLookupClient{}).LookupResolverIP(context.Background()) + rlc := resolverLookupClient{ + Resolver: netxlite.NewStdlibResolver(model.DiscardLogger), + } + addr, err := rlc.LookupResolverIP(context.Background()) if err != nil { t.Fatal(err) } @@ -26,7 +32,9 @@ func (bhl brokenHostLookupper) LookupHost(ctx context.Context, host string) ([]s func TestLookupResolverIPFailure(t *testing.T) { expected := errors.New("mocked error") - rlc := resolverLookupClient{} + rlc := resolverLookupClient{ + Resolver: netxlite.NewStdlibResolver(model.DiscardLogger), + } addr, err := rlc.do(context.Background(), brokenHostLookupper{ err: expected, }) @@ -39,7 +47,9 @@ func TestLookupResolverIPFailure(t *testing.T) { } func TestLookupResolverIPNoAddressReturned(t *testing.T) { - rlc := resolverLookupClient{} + rlc := resolverLookupClient{ + Resolver: netxlite.NewStdlibResolver(model.DiscardLogger), + } addr, err := rlc.do(context.Background(), brokenHostLookupper{}) if !errors.Is(err, ErrNoIPAddressReturned) { t.Fatalf("not the error we expected: %+v", err) diff --git a/internal/engine/geolocate/stun.go b/internal/engine/geolocate/stun.go index cb3746d..d6133a1 100644 --- a/internal/engine/geolocate/stun.go +++ b/internal/engine/geolocate/stun.go @@ -2,43 +2,51 @@ package geolocate import ( "context" + "net" "net/http" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" "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 } type stunConfig struct { - Dial func(network string, address string) (stunClient, error) - Endpoint string - Logger model.Logger + Dialer model.Dialer // optional + Endpoint string + Logger model.Logger + NewClient func(conn net.Conn) (stunClient, error) // optional + Resolver model.Resolver } -func stunDialer(network string, address string) (stunClient, error) { - return stun.Dial(network, address) +func stunNewClient(conn net.Conn) (stunClient, error) { + return stun.NewClient(conn) } func stunIPLookup(ctx context.Context, config stunConfig) (string, error) { config.Logger.Debugf("STUNIPLookup: start using %s", config.Endpoint) ip, err := func() (string, error) { - dial := config.Dial - if dial == nil { - dial = stunDialer + dialer := config.Dialer + if dialer == nil { + dialer = netxlite.NewDialerWithResolver(config.Logger, config.Resolver) } - clnt, err := dial("udp", config.Endpoint) + conn, err := dialer.DialContext(ctx, "udp", config.Endpoint) if err != nil { return model.DefaultProbeIP, err } + newClient := config.NewClient + if newClient == nil { + newClient = stunNewClient + } + clnt, err := newClient(conn) + if err != nil { + conn.Close() + return model.DefaultProbeIP, err + } defer clnt.Close() message := stun.MustBuild(stun.TransactionID, stun.BindingRequest) errch, ipch := make(chan error, 1), make(chan string, 1) @@ -78,10 +86,12 @@ func stunEkigaIPLookup( httpClient *http.Client, logger model.Logger, userAgent string, + resolver model.Resolver, ) (string, error) { return stunIPLookup(ctx, stunConfig{ Endpoint: "stun.ekiga.net:3478", Logger: logger, + Resolver: resolver, }) } @@ -90,9 +100,11 @@ func stunGoogleIPLookup( httpClient *http.Client, logger model.Logger, userAgent string, + resolver model.Resolver, ) (string, error) { return stunIPLookup(ctx, stunConfig{ Endpoint: "stun.l.google.com:19302", Logger: logger, + Resolver: resolver, }) } diff --git a/internal/engine/geolocate/stun_test.go b/internal/engine/geolocate/stun_test.go index d89a5bf..c161c06 100644 --- a/internal/engine/geolocate/stun_test.go +++ b/internal/engine/geolocate/stun_test.go @@ -10,6 +10,8 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/model/mocks" + "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/pion/stun" ) @@ -19,6 +21,7 @@ func TestSTUNIPLookupCanceledContext(t *testing.T) { ip, err := stunIPLookup(ctx, stunConfig{ Endpoint: "stun.ekiga.net:3478", Logger: log.Log, + Resolver: netxlite.NewStdlibResolver(model.DiscardLogger), }) if !errors.Is(err, context.Canceled) { t.Fatalf("not the error we expected: %+v", err) @@ -32,8 +35,10 @@ func TestSTUNIPLookupDialFailure(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() ip, err := stunIPLookup(ctx, stunConfig{ - Dial: func(network, address string) (stunClient, error) { - return nil, expected + Dialer: &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) { + return nil, expected + }, }, Endpoint: "stun.ekiga.net:3478", Logger: log.Log, @@ -70,11 +75,17 @@ func TestSTUNIPLookupStartReturnsError(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() ip, err := stunIPLookup(ctx, stunConfig{ - Dial: func(network, address string) (stunClient, error) { - return MockableSTUNClient{StartErr: expected}, nil + Dialer: &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) { + conn := &mocks.Conn{} + return conn, nil + }, }, Endpoint: "stun.ekiga.net:3478", Logger: log.Log, + NewClient: func(conn net.Conn) (stunClient, error) { + return MockableSTUNClient{StartErr: expected}, nil + }, }) if !errors.Is(err, expected) { t.Fatalf("not the error we expected: %+v", err) @@ -88,13 +99,19 @@ func TestSTUNIPLookupStunEventContainsError(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() ip, err := stunIPLookup(ctx, stunConfig{ - Dial: func(network, address string) (stunClient, error) { + Dialer: &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) { + conn := &mocks.Conn{} + return conn, nil + }, + }, + Endpoint: "stun.ekiga.net:3478", + Logger: log.Log, + NewClient: func(conn net.Conn) (stunClient, error) { return MockableSTUNClient{Event: stun.Event{ Error: expected, }}, nil }, - Endpoint: "stun.ekiga.net:3478", - Logger: log.Log, }) if !errors.Is(err, expected) { t.Fatalf("not the error we expected: %+v", err) @@ -107,13 +124,19 @@ func TestSTUNIPLookupStunEventContainsError(t *testing.T) { func TestSTUNIPLookupCannotDecodeMessage(t *testing.T) { ctx := context.Background() ip, err := stunIPLookup(ctx, stunConfig{ - Dial: func(network, address string) (stunClient, error) { + Dialer: &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) { + conn := &mocks.Conn{} + return conn, nil + }, + }, + Endpoint: "stun.ekiga.net:3478", + Logger: log.Log, + NewClient: func(conn net.Conn) (stunClient, error) { return MockableSTUNClient{Event: stun.Event{ Message: &stun.Message{}, }}, nil }, - Endpoint: "stun.ekiga.net:3478", - Logger: log.Log, }) if !errors.Is(err, stun.ErrAttributeNotFound) { t.Fatalf("not the error we expected: %+v", err) @@ -129,6 +152,7 @@ func TestIPLookupWorksUsingSTUNEkiga(t *testing.T) { http.DefaultClient, log.Log, model.HTTPHeaderUserAgent, + netxlite.NewStdlibResolver(model.DiscardLogger), ) if err != nil { t.Fatal(err) @@ -144,6 +168,7 @@ func TestIPLookupWorksUsingSTUNGoogle(t *testing.T) { http.DefaultClient, log.Log, model.HTTPHeaderUserAgent, + netxlite.NewStdlibResolver(model.DiscardLogger), ) if err != nil { t.Fatal(err) diff --git a/internal/engine/geolocate/ubuntu.go b/internal/engine/geolocate/ubuntu.go index c37e8c6..0f75afb 100644 --- a/internal/engine/geolocate/ubuntu.go +++ b/internal/engine/geolocate/ubuntu.go @@ -19,6 +19,7 @@ func ubuntuIPLookup( httpClient *http.Client, logger model.Logger, userAgent string, + resolver model.Resolver, ) (string, error) { data, err := (&httpx.APIClientTemplate{ BaseURL: "https://geoip.ubuntu.com/", diff --git a/internal/engine/geolocate/ubuntu_test.go b/internal/engine/geolocate/ubuntu_test.go index 22e1d28..d6edf7c 100644 --- a/internal/engine/geolocate/ubuntu_test.go +++ b/internal/engine/geolocate/ubuntu_test.go @@ -10,6 +10,7 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestUbuntuParseError(t *testing.T) { @@ -23,6 +24,7 @@ func TestUbuntuParseError(t *testing.T) { }}, log.Log, model.HTTPHeaderUserAgent, + netxlite.NewStdlibResolver(model.DiscardLogger), ) if err == nil || !strings.HasPrefix(err.Error(), "XML syntax error") { t.Fatalf("not the error we expected: %+v", err) @@ -38,6 +40,7 @@ func TestIPLookupWorksUsingUbuntu(t *testing.T) { http.DefaultClient, log.Log, model.HTTPHeaderUserAgent, + netxlite.NewStdlibResolver(model.DiscardLogger), ) if err != nil { t.Fatal(err)