Certificate Transparency: Code for unpacking EV cert hashes whitelist

net/cert/ct_ev_whitelist.h

Index: net/cert/ct_ev_whitelist.h
diff --git a/net/cert/ct_ev_whitelist.h b/net/cert/ct_ev_whitelist.h
new file mode 100644
index 0000000000000000000000000000000000000000..458a988bee911e0e40717e3fb041b0be36ec0058
--- /dev/null
+++ b/net/cert/ct_ev_whitelist.h
@@ -0,0 +1,93 @@
+// 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. Bits are read MSB-first from the stream.
+// 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.

[Ryan Sleevi, 2014/09/08 19:48:00]

First glance: This comment needs serious rewording, but I'm not sure what to
suggest. What it doesn't tell me is much about this class or its use.

[Eran Messeri, 2014/09/10 12:42:25]

Tried re-wording it a bit, although I think that the name is representative
enough of what the class does so only little documentation is necessary.

+//
+// This class is declared here so it can be tested.

[Ryan Sleevi, 2014/09/08 19:48:00]

Delete this comment (and then line 25)

[Eran Messeri, 2014/09/10 12:42:24]

Done.

+class NET_EXPORT_PRIVATE BitStreamReader {
+ public:
+  BitStreamReader(const char* source, size_t length);

[Ryan Sleevi, 2014/09/08 19:48:00]

Design choice: Why not base::StringPiece as our preferred structure of bytes &
length.

[Eran Messeri, 2014/09/10 12:42:25]

Done. Note that the implication is copying the data to a member field.

+
+  // Reads unary-encoded number into |out|. Returns true if
+  // there was at least one bit to read, false otherwise.
+  bool ReadUnaryEncoding(uint64* out);

[Ryan Sleevi, 2014/09/08 19:48:00]

1) #include <stdint.h>
2) uint64_t as the type
3) newline before line 34.

[Eran Messeri, 2014/09/10 12:42:24]

Done.

+  // Reads |num_bits| (up to 64) into |out|. |out| is filled from the MSB to the
+  // LSB. If |num_bits| is less than 64, the most significant |64 - num_bits|
+  // bits are unused and left as zeros. Returns true if the stream had the
+  // requested |num_bits|, false otherwise.
+  bool ReadBits(uint8 num_bits, uint64* out);

[Ryan Sleevi, 2014/09/08 19:48:00]

1) uint8_t, uint64_t
2) newline before line 39

[Eran Messeri, 2014/09/10 12:42:25]

Done.

+  // Returns the number of bits left in the stream.
+  uint64 BitsLeft() const;
+
+ private:
+  // Reads a single bit. Within a byte, the bits are read from the MSB to the
+  // LSB.
+  uint8 ReadBit();
+
+  const char* const source_;
+  const size_t length_;
+
+  // Index of the byte currently being read from.
+  uint64 current_byte_;
+  // Index of the last bit read within |current_byte_|. Since bits are read

[Ryan Sleevi, 2014/09/08 19:48:00]

newline between 51/52

[Eran Messeri, 2014/09/10 12:42:24]

Done.

+  // from the MSB to the LSB, this value is initialized to 7 and decremented
+  // after each read.
+  int8 current_bit_;
+};
+
+// 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.
+NET_EXPORT_PRIVATE bool UncompressEVWhitelist(
+    const std::string& compressed_whitelist,
+    std::set<std::string>* uncompressed_list);

[Ryan Sleevi, 2014/09/08 19:48:00]

From an API design, it seems a very design-limiting choice to use global
functions like this, and to force the data type as an std::set<std::string>*

It seems like you're better off exposing the EV whitelist as a class, with a
series of helper methods to determine whether or not a given certificate is
whitelisted.

On top of that base (pure-virtual) interface, you have two concrete
implementations:
- One which can read data from a file on disk
  - The Golumb coding, the internal structure of the data, etc is entirely
opaque in this interface (e.g. it's part of the .cc).
  - From an API consumer standpoint, you just check whether or not a certificate
is in the whitelist.
- One which can be used for testing

[Eran Messeri, 2014/09/10 12:42:25]

Done  - created a class for the EV whitelist data.

+
+// Sets the given |ev_whitelist| into the global context.
+// Note that |ev_whitelist| will contain the old EV whitelist data after this
+// call as the implementation is using set::swap() to efficiently switch the
+// sets.
+NET_EXPORT_PRIVATE void SetEVWhitelistData(std::set<std::string>& ev_whitelist);
+
+}  // 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(

[Ryan Sleevi, 2014/09/08 19:48:00]

This being a global in the //net layer is also a reason I'd not LGTM. Using the
abstraction above avoids this.

That is, put differently, there is an EXTREMELY high bar to introduce any new
globals in //net, especially when those globals may have complex
life-time/threading (e.g. in the case of a component update that happens after
the fact)

[Eran Messeri, 2014/09/10 12:42:24]

As we discussed offline, I've switched to the model used for CRLSets - there's
still a global, but it's kept in the SSLConfigService class. I have added a
global for the EV whitelist to that class, PTAL.

+    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);
+
+// Returns true if the global EV certificate hashes whitelist is non-empty,
+// false otherwise.
+NET_EXPORT bool HasValidEVWhitelist();
+
+}  // namespace ct
+
+}  // namespace net
+
+#endif  // NET_CERT_CT_EV_WHITELIST_H_