Conversation
HMBSbige
force-pushed
the
feature/close-notify
branch
from
7d7f3e8 to
77bf722
Compare
algesten
left a comment
algesten
left a comment
There was a problem hiding this comment.
Thanks for looking at this!
A couple of comments and questions. I also see we have some unrelated code cleanup, like moving code around and removing white space, and a change to &mut OsRng. We can keep that in PR, but you need to force push and make "clean" commits I can rebase at the end.
HMBSbige
force-pushed
the
feature/close-notify
branch
from
77bf722 to
9820a7a
Compare
- Remove unnecessary `&mut` on `OsRng` (rust_crypto kx_group) - Replace `if let Err(_)` with idiomatic `.is_err()` (auto cross_matrix) - Inline `self.last_now` and remove extra blank lines in Server::poll_output - Move helper functions before `#[cfg(test)]` per file ordering convention
HMBSbige
force-pushed
the
feature/close-notify
branch
from
9820a7a to
ebb9d22
Compare
algesten
left a comment
algesten
left a comment
There was a problem hiding this comment.
Thanks!
Much improved with the client/server having the state logic.
This read through I got stuck on insert_incoming and the amount of logic being done there. I see dtls13 has set some pattern that I don't quite feel is right.
| // Handle Alert records individually; collect the rest for queuing. | ||
| // A single UDP datagram can contain mixed record types, so we process | ||
| // each record individually without discarding siblings. | ||
| let mut remaining = ArrayVec::new(); |
There was a problem hiding this comment.
The goal throughout dimpl is to avoid extra buffering like this if we can. I.e. here we "pre-process" all the incoming records into a new temporary field.
I'm also in two minds whether this really belongs in a function labelled "insert_incoming" (because it's more like "preprocess").
The code almost feels more related to the decryption pass we do in incoming.rs via the RecordDecrypt trait.
What do you think? Would it sit more naturally in incoming.rs?
| Some(Incoming { | ||
| records: Box::new(Records { records }), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Yeah, this doesn't feel great, recreating the incoming like this. Related to previous comment.
| // each record individually without discarding siblings. | ||
| let mut non_ack_records = ArrayVec::new(); | ||
| for record in incoming.into_records() { | ||
| // Per RFC 9147 §5.10: discard any record whose epoch/sequence |
There was a problem hiding this comment.
I see now where this into_records/from_records pattern comes from. I wasn't aware we had this situation. Feels like we should maybe tidy this up as well and not do this in "insert incoming"
HMBSbige
force-pushed
the
feature/close-notify
branch
from
ebb9d22 to
82fbb62
Compare
Add close_notify support for both DTLS 1.2 and DTLS 1.3, implementing
graceful connection shutdown per RFC 5246 §7.2.1 and RFC 9147 §5.10.
DTLS 1.2: close() sends close_notify and transitions to Closed state.
Receiving close_notify triggers a reciprocal alert and discards pending
writes. No half-close support (full close only).
DTLS 1.3: close() sends close_notify and enters HalfClosedLocal state
where the read half remains open. Receiving close_notify while
half-closed transitions to Closed. Incoming KeyUpdate messages are
still processed (recv keys updated) but no outgoing records are sent.
Engine tracks close_notify at the record layer (filtering app data
after the alert sequence), while client/server handle connection state
and Output::CloseNotify emission.
Error::ConnectionClosed replaces SecurityError("connection closed") for
send_application_data on closed connections.
HMBSbige
force-pushed
the
feature/close-notify
branch
from
82fbb62 to
563ed6a
Compare
HMBSbige
left a comment
HMBSbige
left a comment
There was a problem hiding this comment.
I've extracted the per-record processing out of insert_incoming into dedicated methods:
- dtls12:
extract_alerts()— epoch-aware alert handling + post-close_notify filtering - dtls13:
extract_control_records()— ACK/Alert/CCS + close_notify sequence filtering
insert_incoming is back to capacity-check + dispatch in both versions.
The into_records/from_records decompose-recompose pattern is still there though. I considered moving it into incoming.rs alongside RecordDecrypt, but alert processing needs engine state (peer_encryption_enabled, buffers_free, close_notify_received/close_notify_sequence) which doesn't fit the stateless parsing layer.
If you have a different direction in mind for eliminating the pattern, I'm happy to follow your lead.
2 participants
Closes #72
Code cleanup (commit 1)
&mutonOsRngin rust_crypto kx_groupif let Err(_)with idiomatic.is_err()in auto cross_matrix testsself.last_nowand remove extra blank lines inServer::poll_output(dtls12)#[cfg(test)]per file ordering convention (dtls12 server)close_notify implementation (commit 2)
Dtls::close()API for graceful connection shutdown viaclose_notifyalertOutput::CloseNotifyvariant to signal peer-initiated closure to the applicationError::ConnectionClosedfor send attempts on closed connectionsclose_notifyper RFC 5246 §7.2.1HalfClosedLocal(write-closed, read-open) per RFC 8446 §6.1; receiver filters records by sequence number per RFC 9147 §5.10Output::CloseNotifyemissioncomplete_dtls12_handshake,setup_connected_12_pair,complete_dtls13_handshake,setup_connected_13_pair) from edge.rs into common.rs and consolidate importsOutput::CloseNotifydocumentation and example