sm: close accepted connections that do not complete CER/CEA within a …

[kulcsartibor]

sm: close accepted connections that do not complete CER/CEA within a timeout

Branch: kulcsartibor:feat/handshake-timeout → fiorix:main

This PR adds public API and changes default behaviour, so it is kept separate, as requested.

Problem

The server role had no handshake timer. A peer that connected and sent nothing was kept open indefinitely, holding a goroutine and a file descriptor, bounded only by the process's descriptor limit.

A peer allowlist cannot help, because it can only run once a CER supplies an Origin-Host. RFC 6733 §5.6.1 says the connection should be closed if no CER arrives within an implementation-defined time.

Server.ReadTimeout is not a substitute: it is re-armed before every read, so it would also drop established peers that go quiet between watchdogs.

Change

• sm.Settings.HandshakeTimeout: zero means DefaultHandshakeTimeout (30 s), and a negative value disables the limit. It applies only to connections accepted by diam.Server. A StateMachine used as a client waits for its CEA on its own retransmit schedule, which can legitimately exceed 30 s.
• diam.AcceptHandler: a new optional interface, in the style of ErrorReporter: HandleAccept(c Conn) (onClose func()).
  • conn.serve calls it for connections accepted by Server.Serve, never for dialed ones.
  • It runs before the TLS handshake, so a peer that never sends a ClientHello is bounded too.
  • The returned function runs when the connection closes, and the state machine uses it to stop the timer. Without that, each closed connection stayed reachable until its timer fired: 43.5 MiB retained for 5,000 connect-and-close connections, against 0.1 MiB with the timer stopped.

Behaviour change

Existing users get the 30 s limit without configuring anything. A well-behaved peer sends its CER right after connecting, and the existing test suite passes with the default in effect.

Open questions for review

• Default on or opt-in. Default on protects existing deployments without code changes. Opt-in avoids any behaviour change for users who don't set it. Happy to switch if you prefer opt-in.

• Wrapped handlers. serve looks for AcceptHandler only on Server.Handler. If the state machine is wrapped in middleware, such as examples/middleware's NewTracer(sm), the interface is hidden and the timeout is silently disabled. ErrorReporter and HandshakeNotifier have the same limitation today. Options:

  1. document it, and have wrappers forward the interface;
  2. find the handler another way;
  3. use an explicit Server field instead of an interface.

  Which would you prefer?

• Transport setup. The hook runs before the TLS handshake, as described above. SCTP connections go through the same conn.serve path, but I have not tested SCTP (TestServerClose/sctp does not run on macOS).

Tests

• a silent TCP connection is closed
• a silent TLS connection with no ClientHello is closed
• a handshaked connection still answers a DWR after the timeout
• a negative value disables the limit
• zero resolves to the default
• calling onClose stops the timer
• HandleAccept fires once for an accepted connection, never for a dialed one, and onClose runs when the peer closes

[fiorix]

Reviewed at e33670c. Merging, with the default-on question settled as proposed.

The mechanics check out: the timer starts before the TLS handshake so a silent ClientHello is bounded too, the close is gated on smpeer.FromContext (the same signal handshakeOK uses), the returned stop function keeps closed connections from retaining timers, a negative value disables the limit, and dialed connections are exempt. All four behaviors have tests, and go test -race ./diam/... passes on this head.

On the two design points raised in the description:

• Default stays on (30 s). RFC 6733 section 5.6.1 sanctions closing, it only affects peers that never complete a CER, and the descriptor-exhaustion protection should not require opt-in. It will be called out in the next release notes as a behavior change; deployments with peers that legitimately connect long before sending a CER can set HandshakeTimeout: -1.
• Wrapped handlers. The type assertion missing wrappers is the same limitation ErrorReporter already has, and it's documented on the interface. Acceptable as-is.