Refactor WebCrypto code

content/child/webcrypto/jwk.cc

Index: content/child/webcrypto/jwk.cc
diff --git a/content/child/webcrypto/jwk.cc b/content/child/webcrypto/jwk.cc
index a3d65da2051b8b6f96db322f1206280d7169f8c4..7141c3d6e5b79642f4326349542ce3785fbbaf0b 100644
--- a/content/child/webcrypto/jwk.cc
+++ b/content/child/webcrypto/jwk.cc
@@ -10,15 +10,10 @@
 
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
-#include "base/lazy_instance.h"
 #include "base/strings/string_piece.h"
-#include "base/strings/stringprintf.h"
 #include "content/child/webcrypto/crypto_data.h"
-#include "content/child/webcrypto/platform_crypto.h"
-#include "content/child/webcrypto/shared_crypto.h"
 #include "content/child/webcrypto/status.h"
 #include "content/child/webcrypto/webcrypto_util.h"
-#include "third_party/WebKit/public/platform/WebCryptoKeyAlgorithm.h"
 
 // JSON Web Key Format (JWK)
 // http://tools.ietf.org/html/draft-ietf-jose-json-web-key-21
@@ -209,22 +204,6 @@ namespace webcrypto {
 
 namespace {
 
-// Creates an RSASSA-PKCS1-v1_5 algorithm. It is an error to call this with a
-// hash_id that is not a SHA*.
-blink::WebCryptoAlgorithm CreateRsaSsaImportAlgorithm(
-    blink::WebCryptoAlgorithmId hash_id) {
-  return CreateRsaHashedImportAlgorithm(
-      blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5, hash_id);
-}
-
-// Creates an RSA-OAEP algorithm. It is an error to call this with a hash_id
-// that is not a SHA*.
-blink::WebCryptoAlgorithm CreateRsaOaepImportAlgorithm(
-    blink::WebCryptoAlgorithmId hash_id) {
-  return CreateRsaHashedImportAlgorithm(blink::WebCryptoAlgorithmIdRsaOaep,
-                                        hash_id);
-}
-
 // Web Crypto equivalent usage mask for JWK 'use' = 'enc'.
 const blink::WebCryptoKeyUsageMask kJwkEncUsage =
     blink::WebCryptoKeyUsageEncrypt | blink::WebCryptoKeyUsageDecrypt |
@@ -234,165 +213,39 @@ const blink::WebCryptoKeyUsageMask kJwkEncUsage =
 const blink::WebCryptoKeyUsageMask kJwkSigUsage =
     blink::WebCryptoKeyUsageSign | blink::WebCryptoKeyUsageVerify;
 
-typedef blink::WebCryptoAlgorithm (*AlgorithmCreationFunc)();
-
-class JwkAlgorithmInfo {
+class JwkWriter {
  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) {}
-
-  JwkAlgorithmInfo(AlgorithmCreationFunc algorithm_creation_func,
-                   unsigned int required_key_length_bits)
-      : creation_func_(algorithm_creation_func),
-        required_key_length_bytes_(required_key_length_bits / 8) {
-    DCHECK_EQ(0u, required_key_length_bits % 8);
+  JwkWriter(const std::string& algorithm,
+            bool extractable,
+            blink::WebCryptoKeyUsageMask usage_mask,
+            const std::string& kty) {
+    dict_.SetString("alg", algorithm);
+    dict_.Set("key_ops", CreateJwkKeyOpsFromWebCryptoUsages(usage_mask));
+    dict_.SetBoolean("ext", extractable);
+    dict_.SetString("kty", kty);
   }
 
-  bool CreateImportAlgorithm(blink::WebCryptoAlgorithm* algorithm) const {
-    *algorithm = creation_func_();
-    return !algorithm->isNull();
+  void Set(const std::string& key, const std::string& value) {
+    dict_.SetString(key, value);
   }
 
-  bool IsInvalidKeyByteLength(size_t byte_length) const {
-    if (required_key_length_bytes_ == NO_KEY_SIZE_REQUIREMENT)
-      return false;
-    return required_key_length_bytes_ != byte_length;
+  void SetBase64Encoded(const std::string& key, const CryptoData& value) {
+    dict_.SetString(key,
+                    Base64EncodeUrlSafe(base::StringPiece(
+                        reinterpret_cast<const char*>(value.bytes()),
+                        value.byte_length())));
   }
 
- private:
-  enum { NO_KEY_SIZE_REQUIREMENT = UINT_MAX };
-
-  AlgorithmCreationFunc creation_func_;
-
-  // The expected key size for the algorithm or NO_KEY_SIZE_REQUIREMENT.
-  unsigned int required_key_length_bytes_;
-};
-
-typedef std::map<std::string, JwkAlgorithmInfo> JwkAlgorithmInfoMap;
-
-class JwkAlgorithmRegistry {
- public:
-  JwkAlgorithmRegistry() {
-    // TODO(eroman):
-    // http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-20
-    // says HMAC with SHA-2 should have a key size at least as large as the
-    // hash output.
-    alg_to_info_["HS1"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateHmacImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha1>);
-    alg_to_info_["HS256"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateHmacImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha256>);
-    alg_to_info_["HS384"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateHmacImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha384>);
-    alg_to_info_["HS512"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateHmacImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha512>);
-    alg_to_info_["RS1"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaSsaImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha1>);
-    alg_to_info_["RS256"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaSsaImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha256>);
-    alg_to_info_["RS384"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaSsaImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha384>);
-    alg_to_info_["RS512"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaSsaImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha512>);
-    alg_to_info_["RSA-OAEP"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaOaepImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha1>);
-    alg_to_info_["RSA-OAEP-256"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaOaepImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha256>);
-    alg_to_info_["RSA-OAEP-384"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaOaepImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha384>);
-    alg_to_info_["RSA-OAEP-512"] =
-        JwkAlgorithmInfo(&BindAlgorithmId<CreateRsaOaepImportAlgorithm,
-                                          blink::WebCryptoAlgorithmIdSha512>);
-    alg_to_info_["A128KW"] = JwkAlgorithmInfo(
-        &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesKw>,
-        128);
-    alg_to_info_["A192KW"] = JwkAlgorithmInfo(
-        &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesKw>,
-        192);
-    alg_to_info_["A256KW"] = JwkAlgorithmInfo(
-        &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesKw>,
-        256);
-    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(
-        &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesCbc>,
-        256);
-  }
-
-  // Returns NULL if the algorithm name was not registered.
-  const JwkAlgorithmInfo* GetAlgorithmInfo(const std::string& jwk_alg) const {
-    const JwkAlgorithmInfoMap::const_iterator pos = alg_to_info_.find(jwk_alg);
-    if (pos == alg_to_info_.end())
-      return NULL;
-    return &pos->second;
+  void ToBytes(std::vector<uint8>* utf8_bytes) {
+    std::string json;
+    base::JSONWriter::Write(&dict_, &json);
+    utf8_bytes->assign(json.data(), json.data() + json.size());

[Ryan Sleevi, 2014/07/12 00:55:27]

Why don't you need to cast here?

json is std::string, meaning data is char*
utf8_bytes is vector<uint8>

If you're able to get away without casting, can you not use json.begin(),
json.end()?

[eroman, 2014/07/12 01:59:30]

Done.

   }
 
  private:
-  // Binds a WebCryptoAlgorithmId value to a compatible factory function.
-  typedef blink::WebCryptoAlgorithm (*FuncWithWebCryptoAlgIdArg)(
-      blink::WebCryptoAlgorithmId);
-  template <FuncWithWebCryptoAlgIdArg func,
-            blink::WebCryptoAlgorithmId algorithm_id>
-  static blink::WebCryptoAlgorithm BindAlgorithmId() {
-    return func(algorithm_id);
-  }
-
-  JwkAlgorithmInfoMap alg_to_info_;
+  base::DictionaryValue dict_;
 };
 
-base::LazyInstance<JwkAlgorithmRegistry> jwk_alg_registry =
-    LAZY_INSTANCE_INITIALIZER;
-
-bool ImportAlgorithmsConsistent(const blink::WebCryptoAlgorithm& alg1,
-                                const blink::WebCryptoAlgorithm& alg2) {
-  DCHECK(!alg1.isNull());
-  DCHECK(!alg2.isNull());
-  if (alg1.id() != alg2.id())
-    return false;
-  if (alg1.paramsType() != alg2.paramsType())
-    return false;
-  switch (alg1.paramsType()) {
-    case blink::WebCryptoAlgorithmParamsTypeNone:
-      return true;
-    case blink::WebCryptoAlgorithmParamsTypeRsaHashedImportParams:
-      return ImportAlgorithmsConsistent(alg1.rsaHashedImportParams()->hash(),
-                                        alg2.rsaHashedImportParams()->hash());
-    case blink::WebCryptoAlgorithmParamsTypeHmacImportParams:
-      return ImportAlgorithmsConsistent(alg1.hmacImportParams()->hash(),
-                                        alg2.hmacImportParams()->hash());
-    default:
-      return false;
-  }
-}
-
 // Extracts the required string property with key |path| from |dict| and saves
 // the result to |*result|. If the property does not exist or is not a string,
 // returns an error.
@@ -509,510 +362,379 @@ Status GetOptionalJwkBool(base::DictionaryValue* dict,
   return Status::Success();
 }
 
-// Writes a secret/symmetric key to a JWK dictionary.
-void WriteSecretKey(const std::vector<uint8>& raw_key,
-                    base::DictionaryValue* jwk_dict) {
-  DCHECK(jwk_dict);
-  jwk_dict->SetString("kty", "oct");
-  // For a secret/symmetric key, the only extra JWK field is 'k', containing the
-  // base64url encoding of the raw key.
-  const base::StringPiece key_str(
-      reinterpret_cast<const char*>(Uint8VectorStart(raw_key)), raw_key.size());
-  jwk_dict->SetString("k", Base64EncodeUrlSafe(key_str));
-}
-
-// Writes an RSA public key to a JWK dictionary
-void WriteRsaPublicKey(const std::vector<uint8>& modulus,
-                       const std::vector<uint8>& public_exponent,
-                       base::DictionaryValue* jwk_dict) {
-  DCHECK(jwk_dict);
-  DCHECK(modulus.size());
-  DCHECK(public_exponent.size());
-  jwk_dict->SetString("kty", "RSA");
-  jwk_dict->SetString("n", Base64EncodeUrlSafe(modulus));
-  jwk_dict->SetString("e", Base64EncodeUrlSafe(public_exponent));
+Status VerifyExt(base::DictionaryValue* dict, bool expected_extractable) {
+  // JWK "ext" (optional) --> extractable parameter
+  bool jwk_ext_value = false;
+  bool has_jwk_ext;
+  Status status = GetOptionalJwkBool(dict, "ext", &jwk_ext_value, &has_jwk_ext);
+  if (status.IsError())
+    return status;
+  if (has_jwk_ext && expected_extractable && !jwk_ext_value)
+    return Status::ErrorJwkExtInconsistent();
+  return Status::Success();
 }
 
-// Writes an RSA private key to a JWK dictionary
-Status ExportRsaPrivateKeyJwk(const blink::WebCryptoKey& key,
-                              base::DictionaryValue* jwk_dict) {
-  platform::PrivateKey* private_key;
-  Status status = ToPlatformPrivateKey(key, &private_key);
+Status VerifyUsages(base::DictionaryValue* dict,
+                    blink::WebCryptoKeyUsageMask expected_usage_mask) {
+  // JWK "key_ops" (optional) --> usage_mask parameter
+  base::ListValue* jwk_key_ops_value = NULL;
+  bool has_jwk_key_ops;
+  Status status =
+      GetOptionalJwkList(dict, "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 =
+        GetWebCryptoUsagesFromJwkKeyOps(jwk_key_ops_value, &jwk_key_ops_mask);
+    if (status.IsError())
+      return status;
+    // The input usage_mask must be a subset of jwk_key_ops_mask.
+    if (!ContainsKeyUsages(jwk_key_ops_mask, expected_usage_mask))
+      return Status::ErrorJwkKeyopsInconsistent();
+  }
 
-  // TODO(eroman): Copying the key properties to temporary vectors is
-  // inefficient. Once there aren't two implementations of platform_crypto this
-  // and other code will be easier to streamline.
-  std::vector<uint8> modulus;
-  std::vector<uint8> public_exponent;
-  std::vector<uint8> private_exponent;
-  std::vector<uint8> prime1;
-  std::vector<uint8> prime2;
-  std::vector<uint8> exponent1;
-  std::vector<uint8> exponent2;
-  std::vector<uint8> coefficient;
-
-  status = platform::ExportRsaPrivateKey(private_key,
-                                         &modulus,
-                                         &public_exponent,
-                                         &private_exponent,
-                                         &prime1,
-                                         &prime2,
-                                         &exponent1,
-                                         &exponent2,
-                                         &coefficient);
+  // JWK "use" (optional) --> usage_mask parameter
+  std::string jwk_use_value;
+  bool has_jwk_use;
+  status = GetOptionalJwkString(dict, "use", &jwk_use_value, &has_jwk_use);
   if (status.IsError())
     return status;
+  blink::WebCryptoKeyUsageMask jwk_use_mask = 0;
+  if (has_jwk_use) {
+    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 (!ContainsKeyUsages(jwk_use_mask, expected_usage_mask))
+      return Status::ErrorJwkUseInconsistent();
+  }
 
-  jwk_dict->SetString("kty", "RSA");
-  jwk_dict->SetString("n", Base64EncodeUrlSafe(modulus));
-  jwk_dict->SetString("e", Base64EncodeUrlSafe(public_exponent));
-  jwk_dict->SetString("d", Base64EncodeUrlSafe(private_exponent));
-  // Although these are "optional" in the JWA, WebCrypto spec requires them to
-  // be emitted.
-  jwk_dict->SetString("p", Base64EncodeUrlSafe(prime1));
-  jwk_dict->SetString("q", Base64EncodeUrlSafe(prime2));
-  jwk_dict->SetString("dp", Base64EncodeUrlSafe(exponent1));
-  jwk_dict->SetString("dq", Base64EncodeUrlSafe(exponent2));
-  jwk_dict->SetString("qi", Base64EncodeUrlSafe(coefficient));
+  // If both 'key_ops' and 'use' are present, ensure they are consistent.
+  if (has_jwk_key_ops && has_jwk_use &&
+      !ContainsKeyUsages(jwk_use_mask, jwk_key_ops_mask))
+    return Status::ErrorJwkUseAndKeyopsInconsistent();
 
   return Status::Success();
 }
 
-// Writes a Web Crypto usage mask to a JWK dictionary.
-void WriteKeyOps(blink::WebCryptoKeyUsageMask key_usages,
-                 base::DictionaryValue* jwk_dict) {
-  jwk_dict->Set("key_ops", CreateJwkKeyOpsFromWebCryptoUsages(key_usages));
-}
-
-// Writes a Web Crypto extractable value to a JWK dictionary.
-void WriteExt(bool extractable, base::DictionaryValue* jwk_dict) {
-  jwk_dict->SetBoolean("ext", extractable);
-}
-
-// Writes a Web Crypto algorithm to a JWK dictionary.
-Status WriteAlg(const blink::WebCryptoKeyAlgorithm& algorithm,
-                base::DictionaryValue* jwk_dict) {
-  switch (algorithm.paramsType()) {
-    case blink::WebCryptoKeyAlgorithmParamsTypeAes: {
-      DCHECK(algorithm.aesParams());
-      const char* aes_prefix = "";
-      switch (algorithm.aesParams()->lengthBits()) {
-        case 128:
-          aes_prefix = "A128";
-          break;
-        case 192:
-          aes_prefix = "A192";
-          break;
-        case 256:
-          aes_prefix = "A256";
-          break;
-        default:
-          NOTREACHED();  // bad key length means algorithm was built improperly
-          return Status::ErrorUnexpected();
-      }
-      const char* aes_suffix = "";
-      switch (algorithm.id()) {
-        case blink::WebCryptoAlgorithmIdAesCbc:
-          aes_suffix = "CBC";
-          break;
-        case blink::WebCryptoAlgorithmIdAesCtr:
-          aes_suffix = "CTR";
-          break;
-        case blink::WebCryptoAlgorithmIdAesGcm:
-          aes_suffix = "GCM";
-          break;
-        case blink::WebCryptoAlgorithmIdAesKw:
-          aes_suffix = "KW";
-          break;
-        default:
-          return Status::ErrorUnsupported();
-      }
-      jwk_dict->SetString("alg",
-                          base::StringPrintf("%s%s", aes_prefix, aes_suffix));
-      break;
-    }
-    case blink::WebCryptoKeyAlgorithmParamsTypeHmac: {
-      DCHECK(algorithm.hmacParams());
-      switch (algorithm.hmacParams()->hash().id()) {
-        case blink::WebCryptoAlgorithmIdSha1:
-          jwk_dict->SetString("alg", "HS1");
-          break;
-        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:
-          NOTREACHED();
-          return Status::ErrorUnexpected();
-      }
-      break;
+Status VerifyAlg(base::DictionaryValue* dict,
+                 const std::string& expected_algorithm,
+                 std::string* actual_algorithm) {
+  // JWK "alg" --> algorithm parameter
+  bool has_jwk_alg;
+  std::string jwk_alg_value;
+  Status status =
+      GetOptionalJwkString(dict, "alg", &jwk_alg_value, &has_jwk_alg);
+  if (status.IsError())
+    return status;
+  if (actual_algorithm) {
+    if (has_jwk_alg) {
+      if (jwk_alg_value.empty())
+        return Status::ErrorJwkAlgorithmInconsistent();

[Ryan Sleevi, 2014/07/12 00:55:27]

Why do you place this check here, but then you return an std::string on line
440, suggesting the caller still needs to check if empty(), meaning no alg
present?

[eroman, 2014/07/12 01:59:30]

Thanks for pointing this out, the current structure is in fact quite difficult
to read.

Basically I did witchcraft which allows calling VerifyAlg() in one of two modes:

 (1) (expected_algorithm="", actual_algorithm=XXXX)
 (2) (expected_algorithm=<value>, actual_algorithm=NULL)

Mode 2 means that if "alg" is present in the JWK, it must match
expected_algorithm, otherwise fail with inconsistent algorithm. This is the
simple case.

Mode 1 is used for AES algorithms, and means "get the value of alg". If an "alg"
was present, it is written back to actual_algorithm. If not actual_algorithm is
set to "". If "alg" actually was the empty string, then an error is returned, to
avoid it being interpreted as no algorithm.

I will split these up into separate functions, so there is no need to explain in
detail how the parameters work.

[eroman, 2014/07/14 21:56:10]

Done.

+      *actual_algorithm = jwk_alg_value;
+    } else {
+      *actual_algorithm = std::string();
     }
-    case blink::WebCryptoKeyAlgorithmParamsTypeRsaHashed:
-      switch (algorithm.id()) {
-        case blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5: {
-          switch (algorithm.rsaHashedParams()->hash().id()) {
-            case blink::WebCryptoAlgorithmIdSha1:
-              jwk_dict->SetString("alg", "RS1");
-              break;
-            case blink::WebCryptoAlgorithmIdSha256:
-              jwk_dict->SetString("alg", "RS256");
-              break;
-            case blink::WebCryptoAlgorithmIdSha384:
-              jwk_dict->SetString("alg", "RS384");
-              break;
-            case blink::WebCryptoAlgorithmIdSha512:
-              jwk_dict->SetString("alg", "RS512");
-              break;
-            default:
-              NOTREACHED();
-              return Status::ErrorUnexpected();
-          }
-          break;
-        }
-        case blink::WebCryptoAlgorithmIdRsaOaep: {
-          switch (algorithm.rsaHashedParams()->hash().id()) {
-            case blink::WebCryptoAlgorithmIdSha1:
-              jwk_dict->SetString("alg", "RSA-OAEP");
-              break;
-            case blink::WebCryptoAlgorithmIdSha256:
-              jwk_dict->SetString("alg", "RSA-OAEP-256");
-              break;
-            case blink::WebCryptoAlgorithmIdSha384:
-              jwk_dict->SetString("alg", "RSA-OAEP-384");
-              break;
-            case blink::WebCryptoAlgorithmIdSha512:
-              jwk_dict->SetString("alg", "RSA-OAEP-512");
-              break;
-            default:
-              NOTREACHED();
-              return Status::ErrorUnexpected();
-          }
-          break;
-        }
-        default:
-          NOTREACHED();
-          return Status::ErrorUnexpected();
-      }
-      break;
-    default:
-      return Status::ErrorUnsupported();
   }
+
+  if (has_jwk_alg && !expected_algorithm.empty() &&
+      jwk_alg_value != expected_algorithm) {
+    return Status::ErrorJwkAlgorithmInconsistent();
+  }
+
   return Status::Success();
 }
 
-bool IsRsaKey(const blink::WebCryptoKey& key) {
-  return IsAlgorithmRsa(key.algorithm().id());
-}
+Status ParseJwkCommon(const CryptoData& bytes,
+                      const std::string& expected_algorithm,
+                      bool expected_extractable,
+                      blink::WebCryptoKeyUsageMask expected_usage_mask,
+                      std::string* kty,
+                      std::string* actual_algorithm,
+                      scoped_ptr<base::DictionaryValue>* dict) {
+  // Parse the incoming JWK JSON.
+  base::StringPiece json_string(reinterpret_cast<const char*>(bytes.bytes()),
+                                bytes.byte_length());
 
-Status ImportRsaKey(base::DictionaryValue* dict,
-                    const blink::WebCryptoAlgorithm& algorithm,
-                    bool extractable,
-                    blink::WebCryptoKeyUsageMask usage_mask,
-                    blink::WebCryptoKey* key) {
-  // An RSA public key must have an "n" (modulus) and an "e" (exponent) entry
-  // in the JWK, while an RSA private key must have those, plus at least a "d"
-  // (private exponent) entry.
-  // See http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-18,
-  // section 6.3.
-  std::string jwk_n_value;
-  Status status = GetJwkBytes(dict, "n", &jwk_n_value);
-  if (status.IsError())
-    return status;
-  std::string jwk_e_value;
-  status = GetJwkBytes(dict, "e", &jwk_e_value);
-  if (status.IsError())
-    return status;
+  scoped_ptr<base::Value> value(base::JSONReader::Read(json_string));
+  base::DictionaryValue* dict_value = NULL;
 
-  bool is_public_key = !dict->HasKey("d");
+  if (!value.get() || !value->GetAsDictionary(&dict_value) || !dict_value)
+    return Status::ErrorJwkNotDictionary();
 
-  // Now that the key type is known, do an additional check on the usages to
-  // make sure they are all applicable for this algorithm + key type.
-  status = CheckKeyUsages(algorithm.id(),
-                          is_public_key ? blink::WebCryptoKeyTypePublic
-                                        : blink::WebCryptoKeyTypePrivate,
-                          usage_mask);
+  // Transfer to caller.
+  ignore_result(value.release());

[Ryan Sleevi, 2014/07/12 00:55:27]

This needs more documentation, because I had serious read flags and had to check
the API contracts here, because it "looked" like a UAF

// Release |value|, as ownership will be transferred to |dict| via
// |dict_value|, which points to the same object as |value|.

[eroman, 2014/07/12 01:59:30]

Done.

+  dict->reset(dict_value);
 
+  // JWK "kty". Exit early if this required JWK parameter is missing.
+  Status status = GetJwkString(dict_value, "kty", kty);
   if (status.IsError())
     return status;
 
-  if (is_public_key) {
-    return platform::ImportRsaPublicKey(algorithm,
-                                        extractable,
-                                        usage_mask,
-                                        CryptoData(jwk_n_value),
-                                        CryptoData(jwk_e_value),
-                                        key);
-  }
-
-  std::string jwk_d_value;
-  status = GetJwkBytes(dict, "d", &jwk_d_value);
+  status = VerifyAlg(dict_value, expected_algorithm, actual_algorithm);
   if (status.IsError())
     return status;
 
-  // The "p", "q", "dp", "dq", and "qi" properties are optional. Treat these
-  // properties the same if they are unspecified, as if they were specified-but
-  // empty, since ImportRsaPrivateKey() doesn't do validation checks anyway.
-
-  std::string jwk_p_value;
-  bool has_p;
-  status = GetOptionalJwkBytes(dict, "p", &jwk_p_value, &has_p);
+  status = VerifyExt(dict_value, expected_extractable);
   if (status.IsError())
     return status;
 
-  std::string jwk_q_value;
-  bool has_q;
-  status = GetOptionalJwkBytes(dict, "q", &jwk_q_value, &has_q);
+  status = VerifyUsages(dict_value, expected_usage_mask);
   if (status.IsError())
     return status;
 
-  std::string jwk_dp_value;
-  bool has_dp;
-  status = GetOptionalJwkBytes(dict, "dp", &jwk_dp_value, &has_dp);
-  if (status.IsError())
-    return status;
+  return Status::Success();
+}
 
-  std::string jwk_dq_value;
-  bool has_dq;
-  status = GetOptionalJwkBytes(dict, "dq", &jwk_dq_value, &has_dq);
+Status ReadSecretKeyJwkHelper(const CryptoData& key_data,
+                              const std::string& expected_algorithm,
+                              bool expected_extractable,
+                              blink::WebCryptoKeyUsageMask expected_usage_mask,
+                              std::string* actual_algorithm,
+                              std::vector<uint8>* raw_key_data) {
+  if (!key_data.byte_length())
+    return Status::ErrorImportEmptyKeyData();
+
+  scoped_ptr<base::DictionaryValue> dict;
+  std::string kty;
+  Status status = ParseJwkCommon(key_data,
+                                 expected_algorithm,
+                                 expected_extractable,
+                                 expected_usage_mask,
+                                 &kty,
+                                 actual_algorithm,
+                                 &dict);
   if (status.IsError())
     return status;
 
-  std::string jwk_qi_value;
-  bool has_qi;
-  status = GetOptionalJwkBytes(dict, "qi", &jwk_qi_value, &has_qi);
+  if (kty != "oct")
+    return Status::ErrorJwkUnexpectedKty("oct");
+
+  std::string jwk_k_value;
+  status = GetJwkBytes(dict.get(), "k", &jwk_k_value);
   if (status.IsError())
     return status;
+  raw_key_data->assign(jwk_k_value.begin(), jwk_k_value.end());
 
-  int num_optional_properties = has_p + has_q + has_dp + has_dq + has_qi;
-  if (num_optional_properties != 0 && num_optional_properties != 5)
-    return Status::ErrorJwkIncompleteOptionalRsaPrivateKey();
-
-  return platform::ImportRsaPrivateKey(
-      algorithm,
-      extractable,
-      usage_mask,
-      CryptoData(jwk_n_value),   // modulus
-      CryptoData(jwk_e_value),   // public_exponent
-      CryptoData(jwk_d_value),   // private_exponent
-      CryptoData(jwk_p_value),   // prime1
-      CryptoData(jwk_q_value),   // prime2
-      CryptoData(jwk_dp_value),  // exponent1
-      CryptoData(jwk_dq_value),  // exponent2
-      CryptoData(jwk_qi_value),  // coefficient
-      key);
+  return Status::Success();
 }
 
 }  // namespace
 
-// TODO(eroman): Split this up into smaller functions.
-Status ImportKeyJwk(const CryptoData& key_data,
-                    const blink::WebCryptoAlgorithm& algorithm,
-                    bool extractable,
-                    blink::WebCryptoKeyUsageMask usage_mask,
-                    blink::WebCryptoKey* key) {
-  if (!key_data.byte_length())
-    return Status::ErrorImportEmptyKeyData();
-  DCHECK(key);
+void WriteSecretKeyJwk(const CryptoData& raw_key_data,
+                       const std::string& algorithm,
+                       bool extractable,
+                       blink::WebCryptoKeyUsageMask usage_mask,
+                       std::vector<uint8>* jwk_key_data) {
+  JwkWriter writer(algorithm, extractable, usage_mask, "oct");
+  writer.SetBase64Encoded("k", raw_key_data);
+  writer.ToBytes(jwk_key_data);
+}
 
-  // 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();
+Status ReadSecretKeyJwk(const CryptoData& key_data,
+                        const std::string& expected_algorithm,
+                        bool expected_extractable,
+                        blink::WebCryptoKeyUsageMask expected_usage_mask,
+                        std::vector<uint8>* raw_key_data) {
+  return ReadSecretKeyJwkHelper(key_data,
+                                expected_algorithm,
+                                expected_extractable,
+                                expected_usage_mask,
+                                NULL,
+                                raw_key_data);
+}
 
-  // 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);
+std::string MakeJwkAesAlgorithmName(const std::string& suffix,
+                                    unsigned int keylen_bytes) {
+  if (keylen_bytes == 16)
+    return std::string("A128") + suffix;
+  if (keylen_bytes == 24)
+    return std::string("A192") + suffix;
+  if (keylen_bytes == 32)
+    return std::string("A256") + suffix;
+  return std::string();
+}
+
+Status ReadAesSecretKeyJwk(const CryptoData& key_data,
+                           const std::string& algorithm_name_suffix,
+                           bool expected_extractable,
+                           blink::WebCryptoKeyUsageMask expected_usage_mask,
+                           std::vector<uint8>* raw_key_data) {
+  std::string jwk_algorithm_name;
+  Status status = ReadSecretKeyJwkHelper(key_data,
+                                         "",

[Ryan Sleevi, 2014/07/12 00:55:27]

std::string()

+                                         expected_extractable,
+                                         expected_usage_mask,
+                                         &jwk_algorithm_name,
+                                         raw_key_data);
   if (status.IsError())
     return status;
 
-  // JWK "ext" (optional) --> extractable parameter
-  {
-    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_ext && !jwk_ext_value && extractable)
-      return Status::ErrorJwkExtInconsistent();
+  std::string expected_algorithm_name =
+      MakeJwkAesAlgorithmName(algorithm_name_suffix, raw_key_data->size());
+
+  if (!jwk_algorithm_name.empty() &&
+      jwk_algorithm_name != expected_algorithm_name) {
+    // Give a different error message if the key length was wrong.
+    if (jwk_algorithm_name ==
+            MakeJwkAesAlgorithmName(algorithm_name_suffix, 16) ||
+        jwk_algorithm_name ==
+            MakeJwkAesAlgorithmName(algorithm_name_suffix, 24) ||
+        jwk_algorithm_name ==
+            MakeJwkAesAlgorithmName(algorithm_name_suffix, 32)) {

[Ryan Sleevi, 2014/07/12 00:55:27]

Doesn't this seem like it should be up to the implementation to determine
whether these key sizes are valid?

That is, you seem to be trying to keep knowledge about the set of valid
algorithm names out of this, but then have this?

[eroman, 2014/07/12 01:59:30]

Indeed. Initially I had the code as part of the implementation, and had jwk.cc
simply return the value of "alg":

https://codereview.chromium.org/379383002/diff/160001/content/child/webcrypto...

Once I added the OpenSSL code I found myself duplicating that same code. So I
moved the code to a common location so it could be shared between openssl/nss,
and I figured that location should be jwk.cc

I would like both the RSA JWK parsing, and AES JWK parsing to be part of the
AlgorithmImplementation code once the NSS code is deleted, perhaps a TODO in the
meantime?

+      return Status::ErrorJwkIncorrectKeyLength();
+    }
+    return Status::ErrorJwkAlgorithmInconsistent();
   }
 
-  // JWK "alg" --> algorithm parameter
-  // 1. JWK alg present but unrecognized: error
-  // 2. JWK alg valid and inconsistent with input algorithm: error
-  // 3. JWK alg valid and consistent with input algorithm: use input value
-  // 4. JWK alg is missing: use input value
-  const JwkAlgorithmInfo* algorithm_info = NULL;
-  std::string jwk_alg_value;
-  bool has_jwk_alg;
-  status =
-      GetOptionalJwkString(dict_value, "alg", &jwk_alg_value, &has_jwk_alg);
-  if (status.IsError())
-    return status;
+  return Status::Success();
+}
+
+// Writes an RSA public key to a JWK dictionary
+void WriteRsaPublicKeyJwk(const CryptoData& n,
+                          const CryptoData& e,
+                          const std::string& algorithm,
+                          bool extractable,
+                          blink::WebCryptoKeyUsageMask usage_mask,
+                          std::vector<uint8>* jwk_key_data) {
+  JwkWriter writer(algorithm, extractable, usage_mask, "RSA");
+  writer.SetBase64Encoded("n", n);
+  writer.SetBase64Encoded("e", e);
+  writer.ToBytes(jwk_key_data);
+}
 
-  if (has_jwk_alg) {
-    // JWK alg present
+// Writes an RSA private key to a JWK dictionary
+void WriteRsaPrivateKeyJwk(const CryptoData& n,
+                           const CryptoData& e,
+                           const CryptoData& d,
+                           const CryptoData& p,
+                           const CryptoData& q,
+                           const CryptoData& dp,
+                           const CryptoData& dq,
+                           const CryptoData& qi,
+                           const std::string& algorithm,
+                           bool extractable,
+                           blink::WebCryptoKeyUsageMask usage_mask,
+                           std::vector<uint8>* jwk_key_data) {
+  JwkWriter writer(algorithm, extractable, usage_mask, "RSA");
+
+  writer.SetBase64Encoded("n", n);
+  writer.SetBase64Encoded("e", e);
+  writer.SetBase64Encoded("d", d);
+  // Although these are "optional" in the JWA, WebCrypto spec requires them to
+  // be emitted.
+  writer.SetBase64Encoded("p", p);
+  writer.SetBase64Encoded("q", q);
+  writer.SetBase64Encoded("dp", dp);
+  writer.SetBase64Encoded("dq", dq);
+  writer.SetBase64Encoded("qi", qi);
+  writer.ToBytes(jwk_key_data);
+}
 
-    // TODO(padolph): Validate alg vs kty. For example kty="RSA" implies alg can
-    // only be from the RSA family.
+JwkRsaInfo::JwkRsaInfo() : is_private_key(false) {
+}
 
-    blink::WebCryptoAlgorithm jwk_algorithm =
-        blink::WebCryptoAlgorithm::createNull();
-    algorithm_info = jwk_alg_registry.Get().GetAlgorithmInfo(jwk_alg_value);
-    if (!algorithm_info ||
-        !algorithm_info->CreateImportAlgorithm(&jwk_algorithm))
-      return Status::ErrorJwkUnrecognizedAlgorithm();
+JwkRsaInfo::~JwkRsaInfo() {
+}
 
-    if (!ImportAlgorithmsConsistent(jwk_algorithm, algorithm))
-      return Status::ErrorJwkAlgorithmInconsistent();
-  }
-  DCHECK(!algorithm.isNull());
+Status ReadRsaKeyJwk(const CryptoData& key_data,
+                     const std::string& expected_algorithm,
+                     bool expected_extractable,
+                     blink::WebCryptoKeyUsageMask expected_usage_mask,
+                     JwkRsaInfo* result) {
+  if (!key_data.byte_length())
+    return Status::ErrorImportEmptyKeyData();
 
-  // 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);
+  scoped_ptr<base::DictionaryValue> dict;
+  std::string kty;
+  Status status = ParseJwkCommon(key_data,
+                                 expected_algorithm,
+                                 expected_extractable,
+                                 expected_usage_mask,
+                                 &kty,
+                                 NULL,
+                                 &dict);
   if (status.IsError())
     return status;
-  blink::WebCryptoKeyUsageMask jwk_key_ops_mask = 0;
-  if (has_jwk_key_ops) {
-    status =
-        GetWebCryptoUsagesFromJwkKeyOps(jwk_key_ops_value, &jwk_key_ops_mask);
-    if (status.IsError())
-      return status;
-    // The input usage_mask must be a subset of jwk_key_ops_mask.
-    if (!ContainsKeyUsages(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 (kty != "RSA")
+    return Status::ErrorJwkUnexpectedKty("RSA");
+
+  // An RSA public key must have an "n" (modulus) and an "e" (exponent) entry
+  // in the JWK, while an RSA private key must have those, plus at least a "d"
+  // (private exponent) entry.
+  // See http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-18,
+  // section 6.3.
+  status = GetJwkBytes(dict.get(), "n", &result->n);
+  if (status.IsError())
+    return status;
+  status = GetJwkBytes(dict.get(), "e", &result->e);
   if (status.IsError())
     return status;
-  blink::WebCryptoKeyUsageMask jwk_use_mask = 0;
-  if (has_jwk_use) {
-    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 (!ContainsKeyUsages(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 &&
-      !ContainsKeyUsages(jwk_use_mask, 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;
+  result->is_private_key = dict->HasKey("d");
+  if (!result->is_private_key)
+    return Status::Success();
 
-    // Some JWK alg ID's embed information about the key length in the alg ID
-    // string. For example "A128CBC" implies the JWK carries 128 bits
-    // of key material. For such keys validate that enough bytes were provided.
-    // If this validation is not done, then it would be possible to select a
-    // different algorithm by passing a different lengthed key, since that is
-    // how WebCrypto interprets things.
-    if (algorithm_info &&
-        algorithm_info->IsInvalidKeyByteLength(jwk_k_value.size())) {
-      return Status::ErrorJwkIncorrectKeyLength();
-    }
+  status = GetJwkBytes(dict.get(), "d", &result->d);
+  if (status.IsError())
+    return status;
 
-    return ImportKey(blink::WebCryptoKeyFormatRaw,
-                     CryptoData(jwk_k_value),
-                     algorithm,
-                     extractable,
-                     usage_mask,
-                     key);
-  }
+  // The "p", "q", "dp", "dq", and "qi" properties are optional. Treat these
+  // properties the same if they are unspecified, as if they were specified-but
+  // empty, since ImportRsaPrivateKey() doesn't do validation checks anyway.
 
-  if (jwk_kty_value == "RSA")
-    return ImportRsaKey(dict_value, algorithm, extractable, usage_mask, key);
+  bool has_p;
+  status = GetOptionalJwkBytes(dict.get(), "p", &result->p, &has_p);
+  if (status.IsError())
+    return status;
 
-  return Status::ErrorJwkUnrecognizedKty();
-}
+  bool has_q;
+  status = GetOptionalJwkBytes(dict.get(), "q", &result->q, &has_q);
+  if (status.IsError())
+    return status;
 
-Status ExportKeyJwk(const blink::WebCryptoKey& key,
-                    std::vector<uint8>* buffer) {
-  DCHECK(key.extractable());
-  base::DictionaryValue jwk_dict;
-  Status status = Status::OperationError();
-
-  switch (key.type()) {
-    case blink::WebCryptoKeyTypeSecret: {
-      std::vector<uint8> exported_key;
-      status = ExportKey(blink::WebCryptoKeyFormatRaw, key, &exported_key);
-      if (status.IsError())
-        return status;
-      WriteSecretKey(exported_key, &jwk_dict);
-      break;
-    }
-    case blink::WebCryptoKeyTypePublic: {
-      // TODO(eroman): Update when there are asymmetric keys other than RSA.
-      if (!IsRsaKey(key))
-        return Status::ErrorUnsupported();
-      platform::PublicKey* public_key;
-      status = ToPlatformPublicKey(key, &public_key);
-      if (status.IsError())
-        return status;
-      std::vector<uint8> modulus;
-      std::vector<uint8> public_exponent;
-      status =
-          platform::ExportRsaPublicKey(public_key, &modulus, &public_exponent);
-      if (status.IsError())
-        return status;
-      WriteRsaPublicKey(modulus, public_exponent, &jwk_dict);
-      break;
-    }
-    case blink::WebCryptoKeyTypePrivate: {
-      // TODO(eroman): Update when there are asymmetric keys other than RSA.
-      if (!IsRsaKey(key))
-        return Status::ErrorUnsupported();
-
-      status = ExportRsaPrivateKeyJwk(key, &jwk_dict);
-      if (status.IsError())
-        return status;
-      break;
-    }
+  bool has_dp;
+  status = GetOptionalJwkBytes(dict.get(), "dp", &result->dp, &has_dp);
+  if (status.IsError())
+    return status;
 
-    default:
-      return Status::ErrorUnsupported();
-  }
+  bool has_dq;
+  status = GetOptionalJwkBytes(dict.get(), "dq", &result->dq, &has_dq);
+  if (status.IsError())
+    return status;
 
-  WriteKeyOps(key.usages(), &jwk_dict);
-  WriteExt(key.extractable(), &jwk_dict);
-  status = WriteAlg(key.algorithm(), &jwk_dict);
+  bool has_qi;
+  status = GetOptionalJwkBytes(dict.get(), "qi", &result->qi, &has_qi);
   if (status.IsError())
     return status;
 
-  std::string json;
-  base::JSONWriter::Write(&jwk_dict, &json);
-  buffer->assign(json.data(), json.data() + json.size());
+  int num_optional_properties = has_p + has_q + has_dp + has_dq + has_qi;
+  if (num_optional_properties != 0 && num_optional_properties != 5)
+    return Status::ErrorJwkIncompleteOptionalRsaPrivateKey();
+
   return Status::Success();
 }
 
+const char* GetJwkHmacAlgorithmName(blink::WebCryptoAlgorithmId hash) {
+  switch (hash) {
+    case blink::WebCryptoAlgorithmIdSha1:
+      return "HS1";
+    case blink::WebCryptoAlgorithmIdSha256:
+      return "HS256";
+    case blink::WebCryptoAlgorithmIdSha384:
+      return "HS384";
+    case blink::WebCryptoAlgorithmIdSha512:
+      return "HS512";
+    default:
+      return NULL;
+  }
+}
+
 }  // namespace webcrypto
 
 }  // namespace content