Certificate path builder for new certificate verification library

net/cert/internal/path_builder.h

Index: net/cert/internal/path_builder.h
diff --git a/net/cert/internal/path_builder.h b/net/cert/internal/path_builder.h
new file mode 100644
index 0000000000000000000000000000000000000000..2d33da360f6b2162b15c38c73a04a32687d6e452
--- /dev/null
+++ b/net/cert/internal/path_builder.h
@@ -0,0 +1,128 @@
+// 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 <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/completion_status.h"
+#include "net/der/input.h"
+#include "net/der/parse_values.h"
+
+namespace net {
+
+namespace der {
+struct GeneralizedTime;
+}
+
+class CertPathIter;
+class CertIssuerSource;
+class ParsedCertificate;
+class TrustStore;
+class SignaturePolicy;
+
+class NET_EXPORT CertPathBuilder {

[eroman, 2016/06/17 01:03:22]

Please provide some documentation on the role of this class.

[mattm, 2016/06/18 04:28:56]

Done.

+ public:
+  struct NET_EXPORT ResultPath {

[eroman, 2016/06/17 01:03:22]

Please document

[mattm, 2016/06/18 04:28:56]

Done.

+    ResultPath();
+    ~ResultPath();
+
+    std::vector<scoped_refptr<ParsedCertificate>> path;

[eroman, 2016/06/17 01:03:23]

Please document order, and where the target certificate and trust anchor appear.

[mattm, 2016/06/18 04:28:56]

Done.

+    int rv;

[eroman, 2016/06/17 01:03:23]

Please document the domain of this error.

[mattm, 2016/06/18 04:28:56]

Done.

+  };
+
+  struct NET_EXPORT Result {
+    Result();
+    ~Result();
+
+    // Returns the overall result.
+    int result() {

[eroman, 2016/06/17 01:03:23]

const?

[mattm, 2016/06/18 04:28:56]

Done.

+      if (paths.empty())

[eroman, 2016/06/17 01:03:23]

Would it be worthwhile to instead use:

if (best_result_index >= paths.size())

[mattm, 2016/06/18 04:28:56]

My thought was it's a bug if paths is not empty but best_result_index_ doesn't
have a valid value, so doing the test that way just hides the bug..

+        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.

[eroman, 2016/06/17 01:03:22]

or should the recommendation be best_result_index < paths.size() ?

+    size_t best_result_index = 0;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Result);
+  };
+
+  // TODO(mattm): allow caller specified hook/callback to extend path
+  // verification.
+  //
+  // Creates a CertPathBuilder that attempts to find a path from |cert| to a
+  // trust anchor in |trust_store|, which satisfies |signature_policy| and is
+  // valid at |time|.  Details of attempted path(s) are stored in |*result|.
+  //
+  // The caller must keep |trust_store|, |signature_policy|, and |*result| valid
+  // for the lifetime of the CertPathBuilder.
+  CertPathBuilder(scoped_refptr<ParsedCertificate> cert,
+                  const TrustStore* trust_store,
+                  const SignaturePolicy* signature_policy,
+                  const der::GeneralizedTime& time,
+                  Result* result);
+  ~CertPathBuilder();
+
+  // Adds a CertIssuerSource to provide intermediates for use in path building.
+  // Multiple sources may be added. Must not be called after Run is called.  If
+  // no sources are added, the target cert will only verify if it is a trust
+  // anchor or is directly signed by a trust anchor. The |*cert_issuer_source|
+  // must remain valid for the lifetime of the CertPathBuilder.
+  void AddCertIssuerSource(CertIssuerSource* cert_issuer_source);

[eroman, 2016/06/17 01:03:22]

todo: interface is a bit squirly (destruction, completion, don't call twice,
etc.)

[mattm, 2016/06/18 04:28:56]

Is this a leftover comment? since AddCertIssuerSource can be called twice..

[eroman, 2016/06/20 19:54:19]

Yes sorry, disregard this.

I was a TODO for myself when reviewing, as I didn't understand how everything
fit together. I tend to do a quick pass through CLs leaving notes for myself on
what to look at in more detail, and then do another pass to resolve what I
wanted to dig deeper on.

In this case I forgot to delete this before publishing :)

+
+  // Begins verification of |cert|. If the return value is SYNC then the

[eroman, 2016/06/17 01:03:23]

nit: It is unclear what |cert| is here (it was the |cert| passed in to ctor).

[mattm, 2016/06/18 04:28:56]

Done.

+  // verification is complete and the |result| value can be inspected for the
+  // status, and |callback| will not be called.
+  // If the return value is ASYNC, the |callback| will be called asynchronously
+  // once the verification is complete. |result| should not be examined or
+  // modified until the |callback| is run.
+  // If |callback| is null, verification always completes synchronously, even if
+  // it fails to find a valid path and one could have been found asynchronously.
+  CompletionStatus Run(const base::Closure& callback);

[eroman, 2016/06/17 01:03:22]

Please also mention somewhere:
  * The cancellation semantics -- is it ok to delete CertPathBuilder while a
request is in progress
  * That this is not intended for multiple requests (Run() should not be called
multiple times).

[mattm, 2016/06/18 04:28:56]

Done.

+
+ private:
+  enum State {
+    STATE_NONE,
+    STATE_GET_NEXT_PATH,
+    STATE_GET_NEXT_PATH_COMPLETE,
+  };
+
+  CompletionStatus DoLoop(bool allow_async);
+
+  CompletionStatus DoGetNextPath(bool allow_async);
+  void HandleGotNextPath();
+  CompletionStatus DoGetNextPathComplete();
+
+  void AddResultPath(const std::vector<scoped_refptr<ParsedCertificate>>& path,
+                     bool result);
+
+  base::Closure callback_;
+
+  std::unique_ptr<CertPathIter> cert_path_iter_;
+  const TrustStore* trust_store_;
+  const SignaturePolicy* signature_policy_;
+  const der::GeneralizedTime time_;
+
+  std::vector<scoped_refptr<ParsedCertificate>> next_path_;
+  State next_state_;
+
+  Result* out_result_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertPathBuilder);
+};
+
+}  // namespace net
+
+#endif  // NET_CERT_INTERNAL_PATH_BUILDER_H_