From 5ddd432b8375a1a77d0f92283d224ac117c09fe7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 30 Sep 2021 18:00:38 +0200 Subject: [PATCH 1/8] fix(netxlite/quic): close udp conn after failed handshake (#533) Closes https://github.com/ooni/probe/issues/1794 Backport from master; see https://github.com/ooni/probe/issues/1839. --- internal/netxlite/quic.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/netxlite/quic.go b/internal/netxlite/quic.go index 25a3670a59..d0c1f66941 100644 --- a/internal/netxlite/quic.go +++ b/internal/netxlite/quic.go @@ -154,6 +154,7 @@ func (d *quicDialerQUICGo) DialContext(ctx context.Context, network string, sess, err := d.dialEarlyContext( ctx, pconn, udpAddr, address, tlsConfig, quicConfig) if err != nil { + pconn.Close() // we own it on failure return nil, err } return &quicSessionOwnsConn{EarlySession: sess, conn: pconn}, nil From 36e3b1030c38e8275623ae74275ee193c34ad00d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 5 Oct 2021 12:52:19 +0200 Subject: [PATCH 2/8] feat(webconnectivity): collect timing information (#537) Work related to https://github.com/ooni/probe/issues/1797 Backport from master; see https://github.com/ooni/probe/issues/1839 --- .../webconnectivity/webconnectivity.go | 22 ++++++++++++++++++- .../webconnectivity/webconnectivity_test.go | 2 +- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/internal/engine/experiment/webconnectivity/webconnectivity.go b/internal/engine/experiment/webconnectivity/webconnectivity.go index 4b68cfa4af..9aaa568958 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity.go @@ -19,7 +19,7 @@ import ( const ( testName = "web_connectivity" - testVersion = "0.4.0" + testVersion = "0.4.1" ) // Config contains the experiment config. @@ -63,6 +63,18 @@ type TestKeys struct { // Top-level analysis Summary + + // DNSRuntime is the time to run all DNS checks. + DNSRuntime time.Duration `json:"x_dns_runtime"` + + // THRuntime is the total time to invoke all test helpers. + THRuntime time.Duration `json:"x_th_runtime"` + + // TCPTLSRuntime is the total time to perform TCP/TLS "connects". + TCPTLSRuntime time.Duration `json:"x_tcptls_runtime"` + + // HTTPRuntime is the total time to perform the HTTP GET. + HTTPRuntime time.Duration `json:"x_http_runtime"` } // Measurer performs the measurement. @@ -151,14 +163,17 @@ func (m Measurer) Run( "backend": testhelper, } // 2. perform the DNS lookup step + dnsBegin := time.Now() dnsResult := DNSLookup(ctx, DNSLookupConfig{ Begin: measurement.MeasurementStartTimeSaved, Session: sess, URL: URL}) + tk.DNSRuntime = time.Since(dnsBegin) tk.Queries = append(tk.Queries, dnsResult.TestKeys.Queries...) tk.DNSExperimentFailure = dnsResult.Failure epnts := NewEndpoints(URL, dnsResult.Addresses()) sess.Logger().Infof("using control: %s", testhelper.Address) // 3. perform the control measurement + thBegin := time.Now() tk.Control, err = Control(ctx, sess, testhelper.Address, ControlRequest{ HTTPRequest: URL.String(), HTTPRequestHeaders: map[string][]string{ @@ -168,6 +183,7 @@ func (m Measurer) Run( }, TCPConnect: epnts.Endpoints(), }) + tk.THRuntime = time.Since(thBegin) tk.ControlFailure = archival.NewFailure(err) // 4. analyze DNS results if tk.ControlFailure == nil { @@ -181,12 +197,14 @@ func (m Measurer) Run( // returned by the control experiment. // // See https://github.com/ooni/probe/issues/1414 + tcptlsBegin := time.Now() connectsResult := Connects(ctx, ConnectsConfig{ Begin: measurement.MeasurementStartTimeSaved, Session: sess, TargetURL: URL, URLGetterURLs: epnts.URLs(), }) + tk.TCPTLSRuntime = time.Since(tcptlsBegin) sess.Logger().Infof( "TCP/TLS endpoints: %d/%d reachable", connectsResult.Successes, connectsResult.Total) for _, tcpkeys := range connectsResult.AllKeys { @@ -206,12 +224,14 @@ func (m Measurer) Run( tk.TCPConnectAttempts = connectsResult.Total tk.TCPConnectSuccesses = connectsResult.Successes // 6. perform HTTP/HTTPS measurement + httpBegin := time.Now() httpResult := HTTPGet(ctx, HTTPGetConfig{ Addresses: dnsResult.Addresses(), Begin: measurement.MeasurementStartTimeSaved, Session: sess, TargetURL: URL, }) + tk.HTTPRuntime = time.Since(httpBegin) tk.HTTPExperimentFailure = httpResult.Failure tk.Requests = append(tk.Requests, httpResult.TestKeys.Requests...) // 7. compare HTTP measurement to control diff --git a/internal/engine/experiment/webconnectivity/webconnectivity_test.go b/internal/engine/experiment/webconnectivity/webconnectivity_test.go index 7fed0a5f2f..cef81b3969 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity_test.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity_test.go @@ -21,7 +21,7 @@ func TestNewExperimentMeasurer(t *testing.T) { if measurer.ExperimentName() != "web_connectivity" { t.Fatal("unexpected name") } - if measurer.ExperimentVersion() != "0.4.0" { + if measurer.ExperimentVersion() != "0.4.1" { t.Fatal("unexpected version") } } From 1250c3b7975e78ae197ead612bedc7f90a1c5ffa Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 13 Oct 2021 10:31:46 +0200 Subject: [PATCH 3/8] feat: annotate measurements with their architecture (#540) Closes https://github.com/ooni/probe/issues/1772 Backport from master; see https://github.com/ooni/probe/issues/1839 --- internal/engine/experiment.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index acf7150e2e..c1224a8230 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -7,6 +7,7 @@ import ( "errors" "net/http" "os" + "runtime" "time" "github.com/ooni/probe-cli/v3/internal/bytecounter" @@ -168,6 +169,7 @@ func (e *Experiment) newMeasurement(input string) *model.Measurement { m.AddAnnotation("engine_name", "ooniprobe-engine") m.AddAnnotation("engine_version", version.Version) m.AddAnnotation("platform", e.session.Platform()) + m.AddAnnotation("architecture", runtime.GOARCH) return m } From e0642ea6f6026dd82abfbeae147cba9317c6b4a6 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 13 Oct 2021 13:27:09 +0200 Subject: [PATCH 4/8] fix(oohelperd): return HTTP headers as empty map on error (#541) Part of https://github.com/ooni/probe/issues/1707 Backport from master; see https://github.com/ooni/probe/issues/1839 --- internal/cmd/oohelperd/internal/webconnectivity/http.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/cmd/oohelperd/internal/webconnectivity/http.go b/internal/cmd/oohelperd/internal/webconnectivity/http.go index b66158361a..87180dbf6e 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/http.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/http.go @@ -34,6 +34,7 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) { BodyLength: -1, Failure: newfailure(err), StatusCode: -1, + Headers: map[string]string{}, } return } @@ -53,6 +54,7 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) { BodyLength: -1, Failure: newfailure(err), StatusCode: -1, + Headers: map[string]string{}, } return } From dda21fdac7d9e5a11ab0699b0a2cc485d37f8b8d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 13 Oct 2021 13:50:22 +0200 Subject: [PATCH 5/8] fix(webconnectivity): gather longer HTML titles (#542) Allows us to get http://www.isa.gov.il/Pages/default.aspx's one. Discovered when working on https://github.com/ooni/probe/issues/1707. Backport from master; see https://github.com/ooni/probe/issues/1839 --- internal/engine/experiment/webconnectivity/httpanalysis.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/engine/experiment/webconnectivity/httpanalysis.go b/internal/engine/experiment/webconnectivity/httpanalysis.go index f81baf158c..757acac424 100644 --- a/internal/engine/experiment/webconnectivity/httpanalysis.go +++ b/internal/engine/experiment/webconnectivity/httpanalysis.go @@ -186,7 +186,9 @@ func HTTPHeadersMatch(tk urlgetter.TestKeys, ctrl ControlResponse) *bool { // GetTitle returns the title or an empty string. func GetTitle(measurementBody string) string { - re := regexp.MustCompile(`(?i)([^<]{1,128})`) // like MK + // MK used {1,128} but we're making it larger here to get longer titles + // e.g. 's one + re := regexp.MustCompile(`(?i)([^<]{1,512})`) v := re.FindStringSubmatch(measurementBody) if len(v) < 2 { return "" From 2665e03fc6dd187384fa51e37e1867e59544c3a1 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 13 Oct 2021 16:37:02 +0200 Subject: [PATCH 6/8] fix(oohelperd): reduce errors to what the old TH would emit (#543) Reducing the errors is not done in a perfect way. We have documented the most striking differences inside https://github.com/ooni/probe/issues/1707#issuecomment-942283746 and some attempts to improve the situation further inside https://github.com/ooni/probe/issues/1707#issuecomment-942341255. A better strategy for the future would be to introduce more specific timeout errors, such as dns_timeout_error, etc. More testing may be needed to further validate and compare the old and the new TH, but this requires Jafar improvements to more precisely simulate more complex censorship. Backport from master; see https://github.com/ooni/probe/issues/1839 --- .../oohelperd/internal/webconnectivity/dns.go | 33 ++++++- .../internal/webconnectivity/dns_test.go | 87 +++++++++++++++++++ .../internal/webconnectivity/http.go | 54 +++++++++++- .../internal/webconnectivity/http_test.go | 80 ++++++++++++++++- .../internal/webconnectivity/tcpconnect.go | 29 ++++++- .../webconnectivity/tcpconnect_test.go | 40 +++++++++ 6 files changed, 315 insertions(+), 8 deletions(-) create mode 100644 internal/cmd/oohelperd/internal/webconnectivity/dns_test.go create mode 100644 internal/cmd/oohelperd/internal/webconnectivity/tcpconnect_test.go diff --git a/internal/cmd/oohelperd/internal/webconnectivity/dns.go b/internal/cmd/oohelperd/internal/webconnectivity/dns.go index 221774aad4..d74af22644 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/dns.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/dns.go @@ -7,6 +7,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity" "github.com/ooni/probe-cli/v3/internal/engine/netx" "github.com/ooni/probe-cli/v3/internal/engine/netx/archival" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) // newfailure is a convenience shortcut to save typing @@ -31,5 +32,35 @@ func DNSDo(ctx context.Context, config *DNSConfig) { if addrs == nil { addrs = []string{} // fix: the old test helper did that } - config.Out <- CtrlDNSResult{Failure: newfailure(err), Addrs: addrs} + failure := dnsMapFailure(newfailure(err)) + config.Out <- CtrlDNSResult{Failure: failure, Addrs: addrs} +} + +// dnsMapFailure attempts to map netxlite failures to the strings +// used by the original OONI test helper. +// +// See https://github.com/ooni/backend/blob/6ec4fda5b18/oonib/testhelpers/http_helpers.py#L430 +func dnsMapFailure(failure *string) *string { + switch failure { + case nil: + return nil + default: + switch *failure { + case netxlite.FailureDNSNXDOMAINError: + // We have a name for this string because dnsanalysis.go is + // already checking for this specific error string. + s := webconnectivity.DNSNameError + return &s + case netxlite.FailureDNSNoAnswer, + netxlite.FailureDNSNonRecoverableFailure, + netxlite.FailureDNSRefusedError, + netxlite.FailureDNSServerMisbehaving, + netxlite.FailureDNSTemporaryFailure: + s := "dns_server_failure" + return &s + default: + s := "unknown_error" + return &s + } + } } diff --git a/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go b/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go new file mode 100644 index 0000000000..486dde7d8d --- /dev/null +++ b/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go @@ -0,0 +1,87 @@ +package webconnectivity + +import ( + "context" + "sync" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/netxlite/mocks" +) + +func stringPointerForString(s string) *string { + return &s +} + +func Test_dnsMapFailure(t *testing.T) { + tests := []struct { + name string + failure *string + want *string + }{{ + name: "nil", + failure: nil, + want: nil, + }, { + name: "nxdomain", + failure: stringPointerForString(netxlite.FailureDNSNXDOMAINError), + want: stringPointerForString(webconnectivity.DNSNameError), + }, { + name: "no answer", + failure: stringPointerForString(netxlite.FailureDNSNoAnswer), + want: stringPointerForString("dns_server_failure"), + }, { + name: "non recoverable failure", + failure: stringPointerForString(netxlite.FailureDNSNonRecoverableFailure), + want: stringPointerForString("dns_server_failure"), + }, { + name: "refused", + failure: stringPointerForString(netxlite.FailureDNSRefusedError), + want: stringPointerForString("dns_server_failure"), + }, { + name: "server misbehaving", + failure: stringPointerForString(netxlite.FailureDNSServerMisbehaving), + want: stringPointerForString("dns_server_failure"), + }, { + name: "temporary failure", + failure: stringPointerForString(netxlite.FailureDNSTemporaryFailure), + want: stringPointerForString("dns_server_failure"), + }, { + name: "anything else", + failure: stringPointerForString(netxlite.FailureEOFError), + want: stringPointerForString("unknown_error"), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := dnsMapFailure(tt.failure) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Fatal(diff) + } + }) + } +} + +func TestDNSDo(t *testing.T) { + t.Run("returns non-nil addresses list on nxdomin", func(t *testing.T) { + ctx := context.Background() + config := &DNSConfig{ + Domain: "antani.ooni.org", + Out: make(chan webconnectivity.ControlDNSResult, 1), + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return nil, netxlite.ErrOODNSNoSuchHost + }, + }, + Wg: &sync.WaitGroup{}, + } + config.Wg.Add(1) + DNSDo(ctx, config) + config.Wg.Wait() + resp := <-config.Out + if resp.Addrs == nil || len(resp.Addrs) != 0 { + t.Fatal("returned nil Addrs or Addrs containing replies") + } + }) +} diff --git a/internal/cmd/oohelperd/internal/webconnectivity/http.go b/internal/cmd/oohelperd/internal/webconnectivity/http.go index 87180dbf6e..6487f00df6 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/http.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/http.go @@ -8,6 +8,7 @@ import ( "sync" "github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity" + "github.com/ooni/probe-cli/v3/internal/engine/netx/archival" "github.com/ooni/probe-cli/v3/internal/netxlite" ) @@ -32,7 +33,7 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) { if err != nil { config.Out <- CtrlHTTPResponse{ // fix: emit -1 like the old test helper does BodyLength: -1, - Failure: newfailure(err), + Failure: httpMapFailure(err), StatusCode: -1, Headers: map[string]string{}, } @@ -52,7 +53,7 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) { if err != nil { config.Out <- CtrlHTTPResponse{ // fix: emit -1 like old test helper does BodyLength: -1, - Failure: newfailure(err), + Failure: httpMapFailure(err), StatusCode: -1, Headers: map[string]string{}, } @@ -67,9 +68,56 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) { data, err := netxlite.ReadAllContext(ctx, reader) config.Out <- CtrlHTTPResponse{ BodyLength: int64(len(data)), - Failure: newfailure(err), + Failure: httpMapFailure(err), StatusCode: int64(resp.StatusCode), Headers: headers, Title: webconnectivity.GetTitle(string(data)), } } + +// httpMapFailure attempts to map netxlite failures to the strings +// used by the original OONI test helper. +// +// See https://github.com/ooni/backend/blob/6ec4fda5b18/oonib/testhelpers/http_helpers.py#L361 +func httpMapFailure(err error) *string { + failure := newfailure(err) + failedOperation := archival.NewFailedOperation(err) + switch failure { + case nil: + return nil + default: + switch *failure { + case netxlite.FailureDNSNXDOMAINError, + netxlite.FailureDNSNoAnswer, + netxlite.FailureDNSNonRecoverableFailure, + netxlite.FailureDNSRefusedError, + netxlite.FailureDNSServerMisbehaving, + netxlite.FailureDNSTemporaryFailure: + // Strangely the HTTP code uses the more broad + // dns_lookup_error and does not check for + // the NXDOMAIN-equivalent-error dns_name_error + s := "dns_lookup_error" + return &s + case netxlite.FailureGenericTimeoutError: + // The old TH would return "dns_lookup_error" when + // there is a timeout error during the DNS phase of HTTP. + switch failedOperation { + case nil: + // nothing + default: + switch *failedOperation { + case netxlite.ResolveOperation: + s := "dns_lookup_error" + return &s + } + } + return failure // already using the same name + case netxlite.FailureConnectionRefused: + s := "connection_refused_error" + return &s + default: + s := "unknown_error" + return &s + } + } +} diff --git a/internal/cmd/oohelperd/internal/webconnectivity/http_test.go b/internal/cmd/oohelperd/internal/webconnectivity/http_test.go index 05b00a879a..0347da56c7 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/http_test.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/http_test.go @@ -4,9 +4,11 @@ import ( "context" "errors" "net/http" - "strings" "sync" "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestHTTPDoWithInvalidURL(t *testing.T) { @@ -25,7 +27,7 @@ func TestHTTPDoWithInvalidURL(t *testing.T) { // wait for measurement steps to complete wg.Wait() resp := <-httpch - if resp.Failure == nil || !strings.HasSuffix(*resp.Failure, `invalid port "aaaa" after host`) { + if resp.Failure == nil || *resp.Failure != "unknown_error" { t.Fatal("not the failure we expected") } } @@ -51,7 +53,79 @@ func TestHTTPDoWithHTTPTransportFailure(t *testing.T) { // wait for measurement steps to complete wg.Wait() resp := <-httpch - if resp.Failure == nil || !strings.HasSuffix(*resp.Failure, "mocked error") { + if resp.Failure == nil || *resp.Failure != "unknown_error" { t.Fatal("not the error we expected") } } + +func newErrWrapper(failure, operation string) error { + return &netxlite.ErrWrapper{ + Failure: failure, + Operation: operation, + WrappedErr: nil, // should not matter + } +} + +func newErrWrapperTopLevel(failure string) error { + return newErrWrapper(failure, netxlite.TopLevelOperation) +} + +func Test_httpMapFailure(t *testing.T) { + tests := []struct { + name string + failure error + want *string + }{{ + name: "nil", + failure: nil, + want: nil, + }, { + name: "nxdomain", + failure: newErrWrapperTopLevel(netxlite.FailureDNSNXDOMAINError), + want: stringPointerForString("dns_lookup_error"), + }, { + name: "no answer", + failure: newErrWrapperTopLevel(netxlite.FailureDNSNoAnswer), + want: stringPointerForString("dns_lookup_error"), + }, { + name: "non recoverable failure", + failure: newErrWrapperTopLevel(netxlite.FailureDNSNonRecoverableFailure), + want: stringPointerForString("dns_lookup_error"), + }, { + name: "refused", + failure: newErrWrapperTopLevel(netxlite.FailureDNSRefusedError), + want: stringPointerForString("dns_lookup_error"), + }, { + name: "server misbehaving", + failure: newErrWrapperTopLevel(netxlite.FailureDNSServerMisbehaving), + want: stringPointerForString("dns_lookup_error"), + }, { + name: "temporary failure", + failure: newErrWrapperTopLevel(netxlite.FailureDNSTemporaryFailure), + want: stringPointerForString("dns_lookup_error"), + }, { + name: "timeout outside of dns lookup", + failure: newErrWrapperTopLevel(netxlite.FailureGenericTimeoutError), + want: stringPointerForString(netxlite.FailureGenericTimeoutError), + }, { + name: "timeout inside of dns lookup", + failure: newErrWrapper(netxlite.FailureGenericTimeoutError, netxlite.ResolveOperation), + want: stringPointerForString("dns_lookup_error"), + }, { + name: "connection refused", + failure: newErrWrapperTopLevel(netxlite.FailureConnectionRefused), + want: stringPointerForString("connection_refused_error"), + }, { + name: "anything else", + failure: newErrWrapperTopLevel(netxlite.FailureEOFError), + want: stringPointerForString("unknown_error"), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := httpMapFailure(tt.failure) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Fatal(diff) + } + }) + } +} diff --git a/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect.go b/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect.go index 64b34b2a79..6a58be7ea8 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect.go @@ -6,6 +6,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity" "github.com/ooni/probe-cli/v3/internal/engine/netx" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) // CtrlTCPResult is the result of the TCP check performed by the test helper. @@ -35,8 +36,34 @@ func TCPDo(ctx context.Context, config *TCPConfig) { config.Out <- TCPResultPair{ Endpoint: config.Endpoint, Result: CtrlTCPResult{ - Failure: newfailure(err), + Failure: tcpMapFailure(newfailure(err)), Status: err == nil, }, } } + +// tcpMapFailure attempts to map netxlite failures to the strings +// used by the original OONI test helper. +// +// See https://github.com/ooni/backend/blob/6ec4fda5b18/oonib/testhelpers/http_helpers.py#L392 +func tcpMapFailure(failure *string) *string { + switch failure { + case nil: + return nil + default: + switch *failure { + case netxlite.FailureGenericTimeoutError: + return failure // already using the same name + case netxlite.FailureConnectionRefused: + s := "connection_refused_error" + return &s + default: + // The definition of this error according to Twisted is + // "something went wrong when connecting". Because we are + // indeed basically just connecting here, it seems safe + // to map any other error to "connect_error" here. + s := "connect_error" + return &s + } + } +} diff --git a/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect_test.go b/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect_test.go new file mode 100644 index 0000000000..bb2fb30bbe --- /dev/null +++ b/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect_test.go @@ -0,0 +1,40 @@ +package webconnectivity + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/netxlite" +) + +func Test_tcpMapFailure(t *testing.T) { + tests := []struct { + name string + failure *string + want *string + }{{ + name: "nil", + failure: nil, + want: nil, + }, { + name: "timeout", + failure: stringPointerForString(netxlite.FailureGenericTimeoutError), + want: stringPointerForString(netxlite.FailureGenericTimeoutError), + }, { + name: "connection refused", + failure: stringPointerForString(netxlite.FailureConnectionRefused), + want: stringPointerForString("connection_refused_error"), + }, { + name: "anything else", + failure: stringPointerForString(netxlite.FailureEOFError), + want: stringPointerForString("connect_error"), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tcpMapFailure(tt.failure) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Fatal(diff) + } + }) + } +} From 7b571ee1e7f2e01f5605e73a88a2ef8baf92b7ff Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 15 Oct 2021 12:00:20 +0200 Subject: [PATCH 7/8] fix(wcth): emit empty Addrs when input URL contains addr (#545) Matches the behavior that the legacy TH implements in this situation and reduces slightly the differences. See https://github.com/ooni/probe/issues/1707#issuecomment-944143329 Backport from master; see https://github.com/ooni/probe/issues/1839 --- internal/cmd/oohelperd/internal/webconnectivity/measure.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/cmd/oohelperd/internal/webconnectivity/measure.go b/internal/cmd/oohelperd/internal/webconnectivity/measure.go index 41453f1b04..6be2cd81ba 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/measure.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/measure.go @@ -76,7 +76,12 @@ func Measure(ctx context.Context, config MeasureConfig, creq *CtrlRequest) (*Ctr select { case cresp.DNS = <-dnsch: default: - // we land here when there's no domain name + // we need to emit a non-nil Addrs to match exactly + // the behavior of the legacy TH + cresp.DNS = CtrlDNSResult{ + Failure: nil, + Addrs: []string{}, + } } cresp.HTTPRequest = <-httpch cresp.TCPConnect = make(map[string]CtrlTCPResult) From e62ec5d62514ab6f34610d1c2f4a71024d579dfb Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 15 Oct 2021 16:20:07 +0200 Subject: [PATCH 8/8] fix(wcth): match legacy TH w/ empty DNS reply (#546) See https://github.com/ooni/probe/issues/1707#issuecomment-944322725 Backport from master; see https://github.com/ooni/probe/issues/1839 --- internal/cmd/oohelperd/internal/webconnectivity/dns.go | 9 +++++++-- .../cmd/oohelperd/internal/webconnectivity/dns_test.go | 2 +- internal/netxlite/serialresolver.go | 3 +++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/internal/cmd/oohelperd/internal/webconnectivity/dns.go b/internal/cmd/oohelperd/internal/webconnectivity/dns.go index d74af22644..1125958c6c 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/dns.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/dns.go @@ -51,8 +51,13 @@ func dnsMapFailure(failure *string) *string { // already checking for this specific error string. s := webconnectivity.DNSNameError return &s - case netxlite.FailureDNSNoAnswer, - netxlite.FailureDNSNonRecoverableFailure, + case netxlite.FailureDNSNoAnswer: + // In this case the legacy TH would produce an empty + // reply that is not attached to any error. + // + // See https://github.com/ooni/probe/issues/1707#issuecomment-944322725 + return nil + case netxlite.FailureDNSNonRecoverableFailure, netxlite.FailureDNSRefusedError, netxlite.FailureDNSServerMisbehaving, netxlite.FailureDNSTemporaryFailure: diff --git a/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go b/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go index 486dde7d8d..1e51ea79f4 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go @@ -31,7 +31,7 @@ func Test_dnsMapFailure(t *testing.T) { }, { name: "no answer", failure: stringPointerForString(netxlite.FailureDNSNoAnswer), - want: stringPointerForString("dns_server_failure"), + want: nil, }, { name: "non recoverable failure", failure: stringPointerForString(netxlite.FailureDNSNonRecoverableFailure), diff --git a/internal/netxlite/serialresolver.go b/internal/netxlite/serialresolver.go index 90f7723886..1466120a72 100644 --- a/internal/netxlite/serialresolver.go +++ b/internal/netxlite/serialresolver.go @@ -64,6 +64,9 @@ func (r *SerialResolver) LookupHost(ctx context.Context, hostname string) ([]str addrsA, errA := r.lookupHostWithRetry(ctx, hostname, dns.TypeA) addrsAAAA, errAAAA := r.lookupHostWithRetry(ctx, hostname, dns.TypeAAAA) if errA != nil && errAAAA != nil { + // Note: we choose to return the errA because we assume that + // it's the more meaningful one: the errAAAA may just be telling + // us that there is no AAAA record for the website. return nil, errA } addrs = append(addrs, addrsA...)