bneradt commented on PR #13576:
URL: https://github.com/apache/trafficserver/pull/13576#issuecomment-5359047546
## Review notes
The `InkAPI.cc` fix is correct. `SSLConfigParams::getCTX` sets `ctx_key =
client_cert`, and all three producers of that key — the conf-override path
(`SSLNetVConnection.cc:1215`), `params->clientCertPath`, and sni.yaml
(`SSLSNIConfig.cc:185`) — pass a path already resolved through
`Layout::relative_to`. So both the new comment and `key.assign(cert_path)` are
accurate.
Provenance worth adding to the commit message: this broke in 7dbb6cb188
("Add hook for loading certificate and key data from plugin", #6609,
2021-06-25), which changed `ctx_key` from `{cert}:{key}` to `client_cert` and
did not update `InkAPI.cc`. That makes this a backport candidate for 9.2.x/10.x
— the **Backport** label isn't set.
The test changes, however, still don't assert anything.
---
### 1. Blocker — `Streams.all` is not an AuTest property, so the new
assertions never run
`Streams` exposes `All`, not `all`. In `autest/testenities/streams.py` the
properties are `stdout`, `stderr`, `All`, `Warning`, `Error`, `Debug`,
`Verbose`, and neither `Streams` nor its base `TestEntity` overrides
`__setattr__`. So `s_server.Streams.all = <Tester>` just plants a dead instance
attribute and registers no tester.
Verified four ways against autest 1.10.6:
1. `hasattr(Streams, 'All') == True`, `hasattr(Streams, 'all') == False`.
2. `docsrc/source/API/streams.rst` documents exactly seven attributes; `all`
is not among them.
3. AuTest's own test suite and docs use `Streams.All` 28 times and
`Streams.all` zero times.
4. Direct experiment — a process printing `alice.com` with an assertion
demanding `bob.com`:
```python
# lowercase -> PASSES (assertion silently dropped)
tr.Processes.Default.Streams.all = Testers.ContainsExpression("bob.com",
"demands bob.com")
```
```
Running Test probe: Passed
Failed: 0
Passed: 1
```
```python
# capital -> FAILS, as it should
tr.Processes.Default.Streams.All = Testers.ContainsExpression("bob.com",
"demands bob.com")
```
```
Process: Default: Failed
Test : Checking that ReturnCode == 0 - Passed
file .../stream.all.txt : demands bob.com - Failed
Reason: Contents of .../stream.all.txt did not contains expression:
"bob.com"
```
Consequences for this PR:
- Lines 122 and 149 register nothing. The two assertions that actually prove
the fix — `alice.com` before the update, `bob.com` after — do not execute.
**This test passes with the `InkAPI.cc` change reverted.**
- This is also the real reason the old test masked the bug. The PR
description and #13575 attribute it to "permissive gold-file expressions," but
`client-cert-after.gold` (` ``bob.com`` `) was on `.all` too, so it was never
evaluated at all. The permissive-gold explanation holds only for `update.gold`,
where the duplicate `Content =` assignment overwrote the first tester.
Fix: `Streams.all` → `Streams.All` on both lines. There are 12 more dead
`Streams.all` assignments elsewhere under `tests/` (`h3/h3_sni_check`,
`timeout/quic_no_activity_timeout`, `timeout/default_inactivity_timeout`,
`ip_allow/ip_allow`, `tls/tls_sni_groups`, `tls/allow-plain`,
`tls/tls_hooks_client_verify`); those deserve their own PR, since some may fail
once their checks actually run.
### 2. Should fix — `SSLConfig::acquire()` is never released
`InkAPI.cc:8236` acquires but never calls `SSLConfig::release(params)`, on
any of the three return paths. `TSSslClientContextsNamesGet` and
`TSSslClientContextFindByName` immediately above both release correctly.
Pre-existing, but until this patch the function bailed at
`ca_paths_key.empty()` before doing any work; now that it actually runs, every
plugin-driven cert rotation permanently pins an `SSLConfigParams` generation.
Cheap to fix in a function you're already touching.
### 3. Only the first CA bucket is updated
The lookup loop `break`s at the first top-level `{ca_file}:{ca_path}` bucket
containing the cert. The same cert path can legitimately live under several
buckets — sni.yaml always uses the global CA, while a conf-override with
`ssl_client_ca_cert_name` produces a different top-level key for the same cert.
Stale contexts survive under the others, and which bucket wins is
`unordered_map` iteration order. Also newly reachable as of this patch.
### 4. A failed rebuild silently invalidates the working context
If `SSLCreateClientContext` returns `nullptr`, the code still stores the
null `shared_SSL_CTX` into the map before returning `TS_ERROR`. `getCTX`
lazy-reloads on null so it self-heals — same shape as `clearCTX` — but a
*failed* update shouldn't quietly discard a good context. Worth an early bail,
or a comment saying the null store is deliberate.
### 5. The API doc is now actively wrong
`TSSslClientCertUpdate.en.rst:38` says `cert_path` "should be exact match as
provided in configurations." It must be the path *resolved* against
`proxy.config.ssl.client.cert.path` — this very test writes `client_cert:
"client1.pem"` in sni.yaml but has to pass `{SSLDir}/client1.pem`. This PR is
the right place to correct that sentence. Separately,
`TSSslClientContext.en.rst` still describes the second-level key as "cert/key
paths"; it has been cert-only since 7dbb6cb188.
### 6. Minor
- The `Content =` → `Content +=` change on `traffic_out` is a real
improvement — the second `=` was replacing the first tester via
`TesterSet.Assign`.
- Consider adding `ExcludesExpression("Failed to update client cert")`.
That's the string that actually appeared in the failing sandbox, and it makes
the regression bite even if a "Successfully" line shows up for an unrelated
reason.
- `std::string key{cert_path};` at the declaration reads better than
declare-then-assign.
- The removed `swoc::bwprint` was nullptr-safe (`bwf_base.h:1046` guards `v
!= nullptr`), so no crash was being fixed here — just a key that could never
match.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
