WebCrypto: Add interfaces for importKey().

Source/modules/crypto/SubtleCrypto.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 13 matching lines @@
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "config.h"
 #include "modules/crypto/SubtleCrypto.h"
 
+#include "V8Key.h" // NOTE: This must appear before ScriptPromiseResolver to define toV8()
+#include "bindings/v8/ScriptPromiseResolver.h"
 #include "core/dom/ExceptionCode.h"
 #include "modules/crypto/CryptoOperation.h"
+#include "modules/crypto/Key.h"
 #include "modules/crypto/NormalizeAlgorithm.h"
 #include "public/platform/Platform.h"
-#include "public/platform/WebArrayBuffer.h" // FIXME: temporary
 #include "public/platform/WebCrypto.h"
-#include "wtf/ArrayBuffer.h"
 #include "wtf/ArrayBufferView.h"
-#include "wtf/SHA1.h" // FIXME: temporary
-
 
 namespace WebCore {
 
 namespace {
 
-// FIXME: The following are temporary implementations of what *should* go on the
-//        embedder's side. Since SHA1 is easily implemented, this serves as
-//        a useful proof of concept to get layout tests up and running and
-//        returning correct results, until the embedder's side is implemented.
-//------------------------------------------------------------------------------
-class DummyOperation : public WebKit::WebCryptoOperation {
-public:
-    explicit DummyOperation(WebKit::WebCryptoOperationResult* result) : m_result(result) { }
-
-    virtual void process(const unsigned char* bytes, size_t size) OVERRIDE
-    {
-        m_result->completeWithError();
-        delete this;
+// FIXME: Temporary
+PassRefPtr<CryptoOperation> doDummyOperation(const Dictionary& rawAlgorithm, AlgorithmOperation operationType, ExceptionCode& ec)

[abarth-chromium, 2013/07/23 06:22:07]

Like "get", "do" is a weak verb.  Perhaps "dummyOperation"?

+{
+    WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
+    if (!platformCrypto) {
+        ec = NotSupportedError;
+        return 0;
     }
 
-    virtual void abort() OVERRIDE
-    {
-        delete this;
-    }
-
-    virtual void finish() OVERRIDE
-    {
-        m_result->completeWithError();
-        delete this;
-    }
-
-protected:
-    WebKit::WebCryptoOperationResult* m_result;
-};
-
-class MockSha1Operation : public DummyOperation {
-public:
-    explicit MockSha1Operation(WebKit::WebCryptoOperationResult* result) : DummyOperation(result) { }
-
-    virtual void process(const unsigned char* bytes, size_t size) OVERRIDE
-    {
-        m_sha1.addBytes(bytes, size);
-    }
-
-    virtual void finish() OVERRIDE
-    {
-        Vector<uint8_t, 20> hash;
-        m_sha1.computeHash(hash);
-
-        WebKit::WebArrayBuffer buffer = WebKit::WebArrayBuffer::create(hash.size(), 1);
-        memcpy(buffer.data(), hash.data(), hash.size());
-
-        m_result->completeWithArrayBuffer(buffer);
-        delete this;
-    }
-
-private:
-    SHA1 m_sha1;
-};
-
-class MockPlatformCrypto : public WebKit::WebCrypto {
-public:
-    virtual void digest(const WebKit::WebCryptoAlgorithm& algorithm, WebKit::WebCryptoOperationResult* result) OVERRIDE
-    {
-        if (algorithm.id() == WebKit::WebCryptoAlgorithmIdSha1) {
-            result->initializationSucceded(new MockSha1Operation(result));
-        } else {
-            // Don't fail synchronously, since existing layout tests rely on
-            // digest for testing algorithm normalization.
-            result->initializationSucceded(new DummyOperation(result));
-        }
-    }
-};
-
-WebKit::WebCrypto* mockPlatformCrypto()
-{
-    DEFINE_STATIC_LOCAL(MockPlatformCrypto, crypto, ());
-    return &crypto;
-}
-
-PassRefPtr<CryptoOperation> doDummyOperation(const Dictionary& rawAlgorithm, AlgorithmOperation operationType, ExceptionCode& ec)
-{
     WebKit::WebCryptoAlgorithm algorithm;
     if (!normalizeAlgorithm(rawAlgorithm, operationType, algorithm, ec))
         return 0;
 
     RefPtr<CryptoOperation> op = CryptoOperation::create(algorithm, &ec);
-    op->initializationSucceded(new DummyOperation(op.get()));
+    platformCrypto->digest(algorithm, op.get());
     return op.release();
 }
-//------------------------------------------------------------------------------
+
+class KeyOperation : public WebKit::WebCryptoKeyOperationResult {
+public:
+    KeyOperation(ScriptPromiseResolver* resolver, ExceptionCode* ec)
+        : m_state(Initializing)
+        , m_impl(0)
+        , m_promiseResolver(resolver)
+        , m_exceptionCode(ec)
+    {
+    }
+
+    ~KeyOperation();
+
+    // Implementation of WebKit::WebCryptoKeyOperationResult.
+    virtual void initializationFailed() OVERRIDE;
+    virtual void initializationSucceeded(WebKit::WebCryptoKeyOperation*) OVERRIDE;
+    virtual void completeWithError() OVERRIDE;
+    virtual void completeWithKey(const WebKit::WebCryptoKey&) OVERRIDE;
+
+private:
+    enum State {
+        Initializing,
+        InProgress,
+        Done,
+    };
+
+    State m_state;
+    WebKit::WebCryptoKeyOperation* m_impl;
+    ExceptionCode* m_exceptionCode;

[abarth-chromium, 2013/07/23 06:22:07]

I don't understand how KeyOperation can hold a pointer to m_exceptionCode.  The
ExceptionCode is a stack-allocated object, which means it dies after the stack
unwinds.  The KeyOperation appears to live in the heap and complete its work
asynchronously.  That seems to imply that KeyOperation outlives m_exceptionCode
and therefore cannot keep a raw pointer to m_exceptionCode.

[eroman, 2013/07/23 06:53:41]

It is a little bit tricky, but the m_exceptionCode pointer is cleared before it
goes out of scope.

Upon calling into the Platform key operation, it either calls:
   initializationSucceeded(), initializationFailed(), completeWithKey() or
completeWithError(). These all drop the pointer.

I could instead have them buffer the exception code, and then call another
function to explicitly set it.

[abarth-chromium, 2013/07/23 07:06:14]

That's too tricky.  Is there a simpler design?

+    RefPtr<ScriptPromiseResolver> m_promiseResolver;
+};
+
+KeyOperation::~KeyOperation()
+{
+    // Abort any inprogress operation.
+    switch (m_state) {
+    case Initializing:
+        ASSERT_NOT_REACHED();
+        break;
+    case InProgress:
+        // This will cause m_impl to be deleted.
+        m_state = Done;
+        m_impl->abort();
+        m_impl = 0;
+    case Done:
+        ASSERT(!m_impl);
+        break;
+    }
+}
+
+void KeyOperation::initializationFailed()
+{
+    ASSERT(m_state == Initializing);
+
+    *m_exceptionCode = NotSupportedError;
+
+    m_exceptionCode = 0;
+    m_state = Done;
+    delete this;

[abarth-chromium, 2013/07/23 06:22:07]

This line is suspicious and indicates a bad memory management pattern...  Is
there a better way to structure the ownership relationships between these
objects?

+}
+
+void KeyOperation::initializationSucceeded(WebKit::WebCryptoKeyOperation* operationImpl)
+{
+    ASSERT(m_state == Initializing);
+    ASSERT(operationImpl);
+    ASSERT(!m_impl);
+
+    m_exceptionCode = 0;
+    m_impl = operationImpl;
+    m_state = InProgress;
+}
+
+void KeyOperation::completeWithError()
+{
+    ASSERT(m_state == Initializing || m_state == InProgress);
+
+    m_exceptionCode = 0;
+    m_impl = 0;
+    m_state = Done;
+
+    m_promiseResolver->reject(ScriptValue::createNull());
+    delete this;
+}
+
+void KeyOperation::completeWithKey(const WebKit::WebCryptoKey& key)
+{
+    ASSERT(m_state == Initializing || m_state == InProgress);
+
+    m_exceptionCode = 0;
+    m_impl = 0;
+    m_state = Done;
+
+    m_promiseResolver->fulfill(Key::create(key));
+    delete this;
+}
 
 } // namespace
 
 SubtleCrypto::SubtleCrypto()
 {
     ScriptWrappable::init(this);
 }
 
 PassRefPtr<CryptoOperation> SubtleCrypto::encrypt(const Dictionary& rawAlgorithm, ExceptionCode& ec)
 {
@@ skipping 10 matching lines @@
     return doDummyOperation(rawAlgorithm, Sign, ec);
 }
 
 PassRefPtr<CryptoOperation> SubtleCrypto::verifySignature(const Dictionary& rawAlgorithm, ExceptionCode& ec)
 {
     return doDummyOperation(rawAlgorithm, Verify, ec);
 }
 
 PassRefPtr<CryptoOperation> SubtleCrypto::digest(const Dictionary& rawAlgorithm, ExceptionCode& ec)
 {
-    WebKit::WebCrypto* platformCrypto = mockPlatformCrypto();
+    WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
     if (!platformCrypto) {
         ec = NotSupportedError;
         return 0;
     }
 
     WebKit::WebCryptoAlgorithm algorithm;
     if (!normalizeAlgorithm(rawAlgorithm, Digest, algorithm, ec))
         return 0;
 
     RefPtr<CryptoOperation> op = CryptoOperation::create(algorithm, &ec);
     platformCrypto->digest(algorithm, op.get());
     return op.release();
 }
 
+ScriptObject SubtleCrypto::importKey(const String& rawFormat, ArrayBufferView* keyData, const Dictionary& rawAlgorithm, bool extractable, const Vector<String>& rawKeyUsages, ExceptionCode& ec)
+{
+    WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
+    if (!platformCrypto) {
+        ec = NotSupportedError;
+        return ScriptObject();
+    }
+
+    WebKit::WebCryptoKeyUsageMask keyUsages;
+    if (!Key::parseUsageMask(rawKeyUsages, keyUsages)) {
+        ec = TypeError;
+        return ScriptObject();
+    }
+
+    WebKit::WebCryptoKeyFormat format;
+    if (!Key::parseFormat(rawFormat, format)) {
+        ec = TypeError;
+        return ScriptObject();
+    }
+
+    WebKit::WebCryptoAlgorithm algorithm;
+    if (!normalizeAlgorithmForImportKey(rawAlgorithm, algorithm, ec))
+        return ScriptObject();
+
+    const unsigned char* keyDataBytes = static_cast<unsigned char*>(keyData->baseAddress());
+
+    RefPtr<ScriptPromiseResolver> promiseResolver = ScriptPromiseResolver::create();
+
+    // The |op| object is deleted upon completion of the underlying operation
+    // (i.e. when platformCrypto->importKey() notifies completion).
+    //
+    // FIXME: KeyOperation is never aborted. It should probably be aborted when
+    // the SubtleCrypto object that started it gets deleted. The concern being
+    // if the operation eventually does complete, the ScriptPromiseResolver
+    // might no longer be valid because the context it belonged to got torn
+    // down.
+    KeyOperation* op = new KeyOperation(promiseResolver.get(), &ec);

[abarth-chromium, 2013/07/23 06:22:07]

This is a "naked new", which is where the bad memory pattern originates.  All
calls to "new" should be immediately wrapped in either adoptRef (for ref-counted
objects) or adoptPtr (for non-ref-counted objects).  We prefer to avoid these
sorts of complicated lifetime management patterns because they're fragile and
lead to memory corruption or leaks.  You might be smart enough to author this
code correctly, but, as a group, we're not smart enough to maintain them.

[eroman, 2013/07/23 06:53:41]

I'll think about this some more and see what else I can come up with.
I modeled this similarly to CryptoOperation.

I wanted to have a simple interface for the embedder which can allow them to
notify sync/async completion/error in a consistent manner, since there will be
many such operations implemented on top of this.

Because both the embedder and blink hold pointers to each other reference
counting would cause a cycle and I ended up with explicit deletions

[abarth-chromium, 2013/07/23 07:06:14]

We might want to change CryptoOperation to follow whatever pattern we come up
with here.
Rather than returning a raw pointer to the embedder, perhaps we should return a
value type that acts like a smart pointer.  We can implement this with RefPtr
internally to allow the value type to be copied on the embedder's side, but the
expectation is that the embedder will eventually call the success or failure
function on the API object, which will clear any references the WebCore side has
to any other objects (making it a leaf in the object graph and breaking any
cycles).

That approach doesn't add any extra complexity because we already need a similar
guarantee to make sure we resolve the Promise (and thereby breaking the strong
reference from the ScriptPromiseResolver into the JavaScript heap).
Hopefully the approach above resolves any reference cycles.

+    platformCrypto->importKey(format, keyDataBytes, keyData->byteLength(), algorithm, extractable, keyUsages, op);
+    return promiseResolver->promise();
+}
+
 } // namespace WebCore