From 3cf6f976ae7dab9f99aced7bc15f8dcb1da35e7a Mon Sep 17 00:00:00 2001 From: ooninoob Date: Sat, 19 Nov 2022 17:18:46 +0100 Subject: [PATCH 01/14] Basic SMTP+StartTLS / SMTPS test --- internal/engine/experiment/smtp/smtp.go | 314 ++++++++++++++++++++++++ internal/registry/smtp.go | 22 ++ 2 files changed, 336 insertions(+) create mode 100644 internal/engine/experiment/smtp/smtp.go create mode 100644 internal/registry/smtp.go diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go new file mode 100644 index 0000000..453807f --- /dev/null +++ b/internal/engine/experiment/smtp/smtp.go @@ -0,0 +1,314 @@ +package smtp + +import ( + "context" + "crypto/tls" + "fmt" + "github.com/pkg/errors" + "net" + "net/smtp" + "net/url" + "strconv" + "time" + + "github.com/ooni/probe-cli/v3/internal/engine/experiment/urlgetter" + "github.com/ooni/probe-cli/v3/internal/measurexlite" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/tracex" +) + +var ( + // errNoInputProvided indicates you didn't provide any input + errNoInputProvided = errors.New("not input provided") + + // errInputIsNotAnURL indicates that input is not an URL + errInputIsNotAnURL = errors.New("input is not an URL") + + // errInvalidScheme indicates that the scheme is invalid + errInvalidScheme = errors.New("scheme must be smtp(s)") + + // errInvalidPort indicates that the port provided could not be parsed as an int + errInvalidPort = errors.New("Port number is not a valid integer") +) + +const ( + testName = "smtp" + testVersion = "0.0.1" +) + +// Config contains the experiment config. +type Config struct { + host string + port string + forced_tls bool + noop_count uint8 +} + +func config(input model.MeasurementTarget) (*Config, error) { + if input == "" { + // TODO: static input data (eg. gmail/riseup..) + return nil, errNoInputProvided + } + + parsed, err := url.Parse(string(input)) + if err != nil { + return nil, fmt.Errorf("%w: %s", errInputIsNotAnURL, err.Error()) + } + if parsed.Scheme != "smtp" && parsed.Scheme != "smtps" { + return nil, errInvalidScheme + } + + port := "" + + if parsed.Port() == "" { + // Default ports for StartTLS and forced TLS respectively + if parsed.Scheme == "smtp" { + port = "587" + } else { + port = "465" + } + } else { + // Check that requested port is a valid integer + _, err := strconv.Atoi(parsed.Port()) + if err != nil { + return nil, errInvalidPort + } else { + port = parsed.Port() + } + } + + valid_config := Config{ + host: parsed.Hostname(), + forced_tls: parsed.Scheme == "smtps", + port: port, + noop_count: 10, + } + + return &valid_config, nil +} + +// TestKeys contains the experiment results +type TestKeys struct { + Queries []*model.ArchivalDNSLookupResult `json:"queries"` + TCPConnect []*model.ArchivalTCPConnectResult `json:"tcp_connect"` + TLSHandshakes []*model.ArchivalTLSOrQUICHandshakeResult `json:"tls_handshakes"` + SMTPErrors []*string `json:"smtp"` + NoOpCounter uint8 `json:"successful_noops"` + // Used for global failure (DNS resolution) + Failure string `json:"failure"` +} + +type Measurer struct { + // Config contains the experiment settings. If empty we + // will be using default settings. + Config Config + + // Getter is an optional getter to be used for testing. + Getter urlgetter.MultiGetter +} + +// ExperimentName implements ExperimentMeasurer.ExperimentName +func (m Measurer) ExperimentName() string { + return testName +} + +// ExperimentVersion implements ExperimentMeasurer.ExperimentVersion +func (m Measurer) ExperimentVersion() string { + return testVersion +} + +type SMTPRunner struct { + trace *measurexlite.Trace + logger model.Logger + ctx context.Context + tk *TestKeys + tlsconfig *tls.Config +} + +func (r SMTPRunner) resolve(host string) ([]string, bool) { + r.logger.Infof("Resolving DNS for %s", host) + resolver := r.trace.NewStdlibResolver(r.logger) + addrs, err := resolver.LookupHost(r.ctx, host) + r.tk.Queries = append(r.tk.Queries, r.trace.DNSLookupsFromRoundTrip()...) + if err != nil { + r.tk.Failure = *tracex.NewFailure(err) + return []string{}, false + } + r.logger.Infof("Finished DNS for %s: %v", host, addrs) + + return addrs, true +} + +func (r SMTPRunner) conn(addr string, port string) (net.Conn, bool) { + dialer := r.trace.NewDialerWithoutResolver(r.logger) + conn, err := dialer.DialContext(r.ctx, "tcp", net.JoinHostPort(addr, port)) + r.tk.TCPConnect = append(r.tk.TCPConnect, r.trace.TCPConnects()...) + if err != nil { + r.tk.Failure = *tracex.NewFailure(err) + return nil, false + } + return conn, true +} + +func (r SMTPRunner) handshake(conn net.Conn, host string, port string) (net.Conn, bool) { + r.logger.Infof("Starting TLS handshake with %s:%s", host, port) + thx := r.trace.NewTLSHandshakerStdlib(r.logger) + tconn, _, err := thx.Handshake(r.ctx, conn, r.tlsconfig) + r.tk.TLSHandshakes = append(r.tk.TLSHandshakes, r.trace.FirstTLSHandshakeOrNil()) + if err != nil { + return nil, false + } + r.logger.Infof("Handshake succeeded") + return tconn, true +} + +func (r SMTPRunner) starttls(conn net.Conn, host string, port string, message string) (net.Conn, bool) { + r.logger.Infof("Asking for StartTLS upgrade") + conn.Write([]byte(message)) + tconn, success := r.handshake(conn, host, port) + return tconn, success +} + +func (r SMTPRunner) smtp(conn net.Conn, ehlo string, noop uint8) bool { + client, err := smtp.NewClient(conn, ehlo) + if err != nil { + r.tk.SMTPErrors = append(r.tk.SMTPErrors, []*string{tracex.NewFailure(err)}...) + return false + } + err = client.Hello(ehlo) + if err != nil { + r.tk.SMTPErrors = append(r.tk.SMTPErrors, []*string{tracex.NewFailure(err)}...) + return false + } + + if noop > 0 { + r.logger.Infof("Trying to generate more no-op traffic") + // TODO: noop counter per IP address + r.tk.NoOpCounter = 0 + for r.tk.NoOpCounter < noop { + r.tk.NoOpCounter += 1 + r.logger.Infof("NoOp Iteration %d", r.tk.NoOpCounter) + err = client.Noop() + if err != nil { + r.tk.SMTPErrors = append(r.tk.SMTPErrors, []*string{tracex.NewFailure(err)}...) + break + } + } + + if r.tk.NoOpCounter == noop { + r.logger.Infof("Successfully generated no-op traffic") + return true + } else { + r.logger.Infof("Failed no-op traffic at iteration %d", r.tk.NoOpCounter) + return false + } + } + + return true +} + +// Run implements ExperimentMeasurer.Run +func (m Measurer) Run( + ctx context.Context, sess model.ExperimentSession, + measurement *model.Measurement, callbacks model.ExperimentCallbacks, +) error { + + log := sess.Logger() + trace := measurexlite.NewTrace(0, measurement.MeasurementStartTimeSaved) + + config, err := config(measurement.Input) + if err != nil { + // Invalid input data, we don't even generate report + return err + } + + tk := new(TestKeys) + measurement.TestKeys = tk + + ctx, cancel := context.WithTimeout(ctx, 60*time.Second) + defer cancel() + + tlsconfig := tls.Config{ + InsecureSkipVerify: false, + ServerName: config.host, + } + + runner := SMTPRunner{ + trace: trace, + logger: log, + ctx: ctx, + tk: tk, + tlsconfig: &tlsconfig, + } + + // First resolve DNS + addrs, success := runner.resolve(config.host) + if !success { + return nil + } + + for _, addr := range addrs { + conn, success := runner.conn(addr, config.port) + if !success { + return nil + } + defer conn.Close() + + if config.forced_tls { + // Direct TLS connection + tconn, success := runner.handshake(conn, config.host, config.port) + if !success { + continue + } + defer tconn.Close() + + // Try EHLO + NoOps + if !runner.smtp(tconn, "localhost", 10) { + continue + } + } else { + // StartTLS... first try plaintext EHLO + if !runner.smtp(conn, "localhost", 0) { + continue + } + + // Upgrade via StartTLS and try EHLO + NoOps + tconn, success := runner.starttls(conn, config.host, config.port, "STARTTLS\n") + if !success { + continue + } + defer tconn.Close() + + if !runner.smtp(tconn, "localhost", 10) { + continue + } + } + } + + return nil +} + +// NewExperimentMeasurer creates a new ExperimentMeasurer. +func NewExperimentMeasurer(config Config) model.ExperimentMeasurer { + return Measurer{Config: config} +} + +// SummaryKeys contains summary keys for this experiment. +// +// Note that this structure is part of the ABI contract with ooniprobe +// therefore we should be careful when changing it. +type SummaryKeys struct { + //DNSBlocking bool `json:"facebook_dns_blocking"` + //TCPBlocking bool `json:"facebook_tcp_blocking"` + IsAnomaly bool `json:"-"` +} + +// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys. +func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) { + sk := SummaryKeys{IsAnomaly: false} + _, ok := measurement.TestKeys.(*TestKeys) + if !ok { + return sk, errors.New("invalid test keys type") + } + return sk, nil +} diff --git a/internal/registry/smtp.go b/internal/registry/smtp.go new file mode 100644 index 0000000..cb67e45 --- /dev/null +++ b/internal/registry/smtp.go @@ -0,0 +1,22 @@ +package registry + +// +// Registers the `dnsping' experiment. +// + +import ( + "github.com/ooni/probe-cli/v3/internal/engine/experiment/smtp" + "github.com/ooni/probe-cli/v3/internal/model" +) + +func init() { + AllExperiments["smtp"] = &Factory{ + build: func(config interface{}) model.ExperimentMeasurer { + return smtp.NewExperimentMeasurer( + *config.(*smtp.Config), + ) + }, + config: &smtp.Config{}, + inputPolicy: model.InputOrStaticDefault, + } +} From c80dca4a59306b80768b80de509c81c2fed55f1e Mon Sep 17 00:00:00 2001 From: ooninoob Date: Sun, 20 Nov 2022 16:22:00 +0100 Subject: [PATCH 02/14] Failure only for top level failure --- internal/engine/experiment/smtp/smtp.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go index 453807f..671dd16 100644 --- a/internal/engine/experiment/smtp/smtp.go +++ b/internal/engine/experiment/smtp/smtp.go @@ -144,7 +144,6 @@ func (r SMTPRunner) conn(addr string, port string) (net.Conn, bool) { conn, err := dialer.DialContext(r.ctx, "tcp", net.JoinHostPort(addr, port)) r.tk.TCPConnect = append(r.tk.TCPConnect, r.trace.TCPConnects()...) if err != nil { - r.tk.Failure = *tracex.NewFailure(err) return nil, false } return conn, true From c35e01f64067b98df30ca055c44fd4fbf6279830 Mon Sep 17 00:00:00 2001 From: ooninoob Date: Sun, 20 Nov 2022 18:22:42 +0100 Subject: [PATCH 03/14] more abstractions for errors --- internal/engine/experiment/smtp/smtp.go | 56 ++++++++++++++++++------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go index 671dd16..a0d38ac 100644 --- a/internal/engine/experiment/smtp/smtp.go +++ b/internal/engine/experiment/smtp/smtp.go @@ -92,7 +92,7 @@ type TestKeys struct { Queries []*model.ArchivalDNSLookupResult `json:"queries"` TCPConnect []*model.ArchivalTCPConnectResult `json:"tcp_connect"` TLSHandshakes []*model.ArchivalTLSOrQUICHandshakeResult `json:"tls_handshakes"` - SMTPErrors []*string `json:"smtp"` + SMTPErrors map[string][]*string `json:"smtp"` NoOpCounter uint8 `json:"successful_noops"` // Used for global failure (DNS resolution) Failure string `json:"failure"` @@ -117,12 +117,28 @@ func (m Measurer) ExperimentVersion() string { return testVersion } +// Manages sequential SMTP sessions to the same hostname (over different IPs) +// don't use in parallel! type SMTPRunner struct { trace *measurexlite.Trace logger model.Logger ctx context.Context tk *TestKeys tlsconfig *tls.Config + host string + port string + // addr is changed everytime SMTPRunner.conn(addr) is called + addr string +} + +func (r SMTPRunner) smtp_error(err error) { + key := net.JoinHostPort(r.addr, r.port) + errors, exists := r.tk.SMTPErrors[key] + if exists { + r.tk.SMTPErrors[key] = append(errors, tracex.NewFailure(err)) + } else { + r.tk.SMTPErrors[key] = []*string{tracex.NewFailure(err)} + } } func (r SMTPRunner) resolve(host string) ([]string, bool) { @@ -139,44 +155,49 @@ func (r SMTPRunner) resolve(host string) ([]string, bool) { return addrs, true } -func (r SMTPRunner) conn(addr string, port string) (net.Conn, bool) { +func (r SMTPRunner) conn(addr string) (net.Conn, bool) { + r.addr = addr dialer := r.trace.NewDialerWithoutResolver(r.logger) - conn, err := dialer.DialContext(r.ctx, "tcp", net.JoinHostPort(addr, port)) + conn, err := dialer.DialContext(r.ctx, "tcp", net.JoinHostPort(r.addr, r.port)) r.tk.TCPConnect = append(r.tk.TCPConnect, r.trace.TCPConnects()...) if err != nil { + r.smtp_error(err) return nil, false } return conn, true } -func (r SMTPRunner) handshake(conn net.Conn, host string, port string) (net.Conn, bool) { - r.logger.Infof("Starting TLS handshake with %s:%s", host, port) +func (r SMTPRunner) handshake(conn net.Conn) (net.Conn, bool) { + r.logger.Infof("Starting TLS handshake with %s:%s (%s)", r.host, r.port, r.addr) thx := r.trace.NewTLSHandshakerStdlib(r.logger) tconn, _, err := thx.Handshake(r.ctx, conn, r.tlsconfig) r.tk.TLSHandshakes = append(r.tk.TLSHandshakes, r.trace.FirstTLSHandshakeOrNil()) if err != nil { + r.smtp_error(err) return nil, false } r.logger.Infof("Handshake succeeded") return tconn, true } -func (r SMTPRunner) starttls(conn net.Conn, host string, port string, message string) (net.Conn, bool) { - r.logger.Infof("Asking for StartTLS upgrade") - conn.Write([]byte(message)) - tconn, success := r.handshake(conn, host, port) +func (r SMTPRunner) starttls(conn net.Conn, message string) (net.Conn, bool) { + if message != "" { + r.logger.Infof("Asking for StartTLS upgrade") + conn.Write([]byte(message)) + } + tconn, success := r.handshake(conn) return tconn, success } func (r SMTPRunner) smtp(conn net.Conn, ehlo string, noop uint8) bool { client, err := smtp.NewClient(conn, ehlo) if err != nil { - r.tk.SMTPErrors = append(r.tk.SMTPErrors, []*string{tracex.NewFailure(err)}...) + r.smtp_error(err) return false } err = client.Hello(ehlo) if err != nil { - r.tk.SMTPErrors = append(r.tk.SMTPErrors, []*string{tracex.NewFailure(err)}...) + r.smtp_error(err) return false } @@ -189,7 +210,7 @@ func (r SMTPRunner) smtp(conn net.Conn, ehlo string, noop uint8) bool { r.logger.Infof("NoOp Iteration %d", r.tk.NoOpCounter) err = client.Noop() if err != nil { - r.tk.SMTPErrors = append(r.tk.SMTPErrors, []*string{tracex.NewFailure(err)}...) + r.smtp_error(err) break } } @@ -222,6 +243,8 @@ func (m Measurer) Run( } tk := new(TestKeys) + // TODO: make it so we don't forget it + tk.SMTPErrors = make(map[string][]*string) measurement.TestKeys = tk ctx, cancel := context.WithTimeout(ctx, 60*time.Second) @@ -238,6 +261,9 @@ func (m Measurer) Run( ctx: ctx, tk: tk, tlsconfig: &tlsconfig, + host: config.host, + port: config.port, + addr: "", } // First resolve DNS @@ -247,7 +273,7 @@ func (m Measurer) Run( } for _, addr := range addrs { - conn, success := runner.conn(addr, config.port) + conn, success := runner.conn(addr) if !success { return nil } @@ -255,7 +281,7 @@ func (m Measurer) Run( if config.forced_tls { // Direct TLS connection - tconn, success := runner.handshake(conn, config.host, config.port) + tconn, success := runner.handshake(conn) if !success { continue } @@ -272,7 +298,7 @@ func (m Measurer) Run( } // Upgrade via StartTLS and try EHLO + NoOps - tconn, success := runner.starttls(conn, config.host, config.port, "STARTTLS\n") + tconn, success := runner.starttls(conn, "STARTTLS\n") if !success { continue } From 25ebafa4275ba741d325244aeb2cb70e9ebfe1cf Mon Sep 17 00:00:00 2001 From: ooninoob Date: Sun, 20 Nov 2022 18:30:15 +0100 Subject: [PATCH 04/14] Auto-initialize per-addr keys in SMTPErrors --- internal/engine/experiment/smtp/smtp.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go index a0d38ac..09d3484 100644 --- a/internal/engine/experiment/smtp/smtp.go +++ b/internal/engine/experiment/smtp/smtp.go @@ -133,12 +133,8 @@ type SMTPRunner struct { func (r SMTPRunner) smtp_error(err error) { key := net.JoinHostPort(r.addr, r.port) - errors, exists := r.tk.SMTPErrors[key] - if exists { - r.tk.SMTPErrors[key] = append(errors, tracex.NewFailure(err)) - } else { - r.tk.SMTPErrors[key] = []*string{tracex.NewFailure(err)} - } + // Key is initialized in conn() no need to check here + r.tk.SMTPErrors[key] = append(r.tk.SMTPErrors[key], tracex.NewFailure(err)) } func (r SMTPRunner) resolve(host string) ([]string, bool) { @@ -156,7 +152,13 @@ func (r SMTPRunner) resolve(host string) ([]string, bool) { } func (r SMTPRunner) conn(addr string) (net.Conn, bool) { + // Initialize addr field and corresponding errors in TestKeys r.addr = addr + if r.tk.SMTPErrors == nil { + r.tk.SMTPErrors = make(map[string][]*string) + } + r.tk.SMTPErrors[net.JoinHostPort(addr, r.port)] = []*string{} + dialer := r.trace.NewDialerWithoutResolver(r.logger) conn, err := dialer.DialContext(r.ctx, "tcp", net.JoinHostPort(r.addr, r.port)) r.tk.TCPConnect = append(r.tk.TCPConnect, r.trace.TCPConnects()...) @@ -243,8 +245,6 @@ func (m Measurer) Run( } tk := new(TestKeys) - // TODO: make it so we don't forget it - tk.SMTPErrors = make(map[string][]*string) measurement.TestKeys = tk ctx, cancel := context.WithTimeout(ctx, 60*time.Second) From d81f22d46e99ef7e2b771521cf844a7976c5608a Mon Sep 17 00:00:00 2001 From: ooninoob Date: Sun, 20 Nov 2022 20:10:00 +0100 Subject: [PATCH 05/14] Failed experiment results in numerous nil pointer dereferences --- internal/engine/experiment/smtp/smtp.go | 189 +++++++++++++++--------- 1 file changed, 121 insertions(+), 68 deletions(-) diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go index 09d3484..50b66af 100644 --- a/internal/engine/experiment/smtp/smtp.go +++ b/internal/engine/experiment/smtp/smtp.go @@ -92,7 +92,7 @@ type TestKeys struct { Queries []*model.ArchivalDNSLookupResult `json:"queries"` TCPConnect []*model.ArchivalTCPConnectResult `json:"tcp_connect"` TLSHandshakes []*model.ArchivalTLSOrQUICHandshakeResult `json:"tls_handshakes"` - SMTPErrors map[string][]*string `json:"smtp"` + Errors map[string][]*string `json:"smtp"` NoOpCounter uint8 `json:"successful_noops"` // Used for global failure (DNS resolution) Failure string `json:"failure"` @@ -117,9 +117,7 @@ func (m Measurer) ExperimentVersion() string { return testVersion } -// Manages sequential SMTP sessions to the same hostname (over different IPs) -// don't use in parallel! -type SMTPRunner struct { +type TCPRunner struct { trace *measurexlite.Trace logger model.Logger ctx context.Context @@ -127,17 +125,84 @@ type SMTPRunner struct { tlsconfig *tls.Config host string port string - // addr is changed everytime SMTPRunner.conn(addr) is called - addr string } -func (r SMTPRunner) smtp_error(err error) { - key := net.JoinHostPort(r.addr, r.port) - // Key is initialized in conn() no need to check here - r.tk.SMTPErrors[key] = append(r.tk.SMTPErrors[key], tracex.NewFailure(err)) +type TCPSession struct { + addr string + port string + runner *TCPRunner + errors []*string + tls bool + raw_conn *net.Conn + tls_conn *net.Conn } -func (r SMTPRunner) resolve(host string) ([]string, bool) { +func (s TCPSession) Close() { + if s.tls { + var conn = *s.tls_conn + conn.Close() + //*(s.tls_conn).Close() + } else { + var conn = *s.raw_conn + conn.Close() + //*(s.raw_conn).Close() + } +} + +func (s TCPSession) current_conn() net.Conn { + if s.tls { + return *s.tls_conn + } else { + return *s.raw_conn + } +} + +func (r TCPRunner) conn(addr string, port string) (*TCPSession, bool) { + key := net.JoinHostPort(addr, port) + // Initialize errors + if r.tk.Errors == nil { + r.tk.Errors = make(map[string][]*string) + } + r.tk.Errors[key] = []*string{} + + s := new(TCPSession) + if !s.conn(addr, port, r, r.tk.Errors[key]) { + return nil, false + } + return s, true +} + +func (r TCPRunner) dial(addr string, port string) (net.Conn, error) { + dialer := r.trace.NewDialerWithoutResolver(r.logger) + conn, err := dialer.DialContext(r.ctx, "tcp", net.JoinHostPort(addr, port)) + r.tk.TCPConnect = append(r.tk.TCPConnect, r.trace.TCPConnects()...) + return conn, err + +} + +func (s TCPSession) conn(addr string, port string, runner TCPRunner, errors []*string) bool { + // Initialize addr field and corresponding errors in TestKeys + s.addr = addr + s.port = port + s.tls = false + s.runner = &runner + s.errors = errors + + conn, err := runner.dial(addr, port) + if err != nil { + s.error(err) + return false + } + s.raw_conn = &conn + + return true +} + +func (s TCPSession) error(err error) { + s.errors = append(s.errors, tracex.NewFailure(err)) +} + +func (r TCPRunner) resolve(host string) ([]string, bool) { r.logger.Infof("Resolving DNS for %s", host) resolver := r.trace.NewStdlibResolver(r.logger) addrs, err := resolver.LookupHost(r.ctx, host) @@ -151,77 +216,70 @@ func (r SMTPRunner) resolve(host string) ([]string, bool) { return addrs, true } -func (r SMTPRunner) conn(addr string) (net.Conn, bool) { - // Initialize addr field and corresponding errors in TestKeys - r.addr = addr - if r.tk.SMTPErrors == nil { - r.tk.SMTPErrors = make(map[string][]*string) +func (s TCPSession) handshake() bool { + if s.tls { + // TLS already initialized... + return true } - r.tk.SMTPErrors[net.JoinHostPort(addr, r.port)] = []*string{} - - dialer := r.trace.NewDialerWithoutResolver(r.logger) - conn, err := dialer.DialContext(r.ctx, "tcp", net.JoinHostPort(r.addr, r.port)) - r.tk.TCPConnect = append(r.tk.TCPConnect, r.trace.TCPConnects()...) + s.runner.logger.Infof("Starting TLS handshake with %s:%s", s.addr, s.port) + thx := s.runner.trace.NewTLSHandshakerStdlib(s.runner.logger) + tconn, _, err := thx.Handshake(s.runner.ctx, *s.raw_conn, s.runner.tlsconfig) + s.runner.tk.TLSHandshakes = append(s.runner.tk.TLSHandshakes, s.runner.trace.FirstTLSHandshakeOrNil()) if err != nil { - r.smtp_error(err) - return nil, false + s.error(err) + return false } - return conn, true + + s.tls = true + s.tls_conn = &tconn + s.runner.logger.Infof("Handshake succeeded") + return true } -func (r SMTPRunner) handshake(conn net.Conn) (net.Conn, bool) { - r.logger.Infof("Starting TLS handshake with %s:%s (%s)", r.host, r.port, r.addr) - thx := r.trace.NewTLSHandshakerStdlib(r.logger) - tconn, _, err := thx.Handshake(r.ctx, conn, r.tlsconfig) - r.tk.TLSHandshakes = append(r.tk.TLSHandshakes, r.trace.FirstTLSHandshakeOrNil()) - if err != nil { - r.smtp_error(err) - return nil, false +func (s TCPSession) starttls(message string) bool { + if s.tls { + // TLS already initialized... + return true } - r.logger.Infof("Handshake succeeded") - return tconn, true -} - -func (r SMTPRunner) starttls(conn net.Conn, message string) (net.Conn, bool) { if message != "" { - r.logger.Infof("Asking for StartTLS upgrade") - conn.Write([]byte(message)) + s.runner.logger.Infof("Asking for StartTLS upgrade") + s.current_conn().Write([]byte(message)) } - tconn, success := r.handshake(conn) - return tconn, success + return s.handshake() } -func (r SMTPRunner) smtp(conn net.Conn, ehlo string, noop uint8) bool { - client, err := smtp.NewClient(conn, ehlo) +func (s TCPSession) smtp(ehlo string, noop uint8) bool { + // Auto-choose plaintext/TCP session + client, err := smtp.NewClient(s.current_conn(), ehlo) if err != nil { - r.smtp_error(err) + s.error(err) return false } err = client.Hello(ehlo) if err != nil { - r.smtp_error(err) + s.error(err) return false } if noop > 0 { - r.logger.Infof("Trying to generate more no-op traffic") + s.runner.logger.Infof("Trying to generate more no-op traffic") // TODO: noop counter per IP address - r.tk.NoOpCounter = 0 - for r.tk.NoOpCounter < noop { - r.tk.NoOpCounter += 1 - r.logger.Infof("NoOp Iteration %d", r.tk.NoOpCounter) + s.runner.tk.NoOpCounter = 0 + for s.runner.tk.NoOpCounter < noop { + s.runner.tk.NoOpCounter += 1 + s.runner.logger.Infof("NoOp Iteration %d", s.runner.tk.NoOpCounter) err = client.Noop() if err != nil { - r.smtp_error(err) + s.error(err) break } } - if r.tk.NoOpCounter == noop { - r.logger.Infof("Successfully generated no-op traffic") + if s.runner.tk.NoOpCounter == noop { + s.runner.logger.Infof("Successfully generated no-op traffic") return true } else { - r.logger.Infof("Failed no-op traffic at iteration %d", r.tk.NoOpCounter) + s.runner.logger.Infof("Failed no-op traffic at iteration %d", s.runner.tk.NoOpCounter) return false } } @@ -255,7 +313,7 @@ func (m Measurer) Run( ServerName: config.host, } - runner := SMTPRunner{ + runner := TCPRunner{ trace: trace, logger: log, ctx: ctx, @@ -263,7 +321,6 @@ func (m Measurer) Run( tlsconfig: &tlsconfig, host: config.host, port: config.port, - addr: "", } // First resolve DNS @@ -273,38 +330,34 @@ func (m Measurer) Run( } for _, addr := range addrs { - conn, success := runner.conn(addr) + tcp_session, success := runner.conn(addr, config.port) if !success { - return nil + continue } - defer conn.Close() + defer tcp_session.Close() if config.forced_tls { // Direct TLS connection - tconn, success := runner.handshake(conn) - if !success { + if !tcp_session.handshake() { continue } - defer tconn.Close() // Try EHLO + NoOps - if !runner.smtp(tconn, "localhost", 10) { + if !tcp_session.smtp("localhost", 10) { continue } } else { // StartTLS... first try plaintext EHLO - if !runner.smtp(conn, "localhost", 0) { + if !tcp_session.smtp("localhost", 0) { continue } // Upgrade via StartTLS and try EHLO + NoOps - tconn, success := runner.starttls(conn, "STARTTLS\n") - if !success { + if !tcp_session.starttls("STARTTLS\n") { continue } - defer tconn.Close() - if !runner.smtp(tconn, "localhost", 10) { + if !tcp_session.smtp("localhost", 10) { continue } } From 9783e3bb47b783bbb9946eb3c050c78d970d11a5 Mon Sep 17 00:00:00 2001 From: ooninoob Date: Sun, 20 Nov 2022 21:37:34 +0100 Subject: [PATCH 06/14] Fix broken experiment by properly using pointers --- internal/engine/experiment/smtp/smtp.go | 115 +++++++++++++++--------- 1 file changed, 72 insertions(+), 43 deletions(-) diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go index 50b66af..97c183f 100644 --- a/internal/engine/experiment/smtp/smtp.go +++ b/internal/engine/experiment/smtp/smtp.go @@ -88,14 +88,24 @@ func config(input model.MeasurementTarget) (*Config, error) { } // TestKeys contains the experiment results + type TestKeys struct { - Queries []*model.ArchivalDNSLookupResult `json:"queries"` - TCPConnect []*model.ArchivalTCPConnectResult `json:"tcp_connect"` - TLSHandshakes []*model.ArchivalTLSOrQUICHandshakeResult `json:"tls_handshakes"` - Errors map[string][]*string `json:"smtp"` - NoOpCounter uint8 `json:"successful_noops"` + Queries []*model.ArchivalDNSLookupResult `json:"queries"` + Runs map[string]*IndividualTestKeys `json:"runs"` // Used for global failure (DNS resolution) Failure string `json:"failure"` + // Indicates global failure or individual test failure + Failed bool `json:"failed"` +} + +// IndividualTestKeys contains results for TCP/IP level stuff for each address found +// in the DNS lookup +type IndividualTestKeys struct { + NoOpCounter uint8 + TCPConnect []*model.ArchivalTCPConnectResult `json:"tcp_connect"` + TLSHandshakes []*model.ArchivalTLSOrQUICHandshakeResult `json:"tls_handshakes"` + // Individual failure aborting the test run for this address/port combo + Failure *string `json:"failure"` } type Measurer struct { @@ -117,6 +127,8 @@ func (m Measurer) ExperimentVersion() string { return testVersion } +// Manages sequential TCP sessions to the same hostname (over different IPs) +// don't use in parallel! type TCPRunner struct { trace *measurexlite.Trace logger model.Logger @@ -125,31 +137,31 @@ type TCPRunner struct { tlsconfig *tls.Config host string port string + // addr is changed everytime TCPRunner.conn(addr) is called + addr string } type TCPSession struct { addr string port string runner *TCPRunner - errors []*string + tk *IndividualTestKeys tls bool raw_conn *net.Conn tls_conn *net.Conn } -func (s TCPSession) Close() { +func (s *TCPSession) Close() { if s.tls { - var conn = *s.tls_conn - conn.Close() - //*(s.tls_conn).Close() + var conn = *s.tls_conn + conn.Close() } else { - var conn = *s.raw_conn - conn.Close() - //*(s.raw_conn).Close() - } + var conn = *s.raw_conn + conn.Close() + } } -func (s TCPSession) current_conn() net.Conn { +func (s *TCPSession) current_conn() net.Conn { if s.tls { return *s.tls_conn } else { @@ -157,52 +169,69 @@ func (s TCPSession) current_conn() net.Conn { } } -func (r TCPRunner) conn(addr string, port string) (*TCPSession, bool) { - key := net.JoinHostPort(addr, port) - // Initialize errors - if r.tk.Errors == nil { - r.tk.Errors = make(map[string][]*string) - } - r.tk.Errors[key] = []*string{} +func (r *TCPRunner) run_key() string { + return net.JoinHostPort(r.addr, r.port) +} + +func (r *TCPRunner) get_run() *IndividualTestKeys { + if r.tk.Runs == nil { + r.tk.Runs = make(map[string]*IndividualTestKeys) + } + key := r.run_key() + val, exists := r.tk.Runs[key] + if exists { + return val + } else { + r.tk.Runs[key] = &IndividualTestKeys{} + return r.tk.Runs[key] + } +} + +func (r *TCPRunner) conn(addr string, port string) (*TCPSession, bool) { + r.addr = addr + run := r.get_run() s := new(TCPSession) - if !s.conn(addr, port, r, r.tk.Errors[key]) { + if !s.conn(addr, port, r, run) { return nil, false } return s, true } -func (r TCPRunner) dial(addr string, port string) (net.Conn, error) { +func (r *TCPRunner) dial(addr string, port string) (net.Conn, error) { dialer := r.trace.NewDialerWithoutResolver(r.logger) conn, err := dialer.DialContext(r.ctx, "tcp", net.JoinHostPort(addr, port)) - r.tk.TCPConnect = append(r.tk.TCPConnect, r.trace.TCPConnects()...) + run := r.get_run() + run.TCPConnect = append(run.TCPConnect, r.trace.TCPConnects()...) return conn, err } -func (s TCPSession) conn(addr string, port string, runner TCPRunner, errors []*string) bool { +func (s *TCPSession) conn(addr string, port string, runner *TCPRunner, tk *IndividualTestKeys) bool { // Initialize addr field and corresponding errors in TestKeys s.addr = addr s.port = port s.tls = false - s.runner = &runner - s.errors = errors + s.runner = runner + s.tk = tk conn, err := runner.dial(addr, port) if err != nil { s.error(err) return false } - s.raw_conn = &conn + s.raw_conn = &conn return true } -func (s TCPSession) error(err error) { - s.errors = append(s.errors, tracex.NewFailure(err)) +func (s *TCPSession) error(err error) { + s.runner.tk.Failed = true + s.tk.Failure = tracex.NewFailure(err) + //s. = append(s.errors, tracex.NewFailure(err)) } -func (r TCPRunner) resolve(host string) ([]string, bool) { +func (r *TCPRunner) resolve(host string) ([]string, bool) { r.logger.Infof("Resolving DNS for %s", host) resolver := r.trace.NewStdlibResolver(r.logger) addrs, err := resolver.LookupHost(r.ctx, host) @@ -216,7 +245,7 @@ func (r TCPRunner) resolve(host string) ([]string, bool) { return addrs, true } -func (s TCPSession) handshake() bool { +func (s *TCPSession) handshake() bool { if s.tls { // TLS already initialized... return true @@ -224,7 +253,7 @@ func (s TCPSession) handshake() bool { s.runner.logger.Infof("Starting TLS handshake with %s:%s", s.addr, s.port) thx := s.runner.trace.NewTLSHandshakerStdlib(s.runner.logger) tconn, _, err := thx.Handshake(s.runner.ctx, *s.raw_conn, s.runner.tlsconfig) - s.runner.tk.TLSHandshakes = append(s.runner.tk.TLSHandshakes, s.runner.trace.FirstTLSHandshakeOrNil()) + s.tk.TLSHandshakes = append(s.tk.TLSHandshakes, s.runner.trace.FirstTLSHandshakeOrNil()) if err != nil { s.error(err) return false @@ -236,7 +265,7 @@ func (s TCPSession) handshake() bool { return true } -func (s TCPSession) starttls(message string) bool { +func (s *TCPSession) starttls(message string) bool { if s.tls { // TLS already initialized... return true @@ -248,7 +277,7 @@ func (s TCPSession) starttls(message string) bool { return s.handshake() } -func (s TCPSession) smtp(ehlo string, noop uint8) bool { +func (s *TCPSession) smtp(ehlo string, noop uint8) bool { // Auto-choose plaintext/TCP session client, err := smtp.NewClient(s.current_conn(), ehlo) if err != nil { @@ -264,10 +293,10 @@ func (s TCPSession) smtp(ehlo string, noop uint8) bool { if noop > 0 { s.runner.logger.Infof("Trying to generate more no-op traffic") // TODO: noop counter per IP address - s.runner.tk.NoOpCounter = 0 - for s.runner.tk.NoOpCounter < noop { - s.runner.tk.NoOpCounter += 1 - s.runner.logger.Infof("NoOp Iteration %d", s.runner.tk.NoOpCounter) + s.tk.NoOpCounter = 0 + for s.tk.NoOpCounter < noop { + s.tk.NoOpCounter += 1 + s.runner.logger.Infof("NoOp Iteration %d", s.tk.NoOpCounter) err = client.Noop() if err != nil { s.error(err) @@ -275,11 +304,11 @@ func (s TCPSession) smtp(ehlo string, noop uint8) bool { } } - if s.runner.tk.NoOpCounter == noop { + if s.tk.NoOpCounter == noop { s.runner.logger.Infof("Successfully generated no-op traffic") return true } else { - s.runner.logger.Infof("Failed no-op traffic at iteration %d", s.runner.tk.NoOpCounter) + s.runner.logger.Infof("Failed no-op traffic at iteration %d", s.tk.NoOpCounter) return false } } @@ -313,7 +342,7 @@ func (m Measurer) Run( ServerName: config.host, } - runner := TCPRunner{ + runner := &TCPRunner{ trace: trace, logger: log, ctx: ctx, From c95fb894f0a6a36696285144dc55a6852df66bef Mon Sep 17 00:00:00 2001 From: ooninoob Date: Mon, 21 Nov 2022 14:13:41 +0100 Subject: [PATCH 07/14] Test suite expects static Config not depending on input --- internal/engine/experiment/smtp/smtp.go | 26 +++++++++---------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go index 97c183f..568db04 100644 --- a/internal/engine/experiment/smtp/smtp.go +++ b/internal/engine/experiment/smtp/smtp.go @@ -8,7 +8,6 @@ import ( "net" "net/smtp" "net/url" - "strconv" "time" "github.com/ooni/probe-cli/v3/internal/engine/experiment/urlgetter" @@ -26,9 +25,6 @@ var ( // errInvalidScheme indicates that the scheme is invalid errInvalidScheme = errors.New("scheme must be smtp(s)") - - // errInvalidPort indicates that the port provided could not be parsed as an int - errInvalidPort = errors.New("Port number is not a valid integer") ) const ( @@ -37,14 +33,16 @@ const ( ) // Config contains the experiment config. -type Config struct { +type Config struct{} + +type RuntimeConfig struct { host string port string forced_tls bool noop_count uint8 } -func config(input model.MeasurementTarget) (*Config, error) { +func config(input model.MeasurementTarget) (*RuntimeConfig, error) { if input == "" { // TODO: static input data (eg. gmail/riseup..) return nil, errNoInputProvided @@ -68,16 +66,11 @@ func config(input model.MeasurementTarget) (*Config, error) { port = "465" } } else { - // Check that requested port is a valid integer - _, err := strconv.Atoi(parsed.Port()) - if err != nil { - return nil, errInvalidPort - } else { - port = parsed.Port() - } + // Valid port is checked by URL parsing + port = parsed.Port() } - valid_config := Config{ + valid_config := RuntimeConfig{ host: parsed.Hostname(), forced_tls: parsed.Scheme == "smtps", port: port, @@ -321,7 +314,6 @@ func (m Measurer) Run( ctx context.Context, sess model.ExperimentSession, measurement *model.Measurement, callbacks model.ExperimentCallbacks, ) error { - log := sess.Logger() trace := measurexlite.NewTrace(0, measurement.MeasurementStartTimeSaved) @@ -372,7 +364,7 @@ func (m Measurer) Run( } // Try EHLO + NoOps - if !tcp_session.smtp("localhost", 10) { + if !tcp_session.smtp("localhost", config.noop_count) { continue } } else { @@ -386,7 +378,7 @@ func (m Measurer) Run( continue } - if !tcp_session.smtp("localhost", 10) { + if !tcp_session.smtp("localhost", config.noop_count) { continue } } From b4d5eebb60860171fcb0fa07d70ddce5d25c3fec Mon Sep 17 00:00:00 2001 From: ooninoob Date: Mon, 21 Nov 2022 14:14:22 +0100 Subject: [PATCH 08/14] First iteration for tests --- internal/engine/experiment/smtp/smtp_test.go | 185 +++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 internal/engine/experiment/smtp/smtp_test.go diff --git a/internal/engine/experiment/smtp/smtp_test.go b/internal/engine/experiment/smtp/smtp_test.go new file mode 100644 index 0000000..e49af52 --- /dev/null +++ b/internal/engine/experiment/smtp/smtp_test.go @@ -0,0 +1,185 @@ +package smtp + +import ( + "bufio" + "context" + "crypto/tls" + "errors" + "fmt" + "net" + "strings" + "testing" + + "github.com/ooni/probe-cli/v3/internal/engine/mockable" + "github.com/ooni/probe-cli/v3/internal/model" +) + +func plaintextListener() net.Listener { + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + if l, err = net.Listen("tcp6", "[::1]:0"); err != nil { + panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err)) + } + } + return l +} + +func tlsListener(l net.Listener) net.Listener { + return tls.NewListener(l, &tls.Config{}) +} + +func listener_addr(l net.Listener) string { + return l.Addr().String() +} + +func ValidSMTPServer(conn net.Conn) { + for { + command, err := bufio.NewReader(conn).ReadString('\n') + if err != nil { + return + } + + if command == "" { + } else if command == "NOOP" { + conn.Write([]byte("250 2.0.0 Ok\n")) + } else if command == "STARTTLS" { + conn.Write([]byte("220 2.0.0 Ready to start TLS\n")) + // TODO: conn.Close does not actually close connection? or does client not detect it? + conn.Close() + return + } else if strings.HasPrefix(command, "EHLO") { + conn.Write([]byte("250 mock.example.com\n")) + } + conn.Write([]byte("\n")) + } +} + +func TCPServer(l net.Listener) { + for { + conn, err := l.Accept() + if err != nil { + continue + } + defer conn.Close() + conn.Write([]byte("220 mock.example.com ESMTP (spam is not appreciated)\n")) + ValidSMTPServer(conn) + } +} + +func TestMeasurer_run(t *testing.T) { + // runHelper is an helper function to run this set of tests. + runHelper := func(input string) (*model.Measurement, model.ExperimentMeasurer, error) { + m := NewExperimentMeasurer(Config{}) + if m.ExperimentName() != "smtp" { + t.Fatal("invalid experiment name") + } + if m.ExperimentVersion() != "0.0.1" { + t.Fatal("invalid experiment version") + } + ctx := context.Background() + meas := &model.Measurement{ + Input: model.MeasurementTarget(input), + } + sess := &mockable.Session{ + MockableLogger: model.DiscardLogger, + } + callbacks := model.NewPrinterCallbacks(model.DiscardLogger) + err := m.Run(ctx, sess, meas, callbacks) + return meas, m, err + } + + t.Run("with empty input", func(t *testing.T) { + _, _, err := runHelper("") + if !errors.Is(err, errNoInputProvided) { + t.Fatal("unexpected error", err) + } + }) + + t.Run("with invalid URL", func(t *testing.T) { + _, _, err := runHelper("\t") + if !errors.Is(err, errInputIsNotAnURL) { + t.Fatal("unexpected error", err) + } + }) + + t.Run("with invalid scheme", func(t *testing.T) { + _, _, err := runHelper("https://8.8.8.8:443/") + if !errors.Is(err, errInvalidScheme) { + t.Fatal("unexpected error", err) + } + }) + + t.Run("with broken TLS", func(t *testing.T) { + p := plaintextListener() + defer p.Close() + + l := tlsListener(p) + defer l.Close() + addr := listener_addr(l) + go TCPServer(l) + + meas, m, err := runHelper("smtps://" + addr) + if err != nil { + t.Fatal(err) + } + + tk := meas.TestKeys.(*TestKeys) + + for _, run := range tk.Runs { + for _, handshake := range run.TLSHandshakes { + if *handshake.Failure != "unknown_failure: remote error: tls: unrecognized name" { + t.Fatal("expected unrecognized_name in TLS handshake") + } + } + + if run.NoOpCounter != 0 { + t.Fatalf("expected to not have any noops, not %d noops", run.NoOpCounter) + } + } + + ask, err := m.GetSummaryKeys(meas) + if err != nil { + t.Fatal("cannot obtain summary") + } + summary := ask.(SummaryKeys) + if summary.IsAnomaly { + t.Fatal("expected no anomaly") + } + }) + + t.Run("with broken starttls", func(t *testing.T) { + l := plaintextListener() + defer l.Close() + addr := listener_addr(l) + + go TCPServer(l) + + meas, m, err := runHelper("smtp://" + addr) + if err != nil { + t.Fatal(err) + } + + tk := meas.TestKeys.(*TestKeys) + + for _, run := range tk.Runs { + for _, handshake := range run.TLSHandshakes { + if *handshake.Failure != "generic_timeout_error" { + t.Fatal("expected timeout in TLS handshake") + } + } + + if run.NoOpCounter != 0 { + t.Fatalf("expected to not have any noops, not %d noops", run.NoOpCounter) + } + } + + ask, err := m.GetSummaryKeys(meas) + if err != nil { + t.Fatal("cannot obtain summary") + } + summary := ask.(SummaryKeys) + if summary.IsAnomaly { + t.Fatal("expected no anomaly") + } + }) +} From 4c96b2f0f4f356ad3f0daf7062e85e4a4de80052 Mon Sep 17 00:00:00 2001 From: ooninoob Date: Sun, 20 Nov 2022 21:04:57 +0100 Subject: [PATCH 09/14] Rebase on experiment --- internal/engine/experiment/smtp/smtp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go index 568db04..d97446c 100644 --- a/internal/engine/experiment/smtp/smtp.go +++ b/internal/engine/experiment/smtp/smtp.go @@ -230,7 +230,7 @@ func (r *TCPRunner) resolve(host string) ([]string, bool) { addrs, err := resolver.LookupHost(r.ctx, host) r.tk.Queries = append(r.tk.Queries, r.trace.DNSLookupsFromRoundTrip()...) if err != nil { - r.tk.Failure = *tracex.NewFailure(err) + r.global_error(err) return []string{}, false } r.logger.Infof("Finished DNS for %s: %v", host, addrs) From 95b245cda46992020f6ee6f4c8624471104b1fbb Mon Sep 17 00:00:00 2001 From: ooninoob Date: Mon, 21 Nov 2022 14:21:06 +0100 Subject: [PATCH 10/14] missed that in rebase --- internal/engine/experiment/smtp/smtp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/engine/experiment/smtp/smtp.go b/internal/engine/experiment/smtp/smtp.go index d97446c..568db04 100644 --- a/internal/engine/experiment/smtp/smtp.go +++ b/internal/engine/experiment/smtp/smtp.go @@ -230,7 +230,7 @@ func (r *TCPRunner) resolve(host string) ([]string, bool) { addrs, err := resolver.LookupHost(r.ctx, host) r.tk.Queries = append(r.tk.Queries, r.trace.DNSLookupsFromRoundTrip()...) if err != nil { - r.global_error(err) + r.tk.Failure = *tracex.NewFailure(err) return []string{}, false } r.logger.Infof("Finished DNS for %s: %v", host, addrs) From c2ea0b470485c2699ce888286ce444a77a7a50c3 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 21 Nov 2022 16:28:53 +0100 Subject: [PATCH 11/14] feat(webconnectivity): try all the available THs (#980) We introduce a fork of internal/httpx, named internal/httpapi, where there is a clear split between the concept of an API endpoint (such as https://0.th.ooni.org/) and of an API descriptor (such as using `GET` to access /api/v1/test-list/url). Additionally, httpapi allows to create a SequenceCaller that tries to call a given API descriptor using multiple API endpoints. The SequenceCaller will stop once an endpoint works or when all the available endpoints have been tried unsuccessfully. The definition of "success" is the following: we consider "failure" any error that occurs during the HTTP round trip or when reading the response body. We DO NOT consider "failure" errors (1) when parsing the input URL; (2) when the server returns >= 400; (3) when the server returns a string that does not parse as valid JSON. The idea of this classification of failures is that we ONLY want to retry when we see what looks like a network error that may be caused by (collateral or targeted) censorship. We take advantage of the availability of this new package and we refactor web_connectivity@v0.4 and web_connectivity@v0.5 to use a SequenceCaller for calling the web connectivity TH API. This means that we will now try all the available THs advertised by the backend rather than just selecting and using the first one provided by the backend. Because this diff is designed to be backported to the `release/3.16` branch, we have omitted additional changes to always use httpapi where we are currently using httpx. Yet, to remind ourselves about the need to do that, we have deprecated the httpx package. We will rewrite all the code currently using httpx to use httpapi as part of future work. It is also worth noting that httpapi will allow us to refactor the backend code such that (1) we remove code to select a backend URL endpoint at the beginning and (2) we try several endpoints. The design of the code is such that we can add to the mix some endpoints using as `http.Client` a special client using a tunnel. This will allow us to automatically fallback backend queries. Closes https://github.com/ooni/probe/issues/2353. Related to https://github.com/ooni/probe/issues/1519. --- .../experiment/webconnectivity/control.go | 30 +- .../webconnectivity/webconnectivity.go | 24 +- .../webconnectivity/webconnectivity_test.go | 2 +- .../webconnectivity/cleartextflow.go | 2 +- .../experiment/webconnectivity/control.go | 33 +- .../webconnectivity/dnsresolvers.go | 13 +- .../experiment/webconnectivity/measurer.go | 26 +- .../experiment/webconnectivity/secureflow.go | 2 +- .../experiment/webconnectivity/testkeys.go | 20 + internal/httpapi/call.go | 181 +++ internal/httpapi/call_test.go | 1163 +++++++++++++++++ internal/httpapi/descriptor.go | 155 +++ internal/httpapi/descriptor_test.go | 248 ++++ internal/httpapi/doc.go | 15 + internal/httpapi/endpoint.go | 76 ++ internal/httpapi/endpoint_test.go | 69 + internal/httpapi/sequence.go | 92 ++ internal/httpapi/sequence_test.go | 358 +++++ internal/httpx/httpx.go | 4 + 19 files changed, 2446 insertions(+), 67 deletions(-) create mode 100644 internal/httpapi/call.go create mode 100644 internal/httpapi/call_test.go create mode 100644 internal/httpapi/descriptor.go create mode 100644 internal/httpapi/descriptor_test.go create mode 100644 internal/httpapi/doc.go create mode 100644 internal/httpapi/endpoint.go create mode 100644 internal/httpapi/endpoint_test.go create mode 100644 internal/httpapi/sequence.go create mode 100644 internal/httpapi/sequence_test.go diff --git a/internal/engine/experiment/webconnectivity/control.go b/internal/engine/experiment/webconnectivity/control.go index ccaf870..bd953a1 100644 --- a/internal/engine/experiment/webconnectivity/control.go +++ b/internal/engine/experiment/webconnectivity/control.go @@ -4,9 +4,10 @@ import ( "context" "github.com/ooni/probe-cli/v3/internal/geoipx" - "github.com/ooni/probe-cli/v3/internal/httpx" + "github.com/ooni/probe-cli/v3/internal/httpapi" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" ) // Redirect to types defined inside the model package @@ -21,22 +22,23 @@ type ( // Control performs the control request and returns the response. func Control( ctx context.Context, sess model.ExperimentSession, - thAddr string, creq ControlRequest) (out ControlResponse, err error) { - clnt := &httpx.APIClientTemplate{ - BaseURL: thAddr, - HTTPClient: sess.DefaultHTTPClient(), - Logger: sess.Logger(), - UserAgent: sess.UserAgent(), - } + testhelpers []model.OOAPIService, creq ControlRequest) (ControlResponse, *model.OOAPIService, error) { + seqCaller := httpapi.NewSequenceCaller( + httpapi.MustNewPOSTJSONWithJSONResponseDescriptor(sess.Logger(), "/", creq).WithBodyLogging(true), + httpapi.NewEndpointList(sess.DefaultHTTPClient(), sess.UserAgent(), testhelpers...)..., + ) sess.Logger().Infof("control for %s...", creq.HTTPRequest) - // make sure error is wrapped - err = clnt.WithBodyLogging().Build().PostJSON(ctx, "/", creq, &out) - if err != nil { - err = netxlite.NewTopLevelGenericErrWrapper(err) - } + var out ControlResponse + idx, err := seqCaller.CallWithJSONResponse(ctx, &out) sess.Logger().Infof("control for %s... %+v", creq.HTTPRequest, model.ErrorToStringOrOK(err)) + if err != nil { + // make sure error is wrapped + err = netxlite.NewTopLevelGenericErrWrapper(err) + return ControlResponse{}, nil, err + } fillASNs(&out.DNS) - return + runtimex.Assert(idx >= 0 && idx < len(testhelpers), "idx out of bounds") + return out, &testhelpers[idx], nil } // fillASNs fills the ASNs array of ControlDNSResult. For each Addr inside diff --git a/internal/engine/experiment/webconnectivity/webconnectivity.go b/internal/engine/experiment/webconnectivity/webconnectivity.go index 526b9f7..bdbea35 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity.go @@ -15,7 +15,7 @@ import ( const ( testName = "web_connectivity" - testVersion = "0.4.1" + testVersion = "0.4.2" ) // Config contains the experiment config. @@ -145,19 +145,9 @@ func (m Measurer) Run( } // 1. find test helper testhelpers, _ := sess.GetTestHelpersByName("web-connectivity") - var testhelper *model.OOAPIService - for _, th := range testhelpers { - if th.Type == "https" { - testhelper = &th - break - } - } - if testhelper == nil { + if len(testhelpers) < 1 { return ErrNoAvailableTestHelpers } - measurement.TestHelpers = map[string]interface{}{ - "backend": testhelper, - } // 2. perform the DNS lookup step dnsBegin := time.Now() dnsResult := DNSLookup(ctx, DNSLookupConfig{ @@ -167,10 +157,11 @@ func (m Measurer) Run( 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) + sess.Logger().Infof("using control: %+v", testhelpers) // 3. perform the control measurement thBegin := time.Now() - tk.Control, err = Control(ctx, sess, testhelper.Address, ControlRequest{ + var usedTH *model.OOAPIService + tk.Control, usedTH, err = Control(ctx, sess, testhelpers, ControlRequest{ HTTPRequest: URL.String(), HTTPRequestHeaders: map[string][]string{ "Accept": {model.HTTPHeaderAccept}, @@ -179,6 +170,11 @@ func (m Measurer) Run( }, TCPConnect: epnts.Endpoints(), }) + if usedTH != nil { + measurement.TestHelpers = map[string]interface{}{ + "backend": usedTH, + } + } tk.THRuntime = time.Since(thBegin) tk.ControlFailure = tracex.NewFailure(err) // 4. analyze DNS results diff --git a/internal/engine/experiment/webconnectivity/webconnectivity_test.go b/internal/engine/experiment/webconnectivity/webconnectivity_test.go index c1fcb37..6db3989 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.1" { + if measurer.ExperimentVersion() != "0.4.2" { t.Fatal("unexpected version") } } diff --git a/internal/experiment/webconnectivity/cleartextflow.go b/internal/experiment/webconnectivity/cleartextflow.go index 351a27c..d515d4e 100644 --- a/internal/experiment/webconnectivity/cleartextflow.go +++ b/internal/experiment/webconnectivity/cleartextflow.go @@ -285,7 +285,7 @@ func (t *CleartextFlow) maybeFollowRedirects(ctx context.Context, resp *http.Res WaitGroup: t.WaitGroup, Referer: resp.Request.URL.String(), Session: nil, // no need to issue another control request - THAddr: "", // ditto + TestHelpers: nil, // ditto UDPAddress: t.UDPAddress, } resolvers.Start(ctx) diff --git a/internal/experiment/webconnectivity/control.go b/internal/experiment/webconnectivity/control.go index 3e58f42..bf7003a 100644 --- a/internal/experiment/webconnectivity/control.go +++ b/internal/experiment/webconnectivity/control.go @@ -8,10 +8,11 @@ import ( "time" "github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity" - "github.com/ooni/probe-cli/v3/internal/httpx" + "github.com/ooni/probe-cli/v3/internal/httpapi" "github.com/ooni/probe-cli/v3/internal/measurexlite" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" ) // EndpointMeasurementsStarter is used by Control to start extra @@ -51,8 +52,8 @@ type Control struct { // Session is the MANDATORY session to use. Session model.ExperimentSession - // THAddr is the MANDATORY TH's URL. - THAddr string + // TestHelpers is the MANDATORY list of test helpers. + TestHelpers []model.OOAPIService // URL is the MANDATORY URL we are measuring. URL *url.URL @@ -102,26 +103,20 @@ func (c *Control) Run(parentCtx context.Context) { // create logger for this operation ol := measurexlite.NewOperationLogger( c.Logger, - "control for %s using %s", + "control for %s using %+v", creq.HTTPRequest, - c.THAddr, + c.TestHelpers, ) - // create an API client - clnt := (&httpx.APIClientTemplate{ - Accept: "", - Authorization: "", - BaseURL: c.THAddr, - HTTPClient: c.Session.DefaultHTTPClient(), - Host: "", // use the one inside the URL - LogBody: true, - Logger: c.Logger, - UserAgent: c.Session.UserAgent(), - }).Build() + // create an httpapi sequence caller + seqCaller := httpapi.NewSequenceCaller( + httpapi.MustNewPOSTJSONWithJSONResponseDescriptor(c.Logger, "/", creq).WithBodyLogging(true), + httpapi.NewEndpointList(c.Session.DefaultHTTPClient(), c.Session.UserAgent(), c.TestHelpers...)..., + ) // issue the control request and wait for the response var cresp webconnectivity.ControlResponse - err := clnt.PostJSON(opCtx, "/", creq, &cresp) + idx, err := seqCaller.CallWithJSONResponse(opCtx, &cresp) if err != nil { // make sure error is wrapped err = netxlite.NewTopLevelGenericErrWrapper(err) @@ -134,6 +129,10 @@ func (c *Control) Run(parentCtx context.Context) { c.TestKeys.SetControl(&cresp) ol.Stop(nil) + // record the specific TH that worked + runtimex.Assert(idx >= 0 && idx < len(c.TestHelpers), "idx out of bounds") + c.TestKeys.setTestHelper(&c.TestHelpers[idx]) + // if the TH returned us addresses we did not previously were // aware of, make sure we also measure them c.maybeStartExtraMeasurements(parentCtx, cresp.DNS.Addrs) diff --git a/internal/experiment/webconnectivity/dnsresolvers.go b/internal/experiment/webconnectivity/dnsresolvers.go index 0c9df8d..8709635 100644 --- a/internal/experiment/webconnectivity/dnsresolvers.go +++ b/internal/experiment/webconnectivity/dnsresolvers.go @@ -67,8 +67,9 @@ type DNSResolvers struct { // always follow the redirect chain caused by the provided URL. Session model.ExperimentSession - // THAddr is the OPTIONAL test helper address. - THAddr string + // TestHelpers is the OPTIONAL list of test helpers. If the list is + // empty, we are not going to try to contact any test helper. + TestHelpers []model.OOAPIService // UDPAddress is the OPTIONAL address of the UDP resolver to use. If this // field is not set we use a default one (e.g., `8.8.8.8:53`). @@ -498,15 +499,15 @@ func (t *DNSResolvers) startSecureFlows( } } -// maybeStartControlFlow starts the control flow iff .Session and .THAddr are set. +// maybeStartControlFlow starts the control flow iff .Session and .TestHelpers are set. func (t *DNSResolvers) maybeStartControlFlow( ctx context.Context, ps *prioritySelector, addresses []DNSEntry, ) { - // note: for subsequent requests we don't set .Session and .THAddr hence + // note: for subsequent requests we don't set .Session and .TestHelpers hence // we are not going to query the test helper more than once - if t.Session != nil && t.THAddr != "" { + if t.Session != nil && len(t.TestHelpers) > 0 { var addrs []string for _, addr := range addresses { addrs = append(addrs, addr.Addr) @@ -518,7 +519,7 @@ func (t *DNSResolvers) maybeStartControlFlow( PrioSelector: ps, TestKeys: t.TestKeys, Session: t.Session, - THAddr: t.THAddr, + TestHelpers: t.TestHelpers, URL: t.URL, WaitGroup: t.WaitGroup, } diff --git a/internal/experiment/webconnectivity/measurer.go b/internal/experiment/webconnectivity/measurer.go index c6731e4..5a0b977 100644 --- a/internal/experiment/webconnectivity/measurer.go +++ b/internal/experiment/webconnectivity/measurer.go @@ -36,7 +36,7 @@ func (m *Measurer) ExperimentName() string { // ExperimentVersion implements model.ExperimentMeasurer. func (m *Measurer) ExperimentVersion() string { - return "0.5.18" + return "0.5.19" } // Run implements model.ExperimentMeasurer. @@ -89,17 +89,7 @@ func (m *Measurer) Run(ctx context.Context, sess model.ExperimentSession, // obtain the test helper's address testhelpers, _ := sess.GetTestHelpersByName("web-connectivity") - var thAddr string - for _, th := range testhelpers { - if th.Type == "https" { - thAddr = th.Address - measurement.TestHelpers = map[string]any{ - "backend": &th, - } - break - } - } - if thAddr == "" { + if len(testhelpers) < 1 { sess.Logger().Warnf("continuing without a valid TH address") tk.SetControlFailure(webconnectivity.ErrNoAvailableTestHelpers) } @@ -120,7 +110,7 @@ func (m *Measurer) Run(ctx context.Context, sess model.ExperimentSession, CookieJar: jar, Referer: "", Session: sess, - THAddr: thAddr, + TestHelpers: testhelpers, UDPAddress: "", } resos.Start(ctx) @@ -137,6 +127,16 @@ func (m *Measurer) Run(ctx context.Context, sess model.ExperimentSession, // perform any deferred computation on the test keys tk.Finalize(sess.Logger()) + // set the test helper we used + // TODO(bassosimone): it may be more informative to know about all the + // test helpers we _tried_ to use, however the data format does not have + // support for that as far as I can tell... + if th := tk.getTestHelper(); th != nil { + measurement.TestHelpers = map[string]interface{}{ + "backend": th, + } + } + // return whether there was a fundamental failure, which would prevent // the measurement from being submitted to the OONI collector. return tk.fundamentalFailure diff --git a/internal/experiment/webconnectivity/secureflow.go b/internal/experiment/webconnectivity/secureflow.go index 68f41e1..6a47b73 100644 --- a/internal/experiment/webconnectivity/secureflow.go +++ b/internal/experiment/webconnectivity/secureflow.go @@ -337,7 +337,7 @@ func (t *SecureFlow) maybeFollowRedirects(ctx context.Context, resp *http.Respon WaitGroup: t.WaitGroup, Referer: resp.Request.URL.String(), Session: nil, // no need to issue another control request - THAddr: "", // ditto + TestHelpers: nil, // ditto UDPAddress: t.UDPAddress, } resolvers.Start(ctx) diff --git a/internal/experiment/webconnectivity/testkeys.go b/internal/experiment/webconnectivity/testkeys.go index 37dbc94..5cd917f 100644 --- a/internal/experiment/webconnectivity/testkeys.go +++ b/internal/experiment/webconnectivity/testkeys.go @@ -134,6 +134,10 @@ type TestKeys struct { // mu provides mutual exclusion for accessing the test keys. mu *sync.Mutex + + // testHelper is used to communicate the TH that worked to the main + // goroutine such that we can fill measurement.TestHelpers. + testHelper *model.OOAPIService } // ConnPriorityLogEntry is an entry in the TestKeys.ConnPriorityLog slice. @@ -302,6 +306,21 @@ func (tk *TestKeys) AppendConnPriorityLogEntry(entry *ConnPriorityLogEntry) { tk.mu.Unlock() } +// setTestHelper sets .testHelper in a thread safe way +func (tk *TestKeys) setTestHelper(th *model.OOAPIService) { + tk.mu.Lock() + tk.testHelper = th + tk.mu.Unlock() +} + +// getTestHelper gets .testHelper in a thread safe way +func (tk *TestKeys) getTestHelper() (th *model.OOAPIService) { + tk.mu.Lock() + th = tk.testHelper + tk.mu.Unlock() + return +} + // NewTestKeys creates a new instance of TestKeys. func NewTestKeys() *TestKeys { return &TestKeys{ @@ -348,6 +367,7 @@ func NewTestKeys() *TestKeys { ControlRequest: nil, fundamentalFailure: nil, mu: &sync.Mutex{}, + testHelper: nil, } } diff --git a/internal/httpapi/call.go b/internal/httpapi/call.go new file mode 100644 index 0000000..1228080 --- /dev/null +++ b/internal/httpapi/call.go @@ -0,0 +1,181 @@ +package httpapi + +// +// Calling HTTP APIs. +// + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + + "github.com/ooni/probe-cli/v3/internal/netxlite" +) + +// joinURLPath appends |resourcePath| to |urlPath|. +func joinURLPath(urlPath, resourcePath string) string { + if resourcePath == "" { + if urlPath == "" { + return "/" + } + return urlPath + } + if !strings.HasSuffix(urlPath, "/") { + urlPath += "/" + } + resourcePath = strings.TrimPrefix(resourcePath, "/") + return urlPath + resourcePath +} + +// newRequest creates a new http.Request from the given |ctx|, |endpoint|, and |desc|. +func newRequest(ctx context.Context, endpoint *Endpoint, desc *Descriptor) (*http.Request, error) { + URL, err := url.Parse(endpoint.BaseURL) + if err != nil { + return nil, err + } + // BaseURL and resource URL are joined if they have a path + URL.Path = joinURLPath(URL.Path, desc.URLPath) + if len(desc.URLQuery) > 0 { + URL.RawQuery = desc.URLQuery.Encode() + } else { + URL.RawQuery = "" // as documented we only honour desc.URLQuery + } + var reqBody io.Reader + if len(desc.RequestBody) > 0 { + reqBody = bytes.NewReader(desc.RequestBody) + desc.Logger.Debugf("httpapi: request body length: %d", len(desc.RequestBody)) + if desc.LogBody { + desc.Logger.Debugf("httpapi: request body: %s", string(desc.RequestBody)) + } + } + request, err := http.NewRequestWithContext(ctx, desc.Method, URL.String(), reqBody) + if err != nil { + return nil, err + } + request.Host = endpoint.Host // allow cloudfronting + if desc.Authorization != "" { + request.Header.Set("Authorization", desc.Authorization) + } + if desc.ContentType != "" { + request.Header.Set("Content-Type", desc.ContentType) + } + if desc.Accept != "" { + request.Header.Set("Accept", desc.Accept) + } + if endpoint.UserAgent != "" { + request.Header.Set("User-Agent", endpoint.UserAgent) + } + return request, nil +} + +// ErrHTTPRequestFailed indicates that the server returned >= 400. +type ErrHTTPRequestFailed struct { + // StatusCode is the status code that failed. + StatusCode int +} + +// Error implements error. +func (err *ErrHTTPRequestFailed) Error() string { + return fmt.Sprintf("httpapi: http request failed: %d", err.StatusCode) +} + +// errMaybeCensorship indicates that there was an error at the networking layer +// including, e.g., DNS, TCP connect, TLS. When we see this kind of error, we +// will consider retrying with another endpoint under the assumption that it +// may be that the current endpoint is censored. +type errMaybeCensorship struct { + // Err is the underlying error + Err error +} + +// Error implements error +func (err *errMaybeCensorship) Error() string { + return err.Err.Error() +} + +// Unwrap allows to get the underlying error +func (err *errMaybeCensorship) Unwrap() error { + return err.Err +} + +// docall calls the API represented by the given request |req| on the given |endpoint| +// and returns the response and its body or an error. +func docall(endpoint *Endpoint, desc *Descriptor, request *http.Request) (*http.Response, []byte, error) { + // Implementation note: remember to mark errors for which you want + // to retry with another endpoint using errMaybeCensorship. + response, err := endpoint.HTTPClient.Do(request) + if err != nil { + return nil, nil, &errMaybeCensorship{err} + } + defer response.Body.Close() + // Implementation note: always read and log the response body since + // it's quite useful to see the response JSON on API error. + r := io.LimitReader(response.Body, DefaultMaxBodySize) + data, err := netxlite.ReadAllContext(request.Context(), r) + if err != nil { + return response, nil, &errMaybeCensorship{err} + } + desc.Logger.Debugf("httpapi: response body length: %d bytes", len(data)) + if desc.LogBody { + desc.Logger.Debugf("httpapi: response body: %s", string(data)) + } + if response.StatusCode >= 400 { + return response, nil, &ErrHTTPRequestFailed{response.StatusCode} + } + return response, data, nil +} + +// call is like Call but also returns the response. +func call(ctx context.Context, desc *Descriptor, endpoint *Endpoint) (*http.Response, []byte, error) { + timeout := desc.Timeout + if timeout <= 0 { + timeout = DefaultCallTimeout // as documented + } + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + request, err := newRequest(ctx, endpoint, desc) + if err != nil { + return nil, nil, err + } + return docall(endpoint, desc, request) +} + +// Call invokes the API described by |desc| on the given HTTP |endpoint| and +// returns the response body (as a slice of bytes) or an error. +// +// Note: this function returns ErrHTTPRequestFailed if the HTTP status code is +// greater or equal than 400. You could use errors.As to obtain a copy of the +// error that was returned and see for yourself the actual status code. +func Call(ctx context.Context, desc *Descriptor, endpoint *Endpoint) ([]byte, error) { + _, rawResponseBody, err := call(ctx, desc, endpoint) + return rawResponseBody, err +} + +// goodContentTypeForJSON tracks known-good content-types for JSON. If the content-type +// is not in this map, |CallWithJSONResponse| emits a warning message. +var goodContentTypeForJSON = map[string]bool{ + applicationJSON: true, +} + +// CallWithJSONResponse is like Call but also assumes that the response is a +// JSON body and attempts to parse it into the |response| field. +// +// Note: this function returns ErrHTTPRequestFailed if the HTTP status code is +// greater or equal than 400. You could use errors.As to obtain a copy of the +// error that was returned and see for yourself the actual status code. +func CallWithJSONResponse(ctx context.Context, desc *Descriptor, endpoint *Endpoint, response any) error { + httpResp, rawRespBody, err := call(ctx, desc, endpoint) + if err != nil { + return err + } + if ctype := httpResp.Header.Get("Content-Type"); !goodContentTypeForJSON[ctype] { + desc.Logger.Warnf("httpapi: unexpected content-type: %s", ctype) + // fallthrough + } + return json.Unmarshal(rawRespBody, response) +} diff --git a/internal/httpapi/call_test.go b/internal/httpapi/call_test.go new file mode 100644 index 0000000..8fd45d7 --- /dev/null +++ b/internal/httpapi/call_test.go @@ -0,0 +1,1163 @@ +package httpapi + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "syscall" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/model/mocks" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +func Test_joinURLPath(t *testing.T) { + tests := []struct { + name string + urlPath string + resourcePath string + want string + }{{ + name: "whole path inside urlPath and empty resourcePath", + urlPath: "/robots.txt", + resourcePath: "", + want: "/robots.txt", + }, { + name: "empty urlPath and slash-prefixed resourcePath", + urlPath: "", + resourcePath: "/foo", + want: "/foo", + }, { + name: "slash urlPath and slash-prefixed resourcePath", + urlPath: "/", + resourcePath: "/foo", + want: "/foo", + }, { + name: "empty urlPath and empty resourcePath", + urlPath: "", + resourcePath: "", + want: "/", + }, { + name: "non-slash-terminated urlPath and slash-prefixed resourcePath", + urlPath: "/foo", + resourcePath: "/bar", + want: "/foo/bar", + }, { + name: "slash-terminated urlPath and slash-prefixed resourcePath", + urlPath: "/foo/", + resourcePath: "/bar", + want: "/foo/bar", + }, { + name: "slash-terminated urlPath and non-slash-prefixed resourcePath", + urlPath: "/foo", + resourcePath: "bar", + want: "/foo/bar", + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := joinURLPath(tt.urlPath, tt.resourcePath) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Fatal(diff) + } + }) + } +} + +func Test_newRequest(t *testing.T) { + type args struct { + ctx context.Context + endpoint *Endpoint + desc *Descriptor + } + tests := []struct { + name string + args args + wantFn func(*testing.T, *http.Request) + wantErr error + }{{ + name: "url.Parse fails", + args: args{ + ctx: nil, + endpoint: &Endpoint{ + BaseURL: "\t\t\t", // does not parse! + HTTPClient: nil, + Host: "", + UserAgent: "", + }, + desc: &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: nil, + MaxBodySize: 0, + Method: "", + RequestBody: nil, + Timeout: 0, + URLPath: "", + URLQuery: nil, + }, + }, + wantFn: nil, + wantErr: errors.New(`parse "\t\t\t": net/url: invalid control character in URL`), + }, { + name: "http.NewRequestWithContext fails", + args: args{ + ctx: nil, // causes http.NewRequestWithContext to fail + endpoint: &Endpoint{ + BaseURL: "https://example.com/", + HTTPClient: nil, + Host: "", + UserAgent: "", + }, + desc: &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: nil, + MaxBodySize: 0, + Method: "", + RequestBody: nil, + Timeout: 0, + URLPath: "", + URLQuery: nil, + }, + }, + wantFn: nil, + wantErr: errors.New("net/http: nil Context"), + }, { + name: "successful case with GET method, no body, and no extra headers", + args: args{ + ctx: context.Background(), + endpoint: &Endpoint{ + BaseURL: "https://example.com/", + HTTPClient: nil, + Host: "", + UserAgent: "", + }, + desc: &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: nil, + MaxBodySize: 0, + Method: http.MethodGet, + RequestBody: nil, + Timeout: 0, + URLPath: "", + URLQuery: nil, + }, + }, + wantFn: func(t *testing.T, req *http.Request) { + if req == nil { + t.Fatal("expected non-nil request") + } + if req.Method != http.MethodGet { + t.Fatal("invalid method") + } + if req.URL.String() != "https://example.com/" { + t.Fatal("invalid URL") + } + if req.Body != nil { + t.Fatal("invalid body", req.Body) + } + }, + wantErr: nil, + }, { + name: "successful case with POST method and body", + args: args{ + ctx: context.Background(), + endpoint: &Endpoint{ + BaseURL: "https://example.com/", + HTTPClient: nil, + Host: "", + UserAgent: "", + }, + desc: &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: model.DiscardLogger, + MaxBodySize: 0, + Method: http.MethodPost, + RequestBody: []byte("deadbeef"), + Timeout: 0, + URLPath: "", + URLQuery: nil, + }, + }, + wantFn: func(t *testing.T, req *http.Request) { + if req == nil { + t.Fatal("expected non-nil request") + } + if req.Method != http.MethodPost { + t.Fatal("invalid method") + } + if req.URL.String() != "https://example.com/" { + t.Fatal("invalid URL") + } + data, err := netxlite.ReadAllContext(context.Background(), req.Body) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff([]byte("deadbeef"), data); diff != "" { + t.Fatal(diff) + } + }, + wantErr: nil, + }, { + name: "with GET method and custom headers", + args: args{ + ctx: context.Background(), + endpoint: &Endpoint{ + BaseURL: "https://example.com/", + HTTPClient: nil, + Host: "antani.org", + UserAgent: "httpclient/1.0.1", + }, + desc: &Descriptor{ + Accept: "application/json", + Authorization: "deafbeef", + ContentType: "text/plain", + LogBody: false, + Logger: nil, + MaxBodySize: 0, + Method: http.MethodPut, + RequestBody: nil, + Timeout: 0, + URLPath: "", + URLQuery: nil, + }, + }, + wantFn: func(t *testing.T, req *http.Request) { + if req == nil { + t.Fatal("expected non-nil request") + } + if req.Method != http.MethodPut { + t.Fatal("invalid method") + } + if req.Host != "antani.org" { + t.Fatal("invalid request host") + } + if req.URL.String() != "https://example.com/" { + t.Fatal("invalid URL") + } + if req.Header.Get("Authorization") != "deafbeef" { + t.Fatal("invalid authorization") + } + if req.Header.Get("Content-Type") != "text/plain" { + t.Fatal("invalid content-type") + } + if req.Header.Get("Accept") != "application/json" { + t.Fatal("invalid accept") + } + if req.Header.Get("User-Agent") != "httpclient/1.0.1" { + t.Fatal("invalid user-agent") + } + }, + wantErr: nil, + }, { + name: "we join the urlPath with the resourcePath", + args: args{ + ctx: context.Background(), + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/api/v1", + HTTPClient: nil, + Host: "", + UserAgent: "", + }, + desc: &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: nil, + MaxBodySize: 0, + Method: http.MethodGet, + RequestBody: nil, + Timeout: 0, + URLPath: "/test-list/urls", + URLQuery: nil, + }, + }, + wantFn: func(t *testing.T, req *http.Request) { + if req == nil { + t.Fatal("expected non-nil request") + } + if req.Method != http.MethodGet { + t.Fatal("invalid method") + } + if req.URL.String() != "https://www.example.com/api/v1/test-list/urls" { + t.Fatal("invalid URL") + } + }, + wantErr: nil, + }, { + name: "we discard any query element inside the Endpoint.BaseURL", + args: args{ + ctx: context.Background(), + endpoint: &Endpoint{ + BaseURL: "https://example.org/api/v1/?probe_cc=IT", + HTTPClient: nil, + Host: "", + UserAgent: "", + }, + desc: &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: nil, + MaxBodySize: 0, + Method: http.MethodGet, + RequestBody: nil, + Timeout: 0, + URLPath: "", + URLQuery: nil, + }, + }, + wantFn: func(t *testing.T, req *http.Request) { + if req == nil { + t.Fatal("expected non-nil request") + } + if req.Method != http.MethodGet { + t.Fatal("invalid method") + } + if req.URL.String() != "https://example.org/api/v1/" { + t.Fatal("invalid URL") + } + }, + wantErr: nil, + }, { + name: "we include query elements from Descriptor.URLQuery", + args: args{ + ctx: context.Background(), + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/api/v1/", + HTTPClient: nil, + Host: "", + UserAgent: "", + }, + desc: &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: nil, + MaxBodySize: 0, + Method: http.MethodGet, + RequestBody: nil, + Timeout: 0, + URLPath: "test-list/urls", + URLQuery: map[string][]string{ + "probe_cc": {"IT"}, + }, + }, + }, + wantFn: func(t *testing.T, req *http.Request) { + if req == nil { + t.Fatal("expected non-nil request") + } + if req.Method != http.MethodGet { + t.Fatal("invalid method") + } + if req.URL.String() != "https://www.example.com/api/v1/test-list/urls?probe_cc=IT" { + t.Fatal("invalid URL") + } + }, + wantErr: nil, + }, { + name: "with as many implicitly-initialized fields as possible", + args: args{ + ctx: context.Background(), + endpoint: &Endpoint{ + BaseURL: "https://example.com/", + }, + desc: &Descriptor{}, + }, + wantFn: func(t *testing.T, req *http.Request) { + if req == nil { + t.Fatal("expected non-nil request") + } + if req.Method != http.MethodGet { + t.Fatal("invalid method") + } + if req.URL.String() != "https://example.com/" { + t.Fatal("invalid URL") + } + }, + wantErr: nil, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := newRequest(tt.args.ctx, tt.args.endpoint, tt.args.desc) + switch { + case err == nil && tt.wantErr == nil: + // nothing + case err != nil && tt.wantErr == nil: + t.Fatalf("expected error but got %s", err.Error()) + case err == nil && tt.wantErr != nil: + t.Fatalf("expected %s but got ", tt.wantErr.Error()) + case err.Error() == tt.wantErr.Error(): + // nothing + default: + t.Fatalf("expected %s but got %s", err.Error(), tt.wantErr.Error()) + } + if tt.wantFn != nil { + tt.wantFn(t, got) + return + } + if got != nil { + t.Fatal("got response with nil tt.wantFn") + } + }) + } +} + +func TestCall(t *testing.T) { + type args struct { + ctx context.Context + desc *Descriptor + endpoint *Endpoint + } + tests := []struct { + name string + args args + want []byte + wantErr error + errfn func(t *testing.T, err error) + }{{ + name: "newRequest fails", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: nil, + MaxBodySize: 0, + Method: "", + RequestBody: nil, + Timeout: 0, + URLPath: "", + URLQuery: nil, + }, + endpoint: &Endpoint{ + BaseURL: "\t\t\t", // causes newRequest to fail + HTTPClient: nil, + Host: "", + UserAgent: "", + }, + }, + want: nil, + wantErr: errors.New(`parse "\t\t\t": net/url: invalid control character in URL`), + errfn: nil, + }, { + name: "endpoint.HTTPClient.Do fails", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + }, + endpoint: &Endpoint{ + BaseURL: "https://example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + }, + }, + want: nil, + wantErr: io.EOF, + errfn: func(t *testing.T, err error) { + var expect *errMaybeCensorship + if !errors.As(err, &expect) { + t.Fatal("unexpected error type") + } + }, + }, { + name: "reading body fails", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + }, + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Body: io.NopCloser(&mocks.Reader{ + MockRead: func(b []byte) (int, error) { + return 0, netxlite.ECONNRESET + }, + }), + } + return resp, nil + }, + }, + }, + }, + want: nil, + wantErr: errors.New(netxlite.FailureConnectionReset), + errfn: func(t *testing.T, err error) { + var expect *errMaybeCensorship + if !errors.As(err, &expect) { + t.Fatal("unexpected error type") + } + }, + }, { + name: "status code indicates failure", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + }, + endpoint: &Endpoint{ + BaseURL: "https://example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader("deadbeef")), + StatusCode: 403, + } + return resp, nil + }, + }, + }, + }, + want: nil, + wantErr: errors.New("httpapi: http request failed: 403"), + errfn: func(t *testing.T, err error) { + var expect *ErrHTTPRequestFailed + if !errors.As(err, &expect) { + t.Fatal("invalid error type") + } + }, + }, { + name: "success with log body flag", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + LogBody: true, // as documented by this test's name + Logger: model.DiscardLogger, + Method: http.MethodGet, + }, + endpoint: &Endpoint{ + BaseURL: "https://example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader("deadbeef")), + StatusCode: 200, + } + return resp, nil + }, + }, + }, + }, + want: []byte("deadbeef"), + wantErr: nil, + errfn: nil, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Call(tt.args.ctx, tt.args.desc, tt.args.endpoint) + switch { + case err == nil && tt.wantErr == nil: + // nothing + case err != nil && tt.wantErr == nil: + t.Fatalf("expected error but got %s", err.Error()) + case err == nil && tt.wantErr != nil: + t.Fatalf("expected %s but got ", tt.wantErr.Error()) + case err.Error() == tt.wantErr.Error(): + // nothing + default: + t.Fatalf("expected %s but got %s", err.Error(), tt.wantErr.Error()) + } + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Fatal(diff) + } + }) + } +} + +func TestCallWithJSONResponse(t *testing.T) { + type response struct { + Name string + Age int64 + } + expectedResponse := response{ + Name: "sbs", + Age: 99, + } + type args struct { + ctx context.Context + desc *Descriptor + endpoint *Endpoint + } + tests := []struct { + name string + args args + wantErr error + errfn func(*testing.T, error) + }{{ + name: "call fails", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + }, + endpoint: &Endpoint{ + BaseURL: "\t\t\t\t", // causes failure + }, + }, + wantErr: errors.New(`parse "\t\t\t\t": net/url: invalid control character in URL`), + errfn: nil, + }, { + name: "with error during httpClient.Do", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + }, + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/a", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + }, + }, + wantErr: io.EOF, + errfn: func(t *testing.T, err error) { + var expect *errMaybeCensorship + if !errors.As(err, &expect) { + t.Fatal("invalid error type") + } + }, + }, { + name: "with error when reading the response body", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + }, + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/a", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Body: io.NopCloser(&mocks.Reader{ + MockRead: func(b []byte) (int, error) { + return 0, netxlite.ECONNRESET + }, + }), + StatusCode: 200, + } + return resp, nil + }, + }, + }, + }, + wantErr: errors.New(netxlite.FailureConnectionReset), + errfn: func(t *testing.T, err error) { + var expect *errMaybeCensorship + if !errors.As(err, &expect) { + t.Fatal("invalid error type") + } + }, + }, { + name: "with HTTP failure", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + }, + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/a", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(`{"Name": "sbs", "Age": 99}`)), + StatusCode: 400, + } + return resp, nil + }, + }, + }, + }, + wantErr: errors.New("httpapi: http request failed: 400"), + errfn: func(t *testing.T, err error) { + var expect *ErrHTTPRequestFailed + if !errors.As(err, &expect) { + t.Fatal("invalid error type") + } + }, + }, { + name: "with good response and missing header", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + }, + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/a", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(`{"Name": "sbs", "Age": 99}`)), + StatusCode: 200, + } + return resp, nil + }, + }, + }, + }, + wantErr: nil, + errfn: nil, + }, { + name: "with good response and good header", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + Logger: model.DiscardLogger, + }, + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/a", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Header: http.Header{ + "Content-Type": {"application/json"}, + }, + Body: io.NopCloser(strings.NewReader(`{"Name": "sbs", "Age": 99}`)), + StatusCode: 200, + } + return resp, nil + }, + }, + }, + }, + wantErr: nil, + errfn: nil, + }, { + name: "response is not JSON", + args: args{ + ctx: context.Background(), + desc: &Descriptor{ + LogBody: false, + Logger: model.DiscardLogger, + Method: http.MethodGet, + }, + endpoint: &Endpoint{ + BaseURL: "https://www.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Header: http.Header{ + "Content-Type": {"application/json"}, + }, + Body: io.NopCloser(strings.NewReader(`{`)), // invalid JSON + StatusCode: 200, + } + return resp, nil + }, + }, + }, + }, + wantErr: errors.New("unexpected end of JSON input"), + errfn: nil, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var response response + err := CallWithJSONResponse(tt.args.ctx, tt.args.desc, tt.args.endpoint, &response) + switch { + case err == nil && tt.wantErr == nil: + if diff := cmp.Diff(expectedResponse, response); err != nil { + t.Fatal(diff) + } + case err != nil && tt.wantErr == nil: + t.Fatalf("expected error but got %s", err.Error()) + case err == nil && tt.wantErr != nil: + t.Fatalf("expected %s but got ", tt.wantErr.Error()) + case err.Error() == tt.wantErr.Error(): + // nothing + default: + t.Fatalf("expected %s but got %s", err.Error(), tt.wantErr.Error()) + } + if tt.errfn != nil { + tt.errfn(t, err) + } + }) + } +} + +func TestCallHonoursContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // should fail HTTP request immediately + desc := &Descriptor{ + LogBody: false, + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/robots.txt", + } + endpoint := &Endpoint{ + BaseURL: "https://www.example.com/", + HTTPClient: http.DefaultClient, + UserAgent: model.HTTPHeaderUserAgent, + } + body, err := Call(ctx, desc, endpoint) + if !errors.Is(err, context.Canceled) { + t.Fatal("unexpected err", err) + } + if len(body) > 0 { + t.Fatal("expected zero-length body") + } +} + +func TestCallWithJSONResponseHonoursContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // should fail HTTP request immediately + desc := &Descriptor{ + LogBody: false, + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/robots.txt", + } + endpoint := &Endpoint{ + BaseURL: "https://www.example.com/", + HTTPClient: http.DefaultClient, + UserAgent: model.HTTPHeaderUserAgent, + } + var resp url.URL + err := CallWithJSONResponse(ctx, desc, endpoint, &resp) + if !errors.Is(err, context.Canceled) { + t.Fatal("unexpected err", err) + } +} + +func TestDescriptorLogging(t *testing.T) { + + // This test was originally written for the httpx package and we have adapted it + // by keeping the ~same implementation with a custom callx function that converts + // the previous semantics of httpx to the new semantics of httpapi. + callx := func(baseURL string, logBody bool, logger model.Logger, request, response any) error { + desc := MustNewPOSTJSONWithJSONResponseDescriptor(logger, "/", request).WithBodyLogging(logBody) + runtimex.Assert(desc.LogBody == logBody, "desc.LogBody should be equal to logBody here") + endpoint := &Endpoint{ + BaseURL: baseURL, + HTTPClient: http.DefaultClient, + } + return CallWithJSONResponse(context.Background(), desc, endpoint, response) + } + + // we also needed to create a constructor for the logger + newlogger := func(logs chan string) model.Logger { + return &mocks.Logger{ + MockDebugf: func(format string, v ...interface{}) { + logs <- fmt.Sprintf(format, v...) + }, + MockWarnf: func(format string, v ...interface{}) { + logs <- fmt.Sprintf(format, v...) + }, + } + } + + t.Run("body logging enabled, 200 Ok, and without content-type", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("[]")) + }, + )) + logs := make(chan string, 1024) + defer server.Close() + var ( + input []string + output []string + ) + logger := newlogger(logs) + err := callx(server.URL, true, logger, input, &output) + var found int + close(logs) + for entry := range logs { + if strings.HasPrefix(entry, "httpapi: request body: ") { + // we expect this because body logging is enabled + found |= 1 << 0 + continue + } + if strings.HasPrefix(entry, "httpapi: response body: ") { + // we expect this because body logging is enabled + found |= 1 << 1 + continue + } + if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { + // we would expect this because the server does not send us any content-type + found |= 1 << 2 + continue + } + if strings.HasPrefix(entry, "httpapi: request body length: ") { + // we should see this because we sent a body + found |= 1 << 3 + continue + } + if strings.HasPrefix(entry, "httpapi: response body length: ") { + // we should see this because we receive a body + found |= 1 << 4 + continue + } + } + if found != (1<<0 | 1<<1 | 1<<2 | 1<<3 | 1<<4) { + t.Fatal("did not find the expected logs") + } + if err != nil { + t.Fatal(err) + } + }) + + t.Run("body logging enabled, 200 Ok, and with content-type", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("content-type", "application/json") + w.Write([]byte("[]")) + }, + )) + logs := make(chan string, 1024) + defer server.Close() + var ( + input []string + output []string + ) + logger := newlogger(logs) + err := callx(server.URL, true, logger, input, &output) + var found int + close(logs) + for entry := range logs { + if strings.HasPrefix(entry, "httpapi: request body: ") { + // we expect this because body logging is enabled + found |= 1 << 0 + continue + } + if strings.HasPrefix(entry, "httpapi: response body: ") { + // we expect this because body logging is enabled + found |= 1 << 1 + continue + } + if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { + // we do not expect this because the server sends us a content-type + found |= 1 << 2 + continue + } + if strings.HasPrefix(entry, "httpapi: request body length: ") { + // we should see this because we sent a body + found |= 1 << 3 + continue + } + if strings.HasPrefix(entry, "httpapi: response body length: ") { + // we should see this because we receive a body + found |= 1 << 4 + continue + } + } + if found != (1<<0 | 1<<1 | 1<<3 | 1<<4) { + t.Fatal("did not find the expected logs") + } + if err != nil { + t.Fatal(err) + } + }) + + t.Run("body logging enabled and 401 Unauthorized", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(401) + w.Write([]byte("[]")) + }, + )) + logs := make(chan string, 1024) + defer server.Close() + var ( + input []string + output []string + ) + logger := newlogger(logs) + err := callx(server.URL, true, logger, input, &output) + var found int + close(logs) + for entry := range logs { + if strings.HasPrefix(entry, "httpapi: request body: ") { + // should occur because body logging is enabled + found |= 1 << 0 + continue + } + if strings.HasPrefix(entry, "httpapi: response body: ") { + // should occur because body logging is enabled + found |= 1 << 1 + continue + } + if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { + // note: this one should not occur because the code is 401 so we're not + // actually going to parse the JSON document + found |= 1 << 2 + continue + } + if strings.HasPrefix(entry, "httpapi: request body length: ") { + // we should see this because we send a body + found |= 1 << 3 + continue + } + if strings.HasPrefix(entry, "httpapi: response body length: ") { + // we should see this because we receive a body + found |= 1 << 4 + continue + } + } + if found != (1<<0 | 1<<1 | 1<<3 | 1<<4) { + t.Fatal("did not find the expected logs") + } + var failure *ErrHTTPRequestFailed + if !errors.As(err, &failure) || failure.StatusCode != 401 { + t.Fatal("unexpected err", err) + } + }) + + t.Run("body logging NOT enabled and 200 Ok", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("[]")) + }, + )) + logs := make(chan string, 1024) + defer server.Close() + var ( + input []string + output []string + ) + logger := newlogger(logs) + err := callx(server.URL, false, logger, input, &output) // no logging + var found int + close(logs) + for entry := range logs { + if strings.HasPrefix(entry, "httpapi: request body: ") { + // should not see it: body logging is disabled + found |= 1 << 0 + continue + } + if strings.HasPrefix(entry, "httpapi: response body: ") { + // should not see it: body logging is disabled + found |= 1 << 1 + continue + } + if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { + // this one should be logged ANYWAY because it's orthogonal to the + // body logging so we should see it also in this case. + found |= 1 << 2 + continue + } + if strings.HasPrefix(entry, "httpapi: request body length: ") { + // should see this because we send a body + found |= 1 << 3 + continue + } + if strings.HasPrefix(entry, "httpapi: response body length: ") { + // should see this because we're receiving a body + found |= 1 << 4 + continue + } + } + if found != (1<<2 | 1<<3 | 1<<4) { + t.Fatal("did not find the expected logs") + } + if err != nil { + t.Fatal(err) + } + }) + + t.Run("body logging NOT enabled and 401 Unauthorized", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(401) + w.Write([]byte("[]")) + }, + )) + logs := make(chan string, 1024) + defer server.Close() + var ( + input []string + output []string + ) + logger := newlogger(logs) + err := callx(server.URL, false, logger, input, &output) // no logging + var found int + close(logs) + for entry := range logs { + if strings.HasPrefix(entry, "httpapi: request body: ") { + // should not see it: body logging is disabled + found |= 1 << 0 + continue + } + if strings.HasPrefix(entry, "httpapi: response body: ") { + // should not see it: body logging is disabled + found |= 1 << 1 + continue + } + if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { + // should not see it because we don't parse the body on 401 errors + found |= 1 << 2 + continue + } + if strings.HasPrefix(entry, "httpapi: request body length: ") { + // we send a body so we should see it + found |= 1 << 3 + continue + } + if strings.HasPrefix(entry, "httpapi: response body length: ") { + // we receive a body so we should see it + found |= 1 << 4 + continue + } + } + if found != (1<<3 | 1<<4) { + t.Fatal("did not find the expected logs") + } + var failure *ErrHTTPRequestFailed + if !errors.As(err, &failure) || failure.StatusCode != 401 { + t.Fatal("unexpected err", err) + } + }) +} + +func Test_errMaybeCensorship_Unwrap(t *testing.T) { + t.Run("for errors.Is", func(t *testing.T) { + var err error = &errMaybeCensorship{io.EOF} + if !errors.Is(err, io.EOF) { + t.Fatal("cannot unwrap") + } + }) + + t.Run("for errors.As", func(t *testing.T) { + var err error = &errMaybeCensorship{netxlite.ECONNRESET} + var syserr syscall.Errno + if !errors.As(err, &syserr) || syserr != netxlite.ECONNRESET { + t.Fatal("cannot unwrap") + } + }) +} diff --git a/internal/httpapi/descriptor.go b/internal/httpapi/descriptor.go new file mode 100644 index 0000000..ed35e85 --- /dev/null +++ b/internal/httpapi/descriptor.go @@ -0,0 +1,155 @@ +package httpapi + +// +// HTTP API descriptor (e.g., GET /api/v1/test-list/urls) +// + +import ( + "encoding/json" + "net/http" + "net/url" + "time" + + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +// Descriptor contains the parameters for calling a given HTTP +// API (e.g., GET /api/v1/test-list/urls). +// +// The zero value of this struct is invalid. Please, fill all the +// fields marked as MANDATORY for correct initialization. +type Descriptor struct { + // Accept contains the OPTIONAL accept header. + Accept string + + // Authorization is the OPTIONAL authorization. + Authorization string + + // ContentType is the OPTIONAL content-type header. + ContentType string + + // LogBody OPTIONALLY enables logging bodies. + LogBody bool + + // Logger is the MANDATORY logger to use. + // + // For example, model.DiscardLogger. + Logger model.Logger + + // MaxBodySize is the OPTIONAL maximum response body size. If + // not set, we use the |DefaultMaxBodySize| constant. + MaxBodySize int64 + + // Method is the MANDATORY request method. + Method string + + // RequestBody is the OPTIONAL request body. + RequestBody []byte + + // Timeout is the OPTIONAL timeout for this call. If no timeout + // is specified we will use the |DefaultCallTimeout| const. + Timeout time.Duration + + // URLPath is the MANDATORY URL path. + URLPath string + + // URLQuery is the OPTIONAL query. + URLQuery url.Values +} + +// WithBodyLogging returns a SHALLOW COPY of |Descriptor| with LogBody set to |value|. You SHOULD +// only use this method when initializing the descriptor you want to use. +func (desc *Descriptor) WithBodyLogging(value bool) *Descriptor { + out := &Descriptor{} + *out = *desc + out.LogBody = value + return out +} + +// DefaultMaxBodySize is the default value for the maximum +// body size you can fetch using the httpapi package. +const DefaultMaxBodySize = 1 << 22 + +// DefaultCallTimeout is the default timeout for an httpapi call. +const DefaultCallTimeout = 60 * time.Second + +// NewGETJSONDescriptor is a convenience factory for creating a new descriptor +// that uses the GET method and expects a JSON response. +func NewGETJSONDescriptor(logger model.Logger, urlPath string) *Descriptor { + return NewGETJSONWithQueryDescriptor(logger, urlPath, url.Values{}) +} + +// applicationJSON is the content-type for JSON +const applicationJSON = "application/json" + +// NewGETJSONWithQueryDescriptor is like NewGETJSONDescriptor but it also +// allows you to provide |query| arguments. Leaving |query| nil or empty +// is equivalent to calling NewGETJSONDescriptor directly. +func NewGETJSONWithQueryDescriptor(logger model.Logger, urlPath string, query url.Values) *Descriptor { + return &Descriptor{ + Accept: applicationJSON, + Authorization: "", + ContentType: "", + LogBody: false, + Logger: logger, + MaxBodySize: DefaultMaxBodySize, + Method: http.MethodGet, + RequestBody: nil, + Timeout: DefaultCallTimeout, + URLPath: urlPath, + URLQuery: query, + } +} + +// NewPOSTJSONWithJSONResponseDescriptor creates a descriptor that POSTs a JSON document +// and expects to receive back a JSON document from the API. +// +// This function ONLY fails if we cannot serialize the |request| to JSON. So, if you know +// that |request| is JSON-serializable, you can safely call MustNewPostJSONWithJSONResponseDescriptor instead. +func NewPOSTJSONWithJSONResponseDescriptor(logger model.Logger, urlPath string, request any) (*Descriptor, error) { + rawRequest, err := json.Marshal(request) + if err != nil { + return nil, err + } + desc := &Descriptor{ + Accept: applicationJSON, + Authorization: "", + ContentType: applicationJSON, + LogBody: false, + Logger: logger, + MaxBodySize: DefaultMaxBodySize, + Method: http.MethodPost, + RequestBody: rawRequest, + Timeout: DefaultCallTimeout, + URLPath: urlPath, + URLQuery: nil, + } + return desc, nil +} + +// MustNewPOSTJSONWithJSONResponseDescriptor is like NewPOSTJSONWithJSONResponseDescriptor except that +// it panics in case it's not possible to JSON serialize the |request|. +func MustNewPOSTJSONWithJSONResponseDescriptor(logger model.Logger, urlPath string, request any) *Descriptor { + desc, err := NewPOSTJSONWithJSONResponseDescriptor(logger, urlPath, request) + runtimex.PanicOnError(err, "NewPOSTJSONWithJSONResponseDescriptor failed") + return desc +} + +// NewGETResourceDescriptor creates a generic descriptor for GETting a +// resource of unspecified type using the given |urlPath|. +func NewGETResourceDescriptor(logger model.Logger, urlPath string) *Descriptor { + return &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: logger, + MaxBodySize: DefaultMaxBodySize, + Method: http.MethodGet, + RequestBody: nil, + Timeout: DefaultCallTimeout, + URLPath: urlPath, + URLQuery: url.Values{}, + } +} diff --git a/internal/httpapi/descriptor_test.go b/internal/httpapi/descriptor_test.go new file mode 100644 index 0000000..c3c58c0 --- /dev/null +++ b/internal/httpapi/descriptor_test.go @@ -0,0 +1,248 @@ +package httpapi + +import ( + "log" + "net/http" + "net/url" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" +) + +func TestDescriptor_WithBodyLogging(t *testing.T) { + type fields struct { + Accept string + Authorization string + ContentType string + LogBody bool + Logger model.Logger + MaxBodySize int64 + Method string + RequestBody []byte + Timeout time.Duration + URLPath string + URLQuery url.Values + } + tests := []struct { + name string + fields fields + want *Descriptor + }{{ + name: "with empty fields", + fields: fields{}, // LogBody defaults to false + want: &Descriptor{ + LogBody: true, + }, + }, { + name: "with nonempty fields", + fields: fields{ + Accept: "xx", + Authorization: "y", + ContentType: "zzz", + LogBody: false, // obviously must be false + Logger: model.DiscardLogger, + MaxBodySize: 123, + Method: "POST", + RequestBody: []byte("123"), + Timeout: 15555, + URLPath: "/", + URLQuery: map[string][]string{ + "a": {"b"}, + }, + }, + want: &Descriptor{ + Accept: "xx", + Authorization: "y", + ContentType: "zzz", + LogBody: true, + Logger: model.DiscardLogger, + MaxBodySize: 123, + Method: "POST", + RequestBody: []byte("123"), + Timeout: 15555, + URLPath: "/", + URLQuery: map[string][]string{ + "a": {"b"}, + }, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + desc := &Descriptor{ + Accept: tt.fields.Accept, + Authorization: tt.fields.Authorization, + ContentType: tt.fields.ContentType, + LogBody: tt.fields.LogBody, + Logger: tt.fields.Logger, + MaxBodySize: tt.fields.MaxBodySize, + Method: tt.fields.Method, + RequestBody: tt.fields.RequestBody, + Timeout: tt.fields.Timeout, + URLPath: tt.fields.URLPath, + URLQuery: tt.fields.URLQuery, + } + got := desc.WithBodyLogging(true) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Fatal(diff) + } + }) + } +} + +func TestNewGetJSONDescriptor(t *testing.T) { + expected := &Descriptor{ + Accept: "application/json", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: model.DiscardLogger, + MaxBodySize: DefaultMaxBodySize, + Method: http.MethodGet, + RequestBody: nil, + Timeout: DefaultCallTimeout, + URLPath: "/robots.txt", + URLQuery: url.Values{}, + } + got := NewGETJSONDescriptor(model.DiscardLogger, "/robots.txt") + if diff := cmp.Diff(expected, got); diff != "" { + t.Fatal(diff) + } +} + +func TestNewGetJSONWithQueryDescriptor(t *testing.T) { + query := url.Values{ + "a": {"b"}, + "c": {"d"}, + } + expected := &Descriptor{ + Accept: "application/json", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: model.DiscardLogger, + MaxBodySize: DefaultMaxBodySize, + Method: http.MethodGet, + RequestBody: nil, + Timeout: DefaultCallTimeout, + URLPath: "/robots.txt", + URLQuery: query, + } + got := NewGETJSONWithQueryDescriptor(model.DiscardLogger, "/robots.txt", query) + if diff := cmp.Diff(expected, got); diff != "" { + t.Fatal(diff) + } +} + +func TestNewPOSTJSONWithJSONResponseDescriptor(t *testing.T) { + type request struct { + Name string + Age int64 + } + + t.Run("with failure", func(t *testing.T) { + request := make(chan int64) + got, err := NewPOSTJSONWithJSONResponseDescriptor(model.DiscardLogger, "/robots.txt", request) + if err == nil || err.Error() != "json: unsupported type: chan int64" { + log.Fatal("unexpected err", err) + } + if got != nil { + log.Fatal("expected to get a nil Descriptor") + } + }) + + t.Run("with success", func(t *testing.T) { + request := request{ + Name: "sbs", + Age: 99, + } + expected := &Descriptor{ + Accept: "application/json", + Authorization: "", + ContentType: "application/json", + LogBody: false, + Logger: model.DiscardLogger, + MaxBodySize: DefaultMaxBodySize, + Method: http.MethodPost, + RequestBody: []byte(`{"Name":"sbs","Age":99}`), + Timeout: DefaultCallTimeout, + URLPath: "/robots.txt", + URLQuery: nil, + } + got, err := NewPOSTJSONWithJSONResponseDescriptor(model.DiscardLogger, "/robots.txt", request) + if err != nil { + log.Fatal(err) + } + if diff := cmp.Diff(expected, got); diff != "" { + t.Fatal(diff) + } + }) +} + +func TestMustNewPOSTJSONWithJSONResponseDescriptor(t *testing.T) { + type request struct { + Name string + Age int64 + } + + t.Run("with failure", func(t *testing.T) { + var panicked bool + func() { + defer func() { + if r := recover(); r != nil { + panicked = true + } + }() + request := make(chan int64) + _ = MustNewPOSTJSONWithJSONResponseDescriptor(model.DiscardLogger, "/robots.txt", request) + }() + if !panicked { + t.Fatal("did not panic") + } + }) + + t.Run("with success", func(t *testing.T) { + request := request{ + Name: "sbs", + Age: 99, + } + expected := &Descriptor{ + Accept: "application/json", + Authorization: "", + ContentType: "application/json", + LogBody: false, + Logger: model.DiscardLogger, + MaxBodySize: DefaultMaxBodySize, + Method: http.MethodPost, + RequestBody: []byte(`{"Name":"sbs","Age":99}`), + Timeout: DefaultCallTimeout, + URLPath: "/robots.txt", + URLQuery: nil, + } + got := MustNewPOSTJSONWithJSONResponseDescriptor(model.DiscardLogger, "/robots.txt", request) + if diff := cmp.Diff(expected, got); diff != "" { + t.Fatal(diff) + } + }) +} + +func TestNewGetResourceDescriptor(t *testing.T) { + expected := &Descriptor{ + Accept: "", + Authorization: "", + ContentType: "", + LogBody: false, + Logger: model.DiscardLogger, + MaxBodySize: DefaultMaxBodySize, + Method: http.MethodGet, + RequestBody: nil, + Timeout: DefaultCallTimeout, + URLPath: "/robots.txt", + URLQuery: url.Values{}, + } + got := NewGETResourceDescriptor(model.DiscardLogger, "/robots.txt") + if diff := cmp.Diff(expected, got); diff != "" { + t.Fatal(diff) + } +} diff --git a/internal/httpapi/doc.go b/internal/httpapi/doc.go new file mode 100644 index 0000000..0c1361c --- /dev/null +++ b/internal/httpapi/doc.go @@ -0,0 +1,15 @@ +// Package httpapi contains code for calling HTTP APIs. +// +// We model HTTP APIs as follows: +// +// 1. |Endpoint| is an API endpoint (e.g., https://api.ooni.io); +// +// 2. |Descriptor| describes the specific API you want to use (e.g., +// GET /api/v1/test-list/urls with JSON response body). +// +// Generally, you use |Call| to call the API identified by a |Descriptor| +// on the specified |Endpoint|. However, there are cases where you +// need more complex calling patterns. For example, with |SequenceCaller| +// you can invoke the same API |Descriptor| with multiple equivalent +// API |Endpoint|s until one of them succeeds or all fail. +package httpapi diff --git a/internal/httpapi/endpoint.go b/internal/httpapi/endpoint.go new file mode 100644 index 0000000..acfc4d8 --- /dev/null +++ b/internal/httpapi/endpoint.go @@ -0,0 +1,76 @@ +package httpapi + +// +// HTTP API Endpoint (e.g., https://api.ooni.io) +// + +import "github.com/ooni/probe-cli/v3/internal/model" + +// Endpoint models an HTTP endpoint on which you can call +// several HTTP APIs (e.g., https://api.ooni.io) using a +// given HTTP client potentially using a circumvention tunnel +// mechanism such as psiphon or torsf. +// +// The zero value of this struct is invalid. Please, fill all the +// fields marked as MANDATORY for correct initialization. +type Endpoint struct { + // BaseURL is the MANDATORY endpoint base URL. We will honour the + // path of this URL and prepend it to the actual path specified inside + // a |Descriptor.URLPath|. However, we will always discard any query + // that may have been set inside the BaseURL. The only query string + // will be composed from the |Descriptor.URLQuery| values. + // + // For example, https://api.ooni.io. + BaseURL string + + // HTTPClient is the MANDATORY HTTP client to use. + // + // For example, http.DefaultClient. You can introduce circumvention + // here by using an HTTPClient bound to a specific tunnel. + HTTPClient model.HTTPClient + + // Host is the OPTIONAL host header to use. + // + // If this field is empty we use the BaseURL's hostname. A specific + // host header may be needed when using cloudfronting. + Host string + + // User-Agent is the OPTIONAL user-agent to use. If empty, + // we'll use the stdlib's default user-agent string. + UserAgent string +} + +// NewEndpointList constructs a list of API endpoints from |services| +// returned by the OONI backend (or known in advance). +// +// Arguments: +// +// - httpClient is the HTTP client to use for accessing the endpoints; +// +// - userAgent is the user agent you would like to use; +// +// - service is the list of services gathered from the backend. +func NewEndpointList(httpClient model.HTTPClient, + userAgent string, services ...model.OOAPIService) (out []*Endpoint) { + for _, svc := range services { + switch svc.Type { + case "https": + out = append(out, &Endpoint{ + BaseURL: svc.Address, + HTTPClient: httpClient, + Host: "", + UserAgent: userAgent, + }) + case "cloudfront": + out = append(out, &Endpoint{ + BaseURL: svc.Address, + HTTPClient: httpClient, + Host: svc.Front, + UserAgent: userAgent, + }) + default: + // nothing! + } + } + return +} diff --git a/internal/httpapi/endpoint_test.go b/internal/httpapi/endpoint_test.go new file mode 100644 index 0000000..7077e14 --- /dev/null +++ b/internal/httpapi/endpoint_test.go @@ -0,0 +1,69 @@ +package httpapi + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/model/mocks" +) + +func TestNewEndpointList(t *testing.T) { + type args struct { + httpClient model.HTTPClient + userAgent string + services []model.OOAPIService + } + defaultHTTPClient := &mocks.HTTPClient{} + tests := []struct { + name string + args args + wantOut []*Endpoint + }{{ + name: "with no services", + args: args{ + httpClient: defaultHTTPClient, + userAgent: model.HTTPHeaderUserAgent, + services: nil, + }, + wantOut: nil, + }, { + name: "common cases", + args: args{ + httpClient: defaultHTTPClient, + userAgent: model.HTTPHeaderUserAgent, + services: []model.OOAPIService{{ + Address: "https://www.example.com/", + Type: "https", + Front: "", + }, { + Address: "https://www.example.org/", + Type: "cloudfront", + Front: "example.org.it", + }, { + Address: "https://nonexistent.onion/", + Type: "onion", + Front: "", + }}, + }, + wantOut: []*Endpoint{{ + BaseURL: "https://www.example.com/", + HTTPClient: defaultHTTPClient, + Host: "", + UserAgent: model.HTTPHeaderUserAgent, + }, { + BaseURL: "https://www.example.org/", + HTTPClient: defaultHTTPClient, + Host: "example.org.it", + UserAgent: model.HTTPHeaderUserAgent, + }}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotOut := NewEndpointList(tt.args.httpClient, tt.args.userAgent, tt.args.services...) + if diff := cmp.Diff(tt.wantOut, gotOut); diff != "" { + t.Fatal(diff) + } + }) + } +} diff --git a/internal/httpapi/sequence.go b/internal/httpapi/sequence.go new file mode 100644 index 0000000..da1f11d --- /dev/null +++ b/internal/httpapi/sequence.go @@ -0,0 +1,92 @@ +package httpapi + +// +// Sequentially call available API endpoints until one succeed +// or all of them fail. A future implementation of this code may +// (probably should?) take into account knowledge of what is +// working and what is not working to optimize the order with +// which to try different alternatives. +// + +import ( + "context" + "errors" + + "github.com/ooni/probe-cli/v3/internal/multierror" +) + +// SequenceCaller calls the API specified by |Descriptor| once for each of +// the available |Endpoints| until one of them succeeds. +// +// CAVEAT: this code will ONLY retry API calls with subsequent endpoints when +// the error originates in the HTTP round trip or while reading the body. +type SequenceCaller struct { + // Descriptor is the API |Descriptor|. + Descriptor *Descriptor + + // Endpoints is the list of |Endpoint| to use. + Endpoints []*Endpoint +} + +// NewSequenceCaller is a factory for creating a |SequenceCaller|. +func NewSequenceCaller(desc *Descriptor, endpoints ...*Endpoint) *SequenceCaller { + return &SequenceCaller{ + Descriptor: desc, + Endpoints: endpoints, + } +} + +// ErrAllEndpointsFailed indicates that all endpoints failed. +var ErrAllEndpointsFailed = errors.New("httpapi: all endpoints failed") + +// shouldRetry returns true when we should try with another endpoint given the +// value of |err| which could (obviously) be nil in case of success. +func (sc *SequenceCaller) shouldRetry(err error) bool { + var kind *errMaybeCensorship + belongs := errors.As(err, &kind) + return belongs +} + +// Call calls |Call| for each |Endpoint| and |Descriptor| until one endpoint succeeds. The +// return value is the response body and the selected endpoint index or the error. +// +// CAVEAT: this code will ONLY retry API calls with subsequent endpoints when +// the error originates in the HTTP round trip or while reading the body. +func (sc *SequenceCaller) Call(ctx context.Context) ([]byte, int, error) { + var selected int + merr := multierror.New(ErrAllEndpointsFailed) + for _, epnt := range sc.Endpoints { + respBody, err := Call(ctx, sc.Descriptor, epnt) + if sc.shouldRetry(err) { + merr.Add(err) + selected++ + continue + } + // Note: some errors will lead us to return + // early as documented for this method + return respBody, selected, err + } + return nil, -1, merr +} + +// CallWithJSONResponse is like |SequenceCaller.Call| except that it invokes the +// underlying |CallWithJSONResponse| rather than invoking |Call|. +// +// CAVEAT: this code will ONLY retry API calls with subsequent endpoints when +// the error originates in the HTTP round trip or while reading the body. +func (sc *SequenceCaller) CallWithJSONResponse(ctx context.Context, response any) (int, error) { + var selected int + merr := multierror.New(ErrAllEndpointsFailed) + for _, epnt := range sc.Endpoints { + err := CallWithJSONResponse(ctx, sc.Descriptor, epnt, response) + if sc.shouldRetry(err) { + merr.Add(err) + selected++ + continue + } + // Note: some errors will lead us to return + // early as documented for this method + return selected, err + } + return -1, merr +} diff --git a/internal/httpapi/sequence_test.go b/internal/httpapi/sequence_test.go new file mode 100644 index 0000000..13cc50f --- /dev/null +++ b/internal/httpapi/sequence_test.go @@ -0,0 +1,358 @@ +package httpapi + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/model/mocks" +) + +func TestSequenceCaller(t *testing.T) { + t.Run("Call", func(t *testing.T) { + t.Run("first success", func(t *testing.T) { + sc := NewSequenceCaller( + &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/", + }, + &Endpoint{ + BaseURL: "https://a.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader("deadbeef")), + } + return resp, nil + }, + }, + }, + &Endpoint{ + BaseURL: "https://b.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + }, + ) + data, idx, err := sc.Call(context.Background()) + if err != nil { + t.Fatal(err) + } + if idx != 0 { + t.Fatal("invalid idx") + } + if diff := cmp.Diff([]byte("deadbeef"), data); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("first HTTP failure and we immediately stop", func(t *testing.T) { + sc := NewSequenceCaller( + &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/", + }, + &Endpoint{ + BaseURL: "https://a.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + StatusCode: 403, // should cause us to return early + Body: io.NopCloser(strings.NewReader("deadbeef")), + } + return resp, nil + }, + }, + }, + &Endpoint{ + BaseURL: "https://b.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + }, + ) + data, idx, err := sc.Call(context.Background()) + var failure *ErrHTTPRequestFailed + if !errors.As(err, &failure) || failure.StatusCode != 403 { + t.Fatal("unexpected err", err) + } + if idx != 0 { + t.Fatal("invalid idx") + } + if len(data) > 0 { + t.Fatal("expected to see no response body") + } + }) + + t.Run("first network failure, second success", func(t *testing.T) { + sc := NewSequenceCaller( + &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/", + }, + &Endpoint{ + BaseURL: "https://a.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF // should cause us to cycle to the second entry + }, + }, + }, + &Endpoint{ + BaseURL: "https://b.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader("abad1dea")), + } + return resp, nil + }, + }, + }, + ) + data, idx, err := sc.Call(context.Background()) + if err != nil { + t.Fatal(err) + } + if idx != 1 { + t.Fatal("invalid idx") + } + if diff := cmp.Diff([]byte("abad1dea"), data); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("all network failure", func(t *testing.T) { + sc := NewSequenceCaller( + &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/", + }, + &Endpoint{ + BaseURL: "https://a.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF // should cause us to cycle to the next entry + }, + }, + }, + &Endpoint{ + BaseURL: "https://b.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF // should cause us to cycle to the next entry + }, + }, + }, + ) + data, idx, err := sc.Call(context.Background()) + if !errors.Is(err, ErrAllEndpointsFailed) { + t.Fatal("unexpected err", err) + } + if idx != -1 { + t.Fatal("invalid idx") + } + if len(data) > 0 { + t.Fatal("expected zero-length data") + } + }) + }) + + t.Run("CallWithJSONResponse", func(t *testing.T) { + type response struct { + Name string + Age int64 + } + + t.Run("first success", func(t *testing.T) { + sc := NewSequenceCaller( + &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/", + }, + &Endpoint{ + BaseURL: "https://a.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`{"Name":"sbs","Age":99}`)), + } + return resp, nil + }, + }, + }, + &Endpoint{ + BaseURL: "https://b.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`{}`)), // different + } + return resp, nil + }, + }, + }, + ) + expect := response{ + Name: "sbs", + Age: 99, + } + var got response + idx, err := sc.CallWithJSONResponse(context.Background(), &got) + if err != nil { + t.Fatal(err) + } + if idx != 0 { + t.Fatal("invalid idx") + } + if diff := cmp.Diff(expect, got); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("first HTTP failure and we immediately stop", func(t *testing.T) { + sc := NewSequenceCaller( + &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/", + }, + &Endpoint{ + BaseURL: "https://a.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + StatusCode: 403, // should be enough to cause us fail immediately + Body: io.NopCloser(strings.NewReader(`{"Age": 155, "Name": "sbs"}`)), + } + return resp, nil + }, + }, + }, + &Endpoint{ + BaseURL: "https://b.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + }, + ) + // even though there is a JSON body we don't care about reading it + // and so we expect to see in output the zero-value struct + expect := response{ + Name: "", + Age: 0, + } + var got response + idx, err := sc.CallWithJSONResponse(context.Background(), &got) + var failure *ErrHTTPRequestFailed + if !errors.As(err, &failure) || failure.StatusCode != 403 { + t.Fatal("unexpected err", err) + } + if idx != 0 { + t.Fatal("invalid idx") + } + if diff := cmp.Diff(expect, got); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("first network failure, second success", func(t *testing.T) { + sc := NewSequenceCaller( + &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/", + }, + &Endpoint{ + BaseURL: "https://a.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF // should cause us to try the next entry + }, + }, + }, + &Endpoint{ + BaseURL: "https://b.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`{"Age":155}`)), + } + return resp, nil + }, + }, + }, + ) + expect := response{ + Name: "", + Age: 155, + } + var got response + idx, err := sc.CallWithJSONResponse(context.Background(), &got) + if err != nil { + t.Fatal(err) + } + if idx != 1 { + t.Fatal("invalid idx") + } + if diff := cmp.Diff(expect, got); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("all network failure", func(t *testing.T) { + sc := NewSequenceCaller( + &Descriptor{ + Logger: model.DiscardLogger, + Method: http.MethodGet, + URLPath: "/", + }, + &Endpoint{ + BaseURL: "https://a.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF // should cause us to try the next entry + }, + }, + }, + &Endpoint{ + BaseURL: "https://b.example.com/", + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF // should cause us to try the next entry + }, + }, + }, + ) + var got response + idx, err := sc.CallWithJSONResponse(context.Background(), &got) + if !errors.Is(err, ErrAllEndpointsFailed) { + t.Fatal("unexpected err", err) + } + if idx != -1 { + t.Fatal("invalid idx") + } + }) + }) +} diff --git a/internal/httpx/httpx.go b/internal/httpx/httpx.go index f330745..8083f10 100644 --- a/internal/httpx/httpx.go +++ b/internal/httpx/httpx.go @@ -1,4 +1,8 @@ // Package httpx contains http extensions. +// +// Deprecated: new code should use httpapi instead. While this package and httpapi +// are basically using the same implementation, the API exposed by httpapi allows +// us to try the same request with multiple HTTP endpoints. package httpx import ( From fce6eb779b2576e2b7e14daf7300ed01ad172a2d Mon Sep 17 00:00:00 2001 From: ooninoob Date: Mon, 21 Nov 2022 19:25:23 +0100 Subject: [PATCH 12/14] Start work on IMAP probe --- internal/engine/experiment/imap/.smtp.go.swp | Bin 0 -> 24576 bytes .../engine/experiment/imap/.smtp_test.go.swp | Bin 0 -> 28672 bytes internal/engine/experiment/imap/imap.go | 416 ++++++++++++++++++ internal/engine/experiment/imap/imap_test.go | 186 ++++++++ 4 files changed, 602 insertions(+) create mode 100644 internal/engine/experiment/imap/.smtp.go.swp create mode 100644 internal/engine/experiment/imap/.smtp_test.go.swp create mode 100644 internal/engine/experiment/imap/imap.go create mode 100644 internal/engine/experiment/imap/imap_test.go diff --git a/internal/engine/experiment/imap/.smtp.go.swp b/internal/engine/experiment/imap/.smtp.go.swp new file mode 100644 index 0000000000000000000000000000000000000000..c4f8bb682bb7263202a642c7b577a5ee662f82a3 GIT binary patch literal 24576 zcmeI4e~=_qeZU*}AsZEuq(UlG%6Fr8nO*jJ7m0c<9^`KC;1=%o?soV1;c* z_qwNdcJ}U)N-9;RxA@%7bibe9_kHhuzu)(k+0jcUcBxJ7TO58)cbpfmJzRU~<&Dlo z`%iE>i66`3WBoL}b{e&hmCA9hmjXxj#L@gnJC0`kjkRVl5`?KAhhB5U59fo>AMuyk zejK#?Fda!+X36v7} zUyy)TcbwN!?v)Y%;QT-H|Nrgvj`ITi3p@pn!6Wb~_!!&;zYMoG8ZZwP zI31pP4Q0ch!u@a?ya&#MP4LQ((MI?>d<`CjufV5a39f(%I3HXX0tGJ(kPp5NUxUxV z9T3A+uoEU=2W*71VF+G1)p33Z--3@r0vE!1_|B{88+--63`*%|q}-_ySxHyWu=|2W*1p3CuhW_ro1<1I)t| zoCkw&CVUnB_%eJ6{tEsO?uHM;wIKSlABsN%>4GvpLla>=SPbeNuQ}r<=_USh;ta%{ zF!W;*49$%1nbKdJfp!$9I!}_uK{)RWc=fuZQOZ6@m)pJ~!?d3yK@`g4PAyf39p{Km zoaG!DQ95&ws9M7d=l#0!7yWoS<#vgbV5)R$Q5aSw;h>U~UbE@f3+>429cV;}X<7ax zZ6;)%3+B}jAKh{N+3EJ0j^&-LrAxXON|7b?mr_YCzny_*G(WH9pjuJgZ@N46cgl&q zny)hrwR|t>#Qstc=t~&Ri#~scK8@G7Uzi+w=Q8HcfTV3Xn0QNvqzgpw({rasenQyvz!8SEcW5$|P+* zYC9ysnvIZ`$K3*5mFV3?e#$K-)rp()`jfk>(@X>N)ES_cHUSFeg(_;N$O_q;#3J=s zU&gDh=Au~36V-Q_=tiYfrdbqpC}=iKYLckBKj%>s1DK|KPh4G@%~eCeydY#$8Pe{x zs%~O6F{k`iJ6%?XtUhH@?y8gY?m{nUqJ3(}%q(^FyOSs!tIV0lm8*57-5IXz3+2r+ znWdT10MQb~ls8>$h$NVEDBg&k^0lZn8#x0rJEwPeVLfSh3#jS-14Cv?Y>8`)V3EaR z=k)G)@4kugJkgY&hyZMpn1%NdZ$4E07Fq0bV*&*}p6*R~)#N%I10?Ssfs@=Ixx z(eCzm)R8M|B_C*fu$>no+dSX^ZCf$&(Kmf}vZ978YF? znb@P6EM84Y?aa;T$G%rt4AcKV>%|=l(>x7JlUoVVP&0{pwd^4Iw3A52c z^MZVpHp69Kbv58<3a~HJ-=1=-(rfw?H9MmAgU$d;Rw8n4dF}nC!~=ac2ol&)MQaji z{oW2Y1N*53HSE-4U8xPMagv^_X0f!3ad%{76!P)PW#2Ph>`)Y!nGL97P#48u@gG)U z&~%&uCdxnt|} zk4Z(+$qJn3HR*QPMY%dif?_Mw*k*-XWp5kT&0<5;P13V#b5_qDnowm3WKFtMI-wh- zH@=2nTM)%rR{N%Q>hw%q%20ljo#Zfiq=#mMrkOwYwzpQ~S@(7R>aCmJF0X>Qtk$01 zRdi{zV4GdOq>V<`r&BLZndK~#dND*!O1?4(ZRTLW6j*i&C7b?exk;&Z-$oXditWGM zq!^=N<;U?vsHJ9FR4mE1ah)B3irTQDr}^2dM7LjrQ7n6|mAc#GIEv@0mGy_l)OyCy zRM_sM6Uk(hZV3^rer6I@2N?*2M5`2K(?XGpQ3#z(#*{6^-6LaPH8Z6egp@~4GrGkV zP|jpDA+Nb&i?JnUjEwWl?rporPTe?woQ@J%$>|P-o^^6y@l?|%u&@w|Pv#XB6=t-4X=Z?ZdiaP;bNh&X^xYFp|JZVofb3M(R#C`c)E#xtL7(NZwhoxd znUXljf@asPVzYUkcr7#Rb~p=O#;5-dJO|%~$KZCj2|fVJFbQvlGvN$)13Znt|0nQ!a1VSG8n7QO zg(=t#ldu(j9xi~B;Pb=+eiJ?dcY+63!I|(3v4BV5cBsJ)xDX^}@E?rny>J6u2D{)a z_$fFQWXw;7@~4!*|8EIk%vnn+SupmzHg*foiaWX8evOZ`KrBm%)ghZ4*4(;iLCm;= z!-@|Wa4oY{h?(SOzJ^=PBSU0Lbh_zItL4SZ5*j-^k!%U0me*V!!#l(JKGG+iK}4aD zrCO*)S*N*RpkbQnzq6Vna&-7e!7jt}SXodp_F;VQZotJB7`rR8xY{+!+q}Z{+-ST` zT-dG6sJ0-%fR&zU&FA~V!4yAx)}}C7@ac}8r5+~5EZXoZNTh2M(nnmn*vIYiiFBx> z5p|k4IzGWZo_3n*P{R*Rj8GaFq^?+uSnNbCjPZs2ggr`}QNkd6!AMV4G^fq7En6pa zOpmBsN_Zx1D6vcQAGlGBlvHe0Zh$6cyp`4;9rlr!NHg}9zp zFEB4x4^q`6v4-!UHQHCFLs_M>{fbbv*w>K)Plf)We!;k+Q{44}wV$Z}8IbMO!i~gu|K^}03kUim}JTYen`XL{6COI82sk9Ts zGb23Nj5e?k<2fVBOe)iUOmtuT+N@-KLc!vqdn+YQgo$5c;hSCv+W5u6T(&LoGHeqWGt5Y@GTP1_cCcAR7Z}Wu_vw@#N0AyG;19MhFXf-4nk6c*q2sX)} z^@+`xVaj7O=4)~5btiOmh9;Q98~SM*Z;ozQX7%Y!b#yp;p$!AOzRPE`cp!jjZ_%gw zGQ=V;SvBp^5Yikpp#_;U1?lwJ`9vT6$$&)zVQh&~^ZaWgbb2$`0>R=_k5 zSptUp*d(Y{Rdy487ww4En~qD0;@-jD>f3SCE1?}cZTv8N1Bn}(1;FS#VxEhLv)Nd8 z7;TK&tg+tQ91$*}C7hBmkTg=Qud}TrZo1$WLiY%D#o}vg(~VfWvT3tSVJ|e7b6u$u zS1D93HlXZty){aJQg$`7KRQ5)4y%b{NkaVpi|`en#FrHRKfB-mJ^cEA1Ud767#@Oq;ZC>#5(r=qY=QIPop2t! z794mUU;kK4 zJO|%~$KjK3C;U2m2z~)>1c?`%3ZKL8zZdE-1$$vPOoGG$J_dKd&2SwAa1|T?i2-bY zGvQP?8NP>a|6O2-{&Cw!$g!_xSVo!XX%jv)~0}FY^C8$b9?{^7%&i z09*-sU=%9wMtFiT`&H}bKaUUpRrnJ88Qc$_h2Mu?gL!xtTn-WksK8IcOPv2d2hYOe za0l?`AWjP|gMBaqKMgMuBYFVtgS+5XxEZd2t6?u}g$v+(7=eG~KET89DYy*|!A-!A zR``{(Qk}h7ox=;+%2&;wcV^Q^)`FgVo7E`USawXHThB9n27?vC|4V}N>g z7))aNa}t~H0xfYDR?TiEx31uPHQH-gWUE!;7H2XUWZrfI{3~Yh4!_xqs(zyxG3g82 z)^V0))`EOP5{p`b&EtBBs3tDkwj9~(BTSBrl&uI^ZRw=Z##$r7FzZ^TBbWzz(H9fX z9OWnac7|Q^Ik^`rR7w$l2#Mh=%d)0VLI;g&$KFpz?3$Xm1)I|Ju(wt2Wijh@b^n3c zWq!5TT?kIf(a2h3wI$Jhr07XzX{(O0@2wAR3lX8~BaEVC?J6(oBF9Wk7T~&snFF~E z5nRhzoXaV?S;L!RJ@%JF=5AzF8FBOzfvIo}z01$Bjj`)C2x zJ`}sByIJ<^lgH3JJ#*-FUTH#UUc2pwb##SUwc?%Zn|9aL#*C|N-eQv7@nnE$M|*V( zhs6RH?G{t8vE=5}P`E8f8+b!}^$zLY73;{XU@5M#q)nbIo2g%nr$Xc7>9pPqX1j$O z+dxaNueG3+MP@mrs9Y_L%f3Dt(+75mE53j<%Q_Z?Ir%@MIeEqSSUME4V!l{U#13)n z_Sie9Y0}_GRF2d||7o^bzle#WeF@|6Ab>)n_a88tDN&znQHf=&#deZt#%3-riOaAg z3x|DZjgVDKm(G}-!sn_}d^)W7CdIk=nS63#9JQw8e!@%~V6rg3Xrk??RTt|>#TdA9 zJ7Nc(Q5A1bJO=A?Whq22vMOk>(Sa$sU4R=yo?HpCkD0A#(#Y_4g=YmfXyg#yiMbSF~?+nRfP3)&{e*6ci>?l4GnTk}sd_wvlyKP<@kbSl=obWSf^| zMVgF_oArgSCcS>k61zE5Of%{DZrm?37x^l>V(%DbtF(SbaK$H(Csze(5gT37#$l8Z zHsWVdSD9DXe-)Qb=mzJznbe$rG0iL}1p~UE`dn2h&g6XS%O#{8aF?xhoa!W&(w#Y- zu<*P)ce-hlY`)MIqxu}QALNVnsEcDD!Y4wqjbGFwu5~tpDg;%~bWRoBi=!Zv$+d@W z7Doz=H2Hg`mz3tm;-cf8_nBoz3s=q?DQHchGJ~WYA_=WM-P-|~!61`H;)Mm3)ORpMY&QGG(;{ubSqi8 zA{3gh*T*UDFzqMSdjPv;E*N{72ECM>ZOB;1(6`{xOQG+)iq zxFF7TRu?szA*K5V`66ngn~ zY=2hiq!|&J2_ok;7Urxgc1&!Mx?AQ(YQe(jCq-Uo=GCz+y_F`ko<*X)g-S(7+-`_d z?kdP5B)e8Sj+*26n`Y~@P08}9p=^dB=4_=eJR_aHjixs+NBsZ0@k{TL`@bB{ zgzwVFe}MZz`g;opx{3NRIqy! literal 0 HcmV?d00001 diff --git a/internal/engine/experiment/imap/.smtp_test.go.swp b/internal/engine/experiment/imap/.smtp_test.go.swp new file mode 100644 index 0000000000000000000000000000000000000000..5a742bedb807ab21ba6fcf0ea4c3a34e2c981d9b GIT binary patch literal 28672 zcmeI43veA*d4M-0DHSaR9NL7uIKITml`CCIj$<6z0USBOF1DmviUY(!v$}V6wP^3| zo!z~%ge`CaVFqX?FeMpi0|Ti`!j!ahS{|JVOlZSkT1Wz&feD3{OnB7LmJnxJXs1KJ z|LpTVBs(t{XLgg%d-uH0zq{x0|Nm-sc>nIbYHMYa!SQ0l*s^@6dd~k28rvrfqZL@b z1fT41%x35{PnOCs-YNwSP5NGasOfvN)?n4Khio^reAje_EVpjE){wQ-w0yf^x#3XI z2%Gw!na~QtO5Hno6-A0d0)+&6NuU+Z4Q_p*arvf^VTpc4X}vmU-F3ZE7GZ@13JDYv zC?rrwppZZzfkFa>1PTc}M1PTch5-224NT85FA%Q{y zPeB4^%`hJRO~Y7zA(^rNC;R^sFEWhB;W&H;z7Ai2&%izKr*Ip*8{P$zupV9x7sG%3 zmSOw@{4Kl5UV0AGa7Q711hu?v9@DlhJlr{Ilop1}h18Pu(61)I@io)fG@B_F5mf=b$ z!5MHIh0rnhC^Vo7vv4nb7Cs0^;7#yG*bmbn$BNOn(r@(jaqjC24HX|N=`RVGviwEM zA2%D;XdJIpso_-@D%O(OXgXG<>NQFOZFzRPfmLn!*3^RC+>e*Y-14ZJGo8T7l7&uC z_1rnTu12p?TvW#N)ghx#YH3(zP?^S=`k7HC!5qL!|&J53QxJa=jhSPvgeowZgJavSoN<$aQ7PB1tXF?|8bY4X*p51sW|hjRX+X!)7{BH2vK*Xs$|aH5 z=+IE--5b4P>nlsS%IlumZ#s5uYVY)9bo=QmMd;a)zGR*?>)(^W!Be+wASmOpt3sgT-!&Ovsnc6y=596c~G*i8D` zRc|awS?)1prKA2{26A)Ov7(V|RP{(ZKuyvFspL`_IwU>ABM-ah$~&W{4=u1He{O`E zHvPI4mbEzgr6}n~T$GHYxujAS9hS&sbiSId)_t$#)?|=~C#vLSPoCS%6Hzf!Nvq_R z$Q$3NT$6e!G7m{qxnD-i(r{(CGE&m_HJbg_DAZ-{>s6x6iE(iduF z+95RtqbeDcCK(p(0iznspL>G&jR z^&qLmtb&k7px&>_8>8EqlQGYceonfJ)uW#0>t~(TWM$idGF`=p#}uM#kxr@5lO!sf zCpxe~<;^J>o`VVl^Ux6YF9_Sa%`CoIqB=s!9!8U0{5m+59krFNkeXlauT1HF@gzuJ zWrA2O*=pe;mRxo{zs&SZoCTh0TPQ5c#A_}6NL#&nw>@&zNg(&xWj$vn_a) zi?-ml*5)dWC*c{hD|J^u+F8lIc5HeBqdQ=Vpd1y+PjbSk7SNThE1<1i)SM(p&Uz4D zz5l0}ti2~zSlJVcu0MVqWy#W3mMnUC!IN}lXFy$=TL`l<*N-_tSrXR0`I7TjQTfTF z;;=ko6wzZ_({$}>d9D#wrkXN0%#};BERfe#mgTm?YKhmG_lZs@-fdGaVMf)4MJ6!j zqyBg}rG+v($?7|4MY$AKn_Ea$x?yy5w5evopSQn z+dY~0N(^tMcpX~OZ*nObZG-rVF2%nPxKu;mJwJ)9`pdZB4{=W=DxT%y^s}uw+bbD% zL)MR~Y#2=@KeKKrnOqFn|3AS#d^>w~+5fW@*2gcn{uUgAqwo>93FhHixEfvsl5ZPa z3hUs0%K96)6PDlrydHMJ7I-QAf<64t;Zb-e+yd`_x4?eb2C~opKlbd8z!%{zco!5$ zA%Q{yg#-!-6cQ*TP)MMVKp}xb0;jJ8G6hCY#e1URV#Ap;DJ4p^Q;A(G=NfTqM2Qu^ z^%Sa$Rok>iRFWI5M3NmDq`$>t*4aH>o9i`9XF1Uq2CYWJ^p~+Y?1&;&RrCi{%^lxI1NMh+KDuqGi@22z_-qw-oSjGhNwa#{4{nBHXn{|ff)H-PN_F-hw~_WuvVgYaHh0t+PF7>JEu z^ady3M{o~(0^SD;FbXe*2ieDqP5&^A!IiKPE`buPhks>X|2g<;_y8=zG;D`U;6)%d z{6|qv+zUtHR=5S;4A;P8*zF&Lqwrz41O5bBFb4flh9C9OMsVO-*Z~*8e`B{l0sjbh z!iV7q%)(aK4Clg65YUg|9=I7Cuwfpy!UgacHv6x@F}NT89{vbI*bN(CJ&0}pLO71i z{(-6@JYBCZi2mV1zZU0 z;3aSZoBiYP5IhKX!y>!}HbEKWNZyA?)!6$=Les%3XEJqLjak%bsPf0XiDtmln&g)= zdOS4#6^R6NjDp+=_mFrDAd@bg&W+n}3tP~h&b1i=+Qm$wDBlEn|Wt?{; zmy14HtnZnowcW&BnC2seIFB#S)anieg=zlT^4KU$^EuC>!ZiORP4mg9Ea!30izc$n zKCdS+?W7=s;c04pZ@0Zi^YT@Szb4b$_&#r?bGXVD08Q~!h|F&?BgY;D{tQ!awTXsb9Da!d_uB9(*{n^HfHdiO^cn}r2bA9x2&tu z4|3y{*0PMGQ>!gE(rR~9s}}axRVtR&r27F3(>gJ33;0gN$4c(E1V!Ei9f8>tpJkN9 zEGc$!U)lledr%ZR=f;xOy&3T@>3YSd3WwDUUQV>3%NWz=5eUNpI zA%)5^Fj^F*#cQ9(&c16?cW$B7X5f`o6aUAHBUwLnEPwQhvhyj7g(9txqwAh^Jyur? z4|YCQ?T?KVlXl~pU3(^SE94br;omBYCQFg4 zJmQw<)VPu0`%mX=-}MzG>2mPm)xT9KEW#Vpv60O)Q*B z(c=>nllfxlalU4WGLq_2VI?VKeH{PJ-bS|Wk$~jU393ovmE=dqt?Rh2`ejR!w%_EA zl&d9LEnFN!j3iQ5lk;Lo!sDi=@2j@ri(iM_IDBqJ;{6r5h`pCCJK$ax^*wqw^poG| zZY#O;D=;S6MWnL-7vJrR**D7me?jl|u+g)>{~mlE{t&8Ag8SIh-v#f78{rZ-AH<%2 z0)7gQ!2=-v{72yr;2<<$ABbIF_WbhQ!8hR>@JaCDW$-xr`3K-`kiGt4n1Tryfpg$E zd-?CfG583`e*aByIa~zi!^_}!AA525D6GITv>=3?umS#uef>A!>+k@475*4(cnw?# z=fO+i9C(C1{(W#a+y(D}0QSKsTnT5vzq7ahC-^dmF5qMEXYf`SgYUAhzYX3FEeK&8 zp5R{m0KN_X44;Oha0Cv+jj$8MmtXGNxgdW0XLJ4*=TE`M;r(zJYH&3uI2#`2e*YcZ z0k^>n48kG!H~0eF3U7ed!4A+zr!p$pq_3`^eDXO&r5Wcs)y(Z0vaUv*yqGmotPvG! z_nvx+)pGU>&j;pz^dh>ifK}VOl@S3P}By59d|GC1ej8$u1Z4q|SJF1E zu8Z0fE$5ygtzNtI5~)2CYCHMfAydQ|N!!~saoIEQ^fYF9_snL0w)Ba156!NvLsKN( z!=q6*GyfILtnV%0jwy?{U&0qKg~F;(SVbR}$nzO}BfuhdviO#SvH4f^Ey>Bh0TEj} z19tR%pWHdwO++8t#QV@+)mI?VhW<&bj(Tl`vZ1=y_VsBigPwUkk2Evg5atv(G85%C zeW|)b$;S_=OTDGP!7-xwF`FfO^u1%duBQ;GLyjKFH?h-STfvbj6P~mZqsLxbH=$E? zCT&A!G(E9%Vl--`PN8a^6>IIPVT(`KmibVW?@D!&NNf0Ng)T`ai$5WiMPAJciB)#^ zYC}p+j$XazK{<_8Dzg8-ojv!(?8{~Ue?h#}`#F34pTYeg-}=7?-V8SUKFIg}7s6Ta zWA^`#z<1&E@Hx00Zh-UPo0Kiz_kRXH4ey05PzI@Y1cqS<&Vn=HKH5|qg#-!-6cQ*T zP)MMVKp}xb0)+$$2|TAIkZiQF{i%esU-b-WPttzXGlK;7C|Cz~{nK7v`;{a)P0;gscFhmS;JUzQWFBN~=UyF+M``WET P{vN<;N`7eyYQFymc 0 { + s.runner.logger.Infof("Trying to generate no-op traffic") + s.tk.NoOpCounter = 0 + for s.tk.NoOpCounter < noop { + s.tk.NoOpCounter += 1 + s.runner.logger.Infof("NoOp Iteration %d", s.tk.NoOpCounter) + + conn.Write([]byte("A1 NOOP\n")) + command, err := bufio.NewReader(conn).ReadString('\n') + if err != nil { + s.error(err) + break + } + if !strings.Contains(command, "OK NOOP") { + s.error(errors.New("Unexpected IMAP reply: " + command)) + break + } + } + + if s.tk.NoOpCounter == noop { + s.runner.logger.Infof("Successfully generated no-op traffic") + return true + } else { + s.runner.logger.Infof("Failed no-op traffic at iteration %d", s.tk.NoOpCounter) + return false + } + } + + return true +} + +// Run implements ExperimentMeasurer.Run +func (m Measurer) Run( + ctx context.Context, sess model.ExperimentSession, + measurement *model.Measurement, callbacks model.ExperimentCallbacks, +) error { + log := sess.Logger() + trace := measurexlite.NewTrace(0, measurement.MeasurementStartTimeSaved) + + config, err := config(measurement.Input) + if err != nil { + // Invalid input data, we don't even generate report + return err + } + + tk := new(TestKeys) + measurement.TestKeys = tk + + ctx, cancel := context.WithTimeout(ctx, 60*time.Second) + defer cancel() + + tlsconfig := tls.Config{ + InsecureSkipVerify: false, + ServerName: config.host, + } + + runner := &TCPRunner{ + trace: trace, + logger: log, + ctx: ctx, + tk: tk, + tlsconfig: &tlsconfig, + host: config.host, + port: config.port, + } + + // First resolve DNS + addrs, success := runner.resolve(config.host) + if !success { + return nil + } + + for _, addr := range addrs { + tcp_session, success := runner.conn(addr, config.port) + if !success { + continue + } + defer tcp_session.Close() + + if config.forced_tls { + // Direct TLS connection + if !tcp_session.handshake() { + continue + } + + // Try EHLO + NoOps + if !tcp_session.imap(config.noop_count) { + continue + } + } else { + // StartTLS... + if !tcp_session.starttls("A1 STARTTLS\n") { + continue + } + + if !tcp_session.imap(config.noop_count) { + continue + } + } + } + + return nil +} + +// NewExperimentMeasurer creates a new ExperimentMeasurer. +func NewExperimentMeasurer(config Config) model.ExperimentMeasurer { + return Measurer{Config: config} +} + +// SummaryKeys contains summary keys for this experiment. +// +// Note that this structure is part of the ABI contract with ooniprobe +// therefore we should be careful when changing it. +type SummaryKeys struct { + //DNSBlocking bool `json:"facebook_dns_blocking"` + //TCPBlocking bool `json:"facebook_tcp_blocking"` + IsAnomaly bool `json:"-"` +} + +// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys. +func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) { + sk := SummaryKeys{IsAnomaly: false} + _, ok := measurement.TestKeys.(*TestKeys) + if !ok { + return sk, errors.New("invalid test keys type") + } + return sk, nil +} diff --git a/internal/engine/experiment/imap/imap_test.go b/internal/engine/experiment/imap/imap_test.go new file mode 100644 index 0000000..1fbfc15 --- /dev/null +++ b/internal/engine/experiment/imap/imap_test.go @@ -0,0 +1,186 @@ +package imap + +import ( + "bufio" + "context" + "crypto/tls" + //"encoding/json" + "errors" + "fmt" + "net" + "strings" + "testing" + + "github.com/ooni/probe-cli/v3/internal/engine/mockable" + "github.com/ooni/probe-cli/v3/internal/model" +) + +func plaintextListener() net.Listener { + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + if l, err = net.Listen("tcp6", "[::1]:0"); err != nil { + panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err)) + } + } + return l +} + +func tlsListener(l net.Listener) net.Listener { + return tls.NewListener(l, &tls.Config{}) +} + +func listener_addr(l net.Listener) string { + return l.Addr().String() +} + +func ValidIMAPServer(conn net.Conn) { + for { + command, err := bufio.NewReader(conn).ReadString('\n') + if err != nil { + return + } + + if strings.Contains(command, "NOOP") { + conn.Write([]byte("A1 OK NOOP completed.\n")) + } else if command == "STARTTLS" { + conn.Write([]byte("A1 OK Begin TLS negotiation now.\n")) + // TODO: conn.Close does not actually close connection? or does client not detect it? + conn.Close() + return + } + conn.Write([]byte("\n")) + } +} + +func TCPServer(l net.Listener) { + for { + conn, err := l.Accept() + if err != nil { + continue + } + defer conn.Close() + conn.Write([]byte("* OK [CAPABILITY IMAP4rev1 SASL-IR LOGIN-REFERRALS ID ENABLE IDLE LITERAL+ STARTTLS LOGINDISABLED] howdy, ready.\n")) + ValidIMAPServer(conn) + } +} + +func TestMeasurer_run(t *testing.T) { + // runHelper is an helper function to run this set of tests. + runHelper := func(input string) (*model.Measurement, model.ExperimentMeasurer, error) { + m := NewExperimentMeasurer(Config{}) + if m.ExperimentName() != "imap" { + t.Fatal("invalid experiment name") + } + if m.ExperimentVersion() != "0.0.1" { + t.Fatal("invalid experiment version") + } + ctx := context.Background() + meas := &model.Measurement{ + Input: model.MeasurementTarget(input), + } + sess := &mockable.Session{ + MockableLogger: model.DiscardLogger, + } + callbacks := model.NewPrinterCallbacks(model.DiscardLogger) + err := m.Run(ctx, sess, meas, callbacks) + return meas, m, err + } + + t.Run("with empty input", func(t *testing.T) { + _, _, err := runHelper("") + if !errors.Is(err, errNoInputProvided) { + t.Fatal("unexpected error", err) + } + }) + + t.Run("with invalid URL", func(t *testing.T) { + _, _, err := runHelper("\t") + if !errors.Is(err, errInputIsNotAnURL) { + t.Fatal("unexpected error", err) + } + }) + + t.Run("with invalid scheme", func(t *testing.T) { + _, _, err := runHelper("https://8.8.8.8:443/") + if !errors.Is(err, errInvalidScheme) { + t.Fatal("unexpected error", err) + } + }) + + t.Run("with broken TLS", func(t *testing.T) { + p := plaintextListener() + defer p.Close() + + l := tlsListener(p) + defer l.Close() + addr := listener_addr(l) + go TCPServer(l) + + meas, m, err := runHelper("imaps://" + addr) + if err != nil { + t.Fatal(err) + } + + tk := meas.TestKeys.(*TestKeys) + + for _, run := range tk.Runs { + for _, handshake := range run.TLSHandshakes { + if *handshake.Failure != "unknown_failure: remote error: tls: unrecognized name" { + t.Fatal("expected unrecognized_name in TLS handshake") + } + } + + if run.NoOpCounter != 0 { + t.Fatalf("expected to not have any noops, not %d noops", run.NoOpCounter) + } + } + + ask, err := m.GetSummaryKeys(meas) + if err != nil { + t.Fatal("cannot obtain summary") + } + summary := ask.(SummaryKeys) + if summary.IsAnomaly { + t.Fatal("expected no anomaly") + } + }) + + t.Run("with broken starttls", func(t *testing.T) { + l := plaintextListener() + defer l.Close() + addr := listener_addr(l) + + go TCPServer(l) + + meas, m, err := runHelper("imap://" + addr) + if err != nil { + t.Fatal(err) + } + + tk := meas.TestKeys.(*TestKeys) + //bs, _ := json.Marshal(tk) + //fmt.Println(string(bs)) + + for _, run := range tk.Runs { + for _, handshake := range run.TLSHandshakes { + if *handshake.Failure != "unknown_failure: tls: first record does not look like a TLS handshake" { + + t.Fatal("expected broken handshake") + } + } + + if run.NoOpCounter != 0 { + t.Fatalf("expected to not have any noops, not %d noops", run.NoOpCounter) + } + } + + ask, err := m.GetSummaryKeys(meas) + if err != nil { + t.Fatal("cannot obtain summary") + } + summary := ask.(SummaryKeys) + if summary.IsAnomaly { + t.Fatal("expected no anomaly") + } + }) +} From a0dc65641d7a31e116d9411ecf9e69ed1955e792 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 22 Nov 2022 10:43:47 +0100 Subject: [PATCH 13/14] refactor: pass experiment arguments using a struct (#983) Closes https://github.com/ooni/probe/issues/2358. --- internal/engine/experiment.go | 7 +- internal/engine/experiment/dash/dash.go | 8 +- internal/engine/experiment/dash/dash_test.go | 12 +- .../engine/experiment/dnscheck/dnscheck.go | 9 +- .../experiment/dnscheck/dnscheck_test.go | 72 ++++++------ internal/engine/experiment/dnsping/dnsping.go | 10 +- .../engine/experiment/dnsping/dnsping_test.go | 7 +- internal/engine/experiment/example/example.go | 8 +- .../engine/experiment/example/example_test.go | 14 ++- .../experiment/fbmessenger/fbmessenger.go | 8 +- .../fbmessenger/fbmessenger_test.go | 14 ++- internal/engine/experiment/hhfm/hhfm.go | 8 +- internal/engine/experiment/hhfm/hhfm_test.go | 49 ++++++-- internal/engine/experiment/hirl/hirl.go | 8 +- internal/engine/experiment/hirl/hirl_test.go | 49 ++++++-- .../httphostheader/httphostheader.go | 10 +- .../httphostheader/httphostheader_test.go | 36 +++--- internal/engine/experiment/ndt7/ndt7.go | 8 +- internal/engine/experiment/ndt7/ndt7_test.go | 55 +++++---- .../experiment/portfiltering/measurer.go | 10 +- .../experiment/portfiltering/measurer_test.go | 7 +- internal/engine/experiment/psiphon/psiphon.go | 8 +- .../engine/experiment/psiphon/psiphon_test.go | 23 +++- .../engine/experiment/quicping/quicping.go | 11 +- .../experiment/quicping/quicping_test.go | 64 ++++++++--- .../engine/experiment/riseupvpn/riseupvpn.go | 7 +- .../experiment/riseupvpn/riseupvpn_test.go | 28 +++-- internal/engine/experiment/run/dnscheck.go | 7 +- internal/engine/experiment/run/run.go | 8 +- internal/engine/experiment/run/run_test.go | 28 ++++- internal/engine/experiment/run/urlgetter.go | 7 +- internal/engine/experiment/signal/signal.go | 6 +- .../engine/experiment/signal/signal_test.go | 24 ++-- .../simplequicping/simplequicping.go | 11 +- .../simplequicping/simplequicping_test.go | 7 +- .../experiment/sniblocking/sniblocking.go | 10 +- .../sniblocking/sniblocking_test.go | 36 +++--- .../stunreachability/stunreachability.go | 8 +- .../stunreachability/stunreachability_test.go | 108 +++++++++--------- internal/engine/experiment/tcpping/tcpping.go | 10 +- .../engine/experiment/tcpping/tcpping_test.go | 7 +- .../engine/experiment/telegram/telegram.go | 7 +- .../experiment/telegram/telegram_test.go | 19 +-- .../experiment/tlsmiddlebox/measurer.go | 10 +- .../experiment/tlsmiddlebox/measurer_test.go | 7 +- internal/engine/experiment/tlsping/tlsping.go | 10 +- .../engine/experiment/tlsping/tlsping_test.go | 7 +- internal/engine/experiment/tlstool/tlstool.go | 11 +- .../engine/experiment/tlstool/tlstool_test.go | 36 +++--- internal/engine/experiment/tor/tor.go | 10 +- internal/engine/experiment/tor/tor_test.go | 60 +++++----- .../experiment/torsf/integration_test.go | 7 +- internal/engine/experiment/torsf/torsf.go | 8 +- .../engine/experiment/torsf/torsf_test.go | 35 +++++- .../engine/experiment/urlgetter/urlgetter.go | 8 +- .../experiment/urlgetter/urlgetter_test.go | 20 ++-- .../experiment/vanillator/integration_test.go | 7 +- .../experiment/vanillator/vanillator.go | 8 +- .../experiment/vanillator/vanillator_test.go | 21 +++- .../webconnectivity/webconnectivity.go | 11 +- .../webconnectivity/webconnectivity_test.go | 42 ++++++- .../engine/experiment/whatsapp/whatsapp.go | 9 +- .../experiment/whatsapp/whatsapp_test.go | 21 +++- .../engine/experiment_integration_test.go | 5 +- .../experiment/webconnectivity/measurer.go | 6 +- internal/model/experiment.go | 18 ++- .../experiment/torsf/chapter01/README.md | 7 +- .../experiment/torsf/chapter01/main.go | 7 +- .../experiment/torsf/chapter02/README.md | 8 +- .../experiment/torsf/chapter02/main.go | 7 +- .../experiment/torsf/chapter02/torsf.go | 8 +- .../experiment/torsf/chapter03/README.md | 8 +- .../experiment/torsf/chapter03/main.go | 7 +- .../experiment/torsf/chapter03/torsf.go | 8 +- .../experiment/torsf/chapter04/main.go | 7 +- .../experiment/torsf/chapter04/torsf.go | 8 +- 76 files changed, 824 insertions(+), 481 deletions(-) diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index f40f4c3..325bb6f 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -92,7 +92,12 @@ func (eaw *experimentAsyncWrapper) RunAsync( out := make(chan *model.ExperimentAsyncTestKeys) measurement := eaw.experiment.newMeasurement(input) start := time.Now() - err := eaw.experiment.measurer.Run(ctx, eaw.session, measurement, eaw.callbacks) + args := &model.ExperimentArgs{ + Callbacks: eaw.callbacks, + Measurement: measurement, + Session: eaw.session, + } + err := eaw.experiment.measurer.Run(ctx, args) stop := time.Now() if err != nil { return nil, err diff --git a/internal/engine/experiment/dash/dash.go b/internal/engine/experiment/dash/dash.go index a44021d..8489286 100644 --- a/internal/engine/experiment/dash/dash.go +++ b/internal/engine/experiment/dash/dash.go @@ -249,10 +249,10 @@ func (m Measurer) ExperimentVersion() string { } // Run implements model.ExperimentMeasurer.Run. -func (m Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session tk := new(TestKeys) measurement.TestKeys = tk saver := &tracex.Saver{} diff --git a/internal/engine/experiment/dash/dash_test.go b/internal/engine/experiment/dash/dash_test.go index 7b5addc..b2a910c 100644 --- a/internal/engine/experiment/dash/dash_test.go +++ b/internal/engine/experiment/dash/dash_test.go @@ -270,15 +270,15 @@ func TestMeasureWithCancelledContext(t *testing.T) { cancel() // cause failure measurement := new(model.Measurement) m := &Measurer{} - err := m.Run( - ctx, - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableHTTPClient: http.DefaultClient, MockableLogger: log.Log, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := m.Run(ctx, args) // See corresponding comment in Measurer.Run implementation to // understand why here it's correct to return nil. if !errors.Is(err, nil) { diff --git a/internal/engine/experiment/dnscheck/dnscheck.go b/internal/engine/experiment/dnscheck/dnscheck.go index 66b805d..d9912cb 100644 --- a/internal/engine/experiment/dnscheck/dnscheck.go +++ b/internal/engine/experiment/dnscheck/dnscheck.go @@ -120,10 +120,11 @@ var ( ) // Run implements model.ExperimentSession.Run -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session + // 1. fill the measurement with test keys tk := new(TestKeys) tk.Lookups = make(map[string]urlgetter.TestKeys) diff --git a/internal/engine/experiment/dnscheck/dnscheck_test.go b/internal/engine/experiment/dnscheck/dnscheck_test.go index 2bc1966..c217eb6 100644 --- a/internal/engine/experiment/dnscheck/dnscheck_test.go +++ b/internal/engine/experiment/dnscheck/dnscheck_test.go @@ -56,12 +56,12 @@ func TestExperimentNameAndVersion(t *testing.T) { func TestDNSCheckFailsWithoutInput(t *testing.T) { measurer := NewExperimentMeasurer(Config{Domain: "example.com"}) - err := measurer.Run( - context.Background(), - newsession(), - new(model.Measurement), - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: new(model.Measurement), + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, ErrInputRequired) { t.Fatal("expected no input error") } @@ -69,12 +69,12 @@ func TestDNSCheckFailsWithoutInput(t *testing.T) { func TestDNSCheckFailsWithInvalidURL(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) - err := measurer.Run( - context.Background(), - newsession(), - &model.Measurement{Input: "Not a valid URL \x7f"}, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &model.Measurement{Input: "Not a valid URL \x7f"}, + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, ErrInvalidURL) { t.Fatal("expected invalid input error") } @@ -82,12 +82,12 @@ func TestDNSCheckFailsWithInvalidURL(t *testing.T) { func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) - err := measurer.Run( - context.Background(), - newsession(), - &model.Measurement{Input: "file://1.1.1.1"}, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &model.Measurement{Input: "file://1.1.1.1"}, + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, ErrUnsupportedURLScheme) { t.Fatal("expected unsupported scheme error") } @@ -100,12 +100,12 @@ func TestWithCancelledContext(t *testing.T) { DefaultAddrs: "1.1.1.1 1.0.0.1", }) measurement := &model.Measurement{Input: "dot://one.one.one.one"} - err := measurer.Run( - ctx, - newsession(), - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: newsession(), + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -147,12 +147,12 @@ func TestDNSCheckValid(t *testing.T) { DefaultAddrs: "1.1.1.1 1.0.0.1", }) measurement := model.Measurement{Input: "dot://one.one.one.one:853"} - err := measurer.Run( - context.Background(), - newsession(), - &measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &measurement, + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatalf("unexpected error: %s", err.Error()) } @@ -195,12 +195,12 @@ func TestDNSCheckWait(t *testing.T) { measurer := &Measurer{Endpoints: endpoints} run := func(input string) { measurement := model.Measurement{Input: model.MeasurementTarget(input)} - err := measurer.Run( - context.Background(), - newsession(), - &measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &measurement, + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatalf("unexpected error: %s", err.Error()) } diff --git a/internal/engine/experiment/dnsping/dnsping.go b/internal/engine/experiment/dnsping/dnsping.go index 648b323..cef7a93 100644 --- a/internal/engine/experiment/dnsping/dnsping.go +++ b/internal/engine/experiment/dnsping/dnsping.go @@ -85,12 +85,10 @@ var ( ) // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session if measurement.Input == "" { return errNoInputProvided } diff --git a/internal/engine/experiment/dnsping/dnsping_test.go b/internal/engine/experiment/dnsping/dnsping_test.go index 3255a31..0a9ef50 100644 --- a/internal/engine/experiment/dnsping/dnsping_test.go +++ b/internal/engine/experiment/dnsping/dnsping_test.go @@ -61,7 +61,12 @@ func TestMeasurer_run(t *testing.T) { MockableLogger: model.DiscardLogger, } callbacks := model.NewPrinterCallbacks(model.DiscardLogger) - err := m.Run(ctx, sess, meas, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: meas, + Session: sess, + } + err := m.Run(ctx, args) return meas, m, err } diff --git a/internal/engine/experiment/example/example.go b/internal/engine/experiment/example/example.go index 1a7f329..c2c1c01 100644 --- a/internal/engine/experiment/example/example.go +++ b/internal/engine/experiment/example/example.go @@ -57,10 +57,10 @@ func (m Measurer) ExperimentVersion() string { var ErrFailure = errors.New("mocked error") // Run implements model.ExperimentMeasurer.Run. -func (m Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session var err error if m.config.ReturnError { err = ErrFailure diff --git a/internal/engine/experiment/example/example_test.go b/internal/engine/experiment/example/example_test.go index dc7e218..29c28fa 100644 --- a/internal/engine/experiment/example/example_test.go +++ b/internal/engine/experiment/example/example_test.go @@ -26,7 +26,12 @@ func TestSuccess(t *testing.T) { sess := &mockable.Session{MockableLogger: log.Log} callbacks := model.NewPrinterCallbacks(sess.Logger()) measurement := new(model.Measurement) - err := m.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := m.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -47,7 +52,12 @@ func TestFailure(t *testing.T) { ctx := context.Background() sess := &mockable.Session{MockableLogger: log.Log} callbacks := model.NewPrinterCallbacks(sess.Logger()) - err := m.Run(ctx, sess, new(model.Measurement), callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: new(model.Measurement), + Session: sess, + } + err := m.Run(ctx, args) if !errors.Is(err, example.ErrFailure) { t.Fatal("expected an error here") } diff --git a/internal/engine/experiment/fbmessenger/fbmessenger.go b/internal/engine/experiment/fbmessenger/fbmessenger.go index 49bcbeb..3901845 100644 --- a/internal/engine/experiment/fbmessenger/fbmessenger.go +++ b/internal/engine/experiment/fbmessenger/fbmessenger.go @@ -157,10 +157,10 @@ func (m Measurer) ExperimentVersion() string { } // Run implements ExperimentMeasurer.Run -func (m Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() urlgetter.RegisterExtensions(measurement) diff --git a/internal/engine/experiment/fbmessenger/fbmessenger_test.go b/internal/engine/experiment/fbmessenger/fbmessenger_test.go index 0545cd4..fa631c3 100644 --- a/internal/engine/experiment/fbmessenger/fbmessenger_test.go +++ b/internal/engine/experiment/fbmessenger/fbmessenger_test.go @@ -35,7 +35,12 @@ func TestSuccess(t *testing.T) { sess := newsession(t) measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -97,7 +102,12 @@ func TestWithCancelledContext(t *testing.T) { sess := &mockable.Session{MockableLogger: log.Log} measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } diff --git a/internal/engine/experiment/hhfm/hhfm.go b/internal/engine/experiment/hhfm/hhfm.go index 574cfbd..8518432 100644 --- a/internal/engine/experiment/hhfm/hhfm.go +++ b/internal/engine/experiment/hhfm/hhfm.go @@ -90,10 +90,10 @@ var ( ) // Run implements ExperimentMeasurer.Run. -func (m Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() urlgetter.RegisterExtensions(measurement) diff --git a/internal/engine/experiment/hhfm/hhfm_test.go b/internal/engine/experiment/hhfm/hhfm_test.go index 4d95f70..157ab18 100644 --- a/internal/engine/experiment/hhfm/hhfm_test.go +++ b/internal/engine/experiment/hhfm/hhfm_test.go @@ -45,7 +45,12 @@ func TestSuccess(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -153,7 +158,12 @@ func TestCancelledContext(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -259,7 +269,12 @@ func TestNoHelpers(t *testing.T) { sess := &mockable.Session{} measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, hhfm.ErrNoAvailableTestHelpers) { t.Fatal("not the error we expected") } @@ -309,7 +324,12 @@ func TestNoActualHelpersInList(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, hhfm.ErrNoAvailableTestHelpers) { t.Fatal("not the error we expected") } @@ -362,7 +382,12 @@ func TestWrongTestHelperType(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, hhfm.ErrInvalidHelperType) { t.Fatal("not the error we expected") } @@ -415,7 +440,12 @@ func TestNewRequestFailure(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { t.Fatal("not the error we expected") } @@ -472,7 +502,12 @@ func TestInvalidJSONBody(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } diff --git a/internal/engine/experiment/hirl/hirl.go b/internal/engine/experiment/hirl/hirl.go index 6cc3517..e4931d9 100644 --- a/internal/engine/experiment/hirl/hirl.go +++ b/internal/engine/experiment/hirl/hirl.go @@ -78,10 +78,10 @@ var ( ) // Run implements ExperimentMeasurer.Run. -func (m Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session tk := new(TestKeys) measurement.TestKeys = tk if len(m.Methods) < 1 { diff --git a/internal/engine/experiment/hirl/hirl_test.go b/internal/engine/experiment/hirl/hirl_test.go index 1299307..40fec23 100644 --- a/internal/engine/experiment/hirl/hirl_test.go +++ b/internal/engine/experiment/hirl/hirl_test.go @@ -42,7 +42,12 @@ func TestSuccess(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -91,7 +96,12 @@ func TestCancelledContext(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -190,7 +200,12 @@ func TestWithFakeMethods(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -251,7 +266,12 @@ func TestWithNoMethods(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, hirl.ErrNoMeasurementMethod) { t.Fatal("not the error we expected") } @@ -279,7 +299,12 @@ func TestNoHelpers(t *testing.T) { sess := &mockable.Session{} measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, hirl.ErrNoAvailableTestHelpers) { t.Fatal("not the error we expected") } @@ -311,7 +336,12 @@ func TestNoActualHelperInList(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, hirl.ErrNoAvailableTestHelpers) { t.Fatal("not the error we expected") } @@ -346,7 +376,12 @@ func TestWrongTestHelperType(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, hirl.ErrInvalidHelperType) { t.Fatal("not the error we expected") } diff --git a/internal/engine/experiment/httphostheader/httphostheader.go b/internal/engine/experiment/httphostheader/httphostheader.go index 70e4087..0a6a5b4 100644 --- a/internal/engine/experiment/httphostheader/httphostheader.go +++ b/internal/engine/experiment/httphostheader/httphostheader.go @@ -46,12 +46,10 @@ func (m *Measurer) ExperimentVersion() string { } // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session if measurement.Input == "" { return errors.New("experiment requires input") } diff --git a/internal/engine/experiment/httphostheader/httphostheader_test.go b/internal/engine/experiment/httphostheader/httphostheader_test.go index efd88ce..2c9c556 100644 --- a/internal/engine/experiment/httphostheader/httphostheader_test.go +++ b/internal/engine/experiment/httphostheader/httphostheader_test.go @@ -30,12 +30,12 @@ func TestMeasurerMeasureNoMeasurementInput(t *testing.T) { measurer := NewExperimentMeasurer(Config{ TestHelperURL: "http://www.google.com", }) - err := measurer.Run( - context.Background(), - newsession(), - new(model.Measurement), - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &model.Measurement{}, + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if err == nil || err.Error() != "experiment requires input" { t.Fatal("not the error we expected") } @@ -44,12 +44,12 @@ func TestMeasurerMeasureNoMeasurementInput(t *testing.T) { func TestMeasurerMeasureNoTestHelper(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) measurement := &model.Measurement{Input: "x.org"} - err := measurer.Run( - context.Background(), - newsession(), - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } @@ -75,12 +75,12 @@ func TestRunnerHTTPSetHostHeader(t *testing.T) { measurement := &model.Measurement{ Input: "x.org", } - err := measurer.Run( - context.Background(), - newsession(), - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if host != "x.org" { t.Fatal("not the host we expected") } diff --git a/internal/engine/experiment/ndt7/ndt7.go b/internal/engine/experiment/ndt7/ndt7.go index 8b403c6..7e4bb23 100644 --- a/internal/engine/experiment/ndt7/ndt7.go +++ b/internal/engine/experiment/ndt7/ndt7.go @@ -210,10 +210,10 @@ func (m *Measurer) doUpload( } // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session tk := new(TestKeys) tk.Protocol = 7 measurement.TestKeys = tk diff --git a/internal/engine/experiment/ndt7/ndt7_test.go b/internal/engine/experiment/ndt7/ndt7_test.go index 5ad4f1c..7da663e 100644 --- a/internal/engine/experiment/ndt7/ndt7_test.go +++ b/internal/engine/experiment/ndt7/ndt7_test.go @@ -84,7 +84,12 @@ func TestRunWithCancelledContext(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // immediately cancel meas := &model.Measurement{} - err := m.Run(ctx, sess, meas, model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: meas, + Session: sess, + } + err := m.Run(ctx, args) // Here we get nil because we still want to submit this measurement if !errors.Is(err, nil) { t.Fatal("not the error we expected") @@ -104,15 +109,15 @@ func TestGood(t *testing.T) { } measurement := new(model.Measurement) measurer := NewExperimentMeasurer(Config{}) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableHTTPClient: http.DefaultClient, MockableLogger: log.Log, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } @@ -133,15 +138,15 @@ func TestFailDownload(t *testing.T) { cancel() } meas := &model.Measurement{} - err := measurer.Run( - ctx, - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: meas, + Session: &mockable.Session{ MockableHTTPClient: http.DefaultClient, MockableLogger: log.Log, }, - meas, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(ctx, args) // We expect a nil failure here because we want to submit anyway // a measurement that failed to connect to m-lab. if err != nil { @@ -164,15 +169,15 @@ func TestFailUpload(t *testing.T) { cancel() } meas := &model.Measurement{} - err := measurer.Run( - ctx, - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: meas, + Session: &mockable.Session{ MockableHTTPClient: http.DefaultClient, MockableLogger: log.Log, }, - meas, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(ctx, args) // Here we expect a nil error because we want to submit this measurement if err != nil { t.Fatal(err) @@ -197,15 +202,15 @@ func TestDownloadJSONUnmarshalFail(t *testing.T) { seenError = true return expected } - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &model.Measurement{}, + Session: &mockable.Session{ MockableHTTPClient: http.DefaultClient, MockableLogger: log.Log, }, - new(model.Measurement), - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } diff --git a/internal/engine/experiment/portfiltering/measurer.go b/internal/engine/experiment/portfiltering/measurer.go index 76c9a2d..487f036 100644 --- a/internal/engine/experiment/portfiltering/measurer.go +++ b/internal/engine/experiment/portfiltering/measurer.go @@ -38,12 +38,10 @@ var ( ) // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session // TODO(DecFox): Replace the localhost deployment with an OONI testhelper // Ensure that we only do this once we have a deployed testhelper testhelper := "http://127.0.0.1" diff --git a/internal/engine/experiment/portfiltering/measurer_test.go b/internal/engine/experiment/portfiltering/measurer_test.go index e3a3876..c5ba589 100644 --- a/internal/engine/experiment/portfiltering/measurer_test.go +++ b/internal/engine/experiment/portfiltering/measurer_test.go @@ -29,7 +29,12 @@ func TestMeasurer_run(t *testing.T) { } callbacks := model.NewPrinterCallbacks(model.DiscardLogger) ctx := context.Background() - err := m.Run(ctx, sess, meas, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: meas, + Session: sess, + } + err := m.Run(ctx, args) if err != nil { t.Fatal(err) } diff --git a/internal/engine/experiment/psiphon/psiphon.go b/internal/engine/experiment/psiphon/psiphon.go index 5f55aea..45ed762 100644 --- a/internal/engine/experiment/psiphon/psiphon.go +++ b/internal/engine/experiment/psiphon/psiphon.go @@ -66,10 +66,10 @@ func (m *Measurer) printprogress( } // Run runs the measurement -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session const maxruntime = 300 ctx, cancel := context.WithTimeout(ctx, maxruntime*time.Second) var ( diff --git a/internal/engine/experiment/psiphon/psiphon_test.go b/internal/engine/experiment/psiphon/psiphon_test.go index 112ccdf..4186448 100644 --- a/internal/engine/experiment/psiphon/psiphon_test.go +++ b/internal/engine/experiment/psiphon/psiphon_test.go @@ -33,8 +33,12 @@ func TestRunWithCancelledContext(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // fail immediately measurement := new(model.Measurement) - err := measurer.Run(ctx, newfakesession(), measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: newfakesession(), + } + err := measurer.Run(ctx, args) if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("expected another error here") } @@ -64,8 +68,12 @@ func TestRunWithCustomInputAndCancelledContext(t *testing.T) { } ctx, cancel := context.WithCancel(context.Background()) cancel() // fail immediately - err := measurer.Run(ctx, newfakesession(), measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: newfakesession(), + } + err := measurer.Run(ctx, args) if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("expected another error here") } @@ -84,7 +92,12 @@ func TestRunWillPrintSomethingWithCancelledContext(t *testing.T) { cancel() // fail after we've given the printer a chance to run } observer := observerCallbacks{progress: &atomicx.Int64{}} - err := measurer.Run(ctx, newfakesession(), measurement, observer) + args := &model.ExperimentArgs{ + Callbacks: observer, + Measurement: measurement, + Session: newfakesession(), + } + err := measurer.Run(ctx, args) if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("expected another error here") } diff --git a/internal/engine/experiment/quicping/quicping.go b/internal/engine/experiment/quicping/quicping.go index 972f68e..cedf7c4 100644 --- a/internal/engine/experiment/quicping/quicping.go +++ b/internal/engine/experiment/quicping/quicping.go @@ -221,12 +221,11 @@ func (m *Measurer) receiver( } // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session + host := string(measurement.Input) // allow URL input if u, err := url.ParseRequestURI(host); err == nil { diff --git a/internal/engine/experiment/quicping/quicping_test.go b/internal/engine/experiment/quicping/quicping_test.go index 79a214f..34a8023 100644 --- a/internal/engine/experiment/quicping/quicping_test.go +++ b/internal/engine/experiment/quicping/quicping_test.go @@ -33,8 +33,12 @@ func TestInvalidHost(t *testing.T) { measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("a.a.a.a") sess := &mockable.Session{MockableLogger: log.Log} - err := measurer.Run(context.Background(), sess, measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err == nil { t.Fatal("expected an error here") } @@ -53,8 +57,12 @@ func TestURLInput(t *testing.T) { measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("https://google.com/") sess := &mockable.Session{MockableLogger: log.Log} - err := measurer.Run(context.Background(), sess, measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal("unexpected error") } @@ -73,8 +81,12 @@ func TestSuccess(t *testing.T) { measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("google.com") sess := &mockable.Session{MockableLogger: log.Log} - err := measurer.Run(context.Background(), sess, measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal("did not expect an error here") } @@ -117,8 +129,12 @@ func TestWithCancelledContext(t *testing.T) { sess := &mockable.Session{MockableLogger: log.Log} ctx, cancel := context.WithCancel(context.Background()) cancel() - err := measurer.Run(ctx, sess, measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal("did not expect an error here") } @@ -138,8 +154,12 @@ func TestListenFails(t *testing.T) { measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("google.com") sess := &mockable.Session{MockableLogger: log.Log} - err := measurer.Run(context.Background(), sess, measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err == nil { t.Fatal("expected an error here") } @@ -182,8 +202,12 @@ func TestWriteFails(t *testing.T) { measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("google.com") sess := &mockable.Session{MockableLogger: log.Log} - err := measurer.Run(context.Background(), sess, measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal("unexpected error") } @@ -239,8 +263,12 @@ func TestReadFails(t *testing.T) { measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("google.com") sess := &mockable.Session{MockableLogger: log.Log} - err := measurer.Run(context.Background(), sess, measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal("unexpected error") } @@ -271,8 +299,12 @@ func TestNoResponse(t *testing.T) { measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("ooni.org") sess := &mockable.Session{MockableLogger: log.Log} - err := measurer.Run(context.Background(), sess, measurement, - model.NewPrinterCallbacks(log.Log)) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal("did not expect an error here") } diff --git a/internal/engine/experiment/riseupvpn/riseupvpn.go b/internal/engine/experiment/riseupvpn/riseupvpn.go index 62dd7e3..10aa8de 100644 --- a/internal/engine/experiment/riseupvpn/riseupvpn.go +++ b/internal/engine/experiment/riseupvpn/riseupvpn.go @@ -175,8 +175,11 @@ func (m Measurer) ExperimentVersion() string { } // Run implements ExperimentMeasurer.Run. -func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session + ctx, cancel := context.WithTimeout(ctx, 90*time.Second) defer cancel() testkeys := NewTestKeys() diff --git a/internal/engine/experiment/riseupvpn/riseupvpn_test.go b/internal/engine/experiment/riseupvpn/riseupvpn_test.go index 0e67541..5ae4637 100644 --- a/internal/engine/experiment/riseupvpn/riseupvpn_test.go +++ b/internal/engine/experiment/riseupvpn/riseupvpn_test.go @@ -100,7 +100,7 @@ const ( "cert": "XXXXXXXXXXXXXXXXXXXXXXXXX", "iatMode": "0" } - }, + }, { "type":"openvpn", "protocols":[ @@ -328,7 +328,12 @@ func TestInvalidCaCert(t *testing.T) { sess := &mockable.Session{MockableLogger: log.Log} measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -599,7 +604,12 @@ func TestMissingTransport(t *testing.T) { sess := &mockable.Session{MockableLogger: log.Log} measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err = measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err = measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -790,14 +800,14 @@ func runDefaultMockTest(t *testing.T, multiGetter urlgetter.MultiGetter) *model. } measurement := new(model.Measurement) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: log.Log, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) diff --git a/internal/engine/experiment/run/dnscheck.go b/internal/engine/experiment/run/dnscheck.go index da3fd84..538e9a0 100644 --- a/internal/engine/experiment/run/dnscheck.go +++ b/internal/engine/experiment/run/dnscheck.go @@ -21,5 +21,10 @@ func (m *dnsCheckMain) do(ctx context.Context, input StructuredInput, measurement.TestName = exp.ExperimentName() measurement.TestVersion = exp.ExperimentVersion() measurement.Input = model.MeasurementTarget(input.Input) - return exp.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + return exp.Run(ctx, args) } diff --git a/internal/engine/experiment/run/run.go b/internal/engine/experiment/run/run.go index 6c38057..a25cf89 100644 --- a/internal/engine/experiment/run/run.go +++ b/internal/engine/experiment/run/run.go @@ -46,10 +46,10 @@ type StructuredInput struct { } // Run implements ExperimentMeasurer.ExperimentVersion. -func (Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session var input StructuredInput if err := json.Unmarshal([]byte(measurement.Input), &input); err != nil { return err diff --git a/internal/engine/experiment/run/run_test.go b/internal/engine/experiment/run/run_test.go index 06ae406..6fb3fde 100644 --- a/internal/engine/experiment/run/run_test.go +++ b/internal/engine/experiment/run/run_test.go @@ -31,7 +31,12 @@ func TestRunDNSCheckWithCancelledContext(t *testing.T) { cancel() // fail immediately sess := &mockable.Session{MockableLogger: log.Log} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) // TODO(bassosimone): here we could improve the tests by checking // whether the result makes sense for a cancelled context. if err != nil { @@ -62,7 +67,12 @@ func TestRunURLGetterWithCancelledContext(t *testing.T) { cancel() // fail immediately sess := &mockable.Session{MockableLogger: log.Log} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { // here we expected nil b/c we want to submit the measurement t.Fatal(err) } @@ -86,7 +96,12 @@ func TestRunWithInvalidJSON(t *testing.T) { ctx := context.Background() sess := &mockable.Session{MockableLogger: log.Log} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err == nil || err.Error() != "invalid character '}' looking for beginning of value" { t.Fatalf("not the error we expected: %+v", err) } @@ -100,7 +115,12 @@ func TestRunWithUnknownExperiment(t *testing.T) { ctx := context.Background() sess := &mockable.Session{MockableLogger: log.Log} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err == nil || err.Error() != "no such experiment: antani" { t.Fatalf("not the error we expected: %+v", err) } diff --git a/internal/engine/experiment/run/urlgetter.go b/internal/engine/experiment/run/urlgetter.go index 085f78f..753c051 100644 --- a/internal/engine/experiment/run/urlgetter.go +++ b/internal/engine/experiment/run/urlgetter.go @@ -18,5 +18,10 @@ func (m *urlGetterMain) do(ctx context.Context, input StructuredInput, measurement.TestName = exp.ExperimentName() measurement.TestVersion = exp.ExperimentVersion() measurement.Input = model.MeasurementTarget(input.Input) - return exp.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + return exp.Run(ctx, args) } diff --git a/internal/engine/experiment/signal/signal.go b/internal/engine/experiment/signal/signal.go index 07e01bb..df89fac 100644 --- a/internal/engine/experiment/signal/signal.go +++ b/internal/engine/experiment/signal/signal.go @@ -141,8 +141,10 @@ func (m Measurer) ExperimentVersion() string { } // Run implements ExperimentMeasurer.Run -func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() urlgetter.RegisterExtensions(measurement) diff --git a/internal/engine/experiment/signal/signal_test.go b/internal/engine/experiment/signal/signal_test.go index ecadebe..f78e859 100644 --- a/internal/engine/experiment/signal/signal_test.go +++ b/internal/engine/experiment/signal/signal_test.go @@ -25,14 +25,14 @@ func TestNewExperimentMeasurer(t *testing.T) { func TestGood(t *testing.T) { measurer := signal.NewExperimentMeasurer(signal.Config{}) measurement := new(model.Measurement) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: log.Log, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } @@ -103,14 +103,14 @@ func TestBadSignalCA(t *testing.T) { SignalCA: "INVALIDCA", }) measurement := new(model.Measurement) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: log.Log, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err.Error() != "AppendCertsFromPEM failed" { t.Fatal("not the error we expected") } diff --git a/internal/engine/experiment/simplequicping/simplequicping.go b/internal/engine/experiment/simplequicping/simplequicping.go index eb1ebca..44d2f7e 100644 --- a/internal/engine/experiment/simplequicping/simplequicping.go +++ b/internal/engine/experiment/simplequicping/simplequicping.go @@ -112,12 +112,11 @@ var ( ) // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session + if measurement.Input == "" { return errNoInputProvided } diff --git a/internal/engine/experiment/simplequicping/simplequicping_test.go b/internal/engine/experiment/simplequicping/simplequicping_test.go index cdd4138..12bd5ce 100644 --- a/internal/engine/experiment/simplequicping/simplequicping_test.go +++ b/internal/engine/experiment/simplequicping/simplequicping_test.go @@ -65,7 +65,12 @@ func TestMeasurer_run(t *testing.T) { MockableLogger: model.DiscardLogger, } callbacks := model.NewPrinterCallbacks(model.DiscardLogger) - err := m.Run(ctx, sess, meas, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: meas, + Session: sess, + } + err := m.Run(ctx, args) return meas, m, err } diff --git a/internal/engine/experiment/sniblocking/sniblocking.go b/internal/engine/experiment/sniblocking/sniblocking.go index 4e4df18..9bb3a40 100644 --- a/internal/engine/experiment/sniblocking/sniblocking.go +++ b/internal/engine/experiment/sniblocking/sniblocking.go @@ -233,12 +233,10 @@ func maybeURLToSNI(input model.MeasurementTarget) (model.MeasurementTarget, erro } // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session m.mu.Lock() if m.cache == nil { m.cache = make(map[string]Subresult) diff --git a/internal/engine/experiment/sniblocking/sniblocking_test.go b/internal/engine/experiment/sniblocking/sniblocking_test.go index 63ec9c7..7424fbf 100644 --- a/internal/engine/experiment/sniblocking/sniblocking_test.go +++ b/internal/engine/experiment/sniblocking/sniblocking_test.go @@ -116,12 +116,12 @@ func TestMeasurerMeasureNoMeasurementInput(t *testing.T) { measurer := NewExperimentMeasurer(Config{ ControlSNI: "example.com", }) - err := measurer.Run( - context.Background(), - newsession(), - new(model.Measurement), - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &model.Measurement{}, + Session: newsession(), + } + err := measurer.Run(context.Background(), args) if err.Error() != "Experiment requires measurement.Input" { t.Fatal("not the error we expected") } @@ -136,12 +136,12 @@ func TestMeasurerMeasureWithInvalidInput(t *testing.T) { measurement := &model.Measurement{ Input: "\t", } - err := measurer.Run( - ctx, - newsession(), - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: newsession(), + } + err := measurer.Run(ctx, args) if err == nil { t.Fatal("expected an error here") } @@ -156,12 +156,12 @@ func TestMeasurerMeasureWithCancelledContext(t *testing.T) { measurement := &model.Measurement{ Input: "kernel.org", } - err := measurer.Run( - ctx, - newsession(), - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: newsession(), + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } diff --git a/internal/engine/experiment/stunreachability/stunreachability.go b/internal/engine/experiment/stunreachability/stunreachability.go index ba278c0..870500f 100644 --- a/internal/engine/experiment/stunreachability/stunreachability.go +++ b/internal/engine/experiment/stunreachability/stunreachability.go @@ -73,10 +73,10 @@ var errStunMissingPortInURL = errors.New("stun: missing port in URL") var errUnsupportedURLScheme = errors.New("stun: unsupported URL scheme") // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session tk := new(TestKeys) measurement.TestKeys = tk registerExtensions(measurement) diff --git a/internal/engine/experiment/stunreachability/stunreachability_test.go b/internal/engine/experiment/stunreachability/stunreachability_test.go index 0b6c2be..209fd18 100644 --- a/internal/engine/experiment/stunreachability/stunreachability_test.go +++ b/internal/engine/experiment/stunreachability/stunreachability_test.go @@ -32,12 +32,12 @@ func TestMeasurerExperimentNameVersion(t *testing.T) { func TestRunWithoutInput(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) measurement := new(model.Measurement) - err := measurer.Run( - context.Background(), - &mockable.Session{}, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, errStunMissingInput) { t.Fatal("not the error we expected", err) } @@ -47,12 +47,12 @@ func TestRunWithInvalidURL(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("\t") // <- invalid URL - err := measurer.Run( - context.Background(), - &mockable.Session{}, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := measurer.Run(context.Background(), args) if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { t.Fatal("not the error we expected", err) } @@ -62,12 +62,12 @@ func TestRunWithNoPort(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("stun://stun.ekiga.net") - err := measurer.Run( - context.Background(), - &mockable.Session{}, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, errStunMissingPortInURL) { t.Fatal("not the error we expected", err) } @@ -77,12 +77,12 @@ func TestRunWithUnsupportedURLScheme(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("https://stun.ekiga.net:3478") - err := measurer.Run( - context.Background(), - &mockable.Session{}, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, errUnsupportedURLScheme) { t.Fatal("not the error we expected", err) } @@ -92,14 +92,14 @@ func TestRunWithInput(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget(defaultInput) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: model.DiscardLogger, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } @@ -124,14 +124,14 @@ func TestCancelledContext(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget(defaultInput) - err := measurer.Run( - ctx, - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: model.DiscardLogger, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(ctx, args) if !errors.Is(err, nil) { // nil because we want to submit t.Fatal("not the error we expected", err) } @@ -166,14 +166,14 @@ func TestNewClientFailure(t *testing.T) { measurer := NewExperimentMeasurer(*config) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget(defaultInput) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: model.DiscardLogger, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, nil) { // nil because we want to submit t.Fatal("not the error we expected") } @@ -202,14 +202,14 @@ func TestStartFailure(t *testing.T) { measurer := NewExperimentMeasurer(*config) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget(defaultInput) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: model.DiscardLogger, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, nil) { // nil because we want to submit t.Fatal("not the error we expected") } @@ -242,14 +242,14 @@ func TestReadFailure(t *testing.T) { measurer := NewExperimentMeasurer(*config) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget(defaultInput) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: model.DiscardLogger, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, nil) { // nil because we want to submit t.Fatal("not the error we expected") } diff --git a/internal/engine/experiment/tcpping/tcpping.go b/internal/engine/experiment/tcpping/tcpping.go index c8cfbae..0354233 100644 --- a/internal/engine/experiment/tcpping/tcpping.go +++ b/internal/engine/experiment/tcpping/tcpping.go @@ -82,12 +82,10 @@ var ( ) // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session if measurement.Input == "" { return errNoInputProvided } diff --git a/internal/engine/experiment/tcpping/tcpping_test.go b/internal/engine/experiment/tcpping/tcpping_test.go index faec432..330b9cb 100644 --- a/internal/engine/experiment/tcpping/tcpping_test.go +++ b/internal/engine/experiment/tcpping/tcpping_test.go @@ -51,7 +51,12 @@ func TestMeasurer_run(t *testing.T) { MockableLogger: model.DiscardLogger, } callbacks := model.NewPrinterCallbacks(model.DiscardLogger) - err := m.Run(ctx, sess, meas, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: meas, + Session: sess, + } + err := m.Run(ctx, args) return meas, m, err } diff --git a/internal/engine/experiment/telegram/telegram.go b/internal/engine/experiment/telegram/telegram.go index c9a0902..d9f606e 100644 --- a/internal/engine/experiment/telegram/telegram.go +++ b/internal/engine/experiment/telegram/telegram.go @@ -101,8 +101,11 @@ func (m Measurer) ExperimentVersion() string { } // Run implements ExperimentMeasurer.Run -func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session + ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() urlgetter.RegisterExtensions(measurement) diff --git a/internal/engine/experiment/telegram/telegram_test.go b/internal/engine/experiment/telegram/telegram_test.go index 4d206bf..8a1417c 100644 --- a/internal/engine/experiment/telegram/telegram_test.go +++ b/internal/engine/experiment/telegram/telegram_test.go @@ -28,14 +28,14 @@ func TestNewExperimentMeasurer(t *testing.T) { func TestGood(t *testing.T) { measurer := telegram.NewExperimentMeasurer(telegram.Config{}) measurement := new(model.Measurement) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: log.Log, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } @@ -297,7 +297,12 @@ func TestWeConfigureWebChecksToFailOnHTTPError(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - if err := measurer.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := measurer.Run(ctx, args); err != nil { t.Fatal(err) } if called.Load() < 1 { diff --git a/internal/engine/experiment/tlsmiddlebox/measurer.go b/internal/engine/experiment/tlsmiddlebox/measurer.go index d157ab0..363e7a6 100644 --- a/internal/engine/experiment/tlsmiddlebox/measurer.go +++ b/internal/engine/experiment/tlsmiddlebox/measurer.go @@ -52,12 +52,10 @@ var ( ) // // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session if measurement.Input == "" { return errNoInputProvided } diff --git a/internal/engine/experiment/tlsmiddlebox/measurer_test.go b/internal/engine/experiment/tlsmiddlebox/measurer_test.go index f5bf815..318fd5d 100644 --- a/internal/engine/experiment/tlsmiddlebox/measurer_test.go +++ b/internal/engine/experiment/tlsmiddlebox/measurer_test.go @@ -38,7 +38,12 @@ func TestMeasurer_input_failure(t *testing.T) { }, } callbacks := model.NewPrinterCallbacks(model.DiscardLogger) - err := m.Run(ctx, sess, meas, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: meas, + Session: sess, + } + err := m.Run(ctx, args) return meas, m, err } diff --git a/internal/engine/experiment/tlsping/tlsping.go b/internal/engine/experiment/tlsping/tlsping.go index eda8023..de51864 100644 --- a/internal/engine/experiment/tlsping/tlsping.go +++ b/internal/engine/experiment/tlsping/tlsping.go @@ -112,12 +112,10 @@ var ( ) // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session if measurement.Input == "" { return errNoInputProvided } diff --git a/internal/engine/experiment/tlsping/tlsping_test.go b/internal/engine/experiment/tlsping/tlsping_test.go index 716549f..fad4388 100644 --- a/internal/engine/experiment/tlsping/tlsping_test.go +++ b/internal/engine/experiment/tlsping/tlsping_test.go @@ -58,7 +58,12 @@ func TestMeasurer_run(t *testing.T) { MockableLogger: model.DiscardLogger, } callbacks := model.NewPrinterCallbacks(model.DiscardLogger) - err := m.Run(ctx, sess, meas, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: meas, + Session: sess, + } + err := m.Run(ctx, args) return meas, m, err } diff --git a/internal/engine/experiment/tlstool/tlstool.go b/internal/engine/experiment/tlstool/tlstool.go index 812d081..9791b2a 100644 --- a/internal/engine/experiment/tlstool/tlstool.go +++ b/internal/engine/experiment/tlstool/tlstool.go @@ -78,12 +78,11 @@ var allMethods = []method{{ }} // Run implements ExperimentMeasurer.Run. -func (m Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session + // TODO(bassosimone): wondering whether this experiment should // actually be merged with sniblocking instead? tk := new(TestKeys) diff --git a/internal/engine/experiment/tlstool/tlstool_test.go b/internal/engine/experiment/tlstool/tlstool_test.go index 6f95dde..55a33bc 100644 --- a/internal/engine/experiment/tlstool/tlstool_test.go +++ b/internal/engine/experiment/tlstool/tlstool_test.go @@ -27,12 +27,12 @@ func TestRunWithExplicitSNI(t *testing.T) { }) measurement := new(model.Measurement) measurement.Input = "8.8.8.8:853" - err := measurer.Run( - ctx, - &mockable.Session{}, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -43,12 +43,12 @@ func TestRunWithImplicitSNI(t *testing.T) { measurer := tlstool.NewExperimentMeasurer(tlstool.Config{}) measurement := new(model.Measurement) measurement.Input = "dns.google:853" - err := measurer.Run( - ctx, - &mockable.Session{}, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -60,12 +60,12 @@ func TestRunWithCancelledContext(t *testing.T) { measurer := tlstool.NewExperimentMeasurer(tlstool.Config{}) measurement := new(model.Measurement) measurement.Input = "dns.google:853" - err := measurer.Run( - ctx, - &mockable.Session{}, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } diff --git a/internal/engine/experiment/tor/tor.go b/internal/engine/experiment/tor/tor.go index 19c8f50..ffd52fa 100644 --- a/internal/engine/experiment/tor/tor.go +++ b/internal/engine/experiment/tor/tor.go @@ -166,12 +166,10 @@ func (m *Measurer) ExperimentVersion() string { } // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session targets, err := m.gimmeTargets(ctx, sess) if err != nil { return err // fail the measurement if we cannot get any target diff --git a/internal/engine/experiment/tor/tor_test.go b/internal/engine/experiment/tor/tor_test.go index f2eabeb..4e02549 100644 --- a/internal/engine/experiment/tor/tor_test.go +++ b/internal/engine/experiment/tor/tor_test.go @@ -36,14 +36,14 @@ func TestMeasurerMeasureFetchTorTargetsError(t *testing.T) { measurer.fetchTorTargets = func(ctx context.Context, sess model.ExperimentSession, cc string) (map[string]model.OOAPITorTarget, error) { return nil, expected } - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &model.Measurement{}, + Session: &mockable.Session{ MockableLogger: log.Log, }, - new(model.Measurement), - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if !errors.Is(err, expected) { t.Fatal("not the error we expected") } @@ -55,14 +55,14 @@ func TestMeasurerMeasureFetchTorTargetsEmptyList(t *testing.T) { return nil, nil } measurement := new(model.Measurement) - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{ MockableLogger: log.Log, }, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } @@ -79,14 +79,14 @@ func TestMeasurerMeasureGoodWithMockedOrchestra(t *testing.T) { measurer.fetchTorTargets = func(ctx context.Context, sess model.ExperimentSession, cc string) (map[string]model.OOAPITorTarget, error) { return nil, nil } - err := measurer.Run( - context.Background(), - &mockable.Session{ + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: &model.Measurement{}, + Session: &mockable.Session{ MockableLogger: log.Log, }, - new(model.Measurement), - model.NewPrinterCallbacks(log.Log), - ) + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } @@ -99,12 +99,12 @@ func TestMeasurerMeasureGood(t *testing.T) { measurer := NewMeasurer(Config{}) sess := newsession() measurement := new(model.Measurement) - err := measurer.Run( - context.Background(), - sess, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } @@ -142,12 +142,12 @@ func TestMeasurerMeasureSanitiseOutput(t *testing.T) { key: staticPrivateTestingTarget, } measurement := new(model.Measurement) - err := measurer.Run( - context.Background(), - sess, - measurement, - model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: sess, + } + err := measurer.Run(context.Background(), args) if err != nil { t.Fatal(err) } diff --git a/internal/engine/experiment/torsf/integration_test.go b/internal/engine/experiment/torsf/integration_test.go index 80feb82..11e846d 100644 --- a/internal/engine/experiment/torsf/integration_test.go +++ b/internal/engine/experiment/torsf/integration_test.go @@ -34,7 +34,12 @@ func TestRunWithExistingTor(t *testing.T) { MockableLogger: log.Log, MockableTempDir: tempdir, } - if err = m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err = m.Run(ctx, args); err != nil { t.Fatal(err) } } diff --git a/internal/engine/experiment/torsf/torsf.go b/internal/engine/experiment/torsf/torsf.go index aef0f82..99ad54c 100644 --- a/internal/engine/experiment/torsf/torsf.go +++ b/internal/engine/experiment/torsf/torsf.go @@ -124,10 +124,10 @@ const maxRuntime = 600 * time.Second // set the relevant OONI error inside of the measurement and // return nil. This is important because the caller may not submit // the measurement if this method returns an error. -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session ptl, sfdialer, err := m.setup(ctx, sess.Logger()) if err != nil { // we cannot setup the experiment diff --git a/internal/engine/experiment/torsf/torsf_test.go b/internal/engine/experiment/torsf/torsf_test.go index e71c666..afdd81a 100644 --- a/internal/engine/experiment/torsf/torsf_test.go +++ b/internal/engine/experiment/torsf/torsf_test.go @@ -47,7 +47,12 @@ func TestFailureWithInvalidRendezvousMethod(t *testing.T) { callbacks := &model.PrinterCallbacks{ Logger: model.DiscardLogger, } - err := m.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := m.Run(ctx, args) if !errors.Is(err, ptx.ErrSnowflakeNoSuchRendezvousMethod) { t.Fatal("unexpected error", err) } @@ -70,7 +75,12 @@ func TestFailureToStartPTXListener(t *testing.T) { callbacks := &model.PrinterCallbacks{ Logger: model.DiscardLogger, } - if err := m.Run(ctx, sess, measurement, callbacks); !errors.Is(err, expected) { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := m.Run(ctx, args); !errors.Is(err, expected) { t.Fatal("not the error we expected", err) } if tk := measurement.TestKeys; tk != nil { @@ -108,7 +118,12 @@ func TestSuccessWithMockedTunnelStart(t *testing.T) { callbacks := &model.PrinterCallbacks{ Logger: model.DiscardLogger, } - if err := m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := m.Run(ctx, args); err != nil { t.Fatal(err) } if called.Load() != 1 { @@ -168,7 +183,12 @@ func TestWithCancelledContext(t *testing.T) { callbacks := &model.PrinterCallbacks{ Logger: model.DiscardLogger, } - if err := m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := m.Run(ctx, args); err != nil { t.Fatal(err) } tk := measurement.TestKeys.(*TestKeys) @@ -231,7 +251,12 @@ func TestFailureToStartTunnel(t *testing.T) { callbacks := &model.PrinterCallbacks{ Logger: model.DiscardLogger, } - if err := m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := m.Run(ctx, args); err != nil { t.Fatal(err) } tk := measurement.TestKeys.(*TestKeys) diff --git a/internal/engine/experiment/urlgetter/urlgetter.go b/internal/engine/experiment/urlgetter/urlgetter.go index 8a6593c..3414607 100644 --- a/internal/engine/experiment/urlgetter/urlgetter.go +++ b/internal/engine/experiment/urlgetter/urlgetter.go @@ -97,10 +97,10 @@ func (m Measurer) ExperimentVersion() string { } // Run implements model.ExperimentSession.Run -func (m Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session // When using the urlgetter experiment directly, there is a nonconfigurable // default timeout that applies. When urlgetter is used as a library, it's // instead the responsibility of the user of urlgetter to set timeouts. Note diff --git a/internal/engine/experiment/urlgetter/urlgetter_test.go b/internal/engine/experiment/urlgetter/urlgetter_test.go index eaa8f7b..f9e62da 100644 --- a/internal/engine/experiment/urlgetter/urlgetter_test.go +++ b/internal/engine/experiment/urlgetter/urlgetter_test.go @@ -23,10 +23,12 @@ func TestMeasurer(t *testing.T) { } measurement := new(model.Measurement) measurement.Input = "https://www.google.com" - err := m.Run( - ctx, &mockable.Session{}, - measurement, model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := m.Run(ctx, args) if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("not the error we expected") } @@ -60,10 +62,12 @@ func TestMeasurerDNSCache(t *testing.T) { } measurement := new(model.Measurement) measurement.Input = "https://www.google.com" - err := m.Run( - ctx, &mockable.Session{}, - measurement, model.NewPrinterCallbacks(log.Log), - ) + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: &mockable.Session{}, + } + err := m.Run(ctx, args) if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("not the error we expected") } diff --git a/internal/engine/experiment/vanillator/integration_test.go b/internal/engine/experiment/vanillator/integration_test.go index c569303..c86ae9e 100644 --- a/internal/engine/experiment/vanillator/integration_test.go +++ b/internal/engine/experiment/vanillator/integration_test.go @@ -34,7 +34,12 @@ func TestRunWithExistingTor(t *testing.T) { MockableLogger: log.Log, MockableTempDir: tempdir, } - if err = m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err = m.Run(ctx, args); err != nil { t.Fatal(err) } } diff --git a/internal/engine/experiment/vanillator/vanillator.go b/internal/engine/experiment/vanillator/vanillator.go index 593f07c..1b6d687 100644 --- a/internal/engine/experiment/vanillator/vanillator.go +++ b/internal/engine/experiment/vanillator/vanillator.go @@ -106,10 +106,10 @@ const maxRuntime = 200 * time.Second // set the relevant OONI error inside of the measurement and // return nil. This is important because the caller may not submit // the measurement if this method returns an error. -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session m.registerExtensions(measurement) start := time.Now() ctx, cancel := context.WithTimeout(ctx, maxRuntime) diff --git a/internal/engine/experiment/vanillator/vanillator_test.go b/internal/engine/experiment/vanillator/vanillator_test.go index e8cbdd4..fda2f5e 100644 --- a/internal/engine/experiment/vanillator/vanillator_test.go +++ b/internal/engine/experiment/vanillator/vanillator_test.go @@ -59,7 +59,12 @@ func TestSuccessWithMockedTunnelStart(t *testing.T) { callbacks := &model.PrinterCallbacks{ Logger: model.DiscardLogger, } - if err := m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := m.Run(ctx, args); err != nil { t.Fatal(err) } if called.Load() != 1 { @@ -113,7 +118,12 @@ func TestWithCancelledContext(t *testing.T) { callbacks := &model.PrinterCallbacks{ Logger: model.DiscardLogger, } - if err := m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := m.Run(ctx, args); err != nil { t.Fatal(err) } tk := measurement.TestKeys.(*TestKeys) @@ -170,7 +180,12 @@ func TestFailureToStartTunnel(t *testing.T) { callbacks := &model.PrinterCallbacks{ Logger: model.DiscardLogger, } - if err := m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := m.Run(ctx, args); err != nil { t.Fatal(err) } tk := measurement.TestKeys.(*TestKeys) diff --git a/internal/engine/experiment/webconnectivity/webconnectivity.go b/internal/engine/experiment/webconnectivity/webconnectivity.go index bdbea35..6792311 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity.go @@ -121,12 +121,11 @@ const ( ) // Run implements ExperimentMeasurer.Run. -func (m Measurer) Run( - ctx context.Context, - sess model.ExperimentSession, - measurement *model.Measurement, - callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session + ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() tk := new(TestKeys) diff --git a/internal/engine/experiment/webconnectivity/webconnectivity_test.go b/internal/engine/experiment/webconnectivity/webconnectivity_test.go index 6db3989..6b53693 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity_test.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity_test.go @@ -37,7 +37,12 @@ func TestSuccess(t *testing.T) { sess := newsession(t, true) measurement := &model.Measurement{Input: "http://www.example.com"} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -65,7 +70,12 @@ func TestMeasureWithCancelledContext(t *testing.T) { sess := newsession(t, true) measurement := &model.Measurement{Input: "http://www.example.com"} callbacks := model.NewPrinterCallbacks(log.Log) - if err := measurer.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := measurer.Run(ctx, args); err != nil { t.Fatal(err) } tk := measurement.TestKeys.(*webconnectivity.TestKeys) @@ -99,7 +109,12 @@ func TestMeasureWithNoInput(t *testing.T) { sess := newsession(t, true) measurement := &model.Measurement{Input: ""} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, webconnectivity.ErrNoInput) { t.Fatal(err) } @@ -127,7 +142,12 @@ func TestMeasureWithInputNotBeingAnURL(t *testing.T) { sess := newsession(t, true) measurement := &model.Measurement{Input: "\t\t\t\t\t\t"} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, webconnectivity.ErrInputIsNotAnURL) { t.Fatal(err) } @@ -155,7 +175,12 @@ func TestMeasureWithUnsupportedInput(t *testing.T) { sess := newsession(t, true) measurement := &model.Measurement{Input: "dnslookup://example.com"} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, webconnectivity.ErrUnsupportedInput) { t.Fatal(err) } @@ -183,7 +208,12 @@ func TestMeasureWithNoAvailableTestHelpers(t *testing.T) { sess := newsession(t, false) measurement := &model.Measurement{Input: "https://www.example.com"} callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if !errors.Is(err, webconnectivity.ErrNoAvailableTestHelpers) { t.Fatal(err) } diff --git a/internal/engine/experiment/whatsapp/whatsapp.go b/internal/engine/experiment/whatsapp/whatsapp.go index 24874e2..1a1e3bf 100644 --- a/internal/engine/experiment/whatsapp/whatsapp.go +++ b/internal/engine/experiment/whatsapp/whatsapp.go @@ -154,10 +154,11 @@ func (m Measurer) ExperimentVersion() string { } // Run implements ExperimentMeasurer.Run -func (m Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session + ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() urlgetter.RegisterExtensions(measurement) diff --git a/internal/engine/experiment/whatsapp/whatsapp_test.go b/internal/engine/experiment/whatsapp/whatsapp_test.go index 2162086..c6bb573 100644 --- a/internal/engine/experiment/whatsapp/whatsapp_test.go +++ b/internal/engine/experiment/whatsapp/whatsapp_test.go @@ -35,7 +35,12 @@ func TestSuccess(t *testing.T) { sess := &mockable.Session{MockableLogger: log.Log} measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -70,7 +75,12 @@ func TestFailureAllEndpoints(t *testing.T) { sess := &mockable.Session{MockableLogger: log.Log} measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - err := measurer.Run(ctx, sess, measurement, callbacks) + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + err := measurer.Run(ctx, args) if err != nil { t.Fatal(err) } @@ -598,7 +608,12 @@ func TestWeConfigureWebChecksCorrectly(t *testing.T) { } measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) - if err := measurer.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err := measurer.Run(ctx, args); err != nil { t.Fatal(err) } if called.Load() != 263 { diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index 449edc6..865a452 100644 --- a/internal/engine/experiment_integration_test.go +++ b/internal/engine/experiment_integration_test.go @@ -475,10 +475,7 @@ func (am *antaniMeasurer) ExperimentVersion() string { return "0.1.1" } -func (am *antaniMeasurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (am *antaniMeasurer) Run(ctx context.Context, args *model.ExperimentArgs) error { return nil } diff --git a/internal/experiment/webconnectivity/measurer.go b/internal/experiment/webconnectivity/measurer.go index 5a0b977..dc520cf 100644 --- a/internal/experiment/webconnectivity/measurer.go +++ b/internal/experiment/webconnectivity/measurer.go @@ -40,11 +40,13 @@ func (m *Measurer) ExperimentVersion() string { } // Run implements model.ExperimentMeasurer. -func (m *Measurer) Run(ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { // Reminder: when this function returns an error, the measurement result // WILL NOT be submitted to the OONI backend. You SHOULD only return an error // for fundamental errors (e.g., the input is invalid or missing). + _ = args.Callbacks + measurement := args.Measurement + sess := args.Session // make sure we have a cancellable context such that we can stop any // goroutine running in the background (e.g., priority.go's ones) diff --git a/internal/model/experiment.go b/internal/model/experiment.go index acaadde..1cba6c1 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -117,6 +117,19 @@ func (d PrinterCallbacks) OnProgress(percentage float64, message string) { d.Logger.Infof("[%5.1f%%] %s", percentage*100, message) } +// ExperimentArgs contains the arguments passed to an experiment. +type ExperimentArgs struct { + // Callbacks contains MANDATORY experiment callbacks. + Callbacks ExperimentCallbacks + + // Measurement is the MANDATORY measurement in which the experiment + // must write the results of the measurement. + Measurement *Measurement + + // Session is the MANDATORY session the experiment can use. + Session ExperimentSession +} + // ExperimentMeasurer is the interface that allows to run a // measurement for a specific experiment. type ExperimentMeasurer interface { @@ -133,10 +146,7 @@ type ExperimentMeasurer interface { // set the relevant OONI error inside of the measurement and // return nil. This is important because the caller WILL NOT submit // the measurement if this method returns an error. - Run( - ctx context.Context, sess ExperimentSession, - measurement *Measurement, callbacks ExperimentCallbacks, - ) error + Run(ctx context.Context, args *ExperimentArgs) error // GetSummaryKeys returns summary keys expected by ooni/probe-cli. GetSummaryKeys(*Measurement) (interface{}, error) diff --git a/internal/tutorial/experiment/torsf/chapter01/README.md b/internal/tutorial/experiment/torsf/chapter01/README.md index d357321..eebb606 100644 --- a/internal/tutorial/experiment/torsf/chapter01/README.md +++ b/internal/tutorial/experiment/torsf/chapter01/README.md @@ -211,7 +211,12 @@ need any fancy context and we pass a `context.Background` to `Run`. ```Go ctx := context.Background() - if err = m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err = m.Run(ctx, args); err != nil { log.WithError(err).Fatal("torsf experiment failed") } ``` diff --git a/internal/tutorial/experiment/torsf/chapter01/main.go b/internal/tutorial/experiment/torsf/chapter01/main.go index aef3716..d96c5f3 100644 --- a/internal/tutorial/experiment/torsf/chapter01/main.go +++ b/internal/tutorial/experiment/torsf/chapter01/main.go @@ -212,7 +212,12 @@ func main() { // // ```Go ctx := context.Background() - if err = m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err = m.Run(ctx, args); err != nil { log.WithError(err).Fatal("torsf experiment failed") } // ``` diff --git a/internal/tutorial/experiment/torsf/chapter02/README.md b/internal/tutorial/experiment/torsf/chapter02/README.md index c9a2fbd..36f3323 100644 --- a/internal/tutorial/experiment/torsf/chapter02/README.md +++ b/internal/tutorial/experiment/torsf/chapter02/README.md @@ -117,10 +117,10 @@ chapters, finally, we will modify this function until it is a minimal implementation of the `torsf` experiment. ```Go -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + _ = args.Measurement + sess := args.Session ``` As you can see, this is just a stub implementation that sleeps for one second and prints a logging message. diff --git a/internal/tutorial/experiment/torsf/chapter02/main.go b/internal/tutorial/experiment/torsf/chapter02/main.go index 4464bdc..efded79 100644 --- a/internal/tutorial/experiment/torsf/chapter02/main.go +++ b/internal/tutorial/experiment/torsf/chapter02/main.go @@ -54,7 +54,12 @@ func main() { MockableLogger: log.Log, MockableTempDir: tempdir, } - if err = m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err = m.Run(ctx, args); err != nil { log.WithError(err).Fatal("torsf experiment failed") } data, err := json.Marshal(measurement) diff --git a/internal/tutorial/experiment/torsf/chapter02/torsf.go b/internal/tutorial/experiment/torsf/chapter02/torsf.go index af3e0f6..2547cb4 100644 --- a/internal/tutorial/experiment/torsf/chapter02/torsf.go +++ b/internal/tutorial/experiment/torsf/chapter02/torsf.go @@ -93,10 +93,10 @@ func (m *Measurer) ExperimentVersion() string { // minimal implementation of the `torsf` experiment. // // ```Go -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + _ = args.Callbacks + _ = args.Measurement + sess := args.Session // ``` // As you can see, this is just a stub implementation that sleeps // for one second and prints a logging message. diff --git a/internal/tutorial/experiment/torsf/chapter03/README.md b/internal/tutorial/experiment/torsf/chapter03/README.md index b6b9f85..3ae6719 100644 --- a/internal/tutorial/experiment/torsf/chapter03/README.md +++ b/internal/tutorial/experiment/torsf/chapter03/README.md @@ -32,10 +32,10 @@ print periodic updates via the `callbacks`. We will defer the real work to a private function called `run`. ```Go -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session ``` Let's create an instance of `TestKeys` and let's modify diff --git a/internal/tutorial/experiment/torsf/chapter03/main.go b/internal/tutorial/experiment/torsf/chapter03/main.go index a8dad28..1e9c8ec 100644 --- a/internal/tutorial/experiment/torsf/chapter03/main.go +++ b/internal/tutorial/experiment/torsf/chapter03/main.go @@ -28,7 +28,12 @@ func main() { MockableLogger: log.Log, MockableTempDir: tempdir, } - if err = m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err = m.Run(ctx, args); err != nil { log.WithError(err).Fatal("torsf experiment failed") } data, err := json.Marshal(measurement) diff --git a/internal/tutorial/experiment/torsf/chapter03/torsf.go b/internal/tutorial/experiment/torsf/chapter03/torsf.go index 97b7ae4..e48ca3a 100644 --- a/internal/tutorial/experiment/torsf/chapter03/torsf.go +++ b/internal/tutorial/experiment/torsf/chapter03/torsf.go @@ -65,10 +65,10 @@ type TestKeys struct { // real work to a private function called `run`. // // ```Go -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session // ``` // // Let's create an instance of `TestKeys` and let's modify diff --git a/internal/tutorial/experiment/torsf/chapter04/main.go b/internal/tutorial/experiment/torsf/chapter04/main.go index a8dad28..1e9c8ec 100644 --- a/internal/tutorial/experiment/torsf/chapter04/main.go +++ b/internal/tutorial/experiment/torsf/chapter04/main.go @@ -28,7 +28,12 @@ func main() { MockableLogger: log.Log, MockableTempDir: tempdir, } - if err = m.Run(ctx, sess, measurement, callbacks); err != nil { + args := &model.ExperimentArgs{ + Callbacks: callbacks, + Measurement: measurement, + Session: sess, + } + if err = m.Run(ctx, args); err != nil { log.WithError(err).Fatal("torsf experiment failed") } data, err := json.Marshal(measurement) diff --git a/internal/tutorial/experiment/torsf/chapter04/torsf.go b/internal/tutorial/experiment/torsf/chapter04/torsf.go index d6998b9..d350ee9 100644 --- a/internal/tutorial/experiment/torsf/chapter04/torsf.go +++ b/internal/tutorial/experiment/torsf/chapter04/torsf.go @@ -99,10 +99,10 @@ type TestKeys struct { } // Run implements ExperimentMeasurer.Run. -func (m *Measurer) Run( - ctx context.Context, sess model.ExperimentSession, - measurement *model.Measurement, callbacks model.ExperimentCallbacks, -) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { + callbacks := args.Callbacks + measurement := args.Measurement + sess := args.Session testkeys := &TestKeys{} measurement.TestKeys = testkeys start := time.Now() From 508c4293d594735b67caaa5ad944374062b7af23 Mon Sep 17 00:00:00 2001 From: ooninoob Date: Wed, 23 Nov 2022 10:56:06 +0100 Subject: [PATCH 14/14] Add -tls-proxy-outbound-port flag to jafar (useful for non-HTTPS protocols) Edit README to make it explicit that tlsproxy has nothing to do with HTTP and can be used with any TCP protocol that does TLS handshakes. --- internal/cmd/jafar/README.md | 8 +++++--- internal/cmd/jafar/main.go | 13 +++++++++---- internal/cmd/jafar/tlsproxy/tlsproxy.go | 14 ++++++++++---- internal/cmd/jafar/tlsproxy/tlsproxy_test.go | 4 ++-- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/internal/cmd/jafar/README.md b/internal/cmd/jafar/README.md index 8b4722a..a769753 100644 --- a/internal/cmd/jafar/README.md +++ b/internal/cmd/jafar/README.md @@ -156,14 +156,16 @@ response for every request whose `Host` contains the specified string. ### tls-proxy -TLS proxy is a proxy that routes traffic to specific servers depending +TLS proxy is a TCP proxy that routes traffic to specific servers depending on their SNI value. It is controlled by the following flags: ```bash -tls-proxy-address string - Address where the HTTP proxy should listen (default "127.0.0.1:443") + Address where the TCP+TLS proxy should listen (default "127.0.0.1:443") -tls-proxy-block value - Register keyword triggering TLS censorship + Register SNI header keyword triggering TLS censorship + -tls-proxy-outbound-port + Define the outbound port requests are proxied to (default "443 for HTTPS) ``` The `-tls-proxy-address` flags has the same semantics it has for the DNS diff --git a/internal/cmd/jafar/main.go b/internal/cmd/jafar/main.go index 61f5285..7d376d8 100644 --- a/internal/cmd/jafar/main.go +++ b/internal/cmd/jafar/main.go @@ -58,8 +58,9 @@ var ( tag *string - tlsProxyAddress *string - tlsProxyBlock flagx.StringArray + tlsProxyAddress *string + tlsProxyBlock flagx.StringArray + tlsProxyOutboundPort *string uncensoredResolverDoH *string ) @@ -159,12 +160,16 @@ func init() { // tlsProxy tlsProxyAddress = flag.String( "tls-proxy-address", "127.0.0.1:443", - "Address where the HTTP proxy should listen", + "Address where the TCP+TLS proxy should listen", ) flag.Var( &tlsProxyBlock, "tls-proxy-block", "Register keyword triggering TLS censorship", ) + tlsProxyOutboundPort = flag.String( + "tls-proxy-outbound-port", "443", + "The outbound port where requests should be proxied", + ) // uncensored uncensoredResolverDoH = flag.String( @@ -227,7 +232,7 @@ func iptablesStart() *iptables.CensoringPolicy { } func tlsProxyStart(uncensored *uncensored.Client) net.Listener { - proxy := tlsproxy.NewCensoringProxy(tlsProxyBlock, uncensored) + proxy := tlsproxy.NewCensoringProxy(tlsProxyBlock, uncensored, tlsProxyOutboundPort) listener, err := proxy.Start(*tlsProxyAddress) runtimex.PanicOnError(err, "proxy.Start failed") return listener diff --git a/internal/cmd/jafar/tlsproxy/tlsproxy.go b/internal/cmd/jafar/tlsproxy/tlsproxy.go index 91cede5..d98751f 100644 --- a/internal/cmd/jafar/tlsproxy/tlsproxy.go +++ b/internal/cmd/jafar/tlsproxy/tlsproxy.go @@ -21,8 +21,9 @@ type Dialer interface { // CensoringProxy is a censoring TLS proxy type CensoringProxy struct { - keywords []string - dial func(network, address string) (net.Conn, error) + keywords []string + dial func(network, address string) (net.Conn, error) + outboundPort string } // NewCensoringProxy creates a new CensoringProxy instance using @@ -31,13 +32,18 @@ type CensoringProxy struct { // the SNII record of a ClientHello. dnsNetwork and dnsAddress are // settings to configure the upstream, non censored DNS. func NewCensoringProxy( - keywords []string, uncensored Dialer, + keywords []string, uncensored Dialer, outboundPort *string, ) *CensoringProxy { + defaultPort := "443" + if outboundPort == nil { + outboundPort = &defaultPort + } return &CensoringProxy{ keywords: keywords, dial: func(network, address string) (net.Conn, error) { return uncensored.DialContext(context.Background(), network, address) }, + outboundPort: *outboundPort, } } @@ -146,7 +152,7 @@ func (p *CensoringProxy) handle(clientconn net.Conn) { return } } - serverconn, err := p.dial("tcp", net.JoinHostPort(sni, "443")) + serverconn, err := p.dial("tcp", net.JoinHostPort(sni, p.outboundPort)) if err != nil { log.WithError(err).Warn("tlsproxy: p.dial failed") alertclose(clientconn) diff --git a/internal/cmd/jafar/tlsproxy/tlsproxy_test.go b/internal/cmd/jafar/tlsproxy/tlsproxy_test.go index 86a21d8..1394ba6 100644 --- a/internal/cmd/jafar/tlsproxy/tlsproxy_test.go +++ b/internal/cmd/jafar/tlsproxy/tlsproxy_test.go @@ -94,7 +94,7 @@ func TestFailWriteAfterConnect(t *testing.T) { func TestListenError(t *testing.T) { proxy := NewCensoringProxy( - []string{""}, uncensored.NewClient("https://1.1.1.1/dns-query"), + []string{""}, uncensored.NewClient("https://1.1.1.1/dns-query"), nil, ) listener, err := proxy.Start("8.8.8.8:80") if err == nil { @@ -107,7 +107,7 @@ func TestListenError(t *testing.T) { func newproxy(t *testing.T, blocked string) net.Listener { proxy := NewCensoringProxy( - []string{blocked}, uncensored.NewClient("https://1.1.1.1/dns-query"), + []string{blocked}, uncensored.NewClient("https://1.1.1.1/dns-query"), nil, ) listener, err := proxy.Start("127.0.0.1:0") if err != nil {