Distinguish between SCT invalidity reasons in UMA

content/common/ssl_status_serialization.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/ssl_status_serialization.h"
 
 #include <stdint.h>
 
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
@@ skipping 13 matching lines @@
       return true;
   }
   return false;
 }
 
 // Checks that an integer |sct_status| is a valid net::ct::SCTVerifyStatus enum
 // value. Returns true if valid, false otherwise.
 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:
       // SCT_STATUS_NONE should never happen, so it isn't valid to
       // receive a status of NONE in a serialized SSLStatus.
       return false;
   }
   return false;
 }
 
 }  // namespace
@@ skipping 72 matching lines @@
   // Sanity check |key_exchange_info|: 0 or greater.
   if (ssl_status->key_exchange_info < 0) {
     *ssl_status = SSLStatus();
     return false;
   }
 
   return true;
 }
 
 }  // namespace content