Certificate path builder for new certificate verification library

net/cert/internal/path_builder.h

+// Copyright 2016 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_INTERNAL_PATH_BUILDER_H_
+#define NET_CERT_INTERNAL_PATH_BUILDER_H_
+
+#include <memory>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "base/callback.h"
+#include "net/base/completion_callback.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_export.h"
+#include "net/cert/internal/verify_certificate_chain.h"
+#include "net/der/input.h"
+
+namespace net {
+
+class NET_EXPORT CertSource {
+ public:
+  using IssuerCallback = base::Callback<void(CertVector)>;

[Ryan Sleevi, 2016/05/04 02:42:42]

Musing outloud: Should we return an explicit CertVector, or should there be a
CertSource iterator returned? I'm wondering if that would save us anything, in
the end, and maybe I've been thinking too much about promises/generators lately.
(Soon! C++14! Exciting!)

[mattm, 2016/05/04 23:21:16]

Hm, could perhaps avoid some copies? It's always hard to figure out exactly how
much copying goes on in running a callback. I've given this a shot. (I made the
Request object have a GetNext method, as a bonus the callback returns the same
Request pointer in case the caller wants to keep track of which request
completed.)

+
+  class Request {
+   public:
+    Request() {}
+    // Destruction of the Request cancels it.
+    virtual ~Request() {}
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Request);
+  };
+
+  virtual ~CertSource() {}
+
+  // Append certs to |issuers|. Any existing contents of |issuers| will not be
+  // modified.
+  virtual void SyncGetIssuersOf(const CertThing* cert, CertVector* issuers) = 0;
+  // Returns ERR_IO_PENDING if async callback will be made, or OK if no async
+  // results are available from this source.
+  // If |*out_req| is destroyed, the callback will not be run.
+  // XXX should this take a scoped_refptr?
+  virtual int AsyncGetIssuersOf(const CertThing* cert,
+                                const IssuerCallback& issuers_callback,
+                                std::unique_ptr<Request>* out_req) = 0;

[Ryan Sleevi, 2016/05/04 02:42:42]

Musing outloud: Do we need two methods, or should we just have one method, in
which |out_req| was optional. If nullptr, only synchronous results are allowed.

[mattm, 2016/05/04 23:21:16]

Did consider that, the question then is when doing the async phase do you just
re-get all the sync results and throw them away when de-duping? Or keep a list
of the sources that didn't return results synchronously and only try those ones?

+};
+
+class NET_EXPORT StaticCertSource : public CertSource {

[Ryan Sleevi, 2016/05/04 02:42:41]

Musing outloud: Does it make sense to have an aggregate CertSource that
encompasses all of the logic for interrogating a collection of CertSources? This
means that you could change the CertPath builder interface to take a single
CertSource, or you could create different CertSources for different purposes
(here's the TA CertSource vs Intermediate?).

It also means you can, arguably, implement the sync/async distinction easier -
the CollectionCertSource (I suck at naming) could interrogate all of its
CertSources statically first, and asynchronously)

[mattm, 2016/05/04 23:21:16]

One issue is that CertIssuersIter currently handles each CertSource async
request independently, so it can continue processing as soon as any one of them
completes. If we aggregated them into a collection of certsources it would need
a different interface that can have multiple async callbacks. CertIssuersIter
would still need the logic of handling the multiple callbacks, so I'm not sure
it would clean things up there.

It could be worth considering if it enables a cleaner interface for creating a
CertPathBuilder, though I'm not immediately clear if it does. Probably will
become more obvious once we have more CertSource implementations and see what
the code looks like.

+ public:
+  StaticCertSource(const CertVector& certs);
+  ~StaticCertSource() override;
+
+  // CertSource implementation:
+  void SyncGetIssuersOf(const CertThing* cert, CertVector* issuers) override;
+  int AsyncGetIssuersOf(const CertThing* cert,
+                        const IssuerCallback& issuers_callback,
+                        std::unique_ptr<Request>* out_req) override;
+
+ private:
+  // The certificates that the StaticCertSource can return, keyed on the
+  // normalized subject value.
+  std::unordered_multimap<base::StringPiece,
+                          scoped_refptr<CertThing>,
+                          base::StringPieceHash>
+      intermediates_;
+};

[Ryan Sleevi, 2016/05/04 02:42:42]

There's a part of me that leans to suggest separating these out to individual
files as we proceed.

[mattm, 2016/05/04 23:21:16]

I'll look at sorting out the file layout some in another patchset.

+
+// XXX possible cert sources:
+// * intermediates supplied by server (synchronous)
+// * AIA fetch (async)
+// * intermediates in OS cert store (async?)

[Ryan Sleevi, 2016/05/04 02:42:41]

Aye

+// * certs cached from previous requests (sync?)
+
+class CertPathIter;
+
+class NET_EXPORT CertPathBuilder {
+ public:
+  using CertSources = std::vector<CertSource*>;
+
+  struct ResultPath {
+    ResultPath();
+    ~ResultPath();
+
+    std::vector<scoped_refptr<CertThing>> path;
+    int rv;
+  };
+
+  struct Result {
+    Result();
+    ~Result();
+
+    // Returns the overall result. This is the same value that was returned by
+    // Run (if synchronous) or by the CompletionCallback (if asynchronous).
+    int result() {
+      if (paths.empty())
+        return ERR_CERT_AUTHORITY_INVALID;
+      return paths[best_result_index]->rv;
+    }
+
+    // List of paths that were attempted and the result for each.
+    std::vector<std::unique_ptr<ResultPath>> paths;
+    // Index into |paths|. Before use, |paths.empty()| must be checked.
+    size_t best_result_index = 0;
+  };
+
+  // TODO: allow caller specified filters / checkers, initial values of name
+  // constraints, etc
+  //
+  // Creates a CertPathBuilder that attempts to find a path from |cert| to a
+  // trust anchor in |trust_store|, using intermediates from |cert_sources| if
+  // necessary, which satisfies |signature_policy| and is valid at |time|.
+  // Details of attempted path(s) are stored in |*result|.
+  //
+  // The caller must keep |cert_sources|, |trust_store|, |signature_policy|, and
+  // |*result| valid for the lifetime of the CertPathBuilder.
+  CertPathBuilder(scoped_refptr<CertThing> cert,
+                  const CertSources& cert_sources,
+                  const TrustStore& trust_store,
+                  const SignaturePolicy* signature_policy,
+                  const der::GeneralizedTime& time,
+                  Result* result);

[Ryan Sleevi, 2016/05/04 02:42:42]

I'm wondering whether we should have a CertPathBuilderBuilder (OK, bad name). I
just imagine that there will be a growing set of parameters to CertPathBuilder
as things grow - visitors for debugging interfaces, callbacks for application
policy, etc - that rather than pass all in the Ctor, we have something that
builds an object and then we run.

(To that end, I'm not even sure if we need to break it out into a Builder, per
se, vs SetFoo/SetBar and then validate consistency in the Run() call; the
Builder just helps separate out parameter validation from execution)

For example, what happens if you supply a null |cert|, or an empty trust_store,
etc.

[mattm, 2016/05/04 23:21:16]

I like the idea. For now I changed it so the constructor takes the required args
and then there are setters for the optional ones. (Right now just CertSources.)

+  ~CertPathBuilder();
+
+  // Begins verification of |cert|. If the return value is ERR_IO_PENDING,
+  // |callback| will be called asynchronously with the result. Otherwise, the
+  // result is returned synchronously and |callback| is not called.
+  int Run(const CompletionCallback& callback);
+
+ private:
+  enum State {
+    STATE_NONE,
+    STATE_GET_NEXT_PATH,
+    STATE_GET_NEXT_PATH_COMPLETE,
+  };
+
+  int DoLoop(int result);
+
+  int DoGetNextPath();
+  void HandleGotNextPath(int result);
+  int DoGetNextPathComplete(int result);
+
+  void AddResultPath(const CertVector& path, bool result);
+
+  CompletionCallback callback_;
+
+  std::unique_ptr<CertPathIter> cert_path_iter_;
+  const TrustStore&
+      trust_store_;  // XXX should be a pointer to signify we don't own it?
+  const SignaturePolicy* signature_policy_;
+  const der::GeneralizedTime time_;
+
+  CertVector next_path_;
+  State next_state_;
+
+  Result* out_result_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertPathBuilder);
+};
+
+}  // namespace net
+
+#endif  // NET_CERT_INTERNAL_PATH_BUILDER_H_