Refactor CRX verification in preparation to support CRX₃ files.

components/crx_file/crx_verifier.h

Index: components/crx_file/crx_verifier.h
diff --git a/components/crx_file/crx_verifier.h b/components/crx_file/crx_verifier.h
new file mode 100644
index 0000000000000000000000000000000000000000..2e0ed06c40f96fb13dc75b41671ebd551d65d124
--- /dev/null
+++ b/components/crx_file/crx_verifier.h
@@ -0,0 +1,99 @@
+// Copyright 2017 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 COMPONENTS_CRX_FILE_CRX_VERIFIER_H_
+#define COMPONENTS_CRX_FILE_CRX_VERIFIER_H_
+
+#include <stdint.h>
+#include <string>
+#include <vector>
+
+namespace base {
+class FilePath;
+class File;
+}  // namespace base
+
+namespace crypto {
+class SecureHash;
+}  // namespace crypto
+
+namespace crx_file {
+
+// A CrxVerifier is capable of verifying a Crx file. It accepts both Crx2
+// and Crx3 files that fulfill the specified requirements. To accept Crx3 files
+// only, use a Crx3Verifier.

[Sorin Jianu, 2017/05/15 19:49:53]

comment seems obsolete.

[waffles, 2017/05/16 00:29:03]

Done.

+class CrxVerifier final {
+ public:
+  // Constructs a new CrxVerifier that verifies the file is a well-formed

[Sorin Jianu, 2017/05/15 19:49:53]

Comment seems appropriate as a class comment.

[waffles, 2017/05/16 00:29:03]

Acknowledged.

+  // Crx3 or Crx2 file, and verifies each understood proof on the file.
+  // Additionally, a Crx3 file must have a proof whose public key hashes to the
+  // enclosed Crx ID.
+  CrxVerifier();
+
+  ~CrxVerifier();
+
+  // Once called, this verifier requires at least one of the proofs to use a
+  // public key whose SHA256 hash is equal to |key_hash|. Additional calls to
+  // RequireKeyProof accumulate additional requirements.

[Sorin Jianu, 2017/05/15 19:49:53]

Is there any reason the caller wants to accumulate these instead of providing
them at once?

[waffles, 2017/05/16 00:29:02]

Obsolete.

+  void RequireKeyProof(const std::vector<uint8_t>& key_hash);
+
+  // Once called, this verifier requires that the overall Crx file have a SHA256
+  // hash equal to |expected_hash| (encoded in base 16). Additional calls to
+  // RequireFileHash replace previous expected hashes.

[Sorin Jianu, 2017/05/15 19:49:53]

Why would the caller want to replace the file hash? Can an instance of this
class be used to verify more than one file? If yes, what is the motivation for
this?

[waffles, 2017/05/16 00:29:03]

Obsolete.

+  void RequireFileHash(const std::vector<uint8_t>& expected_hash);
+
+  // Once called, this verifier rejects Crx2 files.
+  void RequireCrx3();
+
+  // Once called, this verifier requires one of the Crx3 proofs to match a
+  // pinned publisher key. (The publisher key is pinned in the crx_file module).
+  // It is an error to call this without first calling RequireCrx3.
+  void RequirePublisherProof();

[Sorin Jianu, 2017/05/15 19:49:53]

Should this function return an error to indicate this?

[waffles, 2017/05/16 00:29:03]

Not sure what you mean.

+
+  // Once called, when Verify is called this verifier will set the contents of
+  // |public_key| to one of the following values: (For Crx2) the public key the
+  // file is signed with, or (for Crx3) the public key that hashes to the
+  // developer proof, or (for Crx3) the empty string if there is no such proof.
+  // In all cases the key is encoded as a base64 string (PEM).
+  // Additional calls to GetPublicKey replace previous
+  // |public_key| parameters.
+  void GetPublicKey(std::string* public_key);

[Sorin Jianu, 2017/05/15 19:49:53]

const?

[Sorin Jianu, 2017/05/15 19:49:53]

why not return by value?

[waffles, 2017/05/16 00:29:02]

Explained this in person. Anyways, obsolete.

[waffles, 2017/05/16 00:29:03]

Explained this in person. Anyways, obsolete.

+
+  // Once called, when Verify is called this verifier will set the contents of
+  // |crx_id| to one of the following values: (For Crx2) the id as computed from
+  // the hash of the signing key, or (for Crx3) the id declared in the file.
+  // Additional calls to GetCrxId replace previous
+  // |crx_id| parameters.
+  void GetCrxId(std::string* crx_id);

[Sorin Jianu, 2017/05/15 19:49:53]

const?

[Sorin Jianu, 2017/05/15 19:49:53]

return by value?

[waffles, 2017/05/16 00:29:02]

Explained this in person. Anyways, obsolete.

[waffles, 2017/05/16 00:29:03]

Explained this in person. Anyways, obsolete.

+
+  enum class Result {

[Sorin Jianu, 2017/05/15 19:49:53]

types usually declared toward the beginning of the section? Maybe move both the
type and the function toward the top. Also, could add more errors to handle
calling functions which currently return void.

[waffles, 2017/05/16 00:29:02]

Done.

+    OK_FULL,   // The file verifies as a correct full CRX file.
+    OK_DELTA,  // The file verifies as a correct differential CRX file.
+    ERROR_FILE_NOT_READABLE,      // Cannot open the CRX file.
+    ERROR_HEADER_INVALID,         // Failed to parse or understand CRX header.
+    ERROR_EXPECTED_HASH_INVALID,  // Expected hash is not well-formed.
+    ERROR_FILE_HASH_FAILED,  // The file's actual hash != the expected hash.
+    ERROR_SIGNATURE_INITIALIZATION_FAILED,  // A signature or key is malformed.
+    ERROR_SIGNATURE_VERIFICATION_FAILED,    // A signature doesn't match.
+    ERROR_REQUIRED_PROOF_MISSING,           // RequireKeyProof was unsatisfied.
+  };
+
+  // Verify the crx file at |crx_path|, subject to this verifier's requirements.
+  Result Verify(const base::FilePath& crx_path) const;
+
+ private:
+  bool allow_crx2_ = true;
+  bool require_publisher_proof_ = false;
+  std::string* crx_id_ = nullptr;

[Sorin Jianu, 2017/05/15 19:49:53]

why pointers?

[waffles, 2017/05/16 00:29:02]

Explained this in person. Anyways, obsolete.

+  std::string* public_key_ = nullptr;
+  std::vector<std::vector<uint8_t>> key_hashes_;
+  std::vector<uint8_t> expected_hash_;

[Sorin Jianu, 2017/05/15 19:49:53]

Is this type copyable?

[waffles, 2017/05/16 00:29:03]

Assuming you are referring to Verifier, obsolete.

+
+  Result VerifyCrx2(base::File* file, crypto::SecureHash* hash) const;
+  Result VerifyCrx3(base::File* file, crypto::SecureHash* hash) const;

[Sorin Jianu, 2017/05/15 19:49:52]

Member function declaration precede data in class definitions, afair.

[waffles, 2017/05/16 00:29:03]

Obsolete.

+};
+
+}  // namespace crx_file
+
+#endif  // COMPONENTS_CRX_FILE_CRX_VERIFIER_H_