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..03e97ad4d67243b44ba57c65d5d513c6b1e00e26
--- /dev/null
+++ b/net/cert/ct_ev_whitelist.h
@@ -0,0 +1,121 @@
+// 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 <stdint.h>
+
+#include <set>
+#include <string>
+
+#include "base/files/file_path.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/strings/string_piece.h"
+#include "net/base/net_export.h"
+
+namespace net {
+
+namespace ct {
+
+namespace internal {
+
+// A class for reading individual bits from a buffer of bytes they are 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.
+class NET_EXPORT_PRIVATE BitStreamReader {

[Ryan Sleevi, 2014/09/11 23:44:38]

Why do you have this in a public header in an ::internal namespace? It seems
unrelated.

Move it to it's own header, and I don't think it belongs in _internal.
_EXPORT_PRIVATE is a sufficient signal that this is an internal implementation
detail, while still being in a .h for testing. If you're actually discretely
unit testing this (which you should)

[Eran Messeri, 2014/10/01 16:08:36]

Short answer is that it's in a public header in an internal namespace for unit
testing it (no net_export moniker anymore since it's not under net anymore).
Should it live in a separate header? not in a namespace? I'll happily move it to
a more suitable location.

+ public:
+  BitStreamReader(const base::StringPiece& source);
+
+  // Reads unary-encoded number into |out|. Returns true if
+  // there was at least one bit to read, false otherwise.
+  bool ReadUnaryEncoding(uint64_t* out);
+
+  // 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_t num_bits, uint64_t* out);
+
+  // Returns the number of bits left in the stream.
+  uint64_t BitsLeft() const;
+
+ private:
+  // Reads a single bit. Within a byte, the bits are read from the MSB to the
+  // LSB.
+  uint8_t ReadBit();
+
+  const base::StringPiece source_;
+
+  // Index of the byte currently being read from.
+  uint64_t current_byte_;
+
+  // Index of the last bit read within |current_byte_|. Since bits are read
+  // from the MSB to the LSB, this value is initialized to 7 and decremented
+  // after each read.
+  int8 current_bit_;
+};
+
+}  // namespace internal
+
+class NET_EXPORT EVCertsWhitelist
+    : public base::RefCountedThreadSafe<EVCertsWhitelist> {

[Ryan Sleevi, 2014/09/11 23:44:38]

There's a really, really high-barrier for introducing new ref-counted classes.
Can you explain why you made this RefCounted? It's a big design constraint to do
so.

[Eran Messeri, 2014/10/01 16:08:36]

(also mentioned offline) It's ref-counted because, IIUC, that will allow safe
updating of the whitelist. I.e.,  the whitelist instance could be used by the
SSLClientSocketNSS, while a new whitelist is set on the IO thread.

Are you suggesting that by setting the list only on the IO thread we could avoid
reference counting? 

+ public:
+  // Returns true if the |certificate_hash| appears in the EV certificate hashes
+  // whitelist.
+  virtual bool ContainsCertificateHash(
+      const std::string& certificate_hash) const = 0;
+
+  // Returns true if the global EV certificate hashes whitelist is non-empty,
+  // false otherwise.
+  virtual bool IsValid() const = 0;
+
+  // Sets the EV whitelist from a compressed string, if it is valid.
+  // 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.
+  virtual bool Update(const std::string& compressed_whitelist) = 0;

[Ryan Sleevi, 2014/09/11 23:44:38]

I don't think this method really belongs/makes sense.

1) It doesn't really fit the virtual interface that I see EVCertsWhitelist
having, which is meant for 'accessing' the whitelist.
2) Because this is pure virtual, every concrete class is going to have to
implement uncompressing compressed_whitelist. That doesn't sound like the right
thing for an interface?

That is, updating the list (and lines 89/90, uncompressing) are more details a
concrete implementation.

I would instead focus your interface simply on accessing (lines 70-71? Is 73-75
needed?), with a concrete implementation that handles the details of
compression/uncompression.

Put differently, the interface you want to thread from //chrome into //net is
simply "here's how to access the list", with "here's how to update the list"
living in //chrome (or //components, w/e), since that's the only layer that
should ever be updating the list. Did that make sense?

[Eran Messeri, 2014/10/01 16:08:36]

Yes - as you recommended, moved the implementation to //chrome/browser/net, this
interface has no update method anymore. A new instance is constructed each time
the list is updated (and IsValid is still there so the list is not replaced if
it's not valid).

+
+ protected:
+  virtual ~EVCertsWhitelist() {}
+  // 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.
+  static bool UncompressEVWhitelist(const std::string& compressed_whitelist,
+                                    std::set<std::string>* uncompressed_list);
+
+  friend class EVCertsWhitelistTest;
+  FRIEND_TEST_ALL_PREFIXES(EVCertsWhitelistTest,
+                           UncompressFailsForTooShortList);
+  FRIEND_TEST_ALL_PREFIXES(EVCertsWhitelistTest,
+                           UncompressFailsForTruncatedList);
+  FRIEND_TEST_ALL_PREFIXES(EVCertsWhitelistTest,
+                           UncompressesWhitelistCorrectly);

[Ryan Sleevi, 2014/09/11 23:44:38]

Generally, pick one or the other here. If you friend the fixture (line 92), then
you should make protected methods that mess with this class, and remove lines
93-98. Otherwise, you explicitly list tests like you do.

[Eran Messeri, 2014/10/01 16:08:35]

Done. Picked FRIEND_TEST_ALL_PREFIXES because the only protected method I want
to test is UncompressEVWhitelist.

+
+ private:
+  friend class base::RefCountedThreadSafe<EVCertsWhitelist>;
+};
+
+// 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);

[Ryan Sleevi, 2014/09/11 23:44:38]

If you did keep this method (still uncomfortable with it, it's the wrong layer
it feels), forward declare base::FilePath.

[Eran Messeri, 2014/10/01 16:08:35]

That now lives in //chrome/browser/net, forward-declared base::FilePath.

+
+namespace internal {
+// For testing purposes
+NET_EXPORT EVCertsWhitelist* GetEmptyEVCertsWhitelist();

[Ryan Sleevi, 2014/09/11 23:44:39]

If it's for testing, why isn't it NET_EXPORT_PRIVATE?

Note that if you take my comments above and separate this into a simple
'testing' interface, then you'd simply create a dummy fake EVCertsWhitelist in
your test .cc, since you'd only have one (two?) methods to implement 

ContainsCertificateHash (always returns false), and MAYBE IsValid() (returns
true?)

[Eran Messeri, 2014/10/01 16:08:35]

Removed this method, per your suggestion there's a DefaultEVCertsWhitelist
subclass that behaves as you described and (as the name suggests) is the default
for SSLConfig instances.

+}  // namespace internal
+
+}  // namespace ct
+
+}  // namespace net
+
+#endif  // NET_CERT_CT_EV_WHITELIST_H_