Add served cert chain to SSLInfo

Add served cert chain to SSLInfo

Add a field to SSLInfo to contain the chain as received by the client,
which can be different than the SSLInfo::cert containing the chain that
was verified. The new |served_cert| field is not persisted to the cache
for an HttpResponseInfo and thus can be null.

BUG=498945
Committed: https://crrev.com/03d644fe40b663dd42397649848484e87062afd6
Cr-Commit-Position: refs/heads/master@{#334292}

[estark, 2015-06-10 18:59:42]

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

[estark, 2015-06-10 19:01:06]

[Hit Enter too soon.]

davidben, could you please take a look? I decide to add the served cert to
SSLInfo first (this CL), and then follow up with by splitting up the
HttpResponseInfo interface into SecurityInfo (persisted) and
TransientSecurityInfo (not persisted). This will unblock some other work.

[estark, 2015-06-12 00:16:53]

CCing rsleevi: FYI, feel free to review if you'd like

[davidben, 2015-06-12 19:45:43]

Looks good but for a naming quibble.

(Ryan, do you have strong opinions? I basically don't care but for both
server_cert and served_cert being used is confusing.)

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

https://codereview.chromium.org/1174073003/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:2427: scoped_refptr<X509Certificate>
server_certificate = ssl_info.served_cert;
It is somewhat confusing that the code alternates between calling it "server
certificate" and "served certificate". I keep thinking one of them is a typo.

Maybe just call it server_cert? That's consistent with what
SSLClientSocketOpenSSL calls it internally. Though I can definitely see the
distinction between server_cert and cert as much more confusing than
served_cert. ("Oh, this is the server_cert; the other must be the client cert.
I'll use this one.")

Maybe unverified_cert or original_cert? Just so we're not one letter off from
server_cert. Or perhaps really we should just be renaming all the server_cert
instances to served_cert...

[served cert is also kind of a tongue-twister to say out loud, but oh well.]

[estark, 2015-06-12 20:29:11]

Thanks David.

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

https://codereview.chromium.org/1174073003/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:2427: scoped_refptr<X509Certificate>
server_certificate = ssl_info.served_cert;
On 2015/06/12 19:45:42, David Benjamin wrote:
> It is somewhat confusing that the code alternates between calling it "server
> certificate" and "served certificate". I keep thinking one of them is a typo.
> 
> Maybe just call it server_cert? That's consistent with what
> SSLClientSocketOpenSSL calls it internally. Though I can definitely see the
> distinction between server_cert and cert as much more confusing than
> served_cert. ("Oh, this is the server_cert; the other must be the client cert.
> I'll use this one.")
> 
> Maybe unverified_cert or original_cert? Just so we're not one letter off from
> server_cert. Or perhaps really we should just be renaming all the server_cert
> instances to served_cert...
> 
> [served cert is also kind of a tongue-twister to say out loud, but oh well.]

I like |unverified_cert| the best, I think. Changed to that. |served_cert| and
|server_cert| both are slightly confusing to me anyway because it's really the
|as_received_by_the_client_cert|, not necessarily the actual cert that the
server served.

[davidben, 2015-06-12 20:37:27]

lgtm, thanks!

It might be worth adjusting HttpResponseInfo::ssl_info's comment to say
something like:

  // The SSL connection info (if HTTPS). Note: when serving a response from the
cache, not every field is present. See HttpResponseInfo::InitFromPickle.

[estark, 2015-06-12 20:41:44]

The CQ bit was checked by estark@chromium.org

[estark, 2015-06-12 20:41:45]

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/1174073003/#ps60001
(title: "update HttpResponseInfo::ssl_info comment")

[commit-bot: I haz the power, 2015-06-12 20:43:14]

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

[Ryan Sleevi, 2015-06-12 21:14:06]

Just nits, nothing to worry about

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

https://codereview.chromium.org/1174073003/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2426: sock->GetSSLInfo(&ssl_info);
Hrm. This should probably be an ASSERT_TRUE() here, otherwise 2427 will cause a
null deref (technically, 2431)

https://codereview.chromium.org/1174073003/diff/60001/net/ssl/ssl_info.h
File net/ssl/ssl_info.h (right):

https://codereview.chromium.org/1174073003/diff/60001/net/ssl/ssl_info.h#newc...
net/ssl/ssl_info.h:92: scoped_refptr<X509Certificate> unverified_cert;
nit: Seems like this might be better up at line 45/46, logically grouping the
certificates.

[estark, 2015-06-12 21:16:26]

The CQ bit was unchecked by estark@chromium.org

[estark, 2015-06-12 21:20:41]

Thanks Ryan.

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

https://codereview.chromium.org/1174073003/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2426: sock->GetSSLInfo(&ssl_info);
On 2015/06/12 21:14:06, Ryan Sleevi wrote:
> Hrm. This should probably be an ASSERT_TRUE() here, otherwise 2427 will cause
a
> null deref (technically, 2431)

Done.

https://codereview.chromium.org/1174073003/diff/60001/net/ssl/ssl_info.h
File net/ssl/ssl_info.h (right):

https://codereview.chromium.org/1174073003/diff/60001/net/ssl/ssl_info.h#newc...
net/ssl/ssl_info.h:92: scoped_refptr<X509Certificate> unverified_cert;
On 2015/06/12 21:14:06, Ryan Sleevi wrote:
> nit: Seems like this might be better up at line 45/46, logically grouping the
> certificates.

Done.

[estark, 2015-06-12 21:20:46]

The CQ bit was checked by estark@chromium.org

[estark, 2015-06-12 21:20:46]

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/1174073003/#ps80001
(title: "rsleevi nits")

[commit-bot: I haz the power, 2015-06-12 21:22:10]

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

[commit-bot: I haz the power, 2015-06-12 23:25:49]

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

[commit-bot: I haz the power, 2015-06-12 23:25:50]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[estark, 2015-06-12 23:27:46]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-06-12 23:28:03]

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

[commit-bot: I haz the power, 2015-06-13 00:11:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-13 00:13:29]

Patchset 5 (id:??) landed as
https://crrev.com/03d644fe40b663dd42397649848484e87062afd6
Cr-Commit-Position: refs/heads/master@{#334292}