fix(tlsmiddlebox): make ClientId settable and validate its value

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
 }

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")
+		}
+	})
 }

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)

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")

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)
+				}
+			}
+		})
+	}
+}