net: add CRL filter infrastructure.

net/base/crl_filter.h

+// Copyright (c) 2011 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_BASE_CRL_FILTER_H_
+#define NET_BASE_CRL_FILTER_H_
+#pragma once
+
+#include <set>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include <base/memory/ref_counted.h>
+#include <base/memory/scoped_ptr.h>
+#include <base/string_piece.h>
+#include <base/synchronization/lock.h>
+
+class DictionaryValue;
+
+namespace net {
+
+class GolombCompressedSet;
+
+// A CRLFilter is a probabilistic data structure for eliminating certificate
+// revocation checks. A CRL filter contains information about some number of
+// globally well known CRLs. Those CRLs are said to be `covered' by the filter.
+//
+// If a certificate specifies a CRL that is covered then the CRLFilter can give
+// a firm "not revoked" answer or a probabilistic "revoked" answer.
+// Additionally, a CRLFilter can contain a list of blocked public keys and, in
+// that case, it can give a firm "revoked" answer.
+class CRLFilter : public base::RefCounted<CRLFilter> {
+ public:
+  enum Result {
+    REVOKED,  // the certificate should be rejected.
+    PROBABLY_REVOKED,  // the certificate should be checked.
+    NOT_REVOKED,  // the certificate is acceptable.
+    UNKNOWN,  // no information available.
+  };
+

[Ryan Sleevi, 2011/06/02 22:01:54]

BUG: No constructors defined, so an implicit (public) constructor will be
created. Given the static method Parse(), I would think the ctor was meant to be
private (and explicitly specified) 

+  ~CRLFilter();

[Ryan Sleevi, 2011/06/02 22:01:54]

If it is RefCounted, shouldn't this be private:, with the Refcounted<CRLFilter>
friend'ed, to prevent both stack allocations and delete's? 

+
+  static CRLFilter* Parse(base::StringPiece data);
+
+  // CheckCertificate returns the information contained in the filter for a
+  // given certificate:
+  //   cert_spki: the SubjectPublicKeyInfo for the certificate
+  //   serial_number: the serial number of the certificate
+  //   crl_urls: the URLs for the CRL for the certificate
+  //   parent_spki: the SubjectPublicKeyInfo of the CRL signer
+  //
+  // This does not check that the CRLFilter is timely. See |not_before| and
+  // |not_after|.
+  Result CheckCertificate(
+      base::StringPiece cert_spki,
+      const std::string& serial_number,
+      std::vector<base::StringPiece> crl_urls,
+      base::StringPiece parent_spki);

[Ryan Sleevi, 2011/06/02 22:01:54]

nit?: Result CheckCertificate(...) const


nit?: base::StringPiece -> const base::StringPiece&
Admittedly, there's no copying of the underlying data going on, but I thought
passing objects by value was something discouraged? Also throughout the .cc file

+
+  // ApplyDelta returns a new CRLFilter that is the result of updating the
+  // current filter with the delta information in |delta_bytes|.
+  CRLFilter* ApplyDelta(base::StringPiece delta_bytes);

[Ryan Sleevi, 2011/06/02 22:01:54]

suggestion: CRLFilter* ApplyDelta(...) const ? Since it returns a new CRLFilter?

+
+  // not_before and not_after return the validity timespan of this filter.
+  // |CheckCertificate| does not check the current time so it's up to the
+  // caller to ensure that the CRLFilter is timely.
+  int64 not_before() const;
+  int64 not_after() const;
+
+  // DebugValues return all GCS values, in order. This should only be used

[Ryan Sleevi, 2011/06/02 22:01:54]

nit: First appearance of "GCS" in the header, without comment that it's a
GolombCompressedSet.

See also
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

+  // for testing.
+  std::vector<uint64> DebugValues();
+  // num_entries returns the number of GCS values in the filter. This should
+  // only be used for testing.
+  unsigned num_entries() const;
+  // max_range returns size of the hash range. This should only be used for
+  // testing.
+  uint64 max_range() const;
+  // SHA256 returns a hash over the header and GCS bytes of the filter. This
+  // should only be used for testing.
+  std::string SHA256() const;
+

[Ryan Sleevi, 2011/06/02 22:01:54]

nit: For these "Should only be used for testing", would it be better to make
them private and friended to the test? That is the dominant pattern in the net/
code

Or even just friend the test, so it can access the data members directly, and
move these helpers into the test file. 

+ private:
+  // These are the range coder symbols used in delta updates.
+  enum {
+    SYMBOL_SAME = 0,
+    SYMBOL_INSERT = 1,
+    SYMBOL_DELETE = 2,
+  };

[Ryan Sleevi, 2011/06/02 22:01:54]

nit: Private, unnamed enum can be moved wholly into the implementation file - no
need for inclusion in the header. 

+
+  static CRLFilter* CRLFilterFromHeader(base::StringPiece header);

[Ryan Sleevi, 2011/06/02 22:01:54]

nit: Static private function can be moved wholly into the implementation file -
no need for inclusion in the header. 

+  bool CRLIsCovered(std::vector<base::StringPiece> crl_urls,
+                    const std::string& parent_spki_sha256);

[Ryan Sleevi, 2011/06/02 22:01:54]

nit?: bool CRLIsCovered(...) const
nit: std::vector<base::StringPiece> crl_urls -> const
std::vector<base::StringPiece>& crl_urls 

+
+  int64 not_before_, not_after_;

[Ryan Sleevi, 2011/06/02 22:01:54]

super-minor nit?: I thought the style guide prohibited multiple variables from
being declared in the same statement, but I'm having trouble finding where.
Still, it seems uncommon in the code that I've seen. 

+  uint64 max_range_;
+  unsigned sequence_;

[Ryan Sleevi, 2011/06/02 22:01:54]

nit: Explicitly sized integer (presumably, int64 based on the JSON
implementation) 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types
http://www.chromium.org/developers/coding-style#TOC-Types

+  unsigned num_entries_;

[Ryan Sleevi, 2011/06/02 22:01:54]

nit: size_t 

+
+  std::string header_bytes_;
+
+  std::set<std::pair<std::string, std::string> > crls_included_;
+  std::string gcs_bytes_;
+  scoped_ptr<GolombCompressedSet> gcs_;

[Ryan Sleevi, 2011/06/02 22:01:54]

DISALLOW_COPY_AND_ASSIGN(CRLFilter)

+};
+
+}  // namespace net
+
+#endif  // NET_BASE_CRL_FILTER_H_