Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3357 +/- ##
==========================================
+ Coverage 85.46% 85.71% +0.24%
==========================================
Files 81 82 +1
Lines 9738 9933 +195
==========================================
+ Hits 8323 8514 +191
Misses 998 998
- Partials 417 421 +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:
|
a7a931a to
1739a6b
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements support for RFC 9335 Cryptex (RTP Header Extension Encryption) in the pion/webrtc library, based on the W3C WebRTC Extensions specification. The implementation adds negotiation capabilities for encrypting RTP header extensions, with configurable policies that allow applications to require, negotiate, or disable this security feature.
Changes:
- Added
RTPHeaderEncryptionPolicyenum withNegotiateandRequireoptions for controlling encryption behavior - Added
CryptexModeenum and integrated Cryptex negotiation into SDP offer/answer exchange - Extended
Configuration,SettingEngine, andDTLSParametersto support Cryptex settings
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
rtpheaderencryptionpolicy.go |
New enum defining RTP header encryption policies with JSON marshaling support |
rtpheaderencryptionpolicy_test.go |
Unit tests for RTPHeaderEncryptionPolicy enum |
cryptexmode.go |
New enum for internal Cryptex mode state (Disabled/Enabled/Required) |
configuration.go |
Added RTPHeaderEncryptionPolicy field to Configuration struct |
settingengine.go |
Added DisableRTPHeaderEncryption method and field |
settingengine_js.go |
Added DisableRTPHeaderEncryption method for JS/WASM build |
peerconnection.go |
Integrated Cryptex negotiation into SDP generation and remote description processing |
peerconnection_js.go |
Minor formatting change |
peerconnection_test.go |
Added comment noting RTPHeaderEncryptionPolicy is not tested for WASM |
sdp.go |
Added cryptex attribute handling in SDP media sections |
sdp_test.go |
Added test for cryptex attribute in generated SDP |
rtptransceiver.go |
Added RtpHeaderEncryptionNegotiated method to query encryption status |
dtlsparameters.go |
Added CryptexMode field to DTLSParameters |
dtlstransport.go |
Integrated Cryptex mode into SRTP session configuration |
cryptex_negotiation_test.go |
Comprehensive integration tests for Cryptex negotiation scenarios |
errors.go |
Added error constants for Cryptex-related failures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1739a6b to
9b914ff
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'm going to review this soon, I think we should try to avoid adding new files to pion/webrtc, i think all the new files can be moved to already existing files :) |
7f850a1 to
aa23775
Compare
I have moved contents of these files elsewhere and removed them. |
aa23775 to
a05382f
Compare
2f8e5d6 to
9b1aba5
Compare
211a2c8 to
2cb8551
Compare
JoTurk
left a comment
There was a problem hiding this comment.
sorry this took time, it's mostly fine, it would be cooler if we could clean the negations / knob area so we don't carry multiple states and types, the mapping between srtpCryptexMode and RTPHeaderEncryptionPolicy could get cleaner, but it's not a blocker IMO.
it would be nice to add a clear test to guarantee that "negotiate" only turns on when actually negotiated.
I'll continue with the test part of the review tomorrow.
thank you. Sorry this took a while.
2cb8551 to
d19f1fe
Compare
|
@sirzooro I just noticed now, why we add and test for |
|
|
@sirzooro this doesn't sound right. per spec this rule only apply to rtp sections not applications.
https://www.rfc-editor.org/rfc/rfc9335.html#section-4 Maybe i missed other spec? |
You are right, I interpreted this incorrectly. I will fix this. |
d19f1fe to
3162c17
Compare
Fixed, |
8d50397 to
068594c
Compare
|
im sry i found few issues, i'll double check tomorrow morning before i comment. |
|
w3c/webrtc-extensions#47 (comment) -- currently pushing for some simplification of the API. I don't see why anyone would want the extra complication of not requiring cryptex but dealing with the result at a transceiver level. |
JoTurk
left a comment
There was a problem hiding this comment.
sorry for being late, more comments.
| func cryptexNegotiatedInSDP(desc *sdp.SessionDescription) (negotiatedForAnyMedia, negotiatedForAllMedia bool) { | ||
| negotiatedForAnyMedia = false | ||
| negotiatedForAllMedia = true | ||
| haveMedia := false | ||
|
|
||
| if _, hasCryptex := desc.Attribute(sdp.AttrKeyCryptex); hasCryptex { | ||
| return true, true | ||
| } | ||
|
|
||
| for _, media := range desc.MediaDescriptions { | ||
| if media.MediaName.Port.Value == 0 || media.MediaName.Media == mediaSectionApplication { | ||
| continue | ||
| } | ||
|
|
||
| haveMedia = true | ||
| hasCryptex := isCryptexSet(media) | ||
| negotiatedForAnyMedia = negotiatedForAnyMedia || hasCryptex | ||
| negotiatedForAllMedia = negotiatedForAllMedia && hasCryptex | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we're going with always negotiating at session-level, if yes, can we consider enforcing/normalizing to session-level only, and simplifying this helper?
There was a problem hiding this comment.
I changed this to make session-level preferred. Media-level may be needed as a fallback, so I kept support for it and updated code to respond with attribute at proper level (session or media).
| @@ -2955,6 +3049,11 @@ func (pc *PeerConnection) generateMatchedSDP( | |||
| isExtmapAllowMixed := isExtMapAllowMixedSet(remoteDescription.parsed) | |||
| localTransceivers := append([]*RTPTransceiver{}, transceivers...) | |||
|
|
|||
| cryptex := false | |||
| if pc.configuration.RTPHeaderEncryptionPolicy != RTPHeaderEncryptionPolicyDisable { | |||
| cryptex, _ = cryptexNegotiatedInSDP(remoteDescription.parsed) | |||
| } | |||
|
|
|||
| detectedPlanB := descriptionIsPlanB(remoteDescription, pc.log) | |||
| if pc.configuration.SDPSemantics != SDPSemanticsUnifiedPlan { | |||
| detectedPlanB = descriptionPossiblyPlanB(remoteDescription) | |||
| @@ -2969,7 +3068,10 @@ func (pc *PeerConnection) generateMatchedSDP( | |||
| } | |||
|
|
|||
| if media.MediaName.Media == mediaSectionApplication { | |||
| mediaSections = append(mediaSections, mediaSection{id: midValue, data: true}) | |||
| mediaSections = append(mediaSections, mediaSection{ | |||
| id: midValue, | |||
| data: true, | |||
| }) | |||
| alreadyHaveApplicationMediaSection = true | |||
|
|
|||
| continue | |||
| @@ -3010,7 +3112,11 @@ func (pc *PeerConnection) generateMatchedSDP( | |||
| } | |||
| mediaTransceivers = append(mediaTransceivers, transceiver) | |||
| } | |||
| mediaSections = append(mediaSections, mediaSection{id: midValue, transceivers: mediaTransceivers}) | |||
| mediaSections = append(mediaSections, mediaSection{ | |||
| id: midValue, | |||
| transceivers: mediaTransceivers, | |||
| cryptex: cryptex, | |||
| }) | |||
| case sdpSemantics == SDPSemanticsUnifiedPlan || sdpSemantics == SDPSemanticsUnifiedPlanWithFallback: | |||
There was a problem hiding this comment.
Does this mean we will never offer cryptex again on later renegotiations if the first negotiation doesn't echo cryptex?
It's fine if we don't support cryptex for renegotiations, but i feel this should be explicit somewhere.
There was a problem hiding this comment.
Yes, I fixed this. Unfortunately now it is not possible to update SRTP context on the fly or recreate it, so renegotiation actually cannot change cryptex mode. I also added extra check for this.
There was a problem hiding this comment.
Interesting point! The RFC only says
Generating subsequent SDP offers and answers MUST use the same procedures for including the "a=cryptex" attribute as the ones on the initial offer and answer.
but this is generation, not handling of subsequent answers. The problem is if you do this:
O: cryptex
A: no
(create SRTP sessions)
O: cryptex
A: cryptex
(create more SRTP sessions)
you end up with a weird mix of cryptex and non-cryptex. Technically this is valid and distinguishable and... you are asking for trouble. And the require policy should prevent this.
There was a problem hiding this comment.
When cryptex attribute is at session level, after renegotiation like in this example old SRTP sessions should start using cryptex too. I am not sure why someone would want to do this. Plus this is a potential attack vector, to trick peer to downgrade its security a bit for existing sessions by disabling header encryption (require policy prevents this). So even for negotiate policy it makes sense to prevent renegotiation of cryptex.
There was a problem hiding this comment.
Good point - @juberti since I see anything in the RFC is it worth pointing it out as an errata?
There was a problem hiding this comment.
Not sure if I understood the whole issue, but note that cryptex attribute only indicate the capability to receive and process cryptex packets. Sending side behavior is not controlled by SDP signaling but by app APIs (negotiate, require).
It could be possible for a forwarding proxy/gateway to renegotiate the SDP to modify the upstream destination server, and change the cryptex flag. It makes not sense for a terminating RTP endpoint to modify the cryptex flag between negotiation.
Also note that cryptex may not be bidirectional, if offerer (A) doesn't add the cryptex attribute in the SDP offer and answerer (B) doesn't add the attribute in the answer SDP, B could still send cryptex packets to A.
Even further, even if both A and B support cryptex in the SDP O/A, they could still send each other plain rtp packet.
It is up to local policy (i.e. APIs) to decide what to do when the other side does not support receiving cryptex or what to do with non-cryptex rtp packet is received.
There was a problem hiding this comment.
@murillo128 thank you, I missed this is declarative (or did not want to see?)
The W3C API controls negotiation but does that mean it is also missing a way to control and determine whether encryption is used? I would strongly prefer not to because this will be a headache.
There was a problem hiding this comment.
I would neither like an API for wether encryption is used on the sender side if the received supports it. I think sender should always use it in that case.
There was a problem hiding this comment.
@murillo128 made a great point in https://mailarchive.ietf.org/arch/msg/avt/pGnete_6IqExxTERNl44_9GqVRU/ which probably requires W3C API changes.
The most pragmatic thing here is to ship cryptex with the two policy options disabled and negotiate for the time being and add "require" with a new definition of "mandatory to negotiate and use" later which shouldn't require many changes to the PR.
There was a problem hiding this comment.
are these the right definitions?
disabled: don't offer a=cryptex, ignore incoming cryptex, and don't send cryptex even if the remote side supports it in SDPnegotiate: offer a=cryptex, handle incoming cryptex (or not), and send cryptex if the remote side supports it in SDP (as usual)require: offer a=cryptex, fail on incoming non-cryptex, and fail if the remote side doesn't support cryptex in SDP
4e0a8ad to
4824643
Compare
This is final part of Cryptex (RFC 9335) implementation for pion/webrtc. Public API is based on WebRTC Extensions specification (W3C Editor's Draft from 19 December 2025).
4824643 to
d7eb692
Compare
This is final part of Cryptex (RFC 9335) implementation for pion/webrtc. Public API is based on WebRTC Extensions specification (W3C Editor's Draft from 19 December 2025).