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..4db35008310069fc7153eb8a75b3ab447f4d550a 100644
--- a/chrome/browser/chromeos/policy/enterprise_install_attributes.cc
+++ b/chrome/browser/chromeos/policy/enterprise_install_attributes.cc
@@ -55,7 +55,7 @@ EnterpriseInstallAttributes::GetEnterpriseOwnedInstallAttributesBlobForTesting(
 
 EnterpriseInstallAttributes::EnterpriseInstallAttributes(
     chromeos::CryptohomeClient* cryptohome_client)
-    : device_locked_(false),
+    : lock_state_(STATE_UNKNOWN),
       registration_mode_(DEVICE_MODE_PENDING),
       cryptohome_client_(cryptohome_client),
       weak_ptr_factory_(this) {
@@ -65,21 +65,25 @@ 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;

[Mattias Nissler (ping if slow), 2015/06/19 07:32:45]

You're changing semantics here. If the file is present, we should assume the
device is locked, even if we fail to make sense of the file contents.

[Thiemo Nagel, 2015/06/19 17:47:04]

That was intentional, but I forgot the intention behind it.  I guess I'll change
it back.  (I don't think it matters a lot.)

+  if (!base::PathExists(cache_file)) {
+    lock_state_ = STATE_NOT_LOCKED;
+    return;
+  }
 
   char buf[16384];
   int len = base::ReadFile(cache_file, buf, sizeof(buf));
   if (len == -1 || len >= static_cast<int>(sizeof(buf))) {
     PLOG(ERROR) << "Failed to read " << cache_file.value();
+    lock_state_ = STATE_NOT_LOCKED;
     return;
   }
 
   cryptohome::SerializedInstallAttributes install_attrs_proto;
   if (!install_attrs_proto.ParseFromArray(buf, len)) {
     LOG(ERROR) << "Failed to parse install attributes cache";
+    lock_state_ = STATE_NOT_LOCKED;
     return;
   }
 
@@ -96,11 +100,12 @@ void EnterpriseInstallAttributes::ReadCacheFile(
   }
 
   DecodeInstallAttributes(attr_map);
+  lock_state_ = STATE_LOCKED;
 }
 
 void EnterpriseInstallAttributes::ReadImmutableAttributes(
     const base::Closure& callback) {
-  if (device_locked_) {
+  if (lock_state_ == STATE_LOCKED) {
     callback.Run();
     return;
   }
@@ -119,7 +124,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 +154,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 +192,7 @@ void EnterpriseInstallAttributes::LockDevice(
     return;
   }
 
+  lock_state_ = STATE_LOCKING;
   cryptohome_client_->InstallAttributesIsReady(
       base::Bind(&EnterpriseInstallAttributes::LockDeviceIfAttributesIsReady,
                  weak_ptr_factory_.GetWeakPtr(),
@@ -203,6 +210,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 +225,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 +247,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 +263,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 +272,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 +290,26 @@ void EnterpriseInstallAttributes::OnReadImmutableAttributes(
 
   if (GetRegistrationUser() != registration_user) {
     LOG(ERROR) << "Locked data doesn't match.";
+    lock_state_ = STATE_NOT_LOCKED;
     callback.Run(LOCK_READBACK_ERROR);
     return;
   }
 
+  lock_state_ = STATE_LOCKED;
   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 +333,31 @@ DeviceMode EnterpriseInstallAttributes::GetMode() {
   return registration_mode_;
 }
 
+void EnterpriseInstallAttributes::CheckConsistency() {

[Mattias Nissler (ping if slow), 2015/06/19 07:32:45]

The code you have here purely tests whether the crypthome DBus interface
supplies consistent values. I don't think that TpmIsOwned is of any relevance
though - it's merely an artifact of how install attributes are implemented in
cryptohome. I think what you really want to test is whether device ownership
agrees with install attributes. Status of the former can be obtained via
DeviceSettingsService.

[Thiemo Nagel, 2015/06/19 17:47:03]

As far as I can see, TpmIsOwned() calls through to the TPM.  In my tests, I can
observe the combination of install attributes missing but TPM reporting as
locked.

[Mattias Nissler (ping if slow), 2015/06/22 07:24:50]

I'm sure you can actually observe this situation. The question however is why
this is relevant for enrollment? IIRC, the goal was to detect whether install
attributes say we're enterprise while device ownership shows a consumer user?
Maybe I'm missing something - just convince me that the condition you're
detecting here is actually interesting to us.

[Thiemo Nagel, 2015/06/24 10:43:35]

Quoting Mattias: 
"For posterity: Offline discussions clarified that this CL is meant to detect
the
situation of stateful data loss, thus comparing TPM ownership state with install
attributes presence on disk is a useful signal."

+  LOG(ERROR) << "CheckConsistency()";
+
+  // LockDeviceIfAttributesIsReady() is not atomic between locking TPM and
+  // reading back install attributes.
+  cryptohome_client_->TpmIsOwned(base::Bind(
+      &EnterpriseInstallAttributes::CheckConsistencyAgainstTpmOwnershipState,
+      weak_ptr_factory_.GetWeakPtr()));
+}
+
+void EnterpriseInstallAttributes::CheckConsistencyAgainstTpmOwnershipState(
+    chromeos::DBusMethodCallStatus call_status,
+    bool result) {
+  unsigned state = lock_state_;
+  state |= 0x4 * (registration_mode_ != DEVICE_MODE_ENTERPRISE);
+  if (call_status == chromeos::DBUS_METHOD_CALL_SUCCESS) {
+    state |= 0x8 * result;
+  } else {
+    state |= 0x10;
+  }
+  UMA_HISTOGRAM_ENUMERATION("Enterprise.AttributesTPMConsistency", state, 24);
+  LOG(ERROR) << "lock_state_: " << lock_state_;
+  LOG(ERROR) << "State: " << state;
+}
+
 // 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