Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (67.60%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #793 +/- ##
==========================================
- Coverage 82.47% 82.32% -0.16%
==========================================
Files 121 123 +2
Lines 6928 6999 +71
==========================================
+ Hits 5714 5762 +48
- Misses 803 821 +18
- Partials 411 416 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c26fc58 to
8043e6f
Compare
8043e6f to
ec6817e
Compare
There was a problem hiding this comment.
Pull request overview
Implements DTLS 1.3 “Unified Header” support (RFC 9147 §4) and wires it into recordlayer.Header marshal/unmarshal paths.
Changes:
- Add
UnifiedHeadermarshal/unmarshal implementation for DTLS 1.3. - Route
recordlayer.Headermarshal/unmarshal through the DTLS 1.3 unified header when DTLS 1.3 is detected/selected. - Add unit tests and supporting errors/helpers (
IsDTLS13Ciphertext).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/protocol/recordlayer/header_13.go | New DTLS 1.3 unified header encoding/decoding. |
| pkg/protocol/recordlayer/header_13_test.go | Tests for unified header and Header.Unmarshal DTLS 1.3 path. |
| pkg/protocol/recordlayer/header.go | DTLS 1.3 branch in Header.Marshal and Header.Unmarshal. |
| pkg/protocol/recordlayer/errors.go | Adds unified-header-specific errors and reformats existing ones. |
| pkg/protocol/content.go | Adds helper to detect DTLS 1.3 ciphertext/unified-header first byte. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if u.Length > 0 { | ||
| contentType |= UnifiedHeaderLengthBit | ||
| head.AddUint16(u.Length) | ||
| } |
There was a problem hiding this comment.
The L bit (length present) is controlled by whether the Length field is included, not by whether the length value is non-zero. Using u.Length > 0 to decide whether to set the length bit makes it impossible to (a) omit the length field for non-zero payloads (common for the last record in a datagram) or (b) include a zero length field if needed. Consider adding an explicit boolean (e.g., LengthPresent) and using it to control the L bit independently from the value.
There was a problem hiding this comment.
The length can be controlled by not initializing Length or by setting it to 0 explicitly, there is no need for a seperate boolean.
There was a problem hiding this comment.
Similar comment to S bit i guess.
|
Can you please look at the AI's comments, I think some of them are worth fixing, I'll re read the rfcs and the code and review all your pending prs, sorry it might take a bit because I caught a fever :) |
JoTurk
left a comment
There was a problem hiding this comment.
Thank you, sorry i was taking a short break, before things really start, I read the spec few times, my comments are nits feel free to ignore them but I think they are kinda important for tests, and for long term. so we don't have to break the api in the future.
Thank you and sorry again!
| if u.Length > 0 { | ||
| contentType |= UnifiedHeaderLengthBit | ||
| head.AddUint16(u.Length) | ||
| } |
There was a problem hiding this comment.
Similar comment to S bit i guess.
| ) | ||
|
|
||
| func TestUnifiedHeader(t *testing.T) { | ||
| uh := UnifiedHeader{SequenceNumber: 0xaabb, SeqBit: true, Length: 42, LengthBit: true, EpochLow: 15} |
There was a problem hiding this comment.
The usage of the UnifiedHeader is now very explicit and I would argue it's easier to forget to set the bit flags (we just have to be aware). But I guess it's worth it with the control and round trip correctness.
Interestingly, boringssl only uses 16-bit seq numbers: https://pigweed.googlesource.com/third_party/github/google/boringssl/%2B/HEAD/ssl/dtls_record.cc#541
There was a problem hiding this comment.
I wonder if we should do the same as boringssl.
|
@JoTurk, thanks for the review! No worries, I have been out sick this week. I have added the bits to the struct, but now the interface is very explicit. We can try this interface out and see if it becomes messy, if it does, we can move the bits to be implicit again. |
|
@theodorsm Sorry about this, I hope you're doing better. |
Description
This PR implements the new DTLS 1.3 Unified Header that is part of the new record layer encoding.
https://datatracker.ietf.org/doc/html/rfc9147#name-the-dtls-record-layer:
Reference issue
Partly fixes #755