MVP of a signal messenger test (#230)

* MVP of a signal messenger test

* Add minimal signal test unit tests

* Add Signal test to the im nettest group

* Add test for https://sfu.voip.signal.org/

* Fix bug in client-side determination of blocking status

* Add uptime.signal.org to the test targets

* Add more tests

* Check for invalid CA being passed
* Check that the update function works as expected

* Update internal/engine/experiment/signal/signal_test.go

Co-authored-by: Simone Basso <bassosimone@gmail.com>

* fix: back out URL we shouldn't have changed

When merging probe-engine into probe-cli, we changed too many URLs
and some of them should not have been changed.

I noticed this during the review of Signal and I choose to add
this commit to revert such changes.

While there, make sure the URL of the experiment is OK.

* fix(signal): reach 100% of coverage

Just so that we can focus on areas of the codebase where we need
more coverage, let us avoid missing an easy line to test.

Co-authored-by: Simone Basso <bassosimone@gmail.com>

cmd/ooniprobe/internal/nettests/groups.go

@@ -35,6 +35,7 @@ var All = map[string]Group{
 		Label: "Instant Messaging",
 		Nettests: []Nettest{
 			FacebookMessenger{},
+			Signal{},
 			Telegram{},
 			WhatsApp{},
 		},

cmd/ooniprobe/internal/nettests/signal.go

@@ -0,0 +1,17 @@
+package nettests
+
+// Signal test implementation
+type Signal struct {
+}
+
+// Run starts the test
+func (h Signal) Run(ctl *Controller) error {
+	builder, err := ctl.Session.NewExperimentBuilder(
+		"signal",
+	)
+	if err != nil {
+		return err
+	}
+
+	return ctl.Run(builder, []string{""})
+}

internal/engine/allexperiments.go

@@ -14,6 +14,7 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/psiphon"
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/riseupvpn"
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/run"
+	"github.com/ooni/probe-cli/v3/internal/engine/experiment/signal"
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/sniblocking"
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/stunreachability"
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/telegram"
@@ -85,7 +86,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 	// TODO(bassosimone): when we can set experiment options using the JSON
 	// we need to get rid of all these multiple experiments.
 	//
-	// See https://github.com/ooni/probe-cli/v3/internal/engine/issues/413
+	// See https://github.com/ooni/probe-engine/issues/413
 	"example_with_input_non_interruptible": func(session *Session) *ExperimentBuilder {
 		return &ExperimentBuilder{
 			build: func(config interface{}) *Experiment {
@@ -216,6 +217,18 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 		}
 	},
 
+	"signal": func(session *Session) *ExperimentBuilder {
+		return &ExperimentBuilder{
+			build: func(config interface{}) *Experiment {
+				return NewExperiment(session, signal.NewExperimentMeasurer(
+					*config.(*signal.Config),
+				))
+			},
+			config:      &signal.Config{},
+			inputPolicy: InputNone,
+		}
+	},
+
 	"sni_blocking": func(session *Session) *ExperimentBuilder {
 		return &ExperimentBuilder{
 			build: func(config interface{}) *Experiment {

internal/engine/experiment/riseupvpn/riseupvpn.go

@@ -186,7 +186,7 @@ func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
 	inputs := []urlgetter.MultiInput{
 
 		// Here we need to provide the method explicitly. See
-		// https://github.com/ooni/probe-cli/v3/internal/engine/issues/827.
+		// https://github.com/ooni/probe-engine/issues/827.
 		{Target: providerURL, Config: urlgetter.Config{
 			CertPool:        certPool,
 			Method:          "GET",

internal/engine/experiment/signal/signal.go

@@ -0,0 +1,194 @@
+// Package signal contains the Signal network experiment.
+//
+// See https://github.com/ooni/spec/blob/master/nettests/ts-029-signal.md.
+package signal
+
+import (
+	"context"
+	"errors"
+	"time"
+
+	"github.com/ooni/probe-cli/v3/internal/engine/experiment/urlgetter"
+	"github.com/ooni/probe-cli/v3/internal/engine/model"
+	"github.com/ooni/probe-cli/v3/internal/engine/netx"
+)
+
+const (
+	testName    = "signal"
+	testVersion = "0.2.0"
+
+	signalCA = `-----BEGIN CERTIFICATE-----
+MIID7zCCAtegAwIBAgIJAIm6LatK5PNiMA0GCSqGSIb3DQEBBQUAMIGNMQswCQYD
+VQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5j
+aXNjbzEdMBsGA1UECgwUT3BlbiBXaGlzcGVyIFN5c3RlbXMxHTAbBgNVBAsMFE9w
+ZW4gV2hpc3BlciBTeXN0ZW1zMRMwEQYDVQQDDApUZXh0U2VjdXJlMB4XDTEzMDMy
+NTIyMTgzNVoXDTIzMDMyMzIyMTgzNVowgY0xCzAJBgNVBAYTAlVTMRMwEQYDVQQI
+DApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNpc2NvMR0wGwYDVQQKDBRP
+cGVuIFdoaXNwZXIgU3lzdGVtczEdMBsGA1UECwwUT3BlbiBXaGlzcGVyIFN5c3Rl
+bXMxEzARBgNVBAMMClRleHRTZWN1cmUwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAw
+ggEKAoIBAQDBSWBpOCBDF0i4q2d4jAXkSXUGpbeWugVPQCjaL6qD9QDOxeW1afvf
+Po863i6Crq1KDxHpB36EwzVcjwLkFTIMeo7t9s1FQolAt3mErV2U0vie6Ves+yj6
+grSfxwIDAcdsKmI0a1SQCZlr3Q1tcHAkAKFRxYNawADyps5B+Zmqcgf653TXS5/0
+IPPQLocLn8GWLwOYNnYfBvILKDMItmZTtEbucdigxEA9mfIvvHADEbteLtVgwBm9
+R5vVvtwrD6CCxI3pgH7EH7kMP0Od93wLisvn1yhHY7FuYlrkYqdkMvWUrKoASVw4
+jb69vaeJCUdU+HCoXOSP1PQcL6WenNCHAgMBAAGjUDBOMB0GA1UdDgQWBBQBixjx
+P/s5GURuhYa+lGUypzI8kDAfBgNVHSMEGDAWgBQBixjxP/s5GURuhYa+lGUypzI8
+kDAMBgNVHRMEBTADAQH/MA0GCSqGSIb3DQEBBQUAA4IBAQB+Hr4hC56m0LvJAu1R
+K6NuPDbTMEN7/jMojFHxH4P3XPFfupjR+bkDq0pPOU6JjIxnrD1XD/EVmTTaTVY5
+iOheyv7UzJOefb2pLOc9qsuvI4fnaESh9bhzln+LXxtCrRPGhkxA1IMIo3J/s2WF
+/KVYZyciu6b4ubJ91XPAuBNZwImug7/srWvbpk0hq6A6z140WTVSKtJG7EP41kJe
+/oF4usY5J7LPkxK3LWzMJnb5EIJDmRvyH8pyRwWg6Qm6qiGFaI4nL8QU4La1x2en
+4DGXRaLMPRwjELNgQPodR38zoCMuA8gHZfZYYoZ7D7Q1wNUiVHcxuFrEeBaYJbLE
+rwLV
+-----END CERTIFICATE-----`
+)
+
+// Config contains the signal experiment config.
+type Config struct {
+	// SignalCA is used to pass in a custom CA in testing
+	SignalCA string
+}
+
+// TestKeys contains signal test keys.
+type TestKeys struct {
+	urlgetter.TestKeys
+	SignalBackendStatus  string  `json:"signal_backend_status"`
+	SignalBackendFailure *string `json:"signal_backend_failure"`
+}
+
+// NewTestKeys creates new signal TestKeys.
+func NewTestKeys() *TestKeys {
+	return &TestKeys{
+		SignalBackendStatus:  "ok",
+		SignalBackendFailure: nil,
+	}
+}
+
+// Update updates the TestKeys using the given MultiOutput result.
+func (tk *TestKeys) Update(v urlgetter.MultiOutput) {
+	// update the easy to update entries first
+	tk.NetworkEvents = append(tk.NetworkEvents, v.TestKeys.NetworkEvents...)
+	tk.Queries = append(tk.Queries, v.TestKeys.Queries...)
+	tk.Requests = append(tk.Requests, v.TestKeys.Requests...)
+	tk.TCPConnect = append(tk.TCPConnect, v.TestKeys.TCPConnect...)
+	tk.TLSHandshakes = append(tk.TLSHandshakes, v.TestKeys.TLSHandshakes...)
+	// Ignore the result of the uptime DNS lookup
+	if v.Input.Target == "dnslookup://uptime.signal.org" {
+		return
+	}
+	if v.TestKeys.Failure != nil {
+		tk.SignalBackendStatus = "blocked"
+		tk.SignalBackendFailure = v.TestKeys.Failure
+		return
+	}
+	return
+}
+
+// Measurer performs the measurement
+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
+}
+
+// Run implements ExperimentMeasurer.Run
+func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
+	measurement *model.Measurement, callbacks model.ExperimentCallbacks) error {
+	ctx, cancel := context.WithTimeout(ctx, 60*time.Second)
+	defer cancel()
+	urlgetter.RegisterExtensions(measurement)
+
+	certPool := netx.NewDefaultCertPool()
+	signalCABytes := []byte(signalCA)
+	if m.Config.SignalCA != "" {
+		signalCABytes = []byte(m.Config.SignalCA)
+	}
+	if certPool.AppendCertsFromPEM(signalCABytes) == false {
+		return errors.New("AppendCertsFromPEM failed")
+	}
+
+	inputs := []urlgetter.MultiInput{
+		// Here we need to provide the method explicitly. See
+		// https://github.com/ooni/probe-engine/issues/827.
+		{Target: "https://textsecure-service.whispersystems.org/", Config: urlgetter.Config{
+			Method:          "GET",
+			FailOnHTTPError: false,
+			CertPool:        certPool,
+		}},
+		{Target: "https://storage.signal.org/", Config: urlgetter.Config{
+			Method:          "GET",
+			FailOnHTTPError: false,
+			CertPool:        certPool,
+		}},
+		{Target: "https://api.directory.signal.org/", Config: urlgetter.Config{
+			Method:          "GET",
+			FailOnHTTPError: false,
+			CertPool:        certPool,
+		}},
+		{Target: "https://cdn.signal.org/", Config: urlgetter.Config{
+			Method:          "GET",
+			FailOnHTTPError: false,
+			CertPool:        certPool,
+		}},
+		{Target: "https://cdn2.signal.org/", Config: urlgetter.Config{
+			Method:          "GET",
+			FailOnHTTPError: false,
+			CertPool:        certPool,
+		}},
+		{Target: "https://sfu.voip.signal.org/", Config: urlgetter.Config{
+			Method:          "GET",
+			FailOnHTTPError: false,
+			CertPool:        certPool,
+		}},
+		{Target: "dnslookup://uptime.signal.org"},
+	}
+	multi := urlgetter.Multi{Begin: time.Now(), Getter: m.Getter, Session: sess}
+	testkeys := NewTestKeys()
+	testkeys.Agent = "redirect"
+	measurement.TestKeys = testkeys
+	for entry := range multi.Collect(ctx, inputs, "signal", callbacks) {
+		testkeys.Update(entry)
+	}
+	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 probe-cli
+// therefore we should be careful when changing it.
+type SummaryKeys struct {
+	SignalBackendStatus  string  `json:"signal_backend_status"`
+	SignalBackendFailure *string `json:"signal_backend_failure"`
+	IsAnomaly            bool    `json:"-"`
+}
+
+// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
+func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
+	sk := SummaryKeys{IsAnomaly: false}
+	tk, ok := measurement.TestKeys.(*TestKeys)
+	if !ok {
+		return nil, errors.New("invalid test keys type")
+	}
+	sk.SignalBackendStatus = tk.SignalBackendStatus
+	sk.SignalBackendFailure = tk.SignalBackendFailure
+	sk.IsAnomaly = tk.SignalBackendStatus == "blocking"
+	return sk, nil
+}

internal/engine/experiment/signal/signal_test.go

@@ -0,0 +1,129 @@
+package signal_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/apex/log"
+	"github.com/ooni/probe-cli/v3/internal/engine/experiment/signal"
+	"github.com/ooni/probe-cli/v3/internal/engine/experiment/urlgetter"
+	"github.com/ooni/probe-cli/v3/internal/engine/internal/mockable"
+	"github.com/ooni/probe-cli/v3/internal/engine/model"
+	"github.com/ooni/probe-cli/v3/internal/engine/netx/errorx"
+)
+
+func TestNewExperimentMeasurer(t *testing.T) {
+	measurer := signal.NewExperimentMeasurer(signal.Config{})
+	if measurer.ExperimentName() != "signal" {
+		t.Fatal("unexpected name")
+	}
+	if measurer.ExperimentVersion() != "0.2.0" {
+		t.Fatal("unexpected version")
+	}
+}
+
+func TestGood(t *testing.T) {
+	measurer := signal.NewExperimentMeasurer(signal.Config{})
+	measurement := new(model.Measurement)
+	err := measurer.Run(
+		context.Background(),
+		&mockable.Session{
+			MockableLogger: log.Log,
+		},
+		measurement,
+		model.NewPrinterCallbacks(log.Log),
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	tk := measurement.TestKeys.(*signal.TestKeys)
+	if tk.Agent != "redirect" {
+		t.Fatal("unexpected Agent")
+	}
+	if tk.FailedOperation != nil {
+		t.Fatal("unexpected FailedOperation")
+	}
+	if tk.Failure != nil {
+		t.Fatal("unexpected Failure")
+	}
+	if len(tk.NetworkEvents) <= 0 {
+		t.Fatal("no NetworkEvents?!")
+	}
+	if len(tk.Queries) <= 0 {
+		t.Fatal("no Queries?!")
+	}
+	if len(tk.Requests) <= 0 {
+		t.Fatal("no Requests?!")
+	}
+	if len(tk.TCPConnect) <= 0 {
+		t.Fatal("no TCPConnect?!")
+	}
+	if len(tk.TLSHandshakes) <= 0 {
+		t.Fatal("no TLSHandshakes?!")
+	}
+	if tk.SignalBackendFailure != nil {
+		t.Fatal("unexpected SignalBackendFailure")
+	}
+	if tk.SignalBackendStatus != "ok" {
+		t.Fatal("unexpected SignalBackendStatus")
+	}
+	sk, err := measurer.GetSummaryKeys(measurement)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, ok := sk.(signal.SummaryKeys); !ok {
+		t.Fatal("invalid type for summary keys")
+	}
+}
+
+func TestUpdate(t *testing.T) {
+	tk := signal.NewTestKeys()
+	tk.Update(urlgetter.MultiOutput{
+		Input: urlgetter.MultiInput{
+			Config: urlgetter.Config{Method: "GET"},
+			Target: "https://textsecure-service.whispersystems.org/",
+		},
+		TestKeys: urlgetter.TestKeys{
+			Failure: (func() *string {
+				s := errorx.FailureEOFError
+				return &s
+			})(),
+		},
+	})
+	if tk.SignalBackendStatus != "blocked" {
+		t.Fatal("SignalBackendStatus should be blocked")
+	}
+	if *tk.SignalBackendFailure != errorx.FailureEOFError {
+		t.Fatal("invalid SignalBackendError")
+	}
+}
+
+func TestBadSignalCA(t *testing.T) {
+	measurer := signal.NewExperimentMeasurer(signal.Config{
+		SignalCA: "INVALIDCA",
+	})
+	measurement := new(model.Measurement)
+	err := measurer.Run(
+		context.Background(),
+		&mockable.Session{
+			MockableLogger: log.Log,
+		},
+		measurement,
+		model.NewPrinterCallbacks(log.Log),
+	)
+	if err.Error() != "AppendCertsFromPEM failed" {
+		t.Fatal("not the error we expected")
+	}
+}
+
+func TestGetSummaryInvalidType(t *testing.T) {
+	measurer := signal.Measurer{}
+	in := make(chan int)
+	out, err := measurer.GetSummaryKeys(&model.Measurement{TestKeys: in})
+	if err == nil || err.Error() != "invalid test keys type" {
+		t.Fatal("not the error we expected", err)
+	}
+	if out != nil {
+		t.Fatal("expected nil output here")
+	}
+}

internal/engine/experiment/sniblocking/sniblocking.go

@@ -260,7 +260,7 @@ func (m *Measurer) Run(
 	// probably want to perform the name resolution before the measurements
 	// or to make sure that the classify logic is robust to that.
 	//
-	// See https://github.com/ooni/probe-cli/v3/internal/engine/issues/392.
+	// See https://github.com/ooni/probe-engine/issues/392.
 	maybeParsed, err := maybeURLToSNI(measurement.Input)
 	if err != nil {
 		return err

internal/engine/experiment/stunreachability/stunreachability_test.go

@@ -28,7 +28,7 @@ func TestMeasurerExperimentNameVersion(t *testing.T) {
 
 func TestRun(t *testing.T) {
 	if os.Getenv("GITHUB_ACTIONS") == "true" {
-		// See https://github.com/ooni/probe-cli/v3/internal/engine/issues/874#issuecomment-679850652
+		// See https://github.com/ooni/probe-engine/issues/874#issuecomment-679850652
 		t.Skip("skipping broken test on GitHub Actions")
 	}
 	measurer := stunreachability.NewExperimentMeasurer(stunreachability.Config{})

internal/engine/experiment/telegram/telegram.go

@@ -120,7 +120,7 @@ func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
 		{Target: "http://149.154.171.5:443/", Config: urlgetter.Config{Method: "POST"}},
 
 		// Here we need to provide the method explicitly. See
-		// https://github.com/ooni/probe-cli/v3/internal/engine/issues/827.
+		// https://github.com/ooni/probe-engine/issues/827.
 		{Target: "http://web.telegram.org/", Config: urlgetter.Config{
 			Method:          "GET",
 			FailOnHTTPError: true,

internal/engine/experiment/webconnectivity/summary.go

@@ -132,7 +132,7 @@ func Summarize(tk *TestKeys) (out Summary) {
 		// TODO(bassosimone): MK flags this as accessible. This result is debateable. We
 		// are doing what MK does. But we most likely want to make it better later.
 		//
-		// See <https://github.com/ooni/probe-cli/v3/internal/engine/issues/579>.
+		// See <https://github.com/ooni/probe-engine/issues/579>.
 		out.Accessible = &accessible
 		out.Status |= StatusSuccessNXDOMAIN | StatusExperimentDNS
 		return

internal/engine/inputloader.go

@@ -156,7 +156,7 @@ func (il inputLoader) loadLocal() ([]model.URLInfo, error) {
 		if err != nil {
 			return nil, err
 		}
-		// See https://github.com/ooni/probe-cli/v3/internal/engine/issues/1123.
+		// See https://github.com/ooni/probe-engine/issues/1123.
 		if len(extra) <= 0 {
 			return nil, fmt.Errorf("%w: %s", ErrDetectedEmptyFile, filepath)
 		}

internal/engine/netx/netx.go

@@ -17,7 +17,7 @@
 // TLS handshake for DoT and DoH).
 //
 // We described the design and implementation of the most recent version of
-// this package at <https://github.com/ooni/probe-cli/v3/internal/engine/issues/359>. Such
+// this package at <https://github.com/ooni/probe-engine/issues/359>. Such
 // issue also links to a previous design document.
 package netx
 

pkg/oonimkall/task_integration_test.go

@@ -314,7 +314,7 @@ func TestMaxRuntime(t *testing.T) {
 	//
 	// In case there are further timeouts, e.g. in the sessionresolver, the
 	// time used by the experiment will be much more. This is for example the
-	// case in https://github.com/ooni/probe-cli/v3/internal/engine/issues/1005.
+	// case in https://github.com/ooni/probe-engine/issues/1005.
 	if time.Now().Sub(begin) > 10*time.Second {
 		t.Fatal("expected shorter runtime")
 	}