Add the Sign-At-Most-Once restriction the enterprise.platformKeys API.

chrome/browser/chromeos/platform_keys/platform_keys_service.cc

+// Copyright 2014 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/platform_keys/platform_keys_service.h"
+
+#include "base/base64.h"
+#include "base/callback.h"
+#include "base/values.h"
+#include "chrome/browser/chromeos/platform_keys/platform_keys.h"
+#include "chrome/browser/extensions/state_store.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace chromeos {
+
+namespace {
+
+const char kErrorInternal[] = "Internal Error.";
+const char kErrorKeyNotAllowedForSigning[] =
+    "This key is not allowed for signing. Either it was used for signing "
+    "before or it was not correctly generated.";
+const char kStateStorePlatformKeys[] = "PlatformKeys";
+
+scoped_ptr<base::StringValue> GetPublicKeyValue(
+    const std::string& public_key_spki_der) {
+  std::string public_key_spki_der_b64;
+  base::Base64Encode(public_key_spki_der, &public_key_spki_der_b64);
+  return make_scoped_ptr(new base::StringValue(public_key_spki_der_b64));
+}
+
+// Wraps |callback| into a void(bool) callback which forwards
+// |public_key_spki_der| if |true| is passed to it.
+void WrapGenerateKeyCallback(
+    const PlatformKeysService::GenerateKeyCallback& callback,
+    const std::string& public_key_spki_der,
+    bool success) {
+  if (success)
+    callback.Run(public_key_spki_der, std::string() /* no error */);
+  else
+    callback.Run(std::string() /* no public key */, kErrorInternal);
+}
+
+// Callback used by |PlatformKeysService::Sign|.
+// Is called with the old validity of |public_key| (or false if an error
+// occurred during reading the StateStore). If allowed, starts the actual
+// signing operation which will call back |callback|. If not allowd, calls

[eroman, 2014/06/12 05:50:45]

typo: allowd -> allowed

[pneubeck (no reviews), 2014/06/12 09:21:37]

Done.

+// |callback| with an error.
+void CheckValidityAndSign(const std::string& token_id,
+                          const std::string& public_key,
+                          const std::string& data,
+                          const PlatformKeysService::SignCallback& callback,
+                          Profile* profile,
+                          bool key_is_valid) {
+  if (!key_is_valid) {
+    callback.Run(std::string() /* no signature */,
+                 kErrorKeyNotAllowedForSigning);
+    return;
+  }
+  platform_keys::subtle::Sign(token_id, public_key, data, callback, profile);
+}
+
+}  // namespace
+
+PlatformKeysService::PlatformKeysService(extensions::StateStore* state_store)
+    : state_store_(state_store), weak_factory_(this) {
+  DCHECK(state_store);
+}
+
+PlatformKeysService::~PlatformKeysService() {
+}
+
+void PlatformKeysService::GenerateRSAKey(const std::string& token_id,
+                                         unsigned int modulus_length,
+                                         const std::string& extension_id,
+                                         const GenerateKeyCallback& callback,
+                                         Profile* profile) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  platform_keys::subtle::GenerateRSAKey(token_id,
+                                        modulus_length,
+                                        base::Bind(&GenerateRSAKeyCallback,
+                                                   extension_id,
+                                                   callback,
+                                                   weak_factory_.GetWeakPtr()),
+                                        profile);
+}
+
+void PlatformKeysService::Sign(const std::string& token_id,
+                               const std::string& public_key,
+                               const std::string& data,
+                               const std::string& extension_id,
+                               const SignCallback& callback,
+                               Profile* profile) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  ReadValidityAndInvalidateKey(extension_id,
+                               public_key,
+                               base::Bind(&CheckValidityAndSign,
+                                          token_id,
+                                          public_key,
+                                          data,
+                                          callback,
+                                          profile));
+}
+
+void PlatformKeysService::RegisterPublicKey(
+    const std::string& extension_id,
+    const std::string& public_key_spki_der,
+    const base::Callback<void(bool)>& callback) {
+  GetPlatformKeysOfExtension(extension_id,
+                             base::Bind(&RegisterPublicKeyGotPlatformKeys,
+                                        extension_id,
+                                        public_key_spki_der,
+                                        callback));
+}
+
+void PlatformKeysService::ReadValidityAndInvalidateKey(
+    const std::string& extension_id,
+    const std::string& public_key_spki_der,
+    const base::Callback<void(bool)>& callback) {
+  GetPlatformKeysOfExtension(
+      extension_id,
+      base::Bind(&InvalidateKey, extension_id, public_key_spki_der, callback));
+}
+
+void PlatformKeysService::GetPlatformKeysOfExtension(
+    const std::string& extension_id,
+    const GetPlatformKeysCallback& callback) {
+  state_store_->GetExtensionValue(extension_id,
+                                  kStateStorePlatformKeys,
+                                  base::Bind(&GotPlatformKeysOfExtension,
+                                             extension_id,
+                                             weak_factory_.GetWeakPtr(),
+                                             callback));
+}
+
+// static
+void PlatformKeysService::GenerateRSAKeyCallback(
+    const std::string& extension_id,
+    const GenerateKeyCallback& callback,
+    base::WeakPtr<PlatformKeysService> service,
+    const std::string& public_key_spki_der,
+    const std::string& error_message) {
+  if (!error_message.empty()) {
+    callback.Run(std::string() /* no public key */, error_message);
+    return;
+  }
+  base::Callback<void(bool)> wrapped_callback(
+      base::Bind(&WrapGenerateKeyCallback, callback, public_key_spki_der));
+  if (!service) {
+    LOG(ERROR) << "PlatformKeysService is already destructed.";
+    wrapped_callback.Run(false);
+    return;
+  }
+  service->RegisterPublicKey(
+      extension_id, public_key_spki_der, wrapped_callback);
+}
+
+// static
+void PlatformKeysService::RegisterPublicKeyGotPlatformKeys(
+    const std::string& extension_id,
+    const std::string& public_key_spki_der,
+    const base::Callback<void(bool)>& callback,
+    base::WeakPtr<PlatformKeysService> service,
+    scoped_ptr<base::ListValue> platform_keys) {
+  if (!service) {
+    LOG(ERROR) << "PlatformKeysService is already destructed.";
+    callback.Run(false);
+    return;
+  }
+
+  if (!platform_keys) {
+    LOG(ERROR) << "Error while reading the platform keys.";
+    callback.Run(false);
+    return;
+  }
+
+  scoped_ptr<base::StringValue> key_value(
+      GetPublicKeyValue(public_key_spki_der));
+
+  DCHECK(platform_keys->end() == platform_keys->Find(*key_value))

[eroman, 2014/06/12 05:50:45]

This is a DCHECK() because it is assumed keys are always generated and shouldn't
collide?

[pneubeck (no reviews), 2014/06/12 09:21:37]

Yes, updated the error string for clarification.

+      << "Public key is already registered";
+  platform_keys->Append(key_value.release());

[eroman, 2014/06/12 05:50:45]

What sort of bounds checks are done? Is it possible for extensions to exhaust
browser memory by generating lots of keys?

[pneubeck (no reviews), 2014/06/12 09:21:36]

No bounds checks so far. And yes an extension can exhaust memory that way. In
the design doc I mentioned a quota policy which we could consider if we see need
for it.

Again, the problem is less serious because only policy pre-installed extensions
can use this API. So we would protect against attackers of such extensions.
I think it's a nice to have (say M38) but not a release blocker.

+
+  service->state_store_->SetExtensionValue(extension_id,
+                                           kStateStorePlatformKeys,
+                                           platform_keys.PassAs<base::Value>());
+  callback.Run(true);
+}
+
+// static
+void PlatformKeysService::InvalidateKey(
+    const std::string& extension_id,
+    const std::string& public_key_spki_der,
+    const base::Callback<void(bool)>& callback,
+    base::WeakPtr<PlatformKeysService> service,
+    scoped_ptr<base::ListValue> platform_keys) {
+  if (!service) {
+    LOG(ERROR) << "PlatformKeysService is already destructed.";
+    callback.Run(false);
+    return;
+  }
+
+  scoped_ptr<base::StringValue> key_value(
+      GetPublicKeyValue(public_key_spki_der));
+
+  size_t index = 0;
+  if (!platform_keys->Remove(*key_value, &index)) {
+    // The key is not found, so it's not valid to use it for signing.
+    callback.Run(false);
+    return;
+  }
+
+  service->state_store_->SetExtensionValue(extension_id,
+                                           kStateStorePlatformKeys,
+                                           platform_keys.PassAs<base::Value>());
+  callback.Run(true);
+}
+
+// static
+void PlatformKeysService::GotPlatformKeysOfExtension(
+    const std::string& extension_id,
+    base::WeakPtr<PlatformKeysService> service,
+    const GetPlatformKeysCallback& callback,
+    scoped_ptr<base::Value> value) {
+  base::ListValue* keys = NULL;
+  if (value) {
+    if (!value->GetAsList(&keys)) {
+      LOG(ERROR) << "Found a value of wrong type.";
+      value.reset();
+    }
+  } else {
+    keys = new base::ListValue;
+    value.reset(keys);

[eroman, 2014/06/12 05:50:45]

why bother with this line, when the next one releases it?

[pneubeck (no reviews), 2014/06/12 09:21:37]

the intention was that both branches have the same postcondition (keys != NULL)
iff (value != NULL).
Also I don't like to have heap allocation without immediately binding it to a
scoped_ptr.

Still, changed it to make it overall a bit simpler.

Maybe a scoped_ptr method for safe downcasting would be reasonable; something
like:

template<class Base, class Derived>
scoped_ptr<Derived> PassAs(scoped_ptr<Base>& base, Derived* derived) {
  DCHECK_EQ(base.get(), derived);
  ignore_result(base.release());
  return make_scoped_ptr(derived);
}

+  }
+  ignore_result(value.release());
+  callback.Run(service, make_scoped_ptr(keys));
+}
+
+}  // namespace chromeos