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..cc0c25637926b7369ea9fb1f94189bc9ccb2717d 100644
--- a/webrtc/pc/channel.cc
+++ b/webrtc/pc/channel.cc
@@ -118,6 +118,10 @@ static const MediaContentDescription* GetContentDescription(
   return static_cast<const MediaContentDescription*>(cinfo->description);
 }
 
+static bool IsInsecureRtp(const std::string& protocol) {
+  return protocol == "RTP/AVPF" || protocol == "RTP/AVP";

[Taylor Brandstetter, 2017/08/26 02:40:38]

Looking elsewhere, it looks like we typically use "cryptos.empty()" to tell if
SDES is being used, and "IsDtlsActive" to tell if DTLS is being used. Also, we
actually currently accept the "RTP/AVP" string in chromium for DTLS (I checked
using the SDP munging sample). So, sorry for leading you wrong by suggesting
using the protocol string, but I think we should aim for minimizing unintended
side effects of this CL, and do something similar to CheckSrtpConfig_n

[Zhi Huang, 2017/08/29 18:40:34]

Removed.

+}
+
 template <class Codec>
 void RtpParametersFromMediaDescription(
     const MediaContentDescriptionImpl<Codec>* desc,
@@ -158,17 +162,20 @@ 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());
+  // Always create an SrtpTransport for |rtp_transport_|. If insecure protocol
+  // is used, |srtp_transport_| will be set to nullptr and |rtp_transport_| will
+  // act as a plain RtpTransport.
+  // TODO(zhihuang): Move the transport creation to the TransportController.
+  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
   rtp_transport_->SignalReadyToSend.connect(
       this, &BaseChannel::OnTransportReadyToSend);
@@ -321,7 +328,10 @@ void BaseChannel::SetTransports_n(
     // Set |writable_| to false such that UpdateWritableState_w can set up
     // DTLS-SRTP when |writable_| becomes true again.
     writable_ = false;
-    srtp_filter_.ResetParams();
+    dtls_active_ = false;
+    if (srtp_transport_) {
+      srtp_transport_->ResetParams();
+    }
   }
 
   // If this BaseChannel doesn't require RTCP mux and we haven't fully
@@ -377,8 +387,8 @@ void BaseChannel::SetTransport_n(
   }
 
   if (rtcp && new_dtls_transport) {
-    RTC_CHECK(!(ShouldSetupDtlsSrtp_n() && srtp_filter_.IsActive()))
-        << "Setting RTCP for DTLS/SRTP after SrtpFilter is active "
+    RTC_CHECK(!(ShouldSetupDtlsSrtp_n() && secure()))
+        << "Setting RTCP for DTLS/SRTP after the channel is secure "
         << "should never happen.";
   }
 
@@ -529,8 +539,7 @@ bool BaseChannel::IsReadyToSendMedia_n() const {
   // and we have had some form of connectivity.
   return enabled() && IsReceiveContentDirection(remote_content_direction_) &&
          IsSendContentDirection(local_content_direction_) &&
-         was_ever_writable() &&
-         (srtp_filter_.IsActive() || !ShouldSetupDtlsSrtp_n());
+         was_ever_writable() && (secure() || !ShouldSetupDtlsSrtp_n());
 }
 
 bool BaseChannel::SendPacket(rtc::CopyOnWriteBuffer* packet,
@@ -588,7 +597,10 @@ void BaseChannel::OnDtlsState(DtlsTransportInternal* transport,
   // TransportChannel) or when TransportChannel is attached after DTLS is
   // negotiated.
   if (state != DTLS_TRANSPORT_CONNECTED) {
-    srtp_filter_.ResetParams();
+    dtls_active_ = false;
+    if (srtp_transport_) {
+      srtp_transport_->ResetParams();
+    }
   }
 }
 
@@ -662,74 +674,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 (!secure() && 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 +689,16 @@ bool BaseChannel::SendPacket(bool rtcp,
     return false;
   }
 
+  if (secure()) {

[pthatcher, 2017/08/28 21:42:56]

I think making the shorter path the early return would be nicer:

if (!srtp_active()) {
  return rtp_transport_->...;
}

...
return srtp_transport->...;

[Zhi Huang, 2017/08/29 18:40:34]

Done.

+    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, options, flags);

[pthatcher, 2017/08/28 21:42:56]

I don't think it should be the responsibility of the BaseChannel to set the "I'm
really SRTP" flag.  That should be the job of the SrtpTransport.  I think
SrtpTransport should have a SendPacket which doesn't take the flags and then it
calls DtlsTransport with the flags.

In fact, it would be even better to have separate SendRtpPacket and
SendRtpPacket so that you wouldn't need to rtcp bool either.  

[Zhi Huang, 2017/08/29 18:40:34]

SrtpTransport doesn't interact with DtlsTransport directly and it relies on the
wrapped RtpTransport to manage the states (ready_to_send_ etc.) The RtpTransport
needs to have the capability to set the flags explicitly so we still need 
    SendPacket(bool rtcp,
               rtc::CopyOnWriteBuffer* packet,
               const rtc::PacketOptions& options,
               int flag);
in RtpTransportInternal and the SrtpTransport still needs to override this
method.

+  }
+
   // 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, options, PF_NORMAL);

[pthatcher, 2017/08/28 21:42:56]

And similarly here, it would make sense to call SendRtpPacket or SendRtcpPacket.
 And not pass in the flag, because it's not doing anything anyway.

[Zhi Huang, 2017/08/29 18:40:34]

Done.

 }
 
 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 (!secure() && 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
@@ -834,6 +756,10 @@ bool BaseChannel::PushdownLocalDescription(
   const ContentInfo* content_info = GetFirstContent(local_desc);
   const MediaContentDescription* content_desc =
       GetContentDescription(content_info);
+  if (IsInsecureRtp(content_desc->protocol())) {
+    RTC_DCHECK(!srtp_required_);

[Taylor Brandstetter, 2017/08/26 02:40:38]

This should be an error rather than a DCHECK since it's possible to hit, right?

[Zhi Huang, 2017/08/29 18:40:34]

I'll remove these for now.

+    srtp_transport_ = nullptr;

[Taylor Brandstetter, 2017/08/26 02:40:39]

This unsets the srtp_transport_ pointer, but rtp_transport_ is still really an
SrtpTransport, correct? So how does this work? Are there any tests that exercise
using plain RTP? I guess there are the ORTC tests, so I wonder if they pass (and
how).

EDIT: Nevermind, I see now. See later comment.

[Zhi Huang, 2017/08/29 18:40:33]

Done.

+  }
   if (content_desc && content_info && !content_info->rejected &&
       !SetLocalContent(content_desc, action, error_desc)) {
     LOG(LS_ERROR) << "Failure in SetLocalContent with action " << action;
@@ -848,6 +774,10 @@ bool BaseChannel::PushdownRemoteDescription(
   const ContentInfo* content_info = GetFirstContent(remote_desc);
   const MediaContentDescription* content_desc =
       GetContentDescription(content_info);
+  if (IsInsecureRtp(content_desc->protocol())) {
+    RTC_DCHECK(!srtp_required_);
+    srtp_transport_ = nullptr;
+  }
   if (content_desc && content_info && !content_info->rejected &&
       !SetRemoteContent(content_desc, action, error_desc)) {
     LOG(LS_ERROR) << "Failure in SetRemoteContent with action " << action;
@@ -995,17 +925,20 @@ bool BaseChannel::SetupDtlsSrtp_n(bool rtcp) {
     recv_key = &server_write_key;
   }
 
-  if (!srtp_filter_.IsActive()) {
+  if (!secure_dtls()) {
     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_);
+      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()));
+      dtls_active_ = ret;
     }
   } else {
     if (rtcp) {
@@ -1013,10 +946,10 @@ 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,
+      RTC_DCHECK(srtp_transport_);
+      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()));
     }
   }

[pthatcher, 2017/08/28 21:42:56]

If this is only used for changing the header extensions IDs, why not have
SetEncryptedHeaderExtensionIds do the update?

[Zhi Huang, 2017/08/29 18:40:34]

SetEncryptedHeaderExtensionIds just stores the updated ids and we need to call
UpdateRtpParams for new ids to take effect.

@@ -1024,14 +957,13 @@ bool BaseChannel::SetupDtlsSrtp_n(bool rtcp) {
   if (!ret) {
     LOG(LS_WARNING) << "DTLS-SRTP key installation failed";
   } else {
-    dtls_keyed_ = true;
     UpdateTransportOverhead();
   }
   return ret;
 }
 
 void BaseChannel::MaybeSetupDtlsSrtp_n() {
-  if (srtp_filter_.IsActive()) {
+  if (secure_dtls()) {
     return;
   }
 
@@ -1138,7 +1070,13 @@ bool BaseChannel::SetSrtp_n(const std::vector<CryptoParams>& cryptos,
   if (!ret) {
     return false;
   }
-  srtp_filter_.SetEncryptedHeaderExtensionIds(src, encrypted_extension_ids);
+
+  if (!srtp_transport_) {
+    LOG(LS_WARNING) << "Setting SRTP without an SrtpTransport.";

[Taylor Brandstetter, 2017/08/26 02:40:38]

It looks like this warning message would be logged for every offer/answer if not
using SRTP. Maybe it should only be logged if "cryptos" is nonempty.

[Zhi Huang, 2017/08/29 18:40:34]

Acknowledged.

+    return true;
+  }
+
+  srtp_transport_->SetEncryptedHeaderExtensionIds(src, encrypted_extension_ids);
   switch (action) {
     case CA_OFFER:
       // If DTLS is already active on the channel, we could be renegotiating
@@ -1164,10 +1102,33 @@ bool BaseChannel::SetSrtp_n(const std::vector<CryptoParams>& cryptos,
     default:
       break;
   }
+
+  // If setting an SDES answer succeeded, apply the negotiated parameters
+  // to the SRTP transport.
+  if ((action == CA_PRANSWER || action == CA_ANSWER) && !dtls && 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.";

[Taylor Brandstetter, 2017/08/26 02:40:38]

Is it even possible to hit this point? Won't "srtp_transport_" already be null,
from PushdownRemoteDescription?

[Zhi Huang, 2017/08/29 18:40:34]

If we still support upgrading rtp_transport to srtp_transport for now, then I
think this could be hit if the crypto is empty in answer.

+      if (action == CA_ANSWER) {
+        // 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();
+      }
+    }
+  }
+
   // Only update SRTP filter if using DTLS. SDES is handled internally
   // by the SRTP filter.
   // TODO(jbauch): Only update if encrypted extension ids have changed.
-  if (ret && dtls_keyed_ && rtp_dtls_transport_ &&
+  if (ret && secure_dtls() && rtp_dtls_transport_ &&
       rtp_dtls_transport_->dtls_state() == DTLS_TRANSPORT_CONNECTED) {
     bool rtcp = false;
     ret = SetupDtlsSrtp_n(rtcp);
@@ -1211,7 +1172,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()) {
@@ -1440,7 +1400,8 @@ void BaseChannel::MaybeCacheRtpAbsSendTimeHeaderExtension_w(
 
 void BaseChannel::CacheRtpAbsSendTimeHeaderExtension_n(
     int rtp_abs_sendtime_extn_id) {
-  rtp_abs_sendtime_extn_id_ = rtp_abs_sendtime_extn_id;
+  RTC_DCHECK(srtp_transport_);
+  srtp_transport_->CacheRtpAbsSendTimeHeaderExtension(rtp_abs_sendtime_extn_id);
 }
 
 void BaseChannel::OnMessage(rtc::Message *pmsg) {
@@ -1724,9 +1685,9 @@ int BaseChannel::GetTransportOverheadPerPacket() const {
           ? kTcpOverhaed
           : kUdpOverhaed;
 
-  if (secure()) {
+  if (secure_sdes()) {
     int srtp_overhead = 0;
-    if (srtp_filter_.GetSrtpOverhead(&srtp_overhead))
+    if (srtp_transport_->GetSrtpOverhead(&srtp_overhead))
       transport_overhead_per_packet += srtp_overhead;
   }