From c94721d9e53e1f1266661f0bb2e0b8132a5e7fb7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 25 Mar 2021 15:18:29 +0100 Subject: [PATCH] fix(sessionresolver): proxy check conditional on existing proxy (#264) There was a face-palming error in the implementation causing the proxy check to be implemented also without a proxy. This meant that we were ALWAYS skipping http3 and system resolvers. The bug has been introduced in 3.8.0. So, the currently released version of the probe, sadly, has this beheavior :-(. Reference issue https://github.com/ooni/probe/issues/1426. --- .../internal/sessionresolver/resolvermaker.go | 2 +- .../sessionresolver/sessionresolver.go | 69 ++++++++++++++++--- .../sessionresolver/sessionresolver_test.go | 2 +- 3 files changed, 60 insertions(+), 13 deletions(-) diff --git a/internal/engine/internal/sessionresolver/resolvermaker.go b/internal/engine/internal/sessionresolver/resolvermaker.go index 1719c6e..0874d7b 100644 --- a/internal/engine/internal/sessionresolver/resolvermaker.go +++ b/internal/engine/internal/sessionresolver/resolvermaker.go @@ -97,7 +97,7 @@ func (r *Resolver) newresolver(URL string) (childResolver, error) { func (r *Resolver) getresolver(URL string) (childResolver, error) { defer r.mu.Unlock() r.mu.Lock() - if re, found := r.res[URL]; found == true { + if re, found := r.res[URL]; found { return re, nil // already created } re, err := r.newresolver(URL) diff --git a/internal/engine/internal/sessionresolver/sessionresolver.go b/internal/engine/internal/sessionresolver/sessionresolver.go index 9b9af01..8e7be14 100644 --- a/internal/engine/internal/sessionresolver/sessionresolver.go +++ b/internal/engine/internal/sessionresolver/sessionresolver.go @@ -17,6 +17,10 @@ // but it will of course be the most popular resolver if anything else // is failing us. (We will still occasionally probe for other working // resolvers and increase their score on success.) +// +// We also support a socks5 proxy. When such a proxy is configured, +// the code WILL skip http3 resolvers AS WELL AS the system +// resolver, in an attempt to avoid leaking your queries. package sessionresolver import ( @@ -34,18 +38,60 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/runtimex" ) -// Resolver is the session resolver. You should create an instance of -// this structure and use it in session.go. +// Resolver is the session resolver. Resolver will try to use +// a bunch of DoT/DoH resolvers before falling back to the +// system resolver. The relative priorities of the resolver +// are stored onto the KVStore such that we can remember them +// and therefore we can generally give preference to underlying +// DoT/DoH resolvers that work better. +// +// You MUST NOT modify public fields of this structure once it +// has been created, because that MAY lead to data races. +// +// You should create an instance of this structure and use +// it in internal/engine/session.go. type Resolver struct { - ByteCounter *bytecounter.Counter // optional - KVStore KVStore // optional - Logger Logger // optional - ProxyURL *url.URL // optional - codec codec + // ByteCounter is the optional byte counter. It will count + // the bytes used by any child resolver except for the + // system resolver, whose bytes ARE NOT counted. If this + // field is not set, then we won't count the bytes. + ByteCounter *bytecounter.Counter + + // KVStore is the optional key-value store where you + // want us to write statistics about which resolver is + // working better in your network. If this field is + // not set, then we'll use a in-memory store. + KVStore KVStore + + // Logger is the optional logger you want us to use + // to emit log messages. + Logger Logger + + // ProxyURL is the optional URL of the socks5 proxy + // we should be using. If not set, then we WON'T use + // any proxy. If set, then we WON'T use any http3 + // based resolvers and we WON'T use the system resolver. + ProxyURL *url.URL + + // codec is the optional codec to use. If not set, we + // will construct a default codec. + codec codec + + // dnsClientMaker is the optional dnsclientmaker to + // use. If not set, we will use the default. dnsClientMaker dnsclientmaker - mu sync.Mutex - once sync.Once - res map[string]childResolver + + // mu provides synchronisation of internal fields. + mu sync.Mutex + + // once ensures that CloseIdleConnection is + // run just once. + once sync.Once + + // res maps a URL to a child resolver. We will + // construct child resolvers just once and we + // will track them into this field. + res map[string]childResolver } // CloseIdleConnections closes the idle connections, if any. This @@ -73,7 +119,8 @@ func (r *Resolver) LookupHost(ctx context.Context, hostname string) ([]string, e defer r.writestate(state) me := multierror.New(ErrLookupHost) for _, e := range state { - if r.shouldSkipWithProxy(e) { + if r.ProxyURL != nil && r.shouldSkipWithProxy(e) { + r.logger().Infof("sessionresolver: skipping with proxy: %+v", e) continue // we cannot proxy this URL so ignore it } addrs, err := r.lookupHost(ctx, e, hostname) diff --git a/internal/engine/internal/sessionresolver/sessionresolver_test.go b/internal/engine/internal/sessionresolver/sessionresolver_test.go index b2b28d3..8b46c1f 100644 --- a/internal/engine/internal/sessionresolver/sessionresolver_test.go +++ b/internal/engine/internal/sessionresolver/sessionresolver_test.go @@ -294,7 +294,7 @@ func TestResolverWorksWithProxy(t *testing.T) { <-done // check results if !errors.Is(err, ErrLookupHost) { - t.Fatal("not the error we expected") + t.Fatal("not the error we expected", err) } if addrs != nil { t.Fatal("expected nil addrs")