feat: give the baseline a real mutual-TLS session - #27
Conversation
📝 WalkthroughWalkthroughThe simulated application now opens and holds a mutual-TLS session with a broker after lwIP and FatFs initialization. Build wiring, heap sizing, certificates, Docker services, smoke tests, console output, and the recorded run report were updated. ChangesBroker mutual-TLS session
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/docker-compose.yml`:
- Around line 66-68: Add a broker healthcheck that verifies openssl s_server is
listening on port 8883, then update the run service’s broker dependency to use
the service_healthy condition instead of start-order-only behavior. Preserve the
existing syslog-ng dependency.
- Around line 56-59: Update the openssl s_server command to include the
-verify_return_error option alongside -Verify 1, ensuring invalid or untrusted
client certificates abort the TLS handshake.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1b1dd1a-c2fb-4aa4-b622-5dc06d5ec005
📒 Files selected for processing (12)
CMakeLists.txtapp/AppConfig.happ/main.capp/sim/SimulatedBrokerSession.capp/sim/SimulatedBrokerSession.happ/sim/SimulatedExistingApp.capp/sim/SimulatedExistingApp.hdocker/docker-compose.ymlrun-report.txtscripts/gen-certs.shscripts/run.shscripts/smoke-oracle.sh
The device's own description says it "already speaks mTLS to other systems", but nothing in the run ever opened such a session. That leaves one number unmeasurable: how much of mbedTLS's memory SolidSyslog actually adds. A baseline whose mbedTLS buffer is sized for a session it never opens charges SolidSyslog nothing for TLS; one sized for credentials alone charges it a whole session that a real mTLS device already holds. This is the endpoint half of making that claim real. `openssl s_server` on 8883, in the oracle's network namespace so the device reaches it through the same slirp gateway, with its own certificate from the run's PKI — a different peer from the collector, so a different certificate. `-Verify` rather than `-verify`, because the lower-case form requests a client certificate and accepts a client that sent none; and `-verify_return_error`, because s_server's default is to report a verification failure and carry on, so without it "required" would not mean "trusted" — the same distinction peer-verify(required-trusted) draws on 6515. Proved before the device runs, for the same reason the four collector listeners are: a dead broker would read as "the baseline uses less memory" rather than as a failure, and nothing would catch it. `-rev` echoes each buffer back reversed, so the check is that a client presenting the device certificate got its probe back — bytes across the session, not merely a completed handshake — and that a client presenting no certificate got nothing. The authenticated probe retries for ten seconds, because depends_on guarantees the broker container started and not that s_server bound its socket. Retrying the real check rather than polling the port separately keeps one mechanism, and it avoids putting a second client in front of a server that handles one connection at a time — the device holds its session for the whole run, so a periodic healthcheck would queue behind it rather than learn anything. Two smaller changes so the next commit has somewhere to report from. The run report now carries the device's [sim] lines, which until now were printed and dropped — including the diagnostics a failed bring-up emits, so a failing run shows its reason rather than only its verdict. And the listener labels lose the word "oracle": the broker is not one, it is the device's own peer, and its cost belongs to the device. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
19cbc5e to
8ffecef
Compare
|
Both findings taken, folded into the 1. Fail closed on invalid client certificates — accepted. Correct, and it matters more here than the generic case: this repo already draws exactly that distinction on the collector's mTLS port, where The smoke check still probes only the certless case, matching what 6515 ships — a rogue-certificate probe would need a second CA in 2. Wait for the broker before probing — accepted, but not with a healthcheck. The race is real: A compose healthcheck is the wrong instrument here, though. Instead the authenticated probe itself retries for ten seconds. One mechanism, not two: the first probe is the wait, the same reply is the proof, and if it never answers it fails exactly as it would have before. The certless probe now runs after it, so the refusal is measured against a broker already known to be up — which per this repo's own note is the only condition under which a refusal proves anything.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@run-report.txt`:
- Line 15: Reconcile the mbedTLS peak measurement in the report with the PR
objective: regenerate run-report.txt from the authoritative capture or update
the objective so both consistently record either 37,224 or 37,236 bytes,
eliminating the 12-byte discrepancy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb0b8ab6-3770-48c0-8edb-43ce43ddb870
📒 Files selected for processing (12)
CMakeLists.txtapp/AppConfig.happ/main.capp/sim/SimulatedBrokerSession.capp/sim/SimulatedBrokerSession.happ/sim/SimulatedExistingApp.capp/sim/SimulatedExistingApp.hdocker/docker-compose.ymlrun-report.txtscripts/gen-certs.shscripts/run.shscripts/smoke-oracle.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- app/sim/SimulatedBrokerSession.h
- app/AppConfig.h
- app/sim/SimulatedExistingApp.h
- CMakeLists.txt
- docker/docker-compose.yml
- scripts/gen-certs.sh
- app/main.c
- app/sim/SimulatedBrokerSession.c
- scripts/smoke-oracle.sh
- app/sim/SimulatedExistingApp.c
The other half. The device now opens a session to the broker at bring-up, over lwIP's raw TCP and mbedTLS, with the credentials DeviceCertStore already holds — and never closes it. Held rather than opened-and-closed on purpose: two sessions that do not overlap measure max(), not sum(), and it is the sum that says what a second session costs. flash_text 361,736 -> 362,968 (+1,232) static_bss 133,152 -> 157,280 (+24,128) mbedtls_peak 22,208 -> 37,224 (+15,016) heap_used 4,440 -> 4,440 The last line is the point of the exercise. mbedTLS's buffer used to peak at 22,208 bytes with SolidSyslog's session in it and nothing else; with the device's own session open alongside it, 37,224. So a second concurrent session costs about 15,000 bytes, not 22,208 — the difference is the parsed certificates, the DRBG and the rest of the first session's one-off state, which a device speaking mTLS has paid for already. Once the baseline is regenerated it will hold that first session, and what SolidSyslog is charged for TLS memory becomes the marginal figure, measured rather than argued. "About", because mbedtls_peak is not byte-reproducible. Five runs of this image gave 37,152 / 37,224 / 37,236 / 37,244 / 37,248 — a spread wider than the run's 64-byte drift tolerance, for the same reason heap_used moves: gen-certs.sh builds a fresh PKI every run and DER lengths shift with the key material. The figures above are the two committed run reports, so the arithmetic is checkable; the marginal cost it yields is good to about a hundred bytes, not to the byte. That has to be settled before the baseline is frozen with this key in it. flash_text and static_bss are exact. The static RAM growth is almost entirely the buffer: 32 KiB to 55 KiB, which is the peak times 1.5 rounded up to the next KiB. The margin is not padding — buffer_alloc hands out contiguous space, and 24 KiB against a 22.2 KiB peak failed on fragmentation where 32 KiB worked. The remaining 576 bytes are the ssl context, its config, and the TCP state. The keep-alive list loses the mbedTLS client surface and most of the TCP entries, because the session calls them for real now and naming a function there that the device runs would say something untrue. Flash did not move for it: those call closures were in the image either way, which is what the list was for. What stays is what is still unexercised — UDP, the file API, and the teardown path a session held for the life of the device never reaches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8ffecef to
a90d433
Compare
|
Fair catch, and the inconsistency was worse than the 12 bytes suggested: the message's headline (37,236) was not even among the three sample runs it went on to list, because it came from a fourth run. Corrected by quoting the two committed run reports, which is what makes the arithmetic checkable — 22,208 -> 37,224, so +15,016. The 12 bytes themselves are not reconcilable, and that is the finding rather than a defect to fix. So the marginal cost is now stated as "about 15,000 bytes" rather than a byte-exact figure, since it is the difference of two measurements that each move by ~100 B. Standing rule taken from this: quote a non-reproducible figure from the committed report, never from whichever run happened to be on screen. |
1 participant
What this tag adds
Not a SolidSyslog step — a baseline change, so that one figure stops being unmeasurable.
The simulated device claims it "already speaks mTLS to other systems", but only ever linked the surface. That left no way to say how much of mbedTLS's memory SolidSyslog actually adds: a baseline sized for a session it never opens charges SolidSyslog nothing for TLS, and one sized for credentials alone charges it a whole session a real mTLS device already holds.
Two commits.
ci:— the endpoint.openssl s_server -rev -Verify 1 -verify_return_erroron 8883, in the oracle's network namespace, with its own certificate from the run's PKI. Proved before the device runs like the four collector listeners are: a client presenting the device certificate gets its probe back, a client presenting none is refused. A dead broker would otherwise read as "the baseline uses less memory" and nothing would fail.feat:— the session. Opened at bring-up over lwIP raw TCP + mbedTLS, and held for the whole run, so it is concurrent with SolidSyslog's rather than sequential.The number this produces
Both figures are from the two committed run reports, so the arithmetic is checkable.
A second concurrent session costs about 15,000 bytes, not 22,208. The difference is the parsed certificates, the DRBG and the rest of the first session's one-off state — which a device speaking mTLS has paid for already. Once the baseline is regenerated it holds that first session, and SolidSyslog's TLS memory charge becomes the marginal figure.
Static RAM is almost all the mbedTLS buffer, 32 KiB -> 55 KiB: the peak times 1.5, rounded up to the next KiB. The margin is fragmentation headroom, not padding — 24 KiB against a 22.2 KiB peak failed where 32 KiB worked.
The keep-alive list loses the mbedTLS client surface and most of the TCP entries: the session calls them for real now. Flash did not move for it — those call closures were in the image either way, which is what the list was for.
Worth knowing before the freeze
"About 15,000", because
mbedtls_peakis not byte-reproducible. Five runs of the same image gave 37,152 / 37,224 / 37,236 / 37,244 / 37,248 — a spread wider than the run's 64-byte drift tolerance, for the same reasonheap_usedmoves. The marginal cost is good to about a hundred bytes, not to the byte, and this has to be settled before the baseline is frozen with the key in it.flash_textandstatic_bssare exact.What this still cannot settle
Whether a given product's two connections truly overlap. A device that serialises them pays less. That stays a disclosure — but a far narrower one than "we never opened a session at all".
Checklist
measurements/<State>.csvcommitted, and a row added tomeasurements/tags.tsv. — nothing is frozen yet;Baseline.csvis regenerated after this lands../run.shgreen (build + QEMU + baseline self-check).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Testing