Refactor WebCrypto code

content/child/webcrypto/algorithm.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_CRYPTO_ALGORITHM_H_
+#define CONTENT_CHILD_WEBCRYPTO_CRYPTO_ALGORITHM_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "third_party/WebKit/public/platform/WebCrypto.h"
+
+namespace content {
+
+namespace webcrypto {
+
+class CryptoData;
+class Status;
+
+// AlgorithmImplementation is a base class for *executing* the operations of an
+// algorithm (generating keys, encrypting, signing, etc.).
+//
+// This is in contrast to blink::WebCryptoAlgorithm which instead *describes*
+// the operation and its parameters.
+//
+// AlgorithmImplementation has reasonable default implementations for all
+// methods which behave as if the operation is it is unsupported, so
+// implementations need only override the applicable methods.
+//
+// Unless stated otherwise methods of AlgorithmImplementation are responsible
+// for sanitizing their inputs. The following can be assumed:
+//
+//   * |algorithm.id()| and |key.algorithm.id()| matches the algorithm under
+//     which the implementation was registerd.
+//   * |algorithm| has the correct parameters type for the operation.
+//   * The key usages have already been verified. In fact in the case of calls
+//     to Encrypt()/Decrypt() the corresponding key usages may not be present
+//     (when wrapping/unwrapping).
+class AlgorithmImplementation {

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

All of these methods should be documented.

It may be sufficient to simply say something like

// The following methods are responsible for performing the algorithm-specific
operations of the same name, as defined in web-crypto

or something less... awful.

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

Done.

+ public:
+  virtual ~AlgorithmImplementation();
+
+  virtual Status Encrypt(const blink::WebCryptoAlgorithm& algorithm,
+                         const blink::WebCryptoKey& key,
+                         const CryptoData& data,
+                         std::vector<uint8>* buffer) const;
+
+  virtual Status Decrypt(const blink::WebCryptoAlgorithm& algorithm,
+                         const blink::WebCryptoKey& key,
+                         const CryptoData& data,
+                         std::vector<uint8>* buffer) const;
+
+  virtual Status Sign(const blink::WebCryptoAlgorithm& algorithm,
+                      const blink::WebCryptoKey& key,
+                      const CryptoData& data,
+                      std::vector<uint8>* buffer) const;
+
+  virtual Status Verify(const blink::WebCryptoAlgorithm& algorithm,
+                        const blink::WebCryptoKey& key,
+                        const CryptoData& signature,
+                        const CryptoData& data,
+                        bool* signature_match) const;
+
+  virtual Status Digest(const blink::WebCryptoAlgorithm& algorithm,
+                        const CryptoData& data,
+                        std::vector<uint8>* buffer) const;
+
+  // When generating a key, VerifyKeyUsagesVeforeGenerateKey() will always be
+  // called before GenerateSecretKey(). Similarly when generating a keypair
+  // VerifyKeyUsagesBeforeGenerateKey() will always be called before
+  // GenerateKeyPair().

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

This is documenting how some other class uses this interface, which is not
generally appropriate.

Instead, document what is expected of callers, including any preconditions

e.g. VerifyKeyUsagesBeforeGenerateKey MUST be called before GenerateKeyPair

Also document what a derived implementation is expected to do in these methods.

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

Done.
Done

+
+  virtual Status VerifyKeyUsagesBeforeGenerateKey(
+      blink::WebCryptoKeyUsageMask usage_mask) const;
+
+  virtual Status GenerateSecretKey(const blink::WebCryptoAlgorithm& algorithm,
+                                   bool extractable,
+                                   blink::WebCryptoKeyUsageMask usage_mask,
+                                   blink::WebCryptoKey* key) const;
+
+  virtual Status VerifyKeyUsagesBeforeGenerateKeyPair(
+      blink::WebCryptoKeyUsageMask combined_usage_mask,
+      blink::WebCryptoKeyUsageMask* public_usage_mask,
+      blink::WebCryptoKeyUsageMask* private_usage_mask) const;
+
+  virtual Status GenerateKeyPair(
+      const blink::WebCryptoAlgorithm& algorithm,
+      bool extractable,
+      blink::WebCryptoKeyUsageMask public_usage_mask,
+      blink::WebCryptoKeyUsageMask private_usage_mask,
+      blink::WebCryptoKey* public_key,
+      blink::WebCryptoKey* private_key) const;
+
+  // -----------------------------------------------
+  // Key import
+  // -----------------------------------------------
+  // VerifyKeyUsagesBeforeImportKey() will always be called before either
+  // importing a key, or unwrapping a key.

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

Again, layering.

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

Done.

+  //
+  // Note that when the format is JWK and importing an asymmetric key,
+  // VerifyKeyUsagesBeforeImportKey() will not know what the key type is yet. In
+  // this case the import function will be responsible for checking the usage.

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

This belongs in ImportKeyJwk. It's unclear from your documentation, for example,
if VerifyKeyUsagesBeforeImportKey is expected to be called for JWK keys, and if
so, what the |format| value will be.

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

Modified the comment. In the case of JWK, VerifyKeyUsagesBeforeImportKey() is
called with format=JWK.

+
+  virtual Status VerifyKeyUsagesBeforeImportKey(
+      blink::WebCryptoKeyFormat format,
+      blink::WebCryptoKeyUsageMask usage_mask) const;
+
+  virtual Status ImportKeyRaw(const CryptoData& key_data,
+                              const blink::WebCryptoAlgorithm& algorithm,
+                              bool extractable,
+                              blink::WebCryptoKeyUsageMask usage_mask,
+                              blink::WebCryptoKey* key) const;
+
+  virtual Status ImportKeyPkcs8(const CryptoData& key_data,
+                                const blink::WebCryptoAlgorithm& algorithm,
+                                bool extractable,
+                                blink::WebCryptoKeyUsageMask usage_mask,
+                                blink::WebCryptoKey* key) const;
+
+  virtual Status ImportKeySpki(const CryptoData& key_data,
+                               const blink::WebCryptoAlgorithm& algorithm,
+                               bool extractable,
+                               blink::WebCryptoKeyUsageMask usage_mask,
+                               blink::WebCryptoKey* key) const;
+
+  virtual Status ImportKeyJwk(const CryptoData& key_data,
+                              const blink::WebCryptoAlgorithm& algorithm,
+                              bool extractable,
+                              blink::WebCryptoKeyUsageMask usage_mask,
+                              blink::WebCryptoKey* key) const;
+
+  // -----------------------------------------------
+  // Key export
+  // -----------------------------------------------
+
+  virtual Status ExportKeyRaw(const blink::WebCryptoKey& key,
+                              std::vector<uint8>* buffer) const;
+
+  virtual Status ExportKeyPkcs8(const blink::WebCryptoKey& key,
+                                std::vector<uint8>* buffer) const;
+
+  virtual Status ExportKeySpki(const blink::WebCryptoKey& key,
+                               std::vector<uint8>* buffer) const;
+
+  virtual Status ExportKeyJwk(const blink::WebCryptoKey& key,
+                              std::vector<uint8>* buffer) const;
+};
+
+}  // namespace webcrypto
+
+}  // namespace content
+
+#endif  // CONTENT_CHILD_WEBCRYPTO_CRYPTO_ALGORITHM_H_