Cleanup HPKP error handling and weak DH key handling

net/url_request/url_request_http_job.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/url_request/url_request_http_job.h"
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 1065 matching lines @@
       }
     }
     if (transaction_ && transaction_->GetResponseInfo()) {
       LogChannelIDAndCookieStores(request_->url(), request_->context(),
                                   transaction_->GetResponseInfo()->ssl_info);
     }
 
     SaveCookiesAndNotifyHeadersComplete(OK);
   } else if (IsCertificateError(result)) {
     // We encountered an SSL certificate error.
-    if (result == ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY ||
-        result == ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN) {
-      // These are hard failures. They're handled separately and don't have
-      // the correct cert status, so set it here.
-      SSLInfo info(transaction_->GetResponseInfo()->ssl_info);
-      info.cert_status = MapNetErrorToCertStatus(result);
-      NotifySSLCertificateError(info, true);
-    } else {
-      // Maybe overridable, maybe not. Ask the delegate to decide.
-      TransportSecurityState* state = context->transport_security_state();
-      const bool fatal =
-          state && state->ShouldSSLErrorsBeFatal(request_info_.url.host());
-      NotifySSLCertificateError(
-          transaction_->GetResponseInfo()->ssl_info, fatal);
-    }
+    // Maybe overridable, maybe not. Ask the delegate to decide.
+    TransportSecurityState* state = context->transport_security_state();

[estark, 2016/06/21 00:29:24]

Is this guaranteed to be non-null? The old code had a check for null |state|
before using it.

[Ryan Sleevi, 2016/06/21 00:39:04]

Yes. See the CL this depends on

In particular,
https://codereview.chromium.org/2067843003/diff/270001/net/socket/ssl_client_...
and
https://codereview.chromium.org/2067843003/diff/270001/net/quic/crypto/proof_...

TransportSecurityState's actually been required for quite some time -
specifically, since
https://chromium.googlesource.com/chromium/src/+/b1c988bcd7869765e1bd56e59278...

This was just an artifact that wasn't caught during that cleanup.

[estark, 2016/06/21 00:49:06]

Ahh, I see, I missed that that had landed. Thanks. Still lgtm.

+    NotifySSLCertificateError(
+        transaction_->GetResponseInfo()->ssl_info,
+        state->ShouldSSLErrorsBeFatal(request_info_.url.host()));
   } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
     NotifyCertificateRequested(
         transaction_->GetResponseInfo()->cert_request_info.get());
   } else {
     // Even on an error, there may be useful information in the response
     // info (e.g. whether there's a cached copy).
     if (transaction_.get())
       response_info_ = transaction_->GetResponseInfo();
     NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED, result));
   }
@@ skipping 577 matching lines @@
   // Notify NetworkQualityEstimator.
   if (request()) {
     NetworkQualityEstimator* network_quality_estimator =
         request()->context()->network_quality_estimator();
     if (network_quality_estimator)
       network_quality_estimator->NotifyURLRequestDestroyed(*request());
   }
 }
 
 }  // namespace net