Skip to content

Add supported_versions extension feature from DTLS v1.3#730

Merged
JoTurk merged 1 commit intomasterfrom
pch07/dtls13-initial-supported-versions-extension
Sep 25, 2025
Merged

Add supported_versions extension feature from DTLS v1.3#730
JoTurk merged 1 commit intomasterfrom
pch07/dtls13-initial-supported-versions-extension

Conversation

@philipch07
Copy link
Contributor

@philipch07 philipch07 commented Sep 20, 2025

Description

This adds the supported_versions extension feature in accordance with DTLS v1.3 section 5.3. Note that it links to TLS 1.3 section 4.2.1.

Reference issue

Completes #729

@codecov
Copy link

codecov bot commented Sep 20, 2025

Codecov Report

❌ Patch coverage is 80.23256% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.64%. Comparing base (120a895) to head (f6b0286).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/protocol/extension/supported_versions.go 80.26% 10 Missing and 5 partials ⚠️
pkg/protocol/extension/extension.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #730      +/-   ##
==========================================
+ Coverage   78.59%   78.64%   +0.04%     
==========================================
  Files         101      102       +1     
  Lines        6830     6916      +86     
==========================================
+ Hits         5368     5439      +71     
- Misses       1087     1098      +11     
- Partials      375      379       +4     
Flag Coverage Δ
go 78.66% <80.23%> (+0.04%) ⬆️
wasm 57.46% <80.23%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@philipch07 philipch07 force-pushed the pch07/dtls13-initial-supported-versions-extension branch 2 times, most recently from 555d156 to e0c1726 Compare September 20, 2025 19:33
@philipch07 philipch07 marked this pull request as ready for review September 20, 2025 22:31
Copy link
Member

@theodorsm theodorsm left a comment

Choose a reason for hiding this comment

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

@philipch07 Thanks for diving into this extension so fast!

Removing the Selected field should simplify this implementation + tests a bit.

Additionally, we should check upon unmarshaling if the parsed major/minor version is supported by us, i.e is it one of version vars defined in version.go. We should discard versions not supported and only append supported versions.

@philipch07
Copy link
Contributor Author

philipch07 commented Sep 21, 2025

Additionally, we should check upon unmarshaling if the parsed major/minor version is supported by us, i.e is it one of version vars defined in version.go. We should discard versions not supported and only append supported versions.

For discarding, I think this can cause an unmatched value with marshalling and unmarshalling: for example, lets say the list passed in is:

[VersionX_Y, Version1_3, Version1_2, Version1_0]

Then it would be marshalled (and later unmarshalled) as:

[Version1_3, Version1_2, Version1_0]

Which is no longer the same as the original input. Is that behavior still desirable? I'm not sure if it causes any issues, but it was just something that I noticed.

(Also thanks for reviewing so quickly! I really appreciate the quick feedback!)

@philipch07 philipch07 force-pushed the pch07/dtls13-initial-supported-versions-extension branch from e0c1726 to fa1758b Compare September 21, 2025 16:33
Copy link
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

I think this should be a dumb packs/unpacks bytes and formats.
no "is this version supported", no negotiation, no state. no policy leaking.

and all the handling/decision-making/logic should be in the handshake layer, like other extensions.

@philipch07 philipch07 force-pushed the pch07/dtls13-initial-supported-versions-extension branch from fa1758b to 41ae364 Compare September 21, 2025 18:10
@philipch07 philipch07 force-pushed the pch07/dtls13-initial-supported-versions-extension branch from 41ae364 to 0d2ee7d Compare September 22, 2025 19:25
Copy link
Member

@theodorsm theodorsm left a comment

Choose a reason for hiding this comment

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

Would be nice with a test case with some non-existent version values too (ex: 0xef, 0x0d)

@philipch07 philipch07 force-pushed the pch07/dtls13-initial-supported-versions-extension branch from a8408e5 to d5947c4 Compare September 24, 2025 22:13
@theodorsm
Copy link
Member

I added the missing extension type bytes in my commit.

@philipch07, thank you so much for taking on this implementation and being patient with feedback!

This PR LGTM now. @joeturki, @Sean-Der, what do you think?

@theodorsm theodorsm requested a review from JoTurk September 25, 2025 16:29
Copy link
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

Thank you so much

Co-authored-by: theodorsm <theodor@midtlien.com>
@JoTurk JoTurk force-pushed the pch07/dtls13-initial-supported-versions-extension branch from d54c125 to f6b0286 Compare September 25, 2025 16:40
@JoTurk
Copy link
Member

JoTurk commented Sep 25, 2025

@philipch07 @theodorsm feel free to merge it, if it's ready :)

@JoTurk JoTurk merged commit f6b0286 into master Sep 25, 2025
18 checks passed
@JoTurk JoTurk deleted the pch07/dtls13-initial-supported-versions-extension branch September 25, 2025 16:52
@philipch07
Copy link
Contributor Author

Thanks @theodorsm and @joeturki for the feedback, fixing it up, and merging! Sorry I didn't see the messages earlier as I'm currently at work. I'm looking forward to the next tasks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants