Bring back SCT_STATUS_INVALID for cached entries

Bring back SCT_STATUS_INVALID for cached entries

https://codereview.chromium.org/2241213002/ split SCT_STATUS_INVALID
into two new enum values to record the different reasons that an SCT
might be invalid. However, we didn't realize that SCT_STATUS_INVALID
might still be present in disk cache entries, resulting in renderer
kills when the browser tries to deserialize SSLStatus objects containing
these cached SCT_STATUS_INVALID values.

This CL brings back SCT_STATUS_INVALID and documents it as deprecated,
but still existent for the sake of cache entries.

BUG=640296
Committed: https://crrev.com/321ed2a53224c50af40387e8211726f8400ecad2
Cr-Commit-Position: refs/heads/master@{#414280}

[estark, 2016-08-24 04:27:27]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 04:27:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-08-24 04:30:35]

estark@chromium.org changed reviewers:
+ avi@chromium.org, eranm@chromium.org, eroman@chromium.org

[estark, 2016-08-24 04:30:35]

avi, eroman: this is a follow-up to a CL you reviewed
(https://codereview.chromium.org/2241213002/) that turned out to cause renderer
kills. I'm not really sure if this is the right way to fix this -- what do you
think?

[estark, 2016-08-24 04:39:07]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 04:39:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2016-08-24 04:51:12]

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

[Ryan Sleevi, 2016-08-24 04:51:13]

As a short-term fix, LGTM, but I'm surprised we ran into this because of disk
cache entries, and we should figure that out. This feels like it fits into that
discussion about ephemeral security state versus persisted security state, and I
don't think we want CT in that persistent state for reasons such as this, or if
we do persist it, we need to make sure we're actively managing the disk cache
serialization; but I'd rather save that for a separate CL so we can stop
crashing users :)

https://codereview.chromium.org/2277653002/diff/20001/net/cert/sct_status_fla...
File net/cert/sct_status_flags.h (right):

https://codereview.chromium.org/2277653002/diff/20001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:29: // out of the disk cache.
Layering: net/cert doesn't know about things like "the disk cache"

// DEPRECATED: No longer in use, but may be present in serialized messages.
DEPRECATED_SCT_STATUS_INVALID = 2,

[Ryan Sleevi, 2016-08-24 04:57:21]

Alternatively, why not just map the old value to the new value in
https://chromium.googlesource.com/chromium/src/+blame/master/net/http/http_re...
?

[estark, 2016-08-24 05:01:41]

On 2016/08/24 04:57:21, Ryan Sleevi (slow) wrote:
> Alternatively, why not just map the old value to the new value in
>
https://chromium.googlesource.com/chromium/src/+blame/master/net/http/http_re...
> ?

If we read out an INVALID value, we'd have to arbitrarily decide to map it to
either INVALID_TIMESTAMP or INVALID_SIGNATURE, which seemed weird to me. Or...
are you suggesting that we re-validate the SCT as we read it out of the cache?

[Ryan Sleevi, 2016-08-24 05:37:22]

On 2016/08/24 05:01:41, estark wrote:
> On 2016/08/24 04:57:21, Ryan Sleevi (slow) wrote:
> > Alternatively, why not just map the old value to the new value in
> >
>
https://chromium.googlesource.com/chromium/src/+blame/master/net/http/http_re...
> > ?
> 
> If we read out an INVALID value, we'd have to arbitrarily decide to map it to
> either INVALID_TIMESTAMP or INVALID_SIGNATURE, which seemed weird to me. Or...
> are you suggesting that we re-validate the SCT as we read it out of the cache?

Well, short-term, I was going to suggest we map it to INVALID_SIGNATURE as a
fuzzy match; it seems better than supporting a deprecated thing in the API
indefinitely. Long-term, I was going to suggest we don't cache those bits. They
only existed to support the CT UI in the Page Info Bubble, we lacked the notion
of ephemeral status on the SSLInfo for cached data at the time, and we hadn't
figured out what the story of SCT validation was going to be (... 3 years later
and we're just starting to touch on it, and none of it would reuse the data from
the cache in the new plans)

[commit-bot: I haz the power, 2016-08-24 06:02:55]

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

[commit-bot: I haz the power, 2016-08-24 06:02:56]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Eran Messeri, 2016-08-24 11:10:46]

On 2016/08/24 at 05:37:22, rsleevi wrote:
> On 2016/08/24 05:01:41, estark wrote:
> > On 2016/08/24 04:57:21, Ryan Sleevi (slow) wrote:
> > > Alternatively, why not just map the old value to the new value in
> > >
> >
https://chromium.googlesource.com/chromium/src/+blame/master/net/http/http_re...
> > > ?
> > 
> > If we read out an INVALID value, we'd have to arbitrarily decide to map it
to
> > either INVALID_TIMESTAMP or INVALID_SIGNATURE, which seemed weird to me.
Or...
> > are you suggesting that we re-validate the SCT as we read it out of the
cache?
> 
> Well, short-term, I was going to suggest we map it to INVALID_SIGNATURE as a
fuzzy match; it seems better than supporting a deprecated thing in the API
indefinitely. Long-term, I was going to suggest we don't cache those bits. They
only existed to support the CT UI in the Page Info Bubble, we lacked the notion
of ephemeral status on the SSLInfo for cached data at the time, and we hadn't
figured out what the story of SCT validation was going to be (... 3 years later
and we're just starting to touch on it, and none of it would reuse the data from
the cache in the new plans)

+1 for mapping to INVALID_SIGNATURE:
- It is not going to mess up our UMA metrics because these only get logged when
the SCT is validated, not when loaded from disk.
- For all intents an purposes, all client code treats both INVALID statuses the
same. 
- UMA shows INVALID_SIGNATURE is a few orders of magnitudes more likely than
INVALID_TIMESTAMP, so the mapping will be right 99% of the cases :)

[estark, 2016-08-24 15:07:08]

On 2016/08/24 11:10:46, Eran Messeri wrote:
> On 2016/08/24 at 05:37:22, rsleevi wrote:
> > On 2016/08/24 05:01:41, estark wrote:
> > > On 2016/08/24 04:57:21, Ryan Sleevi (slow) wrote:
> > > > Alternatively, why not just map the old value to the new value in
> > > >
> > >
>
https://chromium.googlesource.com/chromium/src/+blame/master/net/http/http_re...
> > > > ?
> > > 
> > > If we read out an INVALID value, we'd have to arbitrarily decide to map it
> to
> > > either INVALID_TIMESTAMP or INVALID_SIGNATURE, which seemed weird to me.
> Or...
> > > are you suggesting that we re-validate the SCT as we read it out of the
> cache?
> > 
> > Well, short-term, I was going to suggest we map it to INVALID_SIGNATURE as a
> fuzzy match; it seems better than supporting a deprecated thing in the API
> indefinitely. Long-term, I was going to suggest we don't cache those bits.
They
> only existed to support the CT UI in the Page Info Bubble, we lacked the
notion
> of ephemeral status on the SSLInfo for cached data at the time, and we hadn't
> figured out what the story of SCT validation was going to be (... 3 years
later
> and we're just starting to touch on it, and none of it would reuse the data
from
> the cache in the new plans)
> 
> +1 for mapping to INVALID_SIGNATURE:
> - It is not going to mess up our UMA metrics because these only get logged
when
> the SCT is validated, not when loaded from disk.
> - For all intents an purposes, all client code treats both INVALID statuses
the
> same. 
> - UMA shows INVALID_SIGNATURE is a few orders of magnitudes more likely than
> INVALID_TIMESTAMP, so the mapping will be right 99% of the cases :)

I dunno --  I think it's a pretty high cost that all present and future
consumers can't be sure that INVALID_SIGNATURE means invalid signature. None of
the consumers today care that much about distinguishing invalid signatures from
timestamps, but future consumers might.

[estark, 2016-08-24 15:08:44]

On 2016/08/24 15:07:08, estark wrote:
> On 2016/08/24 11:10:46, Eran Messeri wrote:
> > On 2016/08/24 at 05:37:22, rsleevi wrote:
> > > On 2016/08/24 05:01:41, estark wrote:
> > > > On 2016/08/24 04:57:21, Ryan Sleevi (slow) wrote:
> > > > > Alternatively, why not just map the old value to the new value in
> > > > >
> > > >
> >
>
https://chromium.googlesource.com/chromium/src/+blame/master/net/http/http_re...
> > > > > ?
> > > > 
> > > > If we read out an INVALID value, we'd have to arbitrarily decide to map
it
> > to
> > > > either INVALID_TIMESTAMP or INVALID_SIGNATURE, which seemed weird to me.
> > Or...
> > > > are you suggesting that we re-validate the SCT as we read it out of the
> > cache?
> > > 
> > > Well, short-term, I was going to suggest we map it to INVALID_SIGNATURE as
a
> > fuzzy match; it seems better than supporting a deprecated thing in the API
> > indefinitely. Long-term, I was going to suggest we don't cache those bits.
> They
> > only existed to support the CT UI in the Page Info Bubble, we lacked the
> notion
> > of ephemeral status on the SSLInfo for cached data at the time, and we
hadn't
> > figured out what the story of SCT validation was going to be (... 3 years
> later
> > and we're just starting to touch on it, and none of it would reuse the data
> from
> > the cache in the new plans)
> > 
> > +1 for mapping to INVALID_SIGNATURE:
> > - It is not going to mess up our UMA metrics because these only get logged
> when
> > the SCT is validated, not when loaded from disk.
> > - For all intents an purposes, all client code treats both INVALID statuses
> the
> > same. 
> > - UMA shows INVALID_SIGNATURE is a few orders of magnitudes more likely than
> > INVALID_TIMESTAMP, so the mapping will be right 99% of the cases :)
> 
> I dunno --  I think it's a pretty high cost that all present and future
> consumers can't be sure that INVALID_SIGNATURE means invalid signature. None
of
> the consumers today care that much about distinguishing invalid signatures
from
> timestamps, but future consumers might.

(And DevTools does currently distinguish between the two states, so we would be
introducing an observable bug by mapping INVALID to INVALID_SIGNATURE.)

[Ryan Sleevi, 2016-08-24 16:18:04]

On 2016/08/24 15:08:44, estark wrote:
> On 2016/08/24 15:07:08, estark wrote:
> > I dunno --  I think it's a pretty high cost that all present and future
> > consumers can't be sure that INVALID_SIGNATURE means invalid signature. None
> of
> > the consumers today care that much about distinguishing invalid signatures
> from
> > timestamps, but future consumers might.

If you believe that is a serious concern, then I would argue we should blow up
the cache entries (as we've had to in the past, when changes are backwards
incompatible).

I don't agree, but I also may be missing something, so I do want to suggest an
alternative than keeping an API element around and needing to support it. I LG'd
on the provisional aspect that we'll want to do more here, and I didn't want to
block it too much with discussion (since we're close to branch), but I
definitely don't think we'd want this to live on before the few weeks it'd take
us to discuss, decide, and develop the 'right' fix.

> 
> (And DevTools does currently distinguish between the two states, so we would
be
> introducing an observable bug by mapping INVALID to INVALID_SIGNATURE.)

[estark, 2016-08-24 16:51:21]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 16:52:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-08-24 18:56:28]

Follow-up discussion is on https://crbug.com/640689 about longer-term fixes.

For the short-term, my preference is to leave this CL as-is so as to not show
inaccurate things in DevTools, but I don't feel too strongly either way.

[commit-bot: I haz the power, 2016-08-24 19:45:56]

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

[commit-bot: I haz the power, 2016-08-24 19:45:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nasko, 2016-08-25 00:10:47]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2016-08-25 00:10:48]

content/ LGTM

[estark, 2016-08-25 00:16:00]

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

[estark, 2016-08-25 00:16:00]

felt, could you please review
chrome/browser/ui/website_settings/website_settings.cc?

[felt, 2016-08-25 00:20:07]

chrome/browser/ui/website_settings/website_settings.cc lgtm

[estark, 2016-08-25 00:20:48]

The CQ bit was checked by estark@chromium.org

[estark, 2016-08-25 00:20:48]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2277653002/#ps40001
(title: "fix SCT_STATUS_INVALID comment to not refer to disk cache")

[commit-bot: I haz the power, 2016-08-25 00:21:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 02:58:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-25 03:00:59]

Description was changed from

==========
Bring back SCT_STATUS_INVALID for cached entries

https://codereview.chromium.org/2241213002/ split SCT_STATUS_INVALID
into two new enum values to record the different reasons that an SCT
might be invalid. However, we didn't realize that SCT_STATUS_INVALID
might still be present in disk cache entries, resulting in renderer
kills when the browser tries to deserialize SSLStatus objects containing
these cached SCT_STATUS_INVALID values.

This CL brings back SCT_STATUS_INVALID and documents it as deprecated,
but still existent for the sake of cache entries.

BUG=640296
==========

to

==========
Bring back SCT_STATUS_INVALID for cached entries

https://codereview.chromium.org/2241213002/ split SCT_STATUS_INVALID
into two new enum values to record the different reasons that an SCT
might be invalid. However, we didn't realize that SCT_STATUS_INVALID
might still be present in disk cache entries, resulting in renderer
kills when the browser tries to deserialize SSLStatus objects containing
these cached SCT_STATUS_INVALID values.

This CL brings back SCT_STATUS_INVALID and documents it as deprecated,
but still existent for the sake of cache entries.

BUG=640296

Committed: https://crrev.com/321ed2a53224c50af40387e8211726f8400ecad2
Cr-Commit-Position: refs/heads/master@{#414280}
==========

[commit-bot: I haz the power, 2016-08-25 03:01:00]

Patchset 3 (id:??) landed as
https://crrev.com/321ed2a53224c50af40387e8211726f8400ecad2
Cr-Commit-Position: refs/heads/master@{#414280}