Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 4645001: Change the HTTP cache to cache the entire certificate chain for SSL sites (Closed)

Created:
8 years, 9 months ago by Ryan Sleevi
Modified:
8 years, 3 months ago
CC:
chromium-reviews, pam+watch_chromium.org, cbentzel+watch_chromium.org, darin-cc_chromium.org, PaweĊ‚ Hajdan Jr., Mike Belshe, willchan no longer on Chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : agl feedback #

Patch Set 3 : Rebase to trunk #

Patch Set 4 : Rebase #

Patch Set 5 : Bump the HttpResponseInfo pickled version #

Patch Set 6 : Rebase before commit #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+250 lines, -119 lines) Patch
M net/base/x509_certificate.h View 1 2 3 5 chunks +31 lines, -5 lines 4 comments Download
M net/base/x509_certificate.cc View 1 2 3 4 chunks +67 lines, -4 lines 5 comments Download
M net/base/x509_certificate_mac.cc View 1 2 3 3 chunks +24 lines, -22 lines 0 comments Download
M net/base/x509_certificate_nss.cc View 1 2 3 3 chunks +20 lines, -16 lines 0 comments Download
M net/base/x509_certificate_openssl.cc View 1 2 3 4 5 3 chunks +24 lines, -20 lines 0 comments Download
M net/base/x509_certificate_unittest.cc View 1 2 3 1 chunk +30 lines, -7 lines 0 comments Download
M net/base/x509_certificate_win.cc View 1 2 3 3 chunks +41 lines, -40 lines 2 comments Download
M net/http/http_response_info.cc View 1 2 3 4 4 chunks +13 lines, -5 lines 2 comments Download

Messages

Total messages: 13 (0 generated)
Ryan Sleevi
wtc: This was moved from CL #2944008 after the changes related to this grew. This ...
8 years, 9 months ago (2010-11-07 10:27:48 UTC) #1
agl
I assume that this is so that we can show the chain in the UI ...
8 years, 9 months ago (2010-11-08 23:24:48 UTC) #2
Ryan Sleevi
On 2010/11/08 23:24:48, agl wrote: > I assume that this is so that we can ...
8 years, 9 months ago (2010-11-09 00:26:14 UTC) #3
Ryan Sleevi
wtc: Ping on this, as it may have been lost :)
8 years, 9 months ago (2010-11-18 08:38:17 UTC) #4
Ryan Sleevi
wtc, mbelshe: ping?
8 years, 8 months ago (2010-11-29 23:32:59 UTC) #5
Ryan Sleevi
wtc, mbelshe: Ping on this again. Sorry I let it sit. wtc: for the certificate ...
8 years, 7 months ago (2011-01-13 00:50:24 UTC) #6
agl
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.cc#newcode57 net/http/http_response_info.cc:57: // only one certificate, the response certificate; however, ...
8 years, 4 months ago (2011-04-12 15:00:56 UTC) #7
Ryan Sleevi
agl: Small update from what you LGTM'd. When reviewing one last time before commit, I ...
8 years, 4 months ago (2011-04-17 09:57:47 UTC) #8
agl
On Sun, Apr 17, 2011 at 5:57 AM, <rsleevi@chromium.org> wrote: > agl: Small update from ...
8 years, 4 months ago (2011-04-19 13:51:48 UTC) #9
commit-bot: I haz the power
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 ...
8 years, 4 months ago (2011-04-20 14:02:43 UTC) #10
wtc
rvargas: please review the http_response_info.cc changes in this CL, which I just described to you ...
8 years, 4 months ago (2011-04-20 23:07:58 UTC) #11
rvargas (doing something else)
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.cc#newcode138 net/http/http_response_info.cc:138: // Version 1 only serialized only the end-entity ...
8 years, 4 months ago (2011-04-20 23:51:14 UTC) #12
Ryan Sleevi
8 years, 4 months ago (2011-04-20 23:59:10 UTC) #13
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...

Powered by Google App Engine
This is Rietveld 408576698