Move channel id tests up from OpenSSL and update channelid version.

Move channel id tests up from OpenSSL and update channelid version.

NSS (and soon OpenSSL) runs the new ChannelID version 30032. This patch
moves the simple ChannelID tests from OpenSSL unittests to the general
ssl_client_socket_unittest.cc and updates the channelid version in
TLS Lite.

TLS Lite is not updated with ChannelId resumption support since
ChannelID signature checking is not yet implemented in TLS Lite.

ChannelID Signature checks, along with ChannelId resumption support
and  ChannelID resumption tests should be implemented in further work.

This CL will also roll third_party/openssl:

OpenSSL: rolls DEPS 267648->269063 

Takes in following change:

r269063 | haavardm@opera.com | 2014-05-08 17:48:32 +0200 (to., 08 mai 2014) | 8 lines

New tls channel id version for OpenSSL

New tls channel id version extracted from patch 0015-channelid.patch attached to http://crbug.com/366961.

BUG=366961
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270683

[haavardm, 2014-05-06 13:29:49]

This patch is needed to keep ChannelId tests succeeding after updating OpenSSL
to the new ChannelId version. It will also allow the tests to run for NSS.

I will sync the commit of this CL with rolling out new OpenSSL version.

wtc: Please review the net part.

agl: Please review the TLS Lite part. I only updated the channel_id constant.
Since the current implementation does not check the signature, I did not
implement the ChannelID resumption logic (the current tests doesn't test it
anyway).

rsleevi: FYI

[wtc, 2014-05-06 17:56:24]

Patch set 1 LGTM. Note that there is a bug in the unit tests, marked with "BUG"
in my review comments.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl_unittest.cc:40: #include
"net/ssl/server_bound_cert_service.h"

Some of the headers should be removed, for example, *server_bound_cert*.h.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl_unittest.cc:196:
scoped_ptr<ServerBoundCertService> cert_service_;

Delete the cert_service_ member.

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

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:746: class SSLClientSocketChannelIDTest
: public SSLClientSocketTest {

Nit: I think it is a good idea to copy the ConnectToTestServer and
CreateAndConnectSSLClientSocket methods to this class. Right now the code of
these two methods is inlined in the two unit tests.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:761: scoped_ptr<ServerBoundCertService>
cert_service_;

The cert_service_ member should be private.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2420: SpawnedTestServer::SSLOptions
ssl_options;

Remove the ssl_options variable in both unit tests. It is not used.
Alternatively, pass ssl_options as the second argument to the test_server
constructor.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2428: new TCPClientSocket(addr, NULL,
NetLog::Source()));

Just wanted to confirm that you intended to pass NULL as the second argument to
the TCPClientSocket constructor. I saw some tests that declare a local variable
"CapturingNetLog log;" and pass &log. I guess these two tests don't need that.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2439: transport.Pass(),
test_server.host_port_pair(), kDefaultSSLConfig));

1. BUG: the variable ssl_config is set but not used. ssl_config instead of
kDefaultSSLConfig should be passed here.

Did the test pass in spite of this bug?

2. The FailingChannelID test has the same bug.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2441: rv =
sock->Connect(callback.callback());

Just wanted to point out that you omitted them check

    if (sock_->IsConnected()) {
      LOG(ERROR) << "SSL Socket prematurely connected";
      return false;
    }

in the original code. I think this is OK.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2444: EXPECT_EQ(OK, rv);

Nit: fix indentation.

https://codereview.chromium.org/263213005/diff/1/third_party/tlslite/patches/...
File third_party/tlslite/patches/channel_id.patch (right):

https://codereview.chromium.org/263213005/diff/1/third_party/tlslite/patches/...
third_party/tlslite/patches/channel_id.patch:154: +            self.channel_id =
result.channel_id_key

Haavard: you're right. We only record result.channel_id_key but don't do
anything with result.channel_id_proof.

[Ryan Sleevi, 2014-05-06 18:45:26]

lgtm

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

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:748: void EnabledChannelID() {
these should be "EnableChannelID()" / "EnableFailingChannelID" (note the
present, active voice rather than past-tense)

[haavardm, 2014-05-07 13:53:43]

Wtc: I moved the ConnectToTestServer and CreateAndConnectSSLClientSocket methods
as you suggested, although I moved them to SSLClientSocketTest, so that other
tests than ChannelId also can use them.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl_unittest.cc:40: #include
"net/ssl/server_bound_cert_service.h"
On 2014/05/06 17:56:24, wtc wrote:
> 
> Some of the headers should be removed, for example, *server_bound_cert*.h.

Done.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl_unittest.cc:196:
scoped_ptr<ServerBoundCertService> cert_service_;
On 2014/05/06 17:56:24, wtc wrote:
> 
> Delete the cert_service_ member.

Done.

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

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:746: class SSLClientSocketChannelIDTest
: public SSLClientSocketTest {
On 2014/05/06 17:56:24, wtc wrote:
> 
> Nit: I think it is a good idea to copy the ConnectToTestServer and
> CreateAndConnectSSLClientSocket methods to this class. Right now the code of
> these two methods is inlined in the two unit tests.

I did that to follow the code pattern of the rest of this file. In that case it
would make more sense to add it to SSLClientSocketTest.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:748: void EnabledChannelID() {
On 2014/05/06 18:45:26, Ryan Sleevi wrote:
> these should be "EnableChannelID()" / "EnableFailingChannelID" (note the
> present, active voice rather than past-tense)

Done.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2420: SpawnedTestServer::SSLOptions
ssl_options;
On 2014/05/06 17:56:24, wtc wrote:
> 
> Remove the ssl_options variable in both unit tests. It is not used.
> Alternatively, pass ssl_options as the second argument to the test_server
> constructor.

Done.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2428: new TCPClientSocket(addr, NULL,
NetLog::Source()));
On 2014/05/06 17:56:24, wtc wrote:
> 
> Just wanted to confirm that you intended to pass NULL as the second argument
to
> the TCPClientSocket constructor. I saw some tests that declare a local
variable
> "CapturingNetLog log;" and pass &log. I guess these two tests don't need that.

Yes, that was as intended. Some tests only pass if certain events are present in
the log. That is not needed for these tests.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2439: transport.Pass(),
test_server.host_port_pair(), kDefaultSSLConfig));
On 2014/05/06 17:56:24, wtc wrote:
> 
> 1. BUG: the variable ssl_config is set but not used. ssl_config instead of
> kDefaultSSLConfig should be passed here.
> 

Hm, the evils of copy and paste.. Yes the tests passed. The simple reason is
that channel_id_enabled is on by default.

I will set it explicitly anyway, in case the default value changes.

Done.
> Did the test pass in spite of this bug?
> 
> 2. The FailingChannelID test has the same bug.

https://codereview.chromium.org/263213005/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_unittest.cc:2444: EXPECT_EQ(OK, rv);
On 2014/05/06 17:56:24, wtc wrote:
> 
> Nit: fix indentation.

Done.

[agl, 2014-05-07 15:13:20]

Ready for CQ?

[haavardm, 2014-05-07 16:42:57]

On 2014/05/07 15:13:20, agl wrote:
> Ready for CQ?

I would assume so, if there are no new comments to the latest patch?

I planned to commit the corresponding openssl patch to third_party/openssl first
(https://codereview.chromium.org/259963009/), and then add the DEPS roll in this
patch before committing. This way, all tests should pass.

[haavardm, 2014-05-07 18:10:19]

On 2014/05/07 16:42:57, haavardm wrote:
> On 2014/05/07 15:13:20, agl wrote:
> > Ready for CQ?
> 
> I would assume so, if there are no new comments to the latest patch?
> 
> I planned to commit the corresponding openssl patch to third_party/openssl
first
> (https://codereview.chromium.org/259963009/), and then add the DEPS roll in
this
> patch before committing. This way, all tests should pass.

Heh, I forgot to follow the git cl upload completely, so it never got uploaded.
Here it
is. PTAL.

[wtc, 2014-05-07 19:19:01]

Patch set 2 LGTM.

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:654: }

The indentation of lines 613-654) is off by one. Line 612 (a comment) is
correctly indented.

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:661: CapturingNetLog log_;

Do you think log_ needs to be a protected member? Right now it doesn't seem to
be accessed by unit tests.

[wtc, 2014-05-07 19:23:24]

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:612: // Connect to a HTTPS test server.

Nit: it would be good to point out that this only connects TCP and doesn't do
the SSL handshake.

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:643: bool
CreateAndConnectSSLClientSocket(SSLConfig& ssl_config, int* result) {

Nit: this method is closely relaed to the CreateSSLClientSocket method, so I'd
suggest we put it immediately after CreateSSLClientSocket. We can do this by
listing ConnectToTestServer first, in this order:

  ConnectToTestServer
  CreateSSLClientSocket
  CreateAndConnectSSLClientSocket

[haavardm, 2014-05-07 19:42:59]

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:612: // Connect to a HTTPS test server.
On 2014/05/07 19:23:24, wtc wrote:
> 
> Nit: it would be good to point out that this only connects TCP and doesn't do
> the SSL handshake.

Done.

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:643: bool
CreateAndConnectSSLClientSocket(SSLConfig& ssl_config, int* result) {
On 2014/05/07 19:23:24, wtc wrote:
> 
> Nit: this method is closely relaed to the CreateSSLClientSocket method, so I'd
> suggest we put it immediately after CreateSSLClientSocket. We can do this by
> listing ConnectToTestServer first, in this order:
> 
>   ConnectToTestServer
>   CreateSSLClientSocket
>   CreateAndConnectSSLClientSocket

Done.

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:654: }
On 2014/05/07 19:19:01, wtc wrote:
> 
> The indentation of lines 613-654) is off by one. Line 612 (a comment) is
> correctly indented.

Done.

https://codereview.chromium.org/263213005/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:661: CapturingNetLog log_;
On 2014/05/07 19:19:01, wtc wrote:
> 
> Do you think log_ needs to be a protected member? Right now it doesn't seem to
> be accessed by unit tests.

Many of the the tests below could easily be converted to use
ConnectToTestServer() and CreateAndConnectSSLClientSocket(), and they would need
access to the log_ member. I was about to do that, but found it to be scope
creep for this CL. Keeping it makes the conversion easier in a possible follow
up CL.

[haavardm, 2014-05-08 16:08:54]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2014-05-08 16:12:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haavardm@opera.com/263213005/100001

[wtc, 2014-05-08 17:54:54]

Patch set 5 LGTM.

[commit-bot: I haz the power, 2014-05-08 20:13:39]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[haavardm, 2014-05-08 20:39:17]

On 2014/05/08 20:13:39, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> Please consider checking whether the failures are real,
> and report flakes to mailto:chrome-troopers@google.com.
> The failing builders are:
>   mac_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
>   win_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
>   win_chromium_x64_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

Two of those errors are real. For some reason
SSLClientSocketChannelIDTest.FailingChannelID fails
on Mac and Windows 64. 

Linux with NSS succeeds, which is why I didn't see this.

For some reason ERR_SSL_PROTOCOL_ERROR is returned instead of the expected
ERR_UNEXPECTED.
Is there any reason ChannelId would behave differently on Linux than Mac and
Windows?

[haavardm, 2014-05-08 21:20:06]

On 2014/05/08 20:39:17, haavardm wrote:
> On 2014/05/08 20:13:39, I haz the power (commit-bot) wrote:
> > FYI, CQ is re-trying this CL (attempt #1).
> > Please consider checking whether the failures are real,
> > and report flakes to mailto:chrome-troopers@google.com.
> > The failing builders are:
> >   mac_chromium_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
> >   win_chromium_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
> >   win_chromium_x64_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
> 
> Two of those errors are real. For some reason
> SSLClientSocketChannelIDTest.FailingChannelID fails
> on Mac and Windows 64. 
> 
> Linux with NSS succeeds, which is why I didn't see this.
> 
> For some reason ERR_SSL_PROTOCOL_ERROR is returned instead of the expected
> ERR_UNEXPECTED.
> Is there any reason ChannelId would behave differently on Linux than Mac and
> Windows?

The reason seems to be different code paths due to threading.

On mac ssl_client_socket_nss.cc:2311 (in ClientChannelIDHandler())
core->OnNetworkTaskRunner() is true, while false on linux. 

The result is that on linux ERR_UNEXPECTED is reported to
Core::OnHandshakeIOComplete, while on Mac 
ERR_UNEXPECTED is turned into SECFailure when returning from
ClientChannelIDHandler. This is finally
reported back to the test as ERR_SSL_PROTOCOL_ERROR.

Different errors codes due to differences in how the threading ends up, seems
like a bug to me.

[commit-bot: I haz the power, 2014-05-08 21:37:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-08 21:37:16]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Ryan Sleevi, 2014-05-08 22:02:23]

On 2014/05/08 21:20:06, haavardm wrote:
> On 2014/05/08 20:39:17, haavardm wrote:
> > On 2014/05/08 20:13:39, I haz the power (commit-bot) wrote:
> > > FYI, CQ is re-trying this CL (attempt #1).
> > > Please consider checking whether the failures are real,
> > > and report flakes to mailto:chrome-troopers@google.com.
> > > The failing builders are:
> > >   mac_chromium_rel on tryserver.chromium
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
> > >   win_chromium_rel on tryserver.chromium
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
> > >   win_chromium_x64_rel on tryserver.chromium
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
> > 
> > Two of those errors are real. For some reason
> > SSLClientSocketChannelIDTest.FailingChannelID fails
> > on Mac and Windows 64. 
> > 
> > Linux with NSS succeeds, which is why I didn't see this.
> > 
> > For some reason ERR_SSL_PROTOCOL_ERROR is returned instead of the expected
> > ERR_UNEXPECTED.
> > Is there any reason ChannelId would behave differently on Linux than Mac and
> > Windows?
> 
> The reason seems to be different code paths due to threading.
> 
> On mac ssl_client_socket_nss.cc:2311 (in ClientChannelIDHandler())
> core->OnNetworkTaskRunner() is true, while false on linux. 
> 
> The result is that on linux ERR_UNEXPECTED is reported to
> Core::OnHandshakeIOComplete, while on Mac 
> ERR_UNEXPECTED is turned into SECFailure when returning from
> ClientChannelIDHandler. This is finally
> reported back to the test as ERR_SSL_PROTOCOL_ERROR.
> 
> Different errors codes due to differences in how the threading ends up, seems
> like a bug to me.

As to the difference in behaviours - yes, this is non-ideal.

It seems we have two options, although I'm not 100% sure on either of them
Option 1: Change the Mac code to always return SECWouldBlock, with an additional
tracking flag to know that it wasn't "really" ERR_IO_PENDING in the handshake,
and then replace the error code with the one returned by the DB layer.

Option 2: Force the Linux code to ignore the error code, and then force itself
through to the NSS functions and induce an 'official' NSS error, which will end
up getting mapped to ERR_SSL_PROTOCOL_ERROR

For Option 2,

I *think* that by removing the short-circuit in
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...
and restructuring the code, it should work

Replace 1891-1892 with
if (result != OK)
  return MapNSSError(SSL_ERROR_GET_CHANNEL_ID_FAILED);

and 1897-1898 with
if (result != OK)
  return MapNSSError(SSL_ERROR_GET_CHANNEL_ID_FAILED);

This forces an information loss to go on - we ignore the result code we have,
and instead force it into the exact same pattern that NSS uses when the
synchronous callback fails with SECFailure.

[haavardm, 2014-05-14 08:26:02]

Currently at BlinkOn Zürich, so answering a little slow.

https://codereview.chromium.org/263213005/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/263213005/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2503: EXPECT_EQ(ERR_UNEXPECTED, rv);

An alternative quick fix for now could simply be to expect any error code and
add comment referring to a chromium bug explaining the issue.

// Due to differences in threading linux returns ERR_UNEXPECTED while Mac and
Windows returns ERR_PROTOCOL_ERROR, see bug xxx.
EXPECT_NE(OK, RV);

Then I can fix the mac/linux/windows error code differences in a follow up CL. 

Would that be acceptable, or do you prefer I fix this in this CL?

[wtc, 2014-05-14 16:51:07]

https://codereview.chromium.org/263213005/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/263213005/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2503: EXPECT_EQ(ERR_UNEXPECTED, rv);

Unless the fix is straightforward, I am fine with filing a bug report
and fixing this in a follow-up CL. Nit: the comment should say
"TODO(haavardm): ...". See the Style Guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#TODO_Comments

[haavardm, 2014-05-15 09:25:21]

On 2014/05/14 16:51:07, wtc wrote:
> Unless the fix is straightforward, I am fine with filing a bug report
> and fixing this in a follow-up CL. 

I don't think this is very difficult, but could generate a few more round trips.
It would be nice to get this one committed.

[haavardm, 2014-05-15 12:10:56]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2014-05-15 12:11:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haavardm@opera.com/263213005/120001

[commit-bot: I haz the power, 2014-05-15 13:24:20]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-15 16:30:40]

Change committed as 270683