[webcrypto] Add error messages for failed operations.

content/renderer/webcrypto/webcrypto_impl.cc

 // Copyright (c) 2013 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 "content/renderer/webcrypto/webcrypto_impl.h"
 
 #include <algorithm>
 #include <functional>
 #include <map>
 #include "base/json/json_reader.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 "content/renderer/webcrypto/webcrypto_util.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
 #include "third_party/WebKit/public/platform/WebCryptoKey.h"
+#include "third_party/WebKit/public/platform/WebString.h"
 
 namespace content {
 
+using webcrypto::Status;

[Ryan Sleevi, 2014/01/24 01:21:50]

I thought this was discouraged in Chromium side.

[eroman, 2014/01/24 03:19:21]

The practice is quite common in Chrome code, occurring nearly 13,000 times in
Chromium code:

$ cd src
$ git grep '^using ' | wc -l
12824

I am unaware of any decrees forbidding it.

[Ryan Sleevi, 2014/01/24 20:27:01]

Thread:

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/mn5OV...

Summary:

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/SGDZG...

[eroman, 2014/01/24 21:33:10]

Thanks for the link to the thread!

I'll give it a more detailed look after lunch, but it doesn't seem there was any
real consensus on that thread, and the style guide was never amended.

At any rate, I have uploaded a patchset with the prefix for comparison to see if
you prefer it. My take is the extra verbosity of webcrypto:: inside of a
WebCryptoImpl class is pretty weird.

The summary on that thread leaves the decision up to the author's judgement.

+
 namespace {
 
+// Complete |result| with a failure if |status| indicates an error and return
+// true. Otherwise return false.
+bool completeWithError(const Status& status, blink::WebCryptoResult* result) {

[Ryan Sleevi, 2014/01/24 01:21:50]

naming: I find it counter-intuitive that completeWithError takes a negative
result (an error), and will return true if it was an error, because the phrase
"complete with error" suggests "perform this task", not "is this an error"

if (AbortOnErrorStatus(status, &result))
  return;

style: completeWithError -> CompleteWithError

[eroman, 2014/01/24 03:19:21]

How about CompletedWithError() ?

[Ryan Sleevi, 2014/01/24 20:27:01]

I still find that a bit confusing. ShouldReturnWithError() ?

[eroman, 2014/01/25 03:26:08]

Seeing how this was confusing, I removed the function all together, and replaced
it with something a bit more verbose but easier to understand.

+  if (status.IsError()) {
+    result->completeWithError(blink::WebString::fromUTF8(status.ToString()));
+    return true;
+  }
+  return false;
+}
+
 bool IsAlgorithmAsymmetric(const blink::WebCryptoAlgorithm& 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);
 }
 
 // Binds a specific key length value to a compatible factory function.
 typedef blink::WebCryptoAlgorithm (*AlgFactoryFuncWithOneShortArg)(
@@ skipping 133 matching lines @@
 }
 
 void WebCryptoImpl::encrypt(
     const blink::WebCryptoAlgorithm& algorithm,
     const blink::WebCryptoKey& key,
     const unsigned char* data,
     unsigned data_size,
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   blink::WebArrayBuffer buffer;
-  if (!EncryptInternal(algorithm, key, data, data_size, &buffer)) {
-    result.completeWithError();
-  } else {
+  Status status = EncryptInternal(algorithm, key, data, data_size, &buffer);
+  if (!completeWithError(status, &result)) {
     result.completeWithBuffer(buffer);
   }
 }
 
 void WebCryptoImpl::decrypt(
     const blink::WebCryptoAlgorithm& algorithm,
     const blink::WebCryptoKey& key,
     const unsigned char* data,
     unsigned data_size,
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   blink::WebArrayBuffer buffer;
-  if (!DecryptInternal(algorithm, key, data, data_size, &buffer)) {
-    result.completeWithError();
-  } else {
+  Status status = DecryptInternal(algorithm, key, data, data_size, &buffer);
+  if (!completeWithError(status, &result)) {
     result.completeWithBuffer(buffer);
   }
 }
 
 void WebCryptoImpl::digest(
     const blink::WebCryptoAlgorithm& algorithm,
     const unsigned char* data,
     unsigned data_size,
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   blink::WebArrayBuffer buffer;
-  if (!DigestInternal(algorithm, data, data_size, &buffer)) {
-    result.completeWithError();
-  } else {
+  Status status = DigestInternal(algorithm, data, data_size, &buffer);
+  if (!completeWithError(status, &result)) {
     result.completeWithBuffer(buffer);
   }
 }
 
 void WebCryptoImpl::generateKey(
     const blink::WebCryptoAlgorithm& algorithm,
     bool extractable,
     blink::WebCryptoKeyUsageMask usage_mask,
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   if (IsAlgorithmAsymmetric(algorithm)) {
     blink::WebCryptoKey public_key = blink::WebCryptoKey::createNull();
     blink::WebCryptoKey private_key = blink::WebCryptoKey::createNull();
-    if (!GenerateKeyPairInternal(
-             algorithm, extractable, usage_mask, &public_key, &private_key)) {
-      result.completeWithError();
-    } else {
+    Status status = GenerateKeyPairInternal(
+        algorithm, extractable, usage_mask, &public_key, &private_key);
+    if (!completeWithError(status, &result)) {
       DCHECK(public_key.handle());
       DCHECK(private_key.handle());
       DCHECK_EQ(algorithm.id(), public_key.algorithm().id());
       DCHECK_EQ(algorithm.id(), private_key.algorithm().id());
       DCHECK_EQ(true, public_key.extractable());
       DCHECK_EQ(extractable, private_key.extractable());
       DCHECK_EQ(usage_mask, public_key.usages());
       DCHECK_EQ(usage_mask, private_key.usages());
       result.completeWithKeyPair(public_key, private_key);
     }
   } else {
     blink::WebCryptoKey key = blink::WebCryptoKey::createNull();
-    if (!GenerateKeyInternal(algorithm, extractable, usage_mask, &key)) {
-      result.completeWithError();
-    } else {
+    Status status = GenerateKeyInternal(
+        algorithm, extractable, usage_mask, &key);
+    if (!completeWithError(status, &result)) {
       DCHECK(key.handle());
       DCHECK_EQ(algorithm.id(), key.algorithm().id());
       DCHECK_EQ(extractable, key.extractable());
       DCHECK_EQ(usage_mask, key.usages());
       result.completeWithKey(key);
     }
   }
 }
 
 void WebCryptoImpl::importKey(
     blink::WebCryptoKeyFormat format,
     const unsigned char* key_data,
     unsigned key_data_size,
     const blink::WebCryptoAlgorithm& algorithm_or_null,
     bool extractable,
     blink::WebCryptoKeyUsageMask usage_mask,
     blink::WebCryptoResult result) {
   blink::WebCryptoKey key = blink::WebCryptoKey::createNull();
   if (format == blink::WebCryptoKeyFormatJwk) {
-    if (!ImportKeyJwk(key_data,
-                      key_data_size,
-                      algorithm_or_null,
-                      extractable,
-                      usage_mask,
-                      &key)) {
-      result.completeWithError();
+    Status status = ImportKeyJwk(key_data,
+                                 key_data_size,
+                                 algorithm_or_null,
+                                 extractable,
+                                 usage_mask,
+                                 &key);
+    if (completeWithError(status, &result)) {
       return;
     }
   } else {
-    if (!ImportKeyInternal(format,
-                           key_data,
-                           key_data_size,
-                           algorithm_or_null,
-                           extractable,
-                           usage_mask,
-                           &key)) {
-      result.completeWithError();
+    Status status = ImportKeyInternal(format,
+                                      key_data,
+                                      key_data_size,
+                                      algorithm_or_null,
+                                      extractable,
+                                      usage_mask,
+                                      &key);
+    if (completeWithError(status, &result)) {
       return;
     }
   }
   DCHECK(key.handle());
   DCHECK(!key.algorithm().isNull());
   DCHECK_EQ(extractable, key.extractable());
   result.completeWithKey(key);
 }
 
 void WebCryptoImpl::exportKey(
     blink::WebCryptoKeyFormat format,
     const blink::WebCryptoKey& key,
     blink::WebCryptoResult result) {
   blink::WebArrayBuffer buffer;
-  if (!ExportKeyInternal(format, key, &buffer)) {
-    result.completeWithError();
-    return;
+  Status status = ExportKeyInternal(format, key, &buffer);
+  if (!completeWithError(status, &result)) {
+    result.completeWithBuffer(buffer);
   }
-  result.completeWithBuffer(buffer);
 }
 
 void WebCryptoImpl::sign(
     const blink::WebCryptoAlgorithm& algorithm,
     const blink::WebCryptoKey& key,
     const unsigned char* data,
     unsigned data_size,
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   blink::WebArrayBuffer buffer;
-  if (!SignInternal(algorithm, key, data, data_size, &buffer)) {
-    result.completeWithError();
-  } else {
+  Status status = SignInternal(algorithm, key, data, data_size, &buffer);
+  if (!completeWithError(status, &result)) {
     result.completeWithBuffer(buffer);
   }
 }
 
 void WebCryptoImpl::verifySignature(
     const blink::WebCryptoAlgorithm& algorithm,
     const blink::WebCryptoKey& key,
     const unsigned char* signature,
     unsigned signature_size,
     const unsigned char* data,
     unsigned data_size,
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   bool signature_match = false;
-  if (!VerifySignatureInternal(algorithm,
-                               key,
-                               signature,
-                               signature_size,
-                               data,
-                               data_size,
-                               &signature_match)) {
-    result.completeWithError();
-  } else {
+  Status status = VerifySignatureInternal(algorithm,
+                                          key,
+                                          signature,
+                                          signature_size,
+                                          data,
+                                          data_size,
+                                          &signature_match);
+  if (!completeWithError(status, &result)) {
     result.completeWithBoolean(signature_match);
   }
 }
 
-bool WebCryptoImpl::ImportKeyJwk(
+Status WebCryptoImpl::ImportKeyJwk(
     const unsigned char* key_data,
     unsigned key_data_size,
     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.
@@ skipping 123 matching lines @@
   //   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_size)
-    return false;
+    return Status::ErrorEmptyKeyData();
   DCHECK(key);
 
   // Parse the incoming JWK JSON.
   base::StringPiece json_string(reinterpret_cast<const char*>(key_data),
                                 key_data_size);
   scoped_ptr<base::Value> value(base::JSONReader::Read(json_string));
   // Note, bare pointer dict_value is ok since it points into scoped value.
   base::DictionaryValue* dict_value = NULL;
   if (!value.get() || !value->GetAsDictionary(&dict_value) || !dict_value)
-    return false;
+    return Status::ErrorJwkNotDictionary();
 
   // JWK "kty". Exit early if this required JWK parameter is missing.
   std::string jwk_kty_value;
   if (!dict_value->GetString("kty", &jwk_kty_value))
-    return false;
+    return Status::ErrorJwkMissingKty();
 
   // JWK "extractable" (optional) --> extractable parameter
   {
     bool jwk_extractable_value;
     if (dict_value->GetBoolean("extractable", &jwk_extractable_value)) {
       if (!jwk_extractable_value && extractable)
-        return false;
+        return Status::ErrorJwkExtractableInconsistent();
       extractable = extractable && jwk_extractable_value;
     }
   }
 
   // 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)
   // 5. JWK alg missing AND input algorithm isNull: error
   // 6. JWK alg missing AND input algorithm specified: use input value
   blink::WebCryptoAlgorithm algorithm = blink::WebCryptoAlgorithm::createNull();
   std::string jwk_alg_value;
   if (dict_value->GetString("alg", &jwk_alg_value)) {
     // JWK alg present
 
     // TODO(padolph): Validate alg vs kty. For example kty="RSA" implies alg can
     // only be from the RSA family.
 
     const blink::WebCryptoAlgorithm jwk_algorithm =
         jwk_alg_factory.Get().CreateAlgorithmFromName(jwk_alg_value);
     if (jwk_algorithm.isNull()) {
       // JWK alg unrecognized
-      return false;  // case 1
+      return Status::ErrorJwkUnrecognizedAlgorithm();  // case 1
     }
     // JWK alg valid
     if (algorithm_or_null.isNull()) {
       // input algorithm not specified
       algorithm = jwk_algorithm;  // case 2
     } else {
       // input algorithm specified
       if (!WebCryptoAlgorithmsConsistent(jwk_algorithm, algorithm_or_null))
-        return false;  // case 3
+        return Status::ErrorJwkAlgorithmInconsistent();  // case 3
       algorithm = algorithm_or_null;  // case 4
     }
   } else {
     // JWK alg missing
     if (algorithm_or_null.isNull())
-      return false;  // case 5
+      return Status::ErrorJwkAlgorithmMissing();  // case 5
     algorithm = algorithm_or_null;  // case 6
   }
   DCHECK(!algorithm.isNull());
 
   // JWK "use" (optional) --> usage_mask parameter
   std::string jwk_use_value;
   if (dict_value->GetString("use", &jwk_use_value)) {
     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 false;
+      return Status::ErrorJwkUnrecognizedUsage();
     }
     if ((jwk_usage_mask & usage_mask) != usage_mask) {
       // A usage_mask must be a subset of jwk_usage_mask.
-      return false;
+      return Status::ErrorJwkUsageInconsistent();
     }
   }
 
   // JWK keying material --> ImportKeyInternal()
   if (jwk_kty_value == "oct") {
 
     std::string jwk_k_value;
     if (!GetDecodedUrl64ValueByKey(*dict_value, "k", &jwk_k_value))
-      return false;
+      return Status::ErrorJwkDecodeK();
 
     // TODO(padolph): Some JWK alg ID's embed information about the key length
     // in the alg ID string. For example "A128" implies the JWK carries 128 bits
     // of key material, and "HS512" implies the JWK carries _at least_ 512 bits
     // of key material. For such keys validate the actual key length against the
     // value in the ID.
 
     return ImportKeyInternal(blink::WebCryptoKeyFormatRaw,
                              reinterpret_cast<const uint8*>(jwk_k_value.data()),
                              jwk_k_value.size(),
                              algorithm,
                              extractable,
                              usage_mask,
                              key);
   } else if (jwk_kty_value == "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.
 
     // RSA private key import is not currently supported, so fail here if a "d"
     // entry is found.
     // TODO(padolph): Support RSA private key import.
     if (dict_value->HasKey("d"))
-      return false;
+      return Status::ErrorJwkRsaPrivateKeyUnsupported();
 
     std::string jwk_n_value;
     if (!GetDecodedUrl64ValueByKey(*dict_value, "n", &jwk_n_value))
-      return false;
+      return Status::ErrorJwkDecodeN();
     std::string jwk_e_value;
     if (!GetDecodedUrl64ValueByKey(*dict_value, "e", &jwk_e_value))
-      return false;
+      return Status::ErrorJwkDecodeE();
 
     return ImportRsaPublicKeyInternal(
         reinterpret_cast<const uint8*>(jwk_n_value.data()),
         jwk_n_value.size(),
         reinterpret_cast<const uint8*>(jwk_e_value.data()),
         jwk_e_value.size(),
         algorithm,
         extractable,
         usage_mask,
         key);
 
   } else {
-    return false;
+    return Status::ErrorJwkUnrecognizedKty();
   }
 
-  return true;
+  return Status::Success();
 }
 
 }  // namespace content