Support encrypted RTP extensions (RFC 6904)

webrtc/pc/mediasession.cc

Index: webrtc/pc/mediasession.cc
diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc
index dbd32566d523d3d67d8ca3be8fee17e7190fcf9f..1f0b05f04554b7751d9e81cb101986d2748c7b34 100644
--- a/webrtc/pc/mediasession.cc
+++ b/webrtc/pc/mediasession.cc
@@ -945,6 +945,32 @@ static bool FindByUri(const RtpHeaderExtensions& extensions,
   return false;
 }
 
+static bool FindByUriPreferEncrypted(const RtpHeaderExtensions& extensions,

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

nit: This name sounds like it always prefers encrypted extensions. Something
like "FindByUriWithEncryptionPreference" may be better.

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

Done.

+                                     const webrtc::RtpExtension& ext_to_match,
+                                     bool prefer_encrypted,
+                                     webrtc::RtpExtension* found_extension) {
+  const webrtc::RtpExtension* regular_extension = nullptr;
+  for (RtpHeaderExtensions::const_iterator it = extensions.begin();
+       it  != extensions.end(); ++it) {
+    // We assume that all URIs are given in a canonical format.
+    if (it->uri == ext_to_match.uri) {
+      if (!found_extension) {
+        return true;
+      }
+      if (!prefer_encrypted || it->encrypted) {
+        *found_extension = *it;
+        return true;
+      }
+      regular_extension = &(*it);
+    }
+  }
+  if (regular_extension) {
+    *found_extension = *regular_extension;
+    return true;
+  }
+  return false;
+}
+
 // Iterates through |offered_extensions|, adding each one to |all_extensions|
 // and |used_ids|, and resolving ID conflicts. If an offered extension has the
 // same URI as one in |all_extensions|, it will re-use the same ID and won't be
@@ -984,15 +1010,35 @@ static void FindRtpHdrExtsToOffer(
   }
 }
 
+static void AddEncryptedHdrExts(RtpHeaderExtensions* extensions,
+                                UsedRtpHeaderExtensionIds* used_ids) {

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

This should probably be called something like AddEncryptedVersionsOfHdrExts

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

Done.

+  RtpHeaderExtensions encrypted_extensions;
+  for (const webrtc::RtpExtension& extension : *extensions) {
+    if (extension.encrypted ||
+        !webrtc::RtpExtension::AllowEncrypt(extension.uri)) {
+      continue;
+    }
+
+    webrtc::RtpExtension encrypted(extension);
+    encrypted.encrypted = true;
+    used_ids->FindAndSetIdUsed(&encrypted);
+    encrypted_extensions.push_back(encrypted);

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

Is there a reason the separate "encrypted_extensions" list is used, rather than
calling push_back in "extensions" directly?

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

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

+  }
+  extensions->insert(extensions->end(), encrypted_extensions.begin(),
+      encrypted_extensions.end());
+}
+
 static void NegotiateRtpHeaderExtensions(
     const RtpHeaderExtensions& local_extensions,
     const RtpHeaderExtensions& offered_extensions,
+    bool enable_rtp_header_encryption,
     RtpHeaderExtensions* negotiated_extenstions) {
   RtpHeaderExtensions::const_iterator ours;
   for (ours = local_extensions.begin();
        ours != local_extensions.end(); ++ours) {
     webrtc::RtpExtension theirs;
-    if (FindByUri(offered_extensions, *ours, &theirs)) {
+    if (FindByUriPreferEncrypted(offered_extensions, *ours,
+        enable_rtp_header_encryption, &theirs)) {
       // We respond with their RTP header extension id.
       negotiated_extenstions->push_back(theirs);
     }
@@ -1027,6 +1073,7 @@ static bool CreateMediaContentAnswer(
     const SecurePolicy& sdes_policy,
     const CryptoParamsVec* current_cryptos,
     const RtpHeaderExtensions& local_rtp_extenstions,
+    bool enable_rtp_header_encryption,
     StreamParamsVec* current_streams,
     bool add_legacy_stream,
     bool bundle_enabled,
@@ -1038,6 +1085,7 @@ static bool CreateMediaContentAnswer(
   RtpHeaderExtensions negotiated_rtp_extensions;
   NegotiateRtpHeaderExtensions(local_rtp_extenstions,
                                offer->rtp_header_extensions(),
+                               enable_rtp_header_encryption,
                                &negotiated_rtp_extensions);
   answer->set_rtp_header_extensions(negotiated_rtp_extensions);
 
@@ -1605,6 +1653,10 @@ void MediaSessionDescriptionFactory::GetRtpHdrExtsToOffer(
                         &all_extensions, &used_ids);
   FindRtpHdrExtsToOffer(video_rtp_header_extensions(), video_extensions,
                         &all_extensions, &used_ids);
+  if (enable_rtp_header_encryption_) {
+    AddEncryptedHdrExts(audio_extensions, &used_ids);
+    AddEncryptedHdrExts(video_extensions, &used_ids);

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

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.

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

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.

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

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.

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

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. 

+  }
 }
 
 bool MediaSessionDescriptionFactory::AddTransportOffer(
@@ -1878,6 +1930,7 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer(
           sdes_policy,
           GetCryptos(GetFirstAudioContentDescription(current_description)),
           audio_rtp_extensions_,
+          enable_rtp_header_encryption_,
           current_streams,
           add_legacy_,
           bundle_enabled,
@@ -1934,6 +1987,7 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer(
           sdes_policy,
           GetCryptos(GetFirstVideoContentDescription(current_description)),
           video_rtp_extensions_,
+          enable_rtp_header_encryption_,
           current_streams,
           add_legacy_,
           bundle_enabled,
@@ -1995,6 +2049,7 @@ bool MediaSessionDescriptionFactory::AddDataContentForAnswer(
           sdes_policy,
           GetCryptos(GetFirstDataContentDescription(current_description)),
           RtpHeaderExtensions(),
+          enable_rtp_header_encryption_,
           current_streams,
           add_legacy_,
           bundle_enabled,