Adding TLS support to the TCP Client sockets.

Adding TLS support to the TCP Client sockets.
In case of WebRTC TLS socket will be created when application passes
TURNS url. TurnPort will request for creating TLS sockets.

In other instances TLS will be done using Pseudo-TLS, i.e. when TCPPort
creates ssltcp, it's actually a pseudo-tls.

TBR=vrk@chromium.org,jln@chromium.org

[juberti2, 2013-08-07 06:04:20]

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host.h (right):

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host.h:68: STATE_TLS_CONNECTING,
It seems confusing to have different meanings for OPEN depending on the socket
type. Suggest getting rid of TLS_OPEN and moving TLS_CONNECTING before OPEN;
OPEN can then always mean "ready".

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_tcp.cc:209: void
P2PSocketHostTcpBase::ProcessSSLConnectDone(int status) {
This code looks very similar to the code that is used after setting up SSLTCP.
Can these blocks be combined into a single function?

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_tcp.h (right):

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_tcp.h:65: void StartTls();
Suggest naming these similarly, e.g. ProcessTlsConnectDone

https://codereview.chromium.org/22452002/diff/8001/content/common/p2p_sockets.h
File content/common/p2p_sockets.h (right):

https://codereview.chromium.org/22452002/diff/8001/content/common/p2p_sockets...
content/common/p2p_sockets.h:22: P2P_SOCKET_STUN_TLS_CLIENT,
suggest making the non-STUN case first, as in the previous entries

[Mallinath (Gone from Chromium), 2013-08-07 23:48:19]

ptal

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host.h (right):

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host.h:68: STATE_TLS_CONNECTING,
On 2013/08/07 06:04:21, juberti2 wrote:
> It seems confusing to have different meanings for OPEN depending on the socket
> type. Suggest getting rid of TLS_OPEN and moving TLS_CONNECTING before OPEN;
> OPEN can then always mean "ready".

Done.

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_tcp.cc:209: void
P2PSocketHostTcpBase::ProcessSSLConnectDone(int status) {
On 2013/08/07 06:04:21, juberti2 wrote:
> This code looks very similar to the code that is used after setting up SSLTCP.
> Can these blocks be combined into a single function?

Done.

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_tcp.h (right):

https://codereview.chromium.org/22452002/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_tcp.h:65: void StartTls();
On 2013/08/07 06:04:21, juberti2 wrote:
> Suggest naming these similarly, e.g. ProcessTlsConnectDone

Done.

https://codereview.chromium.org/22452002/diff/8001/content/common/p2p_sockets.h
File content/common/p2p_sockets.h (right):

https://codereview.chromium.org/22452002/diff/8001/content/common/p2p_sockets...
content/common/p2p_sockets.h:22: P2P_SOCKET_STUN_TLS_CLIENT,
On 2013/08/07 06:04:21, juberti2 wrote:
> suggest making the non-STUN case first, as in the previous entries

Done.

[Mallinath (Gone from Chromium), 2013-08-08 22:01:41]

vrk - For content/renderer/media and
Sergey for everything else.

OWNERS, Please review, I need lgtm from you as well.

Thanks,
Mallinath

[juberti2, 2013-08-08 22:16:06]

https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host.h (right):

https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host.h:68: STATE_TLS_CONNECTING,
I was thinking TLS_CONNECTING would come before OPEN, which means "ready to send
and receive".

https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:177:
socket_.reset(socket_factory->CreateSSLClientSocket(
This looks like we throw away the open socket and create a new SSL socket, which
seems wrong. We should create the appropriate type of socket in the beginning.

[Mallinath (Gone from Chromium), 2013-08-08 22:25:59]

https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host.h (right):

https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host.h:68: STATE_TLS_CONNECTING,
On 2013/08/08 22:16:06, juberti2 wrote:
> I was thinking TLS_CONNECTING would come before OPEN, which means "ready to
send
> and receive".

Done.

https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:177:
socket_.reset(socket_factory->CreateSSLClientSocket(
I don't think we are throwing away the open socket. It's passed into
socket_handle(line 161). CreateSSLClientSocket takes ClientSocketHandle as
input.
 
On 2013/08/08 22:16:06, juberti2 wrote:
> This looks like we throw away the open socket and create a new SSL socket,
which
> seems wrong. We should create the appropriate type of socket in the beginning.

[Mallinath (Gone from Chromium), 2013-08-09 22:58:52]

On 2013/08/08 22:25:59, mallinath2 wrote:
>
https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
> File content/browser/renderer_host/p2p/socket_host.h (right):
> 
>
https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
> content/browser/renderer_host/p2p/socket_host.h:68: STATE_TLS_CONNECTING,
> On 2013/08/08 22:16:06, juberti2 wrote:
> > I was thinking TLS_CONNECTING would come before OPEN, which means "ready to
> send
> > and receive".
> 
> Done.
> 
>
https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
> File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):
> 
>
https://codereview.chromium.org/22452002/diff/24001/content/browser/renderer_...
> content/browser/renderer_host/p2p/socket_host_tcp.cc:177:
> socket_.reset(socket_factory->CreateSSLClientSocket(
> I don't think we are throwing away the open socket. It's passed into
> socket_handle(line 161). CreateSSLClientSocket takes ClientSocketHandle as
> input.
>  
> On 2013/08/08 22:16:06, juberti2 wrote:
> > This looks like we throw away the open socket and create a new SSL socket,
> which
> > seems wrong. We should create the appropriate type of socket in the
beginning.

Ping

[juberti2, 2013-08-09 23:49:26]

https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:134: if
(IsTlsClientSocket(type_)) {
I think it would be clearest to write this as below, to make it clear there are
3 different possible paths.

if (TLS) {
  state_ = tls_connecting
  StartTls
} else {
  if (pseudoTls) {
    reset socket
  }
  state_ = OPEN
  DoSendSocketCreateMessage
  DoRead
}

https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:190: return;
return; not needed

[Mallinath (Gone from Chromium), 2013-08-10 00:07:16]

On 2013/08/09 23:49:26, juberti2 wrote:
>
https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
> File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):
> 
>
https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
> content/browser/renderer_host/p2p/socket_host_tcp.cc:134: if
> (IsTlsClientSocket(type_)) {
> I think it would be clearest to write this as below, to make it clear there
are
> 3 different possible paths.
> 
> if (TLS) {
>   state_ = tls_connecting
>   StartTls
> } else {
>   if (pseudoTls) {
>     reset socket
>   }
>   state_ = OPEN
>   DoSendSocketCreateMessage
>   DoRead
> }
> 
>
https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
> content/browser/renderer_host/p2p/socket_host_tcp.cc:190: return;
> return; not needed

Did verified this with TCP, making sure I am not breaking existing TCP and Proxy
support. But this is not tested with real TLS server.

[Mallinath (Gone from Chromium), 2013-08-10 00:07:27]

https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:134: if
(IsTlsClientSocket(type_)) {
On 2013/08/09 23:49:26, juberti2 wrote:
> I think it would be clearest to write this as below, to make it clear there
are
> 3 different possible paths.
> 
> if (TLS) {
>   state_ = tls_connecting
>   StartTls
> } else {
>   if (pseudoTls) {
>     reset socket
>   }
>   state_ = OPEN
>   DoSendSocketCreateMessage
>   DoRead
> }

Done.

https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:190: return;
On 2013/08/09 23:49:26, juberti2 wrote:
> return; not needed

Done.

[juberti2, 2013-08-10 00:35:20]

On 2013/08/10 00:07:27, mallinath2 wrote:
>
https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
> File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):
> 
>
https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
> content/browser/renderer_host/p2p/socket_host_tcp.cc:134: if
> (IsTlsClientSocket(type_)) {
> On 2013/08/09 23:49:26, juberti2 wrote:
> > I think it would be clearest to write this as below, to make it clear there
> are
> > 3 different possible paths.
> > 
> > if (TLS) {
> >   state_ = tls_connecting
> >   StartTls
> > } else {
> >   if (pseudoTls) {
> >     reset socket
> >   }
> >   state_ = OPEN
> >   DoSendSocketCreateMessage
> >   DoRead
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_...
> content/browser/renderer_host/p2p/socket_host_tcp.cc:190: return;
> On 2013/08/09 23:49:26, juberti2 wrote:
> > return; not needed
> 
> Done.

lgtm. Suggest passing www.google.com:443 in as a TLS TURN server and you can see
in wireshark if it successfully sets up TLS and tells the TurnPort to start
sending data. It won't work, but you can at least verify the TLS part works.

[mallinath, 2013-08-10 00:43:21]

Great, I will try that.


On Fri, Aug 9, 2013 at 5:35 PM, <juberti@chromium.org> wrote:

> On 2013/08/10 00:07:27, mallinath2 wrote:
>
> https://codereview.chromium.**org/22452002/diff/30002/**
>
content/browser/renderer_host/**p2p/socket_host_tcp.cc<https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_host/p2p/socket_host_tcp.cc>
>
>> File content/browser/renderer_host/**p2p/socket_host_tcp.cc (right):
>>
>
>
> https://codereview.chromium.**org/22452002/diff/30002/**
>
content/browser/renderer_host/**p2p/socket_host_tcp.cc#**newcode134<https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_host/p2p/socket_host_tcp.cc#newcode134>
>
>> content/browser/renderer_host/**p2p/socket_host_tcp.cc:134: if
>> (IsTlsClientSocket(type_)) {
>> On 2013/08/09 23:49:26, juberti2 wrote:
>> > I think it would be clearest to write this as below, to make it clear
>> there
>> are
>> > 3 different possible paths.
>> >
>> > if (TLS) {
>> >   state_ = tls_connecting
>> >   StartTls
>> > } else {
>> >   if (pseudoTls) {
>> >     reset socket
>> >   }
>> >   state_ = OPEN
>> >   DoSendSocketCreateMessage
>> >   DoRead
>> > }
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.**org/22452002/diff/30002/**
>
content/browser/renderer_host/**p2p/socket_host_tcp.cc#**newcode190<https://codereview.chromium.org/22452002/diff/30002/content/browser/renderer_host/p2p/socket_host_tcp.cc#newcode190>
>
>> content/browser/renderer_host/**p2p/socket_host_tcp.cc:190: return;
>> On 2013/08/09 23:49:26, juberti2 wrote:
>> > return; not needed
>>
>
>  Done.
>>
>
> lgtm. Suggest passing www.google.com:443 in as a TLS TURN server and you
> can see
> in wireshark if it successfully sets up TLS and tells the TurnPort to start
> sending data. It won't work, but you can at least verify the TLS part
> works.
>
>
https://codereview.chromium.**org/22452002/<https://codereview.chromium.org/2...
>

[Sergey Ulanov, 2013-08-12 22:25:13]

lgtm

https://codereview.chromium.org/22452002/diff/53001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22452002/diff/53001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:153: if (state_ !=
STATE_TLS_CONNECTING) {
nit: You can just replace this block with a DCHECK(). This method is called only
from OnConnected() and never externally, so maybe you don't need this DCHECK at
all.

https://codereview.chromium.org/22452002/diff/53001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:184: base::MessageLoop*
message_loop = base::MessageLoop::current();
To post a task for current thread please use
ThreadTaskRunnerHandle()::Get()->PostTask().

https://codereview.chromium.org/22452002/diff/53001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:185: CHECK(message_loop);
Do you need this? the next line will crash anyway. In either case this sholdn't
be a CHECK(), not DCHECK() and you won't need it with
ThreadTaskRunnerHandle()::Get().

[Mallinath (Gone from Chromium), 2013-08-12 22:36:46]

https://codereview.chromium.org/22452002/diff/53001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22452002/diff/53001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:153: if (state_ !=
STATE_TLS_CONNECTING) {
On 2013/08/12 22:25:14, Sergey Ulanov wrote:
> nit: You can just replace this block with a DCHECK(). This method is called
only
> from OnConnected() and never externally, so maybe you don't need this DCHECK
at
> all.

Done.

https://codereview.chromium.org/22452002/diff/53001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:184: base::MessageLoop*
message_loop = base::MessageLoop::current();
On 2013/08/12 22:25:14, Sergey Ulanov wrote:
> To post a task for current thread please use
> ThreadTaskRunnerHandle()::Get()->PostTask().

Done.

https://codereview.chromium.org/22452002/diff/53001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:185: CHECK(message_loop);
On 2013/08/12 22:25:14, Sergey Ulanov wrote:
> Do you need this? the next line will crash anyway. In either case this
sholdn't
> be a CHECK(), not DCHECK() and you won't need it with
> ThreadTaskRunnerHandle()::Get().

Done.