Fix TransportSecurityState unittests to run in --single-process mode.

Fix TransportSecurityState unittests to run in --single-process mode.

Several of the TransportSecurityState unittests do not ASSERT that they are
able to parse a certificate. If parsing fails, they end up causing failures
with Expect-Staple reporting. This CL adds robustness checks by consistently
ASSERTing that certificates could be loaded from disk successfully. This is
the fix for diagnostics, but not the fix for root cause.

The root cause is that several test certificates duplicated the issuer and
serial number tuples, due to their organic growth and addition. On systems
using NSS to represent the OS certificate (e.g. Linux and ChromeOS), due to
NSS unfortunately being implemented assuming X.500's (never implemented)
uniqueness of issuer+serial tuples, if two certificates have the same tuple,
NSS will fail to parse the second certificate if the first certificate is
still in memory.

When running tests with --single-process, and having run a test that
successfully verifies a certificate using a test intermediate, NSS
unfortunately keeps a copy of the intermediate in memory until NSS itself is
unloaded. Since we never unload NSS, this intermediate is always kept around,
thus causing observable side-effects.

On the bots and under normal testing, this doesn't manifest, because the test
harness runs each test separately as needed, to ensure hermeticism. However,
some developers like to use --single-process for speed (less process spawning
overhead) or debugging, despite no bots running this config.

The duplicate serial number itself emerged from side-effects related to bash
functions and environment variables, hence why it was not initially spotted.

To properly resolve this issue, this change does the following:
  1) Fix the test generation script to not leak environment state.
    a) Fixes the duplicate serial number issue.
    b) Fixes incorrect subject names and issuer names due to env bleeding.
  2) Regenerate the test certificates to ensure serial uniqueness.
  3) Update the tests that have hardcoded aspects of the chain (PKP testing
     and name constraint testing).
  4) Add additional assertions to TransportSecurityState to fail quicker.
  5) Remove an unnecessary HPKP test chain that had been refactored away.

BUG=729341
Review-Url: https://codereview.chromium.org/2926463002
Cr-Commit-Position: refs/heads/master@{#477691}
Committed: https://chromium.googlesource.com/chromium/src/+/6ee44a7247c639c0703f291d320bdf05c1531b57

[Ryan Sleevi, 2017-06-05 19:02:02]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 19:02:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2017-06-05 19:04:12]

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

[Ryan Sleevi, 2017-06-05 19:04:14]

estark: For you to sanity check the TSS bits
mmenke: For content/browser/loader stampy
rch: FYI

https://codereview.chromium.org/2926463002/diff/20001/net/http/transport_secu...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/2926463002/diff/20001/net/http/transport_secu...
net/http/transport_security_state_unittest.cc:1961: ssl_info.cert = cert2;
These are all just correctness fixes

[mmenke, 2017-06-05 19:07:00]

On 2017/06/05 19:04:14, Ryan Sleevi wrote:
> estark: For you to sanity check the TSS bits
> mmenke: For content/browser/loader stampy
> rch: FYI
> 
>
https://codereview.chromium.org/2926463002/diff/20001/net/http/transport_secu...
> File net/http/transport_security_state_unittest.cc (right):
> 
>
https://codereview.chromium.org/2926463002/diff/20001/net/http/transport_secu...
> net/http/transport_security_state_unittest.cc:1961: ssl_info.cert = cert2;
> These are all just correctness fixes

content/browser/loader LGTM, stampity stamp.

[estark, 2017-06-05 19:11:17]

TSS LGTM

[Ryan Hamilton, 2017-06-05 19:16:43]

rch@chromium.org changed reviewers:
+ rch@chromium.org

[Ryan Hamilton, 2017-06-05 19:16:44]

Wow! Thanks for doing this. I think the fix to the script is subtle and I'm
missing it. Does "try()" vs "set -e" do the trick?

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

https://codereview.chromium.org/2926463002/diff/20001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-test-certs.sh:9: set -e -x
set -e? clever!

[Ryan Sleevi, 2017-06-05 19:19:40]

On 2017/06/05 19:16:44, Ryan Hamilton wrote:
> Wow! Thanks for doing this. I think the fix to the script is subtle and I'm
> missing it. Does "try()" vs "set -e" do the trick?

It's incredibly subtle :)

The environment bleed is a result of using the bash function, as the CL
description tries to catch. It only happens in SH compatibility mode. Dash and
POSIX sh don't do this.

"set -e -x" makes it error fast (and echo the commands), which is slightly
noiser but at least easier to debug.

We have several other scripts using the 'try' syntax, and I tried to keep CL
scope minimal - especially since I didn't want to re-generate all these certs ;)

Eric's got a new (Python-based) framework for writing the OpenSSL configs. I'm
both less and more of a fan of them - it's a mixed bag, really. So I figured I'd
chat with him before converting the rest over, and instead just do the surgical
fix.

[commit-bot: I haz the power, 2017-06-05 19:20:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 19:20:31]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Ryan Sleevi, 2017-06-05 19:25:44]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 19:26:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Hamilton, 2017-06-05 19:49:46]

On 2017/06/05 19:19:40, Ryan Sleevi wrote:
> On 2017/06/05 19:16:44, Ryan Hamilton wrote:
> > Wow! Thanks for doing this. I think the fix to the script is subtle and I'm
> > missing it. Does "try()" vs "set -e" do the trick?
> 
> It's incredibly subtle :)

That makes me feel better! :)

> The environment bleed is a result of using the bash function, as the CL
> description tries to catch. It only happens in SH compatibility mode. Dash and
> POSIX sh don't do this.

Hm. I'm still too dense to connect the dots. So the script runs with bash as
/bin/sh which puts bash into SH compat mode. It then runs a bunch of commands
via the try() bash function defined in the script. Does that cause something to
leak which doesn't leak when the commands are run directly by the script? If so,
two questions. 
1) What is the different behavior between function and command (does the
function automatically do something with the environment which the direct
command doesn't?)? 
2) In the particular case of these commands, what is leaking that causes
problems? I *though* the serial number when to disk so that one command would
update the serial for the next, so I'm guessing something other than serial is
leaking?
 
> "set -e -x" makes it error fast (and echo the commands), which is slightly
> noiser but at least easier to debug.
> 
> We have several other scripts using the 'try' syntax, and I tried to keep CL
> scope minimal - especially since I didn't want to re-generate all these certs
;)
> 
> Eric's got a new (Python-based) framework for writing the OpenSSL configs. I'm
> both less and more of a fan of them - it's a mixed bag, really. So I figured
I'd
> chat with him before converting the rest over, and instead just do the
surgical
> fix.

Heh, makes sense.

[Ryan Sleevi, 2017-06-05 19:56:04]

On 2017/06/05 19:49:46, Ryan Hamilton wrote:
> Hm. I'm still too dense to connect the dots. So the script runs with bash as
> /bin/sh which puts bash into SH compat mode. It then runs a bunch of commands
> via the try() bash function defined in the script. Does that cause something
to
> leak which doesn't leak when the commands are run directly by the script? If
so,
> two questions. 
> 1) What is the different behavior between function and command (does the
> function automatically do something with the environment which the direct
> command doesn't?)? 

Yes. It causes the environment variables specified for the scope of a single
command (e.g.: CA_NAME or SUBJECT_NAME) to leak into all subsequent invocations
'as if' they had been exported.

> 2) In the particular case of these commands, what is leaking that causes
> problems? I *though* the serial number when to disk so that one command would
> update the serial for the next, so I'm guessing something other than serial is
> leaking?

Right, so apologies for not documenting that subtlety cleaner - I didn't think
anyone would want the longer explanation :)

The serial is persisted to disk (see the "echo 01" commands to bootstrap the
serial). Each invocation of "openssl ca" will increment the serial number for
the CA identified by CA_DIR - such that it should be a monotonically increasing
sequence. As we've accumulated new certs, the general answer has been update the
script, but just run it by hand after 'forcing' the serial number (since the
cert and key are checked in, and the serial is just a text file). This avoids
having to do something like this CL, which is a pain because it requires
updating the hardcoded bits :)

This would have been fine, except the CA_DIR from line 109 began leaking. When
generating the certs by hand, they were coming off the Root CA, but when
generated via the script, they were coming off the intermediates. This threw off
the normal calculations you'd expect to be able to do (count the number of
invocations of "openssl ca" to find the right serial), and as a consequence,
threw everything out.

I could have fixed these up by hand, but that would have been more of a pain due
to the skew, so I went to just regenerate and sort those issues out :)

[commit-bot: I haz the power, 2017-06-05 20:01:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 20:01:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Hamilton, 2017-06-05 20:02:51]

On 2017/06/05 19:56:04, Ryan Sleevi wrote:
> On 2017/06/05 19:49:46, Ryan Hamilton wrote:
> > Hm. I'm still too dense to connect the dots. So the script runs with bash as
> > /bin/sh which puts bash into SH compat mode. It then runs a bunch of
commands
> > via the try() bash function defined in the script. Does that cause something
> to
> > leak which doesn't leak when the commands are run directly by the script? If
> so,
> > two questions. 
> > 1) What is the different behavior between function and command (does the
> > function automatically do something with the environment which the direct
> > command doesn't?)? 
> 
> Yes. It causes the environment variables specified for the scope of a single
> command (e.g.: CA_NAME or SUBJECT_NAME) to leak into all subsequent
invocations
> 'as if' they had been exported.

O_O What is this? I don't even!

*head explodes*

> > 2) In the particular case of these commands, what is leaking that causes
> > problems? I *though* the serial number when to disk so that one command
would
> > update the serial for the next, so I'm guessing something other than serial
is
> > leaking?
> 
> Right, so apologies for not documenting that subtlety cleaner - I didn't think
> anyone would want the longer explanation :)

Sorry, I'm totally obsessive about puzzles! :)

> The serial is persisted to disk (see the "echo 01" commands to bootstrap the
> serial). Each invocation of "openssl ca" will increment the serial number for
> the CA identified by CA_DIR - such that it should be a monotonically
increasing
> sequence. As we've accumulated new certs, the general answer has been update
the
> script, but just run it by hand after 'forcing' the serial number (since the
> cert and key are checked in, and the serial is just a text file). This avoids
> having to do something like this CL, which is a pain because it requires
> updating the hardcoded bits :)
> 
> This would have been fine, except the CA_DIR from line 109 began leaking. When
> generating the certs by hand, they were coming off the Root CA, but when
> generated via the script, they were coming off the intermediates. This threw
off
> the normal calculations you'd expect to be able to do (count the number of
> invocations of "openssl ca" to find the right serial), and as a consequence,
> threw everything out.

AH! I get it. Wow, that is astonishingly subtle. Nice catch!

> I could have fixed these up by hand, but that would have been more of a pain
due
> to the skew, so I went to just regenerate and sort those issues out :)

:)

Thanks for the insane level of detail!

[Ryan Sleevi, 2017-06-06 20:40:07]

Emily: A tweak in the //components/certificate_reporter code that I'd love to
make sure you're OK with.

It looked like the old tests were relying on the fact that the cert didn't have
any identifying data in it; I've rewritten it to be independent of the
underlying representation, but I wanted to make sure you're OK with that.

If so, can you LG and click CQ? Thanks!

[estark, 2017-06-06 21:00:31]

On 2017/06/06 20:40:07, Ryan Sleevi wrote:
> Emily: A tweak in the //components/certificate_reporter code that I'd love to
> make sure you're OK with.
> 
> It looked like the old tests were relying on the fact that the cert didn't
have
> any identifying data in it; I've rewritten it to be independent of the
> underlying representation, but I wanted to make sure you're OK with that.
> 
> If so, can you LG and click CQ? Thanks!

Ah, thanks for fixing that. And TIL about ADD_FAILURE, cool. Still lgtm

[estark, 2017-06-06 21:00:36]

The CQ bit was checked by estark@chromium.org

[estark, 2017-06-06 21:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2926463002/#ps60001
(title: "Extract PEM chain")

[commit-bot: I haz the power, 2017-06-06 21:01:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 22:03:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 22:03:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Sleevi, 2017-06-07 15:43:54]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 15:44:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 17:13:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 17:13:48]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-06-07 17:14:20]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2017-06-07 17:14:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2926463002/#ps80001
(title: "Fixup model tests")

[commit-bot: I haz the power, 2017-06-07 17:14:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 17:21:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496855660741550,
"parent_rev": "c53f4ad87510ee97b5c3425a14c0e79780cdf262", "commit_rev":
"6ee44a7247c639c0703f291d320bdf05c1531b57"}

[commit-bot: I haz the power, 2017-06-07 17:21:54]

Description was changed from

==========
Fix TransportSecurityState unittests to run in --single-process mode.

Several of the TransportSecurityState unittests do not ASSERT that they are
able to parse a certificate. If parsing fails, they end up causing failures
with Expect-Staple reporting. This CL adds robustness checks by consistently
ASSERTing that certificates could be loaded from disk successfully. This is
the fix for diagnostics, but not the fix for root cause.

The root cause is that several test certificates duplicated the issuer and
serial number tuples, due to their organic growth and addition. On systems
using NSS to represent the OS certificate (e.g. Linux and ChromeOS), due to
NSS unfortunately being implemented assuming X.500's (never implemented)
uniqueness of issuer+serial tuples, if two certificates have the same tuple,
NSS will fail to parse the second certificate if the first certificate is
still in memory.

When running tests with --single-process, and having run a test that
successfully verifies a certificate using a test intermediate, NSS
unfortunately keeps a copy of the intermediate in memory until NSS itself is
unloaded. Since we never unload NSS, this intermediate is always kept around,
thus causing observable side-effects.

On the bots and under normal testing, this doesn't manifest, because the test
harness runs each test separately as needed, to ensure hermeticism. However,
some developers like to use --single-process for speed (less process spawning
overhead) or debugging, despite no bots running this config.

The duplicate serial number itself emerged from side-effects related to bash
functions and environment variables, hence why it was not initially spotted.

To properly resolve this issue, this change does the following:
  1) Fix the test generation script to not leak environment state.
    a) Fixes the duplicate serial number issue.
    b) Fixes incorrect subject names and issuer names due to env bleeding.
  2) Regenerate the test certificates to ensure serial uniqueness.
  3) Update the tests that have hardcoded aspects of the chain (PKP testing
     and name constraint testing).
  4) Add additional assertions to TransportSecurityState to fail quicker.
  5) Remove an unnecessary HPKP test chain that had been refactored away.

BUG=729341
==========

to

==========
Fix TransportSecurityState unittests to run in --single-process mode.

Several of the TransportSecurityState unittests do not ASSERT that they are
able to parse a certificate. If parsing fails, they end up causing failures
with Expect-Staple reporting. This CL adds robustness checks by consistently
ASSERTing that certificates could be loaded from disk successfully. This is
the fix for diagnostics, but not the fix for root cause.

The root cause is that several test certificates duplicated the issuer and
serial number tuples, due to their organic growth and addition. On systems
using NSS to represent the OS certificate (e.g. Linux and ChromeOS), due to
NSS unfortunately being implemented assuming X.500's (never implemented)
uniqueness of issuer+serial tuples, if two certificates have the same tuple,
NSS will fail to parse the second certificate if the first certificate is
still in memory.

When running tests with --single-process, and having run a test that
successfully verifies a certificate using a test intermediate, NSS
unfortunately keeps a copy of the intermediate in memory until NSS itself is
unloaded. Since we never unload NSS, this intermediate is always kept around,
thus causing observable side-effects.

On the bots and under normal testing, this doesn't manifest, because the test
harness runs each test separately as needed, to ensure hermeticism. However,
some developers like to use --single-process for speed (less process spawning
overhead) or debugging, despite no bots running this config.

The duplicate serial number itself emerged from side-effects related to bash
functions and environment variables, hence why it was not initially spotted.

To properly resolve this issue, this change does the following:
  1) Fix the test generation script to not leak environment state.
    a) Fixes the duplicate serial number issue.
    b) Fixes incorrect subject names and issuer names due to env bleeding.
  2) Regenerate the test certificates to ensure serial uniqueness.
  3) Update the tests that have hardcoded aspects of the chain (PKP testing
     and name constraint testing).
  4) Add additional assertions to TransportSecurityState to fail quicker.
  5) Remove an unnecessary HPKP test chain that had been refactored away.

BUG=729341

Review-Url: https://codereview.chromium.org/2926463002
Cr-Commit-Position: refs/heads/master@{#477691}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ee44a7247c639c0703f291d320b...
==========

[commit-bot: I haz the power, 2017-06-07 17:21:57]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6ee44a7247c639c0703f291d320b...