Do not do Expect-Staple when OCSPVerifyResult has not been populated

net/http/transport_security_state.cc

 // Copyright (c) 2012 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.
 
 #include "net/http/transport_security_state.h"
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 #include <vector>
@@ skipping 627 matching lines @@
   }
 
   return found;
 }
 
 // Serializes an OCSPVerifyResult::ResponseStatus to a string enum, suitable for
 // the |response-status| field in an Expect-Staple report.
 std::string SerializeExpectStapleResponseStatus(
     OCSPVerifyResult::ResponseStatus status) {
   switch (status) {
+    case OCSPVerifyResult::UNKNOWN:
+      // Reports shouldn't be sent for this response status.
+      NOTREACHED();
+      return "UNKNOWN";
     case OCSPVerifyResult::MISSING:
       return "MISSING";
     case OCSPVerifyResult::PROVIDED:
       return "PROVIDED";
     case OCSPVerifyResult::ERROR_RESPONSE:
       return "ERROR_RESPONSE";
     case OCSPVerifyResult::BAD_PRODUCED_AT:
       return "BAD_PRODUCED_AT";
     case OCSPVerifyResult::NO_MATCHING_RESPONSE:
       return "NO_MATCHING_RESPONSE";
     case OCSPVerifyResult::INVALID_DATE:
       return "INVALID_DATE";
     case OCSPVerifyResult::PARSE_RESPONSE_ERROR:
       return "PARSE_RESPONSE_ERROR";
     case OCSPVerifyResult::PARSE_RESPONSE_DATA_ERROR:
       return "PARSE_RESPONSE_DATA_ERROR";
   }
   return std::string();

[Ryan Sleevi, 2016/12/21 01:35:29]

nit: Any reason there's not a NOTREACHED() here as well?

[estark, 2016/12/21 17:53:12]

Not that I can see, added one.

 }
 
 // Serializes an OCSPRevocationStatus to a string enum, suitable for the
 // |cert-status| field in an Expect-Staple report.
 std::string SerializeExpectStapleRevocationStatus(
     const OCSPRevocationStatus& status) {
   switch (status) {
     case OCSPRevocationStatus::GOOD:
       return "GOOD";
     case OCSPRevocationStatus::REVOKED:
@@ skipping 120 matching lines @@
     return;
   }
 
   // Determine if the host is on the Expect-Staple preload list. If the build is
   // not timely (i.e. the preload list is not fresh), this will fail and return
   // false.
   ExpectStapleState expect_staple_state;
   if (!GetStaticExpectStapleState(host_port_pair.host(), &expect_staple_state))
     return;
 
-  // No report needed if a stapled OCSP response was provided.
-  if (ssl_info.ocsp_result.response_status == OCSPVerifyResult::PROVIDED &&
-      ssl_info.ocsp_result.revocation_status == OCSPRevocationStatus::GOOD) {
+  // No report needed if OCSP details were not checked on this
+  // connection, or if a stapled OCSP response was provided.
+  if (ssl_info.ocsp_result.response_status == OCSPVerifyResult::UNKNOWN ||
+      (ssl_info.ocsp_result.response_status == OCSPVerifyResult::PROVIDED &&
+       ssl_info.ocsp_result.revocation_status == OCSPRevocationStatus::GOOD)) {

[Ryan Sleevi, 2016/12/21 01:35:29]

suggestion: I would actually break these into two conditionals, since it's
logically two different criteria, even if it's the same behaviour

// No report needed if OCSP details were not checked on this connection.
if (ssl_info.ocsp_result.response_status == OCSPVerifyResult::UNKNOWN)
  return;

// No report needed if a stapled OCSP response was provided and it was valid.
if (...)
  return;


(The above nit is a comment on the comment - it's not just stapling - it's a
valid stapled response)

[estark, 2016/12/21 17:53:12]

Done.

     return;
   }
 
   std::string serialized_report;
   if (!SerializeExpectStapleReport(host_port_pair, ssl_info, ocsp_response,
                                    &serialized_report)) {
     return;
   }
   report_sender_->Send(expect_staple_state.report_uri,
                        "application/json; charset=utf-8", serialized_report,
@@ skipping 826 matching lines @@
 TransportSecurityState::PKPStateIterator::PKPStateIterator(
     const TransportSecurityState& state)
     : iterator_(state.enabled_pkp_hosts_.begin()),
       end_(state.enabled_pkp_hosts_.end()) {
 }
 
 TransportSecurityState::PKPStateIterator::~PKPStateIterator() {
 }
 
 }  // namespace