Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #749 +/- ##
==========================================
+ Coverage 81.34% 81.43% +0.09%
==========================================
Files 101 102 +1
Lines 5602 5689 +87
==========================================
+ Hits 4557 4633 +76
- Misses 672 679 +7
- Partials 373 377 +4
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:
|
|
Thanks for this draft! I have made some adjustments to your suggestion. I have taken inspiration from the TLS 1.3 implementation in the standard library where they reuse TLS 1.2 types and extensions rather than renaming or creating new - this simplified the implementation a lot. Additionally, I think we should only stick to the groups supported by the standard library and the TLS 1.3 implementation. Tests were a bit flaky with marshaling and unmarshaling in the same test. Therefore, I added tests with raw bytes captured from the NSS stack of Firefox with Wireshark (pro tip: you can copy bytes as a Go literal) and some handcrafted bytes. |
27c0863 to
a0986ed
Compare
|
@philipch07 @theodorsm thank you <3 i'm going to read the spec and submit a review this week 👍 |
philipch07
left a comment
There was a problem hiding this comment.
@theodorsm Thank you for the changes!! It's much cleaner now and I like the approach that you took regarding the renaming.
I wonder if there's a way that we can be extra clear in the docs about the renaming since the filename may be misleading for readers, but I don't have an answer to that off the top of my head. The comments that you added are still very useful.
faa49a2 to
4cfd7cd
Compare
|
@philipch07 thanks for the feedback. Regarding naming, we could change it to something like |
|
@theodorsm That sounds good to me, and thanks for the updates. I'm looking forward to @joeturki's review and then I think we should be good to go if you're feeling good about it! I'm really happy to see the progress so far :) |
JoTurk
left a comment
There was a problem hiding this comment.
two comments. sorry i lost track of this.
|
@JoTurk, thanks for the review! (no worries about forgetting) I have fixed your comments. |
|
@theodorsm Thanks for updating this! Sorry I was busy recently. I would approve it but I can't since I'm the PR author 😅 |
JoTurk
left a comment
There was a problem hiding this comment.
Thank you for the fixes theo, and welcome back :)
afa0502 to
293cc15
Compare
Description
This adds the
supported_versionsextension feature in accordance with DTLS v1.3 which refers to TLS 1.3 section 4.2.8, 4.2.8.1, and 4.2.8.2.Note about the ci failures:
This currently uses a global variable to test that my current logic is valid, so it will break all the DTLS v1.2 tests. At the moment, this is blocked by #738 as it requires a proper config/switch. I'm not sure what the best way of setting the toggle would be in
extensions.go, but hopefully my current approach makes some sense.Reference issue
Closes #743.