diff --git a/internal/experiment/tlsmiddlebox/config.go b/internal/experiment/tlsmiddlebox/config.go index 7649dfc20b..6de3f01f2a 100644 --- a/internal/experiment/tlsmiddlebox/config.go +++ b/internal/experiment/tlsmiddlebox/config.go @@ -27,7 +27,7 @@ type Config struct { TestHelper string `ooni:"testhelper URL to use for tracing"` // ClientId is the client fingerprint to use - ClientId int `ooni:"ClientHello fingerprint to use"` + ClientId int64 `ooni:"ClientHello fingerprint to use"` } func (c Config) resolverURL() string { @@ -73,7 +73,7 @@ func (c Config) testhelper(address string) (URL *url.URL, err error) { func (c Config) clientid() int { if c.ClientId > 0 { - return c.ClientId + return int(c.ClientId) } return 0 } diff --git a/internal/experiment/tlsmiddlebox/config_test.go b/internal/experiment/tlsmiddlebox/config_test.go index f743d34d1f..0a3eb21ac4 100644 --- a/internal/experiment/tlsmiddlebox/config_test.go +++ b/internal/experiment/tlsmiddlebox/config_test.go @@ -76,8 +76,19 @@ func TestConfig_testhelper(t *testing.T) { } func TestConfig_clientid(t *testing.T) { - c := Config{} - if c.clientid() != 0 { - t.Fatal("invalid default ClientHello ID") - } + t.Run("without config", func(t *testing.T) { + c := Config{} + if c.clientid() != 0 { + t.Fatal("invalid default ClientHello ID") + } + }) + + t.Run("with config", func(t *testing.T) { + c := Config{ + ClientId: 2, + } + if c.clientid() != 2 { + t.Fatal("invalid ClientHello ID") + } + }) } diff --git a/internal/experiment/tlsmiddlebox/measurer.go b/internal/experiment/tlsmiddlebox/measurer.go index cda743756b..eacd694a8e 100644 --- a/internal/experiment/tlsmiddlebox/measurer.go +++ b/internal/experiment/tlsmiddlebox/measurer.go @@ -16,7 +16,7 @@ import ( const ( testName = "tlsmiddlebox" - testVersion = "0.1.2" + testVersion = "0.1.3" ) // Measurer performs the measurement. @@ -49,6 +49,9 @@ var ( // errInvalidTHScheme indicates that the TH scheme is invalid errInvalidTHScheme = errors.New("th scheme must be tlshandshake") + + // errInvalidClientId indicates that the ClientId is invalid + errInvalidClientId = errors.New("ClientId does not match any known fingerprint") ) // // Run implements ExperimentMeasurer.Run. @@ -73,6 +76,9 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { if th.Scheme != "tlshandshake" { return errInvalidTHScheme } + if clientId := m.config.clientid(); clientId > 0 && ClientIDs[clientId] == nil { + return errInvalidClientId + } tk := NewTestKeys() measurement.TestKeys = tk wg := new(sync.WaitGroup) diff --git a/internal/experiment/tlsmiddlebox/measurer_test.go b/internal/experiment/tlsmiddlebox/measurer_test.go index bc187c774e..ee6d41f62b 100644 --- a/internal/experiment/tlsmiddlebox/measurer_test.go +++ b/internal/experiment/tlsmiddlebox/measurer_test.go @@ -18,7 +18,7 @@ func TestMeasurerExperimentNameVersion(t *testing.T) { if measurer.ExperimentName() != "tlsmiddlebox" { t.Fatal("unexpected ExperimentName") } - if measurer.ExperimentVersion() != "0.1.2" { + if measurer.ExperimentVersion() != "0.1.3" { t.Fatal("unexpected ExperimentVersion") } } @@ -82,6 +82,29 @@ func TestMeasurer_input_failure(t *testing.T) { } }) + t.Run("with invalid ClientId", func(t *testing.T) { + m := NewExperimentMeasurer(Config{ + ClientId: 5, // we only know fingerprints between 1 and 4 + }) + meas := &model.Measurement{ + Input: model.MeasurementInput("tlstrace://example.com"), + } + sess := &mocks.Session{ + MockLogger: func() model.Logger { + return model.DiscardLogger + }, + } + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(model.DiscardLogger), + Measurement: meas, + Session: sess, + } + err := m.Run(context.Background(), args) + if !errors.Is(err, errInvalidClientId) { + t.Fatal("unexpected error", err) + } + }) + t.Run("with local listener and successful outcome", func(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index 7eca9e0379..cb6d2ef0bb 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -1115,3 +1115,29 @@ func TestExperimentConfigIsAlwaysAPointerToStruct(t *testing.T) { }) } } + +// This test is important because SetOptionAny can only set fields whose +// kind is int64, bool, or string: any config field exposed as an option +// through the `ooni` tag must use one of these kinds, otherwise it is +// impossible to set it with `miniooni -O` and similar interfaces +func TestExperimentOptionsAreAlwaysSettable(t *testing.T) { + for name, ffunc := range AllExperiments { + t.Run(name, func(t *testing.T) { + factory := ffunc() + valueinfo := reflect.ValueOf(factory.config).Elem() + typeinfo := valueinfo.Type() + for i := 0; i < typeinfo.NumField(); i++ { + field := typeinfo.Field(i) + if !field.IsExported() || field.Tag.Get("ooni") == "" { + continue + } + switch kind := field.Type.Kind(); kind { + case reflect.Int64, reflect.Bool, reflect.String: + // nothing + default: + t.Fatalf("field %s has kind %s, which SetOptionAny cannot set", field.Name, kind) + } + } + }) + } +}