sm: in-band TLS upgrade negotiated by Inband-Security-Id (RFC 6733 §13)

[tridentsx]

RFC 6733 retains the RFC 3588 in-band upgrade for backward compatibility
(§1.1.3, §2.1, §5.6). §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. [...] If
the handshake fails, both ends MUST move to the closed state." TLS
before the CER remains the recommended configuration (§5.6, §13); this
is opt-in through Settings.TLSConfig and off by default.

Negotiation

• Server: a CER asking for Inband-Security-Id=1 is accepted only if
  TLSConfig is set and the connection reports CanStartTLS(); otherwise
  the answer is DIAMETER_NO_COMMON_SECURITY. The success CEA carries
  Inband-Security-Id=1 and the handshake follows it. A failed handshake
  closes the connection.
• Client: the requested mechanism is recorded on the connection and
  smparser.CEA now parses Inband-Security-Id. The client upgrades only
  when it asked for TLS and the CEA selects it. A CEA that omits the AVP
  or answers 0 fails the handshake rather than continuing in cleartext.
  A client that only advertises 1 (TLSConfig nil, as with
  Client.InbandSecurityID today) fails if the peer selects TLS, since
  the peer will then start a handshake; if the peer declines, nothing
  changes.

Transport

• New diam.TLSUpgrader, implemented by the server-side Conn. Its
  CanStartTLS() is false for a MultistreamConn (§2.1 runs DTLS, not TLS,
  over SCTP; there is also no buffered reader), for a connection already
  on TLS, and for a handler not running on the serve goroutine. The
  state machine checks it before answering, so an unsupported transport
  is refused rather than failing after the CEA.
• StartTLS reads through the connection's active read chain: bytes
  already buffered, then the CloseNotify pipe if one is registered
  (starting it if still pending), then the socket. It no longer competes
  with the CloseNotify copy goroutine, and close notification keeps
  working after the upgrade. A failure draining buffered bytes is
  returned.
• rwc and tlsState are swapped under a new RWMutex so Close, RemoteAddr,
  LocalAddr, TLS and Connection stay safe from other goroutines, e.g.
  the HandshakeTimeout timer or a watcher started in OnNewConnection.
  The lock is not held during the handshake, so the timer can still
  abort a stalled one.

Dispatch

• With MaxConcurrentHandlers != 0, the capabilities exchange is
  dispatched on the serve goroutine only if the Handler implements the
  new diam.InbandTLSHandler and reports it enabled (sm.StateMachine does
  when TLSConfig is set), and the connection can still be upgraded. This
  is a behaviour change for such Handlers only; every other Handler,
  connection and command dispatches as before. A Handler that wraps a
  StateMachine without forwarding InbandTLSEnabled keeps concurrent
  dispatch, and in-band TLS is then refused with
  DIAMETER_NO_COMMON_SECURITY rather than raced.

Tests cover each point above; each fails if the corresponding change is
reverted.

[fiorix]

Requesting changes for two blockers:

1. Client-side upgrade is not gated on CEA negotiation. handleCEA calls StartTLSClient whenever Settings.TLSConfig is non-nil and the connection is plain, but smparser.CEA does not parse Inband-Security-Id. A client with TLS config will send a TLS ClientHello after any successful plain CEA, even when the peer did not echo Inband-Security-Id=1.

2. The advertised TLSUpgrader path is unsafe for SCTP/multistream connections. response implements StartTLS, so handleCER accepts in-band TLS for any response connection, but multistream connections skip c.buf initialization in newConn. startTLS then dereferences w.conn.buf.Reader, which can panic after a success CEA.

go test ./diam/sm ./diam -count=1 passed. These are uncovered negative cases and should be fixed before merge.

[fiorix]

Re-reviewed at df08762 against current main. Keeping changes requested. The use case is legitimate but narrow: RFC 6733 retains post-CER TLS only as a compatibility path for older peers and requires both sides to agree on it; ordinary deployments should keep using TLS-before-CER. For this to land it needs a revised implementation covering the four points below. The first three are reproduced defects, not style.

1. The client upgrades without any negotiation. In handleCEA (diam/sm/cea.go:27), a non-nil TLSConfig plus any success CEA triggers StartTLSClient. The CEA parser never reads Inband-Security-Id, and nothing checks what the client requested in its CER. A peer that omits the AVP, or explicitly answers Inband-Security-Id=0, still gets a TLS handshake thrown at it; both cases fail focused tests. Track the requested and selected mechanism and upgrade only on agreement.

2. Multistream connections panic after advertising TLS capability. startTLS (diam/server.go:479) dereferences conn.buf, which newConn leaves nil for MultistreamConn. Since every *response implements TLSUpgrader, the state machine treats SCTP connections as upgrade-capable, and a focused test through the real multistream branch of newConn reproduces the nil dereference. Unsupported transports must be rejected before a success CEA is sent, not discovered by panic. Note also that RFC 6733 pairs SCTP with DTLS, not TLS, so wrapping SCTP in this TCP TLS path would not be correct even without the panic.

3. The upgrade races CloseNotify for the raw reader. Once a CloseNotifier is registered, the connection's read path becomes a pipe fed by a goroutine copying from the raw socket. startTLS drains only the bufio layer and then reads conn.rwc directly, so the handshake and the copy goroutine compete for the same socket: with a CloseNotify registered via OnNewConnection, the end-to-end upgrade test times out; without it, it passes. The upgrade has to take ownership of the active reader and the notifier lifecycle, not just the bufio buffer. Related: in startTLS, if io.ReadFull on the drained bytes fails, the error is swallowed and the already-consumed bytes are silently dropped.

4. The dispatch change contradicts "zero impact when unused". conn.dispatch now forces every command 257 through synchronous handling regardless of whether TLS or this feature is in use. If synchronous CER dispatch is a hard requirement for the upgrade, it needs to be scoped to connections that can actually upgrade, and called out as a behavior change.

The branch also conflicts with current main in diam/sm/cer.go and diam/sm/sm.go and will need a rebase regardless.

[tridentsx]

Thanks for the re-review. Rebased on main (92e4536) as a single commit. Each point below has a test that fails when the corresponding change is reverted.

1. Client upgrades only on agreement. smparser.CEA now parses Inband-Security-Id, and the client records what it requested in its CER. handleCEA upgrades only when it asked for TLS and the CEA selects TLS. A CEA that omits the AVP or answers 0 fails the handshake instead of continuing in cleartext (TestClientDoesNotUpgradeWithoutAgreement; the peer is a raw socket and the test asserts no TLS record is sent, for both cases).

A related case: a client that only advertises Inband-Security-Id=1 (Client.InbandSecurityID from #240, no TLSConfig) used to carry on in cleartext even if the peer selected TLS, although per §13 the peer then starts the handshake. That now fails the handshake (TestClientWithoutTLSConfigFailsWhenPeerSelectsTLS). If the peer declines, nothing changes (TestClientAdvertisesWithoutTLSConfig).

2. Unsupported transports are refused before the CEA. The new TLSUpgrader has CanStartTLS(), which is false for a MultistreamConn (§2.1 runs DTLS, not TLS, over SCTP) and for a connection already on TLS. handleCER checks it before answering, so the peer gets DIAMETER_NO_COMMON_SECURITY rather than a success CEA. Tests: TestCanStartTLSRejectsMultistream (through the multistream branch of newConn; returns an error, no panic) and TestServerRefusesInbandTLSOnNonUpgradableConn (via the state machine; asserts 5017 and that no upgrade is attempted).

3. The upgrade takes over the active reader. startTLS reads through the connection's current chain: buffered bytes, then the CloseNotify pipe if registered, then the socket. If CloseNotify was registered after the serve loop's last read (for example from OnCER), the pipe is still pending; the upgrade starts it on the cleartext socket, as liveSwitchReader.Read would. Close notification keeps working after the upgrade. Tests: TestInbandTLSUpgradeWithCloseNotifier (registered in OnNewConnection; times out if the handshake reads the raw socket) and TestInbandTLSUpgradeWithCloseNotifyDuringCER (registered in OnCER; fails if the pending pipe is not started). A failure draining the buffered bytes is now returned.

Two further problems I found while working on this:

• The handshake timer from sm: close accepted connections that do not complete CER/CEA within a … #266, or any goroutine holding the diam.Conn, can call Close/RemoteAddr/TLS while the upgrade swaps rwc/tlsState. These are now swapped under an RWMutex. The lock is not held during the handshake, so the timer can still close a stalled one. Tests: TestInbandTLSUpgradeConcurrentAccessors (reports a race under -race without the lock) and TestInbandTLSStalledHandshakeIsClosed (fails if the lock is held across the handshake).
• StartTLS is only safe while the handler runs on the serve goroutine. CanStartTLS() now also returns false, and StartTLS returns an error, when that is not the case. A Handler that wraps the StateMachine without forwarding InbandTLSEnabled therefore gets 5017 under concurrent dispatch rather than a race (TestCanStartTLSRejectsOffServeGoroutine, TestServerRefusesInbandTLSUnderConcurrentDispatchWithoutOptIn).

4. Dispatch is unchanged unless the feature is in use. Under MaxConcurrentHandlers, the CER is run on the serve goroutine only when the Handler implements diam.InbandTLSHandler and reports it enabled (sm.StateMachine does when TLSConfig is set), and the connection can still be upgraded. Other Handlers, connections already on TLS, SCTP and all other commands dispatch as before. TestDispatchUnaffectedWithoutInbandTLS covers the four combinations; TestDispatchConcurrentForNonCER checks DWR stays concurrent with the feature on; TestInbandTLSUpgradeUnderConcurrentDispatch checks the upgrade works under concurrent dispatch. The behaviour change is documented on InbandTLSHandler and Settings.TLSConfig.

RFC references now point to: §13 for the MUST on Inband-Security-Id in the CER/CEA exchange and on moving to the closed state if the handshake fails; §1.1.3, §2.1 and §5.6 for the backward-compatibility scenario; §2.1 for SCTP/DTLS; §6.10 for the AVP values and the default when it is omitted. The docs say TLS before the CER remains the recommended configuration.

go test -race ./diam/... passes locally except TestServerClose/sctp, which fails the same way on clean main here and passes in CI.