From 4b8cae692bebb179de7d99dcfb6a1aeb5581c53f Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 13 Oct 2021 16:37:02 +0200 Subject: [PATCH] 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. --- .../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 221774a..d74af22 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 0000000..486dde7 --- /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 87180db..6487f00 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 05b00a8..0347da5 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 64b34b2..6a58be7 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 0000000..bb2fb30 --- /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) + } + }) + } +}