Require ECDHE cipher in remoting client.

Require ECDHE cipher in remoting client.

Also added support for enable_ecdhe flag in SSLClientSocketOpenSSL and
SSLClientSocketNSS. It's not really needed with NSS as the client
is only compiled with BoringSSL, but added it anyway for consistency.

BUG=481163
Committed: https://crrev.com/0144249b934d0869a20a69931e6ec261c5cb27da
Cr-Commit-Position: refs/heads/master@{#334684}

[Sergey Ulanov, 2015-06-15 23:21:13]

sergeyu@chromium.org changed reviewers:
+ davidben@chromium.org

[Sergey Ulanov, 2015-06-15 23:23:36]

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:768: std::string
command("DEFAULT:!SHA256:!SHA384:!AESGCM+AES256:!aPSK");
This is an unrelated cleanup for consistency with ssl_server_socket_openssl.cc
as unsecure ciphers are always disabled in BoringSSL.

[davidben, 2015-06-16 19:18:21]

lgtm

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2768: // Require forward security by
iterating over the cipher suites and
Nit: Maybe just:

  // Iterate over the cipher suites and disable all those that don't use ECDHE.

Since DHE nominally also gets us forward secrecy, but (a) DHE is very sad
because of the group size issue and (b) isn't sufficient for our purposes here
anyway.

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:768: std::string
command("DEFAULT:!SHA256:!SHA384:!AESGCM+AES256:!aPSK");
On 2015/06/15 23:23:36, Sergey Ulanov wrote:
> This is an unrelated cleanup for consistency with ssl_server_socket_openssl.cc
> as unsecure ciphers are always disabled in BoringSSL.

Oof, kept meaning to do that and forgot. Thanks! I suppose it's splittable but
oh well. Didn't end up happening last time. :-)

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:3110:
SpawnedTestServer::SSLOptions::CERT_MISMATCHED_NAME);
This should just be the parameterless one I think. (You don't get far enough to
care about the certificate anyway.)

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:3125: rv = callback.WaitForResult();
Nit: This can be
   int rv = ....(callback.callback());
   rv = callback.GetResult(rv);
Or even
   EXPECT_EQ(OK, callback.GetResult(....(callback.callback())));

I think most of these tests predate GetResult. :-) Or were copied from ones that
did.

[Sergey Ulanov, 2015-06-16 19:29:08]

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2768: // Require forward security by
iterating over the cipher suites and
On 2015/06/16 at 19:18:21, David Benjamin wrote:
> Nit: Maybe just:
> 
>   // Iterate over the cipher suites and disable all those that don't use
ECDHE.
> 
> Since DHE nominally also gets us forward secrecy, but (a) DHE is very sad
because of the group size issue and (b) isn't sufficient for our purposes here
anyway.

Done. Also updated the same comment in ssl_server_socket_nss.cc

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:3110:
SpawnedTestServer::SSLOptions::CERT_MISMATCHED_NAME);
On 2015/06/16 at 19:18:21, David Benjamin wrote:
> This should just be the parameterless one I think. (You don't get far enough
to care about the certificate anyway.)

Done

https://codereview.chromium.org/1191623002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:3125: rv = callback.WaitForResult();
On 2015/06/16 19:18:21, David Benjamin wrote:
> Nit: This can be
>    int rv = ....(callback.callback());
>    rv = callback.GetResult(rv);
> Or even
>    EXPECT_EQ(OK, callback.GetResult(....(callback.callback())));
> 
> I think most of these tests predate GetResult. :-) Or were copied from ones
that
> did.

Done.

[Sergey Ulanov, 2015-06-16 19:29:33]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-06-16 19:29:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1191623002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-06-16 19:32:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191623002/20001

[commit-bot: I haz the power, 2015-06-16 20:43:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-16 20:45:07]

Patchset 2 (id:??) landed as
https://crrev.com/0144249b934d0869a20a69931e6ec261c5cb27da
Cr-Commit-Position: refs/heads/master@{#334684}