Extension API enterprise.platformKeys.

chrome/renderer/extensions/enterprise_certificates_natives.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/renderer/extensions/enterprise_certificates_natives.h"
+
+#include <string>
+
+#include "third_party/WebKit/public/platform/Platform.h"
+#include "third_party/WebKit/public/platform/WebCrypto.h"
+#include "third_party/WebKit/public/platform/WebCryptoKey.h"
+#include "third_party/WebKit/public/web/WebScriptBindings.h"
+#include "v8/include/v8.h"
+
+namespace extensions {
+
+EnterpriseCertificatesNatives::EnterpriseCertificatesNatives(
+    ChromeV8Context* context)
+    : ObjectBackedNativeHandler(context) {
+  RouteFunction("GetPrivateKey",
+                base::Bind(&EnterpriseCertificatesNatives::GetPrivateKey,
+                           base::Unretained(this)));

[eroman, 2014/04/10 18:45:46]

I haven't read through the rest of this, change, but always very cautions when I
see base::Unretained(). Please verify that class remains alive at least as long
as the callback (i.e. that cancellation/shutdown paths are reasonable).

[pneubeck (no reviews), 2014/04/11 14:13:59]

This seems to be the way how ObjectBackedNativeHandlers work.

+}
+
+void EnterpriseCertificatesNatives::GetPrivateKey(
+    const v8::FunctionCallbackInfo<v8::Value>& call_info) {
+  const blink::WebCryptoKey* key =
+      blink::WebScriptBindings::toWebCryptoKey(call_info[0].As<v8::Object>());

[eroman, 2014/04/10 18:45:46]

This function doesn't exist, so presumably depends on a dependent blink
changelist?

Can this return a WebCryptoKey rather than pointer?

[pneubeck (no reviews), 2014/04/11 14:13:59]

Added a link to the blink change to the commit message.
Yes, I didn't see that WebCryptoKey itself is a refptr. Done.

+  if (!key)

[eroman, 2014/04/10 18:45:46]

IMPORTANT: You should also check the key type. Since this function expects
private keys, make sure that:

key->type() == blink::WebCryptoKeyTypePrivate

[pneubeck (no reviews), 2014/04/11 14:13:59]

Done.

+    return;
+
+  blink::WebVector<unsigned char> keyData;

[eroman, 2014/04/10 18:45:46]

style: keyData --> key_data

[pneubeck (no reviews), 2014/04/11 14:13:59]

Done.

+  blink::Platform::current()->crypto()->serializeKeyForClone(*key, keyData);

[eroman, 2014/04/10 18:45:46]

IMPORTANT: serializeKeyForClone() returns a boolean for success, which should be
checked here.

It isn't likely to be an issue for private keys since on failure keyData will
likely be empty (although that is not guaranteed by the API). But for other key
types this would be ambiguous -- a symmetric key with empty data is a valid key
as well.

[pneubeck (no reviews), 2014/04/11 14:13:59]

Done.

+
+  v8::Handle<v8::Array> result =
+      v8::Array::New(call_info.GetIsolate(), keyData.size());

[eroman, 2014/04/10 18:45:46]

IMPORTANT: Please explain this. Why is the key being being copied into a
javascript object?

The key data should not be exposed to javascript as it may have been
unextractable.

My understanding is it was going to be serialized here as a means to pass it
over to the browser process.

I haven't read the rest of the change in detail, but making sure the key data is
not exposed to javascript APIs is very important.

[pneubeck (no reviews), 2014/04/11 14:13:59]

kalman@ mentioned that the javascript side of the custom binding is in principal
as safe as C++ except that one must take more care during the implementation.

To be on the safe side, I changed this to C++ only nonetheless.

+  for (size_t i = 0; i < keyData.size(); ++i) {
+    result->Set(
+        i, v8::Integer::NewFromUnsigned(call_info.GetIsolate(), keyData[i]));
+  }
+
+  call_info.GetReturnValue().Set(result);
+}
+
+}  // namespace extensions