Ryan, when you said that we could blow away the cache entries, how is that done exactly? Would that mean returning false from HttpResponseInfo::InitFromPickle() if we run across an SCTVerifyStatus value that isn't recognized?

(Not saying that blowing them away is necessarily the right thing to do -- didn't know it was an option, though, so I'm curious how it's done.)

Right, if we returned false, that would invalidate the entry, which would cause a new network fetch to be returned.

It would not invalidate 100% of the cache; it would primarily invalidate Google resources (as we serve SCTs) and sites that opted in to CT (since we cached those). I'm definitely a fan of 1 over 2, although if we need the API flexibility, we might want to discuss moving the disk cache entry to a proper protobuf format, since we've run into this a few times.

I know Ricardo thought it was crazy we store the certificate information in the cache at all (because of how metadata is stored in the cache), and Gavin may feel the same, so we definitely need to be careful before introducing new certificate-related stuff into the cache (since it's connection oriented, and would end up duplicated in every single entry from that connection that was cached, which in an H/2 world, can quickly mean adding 16-20K-per-resource to the disk cache).

But that's long term. Short term I like 1 over 2, but I don't think 2 would be unreasonable, because it'd be a one-time hit for the entry.

In this particular case, it would only invalidate entries that had INVALID SCTs, right? (Which should be only a very small percentage of them.) Is there a reason we'd need to also invalidate entries with valid SCTs?

Oh, true, it would only affect that (tiny) fraction, which would hopefully primarily be name redacted certificates and certificates from unknown logs, both of which we're not widely seeing.

Note it would not invalidate entries with SCTs from unknown logs - the invalid status is only for SCTs from known logs where the signature didn't validate or the timestamp was wrong.

--- stuff I wrote before your discussion in the bug ---
A few things we could do:

(1) Re-validate the SCT. We should have the data to do it (the certificate + chain, timestamp, signature). I haven't looked at how complex the wiring would be, though, as we'd need a MultiLogCTVerifier instance - in and of itself it's easy to create an initialized one, but we would be validating against the *current* global log list, not the one that the client had when persisting it. And then there's the cascade of re-evaluating CT policy compliance based on the re-validated SCTs, etc. 

(2) Check if the timestamp in the loaded cache entry is in the future. If it is, map the invalid status to SCT_STATUS_INVALID_TIMESTAMP, otherwise SCT_STATUS_INVALID_SIGNATURE. Obviously this mapping is incorrect as time changed since the entry was stored to disk, but a very conservative way to map.

But discarding these entries sounds the simplest thing to do, if possible.

Thinking about this some more, I'm still pretty torn between options 1 and 2 in the bug description.

First off, I think we should consider getting rid of the WebsiteSettings (origin info bubble) strings about CT. I can't recall what the original motivation was for adding them (might have been before my time) -- but IMO details about CT are useful mainly to power-users and developers and thus more rightfully belong in devtools (where they are already displayed) than in WebsiteSettings.

If we do that, then devtools becomes, as far as I know, the only place that would be affected by ceasing to cache SCTs. (Well, besides the MainFrameValidSCTCount histogram.) So the question then is if we care about showing SCTs in devtools for resources loaded from cache.

On the one hand, it will surely create some confusing moments for developers to not see their SCTs on resources that were loaded from cache. On the other hand, I don't think devtools should drive decisions about what gets stored in the disk cache: we should either not show non-cached stuff in devtools, or we should design a UX in devtools that somehow explains why certain information doesn't show up sometimes. (As a strawman, when viewing a resource in the Security panel that was loaded from cache, we could have a message that says "Not all security information is available for resources from the cache; try shift+refresh.")

So I guess I lean slightly towards option 1: remove the WebsiteSettings strings about CT, and then just stop reading/writing SCTs to the cache.

+lgarron for thoughts on removing CT stuff from WebsiteSettings

I think it's fine to remove CT from Page Info/WebsiteSettings on Android, even before we figure out what to do with it in general.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f

commit b082a9ffa25c1bcb6e884379f0f3cf4e0997931f
Author: eranm <eranm@chromium.org>
Date: Tue Sep 13 21:03:26 2016

Certificate Transparency: Remove the obsolete invalid sct status.

Remove the obsolete enum value SCT_STATUS_INVALID which was replaced
by two distinct enum values.

To avoid crashing Chrome clients which have entries cached on disk
with the obsolete enum value, fail to de-serialize such cache entries.

This reverts commit 321ed2a53224c50af40387e8211726f8400ecad2.

BUG=640296, 634006 , 640689 

Review-Url: https://codereview.chromium.org/2294373002
Cr-Commit-Position: refs/heads/master@{#418367}

[modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/chrome/browser/ssl/chrome_expect_ct_reporter.cc
[modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/chrome/browser/ui/website_settings/website_settings.cc
[modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/cert/ct_sct_to_string.cc
[add] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/cert/sct_status_flags.cc
[modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/cert/sct_status_flags.h
[modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/http/http_response_info.cc
[modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/http/http_response_info_unittest.cc
[modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/net.gypi

Closing this out because it was fixed by the commit in Comment 10. I still think we could get away with removing SCTs from the disk cache all together, but I'll file a separate bug for that.

Security>UX component is deprecated in favor of the Team-Security-UX label