[webcrypto] Add JWK import for HMAC and AES-CBC key.

content/renderer/webcrypto/webcrypto_impl.cc

Index: content/renderer/webcrypto/webcrypto_impl.cc
diff --git a/content/renderer/webcrypto/webcrypto_impl.cc b/content/renderer/webcrypto/webcrypto_impl.cc
index f794c59ed8461beb0cbf1634d446e386980e15e5..2335401ec35823a845c9a78c6cfc343e8225151b 100644
--- a/content/renderer/webcrypto/webcrypto_impl.cc
+++ b/content/renderer/webcrypto/webcrypto_impl.cc
@@ -4,15 +4,274 @@
 
 #include "content/renderer/webcrypto/webcrypto_impl.h"
 
+#include <algorithm>
+#include <map>
+#include "base/base64.h"
+#include "base/json/json_reader.h"
+#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/strings/string_piece.h"
+#include "base/values.h"
 #include "third_party/WebKit/public/platform/WebArrayBuffer.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
+#include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
 #include "third_party/WebKit/public/platform/WebCryptoKey.h"
 
 namespace content {
 
+namespace {
+
+// FIXME! Need WebKit::WebCryptoAlgorithmIdNone

[eroman, 2013/10/22 21:49:38]

There are other ways of accomplishing this that I think will be easier to
maintain.

Adding the possibility for an AlgorithmIdNone in the blink API means that every
location which uses algorithm must check for it, when in practice the Algorithm
should always be valid.

I believe the situation you are running into here is that in the Blink API for
ImportKey() I made algorithm a required parameter, whereas in the WebCrypto spec
it is optional. I think the best course of action is to either turn that into an
optional parameter (say by making it a const pointer which has the possibility
of being NULL), or by adding a separate virtual method for importing JWK keys,
in which the Algorithm is optional (and leaving ImportKey() as-is, with a
non-optional Algorithm).

[Ryan Sleevi, 2013/10/24 01:45:58]

There are other formats that may have embedded algorithms (eg: SPKI, PKCS#8), so
let's make it a true optional parameter, as spec'd.

[eroman, 2013/10/24 02:35:04]

I will make the necessary changes to Blink side.

[padolph, 2013/10/28 20:35:07]

This is addressed by your recent Blink checkin. Thanks.

+const WebKit::WebCryptoAlgorithmId WebCryptoAlgorithmIdNone =
+    (WebKit::WebCryptoAlgorithmId)99;
+
+// TODO(padolph) Similar Create*Algorithm() methods are in

[eroman, 2013/10/22 21:49:38]

What do you think about a renderer/webcrypto/webcrypto_util.{h,cc} file? Could
contain these functions, as well as ShrinkBuffer().

OK to leave them in the one file for this changelist.

[padolph, 2013/10/28 20:35:07]

Done.

Should the stuff in webcrypto_util.[h|cc] be in a new 'util' sub-namespace? For
now I just kept them in 'content'. Please advise.

+// webcrypto_impl_unittest.cc. Find a common place to put these.
+
+WebKit::WebCryptoAlgorithm CreateAlgorithm(WebKit::WebCryptoAlgorithmId id) {
+  return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate(id, NULL);
+}
+
+WebKit::WebCryptoAlgorithm CreateAlgorithmWithInnerHash(
+    WebKit::WebCryptoAlgorithmId algorithm_id,
+    unsigned short hash_key_length) {
+  WebKit::WebCryptoAlgorithmId hashId;

[eroman, 2013/10/22 21:49:38]

use hacker_style_variable_naming for chromium code.

[padolph, 2013/10/28 20:35:07]

Done.

+  switch (hash_key_length) {
+    case 160:
+      hashId = WebKit::WebCryptoAlgorithmIdSha1;
+      break;
+    case 224:
+      hashId = WebKit::WebCryptoAlgorithmIdSha224;
+      break;
+    case 256:
+      hashId = WebKit::WebCryptoAlgorithmIdSha256;
+      break;
+    case 384:
+      hashId = WebKit::WebCryptoAlgorithmIdSha384;
+      break;
+    case 512:
+      hashId = WebKit::WebCryptoAlgorithmIdSha384;
+      break;
+  }
+  return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate(

[eroman, 2013/10/22 21:49:38]

Please add a default case to the switch above (perhaps a CHECK or NOTREACHED()),
to guard against an uninitialized value of hashId being used.

[padolph, 2013/10/28 20:35:07]

Done.

+      algorithm_id,
+      new WebKit::WebCryptoHmacParams(CreateAlgorithm(hashId)));
+}
+
+WebKit::WebCryptoAlgorithm CreateHmacAlgorithm(unsigned short hash_key_length) {
+  return CreateAlgorithmWithInnerHash(
+      WebKit::WebCryptoAlgorithmIdHmac,
+      hash_key_length);
+}
+
+WebKit::WebCryptoAlgorithm CreateRsaSsaAlgorithm(
+    unsigned short hash_key_length) {
+  return CreateAlgorithmWithInnerHash(
+      WebKit::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5,
+      hash_key_length);
+}
+
+WebKit::WebCryptoAlgorithm CreateRsaOaepAlgorithm(
+    unsigned short hash_key_length) {
+  return CreateAlgorithmWithInnerHash(
+      WebKit::WebCryptoAlgorithmIdRsaOaep,
+      hash_key_length);
+}
+
+WebKit::WebCryptoAlgorithm CreateAesAlgorithm(
+    WebKit::WebCryptoAlgorithmId aes_alg_id,
+    unsigned short length) {
+  return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate(
+      aes_alg_id,
+      new WebKit::WebCryptoAesKeyGenParams(length));
+}
+
+WebKit::WebCryptoAlgorithm CreateAesCbcAlgorithm(unsigned short length) {
+  return CreateAesAlgorithm(WebKit::WebCryptoAlgorithmIdAesCbc, length);
+}
+
+WebKit::WebCryptoAlgorithm CreateAesGcmAlgorithm(unsigned short length) {
+  return CreateAesAlgorithm(WebKit::WebCryptoAlgorithmIdAesGcm, length);
+}
+
+bool Base64DecodeUrlSafe(const std::string& input, std::string* output) {

[eroman, 2013/10/22 21:49:38]

Please provide a comment pointing to the definition of "Base64url Encoding" in
the jose spec. Or explaining that this is for decoding Base64 URLs where the
padding has been omitted, and certain characters have been replaced.

This is the paragraph I found in jose referring to it:


  Base64url Encoding  Base64 encoding using the URL- and filename-safe
      character set defined in Section 5 of RFC 4648 [RFC4648], with all
      trailing '=' characters omitted (as permitted by Section 3.2).
      (See Appendix C of [JWS] for notes on implementing base64url
      encoding without padding.)

[padolph, 2013/10/28 20:35:07]

Will do. 'base64url' encoding is relatively common, not only for JWK. Indeed
RFC4648 section 5 is a good reference and lists the exact alphabet (regarding
your question below). So I'll put in a link for that.

+  std::string base64EncodedText(input);
+  std::replace(base64EncodedText.begin(), base64EncodedText.end(), '-', '+');

[eroman, 2013/10/22 21:49:38]

Are these transformations enumerated int he JOSE spec?

[padolph, 2013/10/28 20:35:07]

Yes, in RFC4648 Section 5.

+  std::replace(base64EncodedText.begin(), base64EncodedText.end(), '_', '/');
+  base64EncodedText.append((4 - base64EncodedText.size() % 4) % 4, '=');
+  return base::Base64Decode(base64EncodedText, output);
+}
+
+// Identifiers for all JWK "alg" (algorithm) values handled by this code. The

[eroman, 2013/10/22 21:49:38]

There is enough JWK stuff, that I suggest moving this code into its own file.

Perhaps renderer/webcrypto/jwk.{h,cc}

[padolph, 2013/10/28 20:35:07]

Most of this code went away with the function table refactoring. No need for a
separate file.

+// "enum trick" is used to force int type, so a user type is not required in

[eroman, 2013/10/22 21:49:38]

Not sure I know what the "enum trick" refers to. You mean treating these just as
integer constants?

In which case, I would rather give this its own type.

[padolph, 2013/10/28 20:35:07]

This code went away with the function table refactoring.

+// web_crypto_impl.h
+enum {
+  kJwkAlgorithmHs256,
+  kJwkAlgorithmHs384,
+  kJwkAlgorithmHs512,
+  kJwkAlgorithmRs256,
+  kJwkAlgorithmRs384,
+  kJwkAlgorithmRs512,
+  kJwkAlgorithmRsa1_5,
+  kJwkAlgorithmRsaOaep,
+  kJwkAlgorithmA128Kw,
+  kJwkAlgorithmA256Kw,
+  kJwkAlgorithmA128Gcm,
+  kJwkAlgorithmA256Gcm,
+  kJwkAlgorithmA128Cbc,
+  kJwkAlgorithmA256Cbc,
+  kJwkAlgorithmA384Cbc,
+  kJwkAlgorithmA512Cbc
+};
+
+WebKit::WebCryptoAlgorithmId JwkAlgIdToWebCryptoAlgId(int jwk_algorithm_id) {

[eroman, 2013/10/22 21:49:38]

This function appears to be unused

[padolph, 2013/10/28 20:35:07]

Done.

+  switch (jwk_algorithm_id) {
+    case kJwkAlgorithmHs256:
+    case kJwkAlgorithmHs384:
+    case kJwkAlgorithmHs512:
+      return WebKit::WebCryptoAlgorithmIdHmac;
+    case kJwkAlgorithmRs256:
+    case kJwkAlgorithmRs384:
+    case kJwkAlgorithmRs512:
+      return WebKit::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5;
+    case kJwkAlgorithmRsa1_5:
+      return WebKit::WebCryptoAlgorithmIdRsaEsPkcs1v1_5;
+    case kJwkAlgorithmRsaOaep:
+      return WebKit::WebCryptoAlgorithmIdRsaOaep;
+    case kJwkAlgorithmA128Kw:
+    case kJwkAlgorithmA256Kw:
+      // TODO(padolph) Support AES keywrap algorithm, required for JWK but not
+      // present in the Web Crypto spec.
+      return WebCryptoAlgorithmIdNone;
+    case kJwkAlgorithmA128Gcm:
+    case kJwkAlgorithmA256Gcm:
+      return WebKit::WebCryptoAlgorithmIdAesGcm;
+    case kJwkAlgorithmA128Cbc:
+    case kJwkAlgorithmA256Cbc:
+    case kJwkAlgorithmA384Cbc:
+    case kJwkAlgorithmA512Cbc:
+      return WebKit::WebCryptoAlgorithmIdAesCbc;
+    default:
+      DCHECK(false);
+      return WebCryptoAlgorithmIdNone;
+  }
+}
+
+unsigned short JwkAlgIdToKeyLengthBits(int jwk_algorithm_id) {
+  switch (jwk_algorithm_id) {
+    case kJwkAlgorithmA128Kw:
+    case kJwkAlgorithmA128Gcm:
+    case kJwkAlgorithmA128Cbc:
+      return 128;
+    case kJwkAlgorithmHs256:
+    case kJwkAlgorithmA256Kw:
+    case kJwkAlgorithmRs256:
+    case kJwkAlgorithmA256Gcm:
+    case kJwkAlgorithmA256Cbc:
+      return 256;
+    case kJwkAlgorithmHs384:
+    case kJwkAlgorithmRs384:
+    case kJwkAlgorithmA384Cbc:
+      return 384;
+    case kJwkAlgorithmHs512:
+    case kJwkAlgorithmRs512:
+    case kJwkAlgorithmA512Cbc:
+      return 512;
+    default:
+      return 0;
+  }
+}
+
+WebKit::WebCryptoAlgorithm CreateWebCryptoAlgorithmFromJwkAlgorithmId(

[eroman, 2013/10/22 21:49:38]

I think things would be more readable to have a single map from name -->
function, and directly register the algorithm creation functions in there.

[padolph, 2013/10/28 20:35:07]

Done. But since the creation functions have different signatures I had to do
some template stuff to get them all into a map. Much more concise now but is a
different kind of ugly. Please take a look.

+    int jwk_algorithm_id) {
+  const unsigned short key_length_bits =
+      JwkAlgIdToKeyLengthBits(jwk_algorithm_id);
+  DCHECK(key_length_bits);
+  switch(jwk_algorithm_id) {
+    case kJwkAlgorithmHs256:
+    case kJwkAlgorithmHs384:
+    case kJwkAlgorithmHs512:
+      return CreateHmacAlgorithm(key_length_bits);
+    case kJwkAlgorithmRs256:
+    case kJwkAlgorithmRs384:
+    case kJwkAlgorithmRs512:
+      return CreateRsaSsaAlgorithm(key_length_bits);
+    case kJwkAlgorithmRsa1_5:
+      return CreateAlgorithm(WebKit::WebCryptoAlgorithmIdRsaEsPkcs1v1_5);
+    case kJwkAlgorithmRsaOaep:
+      return CreateRsaOaepAlgorithm(key_length_bits);
+    case kJwkAlgorithmA128Kw:
+    case kJwkAlgorithmA256Kw:
+      // TODO(padolph) Support AES keywrap algorithm, required for JWK but not
+      // present in the Web Crypto spec.
+      return CreateAlgorithm(WebCryptoAlgorithmIdNone);
+    case kJwkAlgorithmA128Gcm:
+    case kJwkAlgorithmA256Gcm:
+      return CreateAesGcmAlgorithm(key_length_bits);
+    case kJwkAlgorithmA128Cbc:
+    case kJwkAlgorithmA256Cbc:
+    case kJwkAlgorithmA384Cbc:
+    case kJwkAlgorithmA512Cbc:
+      return CreateAesCbcAlgorithm(key_length_bits);
+    default:
+      DCHECK(false);

[eroman, 2013/10/22 21:49:38]

NOTREACHED()

[padolph, 2013/10/28 20:35:07]

This code is now gone.

+      return CreateAlgorithm(WebCryptoAlgorithmIdNone);
+  }
+}
+
+typedef std::map<std::string, int> StringIntMap;
+
+void InitJwkAlgorithmMap(StringIntMap& jwk_algorithm_map)

[eroman, 2013/10/22 21:49:38]

Chromium style (unlike Blink style) does not like non-const references, a
pointer should be used instead.

That said, I have a different suggestion for this initialization (explained
where this function is used).

[padolph, 2013/10/28 20:35:07]

This code is now gone.

+{
+  // Note: A*CBC are not yet present in the JOSE JWA spec
+  // http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-16
+  jwk_algorithm_map["HS256"   ] = kJwkAlgorithmHs256;

[eroman, 2013/10/22 21:49:38]

style nit: remove the space padding (i personally prefer with the spaces, but
not in line with style guide).

[padolph, 2013/10/28 20:35:07]

Done.

+  jwk_algorithm_map["HS384"   ] = kJwkAlgorithmHs384;
+  jwk_algorithm_map["HS512"   ] = kJwkAlgorithmHs512;
+  jwk_algorithm_map["RS256"   ] = kJwkAlgorithmRs256;
+  jwk_algorithm_map["RS384"   ] = kJwkAlgorithmRs384;
+  jwk_algorithm_map["RS512"   ] = kJwkAlgorithmRs512;
+  jwk_algorithm_map["RSA1_5"  ] = kJwkAlgorithmRsa1_5;
+  jwk_algorithm_map["RSA-OAEP"] = kJwkAlgorithmRsaOaep;
+  jwk_algorithm_map["A128KW"  ] = kJwkAlgorithmA128Kw;
+  jwk_algorithm_map["A256KW"  ] = kJwkAlgorithmA256Kw;
+  jwk_algorithm_map["A128GCM" ] = kJwkAlgorithmA128Gcm;
+  jwk_algorithm_map["A256GCM" ] = kJwkAlgorithmA256Gcm;
+  jwk_algorithm_map["A128CBC" ] = kJwkAlgorithmA128Cbc;
+  jwk_algorithm_map["A256CBC" ] = kJwkAlgorithmA256Cbc;
+  jwk_algorithm_map["A384CBC" ] = kJwkAlgorithmA384Cbc;
+  jwk_algorithm_map["A512CBC" ] = kJwkAlgorithmA512Cbc;
+}
+
+// forward
+bool operator!=(const WebKit::WebCryptoAlgorithm& lhs,
+                const WebKit::WebCryptoAlgorithm& rhs);
+
+// TODO(padolph) Verify this logic is sufficient to judge algorithm
+// "consistency" for JWK import, and for all supported algorithms.
+bool operator==(const WebKit::WebCryptoAlgorithm& lhs,

[eroman, 2013/10/22 21:49:38]

Chromium style guide doesn't like operator overloading. Please use a function
instead like AlgorithmsMatch()

[Ryan Sleevi, 2013/10/24 01:45:58]

Should WebKit::WebCryptoAlgorithm have an Equals() method instead? This all
seems like it should be in the Blink side of things?

[eroman, 2013/10/24 02:35:04]

It isn't obvious what equality means, so I think this is better left to the
consumers rather than in Blink. To give some examples:

  * If optional parameters for an algorithm are missing, does equality infer
defaults?
  * Parameter types may not match. For instance one could be key generation
parameters, another the algorithm parameters. Conceptually the same "algorithm",
but different objects.
  * Do you normalize parameters? For instance BigIntegers can contain
zero-padding but still represent the same number.

I am receptive to the idea, just not sure how general of a mechanism you are
thinking.

[padolph, 2013/10/28 20:35:07]

Function name changed. But I am also very confused about the definition of
"inconsistency" between algorithms in this case.

Also, it seems like a big burden to put on the API user to have to guess what is
inside the JWK and construct a matching algorithm to get the import to work. If
the JWK came from inside a JWE received from a remote source, how will the user
ever know the details?

+                const WebKit::WebCryptoAlgorithm& rhs) {
+  if (lhs.id() != rhs.id())
+    return false;
+  if (lhs.id() == WebKit::WebCryptoAlgorithmIdHmac ||
+      lhs.id() == WebKit::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 ||
+      lhs.id() == WebKit::WebCryptoAlgorithmIdRsaOaep) {
+    if (lhs.hmacParams()->hash() != rhs.hmacParams()->hash())
+      return false;
+  }
+  return true;

[eroman, 2013/10/22 21:49:38]

For the sake of future proofing, I would rather have the default be "false", and
only returning "true" for the hard-coded cases.

[padolph, 2013/10/28 20:35:07]

Done.

+}
+
+bool operator!=(const WebKit::WebCryptoAlgorithm& lhs,
+                const WebKit::WebCryptoAlgorithm& rhs) {
+  return !(lhs == rhs);
+}
+
+}  // namespace
+
 WebCryptoImpl::WebCryptoImpl() {
   Init();
+  InitJwkAlgorithmMap(jwk_algorithm_map_);

[eroman, 2013/10/22 21:49:38]

I would prefer lazy initialization, since JWK may not always be used. Can use
the Singleton pattern or a function-level static for this case.

For instance:

const StringMap& JwkAlgorithms() {
  static StringMap algorithms = CreateJwkAlgorithms();
  return algorithms;
}

[Ryan Sleevi, 2013/10/24 01:45:58]

Don't use a function-level static. They're forbidden by the style guide.

Feel free to through fruit at eroman for suggesting it ;)

Use a base::LazyInstance(::Leaky ? depends on threading lifetimes)

[eroman, 2013/10/24 02:35:04]

My mistake! I got muddled with Blink style vs Chromium style.

[padolph, 2013/10/28 20:35:07]

LazyInstance used.

 }
 
 void WebCryptoImpl::encrypt(
@@ -84,20 +343,30 @@ void WebCryptoImpl::importKey(
   WebKit::WebCryptoKeyType type;
   scoped_ptr<WebKit::WebCryptoKeyHandle> handle;
 
-  if (!ImportKeyInternal(format,
-                         key_data,
-                         key_data_size,
-                         algorithm,
-                         usage_mask,
-                         &handle,
-                         &type)) {
-    result.completeWithError();
-    return;
+  if (format == WebKit::WebCryptoKeyFormatJwk) {
+    if (!ImportKeyJwk(key_data,
+                      key_data_size,
+                      extractable,
+                      algorithm,
+                      usage_mask,
+                      &handle,
+                      &type)) {
+      result.completeWithError();
+    }
+  } else {
+    if (!ImportKeyInternal(format,
+                           key_data,
+                           key_data_size,
+                           algorithm,
+                           usage_mask,
+                           &handle,
+                           &type)) {
+      result.completeWithError();
+    }
   }
 
-  WebKit::WebCryptoKey key(
-      WebKit::WebCryptoKey::create(
-          handle.release(), type, extractable, algorithm, usage_mask));
+  WebKit::WebCryptoKey key(WebKit::WebCryptoKey::create(
+      handle.release(), type, extractable, algorithm, usage_mask));
 
   result.completeWithKey(key);
 }
@@ -138,4 +407,204 @@ void WebCryptoImpl::verifySignature(
   }
 }
 
+bool WebCryptoImpl::ImportKeyJwk(
+    const unsigned char* key_data,
+    unsigned key_data_size,
+    bool extractable,
+    const WebKit::WebCryptoAlgorithm& algorithm,
+    WebKit::WebCryptoKeyUsageMask usage_mask,
+    scoped_ptr<WebKit::WebCryptoKeyHandle>* handle,
+    WebKit::WebCryptoKeyType* type) {
+
+  // JSON Web Key Format (JWK)

[eroman, 2013/10/22 21:49:38]

This is a good comment block, thanks!

+  // http://tools.ietf.org/html/draft-ietf-jose-json-web-key-16
+  // TODO(padolph) Not all possible values are handled by this code right now
+  //
+  // A JWK is a simple JSON dictionary with the following entries
+  // - "kty" (Key Type) Parameter, REQUIRED
+  // - <kty-specific parameters, see below>, REQUIRED
+  // - "use" (Key Use) Parameter, OPTIONAL
+  // - "alg" (Algorithm) Parameter, OPTIONAL
+  // - "extractable" (Key Exportability), OPTIONAL [NOTE: not yet part of JOSE]
+  // (all other entries are ignored)
+  //
+  // Input key_data contains the JWK. To build a Web Crypto Key, the JWK values
+  // are parsed out and used as follows:
+  // Web Crypto Key type            <-- (deduced)
+  // Web Crypto Key extractable     <-- extractable
+  // Web Crypto Key algorithm       <-- alg
+  // Web Crypto Key keyUsage        <-- usage
+  // Web Crypto Key keying material <-- kty-specific parameters
+  //
+  // Values for each entry are case-sensitive and defined in
+  // http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-16.
+  // Note that not all values specified by JOSE are handled by this code. Only
+  // handled values are listed.
+  // - kty (Key Type)
+  //   +-------+--------------------------------------------------------------+
+  //   | "RSA" | RSA [RFC3447]                                                |
+  //   | "oct" | Octet sequence (used to represent symmetric keys)            |
+  //   +-------+--------------------------------------------------------------+
+  // - use (Key Use)
+  //   +-------+--------------------------------------------------------------+
+  //   | "enc" | encrypt and decrypt operations                               |
+  //   | "sig" | sign and verify (MAC) operations                             |
+  //   | "wrap"| key wrap and unwrap [not yet part of JOSE]                   |
+  //   +-------+--------------------------------------------------------------+
+  // - extractable (Key Exportability)
+  //   +-------+--------------------------------------------------------------+
+  //   | true  | Key may be exported from the trusted environment             |
+  //   | false | Key cannot exit the trusted environment                      |
+  //   +-------+--------------------------------------------------------------+
+  // - alg (Algorithm)
+  //   See http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-16
+  //   +--------------+-------------------------------------------------------+
+  //   | Digital Signature or MAC Algorithm                                   |
+  //   +--------------+-------------------------------------------------------+
+  //   | "HS256"      | HMAC using SHA-256 hash algorithm                     |
+  //   | "HS384"      | HMAC using SHA-384 hash algorithm                     |
+  //   | "HS512"      | HMAC using SHA-512 hash algorithm                     |
+  //   | "RS256"      | RSASSA using SHA-256 hash algorithm                   |
+  //   | "RS384"      | RSASSA using SHA-384 hash algorithm                   |
+  //   | "RS512"      | RSASSA using SHA-512 hash algorithm                   |
+  //   +--------------+-------------------------------------------------------|
+  //   | Key Management Algorithm                                             |
+  //   +--------------+-------------------------------------------------------+
+  //   | "RSA1_5"     | RSAES-PKCS1-V1_5 [RFC3447]                            |
+  //   | "RSA-OAEP"   | RSAES using Optimal Asymmetric Encryption Padding     |
+  //   |              | (OAEP) [RFC3447], with the default parameters         |
+  //   |              | specified by RFC3447 in Section A.2.1                 |
+  //   | "A128KW"     | Advanced Encryption Standard (AES) Key Wrap Algorithm |
+  //   |              | [RFC3394] using 128 bit keys                          |
+  //   | "A256KW"     | AES Key Wrap Algorithm using 256 bit keys             |
+  //   | "A128GCM"    | AES in Galois/Counter Mode (GCM) [NIST.800-38D] using |
+  //   |              | 128 bit keys                                          |
+  //   | "A256GCM"    | AES GCM using 256 bit keys                            |
+  //   | "A128CBC"    | AES in Cipher Block Chaining Mode (CBC) with PKCS #5  |
+  //   |              | padding [NIST.800-38A] [not yet part of JOSE]         |
+  //   | "A256CBC"    | AES CBC using 256 bit keys [not yet part of JOSE]     |
+  //   | "A384CBC"    | AES CBC using 384 bit keys [not yet part of JOSE]     |
+  //   | "A512CBC"    | AES CBC using 512 bit keys [not yet part of JOSE]     |
+  //   +--------------+-------------------------------------------------------+
+  //
+  // kty-specific parameters
+  // The value of kty determines the type and content of the keying material
+  // carried in the JWK to be imported. Currently only two possibilities are
+  // supported: a raw key or an RSA public key. RSA private keys are not
+  // supported because typical applications seldom need to import a private key,
+  // and the large number of JWK parameters required to describe one.
+  // - kty == "oct" (symmetric or other raw key)
+  //   +-------+--------------------------------------------------------------+
+  //   | "k"   | Contains the value of the symmetric (or other single-valued) |
+  //   |       | key.  It is represented as the base64url encoding of the     |
+  //   |       | octet sequence containing the key value.                     |
+  //   +-------+--------------------------------------------------------------+
+  // - kty == "RSA" (RSA public key)
+  //   +-------+--------------------------------------------------------------+
+  //   | "n"   | Contains the modulus value for the RSA public key.  It is    |
+  //   |       | represented as the base64url encoding of the value's         |
+  //   |       | unsigned big endian representation as an octet sequence.     |
+  //   +-------+--------------------------------------------------------------+
+  //   | "e"   | Contains the exponent value for the RSA public key.  It is   |
+  //   |       | represented as the base64url encoding of the value's         |
+  //   |       | unsigned big endian representation as an octet sequence.     |
+  //   +-------+--------------------------------------------------------------+
+  //
+  // Conflict resolution
+  // The type, algorithm, extractable, and usage_mask input parameters may be
+  // different from similar values inside the JWK. The Web Crypto spec says that
+  // if a JWK value is present, but is inconsistent with the input value, it is
+  // an error and the operation must fail.
+
+  const char* const foo = reinterpret_cast<const char *>(key_data);

[eroman, 2013/10/22 21:49:38]

Please use a variable name other than "foo".

[padolph, 2013/10/28 20:35:07]

Done.

+  base::StringPiece json_string(foo, key_data_size);
+  scoped_ptr<base::Value> value(base::JSONReader::Read(json_string));
+  // Note, bare pointer dict_value is ok since it points into scoped value.
+  base::DictionaryValue* dict_value = NULL;
+  if (!value.get() || !value->GetAsDictionary(&dict_value) || !dict_value)
+    return false;
+
+  // JWK "kty". Exit early if this required JWK parameter is missing.
+  std::string jwk_kty_value;
+  if (!dict_value->GetString("kty", &jwk_kty_value))
+    return false;
+
+  // JWK "extractable" --> extractable parameter
+  bool jwk_extractable_value;
+  if (dict_value->GetBoolean("extractable", &jwk_extractable_value)) {
+    // collision check
+    if (jwk_extractable_value != extractable)
+      return false;
+  }
+
+  // JWK "alg" --> algorithm parameter
+  std::string jwk_alg_value;
+  if (dict_value->GetString("alg", &jwk_alg_value)) {
+    const StringIntMap::iterator pos = jwk_algorithm_map_.find(jwk_alg_value);

[eroman, 2013/10/22 21:49:38]

const_iterator?

[padolph, 2013/10/28 20:35:07]

Done.

+    if (pos == jwk_algorithm_map_.end())
+      return false;
+    const int jwk_algorithm_id = pos->second;
+    WebKit::WebCryptoAlgorithm webcrypto_algorithm_from_jwk =
+        CreateWebCryptoAlgorithmFromJwkAlgorithmId(jwk_algorithm_id);
+    // collision check
+    if (webcrypto_algorithm_from_jwk != algorithm)
+      return false;
+  }
+
+  // JWK "use" --> usage_mask parameter
+  std::string jwk_use_value;
+  if (dict_value->GetString("use", &jwk_use_value)) {
+    unsigned jwk_usage_mask;
+    if (jwk_use_value == "enc") {
+      jwk_usage_mask =
+          WebKit::WebCryptoKeyUsageEncrypt | WebKit::WebCryptoKeyUsageDecrypt;
+    } else if (jwk_use_value == "sig") {
+      jwk_usage_mask =
+          WebKit::WebCryptoKeyUsageSign | WebKit::WebCryptoKeyUsageVerify;
+    } else if (jwk_use_value == "wrap") {
+      jwk_usage_mask =
+          WebKit::WebCryptoKeyUsageWrapKey | WebKit::WebCryptoKeyUsageUnwrapKey;
+    } else {
+      return false;
+    }
+    // collision check
+    if (!(jwk_usage_mask & usage_mask))

[eroman, 2013/10/22 21:49:38]

Do they need to match exactly?

[Ryan Sleevi, 2013/10/24 01:45:58]

No.

The spec merely says that if they conflict in ways the implementation disagrees
with.

[eroman, 2013/10/24 02:35:04]

Thanks Ryan!

My understanding then is that we want to enforce that "usages" given by user to
importKey() is a subset of the jwk_usages, and if not fail the import. Paul, can
you confirm?

If that understanding is correct, then this line has a bug, since it is testing
for *any* intersection.

Instead I think you want this:

if (jwk_usage_mask & usage_mask != usage_mask) {
  // usage_mask must be a subset of jwk_usage_mask
  return false;
}

Paul: Please add a corresponding unittest.

[padolph, 2013/10/28 20:35:07]

Yea, after thinking about it more, subset makes more sense.
Done.

+      return false;
+  }
+
+  // JWK keying material --> ImportKeyInternal()
+  if (jwk_kty_value == "oct") {
+    // collision check
+    if (*type != WebKit::WebCryptoKeyTypeSecret)
+      return false;
+    std::string jwk_k_value_url64;
+    if (!dict_value->GetString("k", &jwk_k_value_url64))
+      return false;
+    std::string jwk_k_value;
+    if (!Base64DecodeUrlSafe(jwk_k_value_url64, &jwk_k_value))
+      return false;
+    const std::vector<uint8> data(jwk_k_value.begin(), jwk_k_value.end());

[eroman, 2013/10/22 21:49:38]

Rather than copying into a std::vector<>, I believe the string's data an be cast
to uint8 and passed directly to ImportKeyInternal.

[padolph, 2013/10/28 20:35:07]

Done.

+    if (!ImportKeyInternal(WebKit::WebCryptoKeyFormatRaw,
+                           &data[0],

[eroman, 2013/10/22 21:49:38]

Is data guaranteed to have at least 1 element?

[padolph, 2013/10/28 20:35:07]

No. I added a check for that plus a unit test. Thanks.

+                           data.size(),
+                           algorithm,
+                           usage_mask,
+                           handle,
+                           type)) {
+      return false;
+    }
+  } else if (jwk_kty_value == "RSA") {
+    // Only an RSA public key is currently handled.
+    // collision check
+    if (*type != WebKit::WebCryptoKeyTypePublic)
+      return false;
+    // TODO(padolph): JWK import RSA public key
+    return false;
+  } else {
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace content