feat: send over TLS instead of plain TCP - #43
Merged
Merged
Conversation
An mbedTLS stream wrapping the lwIP TCP stream, and the collector moves to 6514.
Server authentication only: the device verifies the collector against the trust
anchor it already holds and presents nothing of its own.
Flash +13,076 B (+688 on the previous stage)
RAM +35,664 B (+28,280)
mbedTLS peak +14,724 B (36,056 absolute)
Log stack +712 B (unchanged)
Service +3,800 B (+2,808)
Flash is the number worth reading. Adding TLS to this device costs well under a
kilobyte, not the ~150 KB mbedTLS occupies, because a device that already speaks
mTLS was carrying mbedTLS long before SolidSyslog arrived. What is measured here is
the code that drives TLS, not TLS itself. It is also smaller than it would have been
as a single hop from UDP: the TCP stream underneath was already linked and paid for.
The RAM accounts for itself exactly, and only 632 bytes of it is the library:
mbedTLS pool, 32 -> 53 KiB +21,504
service seam, 2 -> 8 KiB +6,144
stream and session objects +632
=+28,280
Both of those resizes were forced by the device, in that order, and both failed
loudly first. The pool was sized for the broker session alone, so the handshake
could not allocate: "MbedTlsStream category 0x0402", eighty-four times, and nothing
reached the collector. Given room, the handshake then overflowed the service seam:
"FATAL: stack overflow in task service", which takes the device down before it can
report, so neither figure can be read from the run that fails.
Both are then sized from a run that completes, by the rules the baseline already
uses. The pool is the measured peak times 1.5 rounded up to the next KiB — 36,092
gives 53 KiB, and the margin is fragmentation headroom rather than spare capacity,
because buffer_alloc hands out contiguous space. The seam is twice the measured
high-water of 3,852, and configMINIMAL_STACK_SIZE * 15 falls 24 bytes short of that,
so it takes 16. A peak that moves ~100 bytes run to run can put the first rule
either side of a KiB boundary; 53 or 54 both hold it.
The service stack carries the handshake, which is why it nearly quadruples while the
log stack does not move at all. A task that calls Log is unaffected by the transport
underneath it.
The TLS stream takes the trust anchor and DRBG as handles at create time, not paths
or PEM, so SimulatedExistingApp_StartCrypto() has to run before the scheduler. The
drain window covers a connect and handshake rather than a datagram, sized against
the library's 5 s handshake budget.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe example enables SolidSyslog MbedTLS support, configures authenticated TLS syslog delivery on port 6514, increases TLS-related memory and timing allocations, and updates README and run-report measurements for the TLS stage. ChangesTLS syslog integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HarnessTask
participant Syslog_Start
participant SolidSyslogMbedTlsStream
participant Collector
HarnessTask->>Syslog_Start: Emit BOOT record
Syslog_Start->>SolidSyslogMbedTlsStream: Configure TLS with CA chain, RNG, and ServerName
SolidSyslogMbedTlsStream->>Collector: Connect securely on port 6514
SolidSyslogMbedTlsStream-->>Collector: Send encrypted syslog record
HarnessTask->>HarnessTask: Wait 3000 ms before measurement
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
app/main.c (1)
73-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWait for a real drain/completion signal
LogTask_EmitOnce()only waits for the record to be queued;ServiceTaskdoes the actualSolidSyslog_Service()drain/send loop. Replace the fixed 3-second sleep beforeMeasure_Report()with an explicit service-complete/error signal so the report can’t run before the TLS send finishes.🤖 Prompt for 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. In `@app/main.c` around lines 73 - 74, Replace the fixed vTaskDelay in LogTask_EmitOnce() with synchronization on an explicit ServiceTask completion/error signal emitted after SolidSyslog_Service() finishes draining or fails. Make LogTask_EmitOnce() wait for that signal before calling Measure_Report(), preserving error propagation and preventing the report from running before the TLS send completes.Source: Learnings
🤖 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.
Nitpick comments:
In `@app/main.c`:
- Around line 73-74: Replace the fixed vTaskDelay in LogTask_EmitOnce() with
synchronization on an explicit ServiceTask completion/error signal emitted after
SolidSyslog_Service() finishes draining or fails. Make LogTask_EmitOnce() wait
for that signal before calling Measure_Report(), preserving error propagation
and preventing the report from running before the TLS send completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13bd48cb-9b2b-490f-99a5-2e85127f8b5e
⛔ Files ignored due to path filters (2)
measurements/stages.tsvis excluded by!**/*.tsvmeasurements/tls.csvis excluded by!**/*.csv
📒 Files selected for processing (6)
CMakeLists.txtREADME.mdapp/AppConfig.happ/main.capp/syslog/Syslog.crun-report.md
5 tasks
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
1 participant
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.
Pull request
What this stage adds
An mbedTLS stream wrapping the lwIP TCP stream, and the collector moves to 6514.
Server authentication only: the device verifies the collector against the trust
anchor it already holds and presents nothing of its own.
Flash +13,076 B (+688 on the previous stage)
RAM +35,664 B (+28,280)
mbedTLS peak +14,724 B (36,056 absolute)
Log stack +712 B (unchanged)
Service +3,800 B (+2,808)
Flash is the number worth reading. Adding TLS to this device costs well under a
kilobyte, not the ~150 KB mbedTLS occupies, because a device that already speaks
mTLS was carrying mbedTLS long before SolidSyslog arrived. What is measured here is
the code that drives TLS, not TLS itself. It is also smaller than it would have been
as a single hop from UDP: the TCP stream underneath was already linked and paid for.
The RAM accounts for itself exactly, and only 632 bytes of it is the library:
mbedTLS pool, 32 -> 53 KiB +21,504
service seam, 2 -> 8 KiB +6,144
stream and session objects +632
=+28,280
Both of those resizes were forced by the device, in that order, and both failed
loudly first. The pool was sized for the broker session alone, so the handshake
could not allocate: "MbedTlsStream category 0x0402", eighty-four times, and nothing
reached the collector. Given room, the handshake then overflowed the service seam:
"FATAL: stack overflow in task service", which takes the device down before it can
report, so neither figure can be read from the run that fails.
Both are then sized from a run that completes, by the rules the baseline already
uses. The pool is the measured peak times 1.5 rounded up to the next KiB — 36,092
gives 53 KiB, and the margin is fragmentation headroom rather than spare capacity,
because buffer_alloc hands out contiguous space. The seam is twice the measured
high-water of 3,852, and configMINIMAL_STACK_SIZE * 15 falls 24 bytes short of that,
so it takes 16. A peak that moves ~100 bytes run to run can put the first rule
either side of a KiB boundary; 53 or 54 both hold it.
The service stack carries the handshake, which is why it nearly quadruples while the
log stack does not move at all. A task that calls Log is unaffected by the transport
underneath it.
The TLS stream takes the trust anchor and DRBG as handles at create time, not paths
or PEM, so SimulatedExistingApp_StartCrypto() has to run before the scheduler. The
drain window covers a connect and handshake rather than a datagram, sized against
the library's 5 s handshake budget.
Files
Checklist
measurements/tls.csvcommitted.measurements/stages.tsv.python3 scripts/gen-cost-table.py../run.shgreen (build + QEMU + baseline self-check) —run-report.mdcommitted.Context for review: 16 of 21 sequential single-commit PRs replaying the integration on top of the Baseline root. Each lands green and reviewed before the next is built on it, because a change to an early commit would force every commit above it to be re-run and re-measured.
Summary by CodeRabbit
New Features
Documentation
Performance