docs: S23.22 define the TLS contract, then hold both adapters to it - #730
Merged
Conversation
The TLS platform pages could only describe divergence, and there was nothing to diverge from. This is the contract they measure against: eleven obligations, written from the RFC and from intent rather than distilled from the two adapters, so it yields a gap list rather than a description of the status quo. The policy it opens with is that a fault is reported and delivery continues, because a reporting channel that fails closed is a way to blind the collector at the moment it matters. Peer identity is the one exception, since continuing there means delivering to whoever answered. Part of #708
Contributor
WalkthroughThe PR adds a shared TLS Stream contract, documents Mbed TLS and OpenSSL differences, updates compliance and threat-model guidance, wires the new page into MkDocs, and expands ChangesTLS and structured-data contracts
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 10, 2026
Most of what the page said is the contract's to say once the contract exists — the protocol floor, the mandatory trust anchors, the three states of the declared peer identity, key custody, and revocation, which is now a stated non-obligation rather than a per-platform caveat. All of it comes off. What is left is what this adapter alone can say: it needs OpenSSL 3.0, its credentials are file paths rather than handles, and rotation is therefore a file replacement. Then four divergences from the contract, each tracked: #734, #731, #733, #732. Shorter than before despite gaining the divergences, which is the measure of how much was restated. Part of #708
Same treatment as the OpenSSL page. The protocol floor, the declared peer identity, key custody and revocation come off, being the contract's to state. What is left is what this adapter alone can say: it compiles against your own config, its credentials are pre-built handles rather than paths so rotation means rebuilding the stream, and its coexistence guarantee, which is the one thing here no contract covers. Five divergences, each tracked: #718, #719, #731, #733, #732. It is one more than the OpenSSL page carries, which is the honest count rather than a judgement about either. The contract gains a clause while it is here: a key that does not match its certificate is the same mistake as half a credential, reached differently, and is detectable without going near the network — so it is reported at the same point. Without it #719 had no obligation to be measured against. Part of #708
A device issued a new certificate while running should use it without being restarted. Both adapters already re-read their credential material on every connection, so replacing it behind the same path or handle works — the Mbed TLS page said otherwise and was wrong, and neither page documented that the material must not be replaced under a live connection. What does not work is redirecting a stream at a different source, and ServerName is fixed the same way. That last one is the sharper half: the destination is runtime-reconfigurable through the endpoint callback while the identity its certificate is checked against is not, so re-homing a device leaves it verifying the old name or, where no name was declared, verifying nothing. Address dynamic and identity frozen is the wrong shape. Tracked as #735. The contract obligation now covers both, and the page states the API change that closes it as a planned pre-1.0 break rather than leaving integrators to meet it in a release note. Part of #708
Every one of the seven RFC 5425 citations was wrong. Section 5 is Security Policies — 5.1 to 5.5 are authorization cases — while the requirements the matrix describes live at 3, 4.1, 4.2, 4.2.1, 4.2.3, 4.3.1 and 4.4. Each row is now verified against the section's own text, and the four RFCs read in numeric order. Statuses are stated against the TLS contract, with the general caveat at the top of the page: a status describes the library with a conforming platform under it, and a platform that falls short records the exception on its own page. Beyond the citations: the cipher row carried three different requirements and is split; a row is added for the key-pair MUST the library does not meet, since omitting it is the overstatement being removed everywhere else; and 4.2.3 drops to Partial, the only downgrade, because neither shipped platform lets an administrator select the cryptographic level on the connection that gets negotiated. SD-NAME conformance (6.3.2) is Supported rather than Planned. The element writer owns the framing and bounds each name; the three remaining exclusions are the author's, and now say so on the API instead of being contradicted by a safety claim the code did not keep. Names are authored rather than carried, so an invalid one fails on the first run. sequenceId wrap (7.3.1) is Supported rather than Partial: the wrap is implemented and tested, and the caveats the status was carrying — the concurrent-raise reorder window, the Null counter on pool exhaustion — are not that requirement. Nothing is Planned any more, so the status and its column go. Every count re-derived from the rows rather than carried forward. Part of #708
Both adapters dereference the pointers they were given on every Open — the credential paths, the Mbed TLS handles and ServerName alike. An integrator who owns the buffer or the handle replaces the material, forces a reconnection, and the next connection uses it. A device issued new credentials while running already uses them without restarting. What was written up as a gap — redirecting a stream at a differently allocated handle or a second path — is a convenience nobody has asked for, inferred from the config being copied at create and then stated as a limitation on a compliance page. It comes off the two platform pages and off CR 1.5 and CR 1.8. The planned breaking change goes with it. It rested on the same reading, and announcing a break that is not coming would have integrators insulating setup code for nothing. #735 is rewritten as what is actually left: the TLS stream has no change detection to match the endpoint's version function, so rotation needs a Disconnect the integrator must know to call. Additive if it is ever done. IEC 62443 keeps the rest: CR 1.5 and CR 1.8 now point at the TLS obligations for what a stream must do and for why revocation is not required, rather than restating either. Part of #708
CRA gains a doorway and nothing else: the page is short, well framed and names no backend, so the TLS work here is one sentence where the answer below is TLS, plus an entry in Where to go next. The threat model needed more. Its defended-by-construction list still claimed cipher pinning, which is the overstatement that started this whole thread — the library pins nothing of its own, one backend's list binds TLS 1.2 and below, the other has no cipher configuration at all. Replaced with what is actually defended, and linked to the contract. The obligations table likewise asked the integrator to supply a cipher policy, which reads as a thing that takes effect. Part of #708
Five corrections from reading the set end to end. The policy claimed peer-identity mismatch was its one exception, while the obligations below it block in two further cases — no trust anchors, and a chain that does not validate. It is one principle rather than one exception: delivery stops when the library cannot establish who the peer is, and continues when the peer is established but a credential is imperfect. The 0.1.0 note said both platforms refuse the connection on a partially configured credential. Only one does. The other accepts it in silence and connects without the client certificate, which is the half a reader relying on mutual TLS most needs, and it was the half being flattened. The create-time check listed a random source among the things a stream cannot work without. Only one platform has one, so that is a platform specific in a platform-independent contract; it points at the platform pages instead. The protocol floor sets no ceiling and the cipher obligation asks for the integrator's policy to be passed through, which read as being in tension four sections apart. They are joined now: a policy binding only up to the floor does not bind the connection in use. And the identity field is documented on each platform's config, not a single one. Part of #708
DavidCozens
marked this pull request as ready for review
August 10, 2026 15:14
![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: 12
🤖 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 `@Core/Interface/SolidSyslogSdElement.h`:
- Around line 4-5: Update the documentation comment for SolidSyslogSdElement and
related declarations to assign ownership accurately: SolidSyslogSdElement
handles element and parameter framing, including brackets and separators, while
SolidSyslogSdValue handles SD-PARAM-VALUE escaping. Remove wording that
attributes value escaping to SolidSyslogSdElement.
- Line 31: Rename the first parameter of SolidSyslogSdElement_Begin from its
current concrete-instance name to self in both the header declaration and
function definition, updating any references within the definition accordingly.
In `@docs/cra.md`:
- Around line 66-68: Update the sentence around the TLS obligations link to use
UK English and explicitly refer to the linked page, correcting the subject–verb
agreement while preserving the existing meaning.
In `@docs/iec62443.md`:
- Line 42: In the CR 1.5 description, update the sentence in the Stream role
explanation by adding a comma before “so” between the two independent clauses;
leave the surrounding TLS obligations and authenticator-management content
unchanged.
- Line 43: Update the TLS obligations list in docs/tls.md to remove revocation
checking or clearly distinguish deployment revocation policy from
backend-specific checking, matching the behavior described for the Mbed TLS and
OpenSSL adapters. Keep the CR 1.8 wording in docs/iec62443.md and the
threat-model wording consistent with this contract.
In `@docs/platforms/mbedtls/index.md`:
- Around line 32-36: Update the sentence beginning “But while a connection is
open” to add a comma after “open,” before the adapter’s `ssl_config` clause,
without changing the surrounding explanation.
In `@docs/platforms/mbedtls/setup.md`:
- Around line 4-6: Correct the subject–verb agreement in the introductory
paragraph of docs/platforms/mbedtls/setup.md lines 4-6 and
docs/platforms/openssl/setup.md lines 4-6 by changing “covers” to “cover” after
the plural “TLS obligations” link, or rewriting the subject as singular while
preserving the meaning and UK English style.
In `@docs/rfc-compliance.md`:
- Line 41: Clarify the BOM wording in the RFC compliance table entry by
explicitly stating that a leading BOM in the caller’s body is stripped, ensuring
the wire frame contains exactly one BOM. Preserve the surrounding UTF-8 and
truncation details.
- Around line 38-39: Update the RFC compliance notes to use full public API
identifiers wherever they refer to types, including SolidSyslogOriginSd and
SolidSyslogMetaSd; if “origin SD” or “meta SD” is intended as a prose concept,
label it explicitly as such. Preserve the existing RFC parameter and behavior
descriptions.
- Line 59: Update the cipher-suite row in the RFC compliance table to describe
TLS_RSA_WITH_AES_128_CBC_SHA as optional rather than mandatory, retain its N/A
status and the existing compliance counts, and add an RFC 9662 citation
alongside the RFC 5425 reference.
In `@docs/tls.md`:
- Around line 178-196: Update the “Where this stands at 0.1.0” section to remove
the repeated expired-certificate, partial-client-credential, and create-time
configuration details. Replace them with an aggregate status or links to the
authoritative platform pages, keeping mechanism-specific exceptions documented
only on those pages.
- Around line 24-30: Align the peer-identity policy across the documentation
contract, both backend implementations, and the compliance matrix: either
require a declared server name and identity matching everywhere, or explicitly
preserve the permitted NULL/empty ServerName chain-only behavior everywhere.
Update the documented delivery-stop rule and corresponding backend fields and
matrix entries consistently.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87122dc9-157c-4033-bad4-1dc7c2af9225
📒 Files selected for processing (12)
Core/Interface/SolidSyslogSdElement.hdocs/cra.mddocs/iec62443.mddocs/platforms/mbedtls/index.mddocs/platforms/mbedtls/setup.mddocs/platforms/openssl/index.mddocs/platforms/openssl/setup.mddocs/rfc-compliance.mddocs/security/threat-model.mddocs/tls.mdhooks/page_descriptions.pymkdocs.yml
The review found the compliance matrix written against the unrevised RFC 5425. RFC 9662 is Standards Track and updates it: the 2009 mandatory cipher suite is downgraded to MAY because it offers no forward secrecy, ECDHE-GCM is what SHOULD be offered instead, and TLS 1.3 SHOULD be supported and MUST be preferred where implemented. We meet all of it, and the last part settles something that was a judgement call this morning. The contract sets a floor and no ceiling, so 1.3 is negotiated wherever the peer offers one — that is now a requirement rather than a preference, and it rules out pinning a ceiling to make a cipher list bite. Both floor sites say so, so it cannot be undone while fixing #733. BCP 195 arrives as a normative reference of RFC 9662 rather than as background. It is cited on the contract where it corroborates an obligation — the floor, the cipher shape, what the name check is for — and once where it needed care rather than corroboration: its revocation guidance is addressed to servers, so a client library declining to check is not a departure from it. No rows, no compliance claim: it is advice on using TLS, most of which is the integrator's. Also from the review: the policy said delivery stops when the peer cannot be established, which contradicted the contract's own permission for an undeclared identity two sections later. It stops when the peer fails the check the integrator asked for, and declaring no identity is a decision rather than a failure. The IEC 62443 doorway listed revocation among the things a stream must do, when the contract says the opposite. And the SD element was credited with the value escaping, which belongs to the value sink it hands back. Part of #708
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/iec62443.md`:
- Around line 54-57: Update the paragraph around “device to the collector” in
the IEC 62443 documentation to remove repeated TLS protocol, peer-verification,
endpoint-identity, mutual-TLS, and revocation claims. Keep a link to docs/tls.md
as the authoritative TLS contract and retain links to backend pages for current
limitations, while refocusing the paragraph on IEC 62443 mapping and key-storage
responsibility.
In `@docs/platforms/mbedtls/setup.md`:
- Around line 4-6: Complete the second clause in both introduction sentences: in
docs/platforms/mbedtls/setup.md lines 4-6 and docs/platforms/openssl/setup.md
lines 4-6, state that the respective Mbed TLS or OpenSSL page covers what the
adapter needs and where it falls short of the TLS obligations. Use clear UK
English and preserve the existing links and surrounding documentation.
In `@docs/rfc-compliance.md`:
- Around line 54-60: Update docs/rfc-compliance.md lines 54-60 to describe RFC
9662 as updating RFC 5425 and state that its updates apply to the existing RFC
5425 rows, while retaining the exact offer and preference requirements. In
docs/rfc-compliance.md lines 67-68, change both RFC 9662 §4.2 references to RFC
9662 §4. In docs/tls.md lines 107-110, change the RFC 9662 citation to §4 and
retain BCP 195 §4.2 for the separate TLS 1.2 guidance.
In `@docs/tls.md`:
- Around line 155-157: Revise the paragraph near the BCP 195 §7.5 discussion to
explicitly identify the project’s lack of revocation checking as a BCP 195
deviation, rather than claiming alignment with current TLS practice. Distinguish
client soft-fail behavior from the requirement to implement a
revoked-certificate distrust strategy, and link the applicable compliance or gap
record.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6778cac-cc17-4c4a-8cbf-bee6d0d29101
📒 Files selected for processing (11)
Core/Interface/SolidSyslogSdElement.hPlatform/MbedTls/Source/SolidSyslogMbedTlsStream.cPlatform/OpenSsl/Source/SolidSyslogOpenSslStream.cdocs/cra.mddocs/iec62443.mddocs/platforms/mbedtls/index.mddocs/platforms/mbedtls/setup.mddocs/platforms/openssl/setup.mddocs/rfc-compliance.mddocs/tls.mdmisra_suppressions.txt
BCP 195 §7.5 requires a TLS implementation to implement a strategy to distrust revoked certificates, and no stream here implements one. The contract said the opposite — that declining to check was not a departure from practice — on the strength of the section's server-facing half. That is the shape of overstatement this audit exists to remove, written into the round that was removing them. The decision stands and the reasoning is unchanged. What changes is that it now reads as a deviation with reasons, and says where the obligation can still be met: the integrator's own TLS library can be configured for CRL or OCSP, and this library neither performs that check nor prevents it. An assessment needing the obligation met should say where. RFC 9662 §4 has no subsections — the numbers I cited are its references to RFC 5425's own sections. Three citations corrected, and the note about the section contradicting itself reworded, since that tension is inside §4 rather than between subsections. The IEC 62443 doorway named the four obligations, which is a second copy with nothing asserting it: add one to the contract and that list goes stale in silence. It points at the contract instead. Part of #708
Contributor
☀️ Quality Summary Created by Quality Monitor v4.15.0 (#82d77af). More details are shown in the GitHub Checks Result. |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/iec62443.md (1)
42-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake the mutual-TLS compliance claim conditional on deployment configuration.
A TLS backend can present a client credential, but the collector authenticates the device only when it requests and validates that credential. CR 1.8 also covers collector certificate and endpoint identity checks; it is not equivalent to mutual TLS.
State that these controls depend on the TLS configuration. State that mutual TLS additionally authenticates the device when the collector requires and validates the client credential.
As per path instructions, verify behavioural claims against the implementation and keep TLS obligations in
docs/tls.md.Proposed wording
-| **CR 1.5** — Authenticator management | The Stream role carries transport security, and a TLS backend filling it can present a client credential, so the collector authenticates the device. ... +| **CR 1.5** — Authenticator management | The Stream role carries transport security, and a TLS backend filling it can present a client credential when the deployment configures mutual TLS; the collector authenticates the device only when it requests and validates that credential. ... ... -The identity controls (CR 1.5, CR 1.8) are met by mutual TLS, which authenticates the -device to the collector. +The identity controls depend on the TLS configuration. Server certificate and endpoint +identity checks authenticate the collector; mutual TLS additionally authenticates the +device when the collector requests and validates a client credential.Also applies to: 53-58
🤖 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 `@docs/iec62443.md` around lines 42 - 43, Update the CR 1.5 and CR 1.8 entries to make authentication and certificate-validation claims conditional on the deployment’s TLS configuration. Clarify that mutual TLS authenticates the device only when the collector requests and validates the supplied client credential, while CR 1.8 separately covers collector certificate and endpoint identity checks. Verify these behavioral claims against the implementation and keep the detailed TLS requirements in the existing TLS obligations documentation.Source: Path instructions
🤖 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 `@docs/rfc-compliance.md`:
- Line 68: Update the RFC 9662 §4 entry in docs/rfc-compliance.md so TLS 1.3 is
not unconditionally marked Supported: either add negotiation tests for both
backends and retain that status, or document the Mbed TLS/external-configuration
limitation and adjust the status accordingly. Ensure the explanation reflects
the actual configured test ceiling and negotiated-version coverage.
---
Outside diff comments:
In `@docs/iec62443.md`:
- Around line 42-43: Update the CR 1.5 and CR 1.8 entries to make authentication
and certificate-validation claims conditional on the deployment’s TLS
configuration. Clarify that mutual TLS authenticates the device only when the
collector requests and validates the supplied client credential, while CR 1.8
separately covers collector certificate and endpoint identity checks. Verify
these behavioral claims against the implementation and keep the detailed TLS
requirements in the existing TLS obligations documentation.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a998d38-e04b-4dab-806f-54af4523581a
📒 Files selected for processing (5)
docs/iec62443.mddocs/platforms/mbedtls/setup.mddocs/platforms/openssl/setup.mddocs/rfc-compliance.mddocs/tls.md
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.
Purpose
The TLS platform pages could only usefully describe divergence, and nothing
defined what they diverge from — so the contract page was scheduled after the
pages that needed it. This breaks that cycle: the contract first, then both
adapters written against it, then the compliance statements written against the
contract rather than against whichever adapter was read last.
Part of #708
Change Description
docs/tls.md(new, under Integrate). Written from RFC 5425 and from intent,deliberately not distilled from what the two adapters happen to do — that
would bless the status quo and produce no gap list. Thirteen obligations, opening
with the policy that a fault is reported through the error handler while delivery
continues, because a reporting channel that fails closed is a way to blind the
collector at the moment it matters. The limit is one line rather than a list of
exceptions: delivery stops when the library cannot establish who the peer is.
The two TLS platform pages, rewritten to measure against it. The protocol
floor, mandatory trust anchors, declared peer identity, key custody and
revocation come off both — they are the contract's to state. What is left is what
each adapter alone can say, plus its divergences: four for OpenSSL, five for
Mbed TLS. The OpenSSL page is shorter than before despite gaining them, which
is the measure of how much was being restated.
The compliance pages, pointed at the contract rather than restating it. IEC
62443's CR 1.5 and CR 1.8, one doorway sentence and a nav entry on CRA, and the
threat model, whose defended by construction list still claimed cipher pinning
— the overstatement that started this work. The library pins nothing of its own.
docs/rfc-compliance.md. Every one of the seven RFC 5425 citations waswrong: section 5 is Security Policies, and 5.1–5.5 are authorization cases, not
certificates, ciphers, framing and closure. Each row is re-cited against the
section's own text and the four RFCs now read in numeric order. Beyond the
citations: the cipher row carried three separate requirements and is split; a row
is added for the key-pair MUST the library does not meet; §4.2.3 drops to
Partial; SD-NAME conformance and the sequenceId wrap both move up to Supported.
Nothing is Planned any longer, so that status and its column go, and every count
was re-derived from the rows rather than carried forward.
Core/Interface/SolidSyslogSdElement.hclaimed the element owns the SD-NAMEcharset so an author "cannot desync the framing". It owns a superset —
PRINTUSASCII — and
]in a name does desync it. The guarantee is split so thetrue half survives where it holds: a value cannot desync the framing whatever it
contains; a name is the author's. The three exclusions are now stated on the API.
Deliberate decisions worth knowing
receiver is the enforcement point for our own credential. This makes both
adapters non-conforming in opposite directions, which is the sign the contract
is real rather than a description of whichever was read first.
industrial deployments have no route to a responder.
runtime data, so an invalid one fails visibly on first run — deliberate, and
the reason the compliance row reads Supported rather than the header claiming a
safety property it does not deliver.
Gaps raised, all milestoned 0.2.0
#718 (widened), #719, #731, #732, #733, #734 — and #735, rewritten from a wrong
premise into what is actually left. An earlier commit on this branch claimed a
running stream could not be given new credentials; it can, and the retraction is
in the history and on #650.
Test Evidence
Documentation, plus comment-only changes to one header.
mkdocs build --strict,markdownlint over 63 files,
python3 scripts/check_platform_docs.py(10platforms, 12 roles, boundary clean), 76 hook tests in the pinned image, and
clang-format --dry-run --Werroron the header — all green. No MISRA suppressionreferences that file, so no renumbering. No production logic changed, so nothing
to unit test.
RFC citations were verified against the published RFCs rather than against the
existing table; the evidence ledger backing that is kept for a manual check.
Areas Affected
docs/and one Core header comment. No production logic, no build files, nopublic API change.
Summary by CodeRabbit