From 6a935d5407fa05e0e2d73cc01492ebb888d10a29 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sat, 6 Nov 2021 17:49:58 +0100 Subject: [PATCH] fix(netxlite): ensure HTTP errors are always wrapped (#584) 1. introduce implementations of HTTPTransport and HTTPClient that apply an error wrapping policy using the constructor for a generic top-level error wrapper 2. make sure we use the implementations in point 1 when we are constructing HTTPTransport and HTTPClient 3. make sure we apply error wrapping using the constructor for a generic top-level error wrapper when reading bodies 4. acknowledge that error wrapping would be broken if we do not return the same classification _and_ operation when we wrap an already wrapped error, so fix the to code to do that 5. acknowledge that the classifiers already deal with preserving the error string and explain why this is a quirk and why we cannot remove it right now and what needs to happen to safely remove this quirk from the codebase Closes https://github.com/ooni/probe/issues/1860 --- internal/measurex/http.go | 16 +---- internal/netxlite/classify.go | 13 ++++ internal/netxlite/dnsoverhttps.go | 6 -- internal/netxlite/errwrapper.go | 21 +++++- internal/netxlite/errwrapper_test.go | 17 +++++ internal/netxlite/http.go | 41 +++++++++++- internal/netxlite/http_test.go | 99 +++++++++++++++++++++++++++- internal/netxlite/iox.go | 8 +-- internal/netxlite/iox_test.go | 24 +++++++ 9 files changed, 217 insertions(+), 28 deletions(-) diff --git a/internal/measurex/http.go b/internal/measurex/http.go index 872449e..9d0e4d9 100644 --- a/internal/measurex/http.go +++ b/internal/measurex/http.go @@ -244,7 +244,7 @@ var ErrHTTPTooManyRedirects = errors.New("stopped after 10 redirects") func newHTTPClient(db WritableDB, cookiejar http.CookieJar, txp HTTPTransport, defaultErr error) HTTPClient { - return &httpClientErrWrapper{&http.Client{ + return netxlite.WrapHTTPClient(&http.Client{ Transport: txp, Jar: cookiejar, CheckRedirect: func(req *http.Request, via []*http.Request) error { @@ -260,19 +260,7 @@ func newHTTPClient(db WritableDB, cookiejar http.CookieJar, }) return err }, - }} -} - -type httpClientErrWrapper struct { - HTTPClient -} - -func (c *httpClientErrWrapper) Do(req *http.Request) (*http.Response, error) { - resp, err := c.HTTPClient.Do(req) - if err != nil { - err = netxlite.NewTopLevelGenericErrWrapper(err) - } - return resp, err + }) } // NewCookieJar is a convenience factory for creating an http.CookieJar diff --git a/internal/netxlite/classify.go b/internal/netxlite/classify.go index 56a4f4d..77dae38 100644 --- a/internal/netxlite/classify.go +++ b/internal/netxlite/classify.go @@ -38,6 +38,8 @@ func ClassifyGenericError(err error) string { // The list returned here matches the values used by MK unless // explicitly noted otherwise with a comment. + // QUIRK: we cannot remove this check as long as this function + // is exported and used independently from NewErrWrapper. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it @@ -143,6 +145,9 @@ const ( // If this classifier fails, it calls ClassifyGenericError // and returns to the caller its return value. func ClassifyQUICHandshakeError(err error) string { + + // QUIRK: we cannot remove this check as long as this function + // is exported and used independently from NewErrWrapper. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it @@ -257,10 +262,14 @@ var ( // If this classifier fails, it calls ClassifyGenericError and // returns to the caller its return value. func ClassifyResolverError(err error) string { + + // QUIRK: we cannot remove this check as long as this function + // is exported and used independently from NewErrWrapper. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it } + if errors.Is(err, ErrDNSBogon) { return FailureDNSBogonError // not in MK } @@ -281,10 +290,14 @@ func ClassifyResolverError(err error) string { // If this classifier fails, it calls ClassifyGenericError and // returns to the caller its return value. func ClassifyTLSHandshakeError(err error) string { + + // QUIRK: we cannot remove this check as long as this function + // is exported and used independently from NewErrWrapper. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it } + var x509HostnameError x509.HostnameError if errors.As(err, &x509HostnameError) { // Test case: https://wrong.host.badssl.com/ diff --git a/internal/netxlite/dnsoverhttps.go b/internal/netxlite/dnsoverhttps.go index 70bacfc..e9caa27 100644 --- a/internal/netxlite/dnsoverhttps.go +++ b/internal/netxlite/dnsoverhttps.go @@ -10,12 +10,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/httpheader" ) -// HTTPClient is an http.Client-like interface. -type HTTPClient interface { - Do(req *http.Request) (*http.Response, error) - CloseIdleConnections() -} - // DNSOverHTTPS is a DNS-over-HTTPS DNSTransport. type DNSOverHTTPS struct { // Client is the MANDATORY http client to use. diff --git a/internal/netxlite/errwrapper.go b/internal/netxlite/errwrapper.go index 9d604f8..c618399 100644 --- a/internal/netxlite/errwrapper.go +++ b/internal/netxlite/errwrapper.go @@ -1,6 +1,9 @@ package netxlite -import "encoding/json" +import ( + "encoding/json" + "errors" +) // ErrWrapper is our error wrapper for Go errors. The key objective of // this structure is to properly set Failure, which is also returned by @@ -78,7 +81,19 @@ type Classifier func(err error) string // // This function panics if classifier is nil, or operation // is the empty string or error is nil. +// +// If the err argument has already been classified, the returned +// error wrapper will use the same classification string and +// failed operation of the original wrapped error. func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper { + var wrapper *ErrWrapper + if errors.As(err, &wrapper) { + return &ErrWrapper{ + Failure: wrapper.Failure, + Operation: wrapper.Operation, + WrappedErr: err, + } + } if c == nil { panic("nil classifier") } @@ -97,6 +112,10 @@ func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper { // NewTopLevelGenericErrWrapper wraps an error occurring at top // level using ClassifyGenericError as classifier. +// +// If the err argument has already been classified, the returned +// error wrapper will use the same classification string and +// failed operation of the original error. func NewTopLevelGenericErrWrapper(err error) *ErrWrapper { return NewErrWrapper(ClassifyGenericError, TopLevelOperation, err) } diff --git a/internal/netxlite/errwrapper_test.go b/internal/netxlite/errwrapper_test.go index 1fe8aa4..97cc645 100644 --- a/internal/netxlite/errwrapper_test.go +++ b/internal/netxlite/errwrapper_test.go @@ -3,6 +3,7 @@ package netxlite import ( "encoding/json" "errors" + "fmt" "io" "testing" @@ -101,6 +102,22 @@ func TestNewErrWrapper(t *testing.T) { t.Fatal("unexpected WrappedErr") } }) + + t.Run("when the underlying error is already a wrapped error", func(t *testing.T) { + ew := NewErrWrapper(classifySyscallError, ReadOperation, ECONNRESET) + var err1 error = ew + err2 := fmt.Errorf("cannot read: %w", err1) + ew2 := NewErrWrapper(ClassifyGenericError, TopLevelOperation, err2) + if ew2.Failure != ew.Failure { + t.Fatal("not the same failure") + } + if ew2.Operation != ew.Operation { + t.Fatal("not the same operation") + } + if ew2.WrappedErr != err2 { + t.Fatal("invalid underlying error") + } + }) } func TestNewTopLevelGenericErrWrapper(t *testing.T) { diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go index 8ccbf06..fbd59d7 100644 --- a/internal/netxlite/http.go +++ b/internal/netxlite/http.go @@ -19,6 +19,19 @@ type HTTPTransport interface { CloseIdleConnections() } +// httpTransportErrWrapper is an HTTPTransport with error wrapping. +type httpTransportErrWrapper struct { + HTTPTransport +} + +func (txp *httpTransportErrWrapper) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := txp.HTTPTransport.RoundTrip(req) + if err != nil { + return nil, NewTopLevelGenericErrWrapper(err) + } + return resp, nil +} + // httpTransportLogger is an HTTPTransport with logging. type httpTransportLogger struct { // HTTPTransport is the underlying HTTP transport. @@ -141,12 +154,13 @@ func NewOOHTTPBaseTransport(dialer Dialer, tlsDialer TLSDialer) HTTPTransport { } } -// WrapHTTPTransport creates an HTTPTransport using the given logger. +// WrapHTTPTransport creates an HTTPTransport using the given logger +// and guarantees that returned errors are wrapped. // // This is a low level factory. Consider not using it directly. func WrapHTTPTransport(logger Logger, txp HTTPTransport) HTTPTransport { return &httpTransportLogger{ - HTTPTransport: txp, + HTTPTransport: &httpTransportErrWrapper{txp}, Logger: logger, } } @@ -250,3 +264,26 @@ func NewHTTPTransportStdlib(logger Logger) HTTPTransport { tlsDialer := NewTLSDialer(dialer, NewTLSHandshakerStdlib(logger)) return NewHTTPTransport(logger, dialer, tlsDialer) } + +// HTTPClient is an http.Client-like interface. +type HTTPClient interface { + Do(req *http.Request) (*http.Response, error) + CloseIdleConnections() +} + +// WrapHTTPClient wraps an HTTP client to add error wrapping capabilities. +func WrapHTTPClient(clnt HTTPClient) HTTPClient { + return &httpClientErrWrapper{clnt} +} + +type httpClientErrWrapper struct { + HTTPClient +} + +func (c *httpClientErrWrapper) Do(req *http.Request) (*http.Response, error) { + resp, err := c.HTTPClient.Do(req) + if err != nil { + return nil, NewTopLevelGenericErrWrapper(err) + } + return resp, nil +} diff --git a/internal/netxlite/http_test.go b/internal/netxlite/http_test.go index 2844d8a..8009f60 100644 --- a/internal/netxlite/http_test.go +++ b/internal/netxlite/http_test.go @@ -16,6 +16,49 @@ import ( "github.com/ooni/probe-cli/v3/internal/netxlite/mocks" ) +func TestHTTPTransportErrWrapper(t *testing.T) { + t.Run("RoundTrip", func(t *testing.T) { + t.Run("with failure", func(t *testing.T) { + txp := &httpTransportErrWrapper{ + HTTPTransport: &mocks.HTTPTransport{ + MockRoundTrip: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + } + resp, err := txp.RoundTrip(&http.Request{}) + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not an ErrWrapper") + } + if errWrapper.Failure != FailureEOFError { + t.Fatal("unexpected failure", errWrapper.Failure) + } + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("with success", func(t *testing.T) { + expect := &http.Response{} + txp := &httpTransportErrWrapper{ + HTTPTransport: &mocks.HTTPTransport{ + MockRoundTrip: func(req *http.Request) (*http.Response, error) { + return expect, nil + }, + }, + } + resp, err := txp.RoundTrip(&http.Request{}) + if err != nil { + t.Fatal(err) + } + if resp != expect { + t.Fatal("not the expected response") + } + }) + }) +} + func TestHTTPTransportLogger(t *testing.T) { t.Run("RoundTrip", func(t *testing.T) { t.Run("with failure", func(t *testing.T) { @@ -198,7 +241,8 @@ func TestNewHTTPTransport(t *testing.T) { if logger.Logger != log.Log { t.Fatal("invalid logger") } - connectionsCloser := logger.HTTPTransport.(*httpTransportConnectionsCloser) + errWrapper := logger.HTTPTransport.(*httpTransportErrWrapper) + connectionsCloser := errWrapper.HTTPTransport.(*httpTransportConnectionsCloser) withReadTimeout := connectionsCloser.Dialer.(*httpDialerWithReadTimeout) if withReadTimeout.Dialer != d { t.Fatal("invalid dialer") @@ -412,3 +456,56 @@ func TestNewHTTPTransportStdlib(t *testing.T) { } txp.CloseIdleConnections() } + +func TestHTTPClientErrWrapper(t *testing.T) { + t.Run("Do", func(t *testing.T) { + t.Run("with failure", func(t *testing.T) { + clnt := &httpClientErrWrapper{ + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + } + resp, err := clnt.Do(&http.Request{}) + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not an ErrWrapper") + } + if errWrapper.Failure != FailureEOFError { + t.Fatal("unexpected failure", errWrapper.Failure) + } + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("with success", func(t *testing.T) { + expect := &http.Response{} + clnt := &httpClientErrWrapper{ + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return expect, nil + }, + }, + } + resp, err := clnt.Do(&http.Request{}) + if err != nil { + t.Fatal(err) + } + if resp != expect { + t.Fatal("not the expected response") + } + }) + }) +} + +func TestWrapHTTPClient(t *testing.T) { + origClient := &http.Client{} + wrapped := WrapHTTPClient(origClient) + errWrapper := wrapped.(*httpClientErrWrapper) + innerClient := errWrapper.HTTPClient.(*http.Client) + if innerClient != origClient { + t.Fatal("not the inner client we expected") + } +} diff --git a/internal/netxlite/iox.go b/internal/netxlite/iox.go index 710e1cb..53fa22b 100644 --- a/internal/netxlite/iox.go +++ b/internal/netxlite/iox.go @@ -27,9 +27,9 @@ func ReadAllContext(ctx context.Context, r io.Reader) ([]byte, error) { case data := <-datach: return data, nil case <-ctx.Done(): - return nil, ctx.Err() + return nil, NewTopLevelGenericErrWrapper(ctx.Err()) case err := <-errch: - return nil, err + return nil, NewTopLevelGenericErrWrapper(err) } } @@ -51,8 +51,8 @@ func CopyContext(ctx context.Context, dst io.Writer, src io.Reader) (int64, erro case count := <-countch: return count, nil case <-ctx.Done(): - return 0, ctx.Err() + return 0, NewTopLevelGenericErrWrapper(ctx.Err()) case err := <-errch: - return 0, err + return 0, NewTopLevelGenericErrWrapper(err) } } diff --git a/internal/netxlite/iox_test.go b/internal/netxlite/iox_test.go index d36d173..908fddf 100644 --- a/internal/netxlite/iox_test.go +++ b/internal/netxlite/iox_test.go @@ -36,6 +36,10 @@ func TestReadAllContext(t *testing.T) { if !errors.Is(err, expected) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if len(out) != 0 { t.Fatal("not the expected number of bytes") } @@ -65,6 +69,10 @@ func TestReadAllContext(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if len(out) != 0 { t.Fatal("not the expected number of bytes") } @@ -90,6 +98,10 @@ func TestReadAllContext(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if len(out) != 0 { t.Fatal("not the expected number of bytes") } @@ -123,6 +135,10 @@ func TestCopyContext(t *testing.T) { if !errors.Is(err, expected) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if out != 0 { t.Fatal("not the expected number of bytes") } @@ -152,6 +168,10 @@ func TestCopyContext(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if out != 0 { t.Fatal("not the expected number of bytes") } @@ -177,6 +197,10 @@ func TestCopyContext(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if out != 0 { t.Fatal("not the expected number of bytes") }