Issue 2761143002: Support encrypted RTP extensions (RFC 6904)

joachim

jbauch@webrtc.org changed reviewers: + pthatcher@webrtc.org

3 years, 9 months ago (2017-03-21 00:18:26 UTC) #1

joachim

I finally had some time to continue working on this. Tested compatibility between Chrome 57 ...

3 years, 9 months ago (2017-03-21 00:18:27 UTC) #2

pthatcher1

pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org

3 years, 9 months ago (2017-03-21 07:07:05 UTC) #3

pthatcher1

I'm sure Taylor can give this a more thorough review than I can. https://codereview.webrtc.org/2761143002/diff/1/webrtc/api/peerconnectioninterface.h File ...

3 years, 9 months ago (2017-03-21 07:07:07 UTC) #4

Taylor Brandstetter

Mostly looks good. Most of my concerns are just around the BaseChannel code. https://codereview.webrtc.org/2761143002/diff/1/webrtc/api/peerconnectioninterface.h File ...

3 years, 9 months ago (2017-03-22 18:00:12 UTC) #5

joachim

Description was changed from ========== Support encrypted RTP extensions (RFC 6904) Can be enabled by ...

3 years, 9 months ago (2017-03-23 00:03:07 UTC) #6

joachim

Thank you both for the detailed feedback! Uploaded a new version that addresses all comments ...

3 years, 9 months ago (2017-03-23 00:04:34 UTC) #7

Thank you both for the detailed feedback! Uploaded a new version that addresses
all comments and simplifies the code in a couple of areas.

Ptal

https://codereview.webrtc.org/2761143002/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:876: // will be negotiated. They will be
only used if both peers support ihem.
On 2017/03/21 07:07:06, pthatcher1 wrote:
> ihem => them

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:877: bool enable_rtp_header_encryption =
false;
On 2017/03/21 07:07:06, pthatcher1 wrote:
> I think enable_encrypted_rtp_header_extensions would be slightly better.

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:877: bool enable_rtp_header_encryption =
false;
On 2017/03/22 18:00:10, Taylor Brandstetter wrote:
> Is there a reason this is added here, and not in RTCConfiguration? I think
> that's what we prefer by default.

I assumed that it would more be a general option like "ssl_max_version" or
"crypto_options" which will not be different per PeerConnection.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/config.cc#newcode97
webrtc/config.cc:97: bool RtpExtension::AllowEncrypt(const std::string& uri) {
On 2017/03/21 07:07:06, pthatcher1 wrote:
> A more consistent name would be "IsEncryptionSupported"

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/config.h#newcode71
webrtc/config.h:71: static bool AllowEncrypt(const std::string& uri);
On 2017/03/22 18:00:10, Taylor Brandstetter wrote:
> Can you leave a comment describing this method?

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/config.h#newcode112
webrtc/config.h:112: bool encrypted = false;
On 2017/03/22 18:00:10, Taylor Brandstetter wrote:
> nit: If this was just "encrypt" rather than "encrypted", it would match the
> naming of the ORTC "RtpHeaderExtensionParameters", and the string "encrypt" in
> the URI.

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/media/base/fakertp.h
File webrtc/media/base/fakertp.h (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/media/base/fakertp.h#n...
webrtc/media/base/fakertp.h:46: static const unsigned char
kPcmuWithExtensionsFrame[] = {
On 2017/03/22 18:00:10, Taylor Brandstetter wrote:
> nit: kPcmuFrameWithExtensions?

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/media/engine/webrtcmed...
File webrtc/media/engine/webrtcmediaengine.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/media/engine/webrtcmed...
webrtc/media/engine/webrtcmediaengine.cc:135: int id_used[14] = {false};
On 2017/03/21 07:07:06, pthatcher1 wrote:
> Why was this changed?

This was a leftover from some earlier testing code I missed. Reverted, thanks
for spotting.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode329
webrtc/pc/channel.cc:329: rtp_encrypted_headers_ready_ = 0;
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> This would mean that a new offer/answer is needed if the transport changes
after
> SRTP is already set up? May be a corner case, but that seems incorrect.

Right, I missed this. No longer applies with the change to use
"SetRtpTransportParameters" for setting up the encrypted headers.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode330
webrtc/pc/channel.cc:330: }
On 2017/03/21 07:07:06, pthatcher1 wrote:
> It shouldn't become writable again until we get the local and remote contents,
> so I don't understand why we need to carry around this extra state.

The extra state has now been removed.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode999
webrtc/pc/channel.cc:999: MaybeSetupDtlsSrtp_n();
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> On 2017/03/21 07:07:06, pthatcher1 wrote:
> > Why did this have to change?  
> 
> Because ShouldSetupDtlsSrtp_n now checks it. May want to mention in a comment,
> if it doesn't end up changing.

Right, added a comment for now.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1121
webrtc/pc/channel.cc:1121: }
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> It looks like you're taking the intersection of rtp_local_encrypted_headers
and
> rtp_remote_encrypted_headers twice? The only thing that may change is the
order,
> but it doesn't seem like that should make a difference.

The ids of the extensions could be different for the local/remote side. That's
why both lists must be filtered separately.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1121
webrtc/pc/channel.cc:1121: }
On 2017/03/21 07:07:06, pthatcher1 wrote:
> Why not just pass in all header extensions and let srtp_filter_ check the
> .encrypted bit?  Seems a lot more simple.

The lists already contain only encrypted extensions. This code does the
negotiation and determines which extensions actually will be encrypted for
sending/receiving.

Moved to "SrtpFilter::GetSendRecvEncryptedHeaderExtensions" now.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1150
webrtc/pc/channel.cc:1150: return false;
On 2017/03/21 07:07:06, pthatcher1 wrote:
> It seems like it would be more clear to put this into ShouldSetupDtlsSrtp_n()
> then to put it directly here, especially since that also affects
> IsReadyToSendMedia_n.

Code has been removed in latest update.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1530
webrtc/pc/channel.cc:1530: encrypted_headers));
On 2017/03/22 18:00:10, Taylor Brandstetter wrote:
> Is there a reason why PrepareHeaderExtensions_w is called with all extensions,
> but PrepareHeaderExtensions_n is only called with encrypted extensions?

I wanted to do the actual "work" (i.e. filtering the extensions) on the worker
thread and just store the value on the network thread.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1538
webrtc/pc/channel.cc:1538: void
BaseChannel::PrepareHeaderExtensions_n(ContentSource source,
On 2017/03/22 18:00:10, Taylor Brandstetter wrote:
> Can this be done in SetRtpTransportParameters_n, to avoid the extra thread
hop?

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1542
webrtc/pc/channel.cc:1542: rtp_encrypted_headers_ready_ |=
kLocalEncryptedHeadersReady;
On 2017/03/21 07:07:06, pthatcher1 wrote:
> I don't understand why we need this extra state.    This is basically just
> tracking if we've received a local and remote description, right?  But we
won't
> become writable and get SRTP keys to extract before we have a local and remote
> description.  So why do we need yet more state to track that?

Right, and no longer needed with the latest changes.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1547
webrtc/pc/channel.cc:1547: if (MaybeSetupDtlsSrtp_n()) {
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> If the encrypted headers are only set when SRTP is first set up, that means if
> new ones are added in a subsequent offer/answer, they're ignored. May be a
> corner case, but that seems incorrect.
> 
> Would it be possible to use srtp_update to update the header extensions
whenever
> they change? That would also simplify some things in BaseChannel I think (no
> longer need to wait for extensions to be set before setting up SRTP).

With the change to set the encrypted headers through
"SetRtpTransportParameters", this probably no longer applies. Or am I missing
something?

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1548
webrtc/pc/channel.cc:1548: UpdateMediaSendRecvState();
On 2017/03/21 07:07:06, pthatcher1 wrote:
> This should only be necessary if IsReadyToSendMedia_n() changes.  For audio, I
> believe it will be a no-op if it doesn't:
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvo...
> 
> Video seems to have an effect even if it doesn't change, but that's probably
> incorrect and shouldn't be relied on:
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi...

This is no longer needed with the latest changes.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.h#newcode346
webrtc/pc/channel.h:346: const std::vector<webrtc::RtpExtension>& extensions);
On 2017/03/21 07:07:06, pthatcher1 wrote:
> void PrepareRtpHeaderExtensions_w would be a better name.
> 
> Also, prepare them for what?  This could use a comment.

Method has been removed.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.h#newcode439
webrtc/pc/channel.h:439: int rtp_encrypted_headers_ready_ =
kNoEncryptedHeadersReady;
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> Can this extra state be derived from existing information? In which case it
> could just be replaced by a helper method?

I completely removed the extra state.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel_unittest.cc
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel_unittest.cc...
webrtc/pc/channel_unittest.cc:1362: WaitForThreads();
On 2017/03/21 07:07:06, pthatcher1 wrote:
> This could use a comment explaining why it's needed.

This is no longer needed now.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/mediasession.cc#new...
webrtc/pc/mediasession.cc:948: static bool FindByUriPreferEncrypted(const
RtpHeaderExtensions& extensions,
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> nit: This name sounds like it always prefers encrypted extensions. Something
> like "FindByUriWithEncryptionPreference" may be better.

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/mediasession.cc#new...
webrtc/pc/mediasession.cc:1014: UsedRtpHeaderExtensionIds* used_ids) {
On 2017/03/21 07:07:06, pthatcher1 wrote:
> This should probably be called something like AddEncryptedVersionsOfHdrExts

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/mediasession.cc#new...
webrtc/pc/mediasession.cc:1025: encrypted_extensions.push_back(encrypted);
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> Is there a reason the separate "encrypted_extensions" list is used, rather
than
> calling push_back in "extensions" directly?

That could invalidate the iterator for "extensions" if the underlying vector is
getting reallocated.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/mediasession.cc#new...
webrtc/pc/mediasession.cc:1658: AddEncryptedHdrExts(video_extensions,
&used_ids);
On 2017/03/21 07:07:06, pthatcher1 wrote:
> Why are we storing two copies of the header extension?  Why not instead simply
> have the one copy with a field that is tri-state for encryption:  enabled
(like
> both), disabled (like only the first), and required (like only the second)? 
It
> seems like it would be more simple.

The encrypted extensions are using a different id, so that value would also need
to be stored in the RtpExtension class. I don't think that would make it simpler
than having the encrypted extension as a second entry in the list.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/mediasession.cc#new...
webrtc/pc/mediasession.cc:1658: AddEncryptedHdrExts(video_extensions,
&used_ids);
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> On 2017/03/21 07:07:06, pthatcher1 wrote:
> > Why are we storing two copies of the header extension?  Why not instead
simply
> > have the one copy with a field that is tri-state for encryption:  enabled
> (like
> > both), disabled (like only the first), and required (like only the second)? 
> It
> > seems like it would be more simple.
> 
> I don't have a problem with storing two copies. That maps 1:1 with everything
in
> the API and SDP, and doesn't require defining a new structure.
> 
> But I think it would be simpler if the encrypted versions were added at
> construction time, right after "GetSupportedAudioRtpHeaderExtensions" is
called.
> Then the extra method "AddEncryptedHdrExts" shouldn't be necessary;
> FindRtpHdrExtsToOffer can simply handle both.

I think this would just move the complexity to "FindRtpHdrExtsToOffer" which
currently only searches for extensions by URI ("FindByUri"). The URI is the same
for the regular and the encrypted extension, so it would need special handling
to deal with that.

IMHO the code is more cleaner as-is, but if you want, I will look into
refactoring "FindRtpHdrExtsToOffer" to support this use case.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/mediasession.h
File webrtc/pc/mediasession.h (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/mediasession.h#newc...
webrtc/pc/mediasession.h:570: bool enable_rtp_header_encryption_ = false;
On 2017/03/21 07:07:06, pthatcher1 wrote:
> "encrypted_rtp_extensions_" to match existing names might be better.

The "XXX_rtp_extensions_" members are lists of extensions (e.g.
"audio_rtp_extensions_") where the new member is a flag.

If you want to use a different name, what about
"enable_encrypted_rtp_extensions_"?

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:422: std::vector<int> encrypted_headers;  // No
encrypted headers in this case.
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> Use the pattern of "no_encrypted_headers" here as well?
> 
> Also, if we don't support encrypted header extensions with SDES, that should
be
> documented somewhere, and we should make sure it doesn't result in them being
> sent encrypted, but rather not sent at all.

I now also added support for encrypted header extensions with SDES.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:738: if (!encrypted_headers.empty()) {
On 2017/03/21 07:07:06, pthatcher1 wrote:
> Please stick with "_header_extensions" or "hdr_exts" instead of just
"headers".

Done (here and in various other places).

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter.h
File webrtc/pc/srtpfilter.h (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter.h#newcode82
webrtc/pc/srtpfilter.h:82: // Used with DTLS-SRTP.
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> nit: Update the comments in this file to describe the new parameters.

Functionality has moved to a new method, I added a comment there.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter.h#newcod...
webrtc/pc/srtpfilter.h:250: bool SetKey(int type, int cs, const uint8_t* key,
size_t len,
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> SetKeyAndEncryptedHeaders?

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter_unittest.cc
File webrtc/pc/srtpfilter_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:200: // This expects both packets to be based
on kPcmuWithExtensionsFrame.
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> nit: Comment may look better above method rather than inside it.

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:224: // of header extensions to encrypt.
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> So, unencrypted extensions are always expected to be equal, but encrypted ones
> only are if "expect_equal" is true? Could you summarize this in a comment
above
> this method?

Done.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/webrtcsdp_unittest.cc
File webrtc/pc/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/webrtcsdp_unittest....
webrtc/pc/webrtcsdp_unittest.cc:2178: EXPECT_EQ(sdp_with_extmap, message);
On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> nit: Tests for SDP string equality are fragile since they rely on a specific
> ordering of attributes. Would be better (in my opinion) if the test for
> serialization simply serialized and deserialized the description and verified
> that they're the same, relying on the tests for deserialization to ensure that
> half already works. See the comment above "TestSerialize".

I based my test on "SerializeSessionDescriptionWithExtmap" just above which also
does the string comparison. Changed the new test now.

Taylor Brandstetter

New patchset looks great. I think the only remaining thing to resolve is what happens ...

3 years, 9 months ago (2017-03-23 20:10:57 UTC) #8

New patchset looks great. I think the only remaining thing to resolve is what
happens in situations with multiple offer/answer exchanges. Though if it ends up
being nontrivial to resolve, we could leave it as a bug.

Also, do you plan to make a separate CL for handling this at the media engine
level? I doubt it behaves correctly by default if it sees the same URI twice; it
looks like it takes the first one in the list:
https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcme...

And it will currently use any supported extensions present in the remote
description, even if not present in the local description, whereas the code in
SrtpFilter takes the intersection... Meaning you could run into a situation
where it's supposed to be encrypted but isn't.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1121
webrtc/pc/channel.cc:1121: }
On 2017/03/23 00:04:33, joachim wrote:
> On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> > It looks like you're taking the intersection of rtp_local_encrypted_headers
> and
> > rtp_remote_encrypted_headers twice? The only thing that may change is the
> order,
> > but it doesn't seem like that should make a difference.
> 
> The ids of the extensions could be different for the local/remote side. That's
> why both lists must be filtered separately.

Ah, right. May be helpful to mention this in a comment.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1547
webrtc/pc/channel.cc:1547: if (MaybeSetupDtlsSrtp_n()) {
On 2017/03/23 00:04:33, joachim wrote:
> On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> > If the encrypted headers are only set when SRTP is first set up, that means
if
> > new ones are added in a subsequent offer/answer, they're ignored. May be a
> > corner case, but that seems incorrect.
> > 
> > Would it be possible to use srtp_update to update the header extensions
> whenever
> > they change? That would also simplify some things in BaseChannel I think (no
> > longer need to wait for extensions to be set before setting up SRTP).
> 
> With the change to set the encrypted headers through
> "SetRtpTransportParameters", this probably no longer applies. Or am I missing
> something?

Even though they're set through SetRtpTransportParameters, they're only used in
SrtpFilter::SetRtpParams. So, it's corner case territory, but suppose a
subsequent offer adds a new encrypted extension. How does the SRTP session get
updated?

There's also the situation where an initial offer/answer negotiates an
audio-only m= section, and then a video m= section is negotiated later. I
believe this would result in SetRtpParams being called as soon as the offer is
applied, before the information from the header extensions in the answer is
available.

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/config.cc#newcode111
webrtc/config.cc:111: if (extension.uri == uri)
nit: Our style is to use {}s even for single-line if statements

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1149: LOG(LS_INFO) << "Using " << src << " encrypted
extension: "
nit: "src" will print as "0" or "1" (I believe), which isn't ideal. Maybe add a
stream insertion operator, or just do:

src == CS_LOCAL ? "local" : "remote"

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1017: if (extension.encrypt ||
Could you leave a comment explaining why "extension.encrypt" is checked? I
assume it's because this ensures that the encrypted extension isn't added twice
for a subsequent offer.

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1024: used_ids->FindAndSetIdUsed(&encrypted);
If the same encrypted extension is used for audio and video, won't this result
in it using different IDs? This could be solved by doing the same thing as in
FindRtpHdrExtsToOffer, using the "all_extensions" list.

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/srtpfilter.cc#n...
webrtc/pc/srtpfilter.cc:281: const std::vector<webrtc::RtpExtension>&
extensions) {
The situation that's not being handled right now is when this is called while
"IsActive" is already true.

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/webrtcsdp_unitt...
File webrtc/pc/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/webrtcsdp_unitt...
webrtc/pc/webrtcsdp_unittest.cc:2170: std::string message =
webrtc::SdpSerialize(desc_with_extmap, false);
I think you could just call "TestSerialize" here.

joachim

Description was changed from ========== Support encrypted RTP extensions (RFC 6904) Can be enabled by ...

3 years, 8 months ago (2017-03-30 22:35:12 UTC) #9

joachim

Here is an updated version that now supports adding/changing encrypted extensions when receiving additional offers/answers ...

3 years, 8 months ago (2017-03-30 22:43:49 UTC) #10

Here is an updated version that now supports adding/changing encrypted
extensions when receiving additional offers/answers (test in
channel_unittest.cc).

Regarding selecting the encrypted extensions: I think this is covered by the
changes to "webrtc/media/engine/webrtcmediaengine.cc" which prefer encrypted
extensions.

Only feature missing from what I can tell right now is support for adding
encrypted extensions when sending additional offers/answers (added a TODO). I
will think about that more the next days, but wanted to get your feedback on the
other changes, too.

While working on this I found an issue in upstream libsrtp when GCM ciphers are
used with encrypted extensions. Reported as
https://github.com/cisco/libsrtp/issues/286 with a potential fix in
https://github.com/cisco/libsrtp/pull/287

Once accepted upstream, I will backport it to the libsrtp used by WebRTC. This
is needed before this CL can land (causes msan errors otherwise).

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1121
webrtc/pc/channel.cc:1121: }
On 2017/03/23 20:10:56, Taylor Brandstetter wrote:
> On 2017/03/23 00:04:33, joachim wrote:
> > On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> > > It looks like you're taking the intersection of
rtp_local_encrypted_headers
> > and
> > > rtp_remote_encrypted_headers twice? The only thing that may change is the
> > order,
> > > but it doesn't seem like that should make a difference.
> > 
> > The ids of the extensions could be different for the local/remote side.
That's
> > why both lists must be filtered separately.
> 
> Ah, right. May be helpful to mention this in a comment.

Done (in "SrtpFilter::GetSendRecvEncryptedHeaderExtensions").

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1547
webrtc/pc/channel.cc:1547: if (MaybeSetupDtlsSrtp_n()) {
On 2017/03/23 20:10:56, Taylor Brandstetter wrote:
> On 2017/03/23 00:04:33, joachim wrote:
> > On 2017/03/22 18:00:11, Taylor Brandstetter wrote:
> > > If the encrypted headers are only set when SRTP is first set up, that
means
> if
> > > new ones are added in a subsequent offer/answer, they're ignored. May be a
> > > corner case, but that seems incorrect.
> > > 
> > > Would it be possible to use srtp_update to update the header extensions
> > whenever
> > > they change? That would also simplify some things in BaseChannel I think
(no
> > > longer need to wait for extensions to be set before setting up SRTP).
> > 
> > With the change to set the encrypted headers through
> > "SetRtpTransportParameters", this probably no longer applies. Or am I
missing
> > something?
> 
> Even though they're set through SetRtpTransportParameters, they're only used
in
> SrtpFilter::SetRtpParams. So, it's corner case territory, but suppose a
> subsequent offer adds a new encrypted extension. How does the SRTP session get
> updated?
> 
> There's also the situation where an initial offer/answer negotiates an
> audio-only m= section, and then a video m= section is negotiated later. I
> believe this would result in SetRtpParams being called as soon as the offer is
> applied, before the information from the header extensions in the answer is
> available.

Adding/changing encrypted extensions is now supported with the latest upload.

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/config.cc#newcode111
webrtc/config.cc:111: if (extension.uri == uri)
On 2017/03/23 20:10:56, Taylor Brandstetter wrote:
> nit: Our style is to use {}s even for single-line if statements

Done (mine too, looks like I missed this after moving the function from
channel.cc).

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1149: LOG(LS_INFO) << "Using " << src << " encrypted
extension: "
On 2017/03/23 20:10:56, Taylor Brandstetter wrote:
> nit: "src" will print as "0" or "1" (I believe), which isn't ideal. Maybe add
a
> stream insertion operator, or just do:
> 
> src == CS_LOCAL ? "local" : "remote"

Done.

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1017: if (extension.encrypt ||
On 2017/03/23 20:10:56, Taylor Brandstetter wrote:
> Could you leave a comment explaining why "extension.encrypt" is checked? I
> assume it's because this ensures that the encrypted extension isn't added
twice
> for a subsequent offer.

Done and also updated the condition.

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1024: used_ids->FindAndSetIdUsed(&encrypted);
On 2017/03/23 20:10:56, Taylor Brandstetter wrote:
> If the same encrypted extension is used for audio and video, won't this result
> in it using different IDs? This could be solved by doing the same thing as in
> FindRtpHdrExtsToOffer, using the "all_extensions" list.

Done. Also added a test for encrypted id reuse and offer update
("MediaSessionDescriptionFactoryTest::RtpExtensionIdReusedEncrypted").

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/srtpfilter.cc#n...
webrtc/pc/srtpfilter.cc:281: const std::vector<webrtc::RtpExtension>&
extensions) {
On 2017/03/23 20:10:56, Taylor Brandstetter wrote:
> The situation that's not being handled right now is when this is called while
> "IsActive" is already true.

This is now supported. Encrypted header extensions can be changed.

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/webrtcsdp_unitt...
File webrtc/pc/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/20001/webrtc/pc/webrtcsdp_unitt...
webrtc/pc/webrtcsdp_unittest.cc:2170: std::string message =
webrtc::SdpSerialize(desc_with_extmap, false);
On 2017/03/23 20:10:56, Taylor Brandstetter wrote:
> I think you could just call "TestSerialize" here.

Done.

Taylor Brandstetter

I still don't think the offer/answer logic handles all cases. Let's take a step back ...

3 years, 8 months ago (2017-04-01 00:28:59 UTC) #11

I still don't think the offer/answer logic handles all cases. Let's take a step
back for a second:

The SRTP session is created as soon as the DTLS handshake completes. Or, when an
"m=" section is "bundled" onto another "m=" section whose DTLS handshake already
completed. Due to this, the SRTP session can be created *at any point* in the
offer/answer exchange.

As soon as a local description is set, the SRTP session should prepare to
decrypt extensions (if it's already created). This should happen even if the
local description is an offer, because RTP packets may be received before the
answer is received.

And as soon as a remote description is set, the SRTP session should prepare to
encrypt extensions (if it's already created). This should happen even if the
remote description is an offer, because RTP packets that use those extensions
may be sent before the corresponding answer is created, in a situation that
involves multiple offers/answers; not sure if this is actually right, but it's
our current implementation.

So, the "srtp_update_on_answer_" logic seems wrong, because the update needs to
happen when the offer is set.

Also, the intersection-taking (of GetSendRecvEncryptedHeaderExtensions) seems
wrong; if an extension only appears in one description, that doesn't prevent it
from being sent/received in one direction. In other words, like codecs,
extensions are asymmetrical (or at least, that's our current implementation).
You may have to munge SDP to get this (or receive an answer from a non-WebRTC
endpoint), but it's possible.

If you believe my interpretation of the SDP rules is wrong, let me know, because
it very well may be. RFC5285 seems to leave some things open to interpretation,
so I'm largely going off how our code currently works.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1530
webrtc/pc/channel.cc:1530: encrypted_headers));
On 2017/03/23 00:04:32, joachim wrote:
> On 2017/03/22 18:00:10, Taylor Brandstetter wrote:
> > Is there a reason why PrepareHeaderExtensions_w is called with all
extensions,
> > but PrepareHeaderExtensions_n is only called with encrypted extensions?
> 
> I wanted to do the actual "work" (i.e. filtering the extensions) on the worker
> thread and just store the value on the network thread.

In this case the work is pretty trivial, so I wouldn't worry about it. But as
long as the variables are named clearly, I guess it doesn't hurt.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config.cc#newcode149
webrtc/config.cc:149: }
nit: This code could probably be simplified by using "std::find_if", and/or
doing two passes through the list. Pass 1 = encrypted extensions, pass 2 =
unencrypted. Though it doesn't really matter, this code is clear enough to me.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config_unittest.cc
File webrtc/config_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config_unittest.cc...
webrtc/config_unittest.cc:28: static std::vector<RtpExtension> MakeVector(const
RtpExtension& extension) {
nit: Would it be possible to use the vector constructor that takes an
initializer list, making this unnecessary?

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config_unittest.cc...
webrtc/config_unittest.cc:35: TEST_F(ConfigTest,
TestRtpExtension_FilterDuplicateNonEncrypted) {
nit: I'd change this to "TEST(RtpExtensionTest, FilterDuplicateNonEncrypted)".

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcmediaengine_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcmediaengine_unittest.cc:150: extensions[3].encrypt =
true;
nit: It's a little hard to read this test, since you need to look at what
MakeRedundantExtensions does and figure out what the extensions at indices 2 and
3 are.

Also: unless I'm mistaken, this would result in two extensions having the same
ID, which isn't legal (would be feasible for it to be DCHECKed).

I'd prefer if the test explicitly constructed the list, like the test below
does. The list doesn't need 7 entries; I think just three would be sufficient to
cover the specific functionality being tested in this test.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcmediaengine_unittest.cc:181:
RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3, true));
They both have the same ID; is that intentional?

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:122: const RtpHeaderExtensions& extensions,
I'd just set params->extensions outside of this method, then there'd be no
reason to pass the extensions into it.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:141: const RtpHeaderExtensions& extensions,
Same here.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1084: if (!dtls_keyed_) {
I don't think this works in all cases... If the transport changes, the
SrtpFilter will be reset, but dtls_keyed_ won't be. So, should just check
"srtp_filter_.IsActive()" here.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1101: ret = srtp_filter_.UpdateRtpParams(
If UpdateRtpParams is *always* called with the same keying information (right
now), I'd suggest just removing the arguments for simplicity.

In fact: is there a reason why this couldn't just happen automatically inside
"SetEncryptedHeaderExtensions"?

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1236: srtp_update_on_answer_ = true;
I'm not sure this variable buys anything; an answer will always be set after an
offer, so won't it always be true when it's checked below?

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:915: bool expect_equal) {
Since this is only called with "false" in this file, could remove the parameter.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1069: }
Once we decide how to resolve the other comments: it would be good to have a
version of this test that covered the DTLS code path, and simulated DTLS
completing at different points in the offer/answer exchange.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1050: encrypted_extensions.end());
I still think this code could be simplified... But I'm ok leaving that as
homework for myself. It was already complex before this CL, so it's not your
fault.

joachim

Sorry for the slow response, I had too much other (personal) stuff to look into ...

3 years, 8 months ago (2017-04-17 10:46:10 UTC) #12

Sorry for the slow response, I had too much other (personal) stuff to look into
recently.

Uploaded a new version that removes the intersection in the srtpfilter and so
leaves the negotiation to the higher levels of WebRTC. Also the filter is
updated on each OFFER/ANSWER now instead of after the ANSWER has been received
which I assume should handle the case you describe.

Again thanks for your detailed review, I changed a couple of other areas based
on your feedback.

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/1/webrtc/pc/channel.cc#newcode1530
webrtc/pc/channel.cc:1530: encrypted_headers));
On 2017/04/01 00:28:58, Taylor Brandstetter wrote:
> On 2017/03/23 00:04:32, joachim wrote:
> > On 2017/03/22 18:00:10, Taylor Brandstetter wrote:
> > > Is there a reason why PrepareHeaderExtensions_w is called with all
> extensions,
> > > but PrepareHeaderExtensions_n is only called with encrypted extensions?
> > 
> > I wanted to do the actual "work" (i.e. filtering the extensions) on the
worker
> > thread and just store the value on the network thread.
> 
> In this case the work is pretty trivial, so I wouldn't worry about it. But as
> long as the variables are named clearly, I guess it doesn't hurt.

Ok, the whole separated "preparing" code has been moved to
SetRtpTransportParameters" with clearly named variables ("extensions" and
"encrypted_extension_ids").

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config.cc#newcode149
webrtc/config.cc:149: }
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> nit: This code could probably be simplified by using "std::find_if", and/or
> doing two passes through the list. Pass 1 = encrypted extensions, pass 2 =
> unencrypted. Though it doesn't really matter, this code is clear enough to me.

Acknowledged.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config_unittest.cc
File webrtc/config_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config_unittest.cc...
webrtc/config_unittest.cc:28: static std::vector<RtpExtension> MakeVector(const
RtpExtension& extension) {
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> nit: Would it be possible to use the vector constructor that takes an
> initializer list, making this unnecessary?

Done.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/config_unittest.cc...
webrtc/config_unittest.cc:35: TEST_F(ConfigTest,
TestRtpExtension_FilterDuplicateNonEncrypted) {
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> nit: I'd change this to "TEST(RtpExtensionTest, FilterDuplicateNonEncrypted)".

Done.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcmediaengine_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcmediaengine_unittest.cc:150: extensions[3].encrypt =
true;
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> nit: It's a little hard to read this test, since you need to look at what
> MakeRedundantExtensions does and figure out what the extensions at indices 2
and
> 3 are.
> 
> Also: unless I'm mistaken, this would result in two extensions having the same
> ID, which isn't legal (would be feasible for it to be DCHECKed).
> 
> I'd prefer if the test explicitly constructed the list, like the test below
> does. The list doesn't need 7 entries; I think just three would be sufficient
to
> cover the specific functionality being tested in this test.

The DCHECK is in "ValidateRtpExtensions" (which is called by
"FilterRtpExtensions"), so it would have been caught if I had run the test
locally with DCHECKS enabled. Changed the test (and my local test setup).

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcmediaengine_unittest.cc:181:
RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3, true));
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> They both have the same ID; is that intentional?

No, see above.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:122: const RtpHeaderExtensions& extensions,
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> I'd just set params->extensions outside of this method, then there'd be no
> reason to pass the extensions into it.

The method is called from different locations, so params->extensions would have
to be set in all of them including the "if ..." condition - I wanted to avoid
this code duplication.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:141: const RtpHeaderExtensions& extensions,
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> Same here.

See comment above.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1084: if (!dtls_keyed_) {
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> I don't think this works in all cases... If the transport changes, the
> SrtpFilter will be reset, but dtls_keyed_ won't be. So, should just check
> "srtp_filter_.IsActive()" here.

Done.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1101: ret = srtp_filter_.UpdateRtpParams(
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> If UpdateRtpParams is *always* called with the same keying information (right
> now), I'd suggest just removing the arguments for simplicity.
> 
> In fact: is there a reason why this couldn't just happen automatically inside
> "SetEncryptedHeaderExtensions"?

"srtp_update" in the srtp filter requires the complete keying information,
that's why they are passed from here. Another option would be to save them in
the srtp filter which I wanted to avoid.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1236: srtp_update_on_answer_ = true;
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> I'm not sure this variable buys anything; an answer will always be set after
an
> offer, so won't it always be true when it's checked below?

You're right - removed it.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:915: bool expect_equal) {
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> Since this is only called with "false" in this file, could remove the
parameter.

Moved to "webrtc/media/base/fakertp.cc" to avoid code duplication (method
existed here and in "srtpfilter_unittest.cc").

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1069: }
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> Once we decide how to resolve the other comments: it would be good to have a
> version of this test that covered the DTLS code path, and simulated DTLS
> completing at different points in the offer/answer exchange.

Acknowledged - though I need to find out how to do that ;-)

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1050: encrypted_extensions.end());
On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> I still think this code could be simplified... But I'm ok leaving that as
> homework for myself. It was already complex before this CL, so it's not your
> fault.

Acknowledged.

Taylor Brandstetter

I only have some minor comments left; great work on this. https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): ...

3 years, 8 months ago (2017-04-19 06:48:40 UTC) #13

I only have some minor comments left; great work on this.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:122: const RtpHeaderExtensions& extensions,
On 2017/04/17 10:46:09, joachim wrote:
> On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> > I'd just set params->extensions outside of this method, then there'd be no
> > reason to pass the extensions into it.
> 
> The method is called from different locations, so params->extensions would
have
> to be set in all of them including the "if ..." condition - I wanted to avoid
> this code duplication.

Acknowledged.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1101: ret = srtp_filter_.UpdateRtpParams(
On 2017/04/17 10:46:09, joachim wrote:
> On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> > If UpdateRtpParams is *always* called with the same keying information
(right
> > now), I'd suggest just removing the arguments for simplicity.
> > 
> > In fact: is there a reason why this couldn't just happen automatically
inside
> > "SetEncryptedHeaderExtensions"?
> 
> "srtp_update" in the srtp filter requires the complete keying information,
> that's why they are passed from here. Another option would be to save them in
> the srtp filter which I wanted to avoid.

Oh, I assumed they already were saved in the SRTP filter. If they're not, this
is fine.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1069: }
On 2017/04/17 10:46:09, joachim wrote:
> On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> > Once we decide how to resolve the other comments: it would be good to have a
> > version of this test that covered the DTLS code path, and simulated DTLS
> > completing at different points in the offer/answer exchange.
> 
> Acknowledged - though I need to find out how to do that ;-)

Well, ConnectFakeTransports is what simulates DTLS completing, and you already
call it before the answer is applied. So I'd say it would be sufficient to just
test sending some packets in the "have offer, but not answer" state, expecting
the extensions in the offer to be decrypted successfully.

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/config.cc#newcode99
webrtc/config.cc:99: // and filter out later if external auth is used in
srtpfilter.
Does this TODO need to be addressed before landing?

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1098: // RTCP doesn't need to be updated.
Can you add "because UpdateRtpParams is only used to update the set of encrypted
RTP header extension IDs"?

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1841:
GetFilteredRtpHeaderExtensions(audio->rtp_header_extensions());
Is this necessary, since SetRtpTransportParameters will already ignore any
extensions that aren't encrypted?

joachim

All comments addressed, ptal. https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittest.cc File webrtc/pc/channel_unittest.cc (right): https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittest.cc#newcode1069 webrtc/pc/channel_unittest.cc:1069: } On 2017/04/19 06:48:39, Taylor ...

3 years, 8 months ago (2017-04-19 23:40:19 UTC) #14

All comments addressed, ptal.

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1069: }
On 2017/04/19 06:48:39, Taylor Brandstetter wrote:
> On 2017/04/17 10:46:09, joachim wrote:
> > On 2017/04/01 00:28:59, Taylor Brandstetter wrote:
> > > Once we decide how to resolve the other comments: it would be good to have
a
> > > version of this test that covered the DTLS code path, and simulated DTLS
> > > completing at different points in the offer/answer exchange.
> > 
> > Acknowledged - though I need to find out how to do that ;-)
> 
> Well, ConnectFakeTransports is what simulates DTLS completing, and you already
> call it before the answer is applied. So I'd say it would be sufficient to
just
> test sending some packets in the "have offer, but not answer" state, expecting
> the extensions in the offer to be decrypted successfully.

Done. Also added a check for correct encrypted extensions when updating in a
later OFFER/ANSWER.

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/config.cc#newcode99
webrtc/config.cc:99: // and filter out later if external auth is used in
srtpfilter.
On 2017/04/19 06:48:39, Taylor Brandstetter wrote:
> Does this TODO need to be addressed before landing?

Just noticed it should have been "kAbsSendTimeUri" in the comment. Only applies
if "ENABLE_EXTERNAL_AUTH" is defined, so I updated the code and comment
accordingly.

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1098: // RTCP doesn't need to be updated.
On 2017/04/19 06:48:40, Taylor Brandstetter wrote:
> Can you add "because UpdateRtpParams is only used to update the set of
encrypted
> RTP header extension IDs"?

Done.

https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1841:
GetFilteredRtpHeaderExtensions(audio->rtp_header_extensions());
On 2017/04/19 06:48:40, Taylor Brandstetter wrote:
> Is this necessary, since SetRtpTransportParameters will already ignore any
> extensions that aren't encrypted?

Yes, the same (filtered) list is also passed to
"RtpParametersFromMediaDescription".

Taylor Brandstetter

Just a couple small questions, otherwise lgtm https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc#newcode1492 webrtc/pc/channel.cc:1492: return extensions; ...

3 years, 8 months ago (2017-04-20 06:37:28 UTC) #15

joachim

Thanks, adressed your comments. https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc#newcode1492 webrtc/pc/channel.cc:1492: return extensions; On 2017/04/20 06:37:28, ...

3 years, 8 months ago (2017-04-24 22:07:48 UTC) #16

Taylor Brandstetter

https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittest.cc File webrtc/pc/channel_unittest.cc (right): https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittest.cc#newcode1019 webrtc/pc/channel_unittest.cc:1019: WaitForThreads(); On 2017/04/24 22:07:48, joachim wrote: > On 2017/04/20 ...

3 years, 8 months ago (2017-04-24 23:08:17 UTC) #17

https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1019: WaitForThreads();
On 2017/04/24 22:07:48, joachim wrote:
> On 2017/04/20 06:37:28, Taylor Brandstetter wrote:
> > I don't understand how the additions above are relevant to this test, could
> you
> > explain?
> 
> You mean the code path where "asymmetric" is true? I assumed that was what you
> wanted to have tested in your previous comment (check the behaviour when the
> DTLS-SRTP was only completed on one side).
> This is the case during initial connection (though I agree that it only makes
> partial sense - the receiver can't handle the packets until the transport
> finished connecting). The new code below checks the behaviour when changing
> encrypted extensions while OFFER/ANSWER are only partially processed.

Sorry if I was unclear. My earlier comment was more about having a test that
ensures things work correctly no matter when the DTLS handshake completes
(meaning when ConnectFakeTransports is called in this test). For example: 

1. Before any offer/answer is set, which could happen if a new "m=" section is
"bundled" onto an existing transport.
2. When the offer is set, but no answer is received yet. Could happen in an
"early media" scenario (theoretically), or if the DTLS handshake completes in
the middle of a *second* offer/answer exchange.
3. After a single offer/answer, in the stable signaling state. This is the
typical scenario.

So you could do the following (pseudocode here):

// Call ConnectFakeTransports() here for scenario 1.
channel1_->SetLocalOffer(o);
channel2_->SetRemoteOffer(o);
channel2_->SetLocalAnswer(a);
// Call ConnectFakeTransports() here for scenario 2.
// For scenarios 1 and 2, try sending packets from channel 2 to 1
// at this point.
channel2_->SetRemoteAnswer(a);
// Call ConnectFakeTransports() here for scenario 3.
// Can now try sending some packets in both directions.

It seems like a sensible thing to test, since there were minor issues with these
scenarios in previous iterations of the CL.

joachim

On 2017/04/24 23:08:17, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittest.cc > File webrtc/pc/channel_unittest.cc (right): > > https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittest.cc#newcode1019 ...

3 years, 7 months ago (2017-04-26 22:42:10 UTC) #18

On 2017/04/24 23:08:17, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittes...
> File webrtc/pc/channel_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittes...
> webrtc/pc/channel_unittest.cc:1019: WaitForThreads();
> On 2017/04/24 22:07:48, joachim wrote:
> > On 2017/04/20 06:37:28, Taylor Brandstetter wrote:
> > > I don't understand how the additions above are relevant to this test,
could
> > you
> > > explain?
> > 
> > You mean the code path where "asymmetric" is true? I assumed that was what
you
> > wanted to have tested in your previous comment (check the behaviour when the
> > DTLS-SRTP was only completed on one side).
> > This is the case during initial connection (though I agree that it only
makes
> > partial sense - the receiver can't handle the packets until the transport
> > finished connecting). The new code below checks the behaviour when changing
> > encrypted extensions while OFFER/ANSWER are only partially processed.
> 
> Sorry if I was unclear. My earlier comment was more about having a test that
> ensures things work correctly no matter when the DTLS handshake completes
> (meaning when ConnectFakeTransports is called in this test). For example: 
> 
> 1. Before any offer/answer is set, which could happen if a new "m=" section is
> "bundled" onto an existing transport.
> 2. When the offer is set, but no answer is received yet. Could happen in an
> "early media" scenario (theoretically), or if the DTLS handshake completes in
> the middle of a *second* offer/answer exchange.
> 3. After a single offer/answer, in the stable signaling state. This is the
> typical scenario.

Thanks for your clarification. I'm not sure about scenarios 1 and 2 (see below).
Scenario 3 is the default which is tested right now.

> So you could do the following (pseudocode here):
> 
> // Call ConnectFakeTransports() here for scenario 1.
> channel1_->SetLocalOffer(o);

-> Channel 1 knows which header extensions to send encrypted.

> channel2_->SetRemoteOffer(o);

-> Channel 2 knows which header extensions will be received encrypted.

> channel2_->SetLocalAnswer(a);

-> Channel 2 knows which header extensions to send encrypted.

> // Call ConnectFakeTransports() here for scenario 2.
> // For scenarios 1 and 2, try sending packets from channel 2 to 1
> // at this point.

While sending packets from channel 2 to 1 works in these scenarios and header
extensions are encrypted, channel 1 doesn't know if/which header extensions are
encrypted. This information is only available once the answer (from channel 2)
is processed. Or is this what you want to have tested?

Also can this really happen? OpenSSLStreamAdapter defers completion of the DTLS
connection until the remote fingerprint is available to verify the peer
certificate - and the fingerprint is part of the (remote) answer. So I think
scenarios 1 and 2 in reality become scenario 3 where the DTLS connection is
"connected" after the Offer/Answer have been processed on both sides.

> channel2_->SetRemoteAnswer(a);

Shouldn't this be "channel1_->SetRemoteAnswer(a);"? After calling this, channel
1 knows which header extensions will be received encrypted.

> // Call ConnectFakeTransports() here for scenario 3.
> // Can now try sending some packets in both directions.
> 
> It seems like a sensible thing to test, since there were minor issues with
these
> scenarios in previous iterations of the CL.

Taylor Brandstetter

On 2017/04/26 22:42:10, joachim wrote: > > While sending packets from channel 2 to 1 ...

3 years, 7 months ago (2017-04-26 23:49:18 UTC) #19

joachim

Thanks for being so patient and sorry for the noise ;-) I found the error ...

3 years, 7 months ago (2017-05-02 23:01:56 UTC) #20

joachim

Description was changed from ========== Support encrypted RTP extensions (RFC 6904) Can be enabled by ...

3 years, 7 months ago (2017-05-02 23:09:52 UTC) #21

Taylor Brandstetter

deadbeef@webrtc.org changed reviewers: + stefan@webrtc.org

3 years, 7 months ago (2017-05-05 02:08:58 UTC) #23

Taylor Brandstetter

Oh, you'll also need an owner for the root webrtc/ directory. stefan@?

3 years, 7 months ago (2017-05-05 02:08:59 UTC) #24

pthatcher1

Just some readability suggestions https://codereview.webrtc.org/2761143002/diff/140001/webrtc/base/sslstreamadapter.h File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/2761143002/diff/140001/webrtc/base/sslstreamadapter.h#newcode86 webrtc/base/sslstreamadapter.h:86: bool enable_encrypted_rtp_header_extensions = false; be ...

3 years, 7 months ago (2017-05-05 21:26:39 UTC) #25

joachim

Thanks a lot, all comments addressed. https://codereview.webrtc.org/2761143002/diff/140001/webrtc/base/sslstreamadapter.h File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/2761143002/diff/140001/webrtc/base/sslstreamadapter.h#newcode86 webrtc/base/sslstreamadapter.h:86: bool enable_encrypted_rtp_header_extensions = ...

3 years, 7 months ago (2017-05-06 14:52:33 UTC) #26

joachim

On 2017/05/05 02:08:59, Taylor Brandstetter wrote: > Oh, you'll also need an owner for the ...

3 years, 7 months ago (2017-05-11 10:40:09 UTC) #28

joachim

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the ...

3 years, 7 months ago (2017-05-18 19:36:43 UTC) #31

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2761143002/160001

3 years, 7 months ago (2017-05-18 19:36:53 UTC) #32

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-18 19:38:22 UTC) #33

commit-bot: I haz the power

Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/4431) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, ...

3 years, 7 months ago (2017-05-18 19:38:23 UTC) #34

joachim

On 2017/05/18 19:38:23, commit-bot: I haz the power wrote: > Try jobs failed on following ...

3 years, 7 months ago (2017-05-18 20:11:01 UTC) #35

joachim

Finally found some time for the rebase. Notable changes since the CL I failed to ...

3 years, 6 months ago (2017-06-23 23:59:16 UTC) #36

Taylor Brandstetter

lgtm again, with a minor comment https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc#newcode1164 webrtc/pc/channel.cc:1164: (rtp_dtls_transport_->dtls_state() == DTLS_TRANSPORT_NEW ...

3 years, 5 months ago (2017-06-28 11:28:21 UTC) #37

joachim

Ptal https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc#newcode1164 webrtc/pc/channel.cc:1164: (rtp_dtls_transport_->dtls_state() == DTLS_TRANSPORT_NEW || On 2017/06/28 11:28:20, Taylor ...

3 years, 5 months ago (2017-06-28 17:03:06 UTC) #38

Taylor Brandstetter

https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc#newcode1164 webrtc/pc/channel.cc:1164: (rtp_dtls_transport_->dtls_state() == DTLS_TRANSPORT_NEW || On 2017/06/28 17:03:05, joachim wrote: ...

3 years, 5 months ago (2017-06-29 06:59:09 UTC) #39

joachim

Changed and rebased again to resolve the "base" -> "rtc_base" conflicts. https://codereview.webrtc.org/2761143002/diff/260001/webrtc/p2p/base/fakedtlstransport.h File webrtc/p2p/base/fakedtlstransport.h (right): ...

3 years, 5 months ago (2017-06-29 17:20:59 UTC) #40

joachim

The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run

3 years, 5 months ago (2017-06-29 17:31:11 UTC) #41

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2761143002/300001

3 years, 5 months ago (2017-06-29 17:31:20 UTC) #42

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-06-29 17:40:49 UTC) #44

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/26823)

3 years, 5 months ago (2017-06-29 17:40:50 UTC) #45

joachim

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, pthatcher@webrtc.org, deadbeef@webrtc.org Link ...

3 years, 5 months ago (2017-06-29 19:01:13 UTC) #47

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2761143002/320001

3 years, 5 months ago (2017-06-29 19:01:21 UTC) #48

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 320001, "attempt_start_ts": 1498762872340330, "parent_rev": "c9be3d5e8be9a3cedd9626482b924117e10cc3de", "commit_rev": "5869f50f7a7079055fde7320a3babc5f26134f1c"}

3 years, 5 months ago (2017-06-29 19:31:37 UTC) #49

commit-bot: I haz the power

Description was changed from ========== Support encrypted RTP extensions (RFC 6904) Can be enabled by ...

3 years, 5 months ago (2017-06-29 19:31:42 UTC) #50

commit-bot: I haz the power

3 years, 5 months ago (2017-06-29 19:31:45 UTC) #51

Message was sent while issue was closed.

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/external/webrtc/+/5869f50f7a7079055fde7320a...

Issue 2761143002: Support encrypted RTP extensions (RFC 6904) (Closed)

Description

Patch Set 1 #

Patch Set 2 : Various changes based on feedback from Peter and Taylor. #

Patch Set 3 : More updates + support for adding/changing encrypted extensions. #

Patch Set 4 : Don't negotiate extension ids in SrtpFilter, more changes after feedback from Taylor. #

Patch Set 5 : More feedback from Taylor. #

Patch Set 6 : Filter encrypted extensions. #

Patch Set 7 : Add different test scenarios, fix send/recv direction for extensions in SRTP filter. #

Patch Set 8 : Updated comment. #

Patch Set 9 : Feedback from Peter. #

Patch Set 10 : Rebased #

Patch Set 11 : Added missing deps for tests. #

Patch Set 12 : Updated condition when to update SRTP filter, fixes error in CallTransferredForXXX tests. #

Patch Set 13 : Added missing include for compiling on Windows. #

Patch Set 14 : Only update SRTP filter if DTLS is connected. #

Patch Set 15 : Don't set state to "DTLS_TRANSPORT_CLOSED". #

Patch Set 16 : Another rebase. #

Patch Set 17 : Fix compile error on win_x64 bots. #

Messages