Distinguish between SCT invalidity reasons in UMA

content/common/ssl_status_serialization.cc

Index: content/common/ssl_status_serialization.cc
diff --git a/content/common/ssl_status_serialization.cc b/content/common/ssl_status_serialization.cc
index a105e7ea450fcd953edec497eeb1a87b32eb00c7..1d3d3abc0a7f01055b8e1aaf5089542c1bed0585 100644
--- a/content/common/ssl_status_serialization.cc
+++ b/content/common/ssl_status_serialization.cc
@@ -31,8 +31,9 @@ bool CheckSecurityStyle(int security_style) {
 bool CheckSCTStatus(uint32_t sct_status) {
   switch (sct_status) {
     case net::ct::SCT_STATUS_LOG_UNKNOWN:
-    case net::ct::SCT_STATUS_INVALID:
+    case net::ct::SCT_STATUS_INVALID_SIGNATURE:
     case net::ct::SCT_STATUS_OK:
+    case net::ct::SCT_STATUS_INVALID_TIMESTAMP:
     case net::ct::SCT_STATUS_MAX:

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

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.

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

(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). 

       return true;
     case net::ct::SCT_STATUS_NONE: