Support encrypted RTP extensions (RFC 6904)

webrtc/pc/channel.cc

Index: webrtc/pc/channel.cc
diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc
index ecc1c9a633c6d4a935a146f048483256ccdc5d49..0eea97fba40be6f7c12c61c0156a16a792d48c00 100644
--- a/webrtc/pc/channel.cc
+++ b/webrtc/pc/channel.cc
@@ -43,7 +43,6 @@ struct SendPacketMessageData : public rtc::MessageData {
   rtc::PacketOptions options;
 };
 
-#if defined(ENABLE_EXTERNAL_AUTH)
 // Returns the named header extension if found among all extensions,
 // nullptr otherwise.
 const webrtc::RtpExtension* FindHeaderExtension(
@@ -55,7 +54,6 @@ const webrtc::RtpExtension* FindHeaderExtension(
   }
   return nullptr;
 }
-#endif
 
 }  // namespace
 
@@ -324,6 +322,12 @@ void BaseChannel::SetTransports_n(
     // Set |writable_| to false such that UpdateWritableState_w can set up
     // DTLS-SRTP when |writable_| becomes true again.
     writable_ = false;
+    if (srtp_filter_.IsActive()) {
+      // Only clear header information if transport changes (i.e. SRTP has been
+      // activated before), not on initial transport (which might happen during
+      // offer/answer processing).
+      rtp_encrypted_headers_ready_ = 0;

[Taylor Brandstetter, 2017/03/22 18:00:11]

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.

[joachim, 2017/03/23 00:04:33]

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

+    }

[pthatcher1, 2017/03/21 07:07:06]

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.

[joachim, 2017/03/23 00:04:33]

The extra state has now been removed.

     srtp_filter_.ResetParams();
   }
 
@@ -991,8 +995,8 @@ void BaseChannel::ChannelWritable_n() {
         << selected_candidate_pair_->remote_candidate().ToSensitiveString();
 
   was_ever_writable_ = true;
-  MaybeSetupDtlsSrtp_n();
   writable_ = true;
+  MaybeSetupDtlsSrtp_n();

[pthatcher1, 2017/03/21 07:07:06]

Why did this have to change?  

[Taylor Brandstetter, 2017/03/22 18:00:11]

Because ShouldSetupDtlsSrtp_n now checks it. May want to mention in a comment,
if it doesn't end up changing.

[joachim, 2017/03/23 00:04:32]

Right, added a comment for now.

   UpdateMediaSendRecvState();
 }
 
@@ -1099,10 +1103,29 @@ bool BaseChannel::SetupDtlsSrtp_n(bool rtcp) {
                                      selected_crypto_suite, &(*recv_key)[0],
                                      static_cast<int>(recv_key->size()));
   } else {
+    std::vector<int> send_encrypted_headers;
+    std::vector<int> recv_encrypted_headers;
+
+    // Send local extension header encrypted if the remote also uses it
+    // encrypted and vice versa.
+    for (const webrtc::RtpExtension& extension : rtp_local_encrypted_headers_) {
+      if (FindHeaderExtension(rtp_remote_encrypted_headers_, extension.uri)) {
+        send_encrypted_headers.push_back(extension.id);
+      }
+    }
+    for (const webrtc::RtpExtension& extension :
+         rtp_remote_encrypted_headers_) {
+      if (FindHeaderExtension(rtp_local_encrypted_headers_, extension.uri)) {
+        recv_encrypted_headers.push_back(extension.id);
+      }
+    }

[pthatcher1, 2017/03/21 07:07:06]

Why not just pass in all header extensions and let srtp_filter_ check the
.encrypted bit?  Seems a lot more simple.

[Taylor Brandstetter, 2017/03/22 18:00:11]

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.

[joachim, 2017/03/23 00:04:33]

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.

[joachim, 2017/03/23 00:04:33]

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

[Taylor Brandstetter, 2017/03/23 20:10:56]

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

[joachim, 2017/03/30 22:43:49]

Done (in "SrtpFilter::GetSendRecvEncryptedHeaderExtensions").

+
     ret = srtp_filter_.SetRtpParams(selected_crypto_suite, &(*send_key)[0],
                                     static_cast<int>(send_key->size()),
+                                    send_encrypted_headers,
                                     selected_crypto_suite, &(*recv_key)[0],
-                                    static_cast<int>(recv_key->size()));
+                                    static_cast<int>(recv_key->size()),
+                                    recv_encrypted_headers);
   }
 
   if (!ret) {
@@ -1114,26 +1137,36 @@ bool BaseChannel::SetupDtlsSrtp_n(bool rtcp) {
   return ret;
 }
 
-void BaseChannel::MaybeSetupDtlsSrtp_n() {
+bool BaseChannel::MaybeSetupDtlsSrtp_n() {
   if (srtp_filter_.IsActive()) {
-    return;
+    return false;
+  }
+
+  if (!writable_) {
+    return false;
+  }
+
+  if (rtp_encrypted_headers_ready_ != kAllEncryptedHeadersReady) {
+    return false;

[pthatcher1, 2017/03/21 07:07:06]

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.

[joachim, 2017/03/23 00:04:33]

Code has been removed in latest update.

   }
 
   if (!ShouldSetupDtlsSrtp_n()) {
-    return;
+    return false;
   }
 
   if (!SetupDtlsSrtp_n(false)) {
     SignalDtlsSrtpSetupFailure_n(false);
-    return;
+    return false;
   }
 
   if (rtcp_dtls_transport_) {
     if (!SetupDtlsSrtp_n(true)) {
       SignalDtlsSrtpSetupFailure_n(true);
-      return;
+      return false;
     }
   }
+
+  return true;
 }
 
 void BaseChannel::ChannelNotWritable_n() {
@@ -1480,11 +1513,42 @@ void BaseChannel::MaybeCacheRtpAbsSendTimeHeaderExtension_w(
 #endif
 }
 
+void BaseChannel::PrepareHeaderExtensions_w(ContentSource source,
+    const std::vector<webrtc::RtpExtension>& extensions) {
+  std::vector<webrtc::RtpExtension> encrypted_headers;
+  for (const webrtc::RtpExtension& extension : extensions) {
+    if (extension.encrypted) {
+      LOG(LS_INFO) << "Using " << source << " encrypted extension: "
+          << extension.ToString();
+      encrypted_headers.push_back(extension);
+    }
+  }
+
+  invoker_.AsyncInvoke<void>(
+      RTC_FROM_HERE, network_thread_,
+      Bind(&BaseChannel::PrepareHeaderExtensions_n, this, source,
+           encrypted_headers));

[Taylor Brandstetter, 2017/03/22 18:00:10]

Is there a reason why PrepareHeaderExtensions_w is called with all extensions,
but PrepareHeaderExtensions_n is only called with encrypted extensions?

[joachim, 2017/03/23 00:04:32]

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.

[Taylor Brandstetter, 2017/04/01 00:28:58]

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.

[joachim, 2017/04/17 10:46:09]

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

+}
+
 void BaseChannel::CacheRtpAbsSendTimeHeaderExtension_n(
     int rtp_abs_sendtime_extn_id) {
   rtp_abs_sendtime_extn_id_ = rtp_abs_sendtime_extn_id;
 }
 
+void BaseChannel::PrepareHeaderExtensions_n(ContentSource source,

[Taylor Brandstetter, 2017/03/22 18:00:10]

Can this be done in SetRtpTransportParameters_n, to avoid the extra thread hop?

[joachim, 2017/03/23 00:04:33]

Done.

+    const std::vector<webrtc::RtpExtension> encrypted_headers) {
+  if (source == CS_LOCAL) {
+    rtp_local_encrypted_headers_ = encrypted_headers;
+    rtp_encrypted_headers_ready_ |= kLocalEncryptedHeadersReady;

[pthatcher1, 2017/03/21 07:07:06]

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?

[joachim, 2017/03/23 00:04:32]

Right, and no longer needed with the latest changes.

+  } else {
+    rtp_remote_encrypted_headers_ = encrypted_headers;
+    rtp_encrypted_headers_ready_ |= kRemoteEncryptedHeadersReady;
+  }
+  if (MaybeSetupDtlsSrtp_n()) {

[Taylor Brandstetter, 2017/03/22 18:00:11]

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).

[joachim, 2017/03/23 00:04:33]

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

[Taylor Brandstetter, 2017/03/23 20:10:56]

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.

[joachim, 2017/03/30 22:43:49]

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

+    UpdateMediaSendRecvState();

[pthatcher1, 2017/03/21 07:07:06]

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...

[joachim, 2017/03/23 00:04:32]

This is no longer needed with the latest changes.

+  }
+}
+
 void BaseChannel::OnMessage(rtc::Message *pmsg) {
   TRACE_EVENT0("webrtc", "BaseChannel::OnMessage");
   switch (pmsg->message_id) {
@@ -1804,6 +1868,8 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content,
     return false;
   }
 
+  PrepareHeaderExtensions_w(CS_LOCAL, audio->rtp_header_extensions());
+
   if (!SetRtpTransportParameters(content, action, CS_LOCAL, error_desc)) {
     return false;
   }
@@ -1849,6 +1915,8 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content,
     return false;
   }
 
+  PrepareHeaderExtensions_w(CS_REMOTE, audio->rtp_header_extensions());
+
   if (!SetRtpTransportParameters(content, action, CS_REMOTE, error_desc)) {
     return false;
   }
@@ -2082,6 +2150,8 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,
     return false;
   }
 
+  PrepareHeaderExtensions_w(CS_LOCAL, video->rtp_header_extensions());
+
   if (!SetRtpTransportParameters(content, action, CS_LOCAL, error_desc)) {
     return false;
   }
@@ -2127,6 +2197,8 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content,
     return false;
   }
 
+  PrepareHeaderExtensions_w(CS_REMOTE, video->rtp_header_extensions());
+
   if (!SetRtpTransportParameters(content, action, CS_REMOTE, error_desc)) {
     return false;
   }