fix(tlsmiddlebox): make ClientId settable and validate its value - #1811
Open
munzzyy wants to merge 1 commit into
Open
fix(tlsmiddlebox): make ClientId settable and validate its value#1811munzzyy wants to merge 1 commit into
munzzyy wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Description
tlsmiddlebox declares its
ClientIdoption asint, butSetOptionAnyin internal/registry only knows how to set int64, bool, and string fields. So the option is documented (it has anoonitag and shows up in the options list) but you cannot actually use it. Applying the registry test this PR adds on top of current master:miniooni tlsmiddlebox -O ClientId=2is where that bites. miniooni passes option values as strings, string values for integer options go throughsetOptionInt, andSetOptionAnyonly routes tosetOptionIntfor fields whose kind is int64. Anintfield falls through to the default branch instead, which returnsunsupported option type: string. Every other experiment already declares integer options as int64; tlsmiddlebox was the only one using plain int.There is a second problem behind the first one. OONI Run v2 descriptors apply options with
SetOptionsJSON, which unmarshals straight into the config struct and bypasses the kind check, so a descriptor can set anyClientIdit likes.ClientIDsin tracing.go is amap[int]*utls.ClientHelloIDwith keys 1 to 4, the lookup silently returns nil for any other key, andnetxlite.NewUTLSConndereferences the pointer. Running the measurer withConfig{ClientId: 5}against a local TLS listener on current master:The fix is small:
ClientIdbecomes int64 (withclientid()casting for the map lookup, so tracing.go is untouched), andRunnow rejects a ClientId that does not match any known fingerprint, the same way it already rejects an invalid test helper. I picked an error over a silent fallback to the stdlib handshaker because somebody who asked for a Chrome fingerprint should not quietly measure with the wrong ClientHello.The registry test shown above walks every experiment config and flags any
ooni-tagged field whose kindSetOptionAnycannot set, so a future experiment cannot reintroduce this class of bug.Testing
New coverage: the invalid ClientId path in
Run, the non-defaultclientid()value, and the registry-wide option kind check.go vetandgofmtare clean on both packages.