Fix link error when compiling with OpenSSL while using OS certs.

Fix link error when compiling with OpenSSL while using OS certs.

Turn off OpenSSL client auth tests when use_openssl_certs==0.

OS client certifiates other than OpenSSL's struct x509 are not
yet supported for the OpenSSL integration.


BUG=None
TEST=None

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

[haavardm, 2014-04-10 13:17:41]

This patch was meant to be a part of  https://codereview.chromium.org/206453002/
but somehow fell out during the review.

[wtc, 2014-04-10 14:59:04]

Patch set 1 LGTM. Please try to save the ChannelID tests. It will require some
refactoring. Thanks.

Nit: In the CL's description, change "is not yet" to "are not yet".

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

https://codereview.chromium.org/233133002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl_unittest.cc:69: class
FailingServerBoundCertStore : public ServerBoundCertStore {

This class is used by the two ChannelID tests. We should be able to use it when
USE_OPENSSL_CERTS is not defined. I think it requires spltting
SSLClientSocketOpenSSLClientAuthTest into two classes:

- SSLClientSocketOpenSSLClientAuthTest, without the ChannelID stuff.
- A new SSLClientSocketOpenSSLChannelIDTest, for the ChannelID stuff.

https://codereview.chromium.org/233133002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl_unittest.cc:328:
TEST_F(SSLClientSocketOpenSSLClientAuthTest, SendChannelID) {

Is it necessary to disable the two ChannelID tests? They are no longer
implemented as client certificates.

[haavardm, 2014-04-10 15:16:12]

Thanks for quick review. I'll try to save those to tests.

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

https://codereview.chromium.org/233133002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl_unittest.cc:69: class
FailingServerBoundCertStore : public ServerBoundCertStore {
Why are the channel id tests in the openssl layer? couldn't they be moved to
ssl_client_socket_unittest.cc?

On 2014/04/10 14:59:05, wtc wrote:
> 
> This class is used by the two ChannelID tests. We should be able to use it
when
> USE_OPENSSL_CERTS is not defined. I think it requires spltting
> SSLClientSocketOpenSSLClientAuthTest into two classes:
> 
> - SSLClientSocketOpenSSLClientAuthTest, without the ChannelID stuff.
> - A new SSLClientSocketOpenSSLChannelIDTest, for the ChannelID stuff.

[wtc, 2014-04-10 15:28:59]

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

https://codereview.chromium.org/233133002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl_unittest.cc:69: class
FailingServerBoundCertStore : public ServerBoundCertStore {

On 2014/04/10 15:16:12, haavardm wrote:
> Why are the channel id tests in the openssl layer?

Channel ID was originally called origin-bound certificates and used the SSL/TLS
client authentication mechanism. I suspect that's why the Channel ID tests are
in this file.

> couldn't they be moved to ssl_client_socket_unittest.cc?

I think you are right. I took a quick look at the Channel ID code in this file,
and didn't see anything OpenSSL-specific. Moving these tests to
ssl_client_socket_unittest.cc is a good idea!

[haavardm, 2014-04-10 20:04:32]

As ryan suggested, I uploaded work in progress.

The channelId tests succeed with OpenSSL, but both fail with NSS. Not sure what
happens. From debugging I can see that the channel id extension is not
negotiated in ssl3_HandleServerHello() at ssl3con.c:6725.

[haavardm, 2014-04-22 15:06:29]

Back from holiday, so had a new look at this.

The failure is that NSS defines the channel id extension to be 30032, while
OpenSSL and the test server (src/third_party/tlslite/tlslite/constants.py)
defines it to be 30031. Changing TLS lite to use 30032 "fixes" the issue for
NSS.

As far as I can see rfc5246 still have this as TBD, but I assume 30032 is the
correct number. If so, the failure is in Chromium's OpenSSL lib and in TLS lite.
That the channel id tests do not run for NSS explains why the test error was not
discovered. Also, it seems to suggest that tls channel id does not work on
Android (if not the Google server accepts both 30031 and 30032).

This CL is now about more than fixing a linking error for OpenSSL + OS X. I
suggest to go back to patch set 1 (will only temporarily remove channel id tests
from OS X) and then fix the channel id in a separate CL. 

Does this sound OK?

On 2014/04/10 20:04:32, haavardm wrote:
> As ryan suggested, I uploaded work in progress.
> 
> The channelId tests succeed with OpenSSL, but both fail with NSS. Not sure
what
> happens. From debugging I can see that the channel id extension is not
> negotiated in ssl3_HandleServerHello() at ssl3con.c:6725.

[Ryan Sleevi, 2014-04-22 17:49:26]

On 2014/04/22 15:06:29, haavardm wrote:
> Back from holiday, so had a new look at this.
> 
> The failure is that NSS defines the channel id extension to be 30032, while
> OpenSSL and the test server (src/third_party/tlslite/tlslite/constants.py)
> defines it to be 30031. Changing TLS lite to use 30032 "fixes" the issue for
> NSS.
> 
> As far as I can see rfc5246 still have this as TBD, but I assume 30032 is the
> correct number. If so, the failure is in Chromium's OpenSSL lib and in TLS
lite.
> That the channel id tests do not run for NSS explains why the test error was
not
> discovered. Also, it seems to suggest that tls channel id does not work on
> Android (if not the Google server accepts both 30031 and 30032).

Er, yeah, looks like OpenSSL was missed with
http://src.chromium.org/viewvc/chrome?view=revision&revision=235826 

> 
> This CL is now about more than fixing a linking error for OpenSSL + OS X. I
> suggest to go back to patch set 1 (will only temporarily remove channel id
tests
> from OS X) and then fix the channel id in a separate CL. 
> 
> Does this sound OK?

Yeah, sounds good.

> 
> On 2014/04/10 20:04:32, haavardm wrote:
> > As ryan suggested, I uploaded work in progress.
> > 
> > The channelId tests succeed with OpenSSL, but both fail with NSS. Not sure
> what
> > happens. From debugging I can see that the channel id extension is not
> > negotiated in ssl3_HandleServerHello() at ssl3con.c:6725.

[wtc, 2014-04-22 18:30:40]

On 2014/04/22 17:49:26, Ryan Sleevi wrote:
>
> Er, yeah, looks like OpenSSL was missed with
> http://src.chromium.org/viewvc/chrome?view=revision&revision=235826

Yes. 30032 is a new revision of Channel ID. It is not backward compatible, which
is why it uses a different extension value.

30032 differs from 30031 only in session resumption handshakes. I guess that is
why Haavard reported that changing TLS Lite to use 30032 fixed the problem --
the Channel ID tests probably only do full handshake.

[haavardm, 2014-04-22 19:05:52]

On 2014/04/22 18:30:40, wtc wrote:
> On 2014/04/22 17:49:26, Ryan Sleevi wrote:
> >
> > Er, yeah, looks like OpenSSL was missed with
> > http://src.chromium.org/viewvc/chrome?view=revision&revision=235826
> 

I deleted patch set 2. I can fix the other issues and move the channel id tests
to
ssl_client_socket_unittest.cc in a different CL. Since both third_party/openssl
and third_party/tlslite
needs to be fixed some kind of sync'd release is needed to keep the tests
running on Android.



> Yes. 30032 is a new revision of Channel ID. It is not backward compatible,
which
> is why it uses a different extension value.
> 
> 30032 differs from 30031 only in session resumption handshakes. 

Not sure what you mean here, but as I understand it the channel id extension 
is sent on client/server hello for both full and abbreviated handshake.

I guess that is
> why Haavard reported that changing TLS Lite to use 30032 fixed the problem --
> the Channel ID tests probably only do full handshake.

[Ryan Sleevi, 2014-04-22 19:10:37]

On Tue, Apr 22, 2014 at 12:05 PM, <haavardm@opera.com> wrote:

> On 2014/04/22 18:30:40, wtc wrote:
>
>> On 2014/04/22 17:49:26, Ryan Sleevi wrote:
>> >
>> > Er, yeah, looks like OpenSSL was missed with
>> > http://src.chromium.org/viewvc/chrome?view=revision&revision=235826
>>
>
>
> I deleted patch set 2. I can fix the other issues and move the channel id
> tests
> to
> ssl_client_socket_unittest.cc in a different CL. Since both
> third_party/openssl
> and third_party/tlslite
> needs to be fixed some kind of sync'd release is needed to keep the tests
> running on Android.
>
>
>
>
>  Yes. 30032 is a new revision of Channel ID. It is not backward compatible,
>>
> which
>
>> is why it uses a different extension value.
>>
>
>  30032 differs from 30031 only in session resumption handshakes.
>>
>
> Not sure what you mean here, but as I understand it the channel id
> extension
> is sent on client/server hello for both full and abbreviated handshake.


Just that 30032 is wire-different than 30031, but only in the resumption
handshake case.

Regardless, we can defer that fixing up.


>
>
> I guess that is
>
>> why Haavard reported that changing TLS Lite to use 30032 fixed the
>> problem --
>> the Channel ID tests probably only do full handshake.
>>
>
>
>
>
> https://codereview.chromium.org/233133002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haavardm, 2014-04-22 19:24:29]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2014-04-22 19:25:53]

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

[haavardm, 2014-04-22 19:28:23]

On 2014/04/22 19:10:37, Ryan Sleevi wrote:
> On Tue, Apr 22, 2014 at 12:05 PM, <mailto:haavardm@opera.com> wrote:
> 
> > On 2014/04/22 18:30:40, wtc wrote:
> >
> >> On 2014/04/22 17:49:26, Ryan Sleevi wrote:
> >> >
> >> > Er, yeah, looks like OpenSSL was missed with
> >> > http://src.chromium.org/viewvc/chrome?view=revision&revision=235826
> >>
> >
> >
> > I deleted patch set 2. I can fix the other issues and move the channel id
> > tests
> > to
> > ssl_client_socket_unittest.cc in a different CL. Since both
> > third_party/openssl
> > and third_party/tlslite
> > needs to be fixed some kind of sync'd release is needed to keep the tests
> > running on Android.
> >
> >
> >
> >
> >  Yes. 30032 is a new revision of Channel ID. It is not backward compatible,
> >>
> > which
> >
> >> is why it uses a different extension value.
> >>
> >
> >  30032 differs from 30031 only in session resumption handshakes.
> >>
> >
> > Not sure what you mean here, but as I understand it the channel id
> > extension
> > is sent on client/server hello for both full and abbreviated handshake.
> 
> 
> Just that 30032 is wire-different than 30031, but only in the resumption
> handshake case.

Ah, right (the penny dropped), I thought only the number was changed since it
was TBD on the RFC. 
The tests should probably be expanded to cover both full and resumption
handshake then.
> 
> Regardless, we can defer that fixing up.
> 
> 
> >
> >
> > I guess that is
> >
> >> why Haavard reported that changing TLS Lite to use 30032 fixed the
> >> problem --
> >> the Channel ID tests probably only do full handshake.
> >>
> >
> >
> >
> >
> > https://codereview.chromium.org/233133002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-04-22 21:19:28]

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

[commit-bot: I haz the power, 2014-04-22 21:19:28]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_x64_rel

[haavardm, 2014-04-23 07:35:17]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2014-04-23 07:35:32]

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

[commit-bot: I haz the power, 2014-04-23 11:51:29]

Change committed as 265610

[wtc, 2014-04-25 01:16:13]

On 2014/04/22 15:06:29, haavardm wrote:
> Back from holiday, so had a new look at this.
> 
> The failure is that NSS defines the channel id extension to be 30032, while
> OpenSSL and the test server (src/third_party/tlslite/tlslite/constants.py)
> defines it to be 30031. Changing TLS lite to use 30032 "fixes" the issue for
> NSS.

I filed http://code.google.com/p/chromium/issues/detail?id=366961 for
updating the TLS Channel ID extension support in SSLClientSocketOpenSSL.
I attached an OpenSSL patch to the bug report.

[haavardm, 2014-04-25 12:18:27]

On 2014/04/25 01:16:13, wtc wrote:
> On 2014/04/22 15:06:29, haavardm wrote:
> > Back from holiday, so had a new look at this.
> > 
> > The failure is that NSS defines the channel id extension to be 30032, while
> > OpenSSL and the test server (src/third_party/tlslite/tlslite/constants.py)
> > defines it to be 30031. Changing TLS lite to use 30032 "fixes" the issue for
> > NSS.
> 
> I filed http://code.google.com/p/chromium/issues/detail?id=366961 for
> updating the TLS Channel ID extension support in SSLClientSocketOpenSSL.
> I attached an OpenSSL patch to the bug report.

Ah, ok. I've already made a very similar patch myself based on the NSS 30032
patch (didn't have time to file it yesterday, and on Fridays I work on a 20%
position at the local university)

I didn't keep the old version though. What is the reason the patch file comes
with both the old and new version?

[wtc, 2014-04-25 12:34:47]

On 2014/04/25 12:18:27, haavardm wrote:
>
> I didn't keep the old version though. What is the reason the patch file comes
> with both the old and new version?

We do not need to keep the old version. The patch I attached to the bug report
supports both versions mainly because of the server side -- it needs to support
both the current NSS-based and OpenSSL-based clients.