[webcrypto] JWK: Updated import(ext, key_ops) and added export of symmetric keys

content/renderer/webcrypto/jwk.cc

Index: content/renderer/webcrypto/jwk.cc
diff --git a/content/renderer/webcrypto/jwk.cc b/content/renderer/webcrypto/jwk.cc
index 5d72f45288e0f7f8409154026733ae21dd94892d..aa0f629ec907ee5e86df1d32efa997fa8577f070 100644
--- a/content/renderer/webcrypto/jwk.cc
+++ b/content/renderer/webcrypto/jwk.cc
@@ -5,16 +5,19 @@
 #include <algorithm>
 #include <functional>
 #include <map>
+#include "base/base64.h"
 #include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_piece.h"
-#include "base/values.h"
+#include "base/strings/stringprintf.h"
 #include "content/renderer/webcrypto/crypto_data.h"
 #include "content/renderer/webcrypto/platform_crypto.h"
 #include "content/renderer/webcrypto/shared_crypto.h"
 #include "content/renderer/webcrypto/webcrypto_util.h"
+#include "third_party/WebKit/public/platform/WebCryptoKeyAlgorithm.h"
 
 namespace content {
 
@@ -24,6 +27,43 @@ namespace {
 
 typedef blink::WebCryptoAlgorithm (*AlgorithmCreationFunc)();
 
+// Web Crypto equivalent usage mask for JWK 'use' = 'enc'.
+// TODO(padolph): Add 'deriveBits' once supported by Blink.
+const blink::WebCryptoKeyUsageMask kJwkEncUsage =
+    blink::WebCryptoKeyUsageEncrypt | blink::WebCryptoKeyUsageDecrypt |
+    blink::WebCryptoKeyUsageWrapKey | blink::WebCryptoKeyUsageUnwrapKey |
+    blink::WebCryptoKeyUsageDeriveKey;
+// Web Crypto equivalent usage mask for JWK 'use' = 'sig'.
+const blink::WebCryptoKeyUsageMask kJwkSigUsage =
+    blink::WebCryptoKeyUsageSign | blink::WebCryptoKeyUsageVerify;
+
+// TODO(padolph): Should these IsAlgorithm* functions be moved to
+// webcrypto_util?
+bool IsAlgorithmAsymmetric(const blink::WebCryptoKeyAlgorithm& algorithm) {
+  // TODO(padolph): include all other asymmetric algorithms once they are
+  // defined, e.g. EC and DH.
+  return algorithm.id() == blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5 ||
+         algorithm.id() == blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 ||
+         algorithm.id() == blink::WebCryptoAlgorithmIdRsaOaep;
+}
+
+bool IsAlgorithmAes(const blink::WebCryptoKeyAlgorithm& algorithm) {
+  // TODO(padolph): include other AES symmetric algorithms once they are

[eroman, 2014/03/04 02:55:14]

How about:
  return algorithm.paramsType() == blink::WebCryptoKeyAlgorithmParamsTypeAes;

[padolph, 2014/03/05 03:08:51]

Done.

+  // defined.
+  const blink::WebCryptoAlgorithmId id = algorithm.id();
+  return id == blink::WebCryptoAlgorithmIdAesCbc ||
+         id == blink::WebCryptoAlgorithmIdAesCtr ||
+         id == blink::WebCryptoAlgorithmIdAesGcm ||
+         id == blink::WebCryptoAlgorithmIdAesKw;
+}
+
+bool IsAlgorithmSymmetric(const blink::WebCryptoKeyAlgorithm& algorithm) {
+  // TODO(padolph): include all other symmetric algorithms once they are
+  // defined.
+  return IsAlgorithmAes(algorithm) ||
+         algorithm.id() == blink::WebCryptoAlgorithmIdHmac;
+}
+
 class JwkAlgorithmInfo {
  public:
   JwkAlgorithmInfo()
@@ -94,15 +134,18 @@ class JwkAlgorithmRegistry {
     alg_to_info_["RSA-OAEP"] =
         JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaOaepImportAlgorithm,
                                           blink::WebCryptoAlgorithmIdSha1>);
-    // TODO(padolph): The Web Crypto spec does not enumerate AES-KW 128 yet
-    alg_to_info_["A128KW"] =
-        JwkAlgorithmInfo(&blink::WebCryptoAlgorithm::createNull, 128);
-    // TODO(padolph): The Web Crypto spec does not enumerate AES-KW 256 yet
-    alg_to_info_["A256KW"] =
-        JwkAlgorithmInfo(&blink::WebCryptoAlgorithm::createNull, 256);
+    alg_to_info_["A128KW"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesKw>);

[eroman, 2014/03/04 02:55:14]

Include the key length as the final parameter, like you did for A192GCM.

(This is used for verifying that the correct length key was given, rather than
inferring algorithm from the key length)

[padolph, 2014/03/05 03:08:51]

Done.

+    alg_to_info_["A192KW"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesKw>);
+    alg_to_info_["A256KW"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesKw>);
     alg_to_info_["A128GCM"] = JwkAlgorithmInfo(
         &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesGcm>,
         128);
+    alg_to_info_["A192GCM"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesGcm>,
+        192);
     alg_to_info_["A256GCM"] = JwkAlgorithmInfo(
         &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesGcm>,
         256);
@@ -197,9 +240,29 @@ Status GetOptionalJwkString(base::DictionaryValue* dict,
   return Status::Success();
 }
 
+// Extracts the optional array property with key |path| from |dict| and saves
+// the result to |*result| if it was found. If the property exists and is not an
+// array, returns an error. Otherwise returns success, and sets
+// |*property_exists| if it was found.

[eroman, 2014/03/04 02:55:14]

Can you mention that the pointer *result is owned by the dictionary?

[padolph, 2014/03/05 03:08:51]

Done.

+Status GetOptionalJwkList(base::DictionaryValue* dict,
+                          const std::string& path,
+                          base::ListValue** result,
+                          bool* property_exists) {
+  *property_exists = false;
+  base::Value* value = NULL;
+  if (!dict->Get(path, &value))
+    return Status::Success();
+
+  if (!value->GetAsList(result))
+    return Status::ErrorJwkPropertyWrongType(path, "list");
+
+  *property_exists = true;
+  return Status::Success();
+}
+
 // Extracts the required string property with key |path| from |dict| and saves
-// the base64-decoded bytes to |*result|. If the property does not exist or is
-// not a string, or could not be base64-decoded, returns an error.
+// the base64url-decoded bytes to |*result|. If the property does not exist or
+// is not a string, or could not be base64url-decoded, returns an error.
 Status GetJwkBytes(base::DictionaryValue* dict,
                    const std::string& path,
                    std::string* result) {
@@ -234,6 +297,9 @@ Status GetOptionalJwkBool(base::DictionaryValue* dict,
   return Status::Success();
 }
 
+// Returns true if the set bits in b are a strict subset of the set bits in a.

[eroman, 2014/03/04 02:55:14]

I don't quite understand the comment "strict subset".

In mathematical parlance for B to be a strict subset (or proper subset) of A
means that A != B. However that is not the case for this helper.

Suggest you remove or clarify that comment.

[padolph, 2014/03/05 03:08:51]

Done.

+bool IsBitwiseSubset(unsigned int a, unsigned int b) { return (a & b) == b; }
+
 }  // namespace
 
 Status ImportKeyJwk(const CryptoData& key_data,
@@ -247,16 +313,15 @@ Status ImportKeyJwk(const CryptoData& key_data,
   // a Web Crypto Key.
   //
   // JSON Web Key Format (JWK)
-  // 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
+  // http://tools.ietf.org/html/draft-ietf-jose-json-web-key-21
   //
   // 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
+  // - "key_ops" (Key Operations) Parameter, OPTIONAL
   // - "alg" (Algorithm) Parameter, OPTIONAL
-  // - "extractable" (Key Exportability), OPTIONAL [NOTE: not yet part of JOSE,
-  //   see https://www.w3.org/Bugs/Public/show_bug.cgi?id=23796]
+  // - "ext" (Key Exportability), OPTIONAL
   // (all other entries are ignored)
   //
   // OPTIONAL here means that this code does not require the entry to be present

[eroman, 2014/03/04 02:55:14]

Perhaps as a separate changelist, I suggest moving this big comment block to the
top of the file. Since the JWK format documentation applies not just to this
function, but the file in general.

[padolph, 2014/03/05 03:08:51]

Agreed. Added a TODO.

@@ -271,13 +336,13 @@ Status ImportKeyJwk(const CryptoData& key_data,
   // values are parsed out and combined with the method input parameters to
   // build a Web Crypto Key:
   // Web Crypto Key type            <-- (deduced)
-  // Web Crypto Key extractable     <-- JWK extractable + input extractable
+  // Web Crypto Key extractable     <-- JWK ext + input extractable
   // Web Crypto Key algorithm       <-- JWK alg + input algorithm
-  // Web Crypto Key keyUsage        <-- JWK use + input usage_mask
+  // Web Crypto Key keyUsage        <-- JWK use, key_ops + input usage_mask
   // Web Crypto Key keying material <-- kty-specific parameters
   //
   // Values for each JWK entry are case-sensitive and defined in
-  // http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-16.
+  // http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-18.
   // Note that not all values specified by JOSE are handled by this code. Only
   // handled values are listed.
   // - kty (Key Type)
@@ -285,19 +350,46 @@ Status ImportKeyJwk(const CryptoData& key_data,
   //   | "RSA" | RSA [RFC3447]                                                |
   //   | "oct" | Octet sequence (used to represent symmetric keys)            |
   //   +-------+--------------------------------------------------------------+
+  //
+  // - key_ops (Key Use Details)
+  //   The key_ops field is an array that contains one or more strings from
+  //   the table below, and describes the operations for which this key may be
+  //   used.
+  //   +-------+--------------------------------------------------------------+
+  //   | "encrypt"    | encrypt operations                                    |
+  //   | "decrypt"    | decrypt operations                                    |
+  //   | "sign"       | sign (MAC) operations                                 |
+  //   | "verify"     | verify (MAC) operations                               |
+  //   | "wrapKey"    | key wrap                                              |
+  //   | "unwrapKey"  | key unwrap                                            |
+  //   | "deriveKey"  | key derivation                                        |
+  //   | "deriveBits" | key derivation TODO(padolph): not currently supported |
+  //   +-------+--------------------------------------------------------------+
+  //
   // - use (Key Use)
+  //   The use field contains a single entry from the table below.
   //   +-------+--------------------------------------------------------------+
-  //   | "enc" | encrypt and decrypt operations                               |
-  //   | "sig" | sign and verify (MAC) operations                             |
-  //   | "wrap"| key wrap and unwrap [not yet part of JOSE]                   |
+  //   | "sig"     | equivalent to key_ops of [sign, verify]                  |
+  //   | "enc"     | equivalent to key_ops of [encrypt, decrypt, wrapKey,     |
+  //   |           | unwrapKey, deriveKey, deriveBits]                        |
   //   +-------+--------------------------------------------------------------+
-  // - extractable (Key Exportability)
+  //
+  //   NOTE: If both "use" and "key_ops" JWK members are present, the usages
+  //   specified by them MUST be consistent.  In particular, the "use" value
+  //   "sig" corresponds to "sign" and/or "verify".  The "use" value "enc"
+  //   corresponds to all other values defined above.  If "key_ops" values
+  //   corresponding to both "sig" and "enc" "use" values are present, the "use"
+  //   member SHOULD NOT be present, and if present, its value MUST NOT be
+  //   either "sig" or "enc".
+  //
+  // - ext (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
+  //   See http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-18
   //   +--------------+-------------------------------------------------------+
   //   | Digital Signature or MAC Algorithm                                   |
   //   +--------------+-------------------------------------------------------+
@@ -316,15 +408,17 @@ Status ImportKeyJwk(const CryptoData& key_data,
   //   |              | specified by RFC3447 in Section A.2.1                 |
   //   | "A128KW"     | Advanced Encryption Standard (AES) Key Wrap Algorithm |
   //   |              | [RFC3394] using 128 bit keys                          |
+  //   | "A192KW"     | AES Key Wrap Algorithm using 192 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                                          |
+  //   | "A192GCM"    | AES GCM using 192 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, see     |
   //   |              | https://www.w3.org/Bugs/Public/show_bug.cgi?id=23796  |
-  //   | "A192CBC"    | AES CBC using 192 bit keys [not yet part of JOSE]     |

[eroman, 2014/03/04 02:55:14]

thanks for updating these comments!

-  //   | "A256CBC"    | AES CBC using 256 bit keys [not yet part of JOSE]     |
+  //   | "A192CBC"    | AES CBC using 192 bit keys                            |
+  //   | "A256CBC"    | AES CBC using 256 bit keys                            |
   //   +--------------+-------------------------------------------------------+
   //
   // kty-specific parameters
@@ -366,8 +460,8 @@ Status ImportKeyJwk(const CryptoData& key_data,
   //   algorithm.
   //
   // extractable
-  //   If the JWK extractable is true but the input parameter is false, make the
-  //   Web Crypto Key non-extractable. Conversely, if the JWK extractable is
+  //   If the JWK ext field is true but the input parameter is false, make the
+  //   Web Crypto Key non-extractable. Conversely, if the JWK ext field is
   //   false but the input parameter is true, it is an inconsistency. If both
   //   are true or both are false, use that value.
   //
@@ -396,18 +490,16 @@ Status ImportKeyJwk(const CryptoData& key_data,
   if (status.IsError())
     return status;
 
-  // JWK "extractable" (optional) --> extractable parameter
+  // JWK "ext" (optional) --> extractable parameter
   {
-    bool jwk_extractable_value = false;
-    bool has_jwk_extractable;
-    status = GetOptionalJwkBool(dict_value,
-                                "extractable",
-                                &jwk_extractable_value,
-                                &has_jwk_extractable);
+    bool jwk_ext_value = false;
+    bool has_jwk_ext;
+    status =
+        GetOptionalJwkBool(dict_value, "ext", &jwk_ext_value, &has_jwk_ext);
     if (status.IsError())
       return status;
-    if (has_jwk_extractable && !jwk_extractable_value && extractable)
-      return Status::ErrorJwkExtractableInconsistent();
+    if (has_jwk_ext && !jwk_ext_value && extractable)
+      return Status::ErrorJwkExtInconsistent();
   }
 
   // JWK "alg" (optional) --> algorithm parameter
@@ -460,6 +552,24 @@ Status ImportKeyJwk(const CryptoData& key_data,
   }
   DCHECK(!algorithm.isNull());
 
+  // JWK "key_ops" (optional) --> usage_mask parameter
+  base::ListValue* jwk_key_ops_value = NULL;
+  bool has_jwk_key_ops;
+  status = GetOptionalJwkList(
+      dict_value, "key_ops", &jwk_key_ops_value, &has_jwk_key_ops);
+  if (status.IsError())
+    return status;
+  blink::WebCryptoKeyUsageMask jwk_key_ops_mask = 0;
+  if (has_jwk_key_ops) {
+    Status status =

[eroman, 2014/03/04 02:55:14]

I prefer not shadowing the variable (can be tricky when reading). Can you
instead overwrite status?

[padolph, 2014/03/05 03:08:51]

Done.

+        GetUsagesFromJwkKeyOps(jwk_key_ops_value, &jwk_key_ops_mask);
+    if (status.IsError())
+      return status;
+    // The input usage_mask must be a strict subset of jwk_key_ops_mask.
+    if (!IsBitwiseSubset(jwk_key_ops_mask, usage_mask))
+      return Status::ErrorJwkKeyopsInconsistent();
+  }
+
   // JWK "use" (optional) --> usage_mask parameter
   std::string jwk_use_value;
   bool has_jwk_use;
@@ -467,24 +577,27 @@ Status ImportKeyJwk(const CryptoData& key_data,
       GetOptionalJwkString(dict_value, "use", &jwk_use_value, &has_jwk_use);
   if (status.IsError())
     return status;
+  blink::WebCryptoKeyUsageMask jwk_use_mask = 0;
   if (has_jwk_use) {
-    blink::WebCryptoKeyUsageMask jwk_usage_mask = 0;
-    if (jwk_use_value == "enc") {
-      jwk_usage_mask =
-          blink::WebCryptoKeyUsageEncrypt | blink::WebCryptoKeyUsageDecrypt;
-    } else if (jwk_use_value == "sig") {
-      jwk_usage_mask =
-          blink::WebCryptoKeyUsageSign | blink::WebCryptoKeyUsageVerify;
-    } else if (jwk_use_value == "wrap") {
-      jwk_usage_mask =
-          blink::WebCryptoKeyUsageWrapKey | blink::WebCryptoKeyUsageUnwrapKey;
-    } else {
-      return Status::ErrorJwkUnrecognizedUsage();
-    }
-    if ((jwk_usage_mask & usage_mask) != usage_mask) {
-      // A usage_mask must be a subset of jwk_usage_mask.
-      return Status::ErrorJwkUsageInconsistent();
-    }
+    if (jwk_use_value == "enc")
+      jwk_use_mask = kJwkEncUsage;
+    else if (jwk_use_value == "sig")
+      jwk_use_mask = kJwkSigUsage;
+    else
+      return Status::ErrorJwkUnrecognizedUse();
+    // The input usage_mask must be a subset of jwk_use_mask.
+    if (!IsBitwiseSubset(jwk_use_mask, usage_mask))
+      return Status::ErrorJwkUseInconsistent();
+  }
+
+  // If both 'key_ops' and 'use' are present, ensure they are consistent.
+  if (has_jwk_key_ops && has_jwk_use) {
+    if ((jwk_use_mask == kJwkEncUsage) &&

[eroman, 2014/03/04 02:55:14]

Can you express this in a more future proof way? My concern is that if
jwk_use_mask were to ever be anything other than just kKwkEncUsage or
jkwSigUsage then we stop doing the consistency check.

Would something like this work:

if (has_jwk_key_ops && has_jkw_use && !IsBitwiseSubset(jwk_use_mask,
jkw_key_ops_mask))
   return ....

[padolph, 2014/03/05 03:08:51]

Yes - that works perfectly. Thanks.

+        !IsBitwiseSubset(kJwkEncUsage, jwk_key_ops_mask))
+      return Status::ErrorJwkUseAndKeyopsInconsistent();
+    if ((jwk_use_mask == kJwkSigUsage) &&
+        !IsBitwiseSubset(kJwkSigUsage, jwk_key_ops_mask))
+      return Status::ErrorJwkUseAndKeyopsInconsistent();
   }
 
   // JWK keying material --> ImportKeyInternal()
@@ -549,6 +662,134 @@ Status ImportKeyJwk(const CryptoData& key_data,
   return Status::Success();
 }
 
+Status ExportKeyJwk(const blink::WebCryptoKey& key,
+                    blink::WebArrayBuffer* buffer) {
+
+  // JWK has the following JSON structure

[eroman, 2014/03/04 02:55:14]

This seems like a duplication of the bigger comment block. You can leave it here
if you like, but I think having the format documentation at the top of the file
is sufficient.

[padolph, 2014/03/05 03:08:51]

Removed.

+  // {
+  //   'kty': key type, e.g. 'oct' or 'RSA'
+  //   'key_ops': an array of key usage values from ["sign", "verify",
+  //     "encrypt", "decrypt", "wrapKey", "unwrapKey", "deriveKey"]
+  //   'ext': true or false
+  //   'alg': key algorithm, e.g. 'RSA1_5, 'A128CBC', etc.
+  //   <kty-dependent parms>
+  // }
+  // See http://tools.ietf.org/html/draft-ietf-jose-json-web-key-21
+
+  base::DictionaryValue jwk_dict;
+
+  // ---- 'kty' and kty-dependent parms
+  unsigned key_length_bytes = 0;
+  if (IsAlgorithmSymmetric(key.algorithm())) {
+    DCHECK_EQ(blink::WebCryptoKeyTypeSecret, key.type());

[eroman, 2014/03/04 02:55:14]

In fact, would this not be a simpler way to implement the symmetric check?

[padolph, 2014/03/05 03:08:51]

Done.

+    jwk_dict.SetString("kty", "oct");
+    // For a symmetric key, the only extra field is 'k', containing the
+    // base64url encoding of the raw key.
+    blink::WebArrayBuffer raw_key;
+    Status status = ExportKey(blink::WebCryptoKeyFormatRaw, key, &raw_key);
+    if (status.IsError())
+      return status;
+    DCHECK(!raw_key.isNull());
+    DCHECK(raw_key.data());
+    DCHECK(raw_key.byteLength());
+    key_length_bytes = raw_key.byteLength();
+    const base::StringPiece key_str(static_cast<const char*>(raw_key.data()),
+                                    key_length_bytes);
+    std::string key_b64url;
+    webcrypto::Base64EncodeUrlSafe(key_str, &key_b64url);
+    jwk_dict.SetString("k", key_b64url);
+  } else {

[eroman, 2014/03/04 02:55:14]

Please split this function up into smaller functions.

 * Delegate symmetric keys vs public/private keys to separate functions
 * Serialization of key_ops in separate function
 * Serialization of alg

To structure it I suggest stack-allocating a DictionaryValue in the base
function, and having the helper functions take a pointer to DictionaryValue and
write into that. Pseudo-code:


  DictionaryValue jwk_dict;

  WriteKeyOps(key.usages(), &jwk_dict);
  WiteAlg(key.algorithm(), &jwk_dict);
  WriteExt(key.extractable(), &jwk_dict);

  if (key.type() == ..TypeSecret) {
    WriteSecretKey(key, &jwk_dict);
  } else {
     ...
  }
  
  JsonWriter::Write(..);

[padolph, 2014/03/05 03:08:51]

Done.

+    // TODO(padolph): Handle asymmetric keys, at least the public key.
+    return Status::ErrorUnsupported();
+  }
+
+  // ---- 'key_ops'
+  base::ListValue* key_ops = new base::ListValue;
+  if (key.usages() & blink::WebCryptoKeyUsageEncrypt)

[eroman, 2014/03/04 02:55:14]

This is the opposite of what we use for import right?

Rather than duplicate this in two places, you can create a static mapping like:


struct JwkToWebCryptoUsageMapping {
  const char* jwk_usage;
  ... webcrypto_usage;
};

const JwkToWebCryptoUsageMapping {
  {"encrypt", WebCryptoKeyUsageEncrypt},
  {"decrypt", WebCryptoKeyUsageDecrypt},
  ...
};

The import vs export functions would be an iteration of this same table.

[padolph, 2014/03/05 03:08:51]

Done, but I'm not sure I like the way it turned out. I like the single source of
truth for the mapping, but the impl is ugly using a linear search through the
map array. Also, I had to put the code in util because some of it is used by the
tests. Please take a look.

+    key_ops->AppendString("encrypt");
+  if (key.usages() & blink::WebCryptoKeyUsageDecrypt)
+    key_ops->AppendString("decrypt");
+  if (key.usages() & blink::WebCryptoKeyUsageDeriveKey)
+    key_ops->AppendString("deriveKey");
+  if (key.usages() & blink::WebCryptoKeyUsageSign)
+    key_ops->AppendString("sign");
+  if (key.usages() & blink::WebCryptoKeyUsageUnwrapKey)
+    key_ops->AppendString("unwrapKey");
+  if (key.usages() & blink::WebCryptoKeyUsageVerify)
+    key_ops->AppendString("verify");
+  if (key.usages() & blink::WebCryptoKeyUsageWrapKey)
+    key_ops->AppendString("wrapKey");
+  jwk_dict.Set("key_ops", key_ops);
+
+  // ---- 'ext'
+  jwk_dict.SetBoolean("ext", key.extractable());
+
+  // ---- 'alg'
+  const char* aes_prefix = "";
+  if (IsAlgorithmAes(key.algorithm())) {
+    switch (key_length_bytes) {
+      case 16:
+        aes_prefix = "A128";
+        break;
+      case 24:
+        aes_prefix = "A192";
+        break;
+      case 32:
+        aes_prefix = "A256";
+        break;
+      default:
+        NOTREACHED();  // bad key length means algorithm was built improperly
+        return Status::ErrorUnexpected();
+    }
+  }
+  switch (key.algorithm().id()) {

[eroman, 2014/03/04 02:55:14]

Is there a good way to re-use the table mapping from import? (which similarly
knows about the jwk name, and key length).

A future idea would be to move to virtual methods on KeyAlgorithmParams.

[padolph, 2014/03/05 03:08:51]

Not as it stands now. First, reverse table lookup must be a linear search, often
with functor, which can get ugly. Second, the value to be searched would have to
be the AlgorithmCreationFunc pointer which is not possible to find here.

We would likely have to add another field in the table to represent the
algorithm and search on that plus key length.

Or define a new lookup table with a flattened key that includes alg and key
length. But I'm not sure that's any better than the nested switch we have here.

Having the KeyAlgorithm able to generate it's own JWK string is interesting. But
I'm not sure about building JWK-specific code into Blink.

+    case blink::WebCryptoAlgorithmIdAesCbc:
+      jwk_dict.SetString("alg", base::StringPrintf("%s%s", aes_prefix, "CBC"));
+      break;
+    case blink::WebCryptoAlgorithmIdAesCtr:
+      jwk_dict.SetString("alg", base::StringPrintf("%s%s", aes_prefix, "CTR"));
+      break;
+    case blink::WebCryptoAlgorithmIdAesGcm:
+      jwk_dict.SetString("alg", base::StringPrintf("%s%s", aes_prefix, "GCM"));
+      break;
+    case blink::WebCryptoAlgorithmIdAesKw:
+      jwk_dict.SetString("alg", base::StringPrintf("%s%s", aes_prefix, "KW"));
+      break;
+    case blink::WebCryptoAlgorithmIdHmac:
+      DCHECK(key.algorithm().hmacParams());

[eroman, 2014/03/04 02:55:14]

Have you considered making this outter switch be on the
key.algorithm().paramsType() instead? The way the JWK name is built from the
webcrypto information depends largely on the key type.

[padolph, 2014/03/05 03:08:51]

Done. Maybe a little more readable now, and does not compute aes_prefix except
when required. Thanks.

+      switch (key.algorithm().hmacParams()->hash().id()) {
+        case blink::WebCryptoAlgorithmIdSha256:
+          jwk_dict.SetString("alg", "HS256");
+          break;
+        case blink::WebCryptoAlgorithmIdSha384:
+          jwk_dict.SetString("alg", "HS384");
+          break;
+        case blink::WebCryptoAlgorithmIdSha512:
+          jwk_dict.SetString("alg", "HS512");
+          break;
+        default:
+          // JWA does not allow any other HMAC inner hashes.
+          return Status::ErrorJwkUnsupportedHmacHash();

[eroman, 2014/03/04 02:55:14]

This is in disagreement with the latest revision of WebCrypto spec, which also
enumerates:

HS1, HS224

[padolph, 2014/03/05 03:08:51]

Done.

+      }
+      break;
+    case blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5:
+    case blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5:
+    case blink::WebCryptoAlgorithmIdRsaOaep:
+      // TODO(padolph): Handle RSA key export, at least the public key.
+      return Status::ErrorUnsupported();
+    default:
+      // TODO(padolph): Handle other key export.
+      return Status::ErrorUnsupported();
+  }
+
+  std::string json;
+  base::JSONWriter::Write(&jwk_dict, &json);
+  *buffer = webcrypto::CreateArrayBuffer(
+      reinterpret_cast<const uint8*>(json.data()), json.size());
+
+  return Status::Success();
+}
+
 }  // namespace webcrypto
 
 }  // namespace content