Skip to content

Send flights of handshake messages in single message #2120

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.

Sign up for GitHub

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

Merged
ctz merged 3 commits into from
Sep 16, 2024
Merged

Send flights of handshake messages in single message #2120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2569e82
server/tls12: send first flight in one message
ctz Sep 11, 2024
75e0492
server/tls13: send first flight in one message
ctz Sep 11, 2024
46204e6
client/tls13: send second flight in one message
ctz Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 35 additions & 65 deletions rustls/src/client/tls13.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ use crate::check::inappropriate_handshake_message;
use crate::client::common::{ClientAuthDetails, ClientHelloDetails, ServerCertDetails};
use crate::client::ech::{self, EchState, EchStatus};
use crate::client::{hs, ClientConfig, ClientSessionStore};
use crate::common_state::{CommonState, HandshakeKind, KxState, Protocol, Side, State};
use crate::common_state::{
CommonState, HandshakeFlightTls13, HandshakeKind, KxState, Protocol, Side, State,
};
use crate::conn::ConnectionRandoms;
use crate::crypto::ActiveKeyExchange;
use crate::enums::{
Expand Down Expand Up @@ -1155,12 +1157,11 @@ impl State<ClientConnectionData> for ExpectCertificateVerify<'_> {
}

fn emit_compressed_certificate_tls13(
transcript: &mut HandshakeHash,
flight: &mut HandshakeFlightTls13<'_>,
certkey: &CertifiedKey,
auth_context: Option<Vec<u8>>,
compressor: &dyn compress::CertCompressor,
config: &ClientConfig,
common: &mut CommonState,
) {
let mut cert_payload = CertificatePayloadTls13::new(certkey.cert.iter(), None);
cert_payload.context = PayloadU8::new(auth_context.clone().unwrap_or_default());
Expand All @@ -1170,84 +1171,56 @@ fn emit_compressed_certificate_tls13(
.compression_for(compressor, &cert_payload)
{
Ok(compressed) => compressed,
Err(_) => return emit_certificate_tls13(transcript, Some(certkey), auth_context, common),
Err(_) => return emit_certificate_tls13(flight, Some(certkey), auth_context),
};

let m = Message {
version: ProtocolVersion::TLSv1_3,
payload: MessagePayload::handshake(HandshakeMessagePayload {
typ: HandshakeType::CompressedCertificate,
payload: HandshakePayload::CompressedCertificate(compressed.compressed_cert_payload()),
}),
};
transcript.add_message(&m);
common.send_msg(m, true);
flight.add(HandshakeMessagePayload {
typ: HandshakeType::CompressedCertificate,
payload: HandshakePayload::CompressedCertificate(compressed.compressed_cert_payload()),
});
}

fn emit_certificate_tls13(
transcript: &mut HandshakeHash,
flight: &mut HandshakeFlightTls13<'_>,
certkey: Option<&CertifiedKey>,
auth_context: Option<Vec<u8>>,
common: &mut CommonState,
) {
let certs = certkey
.map(|ck| ck.cert.as_ref())
.unwrap_or(&[][..]);
let mut cert_payload = CertificatePayloadTls13::new(certs.iter(), None);
cert_payload.context = PayloadU8::new(auth_context.unwrap_or_default());

let m = Message {
version: ProtocolVersion::TLSv1_3,
payload: MessagePayload::handshake(HandshakeMessagePayload {
typ: HandshakeType::Certificate,
payload: HandshakePayload::CertificateTls13(cert_payload),
}),
};
transcript.add_message(&m);
common.send_msg(m, true);
flight.add(HandshakeMessagePayload {
typ: HandshakeType::Certificate,
payload: HandshakePayload::CertificateTls13(cert_payload),
});
}

fn emit_certverify_tls13(
transcript: &mut HandshakeHash,
flight: &mut HandshakeFlightTls13<'_>,
signer: &dyn Signer,
common: &mut CommonState,
) -> Result<(), Error> {
let message = construct_client_verify_message(&transcript.current_hash());
let message = construct_client_verify_message(&flight.transcript.current_hash());

let scheme = signer.scheme();
let sig = signer.sign(&message)?;
let dss = DigitallySignedStruct::new(scheme, sig);

let m = Message {
version: ProtocolVersion::TLSv1_3,
payload: MessagePayload::handshake(HandshakeMessagePayload {
typ: HandshakeType::CertificateVerify,
payload: HandshakePayload::CertificateVerify(dss),
}),
};

transcript.add_message(&m);
common.send_msg(m, true);
flight.add(HandshakeMessagePayload {
typ: HandshakeType::CertificateVerify,
payload: HandshakePayload::CertificateVerify(dss),
});
Ok(())
}

fn emit_finished_tls13(
transcript: &mut HandshakeHash,
verify_data: &crypto::hmac::Tag,
common: &mut CommonState,
) {
fn emit_finished_tls13(flight: &mut HandshakeFlightTls13<'_>, verify_data: &crypto::hmac::Tag) {
let verify_data_payload = Payload::new(verify_data.as_ref());

let m = Message {
version: ProtocolVersion::TLSv1_3,
payload: MessagePayload::handshake(HandshakeMessagePayload {
typ: HandshakeType::Finished,
payload: HandshakePayload::Finished(verify_data_payload),
}),
};

transcript.add_message(&m);
common.send_msg(m, true);
flight.add(HandshakeMessagePayload {
typ: HandshakeType::Finished,
payload: HandshakePayload::Finished(verify_data_payload),
});
}

fn emit_end_of_early_data_tls13(transcript: &mut HandshakeHash, common: &mut CommonState) {
Expand Down Expand Up @@ -1321,22 +1294,24 @@ impl State<ClientConnectionData> for ExpectFinished {
.set_handshake_encrypter(cx.common);
}

let mut flight = HandshakeFlightTls13::new(&mut st.transcript);

/* Send our authentication/finished messages. These are still encrypted
* with our handshake keys. */
if let Some(client_auth) = st.client_auth {
match client_auth {
ClientAuthDetails::Empty {
auth_context_tls13: auth_context,
} => {
emit_certificate_tls13(&mut st.transcript, None, auth_context, cx.common);
emit_certificate_tls13(&mut flight, None, auth_context);
}
ClientAuthDetails::Verify {
auth_context_tls13: auth_context,
..
} if cx.data.ech_status == EchStatus::Rejected => {
// If ECH was offered, and rejected, we MUST respond with
// an empty certificate message.
emit_certificate_tls13(&mut st.transcript, None, auth_context, cx.common);
emit_certificate_tls13(&mut flight, None, auth_context);
}
ClientAuthDetails::Verify {
certkey,
Expand All @@ -1346,22 +1321,16 @@ impl State<ClientConnectionData> for ExpectFinished {
} => {
if let Some(compressor) = compressor {
emit_compressed_certificate_tls13(
&mut st.transcript,
&mut flight,
&certkey,
auth_context,
compressor,
&st.config,
cx.common,
);
} else {
emit_certificate_tls13(
&mut st.transcript,
Some(&certkey),
auth_context,
cx.common,
);
emit_certificate_tls13(&mut flight, Some(&certkey), auth_context);
}
emit_certverify_tls13(&mut st.transcript, signer.as_ref(), cx.common)?;
emit_certverify_tls13(&mut flight, signer.as_ref())?;
}
}
}
Expand All @@ -1370,12 +1339,13 @@ impl State<ClientConnectionData> for ExpectFinished {
.key_schedule
.into_pre_finished_client_traffic(
hash_after_handshake,
st.transcript.current_hash(),
flight.transcript.current_hash(),
&*st.config.key_log,
&st.randoms.client,
);

emit_finished_tls13(&mut st.transcript, &verify_data, cx.common);
emit_finished_tls13(&mut flight, &verify_data);
flight.finish(cx.common);

/* We're now sure this server supports TLS1.3. But if we run out of TLS1.3 tickets
* when connecting to it again, we definitely don't want to attempt a TLS1.2 resumption. */
Expand Down
46 changes: 44 additions & 2 deletions rustls/src/common_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ use pki_types::CertificateDer;
use crate::crypto::SupportedKxGroup;
use crate::enums::{AlertDescription, ContentType, HandshakeType, ProtocolVersion};
use crate::error::{Error, InvalidMessage, PeerMisbehaved};
use crate::hash_hs::HandshakeHash;
use crate::log::{debug, error, warn};
use crate::msgs::alert::AlertMessagePayload;
use crate::msgs::base::Payload;
use crate::msgs::codec::Codec;
use crate::msgs::enums::{AlertLevel, KeyUpdateRequest};
use crate::msgs::fragmenter::MessageFragmenter;
use crate::msgs::handshake::CertificateChain;
use crate::msgs::handshake::{CertificateChain, HandshakeMessagePayload};
use crate::msgs::message::{
Message, MessagePayload, OutboundChunks, OutboundOpaqueMessage, OutboundPlainMessage,
PlainMessage,
Expand Down Expand Up @@ -432,7 +434,10 @@ impl CommonState {
self.quic.alert = Some(alert.description);
} else {
debug_assert!(
matches!(m.payload, MessagePayload::Handshake { .. }),
matches!(
m.payload,
MessagePayload::Handshake { .. } | MessagePayload::HandshakeFlight(_)
),
"QUIC uses TLS for the cryptographic handshake only"
);
let mut bytes = Vec::new();
Expand Down Expand Up @@ -974,5 +979,42 @@ impl KxState {
}
}

pub(crate) struct HandshakeFlight<'a, const TLS13: bool> {
Copy link
Member

@cpu cpu Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I briefly considered the restrictiveness of the const generic bool here vs something more expressive but decided it was premature optimization and this seems quite reasonable after all.

pub(crate) transcript: &'a mut HandshakeHash,
body: Vec<u8>,
}

impl<'a, const TLS13: bool> HandshakeFlight<'a, TLS13> {
pub(crate) fn new(transcript: &'a mut HandshakeHash) -> Self {
Self {
transcript,
body: Vec::new(),
}
}

pub(crate) fn add(&mut self, hs: HandshakeMessagePayload<'_>) {
let start_len = self.body.len();
hs.encode(&mut self.body);
self.transcript
.add(&self.body[start_len..]);
}

pub(crate) fn finish(self, common: &mut CommonState) {
common.send_msg(
Message {
version: match TLS13 {
true => ProtocolVersion::TLSv1_3,
false => ProtocolVersion::TLSv1_2,
},
payload: MessagePayload::HandshakeFlight(Payload::new(self.body)),
},
TLS13,
);
}
}

pub(crate) type HandshakeFlightTls12<'a> = HandshakeFlight<'a, false>;
Copy link
Member

@cpu cpu Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the TLS 1.2 use-case landed alongside the new types to avoid dead code warnings? I think this diff would be easier to digest if it were separated out like the TLS 1.3 case but I wager that might produce warnings and I don't feel very strongly.

Copy link
Member

@djc djc Sep 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, not sure I'm convinced of the benefit of this type alias.

Copy link
Member

@cpu cpu Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the type alias is mildly useful in so much as false in the generic bound doesn't clearly map to TLS1.2 without doing a bit of digging around.

Copy link
Member Author

@ctz ctz Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I would like to avoid the true/false thing leaking out of this file, as far as possible.

My second choice would be to stop this type being generic like this, and just have two different constructors (eg, new_tls12(), new_tls13() or similar) and rely on everything being inlined into their uses.

pub(crate) type HandshakeFlightTls13<'a> = HandshakeFlight<'a, true>;

const DEFAULT_RECEIVED_PLAINTEXT_LIMIT: usize = 16 * 1024;
pub(crate) const DEFAULT_BUFFER_LIMIT: usize = 64 * 1024;
Loading