From ebc00a95fe3ad08cc0e0c6b679d036305c7baf38 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 22 May 2022 19:53:37 +0200 Subject: [PATCH] refactor: use ooni/oocrypto instead of ooni/go (#751) Rather than building for Android using ooni/go, we're now using ooni/oocryto as the TLS dependency. Such a repository only forks crypto/tls and some minor crypto packages and includes the same set of patches that we have been using in ooni/go. This new strategy should be better than the previous one in terms of building for Android, because we can use the vanilla go1.18.2 build. It also seems that it is easier to track and merge from upstream with ooni/oocrypto than it is with ooni/go. Should this assessment be wrong, we can revert back to the previous scenario where we used ooni/go. See https://github.com/ooni/probe/issues/2106 for extra context. --- go.mod | 5 +- go.sum | 6 ++- internal/engine/netx/httptransport/system.go | 9 ++-- .../engine/netx/tlsdialer/integration_test.go | 9 ++-- internal/netxlite/tls.go | 12 +++-- internal/netxlite/tls_test.go | 30 ++++++++++- internal/netxlite/utls.go | 52 +++++++++++++++---- internal/netxlite/utls_test.go | 50 ++++++++++++++++++ mk | 36 +++---------- 9 files changed, 151 insertions(+), 58 deletions(-) diff --git a/go.mod b/go.mod index a1644a1b2d..a3278cfc9a 100644 --- a/go.mod +++ b/go.mod @@ -20,13 +20,13 @@ require ( github.com/hexops/gotextdiff v1.0.3 github.com/iancoleman/strcase v0.2.0 github.com/lucas-clemente/quic-go v0.27.0 - github.com/marten-seemann/qtls-go1-17 v0.1.1 github.com/marten-seemann/qtls-go1-18 v0.1.1 github.com/mattn/go-colorable v0.1.12 github.com/miekg/dns v1.1.49 github.com/mitchellh/go-wordwrap v1.0.1 github.com/montanaflynn/stats v0.6.6 github.com/ooni/go-libtor v1.1.5 + github.com/ooni/oocrypto v0.0.0-20220522151937-5069b5e3bf80 github.com/ooni/oohttp v0.0.0-20220521113303-fb27ebcf5f1e github.com/ooni/probe-assets v0.9.0 github.com/ooni/psiphon/tunnel-core v0.0.0-20220519122549-9c044eb6bd83 @@ -41,7 +41,7 @@ require ( gitlab.com/yawning/utls.git v0.0.12-1 golang.org/x/crypto v0.0.0-20220518034528-6f7dac969898 golang.org/x/net v0.0.0-20220520000938-2e3eb7b945c2 - golang.org/x/sys v0.0.0-20220519141025-dcacdad47464 + golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a ) require ( @@ -72,6 +72,7 @@ require ( github.com/klauspost/reedsolomon v1.9.16 // indirect github.com/marten-seemann/qpack v0.2.1 // indirect github.com/marten-seemann/qtls-go1-16 v0.1.5 // indirect + github.com/marten-seemann/qtls-go1-17 v0.1.1 // indirect github.com/mattn/go-isatty v0.0.14 // indirect github.com/mattn/go-sqlite3 v1.14.13 // indirect github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d // indirect diff --git a/go.sum b/go.sum index f0d92449af..4298ad6ed0 100644 --- a/go.sum +++ b/go.sum @@ -614,6 +614,8 @@ github.com/onsi/gomega v1.17.0 h1:9Luw4uT5HTjHTN8+aNcSThgH1vdXnmdJ8xIfZ4wyTRE= github.com/onsi/gomega v1.17.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAlGdZY= github.com/ooni/go-libtor v1.1.5 h1:YbwXR9aLuL37EwL7rksPCQQhcHwoxU+M/v+jwZR+n5Y= github.com/ooni/go-libtor v1.1.5/go.mod h1:q1YyLwRD9GeMyeerVvwc0vJ2YgwDLTp2bdVcrh/JXyI= +github.com/ooni/oocrypto v0.0.0-20220522151937-5069b5e3bf80 h1:ubOaPXDY9TcGc8QfVTx+tAtcw44O2079H/juYnVuI6c= +github.com/ooni/oocrypto v0.0.0-20220522151937-5069b5e3bf80/go.mod h1:tP8tp455ERRdy38YWJQtQHb5LB6I5DvI4ffrnouypnM= github.com/ooni/oohttp v0.0.0-20220521113303-fb27ebcf5f1e h1:hM6+SmEh6aCzXZDIHTwA0UeyjXNy7EfK1NpE3zr3WBo= github.com/ooni/oohttp v0.0.0-20220521113303-fb27ebcf5f1e/go.mod h1:p2VVLbs+BXBIgTHITV9Vw8Rv6G1u66JUWP/8KCgDGNo= github.com/ooni/probe-assets v0.9.0 h1:RBqouztuKP0kWJzYwBjY0fPiwl5MQ8Aosy6ks8j1hR4= @@ -1182,8 +1184,8 @@ golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20211210111614-af8b64212486/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220412211240-33da011f77ad/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.0.0-20220519141025-dcacdad47464 h1:MpIuURY70f0iKp/oooEFtB2oENcHITo/z1b6u41pKCw= -golang.org/x/sys v0.0.0-20220519141025-dcacdad47464/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a h1:dGzPydgVsqGcTRVwiLJ1jVbufYwmzD3LfVPLKsKg+0k= +golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210503060354-a79de5458b56/go.mod h1:tfny5GFUkzUvx4ps4ajbZsCe5lw1metzhBm9T3x7oIY= diff --git a/internal/engine/netx/httptransport/system.go b/internal/engine/netx/httptransport/system.go index 690e825560..f921ec7126 100644 --- a/internal/engine/netx/httptransport/system.go +++ b/internal/engine/netx/httptransport/system.go @@ -1,8 +1,7 @@ package httptransport import ( - "net/http" - + oohttp "github.com/ooni/oohttp" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -13,7 +12,7 @@ import ( // // New code should use netxlite.NewHTTPTransport instead. func NewSystemTransport(config Config) model.HTTPTransport { - txp := http.DefaultTransport.(*http.Transport).Clone() + txp := oohttp.DefaultTransport.(*oohttp.Transport).Clone() txp.DialContext = config.Dialer.DialContext txp.DialTLSContext = config.TLSDialer.DialTLSContext // Better for Cloudflare DNS and also better because we have less @@ -24,12 +23,12 @@ func NewSystemTransport(config Config) model.HTTPTransport { // back the true headers, such as Content-Length. This change is // functional to OONI's goal of observing the network. txp.DisableCompression = true - return &SystemTransportWrapper{txp} + return &SystemTransportWrapper{&oohttp.StdlibTransport{Transport: txp}} } // SystemTransportWrapper adapts *http.Transport to have the .Network method type SystemTransportWrapper struct { - *http.Transport + *oohttp.StdlibTransport } func (txp *SystemTransportWrapper) Network() string { diff --git a/internal/engine/netx/tlsdialer/integration_test.go b/internal/engine/netx/tlsdialer/integration_test.go index 6ab0d48c2f..dc086c94e0 100644 --- a/internal/engine/netx/tlsdialer/integration_test.go +++ b/internal/engine/netx/tlsdialer/integration_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/apex/log" + oohttp "github.com/ooni/oohttp" "github.com/ooni/probe-cli/v3/internal/netxlite" ) @@ -19,9 +20,11 @@ func TestTLSDialerSuccess(t *testing.T) { DebugLogger: log.Log, }, } - txp := &http.Transport{ - DialTLSContext: dialer.DialTLSContext, - ForceAttemptHTTP2: true, + txp := &oohttp.StdlibTransport{ + Transport: &oohttp.Transport{ + DialTLSContext: dialer.DialTLSContext, + ForceAttemptHTTP2: true, + }, } client := &http.Client{Transport: txp} resp, err := client.Get("https://www.google.com") diff --git a/internal/netxlite/tls.go b/internal/netxlite/tls.go index 79845879d7..eceb8b15f6 100644 --- a/internal/netxlite/tls.go +++ b/internal/netxlite/tls.go @@ -13,6 +13,7 @@ import ( "net" "time" + ootls "github.com/ooni/oocrypto/tls" oohttp "github.com/ooni/oohttp" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" @@ -161,7 +162,7 @@ func newTLSHandshaker(th model.TLSHandshaker, logger model.DebugLogger) model.TL type tlsHandshakerConfigurable struct { // NewConn is the OPTIONAL factory for creating a new connection. If // this factory is not set, we'll use the stdlib. - NewConn func(conn net.Conn, config *tls.Config) TLSConn + NewConn func(conn net.Conn, config *tls.Config) (TLSConn, error) // Timeout is the OPTIONAL timeout imposed on the TLS handshake. If zero // or negative, we will use default timeout of 10 seconds. @@ -190,7 +191,10 @@ func (h *tlsHandshakerConfigurable) Handshake( config = config.Clone() config.RootCAs = defaultCertPool } - tlsconn := h.newConn(conn, config) + tlsconn, err := h.newConn(conn, config) + if err != nil { + return nil, tls.ConnectionState{}, err + } if err := tlsconn.HandshakeContext(ctx); err != nil { return nil, tls.ConnectionState{}, err } @@ -198,11 +202,11 @@ func (h *tlsHandshakerConfigurable) Handshake( } // newConn creates a new TLSConn. -func (h *tlsHandshakerConfigurable) newConn(conn net.Conn, config *tls.Config) TLSConn { +func (h *tlsHandshakerConfigurable) newConn(conn net.Conn, config *tls.Config) (TLSConn, error) { if h.NewConn != nil { return h.NewConn(conn, config) } - return tls.Client(conn, config) + return ootls.NewClientConnStdlib(conn, config) } // defaultTLSHandshaker is the default TLS handshaker. diff --git a/internal/netxlite/tls_test.go b/internal/netxlite/tls_test.go index 1fc8c47409..828864c860 100644 --- a/internal/netxlite/tls_test.go +++ b/internal/netxlite/tls_test.go @@ -202,13 +202,13 @@ func TestTLSHandshakerConfigurable(t *testing.T) { expected := errors.New("mocked error") var gotTLSConfig *tls.Config handshaker := &tlsHandshakerConfigurable{ - NewConn: func(conn net.Conn, config *tls.Config) TLSConn { + NewConn: func(conn net.Conn, config *tls.Config) (TLSConn, error) { gotTLSConfig = config return &mocks.TLSConn{ MockHandshakeContext: func(ctx context.Context) error { return expected }, - } + }, nil }, } ctx := context.Background() @@ -235,6 +235,32 @@ func TestTLSHandshakerConfigurable(t *testing.T) { t.Fatal("gotTLSConfig.RootCAs has not been correctly set") } }) + + t.Run("we cannot create a new conn", func(t *testing.T) { + expected := errors.New("mocked error") + handshaker := &tlsHandshakerConfigurable{ + NewConn: func(conn net.Conn, config *tls.Config) (TLSConn, error) { + return nil, expected + }, + } + ctx := context.Background() + config := &tls.Config{} + conn := &mocks.Conn{ + MockSetDeadline: func(t time.Time) error { + return nil + }, + } + tlsConn, connState, err := handshaker.Handshake(ctx, conn, config) + if !errors.Is(err, expected) { + t.Fatal("not the error we expected", err) + } + if !reflect.ValueOf(connState).IsZero() { + t.Fatal("expected zero connState here") + } + if tlsConn != nil { + t.Fatal("expected nil tlsConn here") + } + }) }) } diff --git a/internal/netxlite/utls.go b/internal/netxlite/utls.go index a4ddc5751e..c789c3e62b 100644 --- a/internal/netxlite/utls.go +++ b/internal/netxlite/utls.go @@ -8,7 +8,9 @@ import ( "context" "crypto/tls" "errors" + "fmt" "net" + "reflect" "github.com/ooni/probe-cli/v3/internal/model" utls "gitlab.com/yawning/utls.git" @@ -42,18 +44,48 @@ type utlsConn struct { var _ TLSConn = &utlsConn{} // newConnUTLS returns a NewConn function for creating utlsConn instances. -func newConnUTLS(clientHello *utls.ClientHelloID) func(conn net.Conn, config *tls.Config) TLSConn { - return func(conn net.Conn, config *tls.Config) TLSConn { - uConfig := &utls.Config{ - RootCAs: config.RootCAs, - NextProtos: config.NextProtos, - ServerName: config.ServerName, - InsecureSkipVerify: config.InsecureSkipVerify, - DynamicRecordSizingDisabled: config.DynamicRecordSizingDisabled, +func newConnUTLS(clientHello *utls.ClientHelloID) func(conn net.Conn, config *tls.Config) (TLSConn, error) { + return func(conn net.Conn, config *tls.Config) (TLSConn, error) { + return newConnUTLSWithHelloID(conn, config, clientHello) + } +} + +// errUTLSIncompatibleStdlibConfig indicates that the stdlib config you passed to +// newConnUTLSWithHelloID contains some fields we don't support. +var errUTLSIncompatibleStdlibConfig = errors.New("utls: incompatible stdlib config") + +// newConnUTLSWithHelloID creates a new connection with the given client hello ID. +func newConnUTLSWithHelloID(conn net.Conn, config *tls.Config, cid *utls.ClientHelloID) (TLSConn, error) { + supportedFields := map[string]bool{ + "DynamicRecordSizingDisabled": true, + "InsecureSkipVerify": true, + "NextProtos": true, + "RootCAs": true, + "ServerName": true, + } + value := reflect.ValueOf(config).Elem() + kind := value.Type() + for idx := 0; idx < value.NumField(); idx++ { + field := value.Field(idx) + if field.IsZero() { + continue + } + fieldKind := kind.Field(idx) + if supportedFields[fieldKind.Name] { + continue } - tlsConn := utls.UClient(conn, uConfig, *clientHello) - return &utlsConn{UConn: tlsConn} + err := fmt.Errorf("%w: field %s is nonzero", errUTLSIncompatibleStdlibConfig, fieldKind.Name) + return nil, err + } + uConfig := &utls.Config{ + DynamicRecordSizingDisabled: config.DynamicRecordSizingDisabled, + InsecureSkipVerify: config.InsecureSkipVerify, + RootCAs: config.RootCAs, + NextProtos: config.NextProtos, + ServerName: config.ServerName, } + tlsConn := utls.UClient(conn, uConfig, *cid) + return &utlsConn{UConn: tlsConn}, nil } // ErrUTLSHandshakePanic indicates that there was panic handshaking diff --git a/internal/netxlite/utls_test.go b/internal/netxlite/utls_test.go index ce167e7a1d..e6d35fc381 100644 --- a/internal/netxlite/utls_test.go +++ b/internal/netxlite/utls_test.go @@ -2,7 +2,9 @@ package netxlite import ( "context" + "crypto/tls" "errors" + "net" "sync" "testing" "time" @@ -92,3 +94,51 @@ func TestUTLSConn(t *testing.T) { }) }) } + +func Test_newConnUTLSWithHelloID(t *testing.T) { + tests := []struct { + name string + config *tls.Config + cid *utls.ClientHelloID + wantNilConn bool + wantErr error + }{{ + name: "with only supported fields", + config: &tls.Config{ + DynamicRecordSizingDisabled: true, + InsecureSkipVerify: true, + NextProtos: []string{"h3"}, + RootCAs: NewDefaultCertPool(), + ServerName: "ooni.org", + }, + cid: &utls.HelloFirefox_55, + wantNilConn: false, + wantErr: nil, + }, { + name: "with unsupported fields", + config: &tls.Config{ + Time: func() time.Time { + return time.Now() + }, + }, + cid: &utls.HelloChrome_58, + wantNilConn: true, + wantErr: errUTLSIncompatibleStdlibConfig, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + conn, err := net.Dial("udp", "8.8.8.8:443") // we just need a conn + if err != nil { + t.Fatal(err) + } + defer conn.Close() + got, err := newConnUTLSWithHelloID(conn, tt.config, tt.cid) + if !errors.Is(err, tt.wantErr) { + t.Fatal("unexpected err", err) + } + if got != nil && tt.wantNilConn { + t.Fatal("expected nil conn here") + } + }) + } +} diff --git a/mk b/mk index 7af7820f4c..c3cd5cfdac 100755 --- a/mk +++ b/mk @@ -70,10 +70,6 @@ GOLANG_VERSION_NUMBER = 1.18.2 #help: * MINGW_W64_VERSION : the expected mingw-w64 version. MINGW_W64_VERSION = 10.3.1 -#help: -#help: * OONIGO_BRANCH : the github.com/ooni/go branch to use. -OONIGO_BRANCH = oonigo1.18.2 - #help: #help: * OONI_PSIPHON_TAGS : build tags for `go build -tags ...` that cause #help: the build to embed a psiphon configuration file @@ -199,8 +195,7 @@ GOLANG_DOCKER_IMAGE = golang:$(GOLANG_VERSION_NUMBER)-alpine #help: #help: The `./mk ./CLI/ooniprobe-darwin` command builds the ooniprobe official -#help: command line client for darwin/amd64 and darwin/arm64. This process -#help: entails building ooniprobe and then GPG-signing the binaries. +#help: command line client for darwin/amd64 and darwin/arm64. #help: #help: You can also build the following subtargets: .PHONY: ./CLI/ooniprobe-darwin @@ -264,8 +259,7 @@ GOLANG_DOCKER_IMAGE = golang:$(GOLANG_VERSION_NUMBER)-alpine #help: #help: The `./mk ./CLI/ooniprobe-windows` command builds the ooniprobe official -#help: command line client for windows/386 and windows/amd64. This entails -#help: building and PGP signing the executables. +#help: command line client for windows/386 and windows/amd64. #help: #help: You can also build the following subtargets: .PHONY: ./CLI/ooniprobe-windows @@ -298,19 +292,15 @@ GOLANG_DOCKER_IMAGE = golang:$(GOLANG_VERSION_NUMBER)-alpine ./MOBILE/android/oonimkall.pom: cat ./MOBILE/android/template.pom | sed -e "s/@VERSION@/$(OONIMKALL_V)/g" > ./MOBILE/android/oonimkall.pom -#help: -#help: * `./mk ./MOBILE/android/oonimkall.aar`: the AAR -.PHONY: ./MOBILE/android/oonimkall.aar -./MOBILE/android/oonimkall.aar: android/sdk ooni/go maybe/copypsiphon - PATH=$(OONIGODIR)/bin:$$PATH $(MAKE) -f mk __android_build_with_ooni_go - # GOMOBILE is the full path location to the gomobile binary. We want to # execute this command every time, because its output may depend on context, # for this reason WE ARE NOT using `:=`. GOMOBILE = $(shell go env GOPATH)/bin/gomobile -# Here we use ooni/go to work around https://github.com/ooni/probe/issues/1444 -__android_build_with_ooni_go: search/for/go +#help: +#help: * `./mk ./MOBILE/android/oonimkall.aar`: the AAR +.PHONY: ./MOBILE/android/oonimkall.aar +./MOBILE/android/oonimkall.aar: android/sdk maybe/copypsiphon go get -u golang.org/x/mobile/cmd/gomobile $(GOMOBILE) init PATH=$(shell go env GOPATH)/bin:$$PATH ANDROID_HOME=$(OONI_ANDROID_HOME) ANDROID_NDK_HOME=$(OONI_ANDROID_HOME)/ndk/$(ANDROID_NDK_VERSION) $(GOMOBILE) bind -x -target android -o ./MOBILE/android/oonimkall.aar -tags="$(OONI_PSIPHON_TAGS)" -ldflags '-s -w' $(GOLANG_EXTRA_FLAGS) ./pkg/oonimkall @@ -492,20 +482,6 @@ $(OONIPRIVATE): search/for/git $(GIT_CLONE_DIR) rm -rf $(OONIPRIVATE) git clone $(OONIPRIVATE_REPO) $(OONIPRIVATE) -#help: -#help: The `./mk ooni/go` command builds the latest version of ooni/go. -.PHONY: ooni/go -ooni/go: search/for/bash search/for/git search/for/go $(OONIGODIR) - rm -rf $(OONIGODIR) - git clone -b $(OONIGO_BRANCH) --single-branch --depth 8 $(OONIGO_REPO) $(OONIGODIR) - cd $(OONIGODIR)/src && ./make.bash - -# OONIGODIR is the directory in which we clone ooni/go -OONIGODIR = $(GIT_CLONE_DIR)/github.com/ooni/go - -# OONIGO_REPO is the repository for ooni/go -OONIGO_REPO = https://github.com/ooni/go - #help: #help: The `./mk android/sdk` command ensures we are using the #help: correct version of the Android sdk.