Skip to content

DTLS 1.3 signature_algorithms_cert Extension#788

Merged
adrianosela merged 1 commit intopion:masterfrom
adrianosela:sig-algs-update
Feb 4, 2026
Merged

DTLS 1.3 signature_algorithms_cert Extension#788
adrianosela merged 1 commit intopion:masterfrom
adrianosela:sig-algs-update

Conversation

@adrianosela
Copy link
Copy Markdown
Contributor

@adrianosela adrianosela commented Jan 30, 2026

DTLS 1.3 signature_algorithms_cert Extension

Addresses part of #776: Adds support for the signature_algorithms_cert extension.

Note: Per https://www.rfc-editor.org/rfc/rfc8446#section-4.2.3 and https://www.rfc-editor.org/rfc/rfc8446#section-1.3 this extension may optionally apply to DTLS 1.2, so we can safely ship any net-new additions to the API e.g. CertificateSignatureSchemes in config.

@adrianosela adrianosela requested a review from theodorsm January 30, 2026 18:11
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 30, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 79.24528% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.46%. Comparing base (0a5b311) to head (06fb6dc).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
options.go 0.00% 7 Missing and 1 partial ⚠️
conn.go 22.22% 6 Missing and 1 partial ⚠️
crypto.go 90.24% 2 Missing and 2 partials ⚠️
flight1handler.go 0.00% 3 Missing and 1 partial ⚠️
flight3handler.go 0.00% 3 Missing and 1 partial ⚠️
flight0handler.go 0.00% 2 Missing ⚠️
pkg/protocol/extension/extension.go 50.00% 2 Missing ⚠️
...ocol/extension/generic_signature_hash_extension.go 94.28% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
- Coverage   81.65%   81.46%   -0.20%     
==========================================
  Files         109      111       +2     
  Lines        6384     6501     +117     
==========================================
+ Hits         5213     5296      +83     
- Misses        771      799      +28     
- Partials      400      406       +6
Flag Coverage Δ
go 81.46% <79.24%> (-0.20%) ⬇️

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.

@adrianosela
Copy link
Copy Markdown
Contributor Author

adrianosela commented Jan 30, 2026

This PR needs some clean-up and a lot more unit tests. But I figured I'd open it and ask for any early feedback cc:// @theodorsm @JoTurk

@adrianosela adrianosela force-pushed the sig-algs-update branch 2 times, most recently from f8ccea5 to ba183cf Compare January 31, 2026 20:24
@adrianosela
Copy link
Copy Markdown
Contributor Author

adrianosela commented Jan 31, 2026

This PR is ready for formal review now.

@adrianosela
Copy link
Copy Markdown
Contributor Author

adrianosela commented Feb 1, 2026 •
edited
Loading

If you want to play with the new CertificateSignatureSchemes config setting, you may take a look at the end-to-end listen/dial example in this commit, which I intend to PR after this goes out.

theodorsm
theodorsm requested changes Feb 2, 2026
View reviewed changes
Copy link
Copy Markdown
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.

Tnx! I left some comment.

As this introduces a new config flag, perhaps we should wait until #786 is merged? What do you think @JoTurk?

@JoTurk
Copy link
Copy Markdown
Member

JoTurk commented Feb 2, 2026

I'm going to review this tonight. Looks so cool thank you so much.

perhaps we should wait until #786 is merged? What do you think @JoTurk?

I agree, sorry i got busy with life stuff. I'll get #786 ready for review tonight too.

@adrianosela
Copy link
Copy Markdown
Contributor Author

adrianosela commented Feb 2, 2026

Thanks for the review @theodorsm. Looking forward to your @JoTurk :D

I've made your requested changes in new commits @theodorsm , just to make review easier. Once you are happy I will squash into 1.

@adrianosela adrianosela force-pushed the sig-algs-update branch 2 times, most recently from ddf8c35 to 5c1d633 Compare February 2, 2026 17:57
theodorsm
theodorsm requested changes Feb 2, 2026
View reviewed changes
Copy link
Copy Markdown
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.

Wow, thanks for the fast fix @adrianosela. I like the generic signature extension.

I went over it once more and noticed two things.

@adrianosela adrianosela force-pushed the sig-algs-update branch 2 times, most recently from 211fed9 to 8ec7ed9 Compare February 2, 2026 18:59
@adrianosela
Copy link
Copy Markdown
Contributor Author

adrianosela commented Feb 2, 2026

@theodorsm

  • signature_algotihms_cert is no longer included in a ServerHello: nice catch
  • we now do set the signaturesSchemes as fallback ( as in crypto/tls), followed your suggestion of removing the len check like we do with signatureSchemes

@adrianosela adrianosela requested a review from theodorsm February 2, 2026 19:03
theodorsm
theodorsm requested changes Feb 2, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@theodorsm theodorsm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am so sorry about the back-and-forth, but I had to take a bit longer time to reason about the handshake state machine. I added more comments.

Also, we should do as you did previously, with checking for the length of the provided CertificateSignatureSchemes and not fallback to signaturesSchemes in config. I looked at some captures from firefox and chrome and they do not add the signature_algorithms_cert by default. We have the same policy for both extensions:

Implementations which have the same policy in both cases MAY omit the "signature_algorithms_cert" extension.

@adrianosela adrianosela requested a review from theodorsm February 3, 2026 03:33
theodorsm
theodorsm approved these changes Feb 3, 2026
View reviewed changes
Copy link
Copy Markdown
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.

LGTM! Let's rebase and merge once the config options are merged.

@adrianosela
Copy link
Copy Markdown
Contributor Author

adrianosela commented Feb 4, 2026

I've rebased, squashed all commits, and added a functional config option.

I would like @JoTurk's blessing, or a new +1 from @theodorsm, before merging this.

JoTurk
JoTurk approved these changes Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
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.

code is great thank you.

@adrianosela adrianosela merged commit acad848 into pion:master Feb 4, 2026
19 checks passed
@adrianosela adrianosela deleted the sig-algs-update branch February 4, 2026 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoTurk JoTurk JoTurk approved these changes

@theodorsm theodorsm theodorsm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adrianosela @JoTurk @theodorsm