From da8ce4c3a2813a484606cf9fc74d940785d3d200 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Apr 2024 11:20:20 +0200 Subject: [PATCH] chore(enginelocate): improve ubuntu and cloudflare tests (#1564) Closes https://github.com/ooni/probe/issues/2715 --- internal/enginelocate/cloudflare.go | 16 +- internal/enginelocate/cloudflare_test.go | 238 ++++++++++++++++-- internal/enginelocate/fake_test.go | 28 --- internal/enginelocate/invalid_test.go | 3 +- internal/enginelocate/iplookup.go | 2 +- internal/enginelocate/stun.go | 5 +- internal/enginelocate/ubuntu.go | 18 +- internal/enginelocate/ubuntu_test.go | 292 +++++++++++++++++++---- 8 files changed, 506 insertions(+), 96 deletions(-) delete mode 100644 internal/enginelocate/fake_test.go diff --git a/internal/enginelocate/cloudflare.go b/internal/enginelocate/cloudflare.go index 17a69112a1..ff97b90abe 100644 --- a/internal/enginelocate/cloudflare.go +++ b/internal/enginelocate/cloudflare.go @@ -2,7 +2,7 @@ package enginelocate import ( "context" - "net/http" + "net" "regexp" "strings" @@ -12,22 +12,34 @@ import ( func cloudflareIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, ) (string, error) { + // get the raw response body data, err := (&httpx.APIClientTemplate{ BaseURL: "https://www.cloudflare.com", HTTPClient: httpClient, Logger: logger, UserAgent: model.HTTPHeaderUserAgent, }).WithBodyLogging().Build().FetchResource(ctx, "/cdn-cgi/trace") + + // handle the error case if err != nil { return model.DefaultProbeIP, err } + + // find the IP addr r := regexp.MustCompile("(?:ip)=(.*)") ip := strings.Trim(string(r.Find(data)), "ip=") logger.Debugf("cloudflare: body: %s", ip) + + // make sure the IP addr is valid + if net.ParseIP(ip) == nil { + return model.DefaultProbeIP, ErrInvalidIPAddress + } + + // done! return ip, nil } diff --git a/internal/enginelocate/cloudflare_test.go b/internal/enginelocate/cloudflare_test.go index ff49ce9957..30e5e0bcef 100644 --- a/internal/enginelocate/cloudflare_test.go +++ b/internal/enginelocate/cloudflare_test.go @@ -2,32 +2,234 @@ package enginelocate import ( "context" + "errors" "net" "net/http" + "net/url" "testing" "github.com/apex/log" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) +// cloudflareRealisticresponse is a realistic response returned by cloudflare +// with the IP address modified to belong to a public institution. +var cloudflareRealisticResponse = []byte(` +fl=270f47 +h=www.cloudflare.com +ip=130.192.91.211 +ts=1713946961.154 +visit_scheme=https +uag=Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:125.0) Gecko/20100101 Firefox/125.0 +colo=MXP +sliver=none +http=http/3 +loc=IT +tls=TLSv1.3 +sni=plaintext +warp=off +gateway=off +rbi=off +kex=X25519 +`) + func TestIPLookupWorksUsingcloudlflare(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - - netx := &netxlite.Netx{} - ip, err := cloudflareIPLookup( - context.Background(), - http.DefaultClient, - log.Log, - model.HTTPHeaderUserAgent, - netx.NewStdlibResolver(model.DiscardLogger), - ) - if err != nil { - t.Fatal(err) - } - if net.ParseIP(ip) == nil { - t.Fatalf("not an IP address: '%s'", ip) - } + + // We want to make sure the real server gives us an IP address. + t.Run("is working as intended when using the real server", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + http.DefaultClient, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect this call to succeed + if err != nil { + t.Fatal(err) + } + + // we expect to get back a valid IPv4/IPv6 address + if net.ParseIP(ip) == nil { + t.Fatalf("not an IP address: '%s'", ip) + } + }) + + // But we also want to make sure everything is working as intended when using + // a local HTTP server, as well as that we can handle errors, so that we can run + // tests in short mode. This is done with the tests below. + + t.Run("is working as intended when using a fake server", func(t *testing.T) { + // create a fake server returning an hardcoded IP address. + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(cloudflareRealisticResponse) + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect this call to succeed + if err != nil { + t.Fatal(err) + } + + // we expect to get back a valid IPv4/IPv6 address + if net.ParseIP(ip) == nil { + t.Fatalf("not an IP address: '%s'", ip) + } + + // we expect to see exactly the IP address that we want to see + if ip != "130.192.91.211" { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles network errors", func(t *testing.T) { + // create a fake server resetting the connection for the client. + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see ECONNRESET here + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles parsing errors", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`ipx=130.192.91.211`)) // note: different key name + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an error indicating there's no IP address in the response + if !errors.Is(err, ErrInvalidIPAddress) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles the case where the IP address is invalid", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`ip=foobarbaz`)) // note: invalid IP address + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an error indicating there's no IP address in the response + if !errors.Is(err, ErrInvalidIPAddress) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) } diff --git a/internal/enginelocate/fake_test.go b/internal/enginelocate/fake_test.go deleted file mode 100644 index 72933e9ab9..0000000000 --- a/internal/enginelocate/fake_test.go +++ /dev/null @@ -1,28 +0,0 @@ -package enginelocate - -import ( - "net/http" - "time" - - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -type FakeTransport struct { - Err error - Resp *http.Response -} - -func (txp FakeTransport) RoundTrip(req *http.Request) (*http.Response, error) { - time.Sleep(10 * time.Microsecond) - if req.Body != nil { - netxlite.ReadAllContext(req.Context(), req.Body) - req.Body.Close() - } - if txp.Err != nil { - return nil, txp.Err - } - txp.Resp.Request = req // non thread safe but it doesn't matter - return txp.Resp, nil -} - -func (txp FakeTransport) CloseIdleConnections() {} diff --git a/internal/enginelocate/invalid_test.go b/internal/enginelocate/invalid_test.go index a6ac6b7f97..d9c76c1fda 100644 --- a/internal/enginelocate/invalid_test.go +++ b/internal/enginelocate/invalid_test.go @@ -2,14 +2,13 @@ package enginelocate import ( "context" - "net/http" "github.com/ooni/probe-cli/v3/internal/model" ) func invalidIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, diff --git a/internal/enginelocate/iplookup.go b/internal/enginelocate/iplookup.go index b618a38d68..ca05b75908 100644 --- a/internal/enginelocate/iplookup.go +++ b/internal/enginelocate/iplookup.go @@ -25,7 +25,7 @@ var ( ) type lookupFunc func( - ctx context.Context, client *http.Client, + ctx context.Context, client model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, ) (string, error) diff --git a/internal/enginelocate/stun.go b/internal/enginelocate/stun.go index b001401a96..d659c9de84 100644 --- a/internal/enginelocate/stun.go +++ b/internal/enginelocate/stun.go @@ -3,7 +3,6 @@ package enginelocate import ( "context" "net" - "net/http" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" @@ -86,7 +85,7 @@ func stunIPLookup(ctx context.Context, config stunConfig) (string, error) { func stunEkigaIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, @@ -100,7 +99,7 @@ func stunEkigaIPLookup( func stunGoogleIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, diff --git a/internal/enginelocate/ubuntu.go b/internal/enginelocate/ubuntu.go index de59ec9d89..77d9b5cbcb 100644 --- a/internal/enginelocate/ubuntu.go +++ b/internal/enginelocate/ubuntu.go @@ -3,7 +3,7 @@ package enginelocate import ( "context" "encoding/xml" - "net/http" + "net" "github.com/ooni/probe-cli/v3/internal/httpx" "github.com/ooni/probe-cli/v3/internal/model" @@ -16,25 +16,39 @@ type ubuntuResponse struct { func ubuntuIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, ) (string, error) { + // read the HTTP response body data, err := (&httpx.APIClientTemplate{ BaseURL: "https://geoip.ubuntu.com/", HTTPClient: httpClient, Logger: logger, UserAgent: userAgent, }).WithBodyLogging().Build().FetchResource(ctx, "/lookup") + + // handle the error case if err != nil { return model.DefaultProbeIP, err } + + // parse the XML logger.Debugf("ubuntu: body: %s", string(data)) var v ubuntuResponse err = xml.Unmarshal(data, &v) + + // handle the error case if err != nil { return model.DefaultProbeIP, err } + + // make sure the IP addr is valid + if net.ParseIP(v.IP) == nil { + return model.DefaultProbeIP, ErrInvalidIPAddress + } + + // handle the success case return v.IP, nil } diff --git a/internal/enginelocate/ubuntu_test.go b/internal/enginelocate/ubuntu_test.go index e4e313484b..4f305c7556 100644 --- a/internal/enginelocate/ubuntu_test.go +++ b/internal/enginelocate/ubuntu_test.go @@ -2,56 +2,268 @@ package enginelocate import ( "context" - "io" + "errors" "net" "net/http" + "net/url" "strings" "testing" "github.com/apex/log" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) -func TestUbuntuParseError(t *testing.T) { - netx := &netxlite.Netx{} - ip, err := ubuntuIPLookup( - context.Background(), - &http.Client{Transport: FakeTransport{ - Resp: &http.Response{ - StatusCode: 200, - Body: io.NopCloser(strings.NewReader("<")), - }, - }}, - log.Log, - model.HTTPHeaderUserAgent, - netx.NewStdlibResolver(model.DiscardLogger), - ) - if err == nil || !strings.HasPrefix(err.Error(), "XML syntax error") { - t.Fatalf("not the error we expected: %+v", err) - } - if ip != model.DefaultProbeIP { - t.Fatalf("not the expected IP address: %s", ip) - } -} +// ubuntuRealisticresponse is a realistic response returned by cloudflare +// with the IP address modified to belong to a public institution. +var ubuntuRealisticresponse = []byte(` + +130.192.91.211 +OK +IT +ITA +Italy +09 +Lombardia +Sesto San Giovanni +20099 +45.5349 +9.2295 +0 +Europe/Rome + +`) func TestIPLookupWorksUsingUbuntu(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - - netx := &netxlite.Netx{} - ip, err := ubuntuIPLookup( - context.Background(), - http.DefaultClient, - log.Log, - model.HTTPHeaderUserAgent, - netx.NewStdlibResolver(model.DiscardLogger), - ) - if err != nil { - t.Fatal(err) - } - if net.ParseIP(ip) == nil { - t.Fatalf("not an IP address: '%s'", ip) - } + + // We want to make sure the real server gives us an IP address. + t.Run("is working as intended when using the real server", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + http.DefaultClient, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + if err != nil { + t.Fatal(err) + } + if net.ParseIP(ip) == nil { + t.Fatalf("not an IP address: '%s'", ip) + } + }) + + // But we also want to make sure everything is working as intended when using + // a local HTTP server, as well as that we can handle errors, so that we can run + // tests in short mode. This is done with the tests below. + + t.Run("is working as intended when using a fake server", func(t *testing.T) { + // create a fake server returning an hardcoded IP address. + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(ubuntuRealisticresponse) + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect this call to succeed + if err != nil { + t.Fatal(err) + } + + // we expect to get back a valid IPv4/IPv6 address + if net.ParseIP(ip) == nil { + t.Fatalf("not an IP address: '%s'", ip) + } + + // we expect to see exactly the IP address that we want to see + if ip != "130.192.91.211" { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles network errors", func(t *testing.T) { + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see ECONNRESET here + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles parsing errors", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`<`)) // note: invalid XML + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an XML parsing error here + if err == nil || !strings.HasPrefix(err.Error(), "XML syntax error") { + t.Fatalf("not the error we expected: %+v", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles missing IP address in a valid XML document", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(``)) // note: missing IP address + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an error indicating there's no IP address in the response + if !errors.Is(err, ErrInvalidIPAddress) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles the case where the IP address is invalid", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`foobarbaz`)) // note: not an IP address + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an error indicating there's no IP address in the response + if !errors.Is(err, ErrInvalidIPAddress) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) }