Implement HKDF for webcrypto

content/child/webcrypto/openssl/hkdf_openssl.cc

Index: content/child/webcrypto/openssl/hkdf_openssl.cc
diff --git a/content/child/webcrypto/openssl/hkdf_openssl.cc b/content/child/webcrypto/openssl/hkdf_openssl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ea87825db16f12ae4ea15b979f2090dd548fc1af
--- /dev/null
+++ b/content/child/webcrypto/openssl/hkdf_openssl.cc
@@ -0,0 +1,122 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.

[eroman, 2015/01/07 00:23:42]

2015 perhaps?

[nharper, 2015/01/08 01:31:24]

Done. Is there a doc that describes the appropriate style for copyright/license
headers in chromium?

[eroman, 2015/01/08 02:39:42]

Search for File Headers in:
http://www.chromium.org/developers/coding-style

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <openssl/err.h>
+#include <openssl/hkdf.h>
+
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "content/child/webcrypto/algorithm_implementation.h"
+#include "content/child/webcrypto/crypto_data.h"
+#include "content/child/webcrypto/openssl/key_openssl.h"
+#include "content/child/webcrypto/openssl/util_openssl.h"
+#include "content/child/webcrypto/status.h"
+#include "content/child/webcrypto/webcrypto_util.h"
+#include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
+#include "third_party/WebKit/public/platform/WebCryptoKeyAlgorithm.h"
+
+namespace content {
+
+namespace webcrypto {
+
+namespace {
+
+const blink::WebCryptoKeyUsageMask kValidUsages =
+    blink::WebCryptoKeyUsageDeriveKey | blink::WebCryptoKeyUsageDeriveBits;
+
+class HkdfImplementation : public AlgorithmImplementation {
+ public:
+  HkdfImplementation() {}
+
+  Status VerifyKeyUsagesBeforeImportKey(
+      blink::WebCryptoKeyFormat format,
+      blink::WebCryptoKeyUsageMask usages) const override {
+    if (format != blink::WebCryptoKeyFormatRaw)
+      return Status::ErrorUnsupportedImportKeyFormat();
+    return CheckKeyCreationUsages(kValidUsages, usages, false);
+  }
+
+  Status ImportKeyRaw(const CryptoData& key_data,
+                      const blink::WebCryptoAlgorithm& algorithm,
+                      bool extractable,
+                      blink::WebCryptoKeyUsageMask usages,
+                      blink::WebCryptoKey* key) const override {
+    *key = blink::WebCryptoKey::create(

[eroman, 2015/01/07 00:23:42]

Simplify to:

return CreateWebCryptoSecretKey(...);

[nharper, 2015/01/08 01:31:24]

Done.

+        new SymKeyOpenSsl(key_data), blink::WebCryptoKeyTypeSecret, extractable,
+        blink::WebCryptoKeyAlgorithm::createKdf(
+            blink::WebCryptoAlgorithmIdHkdf),
+        usages);
+    return Status::Success();
+  }
+
+  Status DeriveBits(const blink::WebCryptoAlgorithm& algorithm,
+                    const blink::WebCryptoKey& base_key,
+                    bool has_optional_length_bits,
+                    unsigned int optional_length_bits,
+                    std::vector<uint8_t>* derived_bytes) const override {
+    // TODO(nharper): The spec might change so that an OperationError should be

[eroman, 2015/01/07 00:23:43]

Link to the spec bug if you keep this comment.

That said:
  (1) This TODO would be better if it were in
Status::ErrorHkdfDeriveBitsLengthNotSpecified(), since that is where the code
change will be.

  (2) A bug on crbug.com would alternately be sufficient

[nharper, 2015/01/08 01:31:24]

I've moved the comment to the appropriate part of status.cc and added a link to
the spec bug.

+    // thrown here instead of a TypeError. If it changes, this line will need to
+    // change as well.
+    if (!has_optional_length_bits)
+      return Status::ErrorHkdfDeriveBitsLengthNotSpecified();
+
+    // "If the hash member of normalizedAlgorithm does not describe a recognized

[eroman, 2015/01/07 00:23:42]

It is unclear where this quote comes from (the spec). Given the changing nature
of the spec, I would omit this comment altogether (should be able to look this
up in the spec if needing to validate behaviors).

[nharper, 2015/01/08 01:31:24]

Done.

+    // algorithm that supports the digest operation, then throw a
+    // NotSupportedError"
+    const EVP_MD* digest_algorithm =
+        GetDigest(algorithm.hkdfParams()->hash().id());
+    if (!digest_algorithm)
+      return Status::ErrorUnsupported();
+
+    // Size output to fit length
+    unsigned int derived_bytes_len = NumBitsToBytes(optional_length_bits);
+    derived_bytes->resize(derived_bytes_len);
+
+    // Algorithm dispatch checks that the algorithm in |base_key| matches
+    // |algorithm|.
+    const std::vector<uint8_t>* raw_key =

[eroman, 2015/01/07 00:23:42]

Please use const reference rather than pointers throughout:

const std::vector<uint8_t>& raw_key = ...;

[nharper, 2015/01/08 01:31:24]

Done.

+        &SymKeyOpenSsl::Cast(base_key)->raw_key_data();
+    const blink::WebVector<unsigned char>* salt =

[eroman, 2015/01/07 00:23:42]

Rather than making these aliases, I think it would be cleaner to have this near
the start:

  const blink::.... params = algorithm.hkdfParams();


And then use expressions like:

 params->hash()
 params->salt()

etc..

[nharper, 2015/01/08 01:31:24]

Done.

+        &algorithm.hkdfParams()->salt();
+    const blink::WebVector<unsigned char>* info =
+        &algorithm.hkdfParams()->info();
+    if (!HKDF(derived_bytes->data(), derived_bytes_len, digest_algorithm,

[eroman, 2015/01/07 00:23:43]

I don't believe we can use std::vector::data(), as that is new to c++11. I
recall having trouble compiling such code on Windows.

[nharper, 2015/01/08 01:31:24]

I've removed use of std::vector::data(), although the code is less clean now.

+              raw_key->data(), raw_key->size(), salt->data(), salt->size(),
+              info->data(), info->size())) {
+      uint32_t error = ERR_get_error();
+      if (ERR_GET_LIB(error) == ERR_LIB_HKDF &&
+          ERR_GET_REASON(error) == HKDF_R_OUTPUT_TOO_LARGE)
+        return Status::ErrorHkdfLengthTooLong();
+      return Status::OperationError();
+    }

[eroman, 2015/01/07 01:00:59]

Also, the HKDF spec is woefully underwritten. However there is an additional
concern here that the input is in bits, but the output is in bytes.

For ECDH which similar has this style, I opted to set the unused bits to zero:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=27402

Also need tests for this case.

[nharper, 2015/01/08 01:31:24]

I'm truncating the bits now. I'll add a test for bit length % 8 != 0 as part of
https://codereview.chromium.org/822083003/.

+    return Status::Success();
+  }
+
+  Status SerializeKeyForClone(
+      const blink::WebCryptoKey& key,
+      blink::WebVector<uint8_t>* key_data) const override {
+    key_data->assign(SymKeyOpenSsl::Cast(key)->serialized_key_data());
+    return Status::Success();
+  }
+
+  Status DeserializeKeyForClone(const blink::WebCryptoKeyAlgorithm& algorithm,
+                                blink::WebCryptoKeyType type,
+                                bool extractable,
+                                blink::WebCryptoKeyUsageMask usages,
+                                const CryptoData& key_data,
+                                blink::WebCryptoKey* key) const override {
+    return CreateWebCryptoSecretKey(key_data, algorithm, extractable, usages,
+                                    key);
+  }
+};
+
+}  // namespace
+
+AlgorithmImplementation* CreatePlatformHkdfImplementation() {
+  return new HkdfImplementation;
+}
+
+}  // namespace webcrypto
+
+}  // namespace content