Enable Certificate Transparency in the OpenSSL port.

Enable Certificate Transparency in the OpenSSL port.

Also add tests that assert OCSP responses and SCT extensions are returned in
the expected format.

BUG=408687
TEST=open https://embed.ct.digicert.com/ in Chrome for Android and
     tap the lock icon. Popup should say "The identity of this
     [...] and is publicly auditable."

Committed: https://crrev.com/eb5f8ef3d692e2a9fb6ad6837426da2b9e2c0ee9
Cr-Commit-Position: refs/heads/master@{#293295}

[Ryan Sleevi, 2014-08-28 21:01:02]

drive by

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

https://codereview.chromium.org/517083002/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2558: EXPECT_EQ("",
call.stapled_ocsp_response);
EXPECT_TRUE(.empty())

https://codereview.chromium.org/517083002/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2617: EXPECT_EQ("",
call.sct_list_from_tls_extension);
EXPECT_TRUE(.empty())

[davidben, 2014-08-28 21:23:25]

(Oh, there's a decent chance I'll add you as reviewer for one of those two
pieces anyway. :-P I'm waiting on the two BoringSSL CLs to land and roll in
first just so I can run some try jobs first.)

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

https://codereview.chromium.org/517083002/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2558: EXPECT_EQ("",
call.stapled_ocsp_response);
On 2014/08/28 21:01:02, Ryan Sleevi wrote:
> EXPECT_TRUE(.empty())

Done.

https://codereview.chromium.org/517083002/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2617: EXPECT_EQ("",
call.sct_list_from_tls_extension);
On 2014/08/28 21:01:02, Ryan Sleevi wrote:
> EXPECT_TRUE(.empty())

Done.

[davidben, 2014-08-29 15:33:55]

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

[davidben, 2014-08-29 15:33:55]

Here, you can have this one. This depends on
https://codereview.chromium.org/519473002/.

I've fired some try jobs for now because the NSS bots should be able to run
cleanly. OpenSSL bots may or may not; these tests don't actually assert on the
rest, but they'll hit NOTIMPLEMENTED. It's also possible there's some
higher-level test that will break without the other CL. I'll rerun them once
it's in.

[davidben, 2014-09-02 15:58:31]

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

[davidben, 2014-09-02 15:58:32]

+eranm as reviewer as well

[Ryan Sleevi, 2014-09-02 19:49:07]

https://codereview.chromium.org/517083002/diff/80001/net/cert/asn1_util.h
File net/cert/asn1_util.h (right):

https://codereview.chromium.org/517083002/diff/80001/net/cert/asn1_util.h#new...
net/cert/asn1_util.h:23: static const unsigned kENUMERATED = 0x0a;
Pure pedantry nit: 0x0A;

You can send me hate-mail through interoffice mail.

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:821: // into the certificate verifier.
nit: BUG #?

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1145: sct_list_len),
BUG: The constructor for std::string requires that |s| (aka ocsp_response /
sct_list) be non-NULL (c.f. C++03 21.3.1 p6)

basic_string(const charT* s, size_type n,
const Allocator& a = Allocator());
6 Requires: s shall not be a null pointer and n < npos.


However, both the SSL_get0 functions return NULL in the absence of the relevant
data.

I'd suggest you restructure with

uint8_t* ocsp_response_raw;
size_t ocsp_response_len;
SSL_get0_ocsp_response(..., &.._raw, &..._len);
std::string ocsp_response;
if (...len)
   ocsp_response.assign(..._raw, ..._len);


(note that .assign and .append both inherit the non-NULL requirement)

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

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:663: class MockCTVerifier : public
CTVerifier {
This is a bit weird, in particular, the tracking of Call (which is just a tuple
of params)

Why not use the real GMock to set expectations and assertions on the call
handling?

EXPECT_CALL(verifier, Verify(_, "", "test", _,
_)).WillOnce(Return(ERR_CT_NO_SCTS_VERIFIED_OK));

etc? (Or WillRepeatedly, since you don't want/need to encode that CT is only
verified once, as that's not part of the API contract)

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2621: // SEQUENCE of an ENUMERATED type
and an element tagged with [0] EXPLICIT.
What, precisely, is this testing? Just that a well-formed OCSP response is
received from the test server (e.g. not mangled by the socket)? If so, good to
document that.

[davidben, 2014-09-02 22:13:45]

https://codereview.chromium.org/517083002/diff/80001/net/cert/asn1_util.h
File net/cert/asn1_util.h (right):

https://codereview.chromium.org/517083002/diff/80001/net/cert/asn1_util.h#new...
net/cert/asn1_util.h:23: static const unsigned kENUMERATED = 0x0a;
On 2014/09/02 19:49:06, Ryan Sleevi wrote:
> Pure pedantry nit: 0x0A;
> 
> You can send me hate-mail through interoffice mail.

Done.

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:821: // into the certificate verifier.
On 2014/09/02 19:49:06, Ryan Sleevi wrote:
> nit: BUG #?

Done.

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1145: sct_list_len),
On 2014/09/02 19:49:06, Ryan Sleevi wrote:
> BUG: The constructor for std::string requires that |s| (aka ocsp_response /
> sct_list) be non-NULL (c.f. C++03 21.3.1 p6)
> 
> basic_string(const charT* s, size_type n,
> const Allocator& a = Allocator());
> 6 Requires: s shall not be a null pointer and n < npos.
> 
> 
> However, both the SSL_get0 functions return NULL in the absence of the
relevant
> data.
> 
> I'd suggest you restructure with
> 
> uint8_t* ocsp_response_raw;
> size_t ocsp_response_len;
> SSL_get0_ocsp_response(..., &.._raw, &..._len);
> std::string ocsp_response;
> if (...len)
>    ocsp_response.assign(..._raw, ..._len);
> 
> 
> (note that .assign and .append both inherit the non-NULL requirement)

Done.

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

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:663: class MockCTVerifier : public
CTVerifier {
On 2014/09/02 19:49:06, Ryan Sleevi wrote:
> This is a bit weird, in particular, the tracking of Call (which is just a
tuple
> of params)
> 
> Why not use the real GMock to set expectations and assertions on the call
> handling?
> 
> EXPECT_CALL(verifier, Verify(_, "", "test", _,
> _)).WillOnce(Return(ERR_CT_NO_SCTS_VERIFIED_OK));
> 
> etc? (Or WillRepeatedly, since you don't want/need to encode that CT is only
> verified once, as that's not part of the API contract)

Done. Also trimmed a lot of the tests down as they weren't testing anything
relevant.

https://codereview.chromium.org/517083002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2621: // SEQUENCE of an ENUMERATED type
and an element tagged with [0] EXPLICIT.
On 2014/09/02 19:49:06, Ryan Sleevi wrote:
> What, precisely, is this testing? Just that a well-formed OCSP response is
> received from the test server (e.g. not mangled by the socket)? If so, good to
> document that.

This is testing that we get back a reasonable well-formed OCSP response, yeah.
The main failure modes I was hoping to test for was us returning the
OCSPResponse rather than, say, including the TLS two-byte length prefix[*]. This
was a convenient way to double-check the behavior of NSS and assert that
BoringSSL's matches.

[*] E.g., this doesn't show up in the test because of the point at which
testserver.py injects the string "test", but the SCT list is expected to be
returned with a two-byte length prefix intact.

Added a comment.

[Ryan Sleevi, 2014-09-02 23:05:11]

lgtm

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

https://codereview.chromium.org/517083002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2533: // Check that the OCSP response
is well-formed. It should be the DER encoding of
Move to line 1007

[davidben, 2014-09-02 23:38:43]

Holding on ct_object_extractor_openssl.cc CL before landing. (Otherwise this
won't do anything and just fire a storm of NOTIMPLEMENTED's.)

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

https://codereview.chromium.org/517083002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2533: // Check that the OCSP response
is well-formed. It should be the DER encoding of
On 2014/09/02 23:05:11, Ryan Sleevi wrote:
> Move to line 1007

Done.

[Eran Messeri, 2014-09-03 08:19:00]

lgtm in term of compatibility with the NSS implementation, thanks for doing
this!

[davidben, 2014-09-04 01:02:49]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-09-04 01:05:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/517083002/120001

[commit-bot: I haz the power, 2014-09-04 02:57:22]

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

[commit-bot: I haz the power, 2014-09-04 02:57:23]

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

[davidben, 2014-09-04 13:48:46]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-09-04 13:49:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/517083002/120001

[commit-bot: I haz the power, 2014-09-04 14:15:34]

Committed patchset #7 (id:120001) as ce087ac93e71fe4fa52f7edc9ab6ab5fe4c7b249

[commit-bot: I haz the power, 2014-09-10 03:31:20]

Patchset 7 (id:??) landed as
https://crrev.com/eb5f8ef3d692e2a9fb6ad6837426da2b9e2c0ee9
Cr-Commit-Position: refs/heads/master@{#293295}