Allow remote fingerprint update during a call

talk/session/media/channel.cc

 /*
  * libjingle
  * Copyright 2004 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 159 matching lines @@
       transport_controller_(transport_controller),
       media_channel_(media_channel),
       content_name_(content_name),
       rtcp_transport_enabled_(rtcp),
       transport_channel_(nullptr),
       rtcp_transport_channel_(nullptr),
       enabled_(false),
       writable_(false),
       rtp_ready_to_send_(false),
       rtcp_ready_to_send_(false),
-      was_ever_writable_(false),
+      dtls_srtp_setup_pending_(false),
       local_content_direction_(MD_INACTIVE),
       remote_content_direction_(MD_INACTIVE),
       has_received_packet_(false),
       dtls_keyed_(false),
       secure_required_(false),
       rtp_abs_sendtime_extn_id_(-1) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   LOG(LS_INFO) << "Created channel for " << content_name;
 }
 
@@ skipping 80 matching lines @@
 void BaseChannel::set_transport_channel(TransportChannel* new_tc) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   TransportChannel* old_tc = transport_channel_;
   if (!old_tc && !new_tc) {
     // Nothing to do
     return;
   }
   ASSERT(old_tc != new_tc);
 
+  // When a TransportChannel is attached, we always assume there is something to
+  // do for DTLS/SRTP to prevent sending anything without encrypted.
+  dtls_srtp_setup_pending_ = true;

[pthatcher1, 2015/11/30 20:23:11]

This is kind of hard to read and understand.  In the case where we are writable
already, UpdateWritableState_w() below will cause SRTP to be setup.   In the
case where it's not, it won't be.  But that's not very clear.   It doesn't help
that this line is far away from that line.  

This is already on the worker thread, so no packets will be sent while we're in
this code block.  I think it would be safer to:

1.  Move this line down to write above UpdateWritableState_w() below.
2.  Rename it to "srtp_ready_ = false".  
3.  srtp_filter_.ResetParams(); here as well. 
4.  Put a comment indicating that UpdateWritableState_w will get SRTP ready if
it's writable, or that it will get it ready once it is writable.

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

+
   if (old_tc) {
     DisconnectFromTransportChannel(old_tc);
     transport_controller_->DestroyTransportChannel_w(
         transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP);
   }
 
   transport_channel_ = new_tc;
 
   if (new_tc) {
     ConnectToTransportChannel(new_tc);
@@ skipping 20 matching lines @@
 
   if (old_tc) {
     DisconnectFromTransportChannel(old_tc);
     transport_controller_->DestroyTransportChannel_w(
         transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
   }
 
   rtcp_transport_channel_ = new_tc;
 
   if (new_tc) {
+    // When a TransportChannel is attached, we always assume there is something
+    // to do for DTLS/SRTP to prevent sending anything without encrypted.
+    dtls_srtp_setup_pending_ = true;

[pthatcher1, 2015/11/30 20:23:11]

Same here as with set_transport_channel()

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

+
     ConnectToTransportChannel(new_tc);
     for (const auto& pair : rtcp_socket_options_) {
       new_tc->SetOption(pair.first, pair.second);
     }
   }
 
   // Update aggregate writable/ready-to-send state between RTP and RTCP upon
   // setting new channel
   UpdateWritableState_w();
   SetReadyToSend(true, new_tc && new_tc->writable());
 }
 
 void BaseChannel::ConnectToTransportChannel(TransportChannel* tc) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   tc->SignalWritableState.connect(this, &BaseChannel::OnWritableState);
   tc->SignalReadPacket.connect(this, &BaseChannel::OnChannelRead);
   tc->SignalReadyToSend.connect(this, &BaseChannel::OnReadyToSend);
+  tc->SignalDtlsState.connect(this, &BaseChannel::OnDtlsState);
 }
 
 void BaseChannel::DisconnectFromTransportChannel(TransportChannel* tc) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   tc->SignalWritableState.disconnect(this);
   tc->SignalReadPacket.disconnect(this);
   tc->SignalReadyToSend.disconnect(this);
+  tc->SignalDtlsState.disconnect(this);
 }
 
 bool BaseChannel::Enable(bool enable) {
   worker_thread_->Invoke<void>(Bind(
       enable ? &BaseChannel::EnableMedia_w : &BaseChannel::DisableMedia_w,
       this));
   return true;
 }
 
 bool BaseChannel::AddRecvStream(const StreamParams& sp) {
@@ skipping 52 matching lines @@
 }
 
 bool BaseChannel::IsReadyToReceive() const {
   // Receive data if we are enabled and have local content,
   return enabled() && IsReceiveContentDirection(local_content_direction_);
 }
 
 bool BaseChannel::IsReadyToSend() const {
   // Send outgoing data if we are enabled, have local and remote content,
   // and we have had some form of connectivity.
-  return enabled() &&
-         IsReceiveContentDirection(remote_content_direction_) &&
+  return enabled() && IsReceiveContentDirection(remote_content_direction_) &&
          IsSendContentDirection(local_content_direction_) &&
-         was_ever_writable();
+         !dtls_srtp_setup_pending();

[pthatcher1, 2015/11/30 20:23:11]

I think we need this to be something like:

was_ever_writable() && (srtp_ready() || !NeedSrtp())

to work correctly with SCTP data channels.  

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

 }
 
 bool BaseChannel::SendPacket(rtc::Buffer* packet,
                              const rtc::PacketOptions& options) {
   return SendPacket(false, packet, options);
 }
 
 bool BaseChannel::SendRtcp(rtc::Buffer* packet,
                            const rtc::PacketOptions& options) {
   return SendPacket(true, packet, options);
@@ skipping 34 matching lines @@
   bool rtcp = PacketIsRtcp(channel, data, len);
   rtc::Buffer packet(data, len);
   HandlePacket(rtcp, &packet, packet_time);
 }
 
 void BaseChannel::OnReadyToSend(TransportChannel* channel) {
   ASSERT(channel == transport_channel_ || channel == rtcp_transport_channel_);
   SetReadyToSend(channel == rtcp_transport_channel_, true);
 }
 
+void BaseChannel::OnDtlsState(TransportChannel* channel,
+                              DtlsTransportState state) {
+  if (!ShouldSetupDtlsSrtp()) {

[pthatcher1, 2015/11/30 20:23:11]

I think it would make it more readable to rename ShouldSetupDtlsSrtp to 
NeedSrtp().

[guoweis_webrtc, 2015/12/01 22:05:12]

NeedSrtp doesn't capture the Dtls concept.

+    dtls_srtp_setup_pending_ = false;

[pthatcher1, 2015/11/30 20:23:11]

I think a combination of (srtp_ready() || !NeedSrtp()) would make this line
unecessary.

+    return;
+  }
+
+  // Resetting the srtp filter as long as it's not the CONNECTED signal. For

[pthatcher1, 2015/11/30 20:23:11]

Resetting => Reset
as long as => if 

[pthatcher1, 2015/11/30 20:23:11]

CONNECTED signal => CONNECTED state

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

+  // CONNECTED signal, setting up DTLS-SRTP context is deferred to

[pthatcher1, 2015/11/30 20:23:11]

For CONNECTED signal => For the connected state.

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

+  // ChannelWritable_w to cover other scenarios like the whole channel is
+  // writable (not just this TransportChannel) or when TransportChannel is
+  // attached after DTLS is negotiated.
+  if (state != DTLS_TRANSPORT_CONNECTED) {
+    srtp_filter_.ResetParams();
+    dtls_srtp_setup_pending_ = true;
+  }
+}
+
 void BaseChannel::SetReadyToSend(bool rtcp, bool ready) {
   if (rtcp) {
     rtcp_ready_to_send_ = ready;
   } else {
     rtp_ready_to_send_ = ready;
   }
 
   if (rtp_ready_to_send_ &&
       // In the case of rtcp mux |rtcp_transport_channel_| will be null.
       (rtcp_ready_to_send_ || !rtcp_transport_channel_)) {
@@ skipping 267 matching lines @@
   if (transport_channel_ && transport_channel_->writable() &&
       (!rtcp_transport_channel_ || rtcp_transport_channel_->writable())) {
     ChannelWritable_w();
   } else {
     ChannelNotWritable_w();
   }
 }
 
 void BaseChannel::ChannelWritable_w() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
-  if (writable_)
+  if (writable_) {
     return;
+  }
 
   LOG(LS_INFO) << "Channel writable (" << content_name_ << ")"
-               << (was_ever_writable_ ? "" : " for the first time");
+               << (dtls_srtp_setup_pending_ ? "" : " for the first time");

[pthatcher1, 2015/11/30 20:23:11]

Now this isn't accurate any more.  Perhaps it would be easier to just use
was_ever_writable_ still.

 
   std::vector<ConnectionInfo> infos;
   transport_channel_->GetStats(&infos);
   for (std::vector<ConnectionInfo>::const_iterator it = infos.begin();
        it != infos.end(); ++it) {
     if (it->best_connection) {
       LOG(LS_INFO) << "Using " << it->local_candidate.ToSensitiveString()
                    << "->" << it->remote_candidate.ToSensitiveString();
       break;
     }
   }
 
-  // If we're doing DTLS-SRTP, now is the time.
-  if (!was_ever_writable_ && ShouldSetupDtlsSrtp()) {
-    if (!SetupDtlsSrtp(false)) {
-      SignalDtlsSetupFailure_w(false);
-      return;
-    }
-
-    if (rtcp_transport_channel_) {
-      if (!SetupDtlsSrtp(true)) {
-        SignalDtlsSetupFailure_w(true);
-        return;
-      }
-    }
-  }
-
-  was_ever_writable_ = true;
+  TrySetupDtlsSrtp_w();

[pthatcher1, 2015/11/30 20:23:11]

Why is this TrySetupDtlsSrtp_w() instead of SetupDtlsSrtp_w() or
MaybeSetupDtlsSrtp_w()?

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

   writable_ = true;
   ChangeState();
 }
 
 void BaseChannel::SignalDtlsSetupFailure_w(bool rtcp) {
   ASSERT(worker_thread() == rtc::Thread::Current());
   signaling_thread()->Invoke<void>(Bind(
       &BaseChannel::SignalDtlsSetupFailure_s, this, rtcp));
 }
 
@@ skipping 101 matching lines @@
   }
 
   if (!ret)
     LOG(LS_WARNING) << "DTLS-SRTP key installation failed";
   else
     dtls_keyed_ = true;
 
   return ret;
 }
 
+void BaseChannel::TrySetupDtlsSrtp_w() {
+  if (!dtls_srtp_setup_pending_) {
+    return;
+  }
+
+  if (!ShouldSetupDtlsSrtp()) {
+    dtls_srtp_setup_pending_ = false;
+    return;
+  }
+
+  if (!SetupDtlsSrtp(false)) {
+    SignalDtlsSetupFailure_w(false);
+    return;
+  }
+
+  if (rtcp_transport_channel_) {
+    if (!SetupDtlsSrtp(true)) {
+      SignalDtlsSetupFailure_w(true);
+      return;
+    }
+  }
+  dtls_srtp_setup_pending_ = false;
+}
+
 void BaseChannel::ChannelNotWritable_w() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (!writable_)
     return;
 
   LOG(LS_INFO) << "Channel not writable (" << content_name_ << ")";
   writable_ = false;
   ChangeState();
 }
 
@@ skipping 1350 matching lines @@
   return (data_channel_type_ == DCT_RTP);
 }
 
 void DataChannel::OnStreamClosedRemotely(uint32_t sid) {
   rtc::TypedMessageData<uint32_t>* message =
       new rtc::TypedMessageData<uint32_t>(sid);
   signaling_thread()->Post(this, MSG_STREAMCLOSEDREMOTELY, message);
 }
 
 }  // namespace cricket