Certificate Transparency: Code for unpacking EV cert hashes whitelist

net/cert/ct_ev_whitelist.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 NET_CERT_CT_EV_WHITELIST_H_
+#define NET_CERT_CT_EV_WHITELIST_H_
+
+#include <set>
+#include <string>
+
+#include "base/files/file_path.h"
+#include "net/base/net_export.h"
+
+namespace net {
+
+namespace ct {
+
+namespace internal {
+
+/**
+ * Abstraction over a stream of bits, to be read independently
+ * of the bytes they're packed into. It is limited to 64-bit reads
+ * and is inefficient as a design choice - Since it is used infrequently
+ * to unpack the Golomb-coded EV certificate hashes whitelist in a
+ * blocking thread.
+ *
+ * This class is declared here so it can be tested.
+ */
+class NET_EXPORT_PRIVATE BitStreamReader {
+ public:
+  BitStreamReader(const char* source, size_t length);
+
+  // Reads unary-encoded number into |out|. Returns true if
+  // there was at least one bit to read, false otherwise.
+  bool ReadUnaryEncoding(uint64* out);
+  // Reads |num_bits| (up to 64) into |out|. Returns true if
+  // the stream had the requested |num_bits|, false otherwise.
+  bool ReadBits(uint8 num_bits, uint64* out);

[wtc, 2014/08/20 02:52:07]

Please documents in which direction you are filling in the bits in |*out|. I
think it's from the most significant bit to the least significant bit.

Another thing you should document is where the unused bits are if |num_bits| is
less than 64. I think the unused bits are the most significant |64 - num_bits|
bits.

[Eran Messeri, 2014/09/02 12:20:23]

Good points, done. Also added a class-level comment that the bitstream is read
MSB-first.

+  // Returns the number of bits left in the stream.
+  uint64 BitsLeft() const;
+
+ private:
+  // Returns true if there are more bits to read in the stream.
+  bool HasMoreBits() const;
+  // Reads a single bit.

[wtc, 2014/08/20 02:52:07]

Please document that within a byte, the bits are read from the most significant
bit to the least significant bit.

[Eran Messeri, 2014/09/02 12:20:23]

Done.

+  uint8 ReadBit();
+
+  const char* const source_;
+  const size_t length_;
+
+  uint64 curr_byte_;
+  int8 curr_bit_;

[wtc, 2014/08/20 02:52:07]

Nit: it would be nice to document these two members. I felt like I was given a
puzzle to solve. It's not too hard to figure out what they mean, but I was at
first puzzled by why curr_bit_ is initialized to 7 in the constructor.

[Eran Messeri, 2014/09/02 12:20:23]

Very valid point, code reviews should not be puzzles.
Documented both and added a comment explaining initialization.

+};
+
+// Given a Golomb-coded list of hashes in |compressed_whitelist|, unpack into
+// |uncompressed_list|. Returns true if the format of the compressed whitelist
+// is valid,
+// false otherwise.

[wtc, 2014/08/20 02:52:07]

Nit: please reformat this comment block.

[Eran Messeri, 2014/09/02 12:20:22]

Done, I blame git-cl format for that :)

+NET_EXPORT_PRIVATE bool UncompressEVWhitelist(
+    const std::string& compressed_whitelist,
+    std::set<std::string>* uncompressed_list);
+
+// Sets the given |ev_whitelist| into the global context.
+// Note that |ev_whitelist| will be empty after this call as the implementation
+// is using set::swap() to efficiently switch the sets.

[wtc, 2014/08/20 02:52:07]

IMPORTANT: are you sure this is true? The |ev_whitelist| parameter is passed by
value. For this to be true, it needs to be passed as a reference or pointer.

Also, the "empty" part is only true if the old value is empty.

[Eran Messeri, 2014/09/02 12:20:23]

My bad, it was supposed to be passed by (non-const) reference.  Fixed that and
amended the documentation.

+NET_EXPORT_PRIVATE void SetEVWhitelistData(std::set<std::string> ev_whitelist);

[wtc, 2014/08/20 02:52:07]

This function is only used by SetEVWhitelistFromFile and is not used by any unit
tests. So I think it can be an internal function in the .cc file and shouldn't
be declared here.

[Eran Messeri, 2014/09/02 12:20:23]

It's meant to be used from a test I did not write. It's now being used.

+
+}  // namespace internal
+
+// Sets the global EV certificate hashes whitelist from
+// |compressed_whitelist_file| in the global context, after uncompressing it.
+// If the data in |compressed_whitelist_file| is not a valid compressed
+// whitelist, does nothing.
+NET_EXPORT void SetEVWhitelistFromFile(
+    const base::FilePath& compressed_whitelist_file);
+
+// Returns true if the |certificate_hash| appears in the EV certificate hashes
+// whitelist.
+NET_EXPORT bool IsCertificateHashInWhitelist(
+    const std::string& certificate_hash);
+
+}  // namespace ct
+
+}  // namespace net
+
+#endif  // NET_CERT_CT_EV_WHITELIST_H_