feat: S03.08 TLS hardening — cert validation, cipher pinning, lifecycle cleanup#183
Merged
Merged
Conversation
Adds a new OpenSslIntegrationTests executable that links real libssl (instead of OpenSslFake) and exercises SolidSyslogTlsStream end-to-end via an in-process BIO pair. This sits between unit tests (fast, fake-backed) and BDD (end-to-end via syslog-ng) in the testing pyramid, and will carry the SL4 failure-mode coverage in subsequent commits (expired cert, wrong CN/SAN, wrong CA, unsupported cipher). Harness components: - BioPairStream: SolidSyslogStream adapter over one side of a BIO pair with a cooperative pump callback, so SolidSyslogTlsStream's synchronous SSL_connect path works unchanged over non-blocking in-memory BIOs. - TlsTestCert: programmatic X509 cert generation (OpenSSL 3.0 EVP_RSA_gen), parameterised by CN, SAN DNS names, validity window, and optional issuer for self-signed or chain scenarios. - TlsTestServer: server-side SSL_CTX + SSL + BIO pair; Pump() drives SSL_accept one step at a time as the client-side handshake progresses. Smoke test HandshakeSucceedsAgainstTrustedServerCert verifies all four pieces wire together against a self-signed localhost cert. CI: new openssl-integration job runs in parallel with build-and-test, self-contained (own configure + target build), quality-monitor entry added. Clang and sanitize presets deliberately skip the integration suite — unit tests remain the coverage engine; integration is supplementary evidence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three harness-based characterisation tests confirming the library's existing SSL_VERIFY_PEER plus SSL_set1_host wiring rejects the three SL4 failure modes named in the ticket acceptance: - HandshakeRejectedWhenServerCertIsExpired — cert notAfter in the past - HandshakeRejectedWhenServerCertHostnameDoesNotMatch — CN/SAN mismatch - HandshakeRejectedWhenClientDoesNotTrustServerCert — CA not in trust All three go green on first write — no production change was driven. The tests still earn their keep as regression protection: a future change that disabled SSL_VERIFY_PEER or dropped SSL_set1_host would turn them red. Setup refactor alongside: extracted buildScenario() helper from setup() so each scenario parameterises the cert config (commonName, SANs, validity window) without reconstructing the whole harness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SSL_set1_host configures the expected-hostname check that makes SSL_VERIFY_PEER reject a cert whose subject doesn't match the serverName. If set1_host fails silently (returns 0), the client still runs SSL_connect but without the hostname check — a MITM risk, since any trust-chain-valid cert from any host in the trust store would then be accepted. Return-value check now aborts Open instead. Driven by OpenReturnsFalseWhenSet1HostFails in the fake-based unit suite; OpenSslFake gains a SetSet1HostFails switch following the same pattern as SetConnectFails / SetWriteFails. Flagged in PR #170 review — first of the return-value-checking bullets in the S03.08 acceptance list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Parallel fix to the SSL_set1_host return-value check: SSL_set_tlsext_host_name (the SNI extension setter) can also fail silently. If it does, the client proceeds with SSL_connect but without advertising the intended hostname to the server — the server may then present a default certificate that the client's SSL_set1_host check accepts for a different tenant. Return-value check now aborts Open instead. Driven by OpenReturnsFalseWhenSniHostnameSetupFails; OpenSslFake gains a SetSniHostnameFails switch on the SSL_ctrl SET_TLSEXT_HOSTNAME path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously Open silently proceeded with a NULL SSL_CTX, which would crash on the next libssl call (SSL_CTX_load_verify_locations or SSL_CTX_set_verify deref the CTX). Under fakes this looked like a successful handshake. Either way the library was lying about the Open result. CreateSslContext now returns NULL immediately if SSL_CTX_new fails, and Open treats a NULL CTX as Open failure. Driven by OpenReturnsFalseWhenCtxNewFails; OpenSslFake gains SetCtxNewFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of the SSL_CTX_new guard: if SSL_new returns NULL, the subsequent SSL_set_bio / SSL_ctrl / SSL_set1_host / SSL_connect all deref the NULL pointer. Check the return and bail early. The SSL_CTX allocated on the line above is released by Destroy — the caller is expected to call Destroy after a failed Open, which Destroy already handles. No new cleanup needed here; a broader lifecycle sweep is scheduled for Phase 4. Driven by OpenReturnsFalseWhenSslNewFails; OpenSslFake gains SetSslNewFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If the caller's CA bundle file is missing, unreadable, or malformed, libssl refuses to load any trust anchors. Continuing would make the handshake fail anyway, but failing fast surfaces the misconfiguration cleanly. First of the return-value checks that needs in-function cleanup — CreateSslContext frees the CTX it just allocated before returning NULL, so the caller doesn't have to know about the partial state. Driven by two tests: one for the false return (Open fails), one for the CTX_free call count (no leak). OpenSslFake gains SetLoadVerifyLocationsFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SSL_CTX_set_min_proto_version is a ctrl macro that can fail if libssl rejects the version constant (build-time configuration mismatch or future-version rejection). Treating it as non-failing means the library could silently fall through to a lower protocol floor, breaking SL4 assumptions about TLS 1.2 minimum. CreateSslContext now returns NULL and frees the CTX it allocated on SET_MIN_PROTO_VERSION failure, matching the load_verify_locations path. Last of the CreateSslContext return value checks. Driven by OpenReturnsFalseWhenMinProtoVersionFails and MinProtoVersionFailureFreesCtx; OpenSslFake gains SetMinProtoVersionFails on the SSL_CTX_ctrl SET_MIN_PROTO_VERSION command. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CreateTransportBio allocated a fresh BIO_METHOD per Open and lost the pointer after returning — a pure leak. Now the stream remembers the method pointer on the instance, and Close pairs the allocation with BIO_meth_free. Per-Open lifecycle matches the symmetry with SSL_CTX_new/SSL_free and SSL_new/SSL_free — one Open allocates a full set, one Close releases it. Driven by CloseFreesBioMethod; OpenSslFake gains a counted BIO_meth_free stub that was previously implicit (the method was never freed, so the fake didn't model the call). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Destroy now covers the case where Close wasn't called (e.g. Open partially failed, or the caller's lifecycle skipped Close), releasing the BIO_METHOD the way Close would have. Close in turn nulls the stored bioMethod pointer so a subsequent Destroy won't double-free. Two tests pin the contract: DestroyFreesBioMethodWhenCloseNotCalled and DestroyAfterCloseDoesNotDoubleFreeBioMethod. Together they say: BIO_METHOD is freed once, whether the lifecycle ended at Close, ended at Destroy, or went through both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of the BIO_METHOD idempotence commit for SSL: Destroy now releases the SSL handle if Close wasn't called (e.g. Open failed after SSL_new but before a successful connect). Close nulls the stored pointer so a subsequent Destroy doesn't double-free. Previously Destroy only handled SSL_CTX — any caller that hit an Open failure after SSL_new was leaking an SSL handle until process exit. Not caught under fakes (SSL_free is a stub), real libssl under valgrind would flag it. Driven by DestroyFreesSslWhenCloseNotCalled and DestroyAfterCloseDoesNotDoubleFreeSsl. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Under memory pressure BIO_meth_new can return NULL. Continuing with a NULL BIO_METHOD would crash the subsequent BIO_meth_set_* calls in real libssl. CreateTransportBio now returns NULL on failure, and Open checks the result. The partially-initialised SSL_CTX and SSL are released by Destroy — the test covers return-value behaviour; the lifecycle invariants set in the Destroy-idempotence commits already guarantee no leak. Driven by OpenReturnsFalseWhenBioMethNewFails; OpenSslFake gains SetBioMethNewFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Final return-value check in Open. BIO_new can fail under memory pressure; when it does, the BIO_METHOD we just allocated is freed inline rather than leaving it for Destroy, so callers that skip Destroy still don't leak. Two tests drive this: - OpenReturnsFalseWhenBioNewFails (return propagates via the CreateTransportBio-returns-NULL path added earlier) - BioNewFailureFreesBioMethodInline (cleanup co-located with the failure so it happens even without Destroy) Completes the "every OpenSSL call in Open is return-value-checked" and "partial-init failures release all resources" acceptance items. OpenSslFake gains SetBioNewFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an optional cipherList field to SolidSyslogTlsStreamConfig. Non- NULL values are forwarded to SSL_CTX_set_cipher_list; NULL leaves OpenSSL's default cipher selection in place. The library stays policy-free — no baked-in SL4 cipher list. Callers that need SL4-compliant ciphers set the list at their level, where the security profile lives. docs/iec62443.md will document a recommended example. Driven by OpenPassesCipherListToSslCtx (list forwarded verbatim) and OpenSkipsCipherListSetupWhenNotConfigured (NULL means no call). Failure-handling and cleanup come in the next commit. OpenSslFake gains SSL_CTX_set_cipher_list stub + accessors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SSL_CTX_set_cipher_list returns 0 when every cipher in the list is unrecognised or unavailable in the linked libssl build. Continuing with a CTX that has no cipher suites to offer produces a handshake failure with no meaningful client-side error message — so fail fast at config time and surface the misconfiguration. CreateSslContext frees the CTX it allocated before returning NULL, matching the pattern of the other return-value checks. Driven by OpenReturnsFalseWhenCipherListRejected and CipherListFailureFreesCtx; OpenSslFake gains SetCipherListFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pairs with the fake-based OpenReturnsFalseWhenCipherListRejected: real libssl rejects a cipher name it doesn't recognise, and the library surfaces that as Open failure. Covers the "unsupported cipher" item in the S03.08 acceptance list. Exercises the real SSL_CTX_set_cipher_list validation path, which the fake can only approximate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the SL4 controls landed by S03.08 as part of the SL4 "what's in place" narrative, without promoting the TLS sender from Planned to Available (that's S03.11's scope): - Hostname verification (SNI + cert check) with return-checked setup calls. - Pinned SSL_VERIFY_PEER on the CTX. - Return-checked TLS 1.2 floor. - cipherList config field for caller-supplied SL4 cipher pinning, with a starting-point example (ECDHE+AESGCM:ECDHE+CHACHA20). - Idempotent Close/Destroy lifecycle. Traceability matrix cross-references the S03.08 story for CR 3.9. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Covers the phased planning pass, what worked (test-support trio, pump-on-read, upfront surprises list), what didn't (BDD-first reversal, Phase 2/4 coupling, commit count), the key decisions (cipher policy at caller, per-Open BIO lifecycle, coverage stays unit-only), and deferred items (per-BIO fake refactor, TLS 1.3 cipher suites, max_proto_version control, S03.11 doc promotion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR hardens Platform/OpenSsl TLS handling (hostname checks, TLS1.2 minimum, optional cipher-list pinning), fixes BIO_METHOD/SSL/CTX lifecycle and mid-open cleanup, adds an in-process OpenSSL-based integration test harness (BIO pair, test server, cert generator, adapter), extends OpenSslFake for failure injection, and wires CI to run the new integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Client as SolidSyslogTlsStream (Client)
participant Adapter as BioPairStream
participant BIO as OpenSSL BIO Pair (in-memory)
participant Server as TlsTestServer (Server)
Test->>Server: Create(serverCert, optional cipherList)
Server->>BIO: BIO_new_bio_pair (create client/server BIOs)
Server->>Server: SSL_CTX_new (server mode), load cert/key, SSL_new, SSL_set_bio, set accept state
Test->>Adapter: Create with Server->Client BIO (client-side BIO)
Test->>Client: Create SolidSyslogTlsStream with Adapter transport + caBundle + serverName (+ cipherList)
Test->>Client: Open() -> starts SSL_connect
Client->>Adapter: Send ClientHello via BIO_write
Adapter->>BIO: Write to server side BIO
loop cooperative handshake
Client->>Adapter: Read (BIO_read)
Adapter->>BIO: Read from client-side BIO
alt data available
Adapter->>Client: return bytes
else needs pump
Adapter->>Server: Pump() -> SSL_accept / process incoming data
Server->>BIO: Write ServerHello -> available to client BIO
end
Client->>Client: SSL_connect continues
end
Client->>Client: Check SSL_set_tlsext_host_name / SSL_set1_host succeeded
Client->>Client: Validate certificate chain against caBundle and CN/SAN, check cipher compatibility
alt validation passes
Client->>Test: Open() returns success
else validation fails
Client->>Client: Release SSL, BIO_METHOD, SSL_CTX
Client->>Test: Open() returns failure
end
Test->>Client: Destroy()
Client->>Client: Ensure SSL, BIO_METHOD, SSL_CTX freed (idempotent)
Test->>Server: Destroy()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 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 |
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 752 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
CI checks flagged:
- Format: the new `Tests/OpenSslIntegration/` sources and the
`Tests/Support/OpenSslFake*` additions didn't match the project's
clang-format aggressive alignment rules. Auto-formatted via
`clang-format -i`.
- Tidy:
* CreateSslContext had two adjacent `const char*` params
(bugprone-easily-swappable-parameters). Refactored to take the
`SolidSyslogTlsStreamConfig*` directly — same logic, one pointer,
future-proof for further config fields.
* BioPairStream::Read had two `done = true` arms with identical
bodies (bugprone-branch-clone). Combined under one `||` condition.
* TlsTestCert had `#define` macros for integral constants
(modernize-macro-to-enum). Promoted to an anonymous enum.
* SetValidity had two adjacent `time_t` params. Refactored to take
the TlsTestCertConfig* directly, same pattern as CreateSslContext.
* TlsTestCert_WritePemToFile didn't check fopen. Added a NULL-guard
early return.
No behaviour change; 747 unit tests + 5 integration tests remain
green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
550-611:⚠️ Potential issue | 🟡 MinorAdd
openssl-integrationtoquality-summary.needsso JUnit artifact is available.The new
Integration Tests (OpenSSL)tool entry (lines 607-611) patterns**/junit-openssl-integration/cpputest_*.xml, but thequality-summaryjob (line 552) does not listopenssl-integrationinneeds:. Withif: always(),quality-summarycan start as soon as its listed dependencies finish — potentially beforeopenssl-integrationuploads its artifact — leaving the new tool silently empty on the PR summary.🔧 Suggested fix
- needs: [build-and-test, clang-build-and-test, sanitize, coverage, tidy, cppcheck, format, bdd, windows-build-and-test, bdd-windows] + needs: [build-and-test, clang-build-and-test, sanitize, coverage, tidy, cppcheck, format, bdd, windows-build-and-test, bdd-windows, openssl-integration]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 550 - 611, The quality-summary job may run before the openssl-integration job finishes because quality-summary.needs does not include openssl-integration; update the needs list for the quality-summary job (job name: quality-summary) to include "openssl-integration" so the job waits for that job to complete and its JUnit artifact (pattern **/junit-openssl-integration/cpputest_*.xml) is available when the Quality Monitor step runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Platform/OpenSsl/Source/SolidSyslogTlsStream.c`:
- Around line 199-203: The code only calls SSL_CTX_set_cipher_list which affects
TLS1.2 and earlier, leaving TLS1.3 ciphers uncontrolled; update the TLS setup in
the same block (around SSL_CTX_set_cipher_list call) to either explicitly set
TLS1.3 ciphers with SSL_CTX_set_ciphersuites(ctx, cipherList_tls13) using a
separate TLS1.3-only list (e.g., cipherList_tls13) or explicitly cap the maximum
protocol to TLS1.2 with SSL_CTX_set_max_proto_version(ctx, TLS1_2_VERSION) if
TLS1.3 should be disabled; ensure you check return values and free ctx/return
NULL on failure just like the existing SSL_CTX_set_cipher_list error path.
In `@Tests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cpp`:
- Around line 45-48: The mkstemp call result isn't checked: modify the block
around mkstemp/close/TlsTestCert_WritePemToFile to verify the returned fd is >=
0 before calling close or writing the CA file; if mkstemp fails, fail the test
(or log and abort) instead of proceeding, and only call
TlsTestCert_WritePemToFile when the template was successfully expanded and the
file descriptor is valid; reference the caPath variable, the mkstemp call, the
fd local, and TlsTestCert_WritePemToFile to locate the change.
In `@Tests/OpenSslIntegration/TlsTestCert.c`:
- Line 22: TlsTestCert.c currently calls EVP_RSA_gen which exists only in
OpenSSL 3.0+, so either require OpenSSL 3.0 in the build or add a fallback
keygen path for older OpenSSL: update CMakeLists.txt to use find_package(OpenSSL
3.0 REQUIRED) when SOLIDSYSLOG_OPENSSL is ON, or modify the key generation in
TlsTestCert.c to detect EVP_RSA_gen availability and if missing use the
EVP_PKEY_CTX/EVP_PKEY_keygen flow (creating an EVP_PKEY_CTX for "RSA", set RSA
key bits using TLS_TEST_CERT_RSA_BITS and call EVP_PKEY_keygen) so the code
compiles on OpenSSL 1.1.1 and 3.x.
In `@Tests/OpenSslIntegration/TlsTestServer.c`:
- Around line 17-34: TlsTestServer_Create must validate every allocation and
OpenSSL call: check calloc returned non-NULL, SSL_CTX_new succeeded,
SSL_CTX_use_certificate and SSL_CTX_use_PrivateKey returned success (non-zero),
SSL_CTX_set_cipher_list returned success when config->cipherList is set, SSL_new
succeeded, and BIO_new_bio_pair returned success before using the BIOs; on any
failure unwind/clean already-created resources (free the struct, SSL_free,
SSL_CTX_free, BIO_free or BIO_free_all as appropriate) and return NULL so the
caller fails fast; use the existing symbols TlsTestServer_Create, struct
TlsTestServer, SSL_CTX_new, SSL_CTX_use_certificate, SSL_CTX_use_PrivateKey,
SSL_CTX_set_cipher_list, SSL_new, BIO_new_bio_pair, SSL_set_bio to locate and
implement the checks and cleanup.
In `@Tests/Support/OpenSslFake.c`:
- Around line 514-540: The test OpenSslFake must simulate TLS 1.3 ciphersuite
handling and the production config must expose it: add a fake function
SSL_CTX_set_ciphersuites(SSL_CTX* ctx, const char* str) in
Tests/Support/OpenSslFake.c that mirrors SSL_CTX_set_cipher_list behavior (track
call count, last ctx arg, last ciphers string, and a fails flag), and add helper
accessors OpenSslFake_SetCiphersuitesFails(bool),
OpenSslFake_SetCiphersuitesCallCount(void), and
OpenSslFake_LastCiphersuites(void); then update the production API
(SolidSyslogTlsStreamConfig) to include a ciphersuites string field and modify
CreateSslContext to call SSL_CTX_set_ciphersuites(ctx, config->ciphersuites) in
addition to SSL_CTX_set_cipher_list so TLS 1.3 ciphers can be pinned.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 550-611: The quality-summary job may run before the
openssl-integration job finishes because quality-summary.needs does not include
openssl-integration; update the needs list for the quality-summary job (job
name: quality-summary) to include "openssl-integration" so the job waits for
that job to complete and its JUnit artifact (pattern
**/junit-openssl-integration/cpputest_*.xml) is available when the Quality
Monitor step runs.
🪄 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: d52467ea-bb4e-4acf-888e-afeb22ffae0e
📒 Files selected for processing (18)
.github/workflows/ci.ymlDEVLOG.mdPlatform/OpenSsl/Interface/SolidSyslogTlsStream.hPlatform/OpenSsl/Source/SolidSyslogTlsStream.cTests/CMakeLists.txtTests/OpenSslIntegration/BioPairStream.cTests/OpenSslIntegration/BioPairStream.hTests/OpenSslIntegration/CMakeLists.txtTests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cppTests/OpenSslIntegration/TlsTestCert.cTests/OpenSslIntegration/TlsTestCert.hTests/OpenSslIntegration/TlsTestServer.cTests/OpenSslIntegration/TlsTestServer.hTests/OpenSslIntegration/main.cppTests/SolidSyslogTlsStreamTest.cppTests/Support/OpenSslFake.cTests/Support/OpenSslFake.hdocs/iec62443.md
Comment on lines
+199
to
+203
| if (cipherList != NULL && SSL_CTX_set_cipher_list(ctx, cipherList) != 1) | ||
| { | ||
| SSL_CTX_free(ctx); | ||
| return NULL; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenSSL SSL_CTX_set_cipher_list TLS 1.3 ciphersuites SSL_CTX_set_ciphersuites documentation
💡 Result:
In OpenSSL, SSL_CTX_set_cipher_list configures ciphers for TLS 1.2 and below using the format described in openssl-ciphers(1). It does not affect TLS 1.3 ciphersuites. For TLS 1.3 ciphersuites, use SSL_CTX_set_ciphersuites(SSL_CTX *ctx, const char *str). This sets a colon-separated list of TLS 1.3 ciphersuite names in preference order, e.g., "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256". Valid TLS 1.3 ciphersuites (OpenSSL master/4.0): - TLS_AES_128_GCM_SHA256 - TLS_AES_256_GCM_SHA384 - TLS_CHACHA20_POLY1305_SHA256 - TLS_AES_128_CCM_SHA256 - TLS_AES_128_CCM_8_SHA256 - TLS_SM4_GCM_SM3 - TLS_SM4_CCM_SM3 - TLS_SHA384_SHA384 (integrity-only) - TLS_SHA256_SHA256 (integrity-only, in some versions) The default is "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256". As of OpenSSL 4.0, strings are case-insensitive. Returns 1 on success, 0 on failure. SSL_set_ciphersuites works similarly for individual SSL objects. Use OSSL_default_ciphersuites to get the default TLS 1.3 string. Example: SSL_CTX_set_ciphersuites(ctx, "TLS_AES_128_GCM_SHA256:TLS_CHACHA20_POLY1305_SHA256");
Citations:
- 1: https://docs.openssl.org/3.3/man3/SSL_CTX_set_cipher_list/
- 2: https://docs.openssl.org/3.4/man3/SSL_CTX_set_cipher_list/
- 3: https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_cipher_list.html
- 4: https://docs.openssl.org/master/man3/SSL_CTX_set_cipher_list/
- 5: https://github.com/openssl/openssl/blob/2cf43bb1/doc/man3/SSL_CTX_set_cipher_list.pod
🏁 Script executed:
cat -n Platform/OpenSsl/Source/SolidSyslogTlsStream.c | sed -n '190,210p'Repository: DavidCozens/solid-syslog
Length of output: 565
🏁 Script executed:
cat -n Platform/OpenSsl/Source/SolidSyslogTlsStream.c | sed -n '140,210p'Repository: DavidCozens/solid-syslog
Length of output: 2570
🏁 Script executed:
grep -n "CreateSslContext\|SolidSyslogTlsStream_Open" Platform/OpenSsl/Source/SolidSyslogTlsStream.c | head -20Repository: DavidCozens/solid-syslog
Length of output: 344
🏁 Script executed:
cat -n Platform/OpenSsl/Source/SolidSyslogTlsStream.c | sed -n '45,75p'Repository: DavidCozens/solid-syslog
Length of output: 1110
🏁 Script executed:
grep -n "SSL_CTX_set_max_proto_version\|TLS1_3_VERSION" Platform/OpenSsl/Source/SolidSyslogTlsStream.cRepository: DavidCozens/solid-syslog
Length of output: 50
Add TLS 1.3 ciphersuites control to prevent default negotiation of potentially undesirable TLS 1.3 ciphers.
SSL_CTX_set_cipher_list controls only TLS 1.2 and earlier ciphers. Since the minimum protocol version is set to TLS 1.2 without an explicit maximum cap, TLS 1.3 remains negotiable and will use OpenSSL's default TLS 1.3 ciphersuites. To control TLS 1.3 ciphersuites explicitly, call SSL_CTX_set_ciphersuites(ctx, cipherList_tls13) in addition to SSL_CTX_set_cipher_list, or cap the maximum protocol version if TLS 1.3 support is not intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Platform/OpenSsl/Source/SolidSyslogTlsStream.c` around lines 199 - 203, The
code only calls SSL_CTX_set_cipher_list which affects TLS1.2 and earlier,
leaving TLS1.3 ciphers uncontrolled; update the TLS setup in the same block
(around SSL_CTX_set_cipher_list call) to either explicitly set TLS1.3 ciphers
with SSL_CTX_set_ciphersuites(ctx, cipherList_tls13) using a separate
TLS1.3-only list (e.g., cipherList_tls13) or explicitly cap the maximum protocol
to TLS1.2 with SSL_CTX_set_max_proto_version(ctx, TLS1_2_VERSION) if TLS1.3
should be disabled; ensure you check return values and free ctx/return NULL on
failure just like the existing SSL_CTX_set_cipher_list error path.
Owner
Author
There was a problem hiding this comment.
Deferred to #182 (S03.14). The S03.08 design call was to keep the library policy-free on cipher selection — the caller picks. TLS 1.3 ciphersuite plumbing (SSL_CTX_set_ciphersuites) plus a BDD scenario proving end-to-end TLS 1.3 is a coherent separate thin story. Current state accepts libssl's defaults for TLS 1.3; no intent to cap at 1.2.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — the library intentionally stays policy-free on cipher selection; TLS 1.3 ciphersuite control via SSL_CTX_set_ciphersuites and the accompanying BDD scenario are tracked under #182 (S03.14). I'll note this for future reviews.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 183
File: Platform/OpenSsl/Source/SolidSyslogTlsStream.c:0-0
Timestamp: 2026-04-22T05:25:22.887Z
Learning: In `Platform/OpenSsl/Source/SolidSyslogTlsStream.c` of `DavidCozens/solid-syslog`, `SSL_CTX_set_ciphersuites` (TLS 1.3 ciphersuite control) is intentionally absent in S03.08. The library is policy-free on cipher selection — the caller provides `cipherList` for TLS 1.2 ciphers via `SSL_CTX_set_cipher_list`. TLS 1.3 ciphersuite plumbing (`SSL_CTX_set_ciphersuites`) plus a BDD end-to-end TLS 1.3 scenario are deferred to issue `#182` (S03.14). The current code intentionally accepts libssl defaults for TLS 1.3 with no intent to cap at TLS 1.2. Do not flag missing `SSL_CTX_set_ciphersuites` or the absence of a TLS 1.2 maximum-version cap in future reviews of `SolidSyslogTlsStream.c`.
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: 170
File: CMakeLists.txt:94-100
Timestamp: 2026-04-21T05:30:26.407Z
Learning: In `Example/Threaded/main.c` of `DavidCozens/solid-syslog`, three `#ifdef SOLIDSYSLOG_HAVE_OPENSSL` blocks for TLS wiring are a known temporary stopgap. Issue `#171` (S3.13 under E3) will replace them with per-backend CMake-selected factory files (`ExampleTlsSender_OpenSsl.c` / `_WolfSsl.c` / `_Unavailable.c`) to keep application code free of build-flag knowledge and provide a natural extension point for wolfSSL. Do not flag these `#ifdef` blocks as needing immediate refactoring in future reviews of `main.c`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 170
File: Platform/OpenSsl/Source/SolidSyslogTlsStream.c:58-62
Timestamp: 2026-04-21T06:23:07.350Z
Learning: In `Platform/OpenSsl/Source/SolidSyslogTlsStream.c` of `DavidCozens/solid-syslog`, the return values of `SSL_set_tlsext_host_name`, `SSL_set1_host`, and all other OpenSSL calls inside the `Open` function are intentionally unchecked in S3.7 (PR `#170`, walking-skeleton). Every OpenSSL return-value check, SNI/hostname-setup failure blocking the handshake, mid-Open resource leaks (SSL/CTX/transport cleanup on failure paths), and `BIO_METHOD` lifecycle hardening are deferred to S3.8 (issue `#172`), part of Epic E3 (issue `#5`). Do not flag missing return-value checks or resource-leak paths in `SolidSyslogTlsStream_Open` (or related static helpers) in future reviews until S3.8 is merged.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-21T06:23:37.913Z
Learning: In `DavidCozens/solid-syslog`, `BIO_METHOD` lifecycle (`BIO_meth_free`), `SSL` pointer nulling in `SolidSyslogTlsStream_Close`, and full `Destroy` resource cleanup in `Platform/OpenSsl/Source/SolidSyslogTlsStream.c` are intentionally deferred to issue `#172` (S3.8 hardening), which also covers mid-Open resource leak tightening. Do not flag these as missing in future reviews of `SolidSyslogTlsStream.c`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 170
File: CMakeLists.txt:94-100
Timestamp: 2026-04-21T05:30:26.407Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, `SOLIDSYSLOG_OPENSSL` is an explicit CMake option that defaults to ON when OpenSSL is discoverable at configure time (`find_package(OpenSSL QUIET)`) and can be overridden with `-DSOLIDSYSLOG_OPENSSL=OFF` for UDP/TCP-only builds. It gates: `Platform/OpenSsl` subdirectory, TLS unit tests, OpenSslFake compilation, `ExampleTlsConfig`, and example `libssl`/`libcrypto` linkage. Do not suggest making OpenSSL unconditionally required for POSIX builds.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-21T06:23:37.913Z
Learning: In `DavidCozens/solid-syslog`, the shared `lastSetDataArg` field in `Tests/Support/OpenSslFake.c` (used by both `BIO_set_data` and `BIO_get_data` tracking) is intentionally left shared for now and is tracked for per-BIO state expansion in issue `#172` (S3.8). Do not flag this shared state as a defect in future reviews of `OpenSslFake.c`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-21T06:23:37.913Z
Learning: In `DavidCozens/solid-syslog`, the inconsistent null-check guard pattern in `Tests/SolidSyslogTlsStreamTest.cpp` (lines 161–187) is intentionally deferred to issue `#31` (E12: Error Handling), which covers a project-wide null-guard consistency sweep. Do not flag this pattern inconsistency in future reviews of `SolidSyslogTlsStreamTest.cpp`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-21T06:23:37.913Z
Learning: In `DavidCozens/solid-syslog`, the private CA key `Bdd/syslog-ng/tls/ca.key` is intentionally version-controlled alongside `ca.pem`, `server.key`, and `server.pem` in `Bdd/syslog-ng/tls/`. This is deliberate policy to keep the BDD suite hermetic (no generation step at test time, identical fixtures on every runner). The materials are labelled "TEST ONLY" in `Bdd/syslog-ng/tls/README.md`. Do not flag `ca.key` as a committed secret in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 170
File: Tests/CMakeLists.txt:0-0
Timestamp: 2026-04-21T05:30:21.957Z
Learning: In `DavidCozens/solid-syslog`, `Tests/Support/OpenSslFake.c` is compiled into the `PosixFakes` static library via `POSIX_FAKES_SOURCES` in `Tests/Support/CMakeLists.txt` and is linked into the test binary via `target_link_libraries(${PROJECT_NAME}Tests PRIVATE PosixFakes)`. It does NOT need to appear directly in `TEST_SOURCES`. As of commit 790da74 it is also conditionally included only when `SOLIDSYSLOG_OPENSSL=ON`. Do not flag `OpenSslFake.c` as missing from `TEST_SOURCES` in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/Windows/SolidSyslogWindowsExample.c:50-53
Timestamp: 2026-04-18T07:18:44.356Z
Learning: In `Example/Windows/SolidSyslogWindowsExample.c` of `DavidCozens/solid-syslog`, the `wsaData.wVersion` version-match check after `WSAStartup(MAKEWORD(2,2), &wsaData)` is intentionally omitted. The project targets Windows 10+ / windows-2025 where Winsock 2.2 is guaranteed to be available; CLAUDE.md guidance says to trust internal code and framework guarantees. Do not flag the missing `LOBYTE/HIBYTE(wsaData.wVersion)` version check in future reviews of this file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 140
File: Source/SolidSyslogPosixTcpStream.c:31-36
Timestamp: 2026-04-17T07:50:54.370Z
Learning: In `Source/SolidSyslogPosixTcpStream.c`, `SolidSyslogPosixTcpStream_Destroy()` intentionally does not close the socket fd before nulling the vtable pointers. Through the only supported API path (TcpSender → Stream), every successful `Open` is guaranteed to be matched by a `Close` — either via `TcpSender_Destroy` (when `connected == true`), `SendData` failure recovery, or `Open`'s own connect-failure self-recovery (`OpenClosesSocketOnConnectFailure` test). Standalone "Destroy-without-Close" usage is not a supported scenario; adding defensive cleanup without a driving test would violate the project's "no untested production code" discipline. Formalising this contract (with tests) is deferred to Epic `#31` (error-handling / robustness). Do not flag missing fd cleanup in `SolidSyslogPosixTcpStream_Destroy` in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 149
File: Platform/Windows/Source/SolidSyslogWinsockResolver.c:67-72
Timestamp: 2026-04-17T20:07:40.530Z
Learning: In `Platform/Windows/Source/SolidSyslogWinsockResolver.c` (and its POSIX mirror `Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c`), the dereference of `info->ai_addr` without NULL/addrlen validation, and the unchecked cast of `getPort()` to `uint16_t` before `htons()`, are intentional. The resolver trusts its injected `getHost`/`getPort` callbacks by design (SRP); port-range and `ai_addr` defensive checks belong at the config/caller layer. Any future hardening must be a coordinated change across both resolvers together. Do not flag these as bugs or missing validation in future reviews of either resolver file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 91
File: Source/SolidSyslogTcpSender.c:57-71
Timestamp: 2026-04-09T20:59:09.391Z
Learning: In `Source/SolidSyslogTcpSender.c`, error handling for `connect()` failure and `send()` failure (including partial writes, SIGPIPE mitigation, and reconnection logic) is intentionally deferred to story S15.2. In the current implementation (S15.1), `tcp->connected` is set to `true` unconditionally after `connect()`, and `send()` return values are ignored. S15.2 will test-drive: connect failure → return false (and do NOT set `tcp->connected = true`), send failure → close socket and reconnect, and a `SendAll()` helper that retries on partial writes.
Comment on lines
+514
to
+540
| int SSL_CTX_set_cipher_list(SSL_CTX* ctx, const char* str) | ||
| { | ||
| setCipherListCallCount++; | ||
| lastSetCipherListCtxArg = ctx; | ||
| lastCipherList = str; | ||
| return setCipherListFails ? 0 : 1; | ||
| } | ||
|
|
||
| void OpenSslFake_SetCipherListFails(bool fails) | ||
| { | ||
| setCipherListFails = fails; | ||
| } | ||
|
|
||
| int OpenSslFake_SetCipherListCallCount(void) | ||
| { | ||
| return setCipherListCallCount; | ||
| } | ||
|
|
||
| SSL_CTX* OpenSslFake_LastSetCipherListCtxArg(void) | ||
| { | ||
| return lastSetCipherListCtxArg; | ||
| } | ||
|
|
||
| const char* OpenSslFake_LastCipherList(void) | ||
| { | ||
| return lastCipherList; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenSSL SSL_CTX_set_cipher_list TLS 1.3 SSL_CTX_set_ciphersuites official documentation
💡 Result:
In OpenSSL, SSL_CTX_set_cipher_list configures the list of available ciphers for TLS 1.2 and below protocols (using the format described in openssl-ciphers(1)). It does not affect TLS 1.3 ciphersuites. For TLS 1.3 ciphersuites, use SSL_CTX_set_ciphersuites (or SSL_set_ciphersuites for an individual SSL object). This takes a colon-separated list of TLS 1.3 ciphersuite names in order of preference, such as "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256". Valid TLS 1.3 ciphersuites include: - TLS_AES_128_GCM_SHA256 - TLS_AES_256_GCM_SHA384 - TLS_CHACHA20_POLY1305_SHA256 - TLS_AES_128_CCM_SHA256 - TLS_AES_128_CCM_8_SHA256 - TLS_SM4_GCM_SM3 (OpenSSL 4.0+) - TLS_SM4_CCM_SM3 (OpenSSL 4.0+) - TLS_SHA384_SHA384 (integrity-only) - TLS_SHA256_SHA256 (integrity-only) The default for TLS 1.3 is "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256". These functions return 1 on success (ciphers/ciphersuites selected) and 0 on failure. In OpenSSL 4.0+, the strings are processed case-insensitively. Functions OSSL_default_cipher_list and OSSL_default_ciphersuites return the default strings (new in 3.0). Synopsis: #include <openssl/ssl.h> int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str); int SSL_CTX_set_ciphersuites(SSL_CTX *ctx, const char *str);
Citations:
- 1: https://docs.openssl.org/3.4/man3/SSL_CTX_set_cipher_list/
- 2: https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_cipher_list.html
- 3: https://docs.openssl.org/master/man3/SSL_CTX_set_cipher_list/
- 4: https://github.com/openssl/openssl/blob/2cf43bb1/doc/man3/SSL_CTX_set_cipher_list.pod
🏁 Script executed:
# Search for TLS 1.3 or ciphersuites usage in production code
rg -i "ciphersuite|tls.*1\.3|ssl_op_no_tlsv1_3" --type c --type h -A 3Repository: DavidCozens/solid-syslog
Length of output: 50
🏁 Script executed:
# Check the production TLS stream implementation for TLS 1.3 handling
fd "SolidSyslogTlsStream\.(c|h)$" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat -n {}'Repository: DavidCozens/solid-syslog
Length of output: 8949
🏁 Script executed:
# Search for any mention of ciphersuites in tests
rg -i "ciphersuite" Tests/ --type c -B 2 -A 2Repository: DavidCozens/solid-syslog
Length of output: 50
🏁 Script executed:
# Check if issue `#172` or S3.8 mentions ciphersuites or TLS 1.3
rg -i "ciphersuite|tls.*1\.3|172|s3\.8" --type c --type h | grep -i "issue\|defer\|todo\|fixme" | head -20Repository: DavidCozens/solid-syslog
Length of output: 50
Add support for configuring TLS 1.3 ciphersuites alongside TLS 1.2 ciphers.
Production code enables TLS 1.3 by default (no maximum version constraint) but provides no API to configure its ciphersuites—only SSL_CTX_set_cipher_list for TLS 1.2 and below. If strict cipher pinning is a goal, cipher configuration for TLS 1.3 (SSL_CTX_set_ciphersuites) must be added to the SolidSyslogTlsStreamConfig struct and CreateSslContext. Tests can pass while TLS 1.3 uses OpenSSL defaults, defeating pinning intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Support/OpenSslFake.c` around lines 514 - 540, The test OpenSslFake
must simulate TLS 1.3 ciphersuite handling and the production config must expose
it: add a fake function SSL_CTX_set_ciphersuites(SSL_CTX* ctx, const char* str)
in Tests/Support/OpenSslFake.c that mirrors SSL_CTX_set_cipher_list behavior
(track call count, last ctx arg, last ciphers string, and a fails flag), and add
helper accessors OpenSslFake_SetCiphersuitesFails(bool),
OpenSslFake_SetCiphersuitesCallCount(void), and
OpenSslFake_LastCiphersuites(void); then update the production API
(SolidSyslogTlsStreamConfig) to include a ciphersuites string field and modify
CreateSslContext to call SSL_CTX_set_ciphersuites(ctx, config->ciphersuites) in
addition to SSL_CTX_set_cipher_list so TLS 1.3 ciphers can be pinned.
Owner
Author
There was a problem hiding this comment.
Same deferral as the comment at SolidSyslogTlsStream.c:203 — #182 (S03.14) will add the SSL_CTX_set_ciphersuites fake, the production config field, and a BDD scenario proving end-to-end TLS 1.3. Keeping S03.08 tight on the TLS 1.2 cipher path it was scoped for.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — S03.08 stays scoped to the TLS 1.2 cipher path, and the SSL_CTX_set_ciphersuites fake, production config field, and end-to-end TLS 1.3 BDD scenario are all deferred to S03.14 (#182). I'll note this for future reviews.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 170
File: Platform/OpenSsl/Source/SolidSyslogTlsStream.c:58-62
Timestamp: 2026-04-21T06:23:07.350Z
Learning: In `Platform/OpenSsl/Source/SolidSyslogTlsStream.c` of `DavidCozens/solid-syslog`, the return values of `SSL_set_tlsext_host_name`, `SSL_set1_host`, and all other OpenSSL calls inside the `Open` function are intentionally unchecked in S3.7 (PR `#170`, walking-skeleton). Every OpenSSL return-value check, SNI/hostname-setup failure blocking the handshake, mid-Open resource leaks (SSL/CTX/transport cleanup on failure paths), and `BIO_METHOD` lifecycle hardening are deferred to S3.8 (issue `#172`), part of Epic E3 (issue `#5`). Do not flag missing return-value checks or resource-leak paths in `SolidSyslogTlsStream_Open` (or related static helpers) in future reviews until S3.8 is merged.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 170
File: Tests/CMakeLists.txt:0-0
Timestamp: 2026-04-21T05:30:21.957Z
Learning: In `DavidCozens/solid-syslog`, `Tests/Support/OpenSslFake.c` is compiled into the `PosixFakes` static library via `POSIX_FAKES_SOURCES` in `Tests/Support/CMakeLists.txt` and is linked into the test binary via `target_link_libraries(${PROJECT_NAME}Tests PRIVATE PosixFakes)`. It does NOT need to appear directly in `TEST_SOURCES`. As of commit 790da74 it is also conditionally included only when `SOLIDSYSLOG_OPENSSL=ON`. Do not flag `OpenSslFake.c` as missing from `TEST_SOURCES` in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-21T06:23:37.913Z
Learning: In `DavidCozens/solid-syslog`, the shared `lastSetDataArg` field in `Tests/Support/OpenSslFake.c` (used by both `BIO_set_data` and `BIO_get_data` tracking) is intentionally left shared for now and is tracked for per-BIO state expansion in issue `#172` (S3.8). Do not flag this shared state as a defect in future reviews of `OpenSslFake.c`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 170
File: CMakeLists.txt:94-100
Timestamp: 2026-04-21T05:30:26.407Z
Learning: In `Example/Threaded/main.c` of `DavidCozens/solid-syslog`, three `#ifdef SOLIDSYSLOG_HAVE_OPENSSL` blocks for TLS wiring are a known temporary stopgap. Issue `#171` (S3.13 under E3) will replace them with per-backend CMake-selected factory files (`ExampleTlsSender_OpenSsl.c` / `_WolfSsl.c` / `_Unavailable.c`) to keep application code free of build-flag knowledge and provide a natural extension point for wolfSSL. Do not flag these `#ifdef` blocks as needing immediate refactoring in future reviews of `main.c`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-21T06:23:37.913Z
Learning: In `DavidCozens/solid-syslog`, `BIO_METHOD` lifecycle (`BIO_meth_free`), `SSL` pointer nulling in `SolidSyslogTlsStream_Close`, and full `Destroy` resource cleanup in `Platform/OpenSsl/Source/SolidSyslogTlsStream.c` are intentionally deferred to issue `#172` (S3.8 hardening), which also covers mid-Open resource leak tightening. Do not flag these as missing in future reviews of `SolidSyslogTlsStream.c`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-21T06:23:37.913Z
Learning: In `DavidCozens/solid-syslog`, the inconsistent null-check guard pattern in `Tests/SolidSyslogTlsStreamTest.cpp` (lines 161–187) is intentionally deferred to issue `#31` (E12: Error Handling), which covers a project-wide null-guard consistency sweep. Do not flag this pattern inconsistency in future reviews of `SolidSyslogTlsStreamTest.cpp`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 170
File: CMakeLists.txt:94-100
Timestamp: 2026-04-21T05:30:26.407Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, `SOLIDSYSLOG_OPENSSL` is an explicit CMake option that defaults to ON when OpenSSL is discoverable at configure time (`find_package(OpenSSL QUIET)`) and can be overridden with `-DSOLIDSYSLOG_OPENSSL=OFF` for UDP/TCP-only builds. It gates: `Platform/OpenSsl` subdirectory, TLS unit tests, OpenSslFake compilation, `ExampleTlsConfig`, and example `libssl`/`libcrypto` linkage. Do not suggest making OpenSSL unconditionally required for POSIX builds.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-21T06:23:37.913Z
Learning: In `DavidCozens/solid-syslog`, the private CA key `Bdd/syslog-ng/tls/ca.key` is intentionally version-controlled alongside `ca.pem`, `server.key`, and `server.pem` in `Bdd/syslog-ng/tls/`. This is deliberate policy to keep the BDD suite hermetic (no generation step at test time, identical fixtures on every runner). The materials are labelled "TEST ONLY" in `Bdd/syslog-ng/tls/README.md`. Do not flag `ca.key` as a committed secret in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/Windows/SolidSyslogWindowsExample.c:50-53
Timestamp: 2026-04-18T07:18:44.356Z
Learning: In `Example/Windows/SolidSyslogWindowsExample.c` of `DavidCozens/solid-syslog`, the `wsaData.wVersion` version-match check after `WSAStartup(MAKEWORD(2,2), &wsaData)` is intentionally omitted. The project targets Windows 10+ / windows-2025 where Winsock 2.2 is guaranteed to be available; CLAUDE.md guidance says to trust internal code and framework guarantees. Do not flag the missing `LOBYTE/HIBYTE(wsaData.wVersion)` version check in future reviews of this file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:28.657Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the `_CRT_SECURE_NO_WARNINGS` compile definition is a known temporary stopgap for MSVC CRT deprecation warnings in test fakes. Issue `#126` (S13.3) will introduce a `SafeString` abstraction (Windows impl wraps `_s` variants; POSIX falls through to standard functions); once test fakes migrate, this suppression is removed. Production code is already SDL-clean. Do not flag this definition as an unjustified suppression in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 166
File: Platform/Posix/Interface/SolidSyslogPosixTcpStream.h:8-13
Timestamp: 2026-04-20T08:51:10.974Z
Learning: In `Platform/Posix/Interface/SolidSyslogPosixTcpStream.h`, the inline comment for `SOLIDSYSLOG_TCP_DEFAULT_PORT = 514` ("RFC 6587 convention — same as UDP; no IANA assignment for plain TCP syslog") is a known inaccuracy carried over from the old `SolidSyslogTcpSender.h`. RFC 6587/IANA actually assigns port 601 for TCP syslog; the project intentionally defaults to 514 for compatibility. Correcting this comment is deferred to either the TLS default-port story in Epic E3 or a dedicated follow-up story. Do not re-flag this comment as incorrect in future reviews of this file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 91
File: Source/SolidSyslogTcpSender.c:57-71
Timestamp: 2026-04-09T20:59:09.391Z
Learning: In `Source/SolidSyslogTcpSender.c`, error handling for `connect()` failure and `send()` failure (including partial writes, SIGPIPE mitigation, and reconnection logic) is intentionally deferred to story S15.2. In the current implementation (S15.1), `tcp->connected` is set to `true` unconditionally after `connect()`, and `send()` return values are ignored. S15.2 will test-drive: connect failure → return false (and do NOT set `tcp->connected = true`), send failure → close socket and reconnect, and a `SendAll()` helper that retries on partial writes.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 103
File: Tests/FileFake.c:336-339
Timestamp: 2026-04-11T21:59:19.566Z
Learning: In `Tests/FileFake.c`, the bounds checks `(fake->position + count) <= fake->active->fileSize` and `(fake->position + count) <= FILEFAKE_MAX_SIZE` do not risk `size_t` overflow in practice: `position` is bounded by `FILEFAKE_MAX_SIZE` (4096) and `count` is bounded by record overhead plus `SOLIDSYSLOG_MAX_MESSAGE_SIZE` (517). Do not flag these additions as overflow risks.
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 752 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Accepted: - Integration test now fails loudly on mkstemp failure instead of closing -1 and writing to the unexpanded template path. - CMakeLists.txt enforces OpenSSL >= 3.0 when SOLIDSYSLOG_OPENSSL is ON — the integration harness uses EVP_RSA_gen which is 3.0-only. Configure fails with a clear message on older OpenSSL rather than failing to link with a confusing missing-symbol error. - quality-summary job now depends on openssl-integration so the new JUnit artifact is guaranteed downloaded before the summary runs. - TlsTestServer nulls serverBio after SSL_set_bio to document the ownership transfer (the BIO pair's server side is now owned by SSL, freed via SSL_free). Deferred to S03.14 (#182): - TLS 1.3 ciphersuite control (SSL_CTX_set_ciphersuites) in production and in OpenSslFake. Library stays policy-free on cipher choice per the S03.08 design decision; TLS 1.3 plumbing plus a BDD proof is a coherent separate story. Declined: - Full return-value checks on every OpenSSL and libc call inside TlsTestServer_Create. This is test-harness code running in a controlled CI container where allocations are deterministic; defensive branches create untested paths with no realistic failure mode to cover. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 752 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Open was mixing orchestration with OpenSSL hostname setup detail. The extracted helper collapses eleven lines of nested ifs and five return- points into a single named step whose short-circuit `&&` preserves the original "skip set1_host if SNI setup failed" semantics. Single-exit, positive-logic (== 1 rather than != 1) to match the MISRA style we're moving toward. Open itself still has eight early returns — addressed in subsequent refactorings as the remaining stages (transport BIO wiring, handshake) are extracted. No behaviour change; all seven hostname-related unit tests and the integration suite remain green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the BIO-over-transport wiring (create the custom BIO, attach the transport as its backing data, hand it to the SSL handle) in a single named step. Open loses four lines of OpenSSL-vocabulary detail. Single-exit, positive-logic (`bio != NULL`) form. Eight-line helper replaces four inline lines in Open, for net clarity at the call site where the named operation is doing the explaining. No behaviour change — same BIO_new / BIO_set_data / SSL_set_bio calls in the same order; pointer-chain tests (LastBio, LastSetData*, LastSetBio*) stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulls the SSL/BIO_METHOD free-and-null pair out of both lifecycle endpoints. Close reads as three steps (shutdown, release, close transport). Destroy reads as two (release handshake state, free context). The null-guarding makes ReleaseHandshakeState safe to call from either path regardless of which subset of resources is live — already covered by the existing idempotence tests. No behaviour change; lifecycle-count tests (Close/Destroy variants for SSL_free and BIO_meth_free) stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Open is now a narrative of six named handshake stages combined with short-circuit &&, returning once. One return, all positive logic, one level of abstraction throughout. Introduces three small stream-side wrappers so every step in the chain has the uniform bool(*)(stream) signature: - InitSslContext — stores stream->ctx, returns bool - InitSslSession — stores stream->ssl, returns bool - PerformHandshake — wraps SSL_connect > 0 No behaviour change. Short-circuit evaluation preserves the original bail-out semantics: a failed step skips all subsequent ones exactly as the prior early-return chain did. All 20+ Open-path tests stay green — identical OpenSSL call sequence, unchanged call counts, unchanged pointer-chain assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CreateSslContext now reads as allocate, configure, clean up on
failure — three concepts, single exit. The per-concern configure
step becomes:
ConfigureTrustAnchors(ctx, path) — load CA bundle + pin SSL_VERIFY_PEER
ConfigureProtocolFloor(ctx) — TLS 1.2 floor
ConfigureCipherList(ctx, list) — optional TLS 1.2 cipher pinning
composed by ConfigureSslContext via && chain. Each helper does one
thing at one level of abstraction, named for what it does. Four
duplicated SSL_CTX_free + return NULL branches collapse to a
single cleanup site in CreateSslContext.
Minor ordering shift: SSL_CTX_set_verify now runs after (rather
than between) the fallible configuration steps — grouped with the
trust-chain concern it belongs to. Semantically equivalent; no
test pins the ordering.
All 12+ CreateSslContext-facing tests stay green: identical OpenSSL
call set with unchanged arg captures, call counts, and CTX-free
counts on failure paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two concerns — building the BIO_METHOD vtable and using it to allocate a BIO — are now separate functions, each with a single exit and one clear job. CreateTransportBioMethod handles BIO_meth_new + BIO_meth_set_*; returns NULL if allocation fails (guarding the setters). CreateTransportBio reads as: get the method, use it to make a BIO, clean up inline if BIO allocation fails so the BioNewFailureFreesBioMethodInline contract still holds. Three returns drop to one per function. No behaviour change — OpenSSL call sequence, arg captures, and failure-path cleanup counts all match the existing BIO-pathway tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch/case with two returns becomes a single return over a result accumulator that defaults to 0 (failure) and is set to 1 for the recognised commands. Same behaviour — the three BIO_CTRL tests stay green. Last multi-return function in SolidSyslogTlsStream.c. The file is now fully single-exit, positive-logic, with every function doing one thing at one level of abstraction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Function bodies are now in the "helpers directly beneath first
caller" order that the rest of the codebase follows:
Create
Destroy
ReleaseHandshakeState (Destroy's helper, also shared with Close)
Open
InitSslContext (1st step)
CreateSslContext
ConfigureSslContext
ConfigureTrustAnchors
ConfigureProtocolFloor
ConfigureCipherList
InitSslSession (2nd step)
AttachTransportBio (3rd step)
CreateTransportBio
CreateTransportBioMethod
TransportBioCreate (registered callbacks)
TransportBioCtrl
TransportBioRead
TransportBioWrite
ConfigureExpectedHostname (4th step)
PerformHandshake (5th step)
Send
Read
Close
Reading top-to-bottom takes the reader from public API down through
the handshake pipeline into the raw OpenSSL wiring at the bottom.
Each scroll-down goes one level deeper.
Forward declarations at the top are now alphabetical — a predictable
table of contents that doesn't need manual re-sorting when helpers
are added.
No behaviour change; full test / tidy / format run green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every "if X != NULL, free X, null X" block in the file now lives in a one-job helper named for the resource it releases: ReleaseSsl — frees and nulls stream->ssl ReleaseBioMethod — frees and nulls stream->bioMethod ReleaseSslContext — frees and nulls stream->ctx Destroy becomes two lines of named operations: ReleaseHandshakeState(&instance); ReleaseSslContext(&instance); ReleaseHandshakeState becomes: ReleaseSsl(stream); ReleaseBioMethod(stream); CreateTransportBio's inline BIO_meth_free cleanup collapses to ReleaseBioMethod(stream) — same operation, same name. No behaviour change; all lifecycle-count tests and the integration suite stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The four function-pointer slots on the base struct are constant for
the lifetime of the module — they never change after Create and
never vary between instances (there is only one instance). Lifting
them into a file-scope const VTABLE and assigning it as a struct
copy in Create communicates that invariant and collapses four
per-call writes into one.
Create now reads:
instance.config = *config;
instance.base = VTABLE;
No behaviour change; test/tidy/format all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every static function in the file gets the TlsStream_ prefix, so the 21 internal-linkage identifiers are unique across the codebase and stack traces / debuggers name the module explicitly — e.g. TlsStream_Open instead of bare Open (which collides with Open in PosixTcpStream.c). Addresses MISRA C:2012 Rule 5.9 (internal-linkage identifier uniqueness) for this file. Consistent with the SolidSyslogTlsStream_ prefix on the public API: public gets the full project name, private gets the module-name shorthand. Rest of the codebase (PosixTcpStream.c, PosixUdpSender.c, etc.) will follow the same pattern in a separate chore; this commit establishes the convention on the file we've been refactoring. No behaviour change; test/tidy/format all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Uniform static inline on every private function in the file. The compiler was free to inline these at -O2 anyway; the explicit keyword: - makes intent visible at the declaration - gives the same signal at -O0 (debug builds) - does not break vtable / callback address-taking — static inline still generates an addressable definition when needed MISRA C:2012 has no rule against static inline. Coverage remains 100% (gcov attributes source lines regardless of inlining). No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 752 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
BIO_set_data and BIO_get_data shared a single global storage slot, so
multiple BIOs aliased — bio2's data overwrote bio1's. BIO_new also
returned a singleton pointer, hiding the aliasing further (every BIO
was the same pointer).
Replaced with a small fixed-size pool. Each pool slot is independent;
the address of slot.slot is the BIO handle, slot.data is the per-BIO
storage that backs BIO_set_data / BIO_get_data. BIO_new hands out the
next free slot, Reset zeros the pool. Pool size 4 is plenty for current
tests (production uses one BIO per Open); easy to bump if a future test
needs more.
The existing global lastSetDataArg is preserved so the existing
OpenSslFake_LastSetDataArg accessor (which reports "the most recent
data passed to BIO_set_data") continues to work.
Driven by BioDataIsStoredPerInstance in OpenSslFakeTest.cpp:
- BIO_new returns distinct pointers per call
- BIO_get_data(bio1) returns the data set on bio1, not on bio2
Closes the last outstanding S03.08 acceptance bullet ("OpenSslFake
stores BIO data per-instance").
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 753 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
This was referenced Apr 22, 2026
Closed
7 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.
Closes #172
Purpose
Harden
SolidSyslogTlsStreambeyond the walking-skeleton defaults landed inS03.07. Acceptance-driven by #172: hostname and cert verification on every
OpenSSL call path, explicit cipher-pinning knob for SL4, BIO_METHOD lifecycle
cleanup, idempotent Close/Destroy, and an in-process integration harness
against real libssl so SL4 failure modes can be unit-tested without BDD.
Change Description
New test tier: OpenSSL integration tests (
Tests/OpenSslIntegration/).A CppUTest executable that links the real libssl (unlike
SolidSyslogTestswhich link-interposes via
OpenSslFakes), with support componentsTlsTestCert,TlsTestServer, andBioPairStream. Pump-on-read patternlets our synchronous
SSL_connectrun over non-blocking BIO pairs withoutthreads. CI runs this in a new
openssl-integrationjob in parallel withbuild-and-test; details in.github/workflows/ci.yml.Return-value checks on every OpenSSL call in
Open—SSL_CTX_new,SSL_CTX_load_verify_locations,SSL_CTX_set_min_proto_version,SSL_CTX_set_cipher_list,SSL_new,BIO_meth_new,BIO_new,SSL_set_tlsext_host_name,SSL_set1_host. Each failure aborts Open andreleases what was allocated on the way in. Each one driven red-first by a
fake-based unit test and a
OpenSslFake_Set*Failsswitch.Per-Open BIO_METHOD lifecycle. Allocate in Open (via CreateTransportBio),
store on the instance, free in Close, backup free in Destroy. Symmetric with
SSL/SSL_CTX. Close nulls the pointer so Destroy can't double-free.
Cipher pinning knob.
SolidSyslogTlsStreamConfig.cipherListis forwardedto
SSL_CTX_set_cipher_listif non-NULL. Library ships no baked-in SL4 list —policy stays with the caller (per-ticket decision).
docs/iec62443.mddocuments a starting-point example.
Three characterisation tests in the harness (expired cert, wrong CN/SAN,
wrong CA) went green on first write — OpenSSL defaults already reject these.
They earn their keep as regression protection. A fourth test covers the
libssl-rejects-unknown-cipher path.
Test Evidence
return-value check, every lifecycle invariant, and the cipher-list
plumbing. All red before their production change; strict red/green per
the project's TDD discipline.
Platform/OpenSsl(and across the library —1216/1216 lines, 272/272 functions).
Commit-level TDD trail in the branch history — each
fix:commit has itsfailing test sitting just above its production change in the diff.
Areas Affected
Platform/OpenSsl/Interface/SolidSyslogTlsStream.h—cipherListfieldadded to
SolidSyslogTlsStreamConfig.Platform/OpenSsl/Source/SolidSyslogTlsStream.c— all substantiveproduction changes. No change to the public C API surface beyond the
config field.
Tests/OpenSslIntegration/— new directory, new CMake target, new CI job.Tests/Support/OpenSslFake.{c,h}— new failure switches and BIO_meth_freestub.
Tests/SolidSyslogTlsStreamTest.cpp— 20 new tests..github/workflows/ci.yml— newopenssl-integrationjob, newquality-monitor tools entry.
docs/iec62443.md— SL4 TLS substrate narrative and traceability matrixupdate. (Status stays "Planned" — S03.11 owns the formal promotion.)
DEVLOG.md— appended session entry.Tests/CMakeLists.txt—add_subdirectory(OpenSslIntegration).Branch protection: after merge, add
openssl-integrationto therequired status checks (repo setting, not in the diff).
Deferred from this story — follow-ups for later:
OpenSslFakeper-BIO data refactor (no test fails without it; revisitfor mTLS / S03.09).
max_proto_versioncontrol.Summary by CodeRabbit
New Features
Tests
Chores
Documentation