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

content/renderer/webcrypto/jwk.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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 {
 
 namespace webcrypto {
 
 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()
       : creation_func_(NULL),
         required_key_length_bytes_(NO_KEY_SIZE_REQUIREMENT) {}
 
   explicit JwkAlgorithmInfo(AlgorithmCreationFunc algorithm_creation_func)
       : creation_func_(algorithm_creation_func),
         required_key_length_bytes_(NO_KEY_SIZE_REQUIREMENT) {}
 
@@ skipping 50 matching lines @@
                                           blink::WebCryptoAlgorithmIdSha384>);
     alg_to_info_["RS512"] =
         JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaSsaImportAlgorithm,
                                           blink::WebCryptoAlgorithmIdSha512>);
     alg_to_info_["RSA1_5"] = JwkAlgorithmInfo(
         &BindAlgorithmId<CreateAlgorithm,
                          blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5>);
     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);
     alg_to_info_["A128CBC"] = JwkAlgorithmInfo(
         &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesCbc>,
         128);
     alg_to_info_["A192CBC"] = JwkAlgorithmInfo(
         &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesCbc>,
         192);
     alg_to_info_["A256CBC"] = JwkAlgorithmInfo(
@@ skipping 74 matching lines @@
   if (!dict->Get(path, &value))
     return Status::Success();
 
   if (!value->GetAsString(result))
     return Status::ErrorJwkPropertyWrongType(path, "string");
 
   *property_exists = true;
   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) {
   std::string base64_string;
   Status status = GetJwkString(dict, path, &base64_string);
   if (status.IsError())
     return status;
 
   if (!Base64DecodeUrlSafe(base64_string, result))
     return Status::ErrorJwkBase64Decode(path);
@@ skipping 14 matching lines @@
   if (!dict->Get(path, &value))
     return Status::Success();
 
   if (!value->GetAsBoolean(result))
     return Status::ErrorJwkPropertyWrongType(path, "boolean");
 
   *property_exists = true;
   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,
                     const blink::WebCryptoAlgorithm& algorithm_or_null,
                     bool extractable,
                     blink::WebCryptoKeyUsageMask usage_mask,
                     blink::WebCryptoKey* key) {
 
   // The goal of this method is to extract key material and meta data from the
   // incoming JWK, combine them with the input parameters, and ultimately import
   // 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.

   // in the incoming JWK, because the method input parameters contain similar
   // information. If the optional JWK entry is present, it will be validated
   // against the corresponding input parameter for consistency and combined with
   // it according to rules defined below. A special case is that the method
   // input parameter 'algorithm' is also optional. If it is null, the JWK 'alg'
   // value (if present) is used as a fallback.
   //
   // Input 'key_data' contains the JWK. To build a Web Crypto Key, the JWK
   // 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)
   //   +-------+--------------------------------------------------------------+
   //   | "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                                   |
   //   +--------------+-------------------------------------------------------+
   //   | "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                          |
+  //   | "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
   // 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)
   //   +-------+--------------------------------------------------------------+
@@ skipping 21 matching lines @@
   //
   // algorithm
   //   If an algorithm is provided by both the input parameter and the JWK,
   //   consistency between the two is based only on algorithm ID's (including an
   //   inner hash algorithm if present). In this case if the consistency
   //   check is passed, the input algorithm is used. If only one of either the
   //   input algorithm and JWK alg is provided, it is used as the final
   //   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.
   //
   // usage_mask
   //   The input usage_mask must be a strict subset of the interpreted JWK use
   //   value, else it is judged inconsistent. In all cases the input usage_mask
   //   is used as the final usage_mask.
   //
 
   if (!key_data.byte_length())
     return Status::ErrorImportEmptyKeyData();
   DCHECK(key);
 
   // Parse the incoming JWK JSON.
   base::StringPiece json_string(reinterpret_cast<const char*>(key_data.bytes()),
                                 key_data.byte_length());
   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 Status::ErrorJwkNotDictionary();
 
   // JWK "kty". Exit early if this required JWK parameter is missing.
   std::string jwk_kty_value;
   Status status = GetJwkString(dict_value, "kty", &jwk_kty_value);
   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
   // Note: input algorithm is also optional, so we have six cases to handle.
   // 1. JWK alg present but unrecognized: error
   // 2. JWK alg valid AND input algorithm isNull: use JWK value
   // 3. JWK alg valid AND input algorithm specified, but JWK value
   //      inconsistent with input: error
   // 4. JWK alg valid AND input algorithm specified, both consistent: use
   //      input value (because it has potentially more details)
@@ skipping 32 matching lines @@
       algorithm = algorithm_or_null;                     // case 4
     }
   } else {
     // JWK alg missing
     if (algorithm_or_null.isNull())
       return Status::ErrorJwkAlgorithmMissing();  // case 5
     algorithm = algorithm_or_null;                // case 6
   }
   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;
   status =
       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()
   if (jwk_kty_value == "oct") {
 
     std::string jwk_k_value;
     status = GetJwkBytes(dict_value, "k", &jwk_k_value);
     if (status.IsError())
       return status;
 
@@ skipping 44 matching lines @@
                                         CryptoData(jwk_e_value),
                                         key);
 
   } else {
     return Status::ErrorJwkUnrecognizedKty();
   }
 
   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