Expose EncryptRTP API through SRTP

[kevmo314]

Description

Exposes EncryptRTP() to allow users to manually encrypt packets.

See pion/ice#863.

Reference issue

Fixes #...

[kevmo314]

This assertion is unfortunate but since the underlying connection here is a net.Conn there's no strict requirement to actually support the same API.

[kevmo314]

On the other hand, this change also makes this API unintentionally backwards-compatible so we don't actually need to bump pion/ice and it will get picked up when the parent updates the dep version...

[codecov]

Codecov Report

❌ Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.55%. Comparing base (030374f) to head (dbb7282).

Files with missing lines	Patch %	Lines
session_srtp.go	44.44%	4 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (44.44%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #360      +/-   ##
==========================================
- Coverage   82.63%   82.55%   -0.08%     
==========================================
  Files          19       19              
  Lines        1457     1462       +5     
==========================================
+ Hits         1204     1207       +3     
- Misses        140      141       +1     
- Partials      113      114       +1     

Flag	Coverage Δ
go	82.55% <44.44%> (-0.08%)	⬇️

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.

[JoTurk]

@kevmo314 maybe we have to think about also datachannels, and adding batching in the future, maybe it should be some sort of conn that works with srtp, sctp without any change, Maybe I'm missing something, but I just don't want us to maintain multiple write paths, especially if we're going to add batch writer soon.

[kevmo314]

@kevmo314 maybe we have to think about also datachannels, and adding batching in the future, maybe it should be some sort of conn that works with srtp, sctp without any change, Maybe I'm missing something, but I just don't want us to maintain multiple write paths, especially if we're going to add batch writer soon.

If we do not want ICE semantics to leak into SRTP, then SRTP cannot know about the existence of multiple paths since the net.Conn only exposes a single io.Writer destination. I'm not convinced of the value of creating a pion-specific Conn that exposes that information since that would be ICE leaking semantics through this new Conn type?

Exposing a separate encrypt API therefore seems reasonable, as it's signalling that "I want this packet processed but I will manage the sending myself". That fits in with the batching and datachannels context you raised as well, since it is up to the caller to get the packet to the destination and the caller is choosing to opt out of those features, which seems reasonable.

The crux of the issue is the necessary statefulness of SRTP though:

srtp/srtp.go

Line 146 in 030374f

s.updateRolloverCount(header.SequenceNumber, diff, false, 0)

I believe all the write paths must share the same encryption context otherwise the receiver will not be able to decrypt the packets correctly?