Refactor: Expose JwkReader/JwkWriter helper classes.

content/child/webcrypto/jwk.h

 // 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.
 
 #ifndef CONTENT_CHILD_WEBCRYPTO_JWK_H_
 #define CONTENT_CHILD_WEBCRYPTO_JWK_H_
 
 #include <stdint.h>
 #include <vector>
 
 #include "base/strings/string_piece.h"
 #include "base/values.h"
 #include "content/common/content_export.h"
-#include "third_party/WebKit/public/platform/WebArrayBuffer.h"
 #include "third_party/WebKit/public/platform/WebCrypto.h"
-#include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
 
 namespace content {
 
 namespace webcrypto {
 
 class CryptoData;
 class Status;
 
+// Helper class for parsing a JWK from JSON.

[Ryan Sleevi, 2014/10/30 02:30:40]

Explain why this exists.

// Helper class for parsing a JWK from JSON.
//
// This primarily exists to ensure strict enforcement of the JWK schema,
// as the type and presence of particular fields is security relevant.
// For example, GetString() will ensure a given JSON field is present
// and is a string type, and will fail if these conditions aren't met.

[eroman, 2014/10/30 16:04:35]

Thanks for all the comment reviews BTW!

[eroman, 2014/10/30 17:00:29]

Done.

+//
+// Init() must be called (and succeed) before it is valid to call any other
+// method.
+class JwkReader {
+ public:
+  JwkReader();
+  ~JwkReader();
+
+  // Initializes a JWK reader by parsing the JSON |bytes|. To succeed the JWK

[Ryan Sleevi, 2014/10/30 02:30:40]

"To succeed,"

+  // must have kty of xpected_kty, have an ext compatible with

[Ryan Sleevi, 2014/10/30 02:30:39]

typo: expected_kty

[eroman, 2014/10/30 17:00:29]

Done.

+  // |expected_extractable| and have usages compatible with expected_usages.

[Ryan Sleevi, 2014/10/30 02:30:40]

"|expected_extractable|, and"

Oxford commas rule!

[eroman, 2014/10/30 17:00:29]

Done (changed to bullets)

+  Status Init(const CryptoData& bytes,
+              bool expected_extractable,
+              blink::WebCryptoKeyUsageMask expected_usages,
+              const std::string& expected_kty);
+
+  // Returns true if the key |key| is present.
+  bool HasKey(const std::string& key) const;

[Ryan Sleevi, 2014/10/30 02:30:39]

s/Key/Member/ throughout

That is, the name of properties of objects in JSON is "member"

(and std::string key -> member_name)

[eroman, 2014/10/30 17:00:29]

Done.

+
+  // Extracts the required string property with key |key| and saves
+  // the result to |*result|. If the property does not exist or is not a string,
+  // returns an error.
+  Status GetString(const std::string& key, std::string* result) const;
+
+  // Extracts the optional string property with key |key| and saves
+  // the result to |*result| if it was found. If the property exists and is not
+  // a string, returns an error. Otherwise returns success, and sets
+  // |*property_exists| if it was found.
+  Status GetOptionalString(const std::string& key,
+                           std::string* result,
+                           bool* property_exists) const;
+
+  // Extracts the optional array property with key |key| 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. Note that |*result| is owned by |dict|.

[Ryan Sleevi, 2014/10/30 02:30:39]

|dict_|

[eroman, 2014/10/30 17:00:29]

Done.

+  Status GetOptionalList(const std::string& key,
+                         base::ListValue** result,
+                         bool* property_exists) const;
+
+  // Extracts the required string property with key |key| and saves

[Ryan Sleevi, 2014/10/30 02:30:40]

also s/property/member/ throughout.

+  // 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 GetBytes(const std::string& key, std::string* result) const;
+
+  // Extracts the required base64url property, which is interpreted as being a
+  // big-endian unsigned integer.
+  //
+  // Sequences that contain leading zeros will be rejected.
+  Status GetBigInteger(const std::string& key, std::string* result) const;
+
+  // Extracts the optional boolean property with key |key| and
+  // saves the result to |*result| if it was found. If the property exists and
+  // is not a boolean, returns an error. Otherwise returns success, and sets
+  // |*property_exists| if it was found.
+  Status GetOptionalBool(const std::string& key,
+                         bool* result,
+                         bool* property_exists) const;
+
+  // Gets the optional algorithm ("alg") string.
+  Status GetAlg(std::string* alg, bool* has_alg) const;

[Ryan Sleevi, 2014/10/30 02:30:40]

Why is this a custom method instead of GetOptionalString("alg", ...)

[eroman, 2014/10/30 16:04:35]

I thought it was advantageous to internalize the "alg" string. It isn't likely
to ever need to be changed since JWK is pretty much frozen, but it felt like a
better encapsulation since none of the other core properties (kty, use, key_ops,
ext) needed to be exposed.

If you disagree I can certainly delete this member without much consequence.

+
+  // Checks if the "alg" property matches |expected_algorithm|.
+  Status VerifyAlg(const std::string& expected_algorithm) const;

[Ryan Sleevi, 2014/10/30 02:30:40]

Why isn't this part of Init?

[eroman, 2014/10/30 16:04:35]

The reason for this aberration is AES keys.

In the case of AES keys, the expected name of the algorithm (for instance
"A128GCM") is tied to the length of the key data, and is not known until parsing
the JWK. By not having the check be mandatory in Init() allows using the
JwkReader() to test all the other stuff first and determine the key length.

How about I make these changes:

  * I add |expected_alg| parameter to Init(), since in all other cases that is
cleaner.
  * Allow |expected_alg.empty()| to have a special meaning -- don't test "alg"

[eroman, 2014/10/30 17:00:29]

Done, made an optional part of Init()

+
+ private:
+  scoped_ptr<base::DictionaryValue> dict_;
+};
+
+// Helper class for building the JSON for a JWK.
+class JwkWriter {
+ public:
+  // Initializes a writer, and sets the standard JWK properties as indicated.
+  JwkWriter(const std::string& algorithm,
+            bool extractable,
+            blink::WebCryptoKeyUsageMask usages,
+            const std::string& kty);
+
+  // Sets a string parameter |value|.
+  void SetString(const std::string& key, const std::string& value);
+
+  // Sets a bytes parameter |value|, by base64 url-safe encoding it.
+  void SetBytes(const std::string& key, const CryptoData& value);
+
+  // Flattens the JWK to JSON (utf-8 encoded if necessary, however in practice

[Ryan Sleevi, 2014/10/30 02:30:40]

s/utf-8/UTF-8/

+  // it will be ASCII).
+  void ToJson(std::vector<uint8_t>* utf8_bytes) const;
+
+ private:
+  base::DictionaryValue dict_;
+};
+
 // Writes a JWK-formatted symmetric key to |jwk_key_data|.
 //  * raw_key_data: The actual key data
 //  * algorithm: The JWK algorithm name (i.e. "alg")
 //  * extractable: The JWK extractability (i.e. "ext")
 //  * usages: The JWK usages (i.e. "key_ops")
 void WriteSecretKeyJwk(const CryptoData& raw_key_data,
                        const std::string& algorithm,
                        bool extractable,
                        blink::WebCryptoKeyUsageMask usages,
                        std::vector<uint8_t>* jwk_key_data);
@@ skipping 96 matching lines @@
 // Base64DecodeUrlSafe() above.
 CONTENT_EXPORT std::string Base64EncodeUrlSafe(const base::StringPiece& input);
 CONTENT_EXPORT std::string Base64EncodeUrlSafe(
     const std::vector<uint8_t>& input);
 
 }  // namespace webcrypto
 
 }  // namespace content
 
 #endif  // CONTENT_CHILD_WEBCRYPTO_JWK_H_