feat: S03.09 mutual TLS — client cert + key for SL4 non-repudiation#184
Conversation
Pins current behaviour: when clientCertChainPath and clientKeyPath are both NULL, Open must not call SSL_CTX_use_certificate_chain_file, SSL_CTX_use_PrivateKey_file, or SSL_CTX_check_private_key. Adds the three call counters + failure injectors to OpenSslFake ready to drive the mTLS happy path in subsequent cycles.
… set When clientCertChainPath is configured, Open now calls SSL_CTX_use_certificate_chain_file, SSL_CTX_use_PrivateKey_file and SSL_CTX_check_private_key in order, wiring up client-side TLS identity for mTLS. All three return values are checked — any failure aborts Open and the ctx is freed by the existing error path.
When exactly one of clientCertChainPath and clientKeyPath is set, Open now fails without making any libssl client-identity calls, and the ctx is freed via the existing error path. Prevents a silent downgrade into server-auth TLS when the caller intended mTLS but supplied incomplete material.
Covers SSL_CTX_use_certificate_chain_file, SSL_CTX_use_PrivateKey_file and SSL_CTX_check_private_key: each return-checked, each failure propagates through Open, each call receives the ctx from SSL_CTX_new. Inherits the existing CreateSslContext rollback — no production change needed, the assertions just formalise it.
Adds TlsTestCert_WritePrivateKeyPemToFile so integration scenarios can stage a client key on disk for SolidSyslogTlsStream to load. Adds TlsTestServerConfig.clientCaCert — when set, the in-process server trusts the given CA for client certs and insists on peer verification via SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT.
Four scenarios cover the SL4 CR 2.12 failure surface for client authentication: - handshake succeeds when the client cert is signed by the CA the server trusts; - server rejects the handshake when the client presents no cert; - Open fails locally via SSL_CTX_check_private_key when the key does not match the cert; - server rejects the handshake when the client cert is signed by a CA the server does not trust. TlsTestCert now emits basicConstraints=CA:TRUE so OpenSSL 3 will build a chain through CA-signed client certs. TlsTestServer pins TLS 1.2 in mTLS mode — in TLS 1.3 the client's SSL_connect can return before the server verifies the client cert, which would hide a server-side rejection until the next read.
- syslog-ng gains a second TLS source on port 6515 with peer-verify(required-trusted), leaving the existing 6514 source on optional-untrusted so both paths stay under test. - generate.sh now emits client.pem + client.key signed by the BDD test CA; all test certs regenerated with matching CA. - ExampleMtlsConfig provides host, port, CA bundle, server name and client cert/key paths; the threaded example accepts --transport mtls and wires those paths into SolidSyslogTlsStreamConfig when selected. - New @Mtls @Buffered feature asserts a message is delivered end-to-end over mTLS. The SwitchingSender config maps 'mtls' to the TLS slot since the TLS stream is a singleton — mTLS is a flavour of TLS rather than a new transport slot.
- Fold tls/mtls branches into one in ExampleSwitchConfig (branch-clone). - Replace strcpy-based temp file template with a templated helper that uses memcpy and a static_assert for the destination size. - Re-run clang-format on files touched this story.
|
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 7 minutes and 19 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 (5)
📝 WalkthroughWalkthroughThis PR implements mutual TLS (mTLS) support for Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant Stream as SolidSyslogTlsStream
participant OpenSSL as OpenSSL Library
participant Server as syslog-ng Server
Client->>Stream: Open(tlsConfig with clientCertChainPath, clientKeyPath)
Stream->>OpenSSL: SSL_CTX_new()
OpenSSL-->>Stream: ctx
Stream->>OpenSSL: SSL_CTX_use_certificate_chain_file(clientCertChainPath)
OpenSSL-->>Stream: success/failure
alt Certificate load failed
Stream->>OpenSSL: SSL_CTX_free(ctx)
Stream-->>Client: false (Open failed)
end
Stream->>OpenSSL: SSL_CTX_use_PrivateKey_file(clientKeyPath, PEM)
OpenSSL-->>Stream: success/failure
alt Key load failed
Stream->>OpenSSL: SSL_CTX_free(ctx)
Stream-->>Client: false (Open failed)
end
Stream->>OpenSSL: SSL_CTX_check_private_key(ctx)
OpenSSL-->>Stream: match/mismatch
alt Key mismatch
Stream->>OpenSSL: SSL_CTX_free(ctx)
Stream-->>Client: false (Open failed)
end
Stream->>OpenSSL: SSL_connect()
OpenSSL->>Server: TLS Handshake (presents client cert)
Server->>Server: Verify client cert against CA
alt Client cert invalid/untrusted
Server-->>OpenSSL: Handshake failure
OpenSSL-->>Stream: error
Stream-->>Client: false (Open failed)
end
alt Client cert valid
Server-->>OpenSSL: Handshake success
OpenSSL-->>Stream: connected
Stream-->>Client: true (Open succeeded)
end
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 🚦 Unit Tests (GCC): 100% successful (✔️ 771 passed, 🙈 3 skipped) Created 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Bdd/features/steps/syslog_steps.py (1)
22-27:⚠️ Potential issue | 🟠 MajorUse the mTLS oracle log before waiting for the one-shot transport send.
PER_TRANSPORT_LOG["mtls"]lets theThen ... over mtlsstep countreceived_mtls.log, butrun_threaded_example()still waits and parses viacontext.received_logduring theWhenstep. For the new mTLS scenario, pointcontext.received_logandcontext.lines_beforeat the per-transport log before launching the threaded example.🐛 Proposed fix
`@when`("the threaded example sends a syslog message with transport {transport}") def step_threaded_sends_with_transport(context, transport): + if context.oracle_format == "syslog-ng" and transport in PER_TRANSPORT_LOG: + context.received_log = PER_TRANSPORT_LOG[transport] + context.lines_before = context.lines_before_per_transport.get( + transport, + line_count(context.received_log), + ) run_threaded_example(context, ["--transport", transport])Also applies to: 577-579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Bdd/features/steps/syslog_steps.py` around lines 22 - 27, The test uses PER_TRANSPORT_LOG["mtls"] for counting but still points context.received_log and context.lines_before at the generic log when launching the one-shot threaded sender; update the When-step before calling run_threaded_example() so that for the "mtls" transport you set context.received_log = PER_TRANSPORT_LOG["mtls"] and context.lines_before = len(context.received_log) (same change where other transports set these), ensuring run_threaded_example() and subsequent checks use RECEIVED_MTLS_LOG; mirror this fix wherever the one-shot mtls path is handled (also at the other occurrence around lines 577-579).
🧹 Nitpick comments (2)
Bdd/syslog-ng/tls/generate.sh (1)
41-43: Optional: consider addingextendedKeyUsage=clientAuthto the client cert.Signing
client.pemviaopenssl x509 -reqwithout an-extfileemits a cert with no v3 extensions (including no EKU). syslog-ng'speer-verify(required-trusted)happens to accept this today, but stricter verifiers (or a future OpenSSL default tightening) may reject a client cert that lacksextendedKeyUsage=clientAuth. Adding an explicitclientAuthEKU (mirroring howserver.pemcarries its SAN extfile) would make the test fixture more robust and closer to real-world client certs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Bdd/syslog-ng/tls/generate.sh` around lines 41 - 43, The client certificate signing step (openssl x509 -req producing client.pem from client.csr) currently omits v3 extensions; add an extfile that sets extendedKeyUsage = clientAuth (similar to the server cert SAN extfile) and pass it to the signing command (use -extfile and -extensions) so client.pem explicitly contains the clientAuth EKU, ensuring stricter verifiers accept the cert.Tests/Support/OpenSslFake.c (1)
147-151: Capture the private-key file type argument for unit test assertion.
SSL_CTX_use_PrivateKey_filediscards thetypeparameter, preventing unit tests from asserting thatSSL_FILETYPE_PEMis passed. Add a static variable and accessor to capture this argument, then assert it in the mTLS unit test. Production correctly passes the right value, but the test harness should pin this contract at the unit level.Implementation sketch
Add static variable in
OpenSslFake.c:static int lastUsePrivateKeyFileTypeArg;Update the fake in
OpenSslFake.c:int SSL_CTX_use_PrivateKey_file(SSL_CTX* ctx, const char* file, int type) { usePrivateKeyFileCallCount++; lastUsePrivateKeyFileCtxArg = ctx; lastClientKeyPath = file; lastUsePrivateKeyFileTypeArg = type; return usePrivateKeyFileFails ? 0 : 1; }Expose accessor in
OpenSslFake.h:int OpenSslFake_LastUsePrivateKeyFileTypeArg(void);Assert in mTLS unit test:
STRCMP_EQUAL(SSL_FILETYPE_PEM, OpenSslFake_LastUsePrivateKeyFileTypeArg());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Support/OpenSslFake.c` around lines 147 - 151, Add a static int to capture the `type` arg (e.g., lastUsePrivateKeyFileTypeArg) in OpenSslFake.c, set it inside the fake implementation of SSL_CTX_use_PrivateKey_file alongside usePrivateKeyFileCallCount/lastClientKeyPath/lastUsePrivateKeyFileCtxArg, expose a getter function in OpenSslFake.h (e.g., OpenSslFake_LastUsePrivateKeyFileTypeArg), and update the mTLS unit test to assert STRCMP_EQUAL(SSL_FILETYPE_PEM, OpenSslFake_LastUsePrivateKeyFileTypeArg()) so the test can verify the PEM filetype was passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/iec62443.md`:
- Around line 187-190: Update the epic reference to use the project zero-padding
convention: replace the bare "E3" with "E03" in the markdown snippet and any
other occurrences (e.g., the lines containing "E3" around the E3 work note and
at the other noted location). Verify story references like "S03.10" and "S03.11"
remain zero-padded and unchanged, and commit the corrected epic token "E03" in
docs/iec62443.md so all epic IDs follow E00-E99 formatting.
In `@Example/Threaded/main.c`:
- Around line 109-110: The comment is wrong: setting tlsModeActive = false does
not fall back to TCP because the "tls"/"mtls" transport still selects
EXAMPLE_SWITCH_TLS, which is wired to udpSender; update the code and/or comment
so that when tlsModeActive is false the transport selection for "tls"/"mtls" is
remapped to the TCP branch (or change EXAMPLE_SWITCH_TLS wiring) so it does not
use udpSender; specifically, modify the transport selection logic that checks
tlsModeActive and EXAMPLE_SWITCH_TLS to route to the TCP sender path instead of
udpSender, and update the inline comment to accurately describe the fallback
behavior.
In `@Tests/SolidSyslogTlsStreamTest.cpp`:
- Around line 647-655: The test
SolidSyslogTlsStream/OpenLoadsClientKeyFromConfig currently only asserts the
client key path; modify the OpenSSL fake to record the file type passed into
SSL_CTX_use_PrivateKey_file (add an accessor like
OpenSslFake_LastClientKeyType()) and then update the test to also ASSERT that
OpenSslFake_LastClientKeyType() equals SSL_FILETYPE_PEM after calling
SolidSyslogStream_Open(stream, addr); keep the existing path assertion using
OpenSslFake_LastClientKeyPath() and ensure the fake stores the type parameter
when SSL_CTX_use_PrivateKey_file is invoked.
---
Outside diff comments:
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 22-27: The test uses PER_TRANSPORT_LOG["mtls"] for counting but
still points context.received_log and context.lines_before at the generic log
when launching the one-shot threaded sender; update the When-step before calling
run_threaded_example() so that for the "mtls" transport you set
context.received_log = PER_TRANSPORT_LOG["mtls"] and context.lines_before =
len(context.received_log) (same change where other transports set these),
ensuring run_threaded_example() and subsequent checks use RECEIVED_MTLS_LOG;
mirror this fix wherever the one-shot mtls path is handled (also at the other
occurrence around lines 577-579).
---
Nitpick comments:
In `@Bdd/syslog-ng/tls/generate.sh`:
- Around line 41-43: The client certificate signing step (openssl x509 -req
producing client.pem from client.csr) currently omits v3 extensions; add an
extfile that sets extendedKeyUsage = clientAuth (similar to the server cert SAN
extfile) and pass it to the signing command (use -extfile and -extensions) so
client.pem explicitly contains the clientAuth EKU, ensuring stricter verifiers
accept the cert.
In `@Tests/Support/OpenSslFake.c`:
- Around line 147-151: Add a static int to capture the `type` arg (e.g.,
lastUsePrivateKeyFileTypeArg) in OpenSslFake.c, set it inside the fake
implementation of SSL_CTX_use_PrivateKey_file alongside
usePrivateKeyFileCallCount/lastClientKeyPath/lastUsePrivateKeyFileCtxArg, expose
a getter function in OpenSslFake.h (e.g.,
OpenSslFake_LastUsePrivateKeyFileTypeArg), and update the mTLS unit test to
assert STRCMP_EQUAL(SSL_FILETYPE_PEM,
OpenSslFake_LastUsePrivateKeyFileTypeArg()) so the test can verify the PEM
filetype was passed.
🪄 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: e657f3ef-ce97-4950-9ea5-7c6890dbf7fc
⛔ Files ignored due to path filters (3)
Bdd/syslog-ng/tls/ca.pemis excluded by!**/*.pemBdd/syslog-ng/tls/client.pemis excluded by!**/*.pemBdd/syslog-ng/tls/server.pemis excluded by!**/*.pem
📒 Files selected for processing (28)
.devcontainer/docker-compose.ymlBdd/features/environment.pyBdd/features/mtls_transport.featureBdd/features/steps/syslog_steps.pyBdd/syslog-ng/syslog-ng-full.confBdd/syslog-ng/syslog-ng.confBdd/syslog-ng/tls/ca.keyBdd/syslog-ng/tls/client.keyBdd/syslog-ng/tls/generate.shBdd/syslog-ng/tls/server.keyDEVLOG.mdExample/CMakeLists.txtExample/Common/ExampleCommandLine.cExample/Common/ExampleMtlsConfig.cExample/Common/ExampleMtlsConfig.hExample/Common/ExampleSwitchConfig.cExample/Threaded/main.cPlatform/OpenSsl/Interface/SolidSyslogTlsStream.hPlatform/OpenSsl/Source/SolidSyslogTlsStream.cTests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cppTests/OpenSslIntegration/TlsTestCert.cTests/OpenSslIntegration/TlsTestCert.hTests/OpenSslIntegration/TlsTestServer.cTests/OpenSslIntegration/TlsTestServer.hTests/SolidSyslogTlsStreamTest.cppTests/Support/OpenSslFake.cTests/Support/OpenSslFake.hdocs/iec62443.md
…ILETYPE_PEM assertion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 771 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
CodeRabbit review — remaining itemsFollow-up on the two items that live in the review body (not inline), for traceability: Outside-diff major —
|
1 participant
Purpose
Closes #173.
Ships mutual TLS on
SolidSyslogTlsStream: a client cert chain + matching private key, satisfying IEC 62443 SL4 CR 2.12 (non-repudiation). The SIEM can now cryptographically identify which device sent each audit record.Change Description
API. Two opt-in fields on
SolidSyslogTlsStreamConfig:Semantics:
SSL_CTX_use_certificate_chain_file+SSL_CTX_use_PrivateKey_file(PEM)+SSL_CTX_check_private_key, each return-value-checked, ctx freed on any failure.Rotation. Not a callback — the
SSL_CTXis rebuilt on everyOpen, so on-disk cert rotation is picked up automatically on the next reconnect. S03.10 can revisit if a callback turns out to be needed.Integration harness.
TlsTestCert_WritePrivateKeyPemToFilelets scenarios stage a client key on disk.TlsTestServerConfig.clientCaCertturns the in-process server into a mutual-auth server (SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, TLS 1.2 pinned so server-side rejection is visible withinSSL_connectrather than deferred to the next read as it is in TLS 1.3). Test certs now emitbasicConstraints=CA:TRUEso OpenSSL 3's chain validation builds through CA-signed client certs.BDD. syslog-ng gets a second TLS source on port 6515 with
peer-verify(required-trusted), leaving the existing 6514 source onoptional-untrusted. Both paths stay under test. New@mtls @bufferedscenario;ExampleMtlsConfigwires the threaded example's--transport mtlsmode. The switch config mapsmtlsonto the existing TLS slot (the TlsStream is a singleton, so mTLS is a flavour of TLS — a fourth slot waits on the multi-instance allocation-strategy epic).Docs.
docs/iec62443.mdpromotes mTLS from remaining to available in the SL4 TLS section.Test Evidence
Strict TDD throughout. Each red confirmed before each green, each green committed.
Tests/SolidSyslogTlsStreamTest.cppgains 17 mTLS cases covering: skip path, happy path (cert/key/check invocations + pointer chains), partial-config rejection (both directions), failure injection per libssl call, and ctx-free on every failure path.Tests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cppgains 4 scenarios against real libssl: handshake succeeds with trusted CA-signed client cert; server rejects when client sends nothing; Open fails locally on cert/key mismatch; server rejects when client cert is signed by an untrusted CA.Bdd/features/mtls_transport.featureproves end-to-end delivery over mTLS.Areas Affected
Platform/OpenSsl/(Tier 2):SolidSyslogTlsStream+ its public config header.Tests/Support/OpenSslFake.{h,c}: new captures / counters / failure injectors.Tests/OpenSslIntegration/:TlsTestCert(key PEM writer, CA:TRUE),TlsTestServer(clientCaCert), new mTLS scenarios.Example/(Tier 3): newExampleMtlsConfig;--transport mtlsaccepted; switch config maps it onto the TLS slot.Bdd/: new mTLS source insyslog-ng.conf/syslog-ng-full.conf, regenerated BDD test certs incl. newclient.pem/client.key, newmtlsentry inPER_TRANSPORT_LOG, new feature file.docs/iec62443.md,DEVLOG.md.Summary by CodeRabbit
Release Notes
New Features
Tests