Skip to content

tls_check_cert_select_plugin: unanchored "404" assertion collides with ephemeral port numbers #13501

Description

@cmcfarlen

Description

tests/gold_tests/tls/tls_check_cert_select_plugin.test.py fails intermittently in CI, depending purely on which ephemeral port AuTest happens to assign to the ts process.

The test asserts on the bare substring "404":

tr.Processes.Default.Streams.All += Testers.ExcludesExpression("404", "Should make an exchange")

That substring is not anchored to an HTTP status line, so it also matches the port number in ordinary curl output. When AuTest assigns ts a port containing 404 — for example 62404 — the excluded expression matches lines like:

* Added bar.com:62404:127.0.0.1 to DNS cache
*   Trying 127.0.0.1:62404...

and the run fails:

file .../6-tr-Default/stream.all.txt : Should make an exchange - Failed
   Reason: Contents ... contains expression: "404"
     Details:
       * Added bar.com:62404:127.0.0.1 to DNS cache : 1
         ... 0*   Trying 127.0.0.1:62404... : 6

Test run 6 ("Test new version of bar cert with bad CA") is the one that fails, because it is the only run using ExcludesExpression for "404". The ContainsExpression("404") assertions in the other runs are spuriously satisfied by the same collision, which is arguably worse — they can pass without a real HTTP exchange having occurred.

Second, related bug in the same file

Four assertions write to the wrong test run variable. Lines 103, 116, and 130 use tr where the surrounding run is tr2:

tr2 = Test.AddTestRun("foo.com cert")
...
tr2.Processes.Default.Streams.All = Testers.ExcludesExpression("Could Not Connect", ...)
tr2.Processes.Default.Streams.All += Testers.ContainsExpression("CN=foo.com", ...)
tr2.Processes.Default.Streams.All += Testers.ExcludesExpression("CN=bar.com", ...)
tr.Processes.Default.Streams.All += Testers.ContainsExpression("404", "Should make an exchange")   # <-- tr, not tr2

The effect is that the "Should make an exchange" check for runs 1, 2 and 3 is appended to run 0 instead. Run 0's report shows "Should make an exchange" four times, while runs 1-3 never assert it at all. This also partially masks the port collision, since those misdirected checks are Contains rather than Excludes.

Suggested fix

  1. Anchor the assertion so it cannot match a port number — e.g. match HTTP/1.1 404 or the curl status line < 404, rather than the bare digits.
  2. Change the three misdirected tr. references to tr2. so runs 1-3 assert what they were meant to.

Affected versions

Both bugs were introduced together in 7dbb6cb ("Add hook for loading certificate and key data from plugin", #6609, 2021-06-25), verified with git log -S on the exact strings. The file has not been meaningfully changed since in this respect, so this affects every current line: 9.2.x, 10.0.x, 10.1.x, 10.2.x and master. The earliest release tag containing it is 9.2.0.

Notes

Not a regression from any recent change — it surfaced while reviewing an unrelated 10.2.x backport batch, where the shard drew port 62404. Re-running the shard makes it pass, which is consistent with the port-draw explanation.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions