ooni / probe-cli / 34840782310

71%

Build Type

push

github

Committed by web-flow

Commit Message

fix(echcheck): do not attempt a TLS handshake when the TCP connect failed (#1817)

## Checklist

- [x] I have read the [contribution
guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md)
- [x] reference issue for this pull request: #1728
- [x] if you changed anything related to how experiments work and you
need to reflect these changes in the ooni/spec repository, please link
to the related ooni/spec pull request: not needed, the test keys do not
change shape
- [x] if you changed code inside an experiment, make sure you bump its
version number: 0.3.1 -> 0.3.2

## Description

`handshake()` in `internal/experiment/echcheck` dials the target and
then calls `tls.Client(conn, ...).HandshakeContext()` without checking
whether the dial succeeded. When it did not, `conn` is nil and
`crypto/tls` panics while writing the ClientHello. This is the trace
reported in #1725 and tracked in #1728: it shows up whenever a target
stops answering on its port, as `min-ng.test.defo.ie:15443` did.

The change returns the test keys right after the failed TCP connect has
been recorded, since that is the measurement: a `tcp_connects` entry
with the failure and no `tls_handshakes` entry, because no handshake was
attempted. No shape of `TestKeys` changes.

The regression test binds a port on localhost, closes it, and runs
`startHandshake` against it with and without GREASE. Without the fix the
test panics with the same nil pointer dereference as the issue; with the
fix it passes. Verified with `go test -short
./internal/experiment/echcheck/`.

Small side observation, not addressed here to keep the diff minimal: the
TLS conn is never closed after the handshake, so a successful handshake
leaks the socket until the process exits. Happy to send that as a
separate PR if you want it.

Co-authored-by: freetoshi-nakamoto <macintoshi-nakamoto@users.noreply.github.com>

Coverage Stats

3 of 3 new or added lines in 1 file covered. (100.0%)

5 existing lines in 2 files now uncovered.

28704 of 40414 relevant lines covered (71.02%)

44.81 hits per line