feat: S26.04 mbedTLS client-side TLS session resumption#489
Conversation
Resumption needs MBEDTLS_SSL_SESSION_TICKETS (client) compiled in; default-on upstream and in the integration-linux-mbedtls 3.6.x tree. Best-effort: a trimmed config without it falls back to full handshakes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Eagerly init a per-instance SavedSession slot in Create and capture the negotiated session via mbedtls_ssl_get_session in Open's success tail. Capture is gated on handshake success and is best-effort: a get_session failure leaves HasSavedSession false and never fails the Open. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On the second Open of an instance that captured a session, feed it back via mbedtls_ssl_set_session before the handshake so the peer can issue an abbreviated handshake. First connect and a failed set_session both fall through to a full handshake — restore is best-effort. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Destroy is the one site that frees the saved session — it must survive every fail-fast Close to stay resumable. A second capture frees the prior session first so get_session's deep copy never leaks the old peer cert / ticket. Completes the unit-tier resumption lifecycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a reconnecting socketpair transport double and a two-handshake, ticket-issuing server whose wrapped ticket-parse callback reports when a presented ticket is accepted — the server-side stand-in for mbedTLS's missing client-side SSL_session_reused(). Two tests drive the library's real Close->Open reconnect on one stream: a shared ticket key resumes the second handshake; a rotated key falls back to a full handshake and still delivers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-push housekeeping: clang-format reflow of the new resumption tests and the MISRA 11.3/11.5 suppression line numbers shifted by the capture/restore helpers added to SolidSyslogMbedTlsStream.c. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 45 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds TLS session resumption to ChangesSession resumption lifecycle and capture/restore
Integration test infrastructure and tests
Documentation and maintenance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) 🚧 Error MessagesCreated by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![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 `@Tests/MbedTlsIntegration/MbedTlsResumptionServer.c`:
- Around line 3-18: The translation unit uses uint32_t in the WrappedTicketWrite
and WrappedTicketParse function signatures but does not directly include
<stdint.h>, relying on transitive includes; add an explicit `#include` <stdint.h>
at the top of Tests/MbedTlsIntegration/MbedTlsResumptionServer.c (near the other
system headers) so the definitions for uint32_t are available and the TU no
longer depends on indirect headers.
In `@Tests/MbedTlsIntegration/SolidSyslogMbedTlsStreamIntegrationTest.cpp`:
- Around line 331-433: The test group SolidSyslogMbedTlsStreamResumption is a
shared fixture (it defines setup/teardown and helper methods
BuildBaseConfig/StartServer/ConnectAndDeliver) but is declared with TEST_GROUP;
change it to the repo convention by declaring the fixture with TEST_BASE (for
the base fixture type) and use TEST_GROUP_BASE for any groups that inherit it,
ensuring setup()/teardown() and member state remain in the base fixture; update
any tests that currently reference the old TEST_GROUP to inherit from the new
TEST_GROUP_BASE so the shared setup/teardown and helpers are used correctly.
🪄 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: 4bad45a7-630d-446c-a4cd-acd44c65243c
📒 Files selected for processing (14)
DEVLOG.mdPlatform/MbedTls/Source/SolidSyslogMbedTlsStream.cPlatform/MbedTls/Source/SolidSyslogMbedTlsStreamPrivate.hTests/MbedTls/SolidSyslogMbedTlsStreamTest.cppTests/MbedTlsIntegration/CMakeLists.txtTests/MbedTlsIntegration/MbedTlsResumptionServer.cTests/MbedTlsIntegration/MbedTlsResumptionServer.hTests/MbedTlsIntegration/ReconnectingSocketStream.cTests/MbedTlsIntegration/ReconnectingSocketStream.hTests/MbedTlsIntegration/SolidSyslogMbedTlsStreamIntegrationTest.cppTests/Support/MbedTlsFake.cTests/Support/MbedTlsFake.hdocs/integrating-mbedtls.mdmisra_suppressions.txt
| TEST_GROUP(SolidSyslogMbedTlsStreamResumption) | ||
| { | ||
| mbedtls_entropy_context entropy = {}; | ||
| mbedtls_ctr_drbg_context rng = {}; | ||
| int clientFds[2] = {-1, -1}; | ||
| int serverFds[2] = {-1, -1}; | ||
| struct MbedTlsTestCert trustedCa = {}; | ||
| struct MbedTlsTestCert serverCert = {}; | ||
| struct MbedTlsResumptionServer* server = nullptr; | ||
| struct SolidSyslogStream* transport = nullptr; | ||
| struct SolidSyslogStream* tlsStream = nullptr; | ||
| struct SolidSyslogAddress* addr = nullptr; | ||
|
|
||
| void setup() override | ||
| { | ||
| addr = AddressFake_Get(); | ||
| mbedtls_entropy_init(&entropy); | ||
| mbedtls_ctr_drbg_init(&rng); | ||
| const unsigned char pers[] = "mbedtls-resumption-test"; | ||
| mbedtls_ctr_drbg_seed(&rng, mbedtls_entropy_func, &entropy, pers, sizeof(pers) - 1U); | ||
|
|
||
| /* Two pre-connected socketpairs: one per handshake. The reconnecting | ||
| * transport consumes the client ends in order; the server thread | ||
| * services the server ends in order. No TCP ports -> deterministic. */ | ||
| int pairA[2] = {-1, -1}; | ||
| int pairB[2] = {-1, -1}; | ||
| socketpair(AF_UNIX, SOCK_STREAM, 0, pairA); | ||
| socketpair(AF_UNIX, SOCK_STREAM, 0, pairB); | ||
| clientFds[0] = pairA[0]; | ||
| clientFds[1] = pairB[0]; | ||
| serverFds[0] = pairA[1]; | ||
| serverFds[1] = pairB[1]; | ||
| for (int fd : {clientFds[0], clientFds[1], serverFds[0], serverFds[1]}) | ||
| { | ||
| SetRecvTimeout(fd, 5); | ||
| } | ||
|
|
||
| struct MbedTlsTestCertConfig caConfig = {}; | ||
| caConfig.SubjectName = TEST_CA_SUBJECT; | ||
| caConfig.IsCa = 1; | ||
| MbedTlsTestCert_Create(&caConfig, &trustedCa, &rng); | ||
|
|
||
| struct MbedTlsTestCertConfig serverConfig = {}; | ||
| serverConfig.SubjectName = TEST_SERVER_SUBJECT; | ||
| serverConfig.SubjectAltDns = TEST_SERVER_HOSTNAME; | ||
| serverConfig.IsCa = 0; | ||
| serverConfig.Issuer = &trustedCa; | ||
| MbedTlsTestCert_Create(&serverConfig, &serverCert, &rng); | ||
| } | ||
|
|
||
| void teardown() override | ||
| { | ||
| /* tlsStream first: its Destroy drives the transport Close that closes | ||
| * the current client fd, so it must run before the transport is freed. */ | ||
| if (tlsStream != nullptr) | ||
| { | ||
| SolidSyslogMbedTlsStream_Destroy(tlsStream); | ||
| } | ||
| if (transport != nullptr) | ||
| { | ||
| ReconnectingSocketStream_Destroy(transport); | ||
| } | ||
| if (server != nullptr) | ||
| { | ||
| MbedTlsResumptionServer_Destroy(server); | ||
| } | ||
| MbedTlsTestCert_Destroy(&serverCert); | ||
| MbedTlsTestCert_Destroy(&trustedCa); | ||
| mbedtls_ctr_drbg_free(&rng); | ||
| mbedtls_entropy_free(&entropy); | ||
| } | ||
|
|
||
| struct SolidSyslogMbedTlsStreamConfig BuildBaseConfig(struct SolidSyslogStream* clientTransport) | ||
| { | ||
| struct SolidSyslogMbedTlsStreamConfig cfg = {}; | ||
| cfg.Transport = clientTransport; | ||
| cfg.Sleep = NoOpSleep; | ||
| cfg.Rng = &rng; | ||
| cfg.CaChain = &trustedCa.Cert; | ||
| cfg.ServerName = TEST_SERVER_HOSTNAME; | ||
| return cfg; | ||
| } | ||
|
|
||
| struct MbedTlsResumptionServer* StartServer(bool rotateTicketKey) | ||
| { | ||
| struct MbedTlsResumptionServerConfig serverConfig = {}; | ||
| serverConfig.ServerFds[0] = serverFds[0]; | ||
| serverConfig.ServerFds[1] = serverFds[1]; | ||
| serverConfig.ServerCert = &serverCert; | ||
| serverConfig.Rng = &rng; | ||
| serverConfig.RotateTicketKeyBetweenConnections = rotateTicketKey; | ||
| return MbedTlsResumptionServer_Create(&serverConfig); | ||
| } | ||
|
|
||
| /* One Open -> Send -> (the test closes if reconnecting) cycle on the same | ||
| * stream. Returns whether the Open and Send both succeeded. */ | ||
| bool ConnectAndDeliver(const char* payload) | ||
| { | ||
| bool opened = SolidSyslogStream_Open(tlsStream, addr); | ||
| bool sent = opened && SolidSyslogStream_Send(tlsStream, payload, 8U); | ||
| return opened && sent; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== existing repo usage of base fixtures =="
rg -n --glob 'Tests/**/*.cpp' 'TEST_BASE\(|TEST_GROUP_BASE\('
echo
echo "== new shared fixture declaration in this file =="
rg -n 'TEST_GROUP\(SolidSyslogMbedTlsStreamResumption\)|TEST_BASE\(MbedTlsStreamResumptionBase\)|TEST_GROUP_BASE\(SolidSyslogMbedTlsStreamResumption' \
Tests/MbedTlsIntegration/SolidSyslogMbedTlsStreamIntegrationTest.cppRepository: DavidCozens/solid-syslog
Length of output: 3869
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Tests/MbedTlsIntegration/SolidSyslogMbedTlsStreamIntegrationTest.cpp"
echo "== fixture macros in target file =="
rg -n --glob 'Tests/**/*.cpp' 'TEST_BASE\(|TEST_GROUP_BASE\(|TEST_GROUP\(' "$FILE" || true
echo
echo "== CHECK_* usage in target file (sample) =="
rg -n 'CHECK_[A-Za-z_]+\(' "$FILE" | head -n 50 || true
echo
echo "== any repeated assertion shapes near the new group (context around line 331) =="
sed -n '300,480p' "$FILE" | nl -ba | head -n 250Repository: DavidCozens/solid-syslog
Length of output: 1871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Tests/MbedTlsIntegration/SolidSyslogMbedTlsStreamIntegrationTest.cpp"
echo "== fixture macros in target file =="
rg -n --glob 'Tests/**/*.cpp' 'TEST_BASE\(|TEST_GROUP_BASE\(|TEST_GROUP\(' "$FILE" || true
echo
echo "== CHECK_* usage in target file (sample) =="
rg -n 'CHECK_[A-Za-z_]+\(' "$FILE" | head -n 50 || true
echo
echo "== any repeated assertion shapes near the new group (context around line 331) =="
sed -n '300,480p' "$FILE" | nl -ba | head -n 250Repository: DavidCozens/solid-syslog
Length of output: 1871
Use TEST_BASE / TEST_GROUP_BASE for the shared resumption fixture
SolidSyslogMbedTlsStreamResumption (lines ~331-433) is declared as a plain TEST_GROUP, but this is a shared fixture (shared member state + setup()/teardown() + helpers). Follow the repo convention for Tests/**/*.cpp by converting it to TEST_BASE + TEST_GROUP_BASE.
Possible shape
-TEST_GROUP(SolidSyslogMbedTlsStreamResumption)
+TEST_BASE(MbedTlsStreamResumptionBase)
{
...
};
+
+TEST_GROUP_BASE(SolidSyslogMbedTlsStreamResumption, MbedTlsStreamResumptionBase)
+{
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST_GROUP(SolidSyslogMbedTlsStreamResumption) | |
| { | |
| mbedtls_entropy_context entropy = {}; | |
| mbedtls_ctr_drbg_context rng = {}; | |
| int clientFds[2] = {-1, -1}; | |
| int serverFds[2] = {-1, -1}; | |
| struct MbedTlsTestCert trustedCa = {}; | |
| struct MbedTlsTestCert serverCert = {}; | |
| struct MbedTlsResumptionServer* server = nullptr; | |
| struct SolidSyslogStream* transport = nullptr; | |
| struct SolidSyslogStream* tlsStream = nullptr; | |
| struct SolidSyslogAddress* addr = nullptr; | |
| void setup() override | |
| { | |
| addr = AddressFake_Get(); | |
| mbedtls_entropy_init(&entropy); | |
| mbedtls_ctr_drbg_init(&rng); | |
| const unsigned char pers[] = "mbedtls-resumption-test"; | |
| mbedtls_ctr_drbg_seed(&rng, mbedtls_entropy_func, &entropy, pers, sizeof(pers) - 1U); | |
| /* Two pre-connected socketpairs: one per handshake. The reconnecting | |
| * transport consumes the client ends in order; the server thread | |
| * services the server ends in order. No TCP ports -> deterministic. */ | |
| int pairA[2] = {-1, -1}; | |
| int pairB[2] = {-1, -1}; | |
| socketpair(AF_UNIX, SOCK_STREAM, 0, pairA); | |
| socketpair(AF_UNIX, SOCK_STREAM, 0, pairB); | |
| clientFds[0] = pairA[0]; | |
| clientFds[1] = pairB[0]; | |
| serverFds[0] = pairA[1]; | |
| serverFds[1] = pairB[1]; | |
| for (int fd : {clientFds[0], clientFds[1], serverFds[0], serverFds[1]}) | |
| { | |
| SetRecvTimeout(fd, 5); | |
| } | |
| struct MbedTlsTestCertConfig caConfig = {}; | |
| caConfig.SubjectName = TEST_CA_SUBJECT; | |
| caConfig.IsCa = 1; | |
| MbedTlsTestCert_Create(&caConfig, &trustedCa, &rng); | |
| struct MbedTlsTestCertConfig serverConfig = {}; | |
| serverConfig.SubjectName = TEST_SERVER_SUBJECT; | |
| serverConfig.SubjectAltDns = TEST_SERVER_HOSTNAME; | |
| serverConfig.IsCa = 0; | |
| serverConfig.Issuer = &trustedCa; | |
| MbedTlsTestCert_Create(&serverConfig, &serverCert, &rng); | |
| } | |
| void teardown() override | |
| { | |
| /* tlsStream first: its Destroy drives the transport Close that closes | |
| * the current client fd, so it must run before the transport is freed. */ | |
| if (tlsStream != nullptr) | |
| { | |
| SolidSyslogMbedTlsStream_Destroy(tlsStream); | |
| } | |
| if (transport != nullptr) | |
| { | |
| ReconnectingSocketStream_Destroy(transport); | |
| } | |
| if (server != nullptr) | |
| { | |
| MbedTlsResumptionServer_Destroy(server); | |
| } | |
| MbedTlsTestCert_Destroy(&serverCert); | |
| MbedTlsTestCert_Destroy(&trustedCa); | |
| mbedtls_ctr_drbg_free(&rng); | |
| mbedtls_entropy_free(&entropy); | |
| } | |
| struct SolidSyslogMbedTlsStreamConfig BuildBaseConfig(struct SolidSyslogStream* clientTransport) | |
| { | |
| struct SolidSyslogMbedTlsStreamConfig cfg = {}; | |
| cfg.Transport = clientTransport; | |
| cfg.Sleep = NoOpSleep; | |
| cfg.Rng = &rng; | |
| cfg.CaChain = &trustedCa.Cert; | |
| cfg.ServerName = TEST_SERVER_HOSTNAME; | |
| return cfg; | |
| } | |
| struct MbedTlsResumptionServer* StartServer(bool rotateTicketKey) | |
| { | |
| struct MbedTlsResumptionServerConfig serverConfig = {}; | |
| serverConfig.ServerFds[0] = serverFds[0]; | |
| serverConfig.ServerFds[1] = serverFds[1]; | |
| serverConfig.ServerCert = &serverCert; | |
| serverConfig.Rng = &rng; | |
| serverConfig.RotateTicketKeyBetweenConnections = rotateTicketKey; | |
| return MbedTlsResumptionServer_Create(&serverConfig); | |
| } | |
| /* One Open -> Send -> (the test closes if reconnecting) cycle on the same | |
| * stream. Returns whether the Open and Send both succeeded. */ | |
| bool ConnectAndDeliver(const char* payload) | |
| { | |
| bool opened = SolidSyslogStream_Open(tlsStream, addr); | |
| bool sent = opened && SolidSyslogStream_Send(tlsStream, payload, 8U); | |
| return opened && sent; | |
| } | |
| }; | |
| TEST_BASE(MbedTlsStreamResumptionBase) | |
| { | |
| mbedtls_entropy_context entropy = {}; | |
| mbedtls_ctr_drbg_context rng = {}; | |
| int clientFds[2] = {-1, -1}; | |
| int serverFds[2] = {-1, -1}; | |
| struct MbedTlsTestCert trustedCa = {}; | |
| struct MbedTlsTestCert serverCert = {}; | |
| struct MbedTlsResumptionServer* server = nullptr; | |
| struct SolidSyslogStream* transport = nullptr; | |
| struct SolidSyslogStream* tlsStream = nullptr; | |
| struct SolidSyslogAddress* addr = nullptr; | |
| void setup() override | |
| { | |
| addr = AddressFake_Get(); | |
| mbedtls_entropy_init(&entropy); | |
| mbedtls_ctr_drbg_init(&rng); | |
| const unsigned char pers[] = "mbedtls-resumption-test"; | |
| mbedtls_ctr_drbg_seed(&rng, mbedtls_entropy_func, &entropy, pers, sizeof(pers) - 1U); | |
| /* Two pre-connected socketpairs: one per handshake. The reconnecting | |
| * transport consumes the client ends in order; the server thread | |
| * services the server ends in order. No TCP ports -> deterministic. */ | |
| int pairA[2] = {-1, -1}; | |
| int pairB[2] = {-1, -1}; | |
| socketpair(AF_UNIX, SOCK_STREAM, 0, pairA); | |
| socketpair(AF_UNIX, SOCK_STREAM, 0, pairB); | |
| clientFds[0] = pairA[0]; | |
| clientFds[1] = pairB[0]; | |
| serverFds[0] = pairA[1]; | |
| serverFds[1] = pairB[1]; | |
| for (int fd : {clientFds[0], clientFds[1], serverFds[0], serverFds[1]}) | |
| { | |
| SetRecvTimeout(fd, 5); | |
| } | |
| struct MbedTlsTestCertConfig caConfig = {}; | |
| caConfig.SubjectName = TEST_CA_SUBJECT; | |
| caConfig.IsCa = 1; | |
| MbedTlsTestCert_Create(&caConfig, &trustedCa, &rng); | |
| struct MbedTlsTestCertConfig serverConfig = {}; | |
| serverConfig.SubjectName = TEST_SERVER_SUBJECT; | |
| serverConfig.SubjectAltDns = TEST_SERVER_HOSTNAME; | |
| serverConfig.IsCa = 0; | |
| serverConfig.Issuer = &trustedCa; | |
| MbedTlsTestCert_Create(&serverConfig, &serverCert, &rng); | |
| } | |
| void teardown() override | |
| { | |
| /* tlsStream first: its Destroy drives the transport Close that closes | |
| * the current client fd, so it must run before the transport is freed. */ | |
| if (tlsStream != nullptr) | |
| { | |
| SolidSyslogMbedTlsStream_Destroy(tlsStream); | |
| } | |
| if (transport != nullptr) | |
| { | |
| ReconnectingSocketStream_Destroy(transport); | |
| } | |
| if (server != nullptr) | |
| { | |
| MbedTlsResumptionServer_Destroy(server); | |
| } | |
| MbedTlsTestCert_Destroy(&serverCert); | |
| MbedTlsTestCert_Destroy(&trustedCa); | |
| mbedtls_ctr_drbg_free(&rng); | |
| mbedtls_entropy_free(&entropy); | |
| } | |
| struct SolidSyslogMbedTlsStreamConfig BuildBaseConfig(struct SolidSyslogStream* clientTransport) | |
| { | |
| struct SolidSyslogMbedTlsStreamConfig cfg = {}; | |
| cfg.Transport = clientTransport; | |
| cfg.Sleep = NoOpSleep; | |
| cfg.Rng = &rng; | |
| cfg.CaChain = &trustedCa.Cert; | |
| cfg.ServerName = TEST_SERVER_HOSTNAME; | |
| return cfg; | |
| } | |
| struct MbedTlsResumptionServer* StartServer(bool rotateTicketKey) | |
| { | |
| struct MbedTlsResumptionServerConfig serverConfig = {}; | |
| serverConfig.ServerFds[0] = serverFds[0]; | |
| serverConfig.ServerFds[1] = serverFds[1]; | |
| serverConfig.ServerCert = &serverCert; | |
| serverConfig.Rng = &rng; | |
| serverConfig.RotateTicketKeyBetweenConnections = rotateTicketKey; | |
| return MbedTlsResumptionServer_Create(&serverConfig); | |
| } | |
| /* One Open -> Send -> (the test closes if reconnecting) cycle on the same | |
| * stream. Returns whether the Open and Send both succeeded. */ | |
| bool ConnectAndDeliver(const char* payload) | |
| { | |
| bool opened = SolidSyslogStream_Open(tlsStream, addr); | |
| bool sent = opened && SolidSyslogStream_Send(tlsStream, payload, 8U); | |
| return opened && sent; | |
| } | |
| }; | |
| TEST_GROUP_BASE(SolidSyslogMbedTlsStreamResumption, MbedTlsStreamResumptionBase) | |
| { | |
| }; |
🤖 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 `@Tests/MbedTlsIntegration/SolidSyslogMbedTlsStreamIntegrationTest.cpp` around
lines 331 - 433, The test group SolidSyslogMbedTlsStreamResumption is a shared
fixture (it defines setup/teardown and helper methods
BuildBaseConfig/StartServer/ConnectAndDeliver) but is declared with TEST_GROUP;
change it to the repo convention by declaring the fixture with TEST_BASE (for
the base fixture type) and use TEST_GROUP_BASE for any groups that inherit it,
ensuring setup()/teardown() and member state remain in the base fixture; update
any tests that currently reference the old TEST_GROUP to inherit from the new
TEST_GROUP_BASE so the shared setup/teardown and helpers are used correctly.
There was a problem hiding this comment.
Declining this one. TEST_BASE/TEST_GROUP_BASE is for when multiple TEST_GROUPs in a file share the same fixture storage + setup/teardown (per CLAUDE.md). The two groups here do not share a fixture — SolidSyslogMbedTlsStreamIntegration uses MbedTlsTestServer over a single socketpair, while SolidSyslogMbedTlsStreamResumption uses MbedTlsResumptionServer over two socketpairs with a reconnecting transport. There is no common base to lift. Broader test hygiene is tracked for a separate follow-up PR.
There was a problem hiding this comment.
@DavidCozens, understood — the two groups have genuinely distinct fixtures and there's no shared state to lift. I'll withdraw the suggestion.
(╯°□°）╯
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 489
File: Tests/MbedTlsIntegration/SolidSyslogMbedTlsStreamIntegrationTest.cpp:331-433
Timestamp: 2026-05-30T16:40:23.390Z
Learning: In the solid-syslog codebase (per CLAUDE.md), `TEST_BASE` / `TEST_GROUP_BASE` is only appropriate when *multiple* `TEST_GROUP`s in the same file share identical fixture storage, `setup()`, and `teardown()`. Do NOT suggest converting a standalone `TEST_GROUP` to `TEST_BASE` + `TEST_GROUP_BASE` just because it has helper methods or a non-trivial fixture — the pattern is strictly for lifting a *common* fixture that is reused by two or more test groups. If the test groups in a file have different member state (e.g., different server types, different socketpair counts, different transport types), they should remain as independent `TEST_GROUP`s even if the code looks similar.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 352
File: Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp:175-182
Timestamp: 2026-05-13T22:05:22.272Z
Learning: In the solid-syslog codebase (per CLAUDE.md), avoid asking for additional defensive validation/guards when the alleged error case is provably unreachable given existing constraints. For example, in C++ test fixture helpers (like WriteMessage in Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp), don’t request payload-size < 4 guards when payloadSize is always derived from SOLIDSYSLOG_MAX_MESSAGE_SIZE - 100 (and thus stays >= 412) or from known hardcoded values that keep the “< 4” branch impossible. Only suggest guards if the derivation/constraints do not actually guarantee the precondition across all possible inputs in that code path.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 422
File: Tests/StreamFakeTest.cpp:0-0
Timestamp: 2026-05-22T12:15:25.389Z
Learning: In this codebase’s tests, avoid using conditional compilation (e.g., `#ifdef`) in both production and test code. For test cases that only need a `struct SolidSyslogAddress*` as an opaque pass-through token (the code under test does not dereference or inspect the platform-specific sockaddr contents), use the cross-platform helper `Tests/AddressFake.{h,c}` (via `AddressFake_Get()`), which returns an opaque non-null singleton. Only use the per-platform address creation APIs (`SolidSyslogPosixAddress_Create`, `SolidSyslogWinsockAddress_Create`, `SolidSyslogFreeRtosAddress_Create`) together with the corresponding `Private.h` accessor functions in tests that actually inspect the platform sockaddr internals (e.g., datagram/TCP-stream/resolver tests).
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 458
File: Tests/Lwip/SolidSyslogLwipRawResolverTest.cpp:236-244
Timestamp: 2026-05-26T09:51:03.475Z
Learning: In this repo’s CppUTest tests, when asserting the payload of the `ErrorHandlerFake` (i.e., severity/source/code recorded by the fake), use the established `CHECK_REPORTED(severity, source, code)` macro rather than re-creating the underlying expectations. This macro is the canonical wrapper around `CALLED_FAKE(ErrorHandlerFake_Handle, ONCE)`, `LONGS_EQUAL(severity, ErrorHandlerFake_LastSeverity())`, `POINTERS_EQUAL(source, ErrorHandlerFake_LastSource())`, and `UNSIGNED_LONGS_EQUAL(code, ErrorHandlerFake_LastCode())`, implemented as a `do { } while(0)` macro with the repo’s `NOLINTBEGIN/NOLINTEND` suppressions for `cppcoreguidelines-macro-usage` and `cppcoreguidelines-avoid-do-while`.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
- ConnectAndDeliver made const (clang-tidy readability-make-member- function-const, surfaced by the FreeRTOS tidy lanes that build mbedTLS). - Explicit <stdint.h> in MbedTlsResumptionServer.c for uint32_t in the ticket-callback signatures (was relying on transitive includes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
* Revert "feat: S26.04 mbedTLS client-side TLS session resumption (#489)" This reverts commit d37ef31. * docs: record TLS-resumption out-of-scope decision in DEVLOG The revert itself removes S26.04's DEVLOG entry; this adds an entry capturing the cross-backend decision (revert mbedTLS S26.04, won't-do OpenSSL S26.01) with the architectural rationale and the hand-rolled-test-server lesson, so the direction isn't re-proposed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Closes #487 (E26: TLS Hardening). Independent of sibling #280 (OpenSSL); the OpenSSL backend can now mirror this one.
What
SolidSyslogMbedTlsStreamdid a fresh full handshake on every (re)connect. Under S12.14's fail-fast reconnect model that is the dominant cost of the reconnect loop on constrained targets. This adds client-side session resumption: capture the negotiated session after a successful handshake (mbedtls_ssl_get_session), feed it back before the next handshake on the same instance (mbedtls_ssl_set_session), so the peer can issue an abbreviated handshake.Design notes
mbedtls_ssl_sessionmust survive the fail-fastCloseto be resumable on the nextOpen. It lives for the life of the instance and is freed only in_Cleanup. Eagersession_initinCreatekeeps that free always safe; a second capture frees the prior session first soget_session's deep copy never leaks the old peer cert / ticket.Openand emits no error code — resumption is an optimisation, never a delivery precondition (matches the OpenSSL sibling).MBEDTLS_SSL_SESSION_TICKETS, default-on upstream and in theintegration-linux-mbedtls3.6.x tree) documented indocs/integrating-mbedtls.md.Tests
Tests/MbedTls/): capture after handshake (gated on success, best-effort on failure), restore before the 2nd handshake (and not on the 1st), free on Destroy but not on Close, free-before-recapture.Tests/MbedTlsIntegration/): a new reconnecting socketpair transport double + a two-handshake ticket-issuing server model the library's real Close→Open reconnect. mbedTLS has no client-sideSSL_session_reused(), so the server wraps its ticket-parse callback to report when a presented ticket is accepted. One test resumes (shared ticket key); one rotates the key so the second handshake falls back to full and still delivers. Verified non-vacuous by disabling the production restore and confirming only the resume assertion fails.59 unit + 9 integration tests green locally.
Follow-up
Test/code hygiene pass deferred to a separate PR (per review at RP-B).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
SolidSyslogMbedTlsStream, enabling faster reconnections by reusing negotiated sessions across multiple connections on the same stream instance.Tests
Documentation