sm: implement in-band TLS upgrade for RFC 3588 backward compatibility#247
Open
tridentsx wants to merge 1 commit into
Open
sm: implement in-band TLS upgrade for RFC 3588 backward compatibility#247tridentsx wants to merge 1 commit into
tridentsx wants to merge 1 commit into
Conversation
Implement post-CER/CEA TLS upgrade (Inband-Security-Id negotiation) for backward compatibility with RFC 3588 peers, as retained by RFC 6733 §1.1.3, §5.6, and §13. Features: - TLSUpgrader interface on Conn for StartTLS/StartTLSClient - Server accepts Inband-Security-Id=1 in CER when TLSConfig is set - CEA echoes Inband-Security-Id=1 to confirm TLS negotiation - Client upgrades to TLS after receiving success CEA - Drain bufio.Reader before TLS handshake to prevent stalled reads - Force synchronous CER dispatch to prevent ClientHello being consumed as a diameter message RFC 6733 references: - §1.1.3: "This new approach augments the existing in-band security negotiation, but it does not completely replace it. The old method is kept for backward compatibility reasons." - §5.6: Describes the fallback scenario where TLS is initiated after CER/CEA when connecting to RFC 3588 peers (SHOULD, not MUST) - §13: "For TLS/TCP and DTLS/SCTP connections to be established in the open state, the CER/CEA exchange MUST include an Inband-Security-ID AVP with a value of TLS/TCP and DTLS/SCTP."
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
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.
sm: implement in-band TLS upgrade for RFC 3588 backward compatibility
Summary
Re-implements post-CER/CEA TLS upgrade via
Inband-Security-Idnegotiation, with corrected RFC citations and fixes for the race conditions identified in the original review.This is a backward-compatibility mechanism for interoperating with RFC 3588 peers that do not support TLS-before-CER (port 5658). RFC 6733 explicitly retains this capability.
Zero impact when unused
If no peer sends
Inband-Security-Id=1in CER, or ifSettings.TLSConfigis nil, the new code paths are never entered. The existing behavior is completely unchanged — peers that don't request in-band TLS are unaffected.RFC 6733 basis
§1.1.3 — Changes from RFC 3588:
§5.6 — Peer State Machine (p.69):
§13 — Security Considerations (p.140):
§6.10 — Inband-Security-Id AVP:
Note: NOT RECOMMENDED per RFC 2119 §4 means "there may exist valid reasons in particular circumstances when the particular behavior is acceptable or even useful" — it is not a prohibition.
Prior discussion
This was discussed in #238 where @vingarzan (who raised the original RFC compliance concern) concluded:
The previous PR (#243) was reverted due to an incorrect §6.2 citation. This resubmission uses the correct references throughout.
What's included
TLSUpgraderinterface —StartTLS/StartTLSClientonConnfor upgrading plain connections after CER/CEA.Server-side CER handling — Accepts
Inband-Security-Id=1whenSettings.TLSConfigis set. Performs TLS upgrade after sending success CEA.CEA echoes
Inband-Security-Id=1— Per §13, both peers must agree on the security mechanism.Client-side CEA handling — After receiving a success CEA, the client calls
StartTLSClientto complete the upgrade.Race condition fix: bufio drain — The
bufio.Readermay have pre-read the TLS ClientHello. Before handing the connection totls.Server, buffered bytes are drained and prepended viaio.MultiReader.Race condition fix: synchronous CER dispatch — CER (command code 257) is always dispatched synchronously regardless of
MaxConcurrentHandlers, ensuring the TLS upgrade completes before the serve loop reads the next bytes.Test coverage
StartTLSStartTLSClientstartTLShandleCERhandleCEAsuccessCEAdispatchTests:
TestHandleCER_InbandSecurity_WithTLSConfig— server accepts CER with Inband-Security-Id=1 when TLSConfig is setTestInbandTLSUpgrade_EndToEnd— full plain TCP → CER/CEA → TLS upgrade → DWR/DWA over TLSTestConcurrentServePanicRecovery— updated to use DWR since CER is now synchronousgo test ./...passes (SCTP test excluded — requires kernel SCTP support)