Add UMA for consistency between TPM and install attributes.

chrome/browser/chromeos/policy/enterprise_install_attributes.cc

Index: chrome/browser/chromeos/policy/enterprise_install_attributes.cc
diff --git a/chrome/browser/chromeos/policy/enterprise_install_attributes.cc b/chrome/browser/chromeos/policy/enterprise_install_attributes.cc
index 3fbbc8d00d2e38def39a61379e44f1b2625da2e1..d1bd4a3d463b23082a0e61529479453905a8846d 100644
--- a/chrome/browser/chromeos/policy/enterprise_install_attributes.cc
+++ b/chrome/browser/chromeos/policy/enterprise_install_attributes.cc
@@ -11,6 +11,9 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/time/time.h"
 #include "chrome/browser/chromeos/policy/proto/install_attributes.pb.h"
 #include "chromeos/cryptohome/cryptohome_util.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
@@ -22,6 +25,9 @@ namespace cryptohome_util = chromeos::cryptohome_util;
 
 namespace {
 
+// Retry interval for consistency check against TPM lock state.
+int kDbusRetryIntervalInSeconds = 2;

[Mattias Nissler (ping if slow), 2015/06/23 11:34:09]

2 seems a bit too aggressive. How about 10?

[Thiemo Nagel, 2015/06/23 13:17:55]

I'd like to avoid querying the TPM while enrollment runs.  10 seconds seem to be
sufficient to click through OOBE and start enrollment.  Can we compromise on 5
seconds?

[Mattias Nissler (ping if slow), 2015/06/23 13:43:07]

There are devices with notoriously slow TPMs where we take ages to initialize. I
think in these cases, you're racing against the retry loop in
EnrollmentHandlerChromeOS::HandleLockDeviceResult here as well. If you care
about this, then you should properly synchronize the lock operation and the
consistency check. You could just make sure that the consistency check finishes
first before you start locking.

[Thiemo Nagel, 2015/06/23 16:05:03]

Done.

+
 bool ReadMapKey(const std::map<std::string, std::string>& map,
                 const std::string& key,
                 std::string* value) {
@@ -55,7 +61,8 @@ EnterpriseInstallAttributes::GetEnterpriseOwnedInstallAttributesBlobForTesting(
 
 EnterpriseInstallAttributes::EnterpriseInstallAttributes(
     chromeos::CryptohomeClient* cryptohome_client)
-    : device_locked_(false),
+    : lock_state_(STATE_UNKNOWN),
+      dbus_tries_remaining_(30),

[Mattias Nissler (ping if slow), 2015/06/23 11:34:08]

30 tries seems a bit excessive. How about 10?

[Thiemo Nagel, 2015/06/23 13:17:55]

Let's keep the discussion to kDbusRetryIntervalInSeconds and
kDbusRetryDurationInSeconds.

       registration_mode_(DEVICE_MODE_PENDING),
       cryptohome_client_(cryptohome_client),
       weak_ptr_factory_(this) {
@@ -65,11 +72,14 @@ EnterpriseInstallAttributes::~EnterpriseInstallAttributes() {}
 
 void EnterpriseInstallAttributes::ReadCacheFile(
     const base::FilePath& cache_file) {
-  if (device_locked_ || !base::PathExists(cache_file))
-    return;
+  DCHECK(lock_state_ == STATE_UNKNOWN);
 
-  device_locked_ = true;
+  if (!base::PathExists(cache_file)) {
+    lock_state_ = STATE_NOT_LOCKED;

[Mattias Nissler (ping if slow), 2015/06/23 11:34:09]

Note that the cache file will also be missing if lockbox-cache runs into an
error.

[Thiemo Nagel, 2015/06/23 13:17:55]

Yes, this could happen.  Setting STATE_NOT_LOCKED however seems to be best in
line with the present behaviour of device_locked_ = false.

[Mattias Nissler (ping if slow), 2015/06/23 13:43:07]

I guess this is language nitpicking, but the original intention for the
|device_locked_| flag was that a value of true indicates a locked device,
everything else doesn't state anything. Your STATE_NOT_LOCKED seems to convey
more than that.

[Thiemo Nagel, 2015/06/23 16:05:03]

Alright.  After offline discussion, I went back to the binary state and
documented it properly.

+    return;
+  }
 
+  lock_state_ = STATE_LOCKED;
   char buf[16384];
   int len = base::ReadFile(cache_file, buf, sizeof(buf));
   if (len == -1 || len >= static_cast<int>(sizeof(buf))) {
@@ -100,7 +110,7 @@ void EnterpriseInstallAttributes::ReadCacheFile(
 
 void EnterpriseInstallAttributes::ReadImmutableAttributes(
     const base::Closure& callback) {
-  if (device_locked_) {
+  if (lock_state_ == STATE_LOCKED) {
     callback.Run();
     return;
   }
@@ -119,7 +129,7 @@ void EnterpriseInstallAttributes::ReadAttributesIfReady(
     registration_mode_ = DEVICE_MODE_NOT_SET;
     if (!cryptohome_util::InstallAttributesIsInvalid() &&
         !cryptohome_util::InstallAttributesIsFirstInstall()) {
-      device_locked_ = true;
+      lock_state_ = STATE_LOCKED;
 
       static const char* const kEnterpriseAttributes[] = {
         kAttrEnterpriseDeviceId,
@@ -149,11 +159,12 @@ void EnterpriseInstallAttributes::LockDevice(
     const std::string& device_id,
     const LockResultCallback& callback) {
   DCHECK(!callback.is_null());
+  CHECK_NE(lock_state_, STATE_LOCKING);
   CHECK_NE(device_mode, DEVICE_MODE_PENDING);
   CHECK_NE(device_mode, DEVICE_MODE_NOT_SET);
 
   // Check for existing lock first.
-  if (device_locked_) {
+  if (lock_state_ == STATE_LOCKED) {
     if (device_mode != registration_mode_) {
       callback.Run(LOCK_WRONG_MODE);
       return;
@@ -186,6 +197,7 @@ void EnterpriseInstallAttributes::LockDevice(
     return;
   }
 
+  lock_state_ = STATE_LOCKING;
   cryptohome_client_->InstallAttributesIsReady(
       base::Bind(&EnterpriseInstallAttributes::LockDeviceIfAttributesIsReady,
                  weak_ptr_factory_.GetWeakPtr(),
@@ -203,6 +215,7 @@ void EnterpriseInstallAttributes::LockDeviceIfAttributesIsReady(
     chromeos::DBusMethodCallStatus call_status,
     bool result) {
   if (call_status != chromeos::DBUS_METHOD_CALL_SUCCESS || !result) {
+    lock_state_ = STATE_NOT_LOCKED;
     callback.Run(LOCK_NOT_READY);
     return;
   }
@@ -217,12 +230,14 @@ void EnterpriseInstallAttributes::LockDeviceIfAttributesIsReady(
   // Make sure we really have a working InstallAttrs.
   if (cryptohome_util::InstallAttributesIsInvalid()) {
     LOG(ERROR) << "Install attributes invalid.";
+    lock_state_ = STATE_NOT_LOCKED;
     callback.Run(LOCK_BACKEND_INVALID);
     return;
   }
 
   if (!cryptohome_util::InstallAttributesIsFirstInstall()) {
     LOG(ERROR) << "Install attributes already installed.";
+    lock_state_ = STATE_NOT_LOCKED;
     callback.Run(LOCK_ALREADY_LOCKED);
     return;
   }
@@ -237,6 +252,7 @@ void EnterpriseInstallAttributes::LockDeviceIfAttributesIsReady(
     if (!cryptohome_util::InstallAttributesSet(kAttrConsumerKioskEnabled,
                                                "true")) {
       LOG(ERROR) << "Failed writing attributes.";
+      lock_state_ = STATE_NOT_LOCKED;
       callback.Run(LOCK_SET_ERROR);
       return;
     }
@@ -252,6 +268,7 @@ void EnterpriseInstallAttributes::LockDeviceIfAttributesIsReady(
         !cryptohome_util::InstallAttributesSet(kAttrEnterpriseDeviceId,
                                                device_id)) {
       LOG(ERROR) << "Failed writing attributes.";
+      lock_state_ = STATE_NOT_LOCKED;
       callback.Run(LOCK_SET_ERROR);
       return;
     }
@@ -260,6 +277,7 @@ void EnterpriseInstallAttributes::LockDeviceIfAttributesIsReady(
   if (!cryptohome_util::InstallAttributesFinalize() ||
       cryptohome_util::InstallAttributesIsFirstInstall()) {
     LOG(ERROR) << "Failed locking.";
+    lock_state_ = STATE_NOT_LOCKED;
     callback.Run(LOCK_FINALIZE_ERROR);
     return;
   }
@@ -277,24 +295,26 @@ void EnterpriseInstallAttributes::OnReadImmutableAttributes(
 
   if (GetRegistrationUser() != registration_user) {
     LOG(ERROR) << "Locked data doesn't match.";
+    lock_state_ = STATE_NOT_LOCKED;

[Mattias Nissler (ping if slow), 2015/06/23 11:34:08]

AFAICS, this should not update |lock_state_| - ReadImmutableAttributes might
already have set STATE_LOCKED. Even though you might get a registration user
mismatch here, that doesn't mean the device returns to STATE_NOT_LOCKED.

[Thiemo Nagel, 2015/06/23 13:17:55]

Thanks, I agree, this doesn't seem right.

     callback.Run(LOCK_READBACK_ERROR);
     return;
   }
 
+  lock_state_ = STATE_LOCKED;

[Mattias Nissler (ping if slow), 2015/06/23 11:34:09]

Ditto - ReadImmutableAttributes should decide.

[Thiemo Nagel, 2015/06/23 13:17:55]

Ditto.

   callback.Run(LOCK_SUCCESS);
 }
 
 bool EnterpriseInstallAttributes::IsEnterpriseDevice() {
-  return device_locked_ && !registration_user_.empty();
+  return lock_state_ == STATE_LOCKED && !registration_user_.empty();
 }
 
 bool EnterpriseInstallAttributes::IsConsumerKioskDeviceWithAutoLaunch() {
-  return device_locked_ &&
+  return lock_state_ == STATE_LOCKED &&
          registration_mode_ == DEVICE_MODE_CONSUMER_KIOSK_AUTOLAUNCH;
 }
 
 std::string EnterpriseInstallAttributes::GetRegistrationUser() {
-  if (!device_locked_)
+  if (lock_state_ != STATE_LOCKED)
     return std::string();
 
   return registration_user_;
@@ -318,6 +338,36 @@ DeviceMode EnterpriseInstallAttributes::GetMode() {
   return registration_mode_;
 }
 
+void EnterpriseInstallAttributes::TriggerConsistencyCheck() {
+  cryptohome_client_->TpmIsOwned(base::Bind(
+      &EnterpriseInstallAttributes::CheckConsistencyAgainstTpmOwnershipState,
+      weak_ptr_factory_.GetWeakPtr()));
+}
+
+void EnterpriseInstallAttributes::CheckConsistencyAgainstTpmOwnershipState(
+    chromeos::DBusMethodCallStatus call_status,
+    bool result) {
+  if (call_status != chromeos::DBUS_METHOD_CALL_SUCCESS &&
+      dbus_tries_remaining_) {
+    dbus_tries_remaining_--;

[Mattias Nissler (ping if slow), 2015/06/23 11:34:09]

How about binding this as a function parameter that you decrement before the
retry? That way, you don't have to keep it around in class state.

[Thiemo Nagel, 2015/06/23 13:17:55]

I guess that would make sense only if TriggerConsistencyCheck() is made private.
 (But if we make it private, I'm happy to go that way.)

[Mattias Nissler (ping if slow), 2015/06/23 13:43:07]

Yup, this would require a private helper. That's orthogonal to whether there's a
publicly-callable entry point though.

[Thiemo Nagel, 2015/06/23 16:05:03]

Done.

+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&EnterpriseInstallAttributes::TriggerConsistencyCheck,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::TimeDelta::FromSeconds(kDbusRetryIntervalInSeconds));
+    return;
+  }
+
+  base::HistogramBase::Sample state = lock_state_;

[Mattias Nissler (ping if slow), 2015/06/23 11:34:09]

It seems like the code is written in a way s.t. STATE_UNKNOWN and STATE_LOCKING
are not going to show up at all. Except for the remote chance that
TriggerConsistencyCheck executes in parallel to the lock operation.

Given that, does it really make sense to introduce these two enum constants in
the first place?

It looks like the bit you want here is cache-file-present-or-not. You probably
don't even want to handle the case of enrollment here, it's good enough to just
report things for steady state, no?

[Thiemo Nagel, 2015/06/23 13:17:55]

I guess I need STATE_PENDING to catch the case in which the consistency check is
executed while enrollment takes place.  Since I was already adding another
state, I felt it made sense to differentiate between NOT_LOCKED and not
initialized.  Maybe the real problem here is the clumsy semantics of having to
call ReadCacheFile() after the constructor has run.  Probably it would be best
to avoid STATE_UNKNOWN by having the constructor call ReadCacheFile() and to
inject the cache file name as a constructor parameter.  Wdyt?

[Mattias Nissler (ping if slow), 2015/06/23 13:43:07]

Constructors shouldn't perform heavy-weight initialization per the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Doing_Work_in...

Isn't it good enough to start the consistency check in ReadCacheFile()? That
way, you don't need the UNKNOWN state. And if you properly synchronize
consistency checking and locking, then you don't need PENDING either.

[Thiemo Nagel, 2015/06/23 16:05:03]

Alright.  I'm also renaming ReadCacheFile() to Init() to make clear that without
it being called, the object is not properly initialized.
Done that.

+  state |= 0x4 * (registration_mode_ != DEVICE_MODE_ENTERPRISE);

[Mattias Nissler (ping if slow), 2015/06/23 11:34:09]

Why invert this bit?

[Thiemo Nagel, 2015/06/23 13:17:55]

That seeped through from an earlier implementation.  Fixed.

+  if (call_status == chromeos::DBUS_METHOD_CALL_SUCCESS) {

[Mattias Nissler (ping if slow), 2015/06/23 11:34:09]

nit: no curlies for consistency with the rest of the file.

[Thiemo Nagel, 2015/06/23 13:17:55]

Done.

+    state |= 0x8 * result;
+  } else {
+    state |= 0x10;
+  }
+  UMA_HISTOGRAM_ENUMERATION("Enterprise.AttributesTPMConsistency", state, 24);
+}
+
 // Warning: The values for these keys (but not the keys themselves) are stored
 // in the protobuf with a trailing zero.  Also note that some of these constants
 // have been copied to login_manager/device_policy_service.cc.  Please make sure