-
Notifications
You must be signed in to change notification settings - Fork 7
[WIP] DTLS 1.3 #38
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
base: main
Are you sure you want to change the base?
[WIP] DTLS 1.3 #38
Conversation
Cargo.toml
Outdated
|
|
||
| [dev-dependencies] | ||
| openssl = { version = "0.10.70", features = ["vendored"] } | ||
| boring = "4.19" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clean this up
algesten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been looking through the PR in detail. My gut feeling is that I underestimated just how different DTLS 1.3 is.
I'm totally on the fence on whether it should be co-mingled like it is in this PR, or whether we should just have dtls12, dtls13 as top level modules and accept some code duplication.
We can share buffer handling, crypto primitives etc.
634bf02 to
ba7cc8e
Compare
4738e5d to
62f88bb
Compare
| //! Each epoch has its own traffic secrets derived via the TLS 1.3 key schedule. | ||
| //! | ||
| //! NOTE: This module is prepared for full DTLS 1.3 record layer integration. | ||
| //! Some items are currently unused but will be connected in future work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove what is not used.
| /// The AEAD cipher instance. | ||
| cipher: Box<dyn Cipher>, | ||
| /// Full 12-byte IV for nonce construction. | ||
| iv: Iv13, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iv13 should we just move this to 1.3 mod?
| let aad = Aad13::from_header(header_bytes); | ||
|
|
||
| // Decrypt in place using variable-length AAD for DTLS 1.3 | ||
| let mut tmp = TmpBuf::new(ciphertext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TmpBuf?
| assert_eq!(ctx.send_epoch(), 0); | ||
| assert_eq!(ctx.recv_epoch(), 0); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More tests, check code coverage?
| pub fn as_bytes(&self) -> &[u8] { | ||
| &self.data[..self.len] | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests?
|
|
||
| /// Extract SRTP keying material using TLS 1.3 exporter (RFC 8446 Section 7.5) | ||
| /// This is used for DTLS-SRTP with DTLS 1.3. | ||
| pub fn extract_srtp_keying_material_tls13( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test?
| info: &[u8], | ||
| out: &mut Buf, | ||
| output_len: usize, | ||
| ) -> Result<(), String>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use proper error handling here and not just "String" ?
| pub fn derive_application_secrets( | ||
| &mut self, | ||
| transcript_hash: &[u8], | ||
| ) -> Result<(Buf, Buf), String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error instead of string
| type Dtls13CipherResult = Option<(Box<dyn crate::crypto::Cipher>, [u8; 12], [u8; 16])>; | ||
|
|
||
| /// DTLS 1.3 client state machine. | ||
| pub struct Client13 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we tack on 13 on all these names or should we move them into this mod?
| impl Client13 { | ||
| pub(crate) fn new_with_engine(mut engine: Engine) -> Client13 { | ||
| engine.set_client(true); | ||
| engine.set_dtls13(true); // DTLS 1.3 transcript format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have 2 different engines do we need this?
| state: State::SendClientHello, | ||
| engine, | ||
| last_now: None, | ||
| local_events: VecDeque::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer default instead of new()
| } | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a comment about what test-helpers are for
| server_version, cipher_suite | ||
| ); | ||
|
|
||
| // In DTLS 1.3, the legacy version field is 0xFEFD (DTLS 1.2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add reference for this and why this is
|
|
||
| /// Schedule an immediate ACK (like BoringSSL's `schedule_ack()`). | ||
| /// Used when we want to ACK right away, e.g., after receiving client Finished. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check why this ha s allow(dead_code)
| // delayed ACK when just acknowledging received data. | ||
| let delay = if self.has_gap_in_incoming_handshake() { | ||
| // Gap detected: immediate ACK to trigger fast retransmit | ||
| Duration::from_millis(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duration::ZERO?
| } else { | ||
| // No gap: use RTO/4 delay for potential piggybacking | ||
| let rto = self.flight_backoff.rto(); | ||
| if rto > Duration::from_millis(0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duration::Zero
| if rto > Duration::from_millis(0) { | ||
| rto / 4 | ||
| } else { | ||
| Duration::from_millis(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole logic looks funky, revisit
| invariants are later observed across a catch_unwind boundary. Marking Incoming | ||
| as UnwindSafe is a sound assertion and clarifies behavior for callers. | ||
| */ | ||
| impl std::panic::UnwindSafe for Incoming {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisit this
| ) | ||
| } | ||
|
|
||
| // ============================================================================= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove ===== comments everywhere
| @@ -0,0 +1,1154 @@ | |||
| //! DTLS 1.3 interop tests: dimpl client <-> WolfSSL server | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revisit new test files and see if we can make more code dry and reusable
| //! These tests verify DTLS 1.3 handshake, encryption, SRTP keying material export, | ||
| //! retransmission handling, and application data exchange. | ||
|
|
||
| #![allow(unused)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check allow here
| } | ||
|
|
||
| /// Helper to collect all outputs (packets and events) from an endpoint. | ||
| fn drain_outputs(endpoint: &mut Dtls) -> DrainedOutputs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helpers can be moved?
| ); | ||
| } | ||
|
|
||
| // NOTE: This test is commented out due to WolfSSL state machine quirks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't leave dead code, cleanup
| //! ClientKeyExchange messages from TLS 1.2. | ||
| //! | ||
| //! NOTE: This module is prepared for DTLS 1.3 but not yet fully integrated | ||
| //! into the main extension parsing paths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obsolete comment?
| //! contains extensions that don't need to be in the clear (everything except | ||
| //! key_share, pre_shared_key, and supported_versions). | ||
| //! | ||
| //! NOTE: This module is prepared for DTLS 1.3 but not yet fully integrated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this Note still relevant?
| use std::ops::Range; | ||
|
|
||
| #[derive(Debug, PartialEq, Eq, Default)] | ||
| #[derive(Debug, Clone, PartialEq, Eq, Default)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was Clone added here?
| // to account for fragmentation across multiple records | ||
| const MAX_HANDSHAKE_LENGTH: u32 = 32 * 1024; | ||
| if header.length > MAX_HANDSHAKE_LENGTH { | ||
| return Err(nom::Err::Failure(Error::new(input, ErrorKind::TooLarge))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks quite verbose
| //! The `ParseContext` carries the negotiated DTLS version and enforces | ||
| //! strictness rules to reject messages that are invalid for a given version. | ||
| //! | ||
| //! NOTE: This module is prepared for version-aware parsing but not yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Search for all these NOTE:
| @@ -0,0 +1,471 @@ | |||
| //! DTLS 1.3 Record Layer (RFC 9147) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we ove record.rs and record13.rs to the mods?

POC
It passes some tests, but there is a lot needed here.