Expose when PKP is bypassed in SSLInfo.

net/spdy/spdy_session.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/spdy/spdy_session.h"
 
 #include <algorithm>
 #include <limits>
 #include <map>
 #include <utility>
@@ skipping 636 matching lines @@
   if (ssl_info.channel_id_sent &&
       ChannelIDService::GetDomainForHost(new_hostname) !=
       ChannelIDService::GetDomainForHost(old_hostname)) {
     return false;
   }
 
   bool unused = false;
   if (!ssl_info.cert->VerifyNameMatch(new_hostname, &unused))
     return false;
 
+  // Pinning is bypassed for local roots.
+  if (!ssl_info.is_issued_by_known_root)
+    return true;

[Ryan Sleevi, 2016/06/09 19:17:32]

FWIW, and for better or worse, it was intentional this was omitted, but that
should have been documented.

The thinking at the time was that it's OK for things to be slower if the user is
MITM'ing themselves; that is, you don't get "the performance of pooling for
pinned hosts"

I'm also a little nervous about this conditional being before the pinning check,
rather than being a part of it. This conditional makes it too easy to
short-circuit security checks added later (e.g. as part of line 672)

[davidben, 2016/06/09 19:19:15]

I believe this aligns with the old behavior, but I agree I really don't like
having callers need to care about this. My suggestion (below) was that
CheckPublicKeyPins should either return an enum or return the same boolean as
before and then have a separate output parameter for the diagnositic
information.

[dadrian, 2016/06/09 21:58:57]

Done.

+
   std::string pinning_failure_log;
   // DISABLE_PIN_REPORTS is set here because this check can fail in
   // normal operation without being indicative of a misconfiguration or
   // attack. Port is left at 0 as it is never used.
   if (!transport_security_state->CheckPublicKeyPins(

[davidben, 2016/06/09 15:56:32]

I think it'd be better for this to return an enum. From all three callers, it
looks like it's never correct for the caller of CheckPublicKeyPins to not
special-case is_issued_by_known_root which means this function effective returns
a tri-state. An enum means we're less likely to accidentally omit the check
somewhere.

(In fact, it's a little fishy that this function significantly changed the
semantics of CheckPublicKeyPins from "is this stuff okay for this name?" to "are
the pins valid, independent of whether we'd care" without changing the
documentation of the function.)

[davidben, 2016/06/09 15:58:05]

Another possibility is maybe this returns the old bool (since that's the
important high-order bit. "May I proceed?") and then has some other output
parameter for the diagnostic "PS: it's okay, but it's because I bypassed stuff"
info.

[dadrian, 2016/06/09 16:39:40]

It looks like the current documentation already matches the new behavior. It
implies that the function only looks at key hashes.

           HostPortPair(new_hostname, 0), ssl_info.is_issued_by_known_root,
           ssl_info.public_key_hashes, ssl_info.unverified_cert.get(),
           ssl_info.cert.get(), TransportSecurityState::DISABLE_PIN_REPORTS,
           &pinning_failure_log)) {
     return false;
   }
 
   return true;
 }
 
@@ skipping 2717 matching lines @@
     if (!queue->empty()) {
       SpdyStreamId stream_id = queue->front();
       queue->pop_front();
       return stream_id;
     }
   }
   return 0;
 }
 
 }  // namespace net