Don't process HSTS/HPKP headers when host is an IP address

Don't process HSTS/HPKP headers when host is an IP address

HSTS/HPKP headers should only be parsed when the host is not an IP
address. This change requires fixing the HSTS/HPKP tests to use
localhost test server URLs instead of 127.0.0.1, with a corresponding
cert.

BUG=456712
Committed: https://crrev.com/8488b5886ccec4820578905acd42f95cf42f5b17
Cr-Commit-Position: refs/heads/master@{#323913}

Committed: https://crrev.com/a5da7670ab3e46bfc6c0bc0e3d73c9d7eb835ed9
Cr-Commit-Position: refs/heads/master@{#324359}

[estark, 2015-04-03 19:41:16]

estark@chromium.org changed reviewers:
+ palmer@chromium.org, rsleevi@chromium.org

[estark, 2015-04-03 19:41:16]

Chris, Ryan, could you please take a look? This is the simplified approach to
crbug.com/456712 where I'm just using localhost for the HSTS/HPKP tests.

In follow-up CLs I'll apply some of the cleanup from crrev.com/1058573002 (using
MockCertVerifiers, fixing tests that hardcode 127.0.0.1).

[palmer, 2015-04-03 21:04:51]

LGTM. Thanks!

https://codereview.chromium.org/1059303002/diff/40001/net/data/ssl/scripts/ge...
File net/data/ssl/scripts/generate-test-certs.sh (right):

https://codereview.chromium.org/1059303002/diff/40001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-test-certs.sh:16: "$@" || exit 1
Idea (take it or leave it): Unify try and quiet_try:

try() {
  "$@" || (echo "$@" > /dev/stderr && exit 1)
}

https://codereview.chromium.org/1059303002/diff/40001/net/test/spawned_test_s...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/1059303002/diff/40001/net/test/spawned_test_s...
net/test/spawned_test_server/base_test_server.cc:37: // For
CERT_MISMATCHED_NAME, return a different hostname string
Nit: It's good to delimit identifiers in comments with |...|: "For
|CERT_MISMATCHED_NAME|, return a...".

[estark, 2015-04-03 21:22:31]

Thanks Chris.

https://codereview.chromium.org/1059303002/diff/40001/net/data/ssl/scripts/ge...
File net/data/ssl/scripts/generate-test-certs.sh (right):

https://codereview.chromium.org/1059303002/diff/40001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-test-certs.sh:16: "$@" || exit 1
On 2015/04/03 21:04:51, palmer wrote:
> Idea (take it or leave it): Unify try and quiet_try:
> 
> try() {
>   "$@" || (echo "$@" > /dev/stderr && exit 1)
> }

Done.

https://codereview.chromium.org/1059303002/diff/40001/net/test/spawned_test_s...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/1059303002/diff/40001/net/test/spawned_test_s...
net/test/spawned_test_server/base_test_server.cc:37: // For
CERT_MISMATCHED_NAME, return a different hostname string
On 2015/04/03 21:04:51, palmer wrote:
> Nit: It's good to delimit identifiers in comments with |...|: "For
> |CERT_MISMATCHED_NAME|, return a...".

Done.

[estark, 2015-04-03 21:27:36]

suggestion from Chris to retain exit code of a failed command in |try|

[Ryan Sleevi, 2015-04-04 00:35:24]

Mostly LGTM, but it wouldn't be a review w/o nits.

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/e...
File net/data/ssl/scripts/ee.cnf (right):

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/e...
net/data/ssl/scripts/ee.cnf:41: [req_extensions_none]
Nah, we need a san of localhost.

Recall that I'm trying to kill CN ( crbug.com/308330 , should make a move soon),
so let's make sure our cert matches our desired goal

[req_localhost_san]
subjectAltName = DNS:localhost

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-cross-signed-certs.sh (right):

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-cross-signed-certs.sh:22: "$@" || (e=$?; echo "$@"
> /dev/stderr && exit $e)
bash-dog says "I have no idea what's this doing" (Guessing it mutes output
unless there's an error, in which case, it spits out the command and then passes
the error code through?)

go back to try()? *shrug*

https://codereview.chromium.org/1059303002/diff/100001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/1059303002/diff/100001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:67: CERT_COMMON_NAME_IS_DOMAIN,
So unlike 57-63, this isn't an error case.

How about moving this block to line 52, and renaming it something like

CERT_OK_FOR_LOCALHOST


To indicate it's a valid cert and such.

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5269: std::string test_server_hostname =
https_test_server.GetURL("").host();
s/""/std::string()

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5299: // Make sure this test fails if
the test server is changed to not
newline between 5298/5299

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5301:
ASSERT_TRUE(https_test_server.GetURL("").HostIsIPAddress());
std::string

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5302: std::string test_server_hostname =
https_test_server.GetURL("").host();
ditto

[estark, 2015-04-04 00:40:18]

Thanks rsleevi, quick question inline before I address your nits.

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-cross-signed-certs.sh (right):

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-cross-signed-certs.sh:22: "$@" || (e=$?; echo "$@"
> /dev/stderr && exit $e)
On 2015/04/04 00:35:24, Ryan Sleevi wrote:
> bash-dog says "I have no idea what's this doing" (Guessing it mutes output
> unless there's an error, in which case, it spits out the command and then
passes
> the error code through?)
> 
> go back to try()? *shrug*

So you like try/quiet_try better? My goal is to call do `blah openssl x509 -text
> ..." which means that |blah| can't print to stdout without messing up the
output file.

[Ryan Sleevi, 2015-04-04 00:46:43]

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-cross-signed-certs.sh (right):

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-cross-signed-certs.sh:22: "$@" || (e=$?; echo "$@"
> /dev/stderr && exit $e)
On 2015/04/04 00:40:18, emily wrote:
> So you like try/quiet_try better? My goal is to call do `blah openssl x509
-text
> > ..." which means that |blah| can't print to stdout without messing up the
> output file.

Oh, right, fair point.

Mostly my comment was alternatively phrased "I have no idea what this is doing,
so I'm relying on palmer, who I know has validated that this is valid sh syntax
per line 1 and not some crazy dialect of zsh/bash/fooblahwhateversh" and "I
really like consistency in these scripts, and this changes the expectations for
what 'try' means, so maybe we shouldn't do it, or, if we're going to do it,
update the other scripts to be consistent'

[palmer, 2015-04-04 01:48:56]

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-cross-signed-certs.sh (right):

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-cross-signed-certs.sh:22: "$@" || (e=$?; echo "$@"
> /dev/stderr && exit $e)
Yes, this is all standard POSIX shell syntax, no modern day crazyshell magic. It
means:

run the args as a command

if the exit code was not 0, start a subshell

in which we save the exit code,
print the failed command to stderr,
and if printing succeeded, exit with the exit code the command exited with,
rather than an arbitrary choice like 1 (else exit with echo's error code!)

That said, I would do:

    "$@" || (e=$?; echo "$@" > /dev/stderr; exit $e)

I.e. replace the "&&" with ";". That way, even if echo were to fail for some
reason :) we'd still exit with the saved error code.

We are deep into the nit-weeds on a Friday, and it makes me feel giddy

[estark, 2015-04-06 16:41:13]

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/e...
File net/data/ssl/scripts/ee.cnf (right):

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/e...
net/data/ssl/scripts/ee.cnf:41: [req_extensions_none]
On 2015/04/04 00:35:24, Ryan Sleevi wrote:
> Nah, we need a san of localhost.
> 
> Recall that I'm trying to kill CN ( crbug.com/308330 , should make a move
soon),
> so let's make sure our cert matches our desired goal
> 
> [req_localhost_san]
> subjectAltName = DNS:localhost

Done.

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-cross-signed-certs.sh (right):

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-cross-signed-certs.sh:22: "$@" || (e=$?; echo "$@"
> /dev/stderr && exit $e)
On 2015/04/04 01:48:56, palmer wrote:
> That said, I would do:
> 
>     "$@" || (e=$?; echo "$@" > /dev/stderr; exit $e)
> 
> I.e. replace the "&&" with ";". That way, even if echo were to fail for some
> reason :) we'd still exit with the saved error code.

Done.

https://codereview.chromium.org/1059303002/diff/100001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-cross-signed-certs.sh:22: "$@" || (e=$?; echo "$@"
> /dev/stderr && exit $e)
On 2015/04/04 00:46:43, Ryan Sleevi wrote:
> <snip> or, if we're going to do it,
> update the other scripts to be consistent'

Done.

https://codereview.chromium.org/1059303002/diff/100001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/1059303002/diff/100001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:67: CERT_COMMON_NAME_IS_DOMAIN,
On 2015/04/04 00:35:24, Ryan Sleevi wrote:
> So unlike 57-63, this isn't an error case.
> 
> How about moving this block to line 52, and renaming it something like
> 
> CERT_OK_FOR_LOCALHOST
> 
> 
> To indicate it's a valid cert and such.

Done.

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5269: std::string test_server_hostname =
https_test_server.GetURL("").host();
On 2015/04/04 00:35:24, Ryan Sleevi wrote:
> s/""/std::string()

Done.

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5299: // Make sure this test fails if
the test server is changed to not
On 2015/04/04 00:35:24, Ryan Sleevi wrote:
> newline between 5298/5299

Done.

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5301:
ASSERT_TRUE(https_test_server.GetURL("").HostIsIPAddress());
On 2015/04/04 00:35:24, Ryan Sleevi wrote:
> std::string

Done.

https://codereview.chromium.org/1059303002/diff/100001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5302: std::string test_server_hostname =
https_test_server.GetURL("").host();
On 2015/04/04 00:35:24, Ryan Sleevi wrote:
> ditto

Done.

[estark, 2015-04-06 16:41:39]

The CQ bit was checked by estark@chromium.org

[estark, 2015-04-06 16:41:39]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1059303002/#ps160001
(title: "style fixes and other tweaks")

[commit-bot: I haz the power, 2015-04-06 16:41:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059303002/160001

[commit-bot: I haz the power, 2015-04-06 17:31:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-06 17:36:29]

Patchset 9 (id:??) landed as
https://crrev.com/8488b5886ccec4820578905acd42f95cf42f5b17
Cr-Commit-Position: refs/heads/master@{#323913}

[estark, 2015-04-06 19:54:56]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1066613002/ by estark@chromium.org.

The reason for reverting is: This change seems to break WebSockets tests broken
on Mac:
https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/758

Tests need to be fixed before it can be re-landed..

[estark, 2015-04-06 20:15:59]

rsleevi, palmer: is the cause of this test failure obvious to either of you?
https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/758

I'm looking into it but thought I'd check in case either of you can diagnose it
in 5 seconds or something. :)

[estark, 2015-04-07 00:34:41]

Patchset #10 (id:180001) has been deleted

[estark, 2015-04-07 00:50:46]

Okay, so it turns out this test failure is probably due to a Mac 10.6
bug/weirdness where |getaddrinfo("localhost", 0)| just fails:
https://bugs.python.org/issue20605, https://www.ruby-forum.com/topic/207745

I uploaded a patchset with an ugly workaround (the python test server will
insist on using 127.0.0.1 instead of localhost when creating websockets servers
with port=0). Could you please take a look and let me know if anything better
comes to mind, rsleevi/palmer?

[estark, 2015-04-08 22:19:54]

On 2015/04/07 00:50:46, emily wrote:
> Okay, so it turns out this test failure is probably due to a Mac 10.6
> bug/weirdness where |getaddrinfo("localhost", 0)| just fails:
> https://bugs.python.org/issue20605, https://www.ruby-forum.com/topic/207745
> 
> I uploaded a patchset with an ugly workaround (the python test server will
> insist on using 127.0.0.1 instead of localhost when creating websockets
servers
> with port=0). Could you please take a look and let me know if anything better
> comes to mind, rsleevi/palmer?

ping -- anyone have objections to or suggestions about avoiding
getaddrinfo("localhost", 0) when spawning the test server? This is the hacky
workaround in question:
https://codereview.chromium.org/1059303002/diff2/200001:220001/net/tools/test...

[palmer, 2015-04-08 23:01:49]

Your workaround seems OK to me.

[Ryan Sleevi, 2015-04-09 00:35:43]

On 2015/04/08 23:01:49, palmer wrote:
> Your workaround seems OK to me.

Sorry for the delay - ditto. Thanks for digging in.

[estark, 2015-04-09 00:38:07]

The CQ bit was checked by estark@chromium.org

[estark, 2015-04-09 00:38:07]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1059303002/#ps220001
(title: "ugly workaround for mac 10.6 getaddrinfo bug")

[commit-bot: I haz the power, 2015-04-09 00:39:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059303002/220001

[commit-bot: I haz the power, 2015-04-09 04:00:23]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-04-09 04:01:20]

Patchset 11 (id:??) landed as
https://crrev.com/a5da7670ab3e46bfc6c0bc0e3d73c9d7eb835ed9
Cr-Commit-Position: refs/heads/master@{#324359}