Finished implementing AttestationPolicyObserver.

chrome/browser/chromeos/attestation/attestation_policy_observer.cc

 // Copyright (c) 2013 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 "chrome/browser/chromeos/attestation/attestation_policy_observer.h"
 
 #include <string>
 
 #include "base/bind.h"
+#include "base/callback.h"
+#include "base/time.h"
 #include "chrome/browser/chromeos/attestation/attestation_ca_client.h"
+#include "chrome/browser/chromeos/attestation/attestation_key_payload.pb.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/policy/cloud/cloud_policy_client.h"
 #include "chrome/browser/policy/cloud/cloud_policy_manager.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chromeos/attestation/attestation_flow.h"
 #include "chromeos/cryptohome/async_method_caller.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
+#include "net/cert/x509_certificate.h"
 
 namespace {
 
 // A dbus callback which handles a boolean result.
 //
 // Parameters
 //   on_true - Called when status=success and value=true.
 //   on_false - Called when status=success and value=false.
 //   status - The dbus operation status.
 //   value - The value returned by the dbus operation.
@@ skipping 130 matching lines @@
   cryptohome_client_->TpmAttestationGetCertificate(
       CryptohomeClient::DEVICE_KEY,
       kEnterpriseMachineKey,
       base::Bind(DBusStringCallback,
                  base::Bind(&AttestationPolicyObserver::CheckCertificateExpiry,
                             weak_factory_.GetWeakPtr())));
 }
 
 void AttestationPolicyObserver::CheckCertificateExpiry(
     const std::string& certificate) {
-  // TODO(dkrahn): Check if the certificate will expire soon, for now assume no.
-  CheckIfUploaded(certificate);
+  scoped_refptr<net::X509Certificate> x509(
+      net::X509Certificate::CreateFromBytes(certificate.data(),
+                                            certificate.length()));
+  if (!x509.get() || x509->valid_expiry().is_null()) {
+    LOG(WARNING) << "Failed to parse certificate, cannot check expiry.";
+  } else {
+    base::TimeDelta threshold = base::TimeDelta::FromDays(30);

[Mattias Nissler (ping if slow), 2013/04/22 10:25:42]

const

We usually put constant decls in an anonymous namespace at the top of the file.

Also, is 30 maybe a bit on the large side? What validity periods do we expect
people to use?

[dkrahn, 2013/04/22 22:33:31]

Added a constant member to the class.  

The PCA issues certs with at least a 2 year expiry.  I chose 30 days here
because this logic may only run when the user logs in and I want to replace the
majority of certs before they expire.  If I make this only a few days many users
will not logout / login during that period.  However, 30 is still pretty
arbitrary and not backed by any real data.

[Mattias Nissler (ping if slow), 2013/04/23 10:53:01]

Fair. Would be good to include this additional context in a comment.

+    if ((base::Time::Now() + threshold) > x509->valid_expiry()) {
+      // The certificate has expired or will soon, replace it.
+      GetNewCertificate();
+      return;
+    }
+  }
+
+  // Get the payload and check if the certificate has already been uploaded.
+  GetKeyPayload(base::Bind(&AttestationPolicyObserver::CheckIfUploaded,
+                           weak_factory_.GetWeakPtr(),
+                           certificate));
 }
 
 void AttestationPolicyObserver::UploadCertificate(
     const std::string& certificate) {
-  // TODO(dkrahn): Upload the certificate.
+  policy_client_->UploadCertificate(
+      certificate,
+      base::Bind(&AttestationPolicyObserver::OnUploadComplete,
+                 weak_factory_.GetWeakPtr()));
 }
 
 void AttestationPolicyObserver::CheckIfUploaded(
-    const std::string& certificate) {
-  // TODO(dkrahn): Check if we've already uploaded the certificate.
+    const std::string& certificate,
+    const std::string& key_payload) {
+  AttestationKeyPayload payload_pb;
+  if (!key_payload.empty() &&
+      payload_pb.ParseFromString(key_payload) &&
+      payload_pb.is_certificate_uploaded()) {
+    // Already uploaded... nothing more to do.
+    return;
+  }
+  UploadCertificate(certificate);
+}
+
+void AttestationPolicyObserver::GetKeyPayload(
+    base::Callback<void(const std::string&)> callback) {
+  cryptohome_client_->TpmAttestationGetKeyPayload(
+      CryptohomeClient::DEVICE_KEY,
+      kEnterpriseMachineKey,
+      base::Bind(DBusStringCallback, callback));
+}
+
+void AttestationPolicyObserver::OnUploadComplete(bool status) {
+  if (!status)
+    return;
+  GetKeyPayload(base::Bind(&AttestationPolicyObserver::MarkAsUploaded,
+                           weak_factory_.GetWeakPtr()));
+}
+
+void AttestationPolicyObserver::MarkAsUploaded(const std::string& key_payload) {
+  AttestationKeyPayload payload_pb;
+  if (!key_payload.empty())
+    payload_pb.ParseFromString(key_payload);
+  payload_pb.set_is_certificate_uploaded(true);
+  std::string new_payload;
+  if (!payload_pb.SerializeToString(&new_payload)) {
+    LOG(WARNING) << "Failed to serialize key payload.";
+    return;
+  }
+  cryptohome_client_->TpmAttestationSetKeyPayload(
+      CryptohomeClient::DEVICE_KEY,
+      kEnterpriseMachineKey,
+      new_payload,
+      base::Bind(DBusBoolRedirectCallback, base::Closure(), base::Closure()));

[Mattias Nissler (ping if slow), 2013/04/22 10:25:42]

Here and elsewhere: Should we have some logging in strategic places to be able
to diagnose failures of various sorts? Communication problems with cryptohome
has been a source of problems in the past.

[dkrahn, 2013/04/22 22:33:31]

Added logging to dbus callbacks which reports failed method name.

 }
 
 }  // namespace attestation
 }  // namespace chromeos