Implement Expect-Staple

net/cert/expect_staple_report.h

Index: net/cert/expect_staple_report.h
diff --git a/net/cert/expect_staple_report.h b/net/cert/expect_staple_report.h
new file mode 100644
index 0000000000000000000000000000000000000000..d5176f989991f143879c012c419f74688cc21213
--- /dev/null
+++ b/net/cert/expect_staple_report.h
@@ -0,0 +1,73 @@
+// 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_EXPECT_STAPLE_REPORT_H
+#define NET_CERT_EXPECT_STAPLE_REPORT_H
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/time/time.h"
+#include "net/base/net_export.h"
+#include "net/cert/internal/parse_ocsp.h"
+#include "net/cert/x509_certificate.h"
+
+namespace net {
+
+// An ExpectStapleReport is used to determine if a stapled OCSP response is
+// valid for a given certificate, and contains all the information needed to
+// construct a report payload for sites opting into Expect-Staple.
+class NET_EXPORT ExpectStapleReport {
+ public:
+  ExpectStapleReport();
+  ~ExpectStapleReport();
+
+  // Stores the validity of a single stapled response.

[Ryan Sleevi, 2016/06/17 16:19:54]

From simply reading the header, it's non-obvious why there's a vector of
SingleResults here. If SingleResult applies to the entire stapled response,
there should be only one, presumably - we have no plans to support
multi-stapling.

+  struct SingleResult {

[Ryan Sleevi, 2016/06/17 16:19:54]

What is the API if a SingleResult couldn't be parsed?

[dadrian, 2016/06/17 23:17:36]

Not currently exposed cleanly, I added some comments to the the Expect-Staple
design doc with thoughts on how to expose errors.
https://docs.google.com/document/d/1aISglJIIwglcOAhqNfK-2vtQl-_dWAapc-VLDh-9-BE/

+    bool is_date_valid = false;
+    bool is_correct_certificate = false;
+    OCSPCertStatus::Status status = OCSPCertStatus::Status::UNKNOWN;
+  };
+
+  // Represents where during the staple verification an error occurred.
+  enum class StapleError {
+    OK,
+    PARSE_RESPONSE,
+    BAD_RESPONSE,
+    PARSE_RESPONSE_DATA,
+    PARSE_SINGLE_RESPONSE,
+    NO_MATCHING_RESPONSE,
+  };
+
+  // Creates an ExpectStapleReport from an unparsed OCSP response.
+  // This compares the serial number of the certificate, and verifies that
+  // |verify_time| is within thisUpdate and nextUpdate, and that thisUpdate is
+  // at least as recent as |verify_time - max_age|.

[Ryan Sleevi, 2016/06/16 21:49:29]

DESIGN: This seems to violate the Single-Responsibility-Principle of SOLID
design.

The presumed single responsibility is being a container/abstraction for an OCSP
report. However, this method makes it do OCSP parsing/validation, signature
validation, and constructing a report. That seems well-outside the scope of
here.

From an API design, is it expected this will always be synchronous? In order to
validate OCSP responses, we need an asynchronous API presumably (to consider
trust status)

[svaldez, 2016/06/17 13:33:51]

Its unclear what the division should be otherwise, things like extracting the
individual OCSP SingleResponses and checking age should be included somewhere in
the ExpectStaple layer. 

It might make sense to have FromRawOCSPResponse take in an OCSPResponse instead
of a string, but I don't see a clear split for the rest.

At some point there should be a place where ExpectStaple receives an OCSP
response, and then after that point deals with parsing, validation, single
response extraction. Otherwise we'll need to pass an equivalent of an
ocsp_verify_result all over the place to figure out what the StapleError should
be.

This API should never be async, since trust status checking only is based on the
issuer and a single chain down from it, which we already have from the original
certificate verification.

[Ryan Sleevi, 2016/06/17 16:19:54]

I don't think that intrinsically argues it belongs as a static method on an
object though. Having a free function that handles this could be better, if we
expect the behaviour to be consistent.
Why isn't this part of cert verification? Since it's directly tied to the
suitability of it? Why doesn't CertVerifyResult keep track?

I do want to be clear my unease about having an ExpectStapleReport do OCSP
validation. To me, that's a clear layering - one is simply reporting/diagnostic,
the other is independent logic.

I would suggest chatting with davidben@ in person to get his thoughts, if only
because we were discussing it yesterday evening, but my gut is that 

OCSP parsing
OCSP processing
OCSP verification

All belong in CertVerification. And they'd give out additional details on the
CertVerifyResult, which could then be used to construct an ExpectStapleReport
and send it.
Unfortunately, on the platform APIs we use, the ability to trust a CA for certs
but NOT trust it for OCSP is a thing.

[dadrian, 2016/06/17 17:26:54]

At some point ExpectStaple needs to have validation for every stapled
SingleResponse, and be able to discern between various types of parsing or
validation failures. There's the question of should the OCSP layer contain all
that logic, and just hand ExpectStaple some object, which it can then serialize,
or if ExpectStaple should handle the whole things.

I'd say that these designs are effectively the same, with the only difference
being the name of the code that's currently in expect_staple_report.h. The
current ExpectStapleReport class doesn't actually do any reporting, it basically
just does OCSP validation with detailed error information. The actual
serialization and reporting takes place in TransportSecurityState, which is
exactly how PKP reporting works. The ExpectStapleReport class could just as
easily be named OCSPStaple, there's nothing particularly "report"-like about it,
rather than basic validation.

Then there's the question of how does the rest of the net stack interact with
this class? Right now it's structured so that the OCSP parsing only happens if
the server is in the preload list, and I think it makes sense to leave it that
way. Ideally, OCSP verification would be added to CertVerifyResult and take
place as part of the rest of certificate validation, but given that Expect
Staple is only reporting and only for a tiny subset of hosts, I don't think it
makes sense to add it to certificate verification, especially given that the
prerequisite code for even validating signatures on OCSP responses still hasn't
landed.

In summary, I think the big questions are:

1. What should the ExpectStapleReport class be called, and where should it go?
2. How should the ExpectStapleReport class be constructed?
 

[Ryan Sleevi, 2016/06/17 18:54:25]

That presupposes "ExpectStaple" is a thing. I'm challenging that notion of the
class design, at the fundamental level.

I'm suggesting, yes, that certificate verification is where we should capture
all of the OCSP-related verification logic, and we should surface sufficient
fidelity of information to explain the results.
 
I disagree on that, but apologies if my concerns are not clearer. Even if the
code performs the same logical tasks, the structure and design of it
significantly affect maintainability and testability.
Right, I consider that a bit of a smell - and I can't determine if it's solely
naming or if it's design, but I lean on the latter.
Whether or not that's a good thing is an open question on the other bug :)
Right, and I would push back on that design with similar concerns.
I don't, because we're already doing OCSP work in CertVerifier by supplying the
stapled response, which we're then passing to the crypto libraries (NSS and
Windows only, because the OS X code is a do-nothing stub through 10.11 but is
fixed in Sierra)
I'm not sure why you say that. Is there something I'm missing? This seems the
correct end goal, and is consistent with the short-term need.

(In general, I push back pretty hard whenever someone says "Temporary", because
it has yet to be so in my experience)
Without wanting to be too contrarian, given how much I've pushed back, I don't
think these are the right questions.

1) What is our end-goal for how OCSP fits in the network stack?
2) What is the necessary minimum implementation to meet ExpectStaple?

To me, #1 is clearly that OCSP stapling is integrated into certificate
verification - the CertVerifier interface was intentionally designed to
accommodate this. Our current implementations (NSS & Windows) don't allow us to
fill out diagnostic information about it - but there's no reason we can't use
our in-house OCSP parsing/validation code, across all implementations, to do
that.

The pivot point for adding such logic is the CertVerifyProc base-class, which
performs the pre-/post-(platform) verification tasks, so that all
implementations have consistent inputs/outputs to CVP::VerifyInternal. So adding
an OCSP verification step - presumably, after the OS verification logic - to
CVP::Verify is the right approach, signaling that data back up via
CertVerifyResult.

For reporting/ExpectStaple, the TSS maintains a database of what hosts expect
stapling (which Emily already did). The reporting - for Expect - can go a number
of places, depending on what the ExpectStaple semantics are meant to imply. If,
like PKP-RO, it triggers based on header processing, then that goes into
URLRequestHttpJob

Something like

UrlRequestHttpJob::OnWhatever[]
if (tss->ShouldExpectOCSPStapling(...) && !ssl_info.ocsp.was_awesome) {
  tss->SendExpectOCSPReport(ssl_info.ocsp, ...);
}

If ExpectStaple is meant to be preload-based only (no headers), then we'd need
to push it into the triumvirate, for now - SSLClientSocket,
SpdySession::CanPool, and ProofVerifierChromium - to send the results after
getting the CertVerifyResult from the CertVerifier. 

+  //
+  // TODO(dadrian): Check issuer and signatures. https://crbug.com/620005
+  static std::unique_ptr<ExpectStapleReport> FromRawOCSPResponse(
+      const std::string& raw_response,
+      const base::Time& verify_time,
+      const base::TimeDelta& max_age,
+      const X509Certificate& server_certificate);
+
+  const base::Time& verify_time() const { return verify_time_; }
+
+  StapleError staple_error() const { return staple_error_; }
+
+  const std::vector<SingleResult>& stapled_responses() const {
+    return stapled_responses_;
+  }
+
+ private:
+  base::Time verify_time_;
+  StapleError staple_error_;
+  std::vector<SingleResult> stapled_responses_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExpectStapleReport);
+};
+
+}  // namespace net
+
+#endif /* NET_CERT_EXPECT_STAPLE_REPORT_H */

[Ryan Sleevi, 2016/06/17 16:19:55]

STYLE: Two spaces and use C++ comments

#endif  // NET_CERT_EXPECT_STAPLE_REPORT_H_