Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session's contexts can now be updated #148

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Antonito
Copy link
Member

This PR adds a UpdateContext to the session object, which allow to change the context of an already running SRTP / SRTCP session.

This is needed for DTLS restart, as mentioned in pion/webrtc#1636.

Apart from the UpdateContext, this PR changes two things:

  • remoteContext is now a atomic.Value: it previously had no locking mechanism, and none is required unless a context update is triggered.
    I did not benchmark anything but I assume an atomic Value will be far more efficient than a sync.RwLock in this specific case.

  • session.start no longer keeps its localContext if the creation of remoteContext fails

@codecov
Copy link

codecov bot commented Jun 26, 2021

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 76.71%. Comparing base (88859d2) to head (a8cd890).
Report is 78 commits behind head on master.

Files with missing lines Patch % Lines
session.go 76.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
+ Coverage   76.54%   76.71%   +0.17%     
==========================================
  Files          17       17              
  Lines        1232     1254      +22     
==========================================
+ Hits          943      962      +19     
- Misses        198      200       +2     
- Partials       91       92       +1     
Flag Coverage Δ
go 76.71% <80.00%> (+0.17%) ⬆️
wasm 76.23% <80.00%> (+0.18%) ⬆️

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.

session.go Outdated
@@ -106,17 +108,19 @@ func (s *session) close() error {
}

func (s *session) start(localMasterKey, localMasterSalt, remoteMasterKey, remoteMasterSalt []byte, profile ProtectionProfile, child streamSession) error {
var err error
s.localContext, err = CreateContext(localMasterKey, localMasterSalt, profile, s.localOptions...)
localContext, err := CreateContext(localMasterKey, localMasterSalt, profile, s.localOptions...)
Copy link
Member

Choose a reason for hiding this comment

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

can we call s.UpdateContext? Just brings down the duplicated code (hopefully)

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely right, plus it removes the need for the extra unit test

@Sean-Der
Copy link
Member

Do we also need to update the rolloverCounter and lastSequenceNumber ? I will find the code in libwebrtc

This is needed for DTLS restart, as mentioned in pion/webrtc#1636.
@Antonito
Copy link
Member Author

Antonito commented Mar 4, 2022

Do we also need to update the rolloverCounter and lastSequenceNumber ? I will find the code in libwebrtc

I decided to take a deep look at RFC 3711, and I don' t think the rolloverCounter should be updated:

*  a 32-bit unsigned rollover counter (ROC), which records how many
      times the 16-bit RTP sequence number has been reset to zero after
      passing through 65,535.  Unlike the sequence number (SEQ), which
      SRTP extracts from the RTP packet header, the ROC is maintained by
      SRTP as described in [Section 3.3.1](https://datatracker.ietf.org/doc/html/rfc3711#section-3.3.1).

      We define the index of the SRTP packet corresponding to a given
      ROC and RTP sequence number to be the 48-bit quantity

            i = 2^16 * ROC + SEQ.

About the lastSequenceNumber, I don't think we should update it either:

*  for the receiver only, a 16-bit sequence number s_l, which can be
      thought of as the highest received RTP sequence number (see
      [Section 3.3.1](https://datatracker.ietf.org/doc/html/rfc3711#section-3.3.1) for its handling), which SHOULD be authenticated
      since message authentication is RECOMMENDED,

There's a concept of re-keying (for instance if a key expire), during which both lastSequenceNumber and ROC have a role to play, but that's different from what we do here – we use the same key.

Note that if the master key is to be shared between SRTP streams
   within the same RTP session ([Section 9.1](https://datatracker.ietf.org/doc/html/rfc3711#section-9.1)), although the above bounds
   are on a per stream (i.e., per SSRC) basis, the sender MUST base re-
   key decision on the stream whose sequence number space is the first
   to be exhausted.

I'll try to dig a bit in libwebrtc, see how they handle all of this

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

Successfully merging this pull request may close these issues.

2 participants