Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factor out HandshakeDetails into the explicit state machine #461

Closed
briansmith opened this issue Feb 3, 2021 · 2 comments
Closed

Factor out HandshakeDetails into the explicit state machine #461

briansmith opened this issue Feb 3, 2021 · 2 comments

Comments

@briansmith
Copy link
Contributor

Currently we have, for the client:

pub struct HandshakeDetails {
    pub resuming_session: Option<persist::ClientSessionValue>,
    pub transcript: hash_hs::HandshakeHash,
    pub hash_at_client_recvd_server_hello: Vec<u8>,
    pub randoms: SessionRandoms,
    pub using_ems: bool,
    pub session_id: SessionID,
    pub sent_tls13_fake_ccs: bool,
    pub dns_name: webpki::DNSName,
    pub extra_exts: Vec<ClientExtension>,
}

and for the server:

pub struct HandshakeDetails {
    pub transcript: hash_hs::HandshakeHash,
    pub hash_at_server_fin: Vec<u8>,
    pub session_id: SessionID,
    pub randoms: SessionRandoms,
    pub using_ems: bool,
    pub extra_exts: Vec<ServerExtension>,
}

These structures live from the beginning of the handshake through the end of the handshake. However, the values within the structures aren't needed for the entire handshake, but it is not clear which fields are needed in which parts of the handshake. It is clear that we're wasting some memory storing some state we don't need, but it isn't clear how much we're wasting. Even more importantly, the values within the structures are not valid throughout the whole handshake; again, it isn't clear without external understanding of the TLS state machines when each value might be valid.

Rustls already encodes the handshake in an explicit state machine with explicit states, e.g. ExpectServerHello, and explicit transitions, e.g. ExpectServerDone::into_expect_tls13_encrypted_extensions. These explicit states model (more-or-less) the actual valid and necessary state of the handshake. Values that are needed throughout multiple states are threaded from the old state into the new state through the transition functions. If we "just" move the fields from HandshakeDetails and thread them through the state machine appropriately, then we'll have a state machine that is much more clearly correct. I know this is practical because I did the same in my own Rust TLS implementation.

To demonstrate how we can do this incrementally and relatively easily, I have posted PR #460, which demonstrates reducing the lifetime of the QUIC transport parameters to just the part of the handshake that deals with sending the Client Hello(s). I also have a follow-up PR queued up that factors sent_tls13_fake_ccs out of the client HandshakeDetails and better encapsulates the logic for fake CCS.

I also experimented a bit with other parts of HandshakeDetails. Definitely some are easier to factor out than others. I think the wins in clarity of correctness warrant doing this for all fields of HandshakeDetails, but I think some are higher priority than others. For example, the state machine for the handshake transcript hash has distinct phases (before we're committed to the hash algorithm and we cannot start calculating digests, after we're committed to the hash algorithm and can start calculating digests, and end states at the end of the handshake and when we need to deal with HelloRetryRequest) that should be modeled as its own state machine, with different types at each phase. IMO that refactoring should be done sooner rather than later.

@ctz WDYT? I thought at RWC a year or two ago you mentioned you were working on something similar already. Not sure what your current thinking is.

@sayrer
Copy link
Contributor

sayrer commented Mar 12, 2021

I factored out dns_name in #539, while I was trying to figure out what the ECH API might look like. I'll make that bit into a separate PR once @djc lands his PRs for randoms and transcript.

@djc
Copy link
Member

djc commented Apr 19, 2021

This is complete.

@djc djc closed this as completed Apr 19, 2021
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

No branches or pull requests

3 participants