WebCrypto: Add framework for AlgorithmIdentifier normalization.

Source/modules/crypto/NormalizeAlgorithm.cpp

Index: Source/modules/crypto/NormalizeAlgorithm.cpp
diff --git a/Source/modules/crypto/NormalizeAlgorithm.cpp b/Source/modules/crypto/NormalizeAlgorithm.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..56b1e2e392914323718219f50a8b3c6737a46f8b
--- /dev/null
+++ b/Source/modules/crypto/NormalizeAlgorithm.cpp
@@ -0,0 +1,239 @@
+/*
+ * 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
+ * copyright notice, this list of conditions and the following disclaimer
+ * in the documentation and/or other materials provided with the
+ * distribution.
+ *     * Neither the name of Google Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * 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/NormalizeAlgorithm.h"
+
+#include "bindings/v8/Dictionary.h"
+#include "core/dom/ExceptionCode.h"
+#include "public/platform/WebCryptoAlgorithm.h"
+#include "public/platform/WebCryptoAlgorithmParams.h"
+#include "wtf/ArrayBuffer.h"
+#include "wtf/ArrayBufferView.h"
+#include "wtf/HashMap.h"
+#include "wtf/Uint8Array.h"
+#include "wtf/Vector.h"
+#include "wtf/text/StringHash.h"
+
+namespace WebCore {
+
+namespace {
+
+struct AlgorithmNameMapping {
+    const char* const algorithmName;
+    WebKit::WebCryptoAlgorithmId algorithmId;
+};
+
+// Indicates that the algorithm doesn't support the specified operation.
+enum { UnsupportedOp = -1 };

[abarth-chromium, 2013/07/02 06:46:36]

Rather than use an enum, you can just use a global constant holding the value
-1.

[eroman, 2013/07/02 08:12:27]

Done.

+
+// Either UnsupportedOp, or a value from WebKit::WebCryptoAlgorithmParamsType
+typedef int AlgorithmParamsForOperation;
+
+struct OperationParamsMapping {
+    WebKit::WebCryptoAlgorithmId algorithmId;
+    AlgorithmOperation operation;
+    AlgorithmParamsForOperation params;
+};
+
+const AlgorithmNameMapping algorithmNameMappings[] = {
+    {"AES-CBC", WebKit::AesCbc},
+    {"SHA-1", WebKit::Sha1},
+    {"SHA-224", WebKit::Sha224},
+    {"SHA-256", WebKit::Sha256},
+    {"SHA-384", WebKit::Sha384},
+    {"SHA-512", WebKit::Sha512},
+};
+
+// What operations each algorithm supports, and what parameters it expects.
+const OperationParamsMapping operationParamsMappings[] = {
+    // AES-CBC (section 18.10.)
+    {WebKit::AesCbc, Decrypt, WebKit::AesCbcParams},
+    {WebKit::AesCbc, Encrypt, WebKit::AesCbcParams},
+    {WebKit::AesCbc, GenerateKey, WebKit::AesKeyGenParams},
+
+    // SHA-1 (section 18.16.)
+    {WebKit::Sha1, Digest, WebKit::NoParams},
+
+    // SHA-224 (section 18.16.)
+    {WebKit::Sha224, Digest, WebKit::NoParams},
+
+    // SHA-256 (section 18.16.)
+    {WebKit::Sha256, Digest, WebKit::NoParams},
+
+    // SHA-384 (section 18.16.)
+    {WebKit::Sha384, Digest, WebKit::NoParams},
+
+    // SHA-512 (section 18.16.)
+    {WebKit::Sha512, Digest, WebKit::NoParams},
+};
+
+// This structure describes an algorithm and its supported operations.
+struct AlgorithmInfo {
+    AlgorithmInfo()
+        : algorithmName(0)
+    {
+        for (size_t i = 0; i < WTF_ARRAY_LENGTH(paramsForOperation); ++i)
+            paramsForOperation[i] = UnsupportedOp;
+    }
+
+    WebKit::WebCryptoAlgorithmId algorithmId;
+    const char* algorithmName;
+    AlgorithmParamsForOperation paramsForOperation[NumAlgorithmOperations];
+};
+
+// AlgorithmRegistry enumerates each of the different algorithms and its
+// parameters. This describes the same information as the static tables above,
+// but in a more convenient runtime form.
+class AlgorithmRegistry {
+public:
+    static const AlgorithmInfo* lookupAlgorithmByName(const String& algorithmName);
+
+private:
+    AlgorithmRegistry();
+
+    // Algorithm name to ID.
+    typedef HashMap<String, WebKit::WebCryptoAlgorithmId, CaseFoldingHash> AlgorithmNameToIdMap;
+    AlgorithmNameToIdMap m_algorithmNameToId;
+
+    // Algorithm ID to information.
+    AlgorithmInfo m_algorithms[WebKit::NumAlgorithmId];
+};
+
+// static

[abarth-chromium, 2013/07/02 06:46:36]

Please skip these sorts of comments.  They're ignored by the compiler and
therefore become out of date.

[eroman, 2013/07/02 08:12:27]

Done.

+const AlgorithmInfo* AlgorithmRegistry::lookupAlgorithmByName(const String& algorithmName)
+{
+    // Singleton

[abarth-chromium, 2013/07/02 06:46:36]

Please omit comments that redundantly state what the code does.

[eroman, 2013/07/02 08:12:27]

Done.

+    static AlgorithmRegistry registry;

[abarth-chromium, 2013/07/02 06:46:36]

Please use DEFINE_STATIC_LOCAL.  That lets us avoid exit-time destructors.

[eroman, 2013/07/02 08:12:27]

Done.

+
+    // Do a case-insensitive lookup for algorithmName.

[abarth-chromium, 2013/07/02 06:46:36]

Please omit comments that redundantly state what the code does.

[eroman, 2013/07/02 08:12:27]

Done.

+    AlgorithmNameToIdMap::const_iterator it = registry.m_algorithmNameToId.find(algorithmName);
+    if (it == registry.m_algorithmNameToId.end())
+        return 0;
+    return &registry.m_algorithms[it->value];
+}
+
+AlgorithmRegistry::AlgorithmRegistry()
+{
+    // Initialize AlgorithmRegistry by populating it with the static mappings.

[abarth-chromium, 2013/07/02 06:46:36]

Please skip comments that say what the code does.  We like comments that explain
why the code does what it does, but comments that say what the code does are
redundant can get out of date quickly.

[eroman, 2013/07/02 08:12:27]

Done.

+    for (size_t i = 0; i < WTF_ARRAY_LENGTH(algorithmNameMappings); ++i) {
+        const AlgorithmNameMapping& mapping = algorithmNameMappings[i];
+        m_algorithmNameToId.add(mapping.algorithmName, mapping.algorithmId);
+        m_algorithms[mapping.algorithmId].algorithmName = mapping.algorithmName;
+    }
+
+    for (size_t i = 0; i < WTF_ARRAY_LENGTH(operationParamsMappings); ++i) {
+        const OperationParamsMapping& mapping = operationParamsMappings[i];
+        m_algorithms[mapping.algorithmId].paramsForOperation[mapping.operation] = mapping.params;
+    }
+}
+
+WebKit::WebCryptoAlgorithmParams* parseAesCbcParams(const Dictionary& raw)

[abarth-chromium, 2013/07/02 06:46:36]

WebKit::WebCryptoAlgorithmParams*   --> 
PassOwnPtr<WebKit::WebCryptoAlgorithmParams>

[eroman, 2013/07/02 08:12:27]

Done.

+{
+    RefPtr<ArrayBufferView> iv;
+    if (!raw.get("iv", iv) || !iv)
+        return 0;
+
+    if (iv->byteLength() != 16)
+        return 0;
+
+    return new WebKit::WebCryptoAesCbcParams(static_cast<unsigned char*>(iv->baseAddress()), iv->byteLength());

[abarth-chromium, 2013/07/02 06:46:36]

We try to avoid "naked new" calls.  Please wrap all calls to "new" either in
adoptPtr or adoptRef.  In this case, we should use adoptPtr.  At the API
boundary, we'll need to call leakPtr() to state that we're leaking this pointer
to the embedder.

[eroman, 2013/07/02 08:12:27]

Done.

+}
+
+WebKit::WebCryptoAlgorithmParams* parseAesKeyGenParams(const Dictionary& raw)
+{
+    int32_t length;
+    if (!raw.get("length", length))
+        return 0;
+    if (length < 0 || length > 0xFFFF)
+        return 0;
+    return new WebKit::WebCryptoAesKeyGenParams(length);

[abarth-chromium, 2013/07/02 06:46:36]

Naked new

[eroman, 2013/07/02 08:12:27]

Done.

+}
+
+WebKit::WebCryptoAlgorithmParams* parseAlgorithmParams(const Dictionary& raw, WebKit::WebCryptoAlgorithmParamsType type)
+{
+    switch (type) {
+    case WebKit::NoParams:
+        return 0;
+    case WebKit::AesCbcParams:
+        return parseAesCbcParams(raw);
+    case WebKit::AesKeyGenParams:
+        return parseAesKeyGenParams(raw);
+    }
+

[abarth-chromium, 2013/07/02 06:46:36]

ASSERT_NOT_REACHED() ?

[eroman, 2013/07/02 08:12:27]

Done.

+    return 0;
+}
+
+} // namespace
+
+// TODO(eroman): Throw the correct exception types!

[abarth-chromium, 2013/07/02 06:46:36]

"TODO(eroman)"  -->  "FIXME"

We use FIXME rather than TODO, and we skip the name of the author.  If someone
wants to know who authored the comment, they can look in the revision history.

[eroman, 2013/07/02 08:12:27]

Done.

+bool normalizeAlgorithm(const Dictionary& raw, AlgorithmOperation op, WebKit::WebCryptoAlgorithm& algorithm, ExceptionCode& ec)
+{
+    String algorithmName;
+    if (!raw.get("name", algorithmName)) {
+        // No name was specified by the AlgorithmIdentifier.

[abarth-chromium, 2013/07/02 06:46:36]

Please remove these sorts of redundant comments.

[eroman, 2013/07/02 08:12:27]

Done.

+        ec = NOT_SUPPORTED_ERR;
+        return false;
+    }
+
+    if (!algorithmName.containsOnlyASCII()) {
+        // The spec defines this case separately.

[abarth-chromium, 2013/07/02 06:46:36]

Please add a link to the part of the spec you're referring to.

[eroman, 2013/07/02 08:12:27]

Done.

+        ec = SYNTAX_ERR;
+        return false;
+    }
+
+    const AlgorithmInfo* info = AlgorithmRegistry::lookupAlgorithmByName(algorithmName);
+    if (!info) {
+        // No algorithm by that name exists.

[abarth-chromium, 2013/07/02 06:46:36]

Please omit this comment as well.

[eroman, 2013/07/02 08:12:27]

Done.

+        ec = NOT_SUPPORTED_ERR;
+        return false;
+    }
+
+    if (info->paramsForOperation[op] == UnsupportedOp) {
+        // The algorithm does not support the requested operation.

[abarth-chromium, 2013/07/02 06:46:36]

Please omit this redundant comment.

[eroman, 2013/07/02 08:12:27]

Done.

+        ec = NOT_SUPPORTED_ERR;
+        return false;
+    }
+
+    // Parse the algorithm-specific parameters.

[abarth-chromium, 2013/07/02 06:46:36]

Please omit this redundant comment.

[eroman, 2013/07/02 08:12:27]

Done.

+    WebKit::WebCryptoAlgorithmParamsType paramsType = static_cast<WebKit::WebCryptoAlgorithmParamsType>(info->paramsForOperation[op]);
+    OwnPtr<WebKit::WebCryptoAlgorithmParams> params = adoptPtr(parseAlgorithmParams(raw, paramsType));

[abarth-chromium, 2013/07/02 06:46:36]

I see, we're not actually leaking this pointer.  The proper place to call
adoptPtr is immediately after calling "new".  That way we can check that there
aren't any leaks by looking at each line individually instead of having to
understand the whole file at once.

[eroman, 2013/07/02 08:12:27]

Done.

+
+    if (!params && paramsType != WebKit::NoParams) {
+        // Failed parsing the parameters.

[abarth-chromium, 2013/07/02 06:46:36]

Please omit this redundant comment.

[eroman, 2013/07/02 08:12:27]

Done.

+        ec = NOT_SUPPORTED_ERR;
+        return false;
+    }
+
+    algorithm = WebKit::WebCryptoAlgorithm(info->algorithmId, info->algorithmName, params.release());
+    return true;
+}
+
+} // namespace WebCore