This repository was archived by the owner on Sep 11, 2026. It is now read-only.
Repository navigation
Push SSL Handshake State into the Connection State Machine #252
Closed
SeanTAllen
started this conversation in
Research
Replies: 2 comments 1 reply
Open Question Decisions
|
0 replies
Remaining TestsFour tests from the testing strategy are still unimplemented:
Tests 5 and 6 from the plan are done (PR #253). Additional tests beyond the original plan have also been added: SSL/TLS handshake failure tests (#253), |
1 reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Problem
_Openrepresents two different application-level states: "TCP connected, SSL handshake in progress" and "fully connected, ready for application data." Five boolean flags (_ssl_ready,_ssl_failed,_ssl_auth_failed,_tls_upgrade, plus_sslpresence) are checked ad-hoc across TCPConnection. Security-critical invariants (no sends before handshake, no concurrent TLS operations) are enforced by scattered flag checks rather than structural state.Code that cares about the distinction already has to check flags on top of the state:
idle_timeout()checks(not _ssl_ready) and (not _tls_upgrade),set_timer()duplicates the same check,_hard_close_connected()has a multi-way branch reading 4 flags to determine callback routing, andis_writeable()checks_ssl_ready. The fix is to split the SSL handshake phase into explicit states.Design origin: CLAUDE.md "Future Work" section, Issue #240, Discussion #219.
Design: Two New States
Add
_SSLHandshaking(initial SSL fromssl_client/ssl_serverconstructors) and_TLSUpgrading(mid-stream TLS fromstart_tls()).These are separate states rather than one parameterized state because they have fundamentally different semantics:
_SSLHandshakinghasis_open() = false(application hasn't been notified), while_TLSUpgradinghasis_open() = true(application already received_on_connected/_on_started). They also have different callback routing on failure, differentstart_tls()error returns, and different timer behavior. Two states make these differences structural rather than flag-dependent.Updated State Diagram
Non-SSL connections follow the same paths as today. No change for plaintext.
State Property Table
is_open()is_closed()sends_allowed()_ConnectionNone_finish_initialization. All methods_Unreachable()._ClientConnecting_UnconnectedClosingclose()during connecting._SSLHandshaking_TLSUpgradingstart_tls(). Application already notified._Open_Closing_ClosedTrait Interface Changes
Add two methods to
_ConnectionState:ssl_handshake_complete(): Called by_ssl_poll()whenSSLReadyis reached. Each state handles the transition and callback routing:_ConnectionNone_Unreachable()_ClientConnecting_Unreachable()_UnconnectedClosing_Unreachable()_SSLHandshaking_Open, cancel connect timer, arm idle timer, fire_on_connected()(client) /_on_started()(server)_TLSUpgrading_Open, fire_on_tls_ready()_Openssl.state()returnsSSLReadypersistently)_Closing_Open, handshake was already complete)_Closedsends_allowed(): Returnstrueonly for_Open. Used byis_writeable()to replace the_ssl_readycheck._TLSUpgradinghasis_open() = truebutsends_allowed() = false, correctly reflecting that sends are blocked during the upgrade handshake.Method Tables for New States
_SSLHandshaking:own_event_Open)foreign_event_Open)sendSendErrorNotConnectedclosehard_close()hard_close_Closed,_hard_close_ssl_handshaking()_on_connection_failure(client) or_on_start_failure(server)start_tlsStartTLSNotConnectedread_again_do_read_again()ssl_handshake_complete_Openis_openis_closedsends_allowed_TLSUpgrading:own_event_Open)foreign_event_Open)_OpensendSendErrorNotConnectedclosehard_close()_Closingwould lose upgrade context for callback routinghard_close_Closed,_hard_close_tls_upgrading()_on_tls_failure(reason)then_on_closed()start_tlsStartTLSAlreadyTLSread_again_do_read_again()ssl_handshake_complete_Openis_openis_closedsends_allowedBoolean Flags
Eliminated
_ssl_ready: Replaced by state transitions._SSLHandshaking/_TLSUpgrading->_OpenatSSLReady. The_Open.ssl_handshake_complete()no-op handles the persistentSSLReadyreturn on subsequent calls. All 6 read sites replaced by state dispatch._tls_upgrade: Replaced by the state machine._SSLHandshaking= initial SSL,_TLSUpgrading= upgrade. The state'shard_close()andssl_handshake_complete()implementations encode the correct callback routing. All 5 read sites replaced.Kept
_ssl_failed: Constructor-to-initialization bridge. Set inssl_client/ssl_serverconstructors, consumed once in_complete_client/server_initialization(). Orthogonal to runtime states — it's consumed before any handshake state is reached._ssl_auth_failed: Set in_ssl_poll()onSSLAuthFailimmediately beforehard_close(), read duringhard_close()in the same synchronous call chain. Intra-method communication (same pattern as_connect_timed_out). Used by_hard_close_ssl_handshaking()and_hard_close_tls_upgrading()to distinguishTLSAuthFailedvsTLSGeneralError._sslfield: The SSL session object. Needed for I/O.Code Changes
_ssl_poll()SSLReady branchSimplifies from flag-checking + callback routing to state dispatch:
The
if not _ssl_ready thenguard is no longer needed —_SSLHandshakingand_TLSUpgradingtransition to_Openon the firstSSLReady. Subsequent calls land in_Open.ssl_handshake_complete()which is a no-op. The_tls_upgraderouting is no longer needed — the state knows which callbacks to fire.The SSLAuthFail and SSLError branches are unchanged: they set
_ssl_auth_failed(for auth fail) and callhard_close(), which dispatches through the state._establish_connection()Transition to
_SSLHandshakingwhen SSL present (was_Open):_complete_server_initialization()Same pattern —
_SSLHandshakingwhen SSL present:_do_start_tls()Transition to
_TLSUpgrading(replaces_tls_upgrade = true; _ssl_ready = false):_hard_close_connected()splitExtract common teardown into
_hard_close_cleanup()(cancel tokens, clear pending buffers, cancel timers, unsubscribe event, close fd, dispose SSL). Then three specialized methods:_hard_close_cleanup()— common teardown steps in the exact order they appear in the current_hard_close_connected(): set shutdown flags, fire_notify_send_failedfor pending token, clear pending buffers, cancel idle/connect/user timers, unsubscribe event, set unreadable/unwriteable, close fd, dispose SSL. Preserving this order matters — SSL dispose must happen after fd close, and timer cancellation must happen before event unsubscribe._hard_close_connected()— cleanup +_on_closed()+ spawner notification. Called from_Open.hard_close()and_Closing.hard_close(). Theif not _ssl_readybranch is removed — we can never reach this method with an incomplete handshake._hard_close_ssl_handshaking()— cleanup + callback routing by lifecycle receiver type and timeout flag:_connect_timed_out:_on_connection_failure(ConnectionFailedTimeout)_on_connection_failure(ConnectionFailedSSL)_on_start_failure(StartFailedSSL)_register_spawner). Must replicate the pattern from current_hard_close_connected()lines 767-778: if_spawned_byis present, callspawner._connection_closed()and clear it; if_spawned_byisNone, do nothing — the deferred_register_spawnercall handles the closed-before-registration case by checking_state.is_closed(). This two-phase pattern is a race condition that's easy to get wrong — copy it exactly._hard_close_tls_upgrading()— cleanup +_on_tls_failure(reason)(where reason isTLSAuthFailedorTLSGeneralErrorbased on_ssl_auth_failed) +_on_closed()+ spawner notification._enqueue()guardChange from
is_open()tonot _state.is_closed():During
_SSLHandshaking,_ssl_flush_sends()calls_enqueue()to put handshake protocol data on the wire. Withis_open()returning false in the new state, the current guard would silently drop handshake data.not is_closed()allows enqueue in all states with a live TCP socket (_SSLHandshaking,_TLSUpgrading,_Open) while blocking it in closed states. Preserves existing_Closingbehavior (both guards are false since_Closing.is_closed() = true).Note: this guard also becomes
truefor_ConnectionNoneand_ClientConnecting(both haveis_closed() = false), widening access compared to the oldis_open()guard. This is harmless in practice —_enqueue()is only called from_ssl_flush_sends()and_do_send(), neither of which can be reached during those states. But the widening should be noted in a code comment for future maintainers.Pre-existing issue:
_enqueue()dropping data during_Closingmay cause missing TLS close_notify alerts during graceful shutdown. This is not introduced by this change — it's the current behavior. File a separate issue to track.Simplifications
idle_timeout()armingis_open()+(not _ssl_ready) and (not _tls_upgrade)is_open()only_SSLHandshaking.is_open() = falseblocks arming;_TLSUpgrading.is_open() = trueallows itset_timer()is_open()+match _sslguardis_open()onlyis_writeable()is_open() and _writeable+_ssl_readyis_open() and _writeable and sends_allowed()_do_send()match _ssl | if not _ssl_ready_do_send()only reachable from_Open.send(); handshake is always completeI/O dispatch deduplication
_Open.own_event(),_Closing.own_event(),_SSLHandshaking.own_event(), and_TLSUpgrading.own_event()share identical I/O dispatch logic (~15 lines). Two options:_dispatch_io_event(flags, arg)helper on TCPConnection — each state delegates to it. Eliminates 4-way duplication._Openand_Closingalready duplicate the logic.This is a style choice. The plan favors extraction since going from 2 copies to 4 tips the scale, but the decision is left to Sean.
Security Analysis
Sends before handshake: Structurally prevented.
_SSLHandshaking.send()and_TLSUpgrading.send()returnSendErrorNotConnectedwithout reaching_do_send(). The_ssl_readycheck in_do_send()becomes dead code and is removed.CVE-2021-23222 protection preserved: The
_bytes_in_read_buffer > 0check remains in_do_start_tls(), which runs synchronously in_Openbefore transitioning to_TLSUpgrading. During_SSLHandshaking, incoming data goes through_ssl_poll()wheressl.read()returnsNone(no decrypted application data available during handshake) — it's the SSL library that prevents application data delivery, not the state machine. Afterssl_handshake_complete()transitions to_Openwithin the same_ssl_poll()call, thessl.read()loop correctly delivers any data that became available with the handshake completion.Downgrade prevention: No transition from
_TLSUpgradingback to plaintext_Open. On failure,_TLSUpgradinggoes to_Closedonly. Onclose(),_TLSUpgradingdoes a hard close, preventing the connection from returning to plaintext.Authentication failure routing:
_ssl_poll()sets_ssl_auth_failed = truethen callshard_close()._SSLHandshaking.hard_close()routes to_on_connection_failure/_on_start_failure._TLSUpgrading.hard_close()routes to_on_tls_failure(TLSAuthFailed)then_on_closed(). Authentication failures cannot be silently ignored.start_tls()preconditions watertight:_SSLHandshakingreturnsStartTLSNotConnected._TLSUpgradingreturnsStartTLSAlreadyTLS. All other non-_Openstates returnStartTLSNotConnected. The precondition checks in_do_start_tls()are unchanged.Connection timeout coverage: The connect timer covers
_ClientConnecting+_SSLHandshaking. Armed in_complete_client_initialization(), cancelled inssl_handshake_complete(). If timeout fires during_SSLHandshaking,_connect_timed_outis set andhard_close()routes toConnectionFailedTimeout.Performance Analysis
Hot path (
_Open.own_event()-> read/write): Zero overhead._Open.own_event()delegates to_dispatch_io_event()which is an ordinary method call. The state machine virtual call was already there._do_send()gets simpler: removes thematch _ssl | if not _ssl_readycheck. One fewer branch in the send hot path.is_writeable()trades one match on_sslforsends_allowed()(virtual call, but_Openreturns constanttrue— trivially optimizable).State transition cost:
_SSLHandshakingat TCP connect,_Openat handshake completion. For TLS upgrades:_TLSUpgradingatstart_tls(),_Openat completion. For non-SSL connections: zero additional allocations.Testing Strategy
Existing tests (must all pass unchanged)
This is a behavior-preserving refactoring for all observable behavior. The critical existing tests:
_TestSSLPingPong,_TestSSLSendv— initial SSL full lifecycle_TestSSLIdleTimeout,_TestSSLIdleTimeoutNotArmedDuringHandshake,_TestSSLIdleTimeoutDeferredArm— timer behavior during handshake_TestSSLConnectionTimeoutFires,_TestSSLConnectionTimeoutCancelledOnConnect— connect timeout with SSL_TestSetTimerNotOpenDuringSSLHandshake,_TestSetTimerNotOpenDuringSSLHandshakeServer— timer rejection during handshake_TestStartTLSPingPong,_TestStartTLSPreconditions— TLS upgrade path_TestHardCloseWhileConnecting,_TestCloseWhileConnecting— close during connectingNew tests
_TestSSLSendDuringHandshake— SSL client against plain TCP server (handshake stalls); verifysend()returnsSendErrorNotConnectedduring initial SSL handshake._TestSSLCloseDuringHandshake— SSL client against plain TCP server; timer callsclose()during stalled handshake. Verify_on_connection_failure(ConnectionFailedSSL)fires (close during_SSLHandshakingroutes through hard_close)._TestSSLHardCloseDuringHandshake— SSL client against plain TCP server; timer callshard_close()during stalled handshake. Verify_on_connection_failure(ConnectionFailedSSL)fires._TestSSLServerHardCloseDuringHandshake— SSL server accepts from plain TCP client; timer callshard_close(). Verify_on_start_failure(StartFailedSSL)fires._TestSSLHandshakeCompleteTransitionsToOpen— SSL client/server with valid certs; in_on_connectedcallback, assertis_open() == trueandsend()returnsSendToken._TestStartTLSSendDuringUpgrade— Plaintext connection ->start_tls()against plain TCP server (handshake stalls) -> verifysend()returnsSendErrorNotConnected.Counterfactual verification: after each new test passes, temporarily break the assertion to confirm it fires.
Build and test commands
Implementation Steps
ssl_handshake_complete()andsends_allowed()to_ConnectionStatetrait with implementations in all 6 existing states._dispatch_io_event(flags, arg)helper on TCPConnection from_Open.own_event()._SSLHandshakingstate class._TLSUpgradingstate class._hard_close_cleanup()from_hard_close_connected(). Add_hard_close_ssl_handshaking()and_hard_close_tls_upgrading()._hard_close_connected()— remove theif not _ssl_readybranch._establish_connection()—_SSLHandshakingwhen SSL present._complete_server_initialization()—_SSLHandshakingwhen SSL present._ssl_poll()SSLReady branch —ssl_handshake_complete()dispatch._do_start_tls()— transition to_TLSUpgrading._ssl_readyand_tls_upgradefields and all reads._do_send(),is_writeable(),idle_timeout(),set_timer()._enqueue()guard tonot _state.is_closed().make clean && make ssl=3.0.x.make ci ssl=3.0.x._TLSUpgrading), idle timeout / user timer sections (simplified gating), Future Work (remove SSL state machine bullet).Files modified
lori/_connection_state.pony— trait additions + 2 new state classeslori/tcp_connection.pony— remove 2 flags, refactor hard_close, update transitions, simplify gatinglori/_test_ssl.pony— new tests 1-5lori/_test_start_tls.pony— new test 6lori/_test.pony— register new testsCLAUDE.md— documentation updatesUser-facing impact
None. Internal refactoring. The
is_open()change during initial SSL handshake (true->false) is unobservable — no application callback fires during the handshake window, so the application has no entry point to callis_open(). No release notes or changelog entry needed.Open Questions for Discussion
I/O dispatch deduplication: Extract
_dispatch_io_event()helper (3 agents recommend), or duplicate across 4 states following existing convention (2 agents recommend)? This is a style choice._enqueue()during_Closing: Both the old guard (is_open()) and new guard (not is_closed()) block enqueuing during_Closing. This may cause missing TLS close_notify alerts during graceful shutdown. Pre-existing issue, not introduced by this change. File separately?All reactions