docs: S23.22 read the compliance matrix in order, and fix what it turned up - #752
Merged
Conversation
§7.2 ran to 177 words, nearly double the next longest note, because it restated what its four child rows and §6.3.3 now carry: the escaping, the ip count-then-loop, the per-string limits, and how the config strings are held. It keeps what only it can say — which parameters exist, that each is independently optional, and that a bare [origin] is legal and is what an unwired instance emits. 177 words to 57; §7.3 gets the same treatment, 95 to 49. §7.2.2 now says why the 64-byte bound is 64: §7.2.2 states no length, so it is this library's ceiling, set above the enterprise numbers and sub-identifier OIDs that appear in practice.
S07.05 added it to cap each ip value through the escaped-string writer. S14.05 moved ip emission onto the SD writer, where the integrator's at-callback writes straight into the SolidSyslogSdValue it is handed, so no library code has applied the bound since. It was left declaring a limit the library does not impose.
The BDD targets and the origin SD tests asserted enterpriseId "1.3.6.1.4.1.99999", which is wrong twice over. 99999 is not ours — RFC 5612 reserves 32473 for use in examples, and 99999 points at whatever enterprise holds it. And RFC 5424 §7.2.2 asks for "only the IANA-assigned private enterprise number ... (a single number)", giving "32473.1.2" as the sub-identifier form, so the 1.3.6.1.4.1 arc the number sits under is implied rather than written. The comment in BddTargetEnterpriseId.h taught the wrong form and is corrected with it. The config field comment now says what to supply, that registering one is the caller's job, and that 32473 is available until they have their own. The two test comments that named ORIGIN_IP_MAX now describe the bound their own fake integrator applies, since that constant is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The row named the constant, then compared it to the UDP and TCP defaults and explained why an override exists. The comparison belongs to no requirement and the override is the endpoint callback doing its job.
Contributor
WalkthroughThe change replaces the origin enterprise OID with RFC 5612 example enterprise number ChangesOrigin enterprise ID alignment
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Interface/SolidSyslogOriginSd.h`:
- Around line 47-53: Update the documentation comment for the enterpriseId
parameter to use <registered-pen> in syntax examples and explicitly state that
32473 is reserved for documentation/testing and must not be used in production
messages; retain the existing RFC guidance and truncation details.
In `@docs/rfc-compliance.md`:
- Around line 141-147: Align the summary table’s counting rule with its label:
either rename “Total requirements” to “Total assessed items,” or exclude rows
stating “States no requirement of its own” and recalculate every RFC row; if
excluding them, update RFC 5424 to 35 total, 33 supported, 0 partial, and 2 N/A
and apply the same adjustment consistently to all sections.
🪄 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: f4b1f09e-b159-443a-a789-7d383abd3227
📒 Files selected for processing (6)
Bdd/Targets/Common/BddTargetEnterpriseId.hBdd/features/origin.featureCore/Interface/SolidSyslogOriginSd.hCore/Source/SolidSyslogOriginSdPrivate.hTests/SolidSyslogOriginSdTest.cppdocs/rfc-compliance.md
RFC 5425 §5.1 requires that an authorized peer can be specified by certificate fingerprint. The library authorizes by trust anchor and name, which is §5.2, and offers no fingerprint anywhere. That is a MUST it does not meet, so the matrix gains a Not Met status to say so rather than stretching Partial or N/A over it. docs/tls.md carries the obligation and #753 tracks delivering it on both shipped TLS platforms for 0.2.0. §4.2.2 is N/A on the same reasoning as the key-generation row: publishing your own certificate fingerprint through a management interface is asked of a syslog application, not of a component one is built from. The fingerprint form it defines matters where a peer is authorized by one, which is §5.1. §5.3 is a receiver-side policy. §5.4 and §5.5 are met by requiring peer verification rather than offering to skip it — declining to check a name is the separate, narrower choice the contract already requires be reported. Also §4.2 trimmed to the contract it was restating, and §4.3 added: all syslog messages MUST be sent as TLS application data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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`:
- Around line 100-103: Update the RFC compliance entries covering TLS protocol
versions, cipher suites, early data, security policy, and fingerprint
authorization to retain only their matrix status and a brief link to
docs/tls.md. Remove duplicated obligation explanations and treat docs/tls.md as
the sole authoritative TLS contract; apply the same change to the entries around
the additional referenced section.
- Around line 100-103: Revise the TLS 1.3 row in the RFC compliance table to
qualify support and negotiation: state that TLS 1.3 is used only when the linked
TLS backend enables it and both peers offer it. Remove the implication that an
unset maximum proves TLS 1.3 support, while retaining the TLS 1.2 minimum and
preference behavior.
- Around line 114-115: Use UK English spelling in the specified documentation
prose: in docs/rfc-compliance.md lines 114-115, change “authorizes” and
“authorized” to “authorises” and “authorised”; in docs/tls.md lines 81-89,
change “authorized” to “authorised”; and in docs/tls.md lines 231-234, change
“authorization” and “authorized” to “authorisation” and “authorised”.
- Line 71: Update the RFC 5424 §7.3.1 entry’s description of sequenceId ordering
to state that it is scoped to one originator and sequence interval, resets when
the syslog function starts, and wraps after 2147483647. Remove the claim that
sorting sequenceId alone recovers global order, and describe it only as a scoped
ordering aid across restarts, wrap-around, or multiple originators.
In `@docs/tls.md`:
- Around line 231-234: Update the fingerprint-based peer authorization statement
in the TLS documentation to say that both platforms currently fall short because
neither supports the feature. Keep the existing issue reference and
trust-anchor/name authorization context, and align the wording with the
documented RFC 5425 requirement and related compliance statements.
🪄 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: 50b2f6bc-077f-49df-916c-2d91ec2210c9
📒 Files selected for processing (2)
docs/rfc-compliance.mddocs/tls.md
The six §5 rows landed after the blank line that closed the table, so they rendered as a block of their own rather than as rows.
…about MTU RFC 5426 now walks its tree: §3.4 and §3.5 in the transport section, §4.3 and §4.4 in reliability, and §5 opened out into its six clauses. Each is dispositioned rather than summarised under §5, so a reader can see the threats were considered rather than missed. 7 rows to 17. The §3.2 fragmentation note dropped from 136 words to 75. The obligations it restated are stated on the Datagram vtable — report the path payload and never guess high, distinguish oversize where the platform can — so the row keeps only what is compliance-specific: the 1232 fallback against the 1180 and 480 §3.2 calls safest, and the platforms that cannot report oversize. Those platform obligations were only half documented. Two platform pages already described what an over-large record does to them; the two socket platforms said nothing, and they are the two that handle it well. Each now states its own behaviour: the path MTU is read once connected, the conservative payload is reported before that, and an over-large record is trimmed on a codepoint boundary and resent rather than dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…shortfall Three corrections from review, each verified against the source rather than taken on trust. RFC 5424 §7.3.1 sets sequenceId to 1 when the syslog function starts, so the count restarts every run. The row claimed a SIEM recovers origination order by sorting on it; that holds within one originator between restarts and wraps, and the row now says so. docs/tls.md said neither platform falls short of fingerprint authorisation because neither offers it. That is self-contradictory: an obligation nothing meets is one both fall short of. The enterpriseId comment offered 32473 to anyone without a registered number. RFC 5612 reserves it for examples and testing, so the comment now draws the line at a device that ships. Prose added in this branch uses UK spelling; quoted RFC section titles keep their own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
☀️ Quality Summary Created by Quality Monitor v4.15.0 (#82d77af). More details are shown in the GitHub Checks Result. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/platforms/posix/index.md`:
- Around line 37-41: Prevent the documentation from guaranteeing truncated
delivery while SolidSyslogUdpSender’s retry logic converts repeated OVERSIZE
results to SENT. Qualify the truncated-delivery claim in
docs/platforms/posix/index.md lines 37-41 and docs/platforms/windows/index.md
lines 37-41, or instead correct the retry result handling; also enable coverage
for the `@windows_wip` scenario.
In `@docs/rfc-compliance.md`:
- Line 127: Update docs/rfc-compliance.md:127-127 and the corresponding fallback
descriptions in docs/platforms/posix/index.md:31-35 and
docs/platforms/windows/index.md:31-35 to use family-specific unknown-MTU
bounds—480 octets for IPv4 and 1180 octets for IPv6—instead of presenting 1232
as universal; alternatively, revise the compliance status to explicitly record
this limitation.
🪄 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: f07d3acd-0bee-4d43-8b4c-43358390a804
📒 Files selected for processing (5)
Core/Interface/SolidSyslogOriginSd.hdocs/platforms/posix/index.mddocs/platforms/windows/index.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
Part of #708. The second pass over the RFC compliance matrix, reading it in order
against the RFCs. Four things came out of §7 and its subsections, two of them in code.
Change Description
RFC 5426 §3.6 is now carried. "Syslog senders MUST NOT disable UDP checksums" is a
requirement aimed squarely at a sender, and the matrix had no row for it. Nothing in
Core/orPlatform/setsSO_NO_CHECKor a vendor equivalent, so the stack's defaultchecksumming stands and the row is Supported.
Unnumbered requirements are counted as requirements. A
—in the Section columnmarks a requirement the RFC does not number — from IANA, from another RFC, or from this
library's own contract — rather than something that is not a requirement. The summary
counts them and says so.
§7 reworded, and §7.2 and §7.3 condensed onto their child rows — 177 words to 57
and 95 to 49. Both were restating what the per-parameter rows, §6.3.3 and the config
header now carry. Each keeps what only it can say: which parameters exist, which are
optional, and what an unwired instance emits. §7.2.2 now states why its 64-byte bound is
64, since §7.2.2 itself gives no length.
ORIGIN_IP_MAXis removed. S07.05 added it to cap eachipvalue through theescaped-string writer; S14.05 moved
ipemission onto the SD writer, where theintegrator's at-callback writes straight into the
SolidSyslogSdValue, and no librarycode has applied the bound since. It declared a limit the library does not impose.
The enterprise number in the BDD targets and origin tests was wrong twice over. It
read
1.3.6.1.4.1.99999:whatever enterprise holds it.
number)", giving
32473.1.2as the sub-identifier form. The1.3.6.1.4.1arc isimplied by the parameter, not written into it.
The comment in
BddTargetEnterpriseId.htaught that wrong form and is corrected with it.SolidSyslogOriginSdConfig.EnterpriseIdnow documents what to supply, that registering anumber is the caller's job, and that 32473 is available until they have their own.
Test Evidence
clang-format --dry-run --Werrorclean on every changed C/C++ filemkdocs build --strictexits 0SolidSyslogOriginSdPrivate.his atline 13, the enum's opening brace, and the deletion is at line 17
which was fetched to confirm 32473 rather than taken on trust
The BDD half is the part not provable locally.
origin.featureandBddTargetEnterpriseId.hmove together so they cannot disagree, but the scenariosthemselves only run in CI's
bdd-*lanes.Areas Affected
docs/rfc-compliance.md,Core/Interface/SolidSyslogOriginSd.h(comment),Core/Source/SolidSyslogOriginSdPrivate.h, the origin SD tests, and the BDD targetheader and feature.
No behaviour change: the removed constant was unused, and the enterprise number is test
and example data.
The matrix is still being read in order — RFC 5424 is reviewed to §7.3, and RFC 5425,
5426 and 6587 have not been tree-walked yet. That stays on #708.
Summary by CodeRabbit