Distinguish between SCT invalidity reasons in UMA

Distinguish between SCT invalidity reasons in UMA

As detailed in the bug, Signed Certificate Timestamps (SCTs) can be marked as invalid for two reasons. This change splits the invalid SCT status into two:
* Invalid timestamp.
* Invalid signature.

Since right now we'd only like to collect data on whether invalidity is due to an ecosystem problem (invalid signatures) or misconfigured clients (invalid timestamp), the change only affects the metrics we collect.
All other consumers of the SCT verification status have been adjusted to treat both statuses the same.

Per rsleevi's recommendation, the SCTVerifyStatus enum has been expanded and the value for the old invalid status is no longer used.

BUG=634006
Committed: https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d
Cr-Commit-Position: refs/heads/master@{#412779}

[Eran Messeri, 2016-08-15 12:22:28]

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

[Eran Messeri, 2016-08-15 12:22:42]

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

[commit-bot: I haz the power, 2016-08-15 12:22:53]

Dry run: 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-15 12:27:03]

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

[commit-bot: I haz the power, 2016-08-15 12:27:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Eran Messeri, 2016-08-15 12:58:42]

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

[commit-bot: I haz the power, 2016-08-15 13:03:03]

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

[Eran Messeri, 2016-08-15 13:07:42]

Eric, Emily, PTAL at this final change to resolve crbug.com/634006 by actually
changing the SCTVerifyStatus enum.

Note I'll add more owners to review once you approve of the code.

[estark, 2016-08-15 13:22:08]

lgtm with a couple nits

https://codereview.chromium.org/2241213002/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/2241213002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:156: case
net::ct::SCT_STATUS_INVALID_TIMESTAMP:
Just thinking out loud -- I suppose there's no reason to separate these in the
report format, right? We send the client timestamp with the report, so the
server receiving the reports can distinguish an invalid timestamp from an
invalid signature by itself. Do you agree?

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

https://codereview.chromium.org/2241213002/diff/20001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:23: 
Maybe leave a comment here to indicate that 2 used to exist but is obsolete now:
"SCTVerifyStatus=2 used to represent SCT_STATUS_INVALID, which has now been
split into INVALID_SIGNATURE and INVALID_TIMESTAMP to represent the different
reasons an SCT could be invalid."
(or something like that)

https://codereview.chromium.org/2241213002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2241213002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91511: -  <int value="2"
label="SCT_STATUS_INVALID"/>
A histograms owner will tell you for sure but it looks like the common practice
might be to leave the value and mark it as deprecated, e.g.
https://cs.chromium.org/chromium/src/tools/metrics/histograms/histograms.xml?...

[commit-bot: I haz the power, 2016-08-15 13:58:53]

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

[commit-bot: I haz the power, 2016-08-15 13:58:54]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-08-15 19:20:22]

lgtm

https://codereview.chromium.org/2241213002/diff/20001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2241213002/diff/20001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:37: case net::ct::SCT_STATUS_MAX:
Why is MAX considered valid?

From the definition:

  // The SCT is from a known log, and the signature is valid.
  SCT_STATUS_OK = 3,

  // Used to bound the enum values.
  SCT_STATUS_MAX,


Meaning MAX is 4, which does not map to an actual status.

I don't know the surrounding code, but my usual answer to this code pattern is
to define things as:

  SCT_STATUS_LAST = SCT_STATUS_OK,

Then you don't need to explicitly handle it in switch statements.

https://codereview.chromium.org/2241213002/diff/20001/net/cert/ct_sct_to_stri...
File net/cert/ct_sct_to_string.cc (right):

https://codereview.chromium.org/2241213002/diff/20001/net/cert/ct_sct_to_stri...
net/cert/ct_sct_to_string.cc:73: case SCT_STATUS_MAX:
See earlier comment -- we can remove handling of MAX unless there is a good
reason to have it in the code.

[Eran Messeri, 2016-08-16 14:10:21]

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

[Eran Messeri, 2016-08-16 14:13:44]

eranm@chromium.org changed reviewers:
+ avi@chromium.org

[Eran Messeri, 2016-08-16 14:17:18]

This change doesn't introduce any behaviour changes, but provides finer-grained
metrics in UMA (for the SCT validation status).

felt, added you as a reviewer for files under chrome/browser.
avi, added you as reviewer for content/common/ssl_status_serialization*
(comments on all other files are, of course, always welcome!).

PTAL.

https://codereview.chromium.org/2241213002/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/2241213002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:156: case
net::ct::SCT_STATUS_INVALID_TIMESTAMP:
On 2016/08/15 at 13:22:08, estark wrote:
> Just thinking out loud -- I suppose there's no reason to separate these in the
report format, right? We send the client timestamp with the report, so the
server receiving the reports can distinguish an invalid timestamp from an
invalid signature by itself. Do you agree?

(per out-of-band discussion)
The server could distinguish if it had the SCT - but we don't send invalid SCTs.
Having the SCTs with a timestamp "in the future" would be useful - if the
timestamp is indeed in the future then this demonstrates log misbehaviour.

Right now I think we could treat both the same (not depart from current
behaviour in this change), later consider changing the reporting format to
include the SCT status and report SCTs that are invalid due to status.

https://codereview.chromium.org/2241213002/diff/20001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2241213002/diff/20001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:37: case net::ct::SCT_STATUS_MAX:
On 2016/08/15 at 19:20:22, eroman wrote:
> Why is MAX considered valid?
> 
> From the definition:
> 
>   // The SCT is from a known log, and the signature is valid.
>   SCT_STATUS_OK = 3,
> 
>   // Used to bound the enum values.
>   SCT_STATUS_MAX,
> 
> 
> Meaning MAX is 4, which does not map to an actual status.
> 
> I don't know the surrounding code, but my usual answer to this code pattern is
to define things as:
> 
>   SCT_STATUS_LAST = SCT_STATUS_OK,
> 
> Then you don't need to explicitly handle it in switch statements.

(1) MAX shouldn't be considered valid - I've moved it to the SCT_STATUS_NONE
case.
(2) The SCTVerifyStatus enum is used in a histogram, and the common pattern
seems to be to have a _MAX value that is passed into the
UMA_HISTOGRAM_ENUMERATION macro as the boundary value.

I do not mind changing _MAX to _LAST as you suggested and passing
SCT_STATUS_LAST + 1 to the histogram macro - just confirm you're happy with this
deviation from the common approach (I also see a slight disadvantage with the
_LAST approach is that it has to be changed in addition to the new values being
added, whereas with the _MAX the only thing to be aware of is that new values
should be added before it).

https://codereview.chromium.org/2241213002/diff/20001/net/cert/ct_sct_to_stri...
File net/cert/ct_sct_to_string.cc (right):

https://codereview.chromium.org/2241213002/diff/20001/net/cert/ct_sct_to_stri...
net/cert/ct_sct_to_string.cc:73: case SCT_STATUS_MAX:
On 2016/08/15 at 19:20:22, eroman wrote:
> See earlier comment -- we can remove handling of MAX unless there is a good
reason to have it in the code.

See my response above - it is a bit of a nuisance that the enum is used directly
for a histogram and so has this _MAX value. But that seems like a common
pattern:
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?q=UMA_HI...

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

https://codereview.chromium.org/2241213002/diff/20001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:23: 
On 2016/08/15 at 13:22:08, estark wrote:
> Maybe leave a comment here to indicate that 2 used to exist but is obsolete
now:
> "SCTVerifyStatus=2 used to represent SCT_STATUS_INVALID, which has now been
split into INVALID_SIGNATURE and INVALID_TIMESTAMP to represent the different
reasons an SCT could be invalid."
> (or something like that)

Done, thanks for the text.

https://codereview.chromium.org/2241213002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2241213002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91511: -  <int value="2"
label="SCT_STATUS_INVALID"/>
On 2016/08/15 at 13:22:08, estark wrote:
> A histograms owner will tell you for sure but it looks like the common
practice might be to leave the value and mark it as deprecated, e.g.
https://cs.chromium.org/chromium/src/tools/metrics/histograms/histograms.xml?...

Added back the invalid status, prefixing with DEPRECATED, as suggested.

[Avi (use Gerrit), 2016-08-16 15:25:47]

https://codereview.chromium.org/2241213002/diff/40001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2241213002/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:39: case net::ct::SCT_STATUS_MAX:
This is wrong (see my other note). MAX shouldn't be its own value, so it
shouldn't be in this switch at all.

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

https://codereview.chromium.org/2241213002/diff/40001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:38: SCT_STATUS_MAX,
That's not how MAX works; you assign MAX to the last valid value in the enum:

SCT_STATUS_MAX = SCT_STATUS_INVALID_TIMESTAMP,

(See https://cs.chromium.org/search/?q=FILTER_TYPE_LAST or
https://cs.chromium.org/search/?q=EVENT_MEDIA_TYPE_LAST or any number of other
enums.)

The issue is that SCT_STATUS_MAX is used in content/common/resource_messages.h
with:

IPC_ENUM_TRAITS_MAX_VALUE(net::ct::SCTVerifyStatus, net::ct::SCT_STATUS_MAX)

but if you look at the definition of IPC_ENUM_TRAITS_MAX_VALUE it says:

// Convenience macro for defining enumerated type traits for types which are
// range-checked by the IPC system to be in the range of 0..maxvalue inclusive.
// This macro should not need to be subsequently redefined.

so we're currently allowing the wrong range of enum values to pass over IPC.

We should fix this here in this CL.

[Eran Messeri, 2016-08-16 16:06:09]

Avi, thanks for the quick review. PTAL - fixed as you suggested.

https://codereview.chromium.org/2241213002/diff/40001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2241213002/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:39: case net::ct::SCT_STATUS_MAX:
On 2016/08/16 at 15:25:47, Avi wrote:
> This is wrong (see my other note). MAX shouldn't be its own value, so it
shouldn't be in this switch at all.

Done, removed.

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

https://codereview.chromium.org/2241213002/diff/40001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:38: SCT_STATUS_MAX,
On 2016/08/16 at 15:25:47, Avi wrote:
> That's not how MAX works; you assign MAX to the last valid value in the enum:
> 
> SCT_STATUS_MAX = SCT_STATUS_INVALID_TIMESTAMP,
> 
> (See https://cs.chromium.org/search/?q=FILTER_TYPE_LAST or
https://cs.chromium.org/search/?q=EVENT_MEDIA_TYPE_LAST or any number of other
enums.)
> 
> The issue is that SCT_STATUS_MAX is used in content/common/resource_messages.h
with:
> 
> IPC_ENUM_TRAITS_MAX_VALUE(net::ct::SCTVerifyStatus, net::ct::SCT_STATUS_MAX)
> 
> but if you look at the definition of IPC_ENUM_TRAITS_MAX_VALUE it says:
> 
> // Convenience macro for defining enumerated type traits for types which are
> // range-checked by the IPC system to be in the range of 0..maxvalue
inclusive.
> // This macro should not need to be subsequently redefined.
> 
> so we're currently allowing the wrong range of enum values to pass over IPC.
> 
> We should fix this here in this CL.

Fixed.
The discrepancy comes, I believe, from using the same enum for UMA and IPC. The
convention when used for UMA is to have a _MAX value that's after the last valid
value (since UMA_HISTOGRAM_ENUMERATION  states it expects values that are
strictly less than the boundary value).

[Eran Messeri, 2016-08-16 16:07:39]

eranm@chromium.org changed reviewers:
+ isherman@chromium.org

[Eran Messeri, 2016-08-16 16:08:29]

isherman: Please review the histograms.xml change, particularly the way two
values were added to the histogram, replacing a single value that used to
represent both cases.

[Avi (use Gerrit), 2016-08-16 16:53:24]

LGTM

I see, and it's rather unfortunate that we've set things up so that we can't
easily reuse enums for UMA without being tricky.

[Ilya Sherman, 2016-08-16 20:42:14]

histograms.xml lgtm -- this is indeed exactly the way to split a single enum
entry into two.  Thanks!

[Eran Messeri, 2016-08-17 12:41:18]

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

[commit-bot: I haz the power, 2016-08-17 12:41:31]

Dry run: 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-17 14:09:11]

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

[commit-bot: I haz the power, 2016-08-17 14:09:12]

Dry run: This issue passed the CQ dry run.

[Eran Messeri, 2016-08-17 16:11:50]

eranm@chromium.org changed reviewers:
+ raymes@chromium.org
- felt@chromium.org

[Eran Messeri, 2016-08-17 16:14:11]

(switching reviewers as felt@ seems to be out today)
raymes, please review the changes to
chrome/browser/ui/website_settings/website_settings*. Note no functional changes
affecting the UI.

[raymes, 2016-08-18 01:05:01]

website_settings rs lgtm

[Eran Messeri, 2016-08-18 09:56:07]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2016-08-18 09:56:07]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2016-08-18 09:56:07]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2241213002/#ps60001
(title: "Changing _MAX to be the last value")

[Eran Messeri, 2016-08-18 09:56:07]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2241213002/#ps60001
(title: "Changing _MAX to be the last value")

[commit-bot: I haz the power, 2016-08-18 09:56:22]

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-18 10:00:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-18 10:02:44]

Description was changed from

==========
Distinguish between SCT invalidity reasons in UMA

As detailed in the bug, Signed Certificate Timestamps (SCTs) can be marked as
invalid for two reasons. This change splits the invalid SCT status into two:
* Invalid timestamp.
* Invalid signature.

Since right now we'd only like to collect data on whether invalidity is due to
an ecosystem problem (invalid signatures) or misconfigured clients (invalid
timestamp), the change only affects the metrics we collect.
All other consumers of the SCT verification status have been adjusted to treat
both statuses the same.

Per rsleevi's recommendation, the SCTVerifyStatus enum has been expanded and the
value for the old invalid status is no longer used.

BUG=634006
==========

to

==========
Distinguish between SCT invalidity reasons in UMA

As detailed in the bug, Signed Certificate Timestamps (SCTs) can be marked as
invalid for two reasons. This change splits the invalid SCT status into two:
* Invalid timestamp.
* Invalid signature.

Since right now we'd only like to collect data on whether invalidity is due to
an ecosystem problem (invalid signatures) or misconfigured clients (invalid
timestamp), the change only affects the metrics we collect.
All other consumers of the SCT verification status have been adjusted to treat
both statuses the same.

Per rsleevi's recommendation, the SCTVerifyStatus enum has been expanded and the
value for the old invalid status is no longer used.

BUG=634006

Committed: https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d
Cr-Commit-Position: refs/heads/master@{#412779}
==========

[commit-bot: I haz the power, 2016-08-18 10:02:45]

Patchset 4 (id:??) landed as
https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d
Cr-Commit-Position: refs/heads/master@{#412779}