Add disabled certificate check support to IceServer PeerConnection API.

webrtc/api/peerconnection.cc

Index: webrtc/api/peerconnection.cc
diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc
index 46bdec594a9c90fcd1630db748921237807902d2..c2f29d1fba1dd98db49b636532cac94ca4e26b11 100644
--- a/webrtc/api/peerconnection.cc
+++ b/webrtc/api/peerconnection.cc
@@ -284,7 +284,7 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
   int port = kDefaultStunPort;
   if (service_type == TURNS) {
     port = kDefaultStunTlsPort;
-    turn_transport_type = cricket::PROTO_TCP;
+    turn_transport_type = cricket::PROTO_TLS;
   }
 
   std::string address;
@@ -305,10 +305,15 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
       break;
     case TURN:
     case TURNS: {
-      bool secure = (service_type == TURNS);
+      cricket::ProtocolFlags flags = cricket::PROTO_FLAG_NONE;
+      if (server.tls_certificate_policy ==
+          PeerConnectionInterface::kTlsCertPolicyInsecureNoCheck) {
+        flags = (cricket::ProtocolFlags)(
+            flags | cricket::PROTO_FLAG_INSECURE_CERT_CHECK);
+      }
       turn_servers->push_back(
           cricket::RelayServerConfig(address, port, username, server.password,
-                                     turn_transport_type, secure));
+                                     turn_transport_type, flags));

[pthatcher1, 2016/12/07 21:29:35]

Attaching flags onto relay servers feels like the wrong way to do this.  

It should instead be a PortAllocator flag.  See all the other flags we have
here:
https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/portallocato....
 

[Taylor Brandstetter, 2016/12/08 01:36:40]

I disagree. "Verify certificate or not?" is fundamentally a property of an
individual TLS connection, not the whole PeerConnection/PortAllocator. I think
this is a more intuitive and flexible interface, and doesn't have any downsides
I can see.

[pthatcher1, 2016/12/08 02:36:13]

Perhaps I misunderstood the use case we're trying to support.   I thought the
point of the flag was to allow a given PeerConnection to not check TLS certs for
all TURN connections.  But you're saying you want to allow a PeerConnection to
allow some TURN servers to be checked and others to not be checked.   Is that
really a needed use case?  It seems odd to want to do that, and it adds more
complexity to the API and code.  Perhaps the benefit of that use case is worth
the cost.  But it doesn't seem so to me.

Also, where does the flag come from anyway?  It doesn't come from parsing the
TURN URL.  Is there going to be some other mechanism for passing in TURN server
configs that includes the policy?

On 2016/12/08 01:36:40, Taylor Brandstetter wrote:

[Taylor Brandstetter, 2016/12/08 18:51:39]

Maybe there's no obvious use case right now. But:

- It's a more intuitive API to me, since a flag that controls a property of the
connection to the TURN server is in the same place as the TURN server URL.
- The addition to the API surface is the same (one new flag in one structure).
- The complexity/maintenance cost is lower in my estimation (PortAllocator is a
really unfortunate part of the API, I'd like to redo it).
It comes from the ICE server structure in RTCConfiguration.

Even if we decide that doing this per-server isn't an API we want to support:
from an implementation perspective, I'd prefer for the flag to be on the
RelayServerConfig, and not use the PortAllocator flags.

[hnsl1, 2016/12/12 16:08:13]

Imagine a scenario where you have a RTCConfiguration TURNS server with a
sensitive TURN username and password which is expected to validate the TLS
certificate, and you also have a TURNS server with a non-sensitive TURN username
and password which is expected to have a self-signed TLS certificate. If you
moved this flag to PeerConnection/PortAllocator level you would force users to
either send a sensitive TURN username and password insecurely or not use them in
the same RTCConfiguration. That design would be strictly worse and shows that
this flags fundamentally an IceServer level configuration. I will opt to keep
this flag on IceServer and RelayServerConfig level.

And I'd say that yes, this is what we want to do and the above scenario is
already likely in the short-term today.

[pthatcher1, 2016/12/12 23:12:29]

OK, I think I understand the use case.  Let's talk about the API.  I think we
have a few options:

A.  Put it in the protocol, like TURN vs. TURNS:

server = {
  uri: "TURNSU://...",
}

B.  Put it in a URL parameter:

server = {
  uri: "TURNS://...?verify_cert=false",
}

C.  Put it in a separate field
server = {
  uri: "TURNS://...",
  verify_cert: false
}


D.  Put it in a flag
server = {
  uri: "TURNS://...",
  flags: DONT_VERIFY_CERT | X | Y
}



Out of these, I don't like D.  And C+D would be the most difficult to get into
the web API, while A+B would be the easiest.   I kind of like A, but it also
feels a bit hacky.  B feels like it fits into the existing structure the best.  

However, A+B do have one big problem: if you go get a TURN URI from a TURN
server it could be evil and give you a URI with that param on there and you
might not notice unless you go and check it, which is bad.

So, C seems the best, which is what you have, at least for the first half.  I'd
suggest you push that down into the RelayServerConfig as well as
tls_certificate_policy and not as a flags.  
 

       break;
     }
     case INVALID: