Certificate Transparency: Collect metrics on age of SCT vs STH

components/certificate_transparency/single_tree_tracker.cc

Index: components/certificate_transparency/single_tree_tracker.cc
diff --git a/components/certificate_transparency/single_tree_tracker.cc b/components/certificate_transparency/single_tree_tracker.cc
index 7946208ca753303a7589260e7a60fd34a58bb5e0..40232d6ee736103441a90a9acbe5245181003e62 100644
--- a/components/certificate_transparency/single_tree_tracker.cc
+++ b/components/certificate_transparency/single_tree_tracker.cc
@@ -6,12 +6,36 @@
 
 #include <utility>
 
+#include "base/metrics/histogram_macros.h"
 #include "net/cert/ct_log_verifier.h"
 #include "net/cert/signed_certificate_timestamp.h"
 #include "net/cert/x509_certificate.h"
 
 using net::ct::SignedTreeHead;
 
+namespace {
+
+// Measure how often clients encounter very new SCTs, by measuring whether an
+// SCT can be checked for inclusion upon first observation.
+//
+// When an SCT is observed, if the SingleTreeTracker instance has a valid STH
+// and the STH covers the SCT (the timestamp in the SCT is less than MMD +
+// timestamp in the STH), this function should be called with |can_be_checked|
+// set to true.
+// If the STH does not cover the SCT (the timestamp in the SCT is greater than
+// MMD + timestamp in the STH), this function should be called with false.
+//
+// If the SingleTreeTracker does not have a valid STH, then this function
+// should not be called as it would not yield meaningful data on how frequently
+// clients encounter very fresh SCTs, as otherwise all observed SCTs would be
+// logged as if they cannot be checked for inclusion, skewing the data.

[Ryan Sleevi, 2016/07/21 18:15:01]

I don't understand this last comment, from a design perspective.

If the client observes an SCT before it has an STH, are you suggesting you
wouldn't store that SCT to disk for future inclusion checks? That it wouldn't
affect the size of the queue of pending SCTs? That it wouldn't affect the
frequency of updates?

That's the non-obvious part of all of this, and it doesn't seem to have been
answered. 28 - 31 doesn't really feel like it explains why, since if your goal
from 18-19 is to measure on first observation, why don't we observe?

Is it because you're not delivering STHs? Is it because you're deferring loading
of STHs? If so, shouldn't you *also* be measuring how often you encounter an SCT
when there's no STHs, to know what's acceptable for the loading/initialization?

[Eran Messeri, 2016/07/21 20:01:33]

If I understand correctly, the question is why I don't log anything related to
the case where the client doesn't have STHs at all in this change. 

Right now there's a Finch experiment related to the regression caused by STHSet
(https://crbug.com/607946) which means some users don't get STH updates at all,
so I know data from these users will add noise. STHs should be loaded right
after start-up, very few connections should be made during that time. 

Generally, it doesn't seem to me like we need to report data from users without
STHs because:
- We have data from the component updater on how many clients receive component
updates and how long does it take for an update to propagate, so we don't need
to count it here.

- The number of users that never get STHs should be very small, but if we
include in this metric every SCT that couldn't be checked because there's no
valid STH at all, clients that don't have valid STHs will always report that
they can't check inclusion, skewing what we're trying to measure.

- Whatever decisions (on cache size, persisting to disk, etc) would also apply
to clients that don't have valid STHs but because the number of these clients
should be very small (waffles@ sent me some data from the component updater,
have to further analyze it), and their behaviour pathological (i.e. such clients
may never get a fresh STH or get it much later after other clients do) I'm not
sure it's sensible to take data from them into account.

Does it all make sense?

[Ryan Sleevi, 2016/07/21 20:14:18]

No, because we can filter out that population of users.
How do you know it's "very few" if you're not measuring it? With the
TaskScheduler work suggesting it might take 10 minutes (or more) before these
could be loaded, that could be a substantial number of communications.
Similarly, on first install, it may be hours. That seems like precisely the
thing you should be measuring.
But you don't have data on how many connections would be potentially affected,
and so any policies you make based upon this data would be inaccurate.
But you can already filter that out.
The fact that the decisions you would make would affect them seems to be
precisely why you should be taking it into account.
I parse your answers, but I do not understand/agree with the conclusions. So
yes/no?

[Eran Messeri, 2016/07/22 10:40:28]

Acknowledged.

+void LogCanBeCheckedForInclusionToUMA(bool can_be_checked) {
+  UMA_HISTOGRAM_BOOLEAN("Net.CertificateTransparency.CanInclusionCheckSCT",
+                        can_be_checked);
+}
+
+}  // namespace
+
 namespace certificate_transparency {
 
 SingleTreeTracker::SingleTreeTracker(
@@ -34,15 +58,18 @@ void SingleTreeTracker::OnSCTVerified(
   if (verified_sth_.timestamp.is_null() ||
       (verified_sth_.timestamp <
        (sct->timestamp + base::TimeDelta::FromHours(24)))) {
-    // TODO(eranm): UMA - how often SCTs have to wait for a newer STH for
-    // inclusion check.
     entries_status_.insert(
         std::make_pair(sct->timestamp, SCT_PENDING_NEWER_STH));
+
+    // Do not log histogram if there's no STH for this log yet, as it does
+    // not provide any meaningful data on how fresh SCTs usually are.
+    if (!verified_sth_.timestamp.is_null())
+      LogCanBeCheckedForInclusionToUMA(false);
     return;
   }
 
+  LogCanBeCheckedForInclusionToUMA(true);
   // TODO(eranm): Check inclusion here.
-  // TODO(eranm): UMA - how often inclusion can be checked immediately.
   entries_status_.insert(
       std::make_pair(sct->timestamp, SCT_PENDING_INCLUSION_CHECK));
 }