From 1b864ab2505bdd91aec2901e59a824c0481b2751 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 12 Oct 2022 17:57:47 +0200 Subject: [PATCH] fix(geolocate): always use netxlite functionality This change ensures that, in turn, we're able to remote all the traffic generated by geolocate, rather than missing some bits of it that were still using the standard library. Extracted from https://github.com/ooni/probe-cli/pull/969. Closes https://github.com/ooni/probe/issues/1383. Part of https://github.com/ooni/probe/issues/2340. --- 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 b98c1e6cc2..3de61a538b 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 49c50b7c15..4f0b44da41 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 175c7db033..0b88bbaa04 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 aade7c71de..bb62404328 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 b5b40b069b..3d9711f323 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 fb25eb1a97..ef6835b225 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 03331610ee..3d34c3fb74 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 cb3746d80c..d6133a1096 100644 --- a/internal/engine/geolocate/stun.go +++ b/internal/engine/geolocate/stun.go @@ -2,41 +2,49 @@ 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) + } + 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 := dial("udp", config.Endpoint) + clnt, err := newClient(conn) if err != nil { + conn.Close() return model.DefaultProbeIP, err } defer clnt.Close() @@ -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 d89a5bfbaf..c161c069f0 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 c37e8c6079..0f75afb4e0 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 22e1d287f6..d6edf7c015 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)