Finished implementing AttestationPolicyObserver.

chrome/browser/chromeos/attestation/attestation_policy_observer.cc

Index: chrome/browser/chromeos/attestation/attestation_policy_observer.cc
diff --git a/chrome/browser/chromeos/attestation/attestation_policy_observer.cc b/chrome/browser/chromeos/attestation/attestation_policy_observer.cc
index 21664cde31c1188311062f51478bc9a4f81ce67d..e333f0910b879e82f2fe47fd072ef225bd9609ce 100644
--- a/chrome/browser/chromeos/attestation/attestation_policy_observer.cc
+++ b/chrome/browser/chromeos/attestation/attestation_policy_observer.cc
@@ -7,7 +7,10 @@
 #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"
@@ -19,6 +22,7 @@
 #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 {
 
@@ -169,18 +173,77 @@ void AttestationPolicyObserver::GetExistingCertificate() {
 
 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