Clean up comments and code for pin validation.

Clean up comments and code for pin validation.

It should be crystal clear when, and why, pin validation is and is not
performed.

TEST=net_unittests; with an OFFICIAL_BUILD: can still connect to pinned
sites, and https://pinningtest.appspot.com fails with
net::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192654

[palmer, 2013-04-02 23:12:56]

git cl upload early and often --- I haven't tried this on a Chrome build yet
(WFH on Chromebook; will run on Linux and Mac tomorrow), but I wanted to see if
you guys think it's a good change at all. I think it's much more clear, with
less nesting and if-statement bafflement.

[Ryan Sleevi, 2013-04-02 23:28:21]

https://codereview.chromium.org/13466020/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/13466020/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3466: if
(TransportSecurityState::IsBuildTimely() &&
I'm not so sure about moving this clause out, as it will prevent dynamic pins
from working.

[palmer, 2013-04-03 00:07:05]

> (TransportSecurityState::IsBuildTimely() &&
> I'm not so sure about moving this clause out, as it will prevent dynamic pins
> from working.

I don't believe this CL changes *the current semantics*. Do you think it does?
I.e., I think we are already preventing dynamic pins from working on old builds.

But yes, we should consider, as a separate issue, whether build oldness means we
should either (a) not perform pin validation at all (failing open); or (b)
filter out preloaded (hence outdated) pins from the pin sets when performing pin
validation.

[palmer, 2013-04-03 19:16:42]

Tried the manual test on my build, and it works.

Please let me know what you think! Thanks.

[Ryan Sleevi, 2013-04-03 19:33:10]

https://codereview.chromium.org/13466020/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/13466020/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3466: if
(TransportSecurityState::IsBuildTimely() &&
The only thing I'd do is move the IsBuildTimely() check to the last of the
conditional, since everything above it is just an integer comparison, and the
function naming "suggests" it may be an expensive (eg: non-trivial) call.

This is just pedantry though.

https://codereview.chromium.org/13466020/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3485:
UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", true);
This is going to start artificially increasing this #.

In the previous code, we only UMA' success d on HasPublicKeyPins . You now UMA
it unconditionally (CheckPublicKeyPins returns true even if there are no pins)

[palmer, 2013-04-04 20:56:33]

https://codereview.chromium.org/13466020/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/13466020/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3466: if
(TransportSecurityState::IsBuildTimely() &&
> This is just pedantry though.

You know I like pedantry. Done.

https://codereview.chromium.org/13466020/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3485:
UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", true);
> This is going to start artificially increasing this #.
> 
> In the previous code, we only UMA' success d on HasPublicKeyPins . You now UMA
> it unconditionally (CheckPublicKeyPins returns true even if there are no pins)

Oops. Fixed.

[Ryan Sleevi, 2013-04-04 21:24:02]

https://codereview.chromium.org/13466020/diff/11001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/13466020/diff/11001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:3465:
server_cert_verify_result_.public_key_hashes)) {
nit: The original form matches our common pattern of "Handle errors first" - the
point being so that errors are close to their conditionals.

[palmer, 2013-04-04 21:34:53]

https://codereview.chromium.org/13466020/diff/11001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/13466020/diff/11001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:3465:
server_cert_verify_result_.public_key_hashes)) {
On 2013/04/04 21:24:02, Ryan Sleevi wrote:
> nit: The original form matches our common pattern of "Handle errors first" -
the
> point being so that errors are close to their conditionals.

Done.

[Ryan Sleevi, 2013-04-05 21:36:37]

lgtm

[commit-bot: I haz the power, 2013-04-05 21:48:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/13466020/22001

[commit-bot: I haz the power, 2013-04-05 23:54:47]

Change committed as 192654