From 758d227920a23a86acf8ade5c6cc5d02048cd7ee Mon Sep 17 00:00:00 2001 From: munzzyy Date: Sun, 2 Aug 2026 13:40:07 -0500 Subject: [PATCH] fix(tlsmiddlebox): make ClientId settable and validate its value The ClientId option was declared as int, but SetOptionAny only knows how to set int64, bool, and string fields, so configuring the option from the command line never worked. Applying the registry test this commit adds on top of its parent shows it: $ go test -count=1 -run 'TestExperimentOptionsAreAlwaysSettable' ./internal/registry/ --- FAIL: TestExperimentOptionsAreAlwaysSettable (0.00s) --- FAIL: TestExperimentOptionsAreAlwaysSettable/tlsmiddlebox (0.00s) factory_test.go:1138: field ClientId has kind int, which SetOptionAny cannot set Every other experiment already declares integer options as int64, so this switches the field type and keeps clientid() returning int for the ClientIDs map lookup. Setting ClientId was still possible through OONI Run v2 descriptors, whose options bypass SetOptionAny via SetOptionsJSON, and any value outside the known 1-4 range crashed the process: ClientIDs is a map, so the lookup silently returns nil for unknown keys and netxlite.NewUTLSConn dereferences the nil ClientHelloID: panic: runtime error: invalid memory address or nil pointer dereference github.com/ooni/probe-cli/v3/internal/netxlite.NewUTLSConn internal/netxlite/utls.go:83 github.com/ooni/probe-cli/v3/internal/experiment/tlsmiddlebox.(*Measurer).handshakeWithTTL internal/experiment/tlsmiddlebox/tracing.go:100 Run now rejects unknown ClientId values upfront, like we already do for other invalid configuration, and the new registry test makes sure every option declared with an ooni tag uses a kind that SetOptionAny can actually set. --- internal/experiment/tlsmiddlebox/config.go | 4 +-- .../experiment/tlsmiddlebox/config_test.go | 19 +++++++++++--- internal/experiment/tlsmiddlebox/measurer.go | 8 +++++- .../experiment/tlsmiddlebox/measurer_test.go | 25 +++++++++++++++++- internal/registry/factory_test.go | 26 +++++++++++++++++++ 5 files changed, 74 insertions(+), 8 deletions(-) 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) + } + } + }) + } +}