Change the HTTP cache to cache the entire certificate chain for SSL sites

Change the HTTP cache to cache the entire certificate chain for SSL sites

When persisting an X509Certificate to a pickle, such as when storing to the HTTP cache, persist any intermediate certificates in addition to the end-entity certificate. This will allow the complete certificate chain to be displayed to the end user when viewing a cached entry, independent of whether a network request has been made to that site during the browsing session.

R=agl
BUG=7065
TEST=X509CertificateTest.Persist

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82214

[Ryan Sleevi, 2010-11-07 10:27:48]

wtc: This was moved from CL #2944008 after the changes related to this grew.
This change still has the certificates received over the network evicting the
certificates from the cache, as it currently behaves, but also adds that
certificate chains from the cache will evict any singlely-loaded certificates.

mbelshe: Could you check the http_response_info.cc and confirm this is the right
way to do it? Is there a better way to do this that doesn't require introducing
a separate status bit that still keeps the pickle in the correct position?

agl: I know you added the False Start cache to track the Finished message, but
it didn't seem appropriate to re-use that here. The primary reason for this was
because the False Start only caches the most chain for the server, AFAICT, but I
believe the cached data should reflect the server-as-it-was, not the
server-as-it-is. Is there a reason not to do it this way?

Finally, is anyone concerned with overall impact to the HTTP cache size? Each
certificate sent by the server tends to add 1-2K, and most chains are between
1-3 certificates. If the size is a concern, my thought was to split out a new
certificate cache, based on fingerprint plus unique index (in the event of hash
collisions), and have the HTTP response info simply serialize those keys rather
than the full certificates.

[agl, 2010-11-08 23:24:48]

I assume that this is so that we can show the chain in the UI in the event of a
cache hit?

I note that we also save the full chain in SSLHostInfo, although only once per
site. Although the duplication doesn't bother me.

http://codereview.chromium.org/4645001/diff/1/2
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/4645001/diff/1/2#newcode215
net/base/x509_certificate.cc:215: OSCertHandle cert_handle =
ReadCertHandleFromPickle(pickle, pickle_iter);
The difference in behavior between a parse failure of the first certificate
(which desyncs the pickle in the case of a chain) and an intermediate (which
doesn't) is worthy of a comment I think. (Or possibly a unification.)

[Ryan Sleevi, 2010-11-09 00:26:14]

On 2010/11/08 23:24:48, agl wrote:
> I assume that this is so that we can show the chain in the UI in the event of
a
> cache hit?

The immediate problem is UI related. Longer term (and why I originally wrote
this, before http://crbug.com/60256 was filed) was to be able to change how
X509Certificate::Cache. Currently the cache can only remember one chain per
certificate, only the longest chain, and with a preference towards those
received over the network. There are also a few thread-safety issues with how it
does this. Changing the HTTP cache to include the whole chain allows those
issues to be separately addressed, without negatively affecting the info
bubble/cert UI on a cache hit.

> 
> I note that we also save the full chain in SSLHostInfo, although only once per
> site. Although the duplication doesn't bother me.

Right, I realized I had a few typos in my original comment, talking about the
Finished message rather than the certificate chain. I knew you were/are also
working on changes related to the expected verification status, that's why I
mentioned it as something I wasn't sure of whether to reuse and wanted to get
your feedback on.

> http://codereview.chromium.org/4645001/diff/1/2
> File net/base/x509_certificate.cc (right):
> 
> http://codereview.chromium.org/4645001/diff/1/2#newcode215
> net/base/x509_certificate.cc:215: OSCertHandle cert_handle =
> ReadCertHandleFromPickle(pickle, pickle_iter);
> The difference in behavior between a parse failure of the first certificate
> (which desyncs the pickle in the case of a chain) and an intermediate (which
> doesn't) is worthy of a comment I think. (Or possibly a unification.)

Agreed. The original behaviour was that it always advanced the pickle,
regardless of whether or not the (then single) certificate parsed. I retained
that for intermediates, but accidentally omitted it for the server certificate.
I've updated the implementation and documentation in Patch Set 2 to restore and
describe the intended behaviour.

[Ryan Sleevi, 2010-11-18 08:38:17]

wtc: Ping on this, as it may have been lost :)

[Ryan Sleevi, 2010-11-29 23:32:59]

wtc, mbelshe: ping?

[Ryan Sleevi, 2011-01-13 00:50:24]

wtc, mbelshe: Ping on this again. Sorry I let it sit.

wtc: for the certificate changes.
mbelshe: for the response info.

I'd like to look at landing this so that I can continue working on
http://crbug.com/49377, which is now implicated in several tests being flaky and
resulting in them being DISABLED_.

[agl, 2011-04-12 15:00:56]

LGTM

http://codereview.chromium.org/4645001/diff/28018/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/4645001/diff/28018/net/http/http_response_info...
net/http/http_response_info.cc:57: // only one certificate, the response
certificate; however, if this bit is
s/,/:/  s/; h/. H/

(punctuation nits are only suggestions, ignore them if you like.)

[Ryan Sleevi, 2011-04-17 09:57:47]

agl: Small update from what you LGTM'd. When reviewing one last time before
commit, I realized that willchan had removed bit 16 in http://crrev.com/77908.
If bit 16 were re-used in a subsequent modification (eg: if I changed the
HAS_CERT_CHAIN bit to 16 or if a new one was added) without bumping the version,
that could cause de-pickling errors as users upgraded from version 1 with bit16
meaning "alternate protocol available" and version 1.? with bit16 meaning
something else.

As a result, I bumped the version to version 2, so that any future bits added
can continue being added in numerical sequence without issue. Further, since the
version was bumped, it made the new status bit unnecessary, since the version
can just be checked instead.

It's minor, but it seemed an easy win for two birds with one stone.

[agl, 2011-04-19 13:51:48]

On Sun, Apr 17, 2011 at 5:57 AM,  <rsleevi@chromium.org> wrote:
> agl: Small update from what you LGTM'd

Sounds fine.


Cheers

AGL

[commit-bot: I haz the power, 2011-04-20 14:02:43]

Can't apply patch for file net/base/net/base/x509_certificate_win.cc.
A         net/base/net
A         net/base/net/base
patching file net/base/net/base/x509_certificate_win.cc
Hunk #1 FAILED at 550.
Hunk #2 FAILED at 635.
Hunk #3 FAILED at 1044.
3 out of 3 hunks FAILED -- saving rejects to file
net/base/net/base/x509_certificate_win.cc.rej

[wtc, 2011-04-20 23:07:58]

rvargas: please review the http_response_info.cc changes in
this CL, which I just described to you in person.  You may
ignore the rest of the CL.

rsleevi: thank you for fixing this bug in M12.  At first I
thought it risky to bump the HttpResponseInfo pickle version
the day before the M12 branch was cut.  But I found that none
of my suggested changes below will change the on-disk format
of HttpResponseInfo, so this should be fine.

High-level comments:

1. We should remove the enum Source and the related code in
the X509Certificate cache.  The X509Certificate cache should
be able to cache X509Certificate objects containing the
same leaf certificate but different intermediate CA
certificates.

I can take a stab at this.

2. I believe the intermediate_ca_certificates_ member of
X509Certificate stores the intermediate CA certificates
received in an SSL Certificate message, rather than the
certificate chain built by the certificate verification
operation.  If so, we should document this.

Low-level comments follow.

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.c...
net/base/x509_certificate.cc:245: // other reads the caller may perform after
this method returns.
If a certificate fails to parse, the subsequent data in
|pickle| may be invalid and |pickle_iter| may already be
out of sync.

Are you sure we should continue reading?

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.c...
net/base/x509_certificate.cc:253: bool ok = !!cert_handle;
Nit: say
  bool ok = (cert_handle != NULL);

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.c...
net/base/x509_certificate.cc:270: return NULL;
We should also return NULL if |ok| is false.

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.h...
net/base/x509_certificate.h:87: };
IMPORTANT: I added the enum Source to work around this bug.
Therefore, when we fixed this bug, we should be able to
remove enum Source, rather than extending it.

SOURCE_LONE_CERT_IMPORT was intended to mean importing a
certificate from the HTTP cache.  So it is exactly what
SOURCE_FROM_CACHE is, for version 1.

In any case, please try to remove enum Source.

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.h...
net/base/x509_certificate.h:116: enum PickleType {
The motivation for enum PickleType should be documented.
It is not obvious why PickleType does not apply to writing
a certificate to a Pickle unless one also looks at the
corresponding code in http_response_info.cc.

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.h...
net/base/x509_certificate.h:416: static bool
WriteCertHandleToPickle(OSCertHandle handle, Pickle* pickle);
Nit: these two function names should say "OSCertHandle" instead
of just "CertHandle".

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate_w...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:1045: length);
Why don't you use the original code (pickle->BeginWriteData),
which avoids copying?

http://codereview.chromium.org/4645001/diff/38001/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/4645001/diff/38001/net/http/http_response_info...
net/http/http_response_info.cc:139: // while subsequent versions include the
entire chain.
This comment should be moved (or copied) to the definition of
RESPONSE_INFO_HAS_CERT at lines 33-34 above.

[rvargas (doing something else), 2011-04-20 23:51:14]

LGTM

http://codereview.chromium.org/4645001/diff/38001/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/4645001/diff/38001/net/http/http_response_info...
net/http/http_response_info.cc:138: // Version 1 only serialized only the
end-entity certificate,
nit: extra only

[Ryan Sleevi, 2011-04-20 23:59:10]

On 2011/04/20 23:07:58, wtc wrote:
> High-level comments:
> 
> 1. We should remove the enum Source and the related code
> in the X509Certificate cache.  The X509Certificate cache
> should be able to cache X509Certificate objects
> containing the same leaf certificate but different
> intermediate CA certificates.
>
> I can take a stab at this.

Both points are addressed in http://codereview.chromium.org/2944008/

> 2. I believe the intermediate_ca_certificates_ member of
> X509Certificate stores the intermediate CA certificates
> received in an SSL Certificate message, rather than the
> certificate chain built by the certificate verification
> operation.  If so, we should document this.

Currently, that is correct. However, http://codereview.chromium.org/6874039/
changes this by returning the constructed/verified certificate chain if one was
able to be created, which may different from what the server sent. The full
impact of that change on the HTTP cache is described in the first comment, but
summarized is that the HTTP cache will store the full chain that was verified
(to a root) if it was successfully verified. This ensures that users are shown
the exact chain used. Storing only the list that the server sent (as is what is
happens in M12) leaves the edge case where the user removes/revokes a
certificate, whether an intermediate or a root, which would cause the
certificate viewer to be unable to display the certificate chain for cached
entries.

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.c...
net/base/x509_certificate.cc:245: // other reads the caller may perform after
this method returns.
On 2011/04/20 23:07:58, wtc wrote:
> If a certificate fails to parse, the subsequent data in
> |pickle| may be invalid and |pickle_iter| may already be
> out of sync.
> 
> Are you sure we should continue reading?

There are two reasons for why parsing may fail, and my goal was to gracefully
handle one of them.
1) The |pickle_iter| is out of sync. We've already thrown it even more out of
sync by the failed attempt to read a single certificate, so this cannot be
gracefully recovered.
2) The |pickle_iter| is in sync, but the underlying cryptographic library is no
longer able to parse the certificate. This indicates a failure of the library,
but not of the cache, and can be gracefully recovered with respect to the
pickle.

By handling #2 gracefully, the same error handling semantics of M11 and earlier
were preserved - the certificate data is always pulled out of the pickle,
regardless of the state of the underlying cryptographic library. This, in turn,
minimized any impact this change might have on M12, and in HttpResponseInfo
deserialization in general.

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.c...
net/base/x509_certificate.cc:270: return NULL;
On 2011/04/20 23:07:58, wtc wrote:
> We should also return NULL if |ok| is false.

This contradicts the behaviour documented in line 258-259 (which can be
changed). |intermediates| would also need to be freed before returning (which
can also be changed)

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate.h...
net/base/x509_certificate.h:87: };
On 2011/04/20 23:07:58, wtc wrote:
> IMPORTANT: I added the enum Source to work around this bug.
> Therefore, when we fixed this bug, we should be able to
> remove enum Source, rather than extending it.
> 
> SOURCE_LONE_CERT_IMPORT was intended to mean importing a
> certificate from the HTTP cache.  So it is exactly what
> SOURCE_FROM_CACHE is, for version 1.
> 
> In any case, please try to remove enum Source.

The motivation for extending it in M12 was to minimize a regression of the
original issue. Because the user's existing cache only contains single
certificates, any cached resources the user loads after upgrading to M12 would
regress, while any newly cached resources would be fine.

By deferring removal to M13 (as part of the X509Certificate* cache changes),
this allowed a full release for users to transition from single-cert V1 to
chained V2. This should reduce the potential impact to just users who skip M12
entirely.

The preference here is:
1) < M12 HTTP Cache (cached EE only)
2) M12 HTTP Cache (cached EE + cached intermediates)
3) Network in current browsing session (EE + intermediates)

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate_w...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/4645001/diff/38001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:1045: length);
On 2011/04/20 23:07:58, wtc wrote:
> Why don't you use the original code (pickle->BeginWriteData),
> which avoids copying?

Per pickle.cc, there can only be one variable buffer in a Pickle:

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/pickle.c...