Completed the functionalities of SrtpTransport.

webrtc/pc/channel.cc

Index: webrtc/pc/channel.cc
diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc
index 59f0869431ae59aef1e26b3dbc555b0c897e7e14..a4123a89a01f92e12bd3396ed06d91cbe068c34d 100644
--- a/webrtc/pc/channel.cc
+++ b/webrtc/pc/channel.cc
@@ -158,18 +158,22 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread,
       signaling_thread_(signaling_thread),
       content_name_(content_name),
       rtcp_mux_required_(rtcp_mux_required),
-      rtp_transport_(
-          srtp_required
-              ? rtc::WrapUnique<webrtc::RtpTransportInternal>(
-                    new webrtc::SrtpTransport(rtcp_mux_required, content_name))
-              : rtc::MakeUnique<webrtc::RtpTransport>(rtcp_mux_required)),
       srtp_required_(srtp_required),
       media_channel_(media_channel),
       selected_candidate_pair_(nullptr) {
   RTC_DCHECK(worker_thread_ == rtc::Thread::Current());
+  if (srtp_required) {
+    auto transport =
+        rtc::MakeUnique<webrtc::SrtpTransport>(rtcp_mux_required, content_name);
+    srtp_transport_ = transport.get();
+    rtp_transport_ = std::move(transport);
 #if defined(ENABLE_EXTERNAL_AUTH)
-  srtp_filter_.EnableExternalAuth();
+    srtp_transport_->EnableExternalAuth();
 #endif
+  } else {
+    rtp_transport_ = rtc::MakeUnique<webrtc::RtpTransport>(rtcp_mux_required);
+    srtp_transport_ = nullptr;
+  }
   rtp_transport_->SignalReadyToSend.connect(
       this, &BaseChannel::OnTransportReadyToSend);
   // TODO(zstein):  RtpTransport::SignalPacketReceived will probably be replaced
@@ -322,6 +326,9 @@ void BaseChannel::SetTransports_n(
     // DTLS-SRTP when |writable_| becomes true again.
     writable_ = false;
     srtp_filter_.ResetParams();

[Taylor Brandstetter, 2017/08/23 22:13:29]

nit: Don't need to actually reset srtp_filter_ here. If DTLS is being used,
srtp_filter_ is unused, right?

[Zhi Huang, 2017/08/24 23:38:07]

Right. Previously, srtp_filter_.IsActive() can also imply the DTLS keys are
successfully installed. So I think the method |secure()| should be renamed to
|secure_sdes()| as well.

+    if (srtp_transport_) {
+      srtp_transport_->ResetParams();
+    }
   }
 
   // If this BaseChannel doesn't require RTCP mux and we haven't fully
@@ -589,6 +596,9 @@ void BaseChannel::OnDtlsState(DtlsTransportInternal* transport,
   // negotiated.
   if (state != DTLS_TRANSPORT_CONNECTED) {
     srtp_filter_.ResetParams();
+    if (srtp_transport_) {
+      srtp_transport_->ResetParams();
+    }
   }
 }
 
@@ -662,74 +672,7 @@ bool BaseChannel::SendPacket(bool rtcp,
     return false;
   }
 
-  rtc::PacketOptions updated_options;
-  updated_options = options;
-  // Protect if needed.
-  if (srtp_filter_.IsActive()) {
-    TRACE_EVENT0("webrtc", "SRTP Encode");
-    bool res;
-    uint8_t* data = packet->data();
-    int len = static_cast<int>(packet->size());
-    if (!rtcp) {
-    // If ENABLE_EXTERNAL_AUTH flag is on then packet authentication is not done
-    // inside libsrtp for a RTP packet. A external HMAC module will be writing
-    // a fake HMAC value. This is ONLY done for a RTP packet.
-    // Socket layer will update rtp sendtime extension header if present in
-    // packet with current time before updating the HMAC.
-#if !defined(ENABLE_EXTERNAL_AUTH)
-      res = srtp_filter_.ProtectRtp(
-          data, len, static_cast<int>(packet->capacity()), &len);
-#else
-      if (!srtp_filter_.IsExternalAuthActive()) {
-        res = srtp_filter_.ProtectRtp(
-            data, len, static_cast<int>(packet->capacity()), &len);
-      } else {
-        updated_options.packet_time_params.rtp_sendtime_extension_id =
-            rtp_abs_sendtime_extn_id_;
-        res = srtp_filter_.ProtectRtp(
-            data, len, static_cast<int>(packet->capacity()), &len,
-            &updated_options.packet_time_params.srtp_packet_index);
-        // If protection succeeds, let's get auth params from srtp.
-        if (res) {
-          uint8_t* auth_key = NULL;
-          int key_len;
-          res = srtp_filter_.GetRtpAuthParams(
-              &auth_key, &key_len,
-              &updated_options.packet_time_params.srtp_auth_tag_len);
-          if (res) {
-            updated_options.packet_time_params.srtp_auth_key.resize(key_len);
-            updated_options.packet_time_params.srtp_auth_key.assign(
-                auth_key, auth_key + key_len);
-          }
-        }
-      }
-#endif
-      if (!res) {
-        int seq_num = -1;
-        uint32_t ssrc = 0;
-        GetRtpSeqNum(data, len, &seq_num);
-        GetRtpSsrc(data, len, &ssrc);
-        LOG(LS_ERROR) << "Failed to protect " << content_name_
-                      << " RTP packet: size=" << len
-                      << ", seqnum=" << seq_num << ", SSRC=" << ssrc;
-        return false;
-      }
-    } else {
-      res = srtp_filter_.ProtectRtcp(data, len,
-                                     static_cast<int>(packet->capacity()),
-                                     &len);
-      if (!res) {
-        int type = -1;
-        GetRtcpType(data, len, &type);
-        LOG(LS_ERROR) << "Failed to protect " << content_name_
-                      << " RTCP packet: size=" << len << ", type=" << type;
-        return false;
-      }
-    }
-
-    // Update the length of the packet now that we've added the auth tag.
-    packet->SetSize(len);
-  } else if (srtp_required_) {
+  if (!srtp_filter_.IsActive() && srtp_required_) {
     // The audio/video engines may attempt to send RTCP packets as soon as the
     // streams are created, so don't treat this as an error for RTCP.
     // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=6809
@@ -744,9 +687,18 @@ bool BaseChannel::SendPacket(bool rtcp,
     return false;
   }
 
+  rtc::PacketOptions updated_options;
+  updated_options = options;

[Taylor Brandstetter, 2017/08/23 22:13:29]

Since the options are updated inside of srtp_transport_->SendPacket, don't need
to make a copy here.

[Zhi Huang, 2017/08/24 23:38:07]

Done.

+  if (srtp_filter_.IsActive()) {
+    RTC_DCHECK(srtp_transport_);
+    RTC_DCHECK(srtp_transport_->IsActive());
+    // Bon voyage.
+    int flags = secure_dtls() ? PF_SRTP_BYPASS : PF_NORMAL;
+    return srtp_transport_->SendPacket(rtcp, packet, updated_options, flags);
+  }
+
   // Bon voyage.
-  int flags = (secure() && secure_dtls()) ? PF_SRTP_BYPASS : PF_NORMAL;
-  return rtp_transport_->SendPacket(rtcp, packet, updated_options, flags);
+  return rtp_transport_->SendPacket(rtcp, packet, updated_options, PF_NORMAL);
 }
 
 bool BaseChannel::HandlesPayloadType(int packet_type) const {
@@ -761,37 +713,7 @@ void BaseChannel::OnPacketReceived(bool rtcp,
     signaling_thread()->Post(RTC_FROM_HERE, this, MSG_FIRSTPACKETRECEIVED);
   }
 
-  // Unprotect the packet, if needed.
-  if (srtp_filter_.IsActive()) {
-    TRACE_EVENT0("webrtc", "SRTP Decode");
-    char* data = packet->data<char>();
-    int len = static_cast<int>(packet->size());
-    bool res;
-    if (!rtcp) {
-      res = srtp_filter_.UnprotectRtp(data, len, &len);
-      if (!res) {
-        int seq_num = -1;
-        uint32_t ssrc = 0;
-        GetRtpSeqNum(data, len, &seq_num);
-        GetRtpSsrc(data, len, &ssrc);
-        LOG(LS_ERROR) << "Failed to unprotect " << content_name_
-                      << " RTP packet: size=" << len << ", seqnum=" << seq_num
-                      << ", SSRC=" << ssrc;
-        return;
-      }
-    } else {
-      res = srtp_filter_.UnprotectRtcp(data, len, &len);
-      if (!res) {
-        int type = -1;
-        GetRtcpType(data, len, &type);
-        LOG(LS_ERROR) << "Failed to unprotect " << content_name_
-                      << " RTCP packet: size=" << len << ", type=" << type;
-        return;
-      }
-    }
-
-    packet->SetSize(len);
-  } else if (srtp_required_) {
+  if (!srtp_filter_.IsActive() && srtp_required_) {
     // Our session description indicates that SRTP is required, but we got a
     // packet before our SRTP filter is active. This means either that
     // a) we got SRTP packets before we received the SDES keys, in which case
@@ -997,15 +919,20 @@ bool BaseChannel::SetupDtlsSrtp_n(bool rtcp) {
 
   if (!srtp_filter_.IsActive()) {
     if (rtcp) {
-      ret = srtp_filter_.SetRtcpParams(selected_crypto_suite, &(*send_key)[0],
-                                       static_cast<int>(send_key->size()),
-                                       selected_crypto_suite, &(*recv_key)[0],
-                                       static_cast<int>(recv_key->size()));
+      RTC_DCHECK(srtp_transport_);
+      ret = srtp_transport_->SetRtcpParams(
+          selected_crypto_suite, &(*send_key)[0],
+          static_cast<int>(send_key->size()), selected_crypto_suite,
+          &(*recv_key)[0], static_cast<int>(recv_key->size()));
     } else {
-      ret = srtp_filter_.SetRtpParams(selected_crypto_suite, &(*send_key)[0],
-                                      static_cast<int>(send_key->size()),
-                                      selected_crypto_suite, &(*recv_key)[0],
-                                      static_cast<int>(recv_key->size()));
+      RTC_DCHECK(srtp_transport_);
+      // If the SRTP crypto keys are from DTLS handshake, explicitly call
+      // |EnableDtlsSrtp| to activate SrtpFilter.
+      srtp_filter_.EnableDtlsSrtp();

[Taylor Brandstetter, 2017/08/23 22:13:29]

Why does srtp_filter_ even need to know if DTLS-SRTP is used? If DTLS-SRTP is
used, srtp_filter_ isn't really used, right?

[Zhi Huang, 2017/08/24 23:38:07]

Done.

+      ret = srtp_transport_->SetRtpParams(
+          selected_crypto_suite, &(*send_key)[0],
+          static_cast<int>(send_key->size()), selected_crypto_suite,
+          &(*recv_key)[0], static_cast<int>(recv_key->size()));
     }
   } else {
     if (rtcp) {
@@ -1013,10 +940,9 @@ bool BaseChannel::SetupDtlsSrtp_n(bool rtcp) {
       // to update the set of encrypted RTP header extension IDs.
       ret = true;
     } else {
-      ret = srtp_filter_.UpdateRtpParams(
-          selected_crypto_suite,
-          &(*send_key)[0], static_cast<int>(send_key->size()),
-          selected_crypto_suite,
+      ret = srtp_transport_->UpdateRtpParams(
+          selected_crypto_suite, &(*send_key)[0],
+          static_cast<int>(send_key->size()), selected_crypto_suite,
           &(*recv_key)[0], static_cast<int>(recv_key->size()));
     }
   }
@@ -1039,6 +965,10 @@ void BaseChannel::MaybeSetupDtlsSrtp_n() {
     return;
   }
 
+  if (!srtp_transport_) {
+    EnableSrtpTransport_n();
+  }
+
   if (!SetupDtlsSrtp_n(false)) {
     SignalDtlsSrtpSetupFailure_n(false);
     return;
@@ -1122,6 +1052,29 @@ bool BaseChannel::CheckSrtpConfig_n(const std::vector<CryptoParams>& cryptos,
   return true;
 }
 
+void BaseChannel::EnableSrtpTransport_n() {
+  if (srtp_transport_ == nullptr) {
+    rtp_transport_->SignalReadyToSend.disconnect(this);
+    rtp_transport_->SignalPacketReceived.disconnect(this);
+
+    auto transport = rtc::MakeUnique<webrtc::SrtpTransport>(
+        std::move(rtp_transport_), content_name_);
+    srtp_transport_ = transport.get();
+    rtp_transport_ = std::move(transport);
+
+    rtp_transport_->SignalReadyToSend.connect(
+        this, &BaseChannel::OnTransportReadyToSend);
+    rtp_transport_->SignalPacketReceived.connect(
+        this, &BaseChannel::OnPacketReceived);
+
+    if (rtp_abs_sendtime_extn_id_ != -1) {
+      srtp_transport_->CacheRtpAbsSendTimeHeaderExtension(
+          rtp_abs_sendtime_extn_id_);
+    }
+    LOG(LS_INFO) << "Wrapping RtpTransport in SrtpTransport.";

[Taylor Brandstetter, 2017/08/23 22:13:29]

I don't think upgrading from plain RTP to SRTP is something we need to support.
RFC4568 says:

   When the offerer receives the answer, the offerer MUST verify that
   one of the initially offered crypto suites and its accompanying tag
   were accepted and echoed in the answer.

So I'd say we should drop support for this, for both simplicity and standards
compliance. Probably safest to do so in a separate CL.

This means that we'd only create the RTP transport object once the first
description is set, and the "proto" string (e.g., "RTP", "RTP/SAVPF" or
"UDP/TLS/RTP/SAVPF") is known.

[Zhi Huang, 2017/08/24 23:38:07]

I would like to talk about this a little bit more.

+  }
+}
+
 bool BaseChannel::SetSrtp_n(const std::vector<CryptoParams>& cryptos,
                             ContentAction action,
                             ContentSource src,
@@ -1138,7 +1091,13 @@ bool BaseChannel::SetSrtp_n(const std::vector<CryptoParams>& cryptos,
   if (!ret) {
     return false;
   }
-  srtp_filter_.SetEncryptedHeaderExtensionIds(src, encrypted_extension_ids);
+  if (!srtp_transport_ && !dtls) {
+    EnableSrtpTransport_n();
+  }
+  if (srtp_transport_) {
+    srtp_transport_->SetEncryptedHeaderExtensionIds(src,
+                                                    encrypted_extension_ids);
+  }
   switch (action) {
     case CA_OFFER:
       // If DTLS is already active on the channel, we could be renegotiating
@@ -1152,6 +1111,20 @@ bool BaseChannel::SetSrtp_n(const std::vector<CryptoParams>& cryptos,
       // with an answer, because we already have SRTP parameters.
       if (!dtls) {
         ret = srtp_filter_.SetProvisionalAnswer(cryptos, src);
+        if (ret) {
+          if (srtp_filter_.send_cipher_suite() &&
+              srtp_filter_.recv_cipher_suite()) {
+            auto send_key = srtp_filter_.send_key();
+            auto recv_key = srtp_filter_.recv_key();
+            ret = srtp_transport_->SetRtpParams(
+                *(srtp_filter_.send_cipher_suite()), &(*send_key)[0],
+                static_cast<int>(send_key->size()),
+                *(srtp_filter_.recv_cipher_suite()), &(*recv_key)[0],
+                static_cast<int>(recv_key->size()));
+          } else {
+            LOG(LS_INFO) << "No crypto keys are provided for SDES.";
+          }
+        }
       }
       break;
     case CA_ANSWER:
@@ -1159,6 +1132,24 @@ bool BaseChannel::SetSrtp_n(const std::vector<CryptoParams>& cryptos,
       // with an answer, because we already have SRTP parameters.
       if (!dtls) {
         ret = srtp_filter_.SetAnswer(cryptos, src);
+        if (ret) {
+          if (srtp_filter_.send_cipher_suite() &&
+              srtp_filter_.recv_cipher_suite()) {
+            auto send_key = srtp_filter_.send_key();
+            auto recv_key = srtp_filter_.recv_key();
+            ret = srtp_transport_->SetRtpParams(
+                *(srtp_filter_.send_cipher_suite()), &(*send_key)[0],
+                static_cast<int>(send_key->size()),
+                *(srtp_filter_.recv_cipher_suite()), &(*recv_key)[0],
+                static_cast<int>(recv_key->size()));
+          } else {
+            // Explicitly reset the |srtp_transport_| if no crypto param is
+            // provided in the answer. No need to call |ResetParams()| for
+            // |srtp_filter_| because it resets the params inside |SetAnswer|.
+            srtp_transport_->ResetParams();
+            LOG(LS_INFO) << "No crypto keys are provided for SDES.";

[Taylor Brandstetter, 2017/08/23 22:13:29]

This looks the same as the code above; could it be moved out of the switch
statement?

// If setting an SDES answer succeeded, apply the negotiated parameters
// to the SRTP transport.
if ((action == CA_PRANSWER || action == CA_ANSWER) && !dlts && ret) {
  // ...
}

[Zhi Huang, 2017/08/24 23:38:07]

Yes, that would be cleaner.

+          }
+        }
       }
       break;
     default:
@@ -1211,7 +1202,6 @@ bool BaseChannel::SetRtcpMux_n(bool enable,
             transport_name_.empty()
                 ? rtp_transport_->rtp_packet_transport()->debug_name()
                 : transport_name_;
-        ;
         LOG(LS_INFO) << "Enabling rtcp-mux for " << content_name()
                      << "; no longer need RTCP transport for " << debug_name;
         if (rtp_transport_->rtcp_packet_transport()) {
@@ -1441,6 +1431,10 @@ void BaseChannel::MaybeCacheRtpAbsSendTimeHeaderExtension_w(
 void BaseChannel::CacheRtpAbsSendTimeHeaderExtension_n(
     int rtp_abs_sendtime_extn_id) {
   rtp_abs_sendtime_extn_id_ = rtp_abs_sendtime_extn_id;

[Taylor Brandstetter, 2017/08/23 22:13:29]

If this were always called after the SRTP transport is created, it wouldn't need
to be cached in both places.

[Zhi Huang, 2017/08/24 23:38:07]

This is called after SetTransportParameters which create the srtptransport. I'll
add a DCHECK here then.

+  if (srtp_transport_) {
+    srtp_transport_->CacheRtpAbsSendTimeHeaderExtension(
+        rtp_abs_sendtime_extn_id_);
+  }
 }
 
 void BaseChannel::OnMessage(rtc::Message *pmsg) {
@@ -1726,7 +1720,7 @@ int BaseChannel::GetTransportOverheadPerPacket() const {
 
   if (secure()) {
     int srtp_overhead = 0;
-    if (srtp_filter_.GetSrtpOverhead(&srtp_overhead))
+    if (srtp_transport_->GetSrtpOverhead(&srtp_overhead))
       transport_overhead_per_packet += srtp_overhead;
   }