Skip to content

sm: implement in-band TLS upgrade for RFC 3588 backward compatibility#247

Open
tridentsx wants to merge 1 commit into
fiorix:mainfrom
tridentsx:feat/inband-tls-upgrade
Open

sm: implement in-band TLS upgrade for RFC 3588 backward compatibility#247
tridentsx wants to merge 1 commit into
fiorix:mainfrom
tridentsx:feat/inband-tls-upgrade

Conversation

@tridentsx

Copy link
Copy Markdown
Contributor

sm: implement in-band TLS upgrade for RFC 3588 backward compatibility

Summary

Re-implements post-CER/CEA TLS upgrade via Inband-Security-Id negotiation, 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=1 in CER, or if Settings.TLSConfig is 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:

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 — Peer State Machine (p.69):

When connecting to responders that do not conform to this document (i.e., older Diameter implementations that are not prepared to received TLS/TCP and DTLS/SCTP connections in the closed state), the initial TLS/TCP and DTLS/SCTP connection attempt will fail. The initiator MAY then attempt to connect via TCP or SCTP and initiate the TLS/TCP and DTLS/SCTP handshake when both ends are in the open state.

§13 — Security Considerations (p.140):

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. The TLS/TCP and DTLS/SCTP handshake will begin when both ends successfully reach the open state, after completion of the CER/CEA exchange. If the TLS/TCP and DTLS/SCTP handshake is successful, all further messages will be sent via TLS/TCP and DTLS/SCTP. If the handshake fails, both ends MUST move to the closed state.

§6.10 — Inband-Security-Id AVP:

The use of this AVP in CER and CEA messages is NOT RECOMMENDED.

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:

"if there is even a single stack which does that and you need to interconnect with it, of course, you must go for it. [...] If it is RFC6733 compliant too, it's not affecting me or anyone else, of course."

The previous PR (#243) was reverted due to an incorrect §6.2 citation. This resubmission uses the correct references throughout.

What's included

  1. TLSUpgrader interfaceStartTLS / StartTLSClient on Conn for upgrading plain connections after CER/CEA.

  2. Server-side CER handling — Accepts Inband-Security-Id=1 when Settings.TLSConfig is set. Performs TLS upgrade after sending success CEA.

  3. CEA echoes Inband-Security-Id=1 — Per §13, both peers must agree on the security mechanism.

  4. Client-side CEA handling — After receiving a success CEA, the client calls StartTLSClient to complete the upgrade.

  5. Race condition fix: bufio drain — The bufio.Reader may have pre-read the TLS ClientHello. Before handing the connection to tls.Server, buffered bytes are drained and prepended via io.MultiReader.

  6. 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

Function Coverage
StartTLS 100%
StartTLSClient 100%
startTLS 83%
handleCER 81%
handleCEA 86%
successCEA 97%
dispatch 100%

Tests:

  • TestHandleCER_InbandSecurity_WithTLSConfig — server accepts CER with Inband-Security-Id=1 when TLSConfig is set
  • TestInbandTLSUpgrade_EndToEnd — full plain TCP → CER/CEA → TLS upgrade → DWR/DWA over TLS
  • TestConcurrentServePanicRecovery — updated to use DWR since CER is now synchronous
  • go test ./... passes (SCTP test excluded — requires kernel SCTP support)

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."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant