Remove the test-only X509Certificate constructor

Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended
for test-only code that allowed minting a fake
certificate. However, this is a frequent source of test
bugs, because these certificates violate one of
X509Certificate's core guarantees - that there's always
an associated OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either
imports a test certificate directly or mints them on the
fly, as appropriate.

BUG=526899
TBR=rdevlin.cronin@chromium.org

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

[Ryan Sleevi, 2016-06-21 23:43:35]

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

[commit-bot: I haz the power, 2016-06-21 23:44:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084153003/40001

[Ryan Sleevi, 2016-06-22 00:11:47]

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

[commit-bot: I haz the power, 2016-06-22 00:12:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084153003/60001

[Ryan Sleevi, 2016-06-22 00:53:44]

Description was changed from

==========
Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended for test-only
code that allowed minting a fake certificate. However, this is a
frequent source of test bugs, because these certificates violate one
of X509Certificate's core guarantees - that there's always an associated
OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either imports a test
certificate directly or mints them on the fly, as appropriate.

BUG=526899
==========

to

==========
Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended
for test-only code that allowed minting a fake
certificate. However, this is a frequent source of test
bugs, because these certificates violate one of
X509Certificate's core guarantees - that there's always
an associated OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either
imports a test certificate directly or mints them on the
fly, as appropriate.

BUG=526899
==========

[Ryan Sleevi, 2016-06-22 00:53:44]

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

[Ryan Sleevi, 2016-06-22 00:54:04]

rsleevi@chromium.org changed reviewers:
+ eugenebut@chromium.org, wez@chromium.org

[Ryan Sleevi, 2016-06-22 00:55:01]

rsleevi@chromium.org changed reviewers:
+ mattm@chromium.org
- mmenke@chromium.org

[Ryan Sleevi, 2016-06-22 00:55:14]

rsleevi@chromium.org changed reviewers:
+ mmenke@chromium.org
- mattm@chromium.org

[Ryan Sleevi, 2016-06-22 00:56:16]

thakis: Rubberstamp refactor for the //chrome land? :)
wez: cast
eugenebut: iOS
mmenke: //net and resource loader tests

[Eugene But (OOO till 7-30), 2016-06-22 01:11:22]

ios lgtm

[Wez, 2016-06-22 01:22:14]

cast rubber-stampish LGTM

[commit-bot: I haz the power, 2016-06-22 02:17:30]

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

[commit-bot: I haz the power, 2016-06-22 02:17:31]

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

[Ryan Sleevi, 2016-06-22 02:25:39]

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

[commit-bot: I haz the power, 2016-06-22 02:26:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084153003/80001

[commit-bot: I haz the power, 2016-06-22 04:20:05]

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

[commit-bot: I haz the power, 2016-06-22 04:20:06]

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

[Nico, 2016-06-22 14:12:11]

chrome/ lgtm stamp

https://codereview.chromium.org/2084153003/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/2084153003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc:445:
net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"));
(is there a bug somewhere for making CertificateList have unique_ptrs instead of
raw pointers?)

https://codereview.chromium.org/2084153003/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2084153003/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:2847: : certs_(certs) {}
hm, copying a bag of raw pointers smells a bit funny

[Ryan Sleevi, 2016-06-22 15:22:26]

https://codereview.chromium.org/2084153003/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/2084153003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc:445:
net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"));
On 2016/06/22 14:12:11, Nico wrote:
> (is there a bug somewhere for making CertificateList have unique_ptrs instead
of
> raw pointers?)

CertificateList takes scoped_refptr<>s

https://codereview.chromium.org/2084153003/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2084153003/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:2847: : certs_(certs) {}
On 2016/06/22 14:12:11, Nico wrote:
> hm, copying a bag of raw pointers smells a bit funny

It's copying scoped_refptr<>s

[mmenke, 2016-06-22 15:58:42]

You should probably have someone more familiar with the X509Certificate class
review this - I'm not at all familiar it, and am too busy this week to learn it
well enough to review this CL.

[Ryan Sleevi, 2016-06-22 16:00:20]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org
- mmenke@chromium.org

[Ryan Sleevi, 2016-06-22 16:00:21]

Had been trying to diversify since it was largely test code being touched, and
because of sending out too many cleanup CLs, but... Hey Eric, have another
cleanup CL :)

[eroman, 2016-06-22 16:51:08]

lgtm

https://codereview.chromium.org/2084153003/diff/80001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/2084153003/diff/80001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:480: const base::Time& start =
kTestData[i].validity_start;
optional: could instead alias kTestData[i]

https://codereview.chromium.org/2084153003/diff/80001/net/ssl/ssl_client_auth...
File net/ssl/ssl_client_auth_cache_unittest.cc (right):

https://codereview.chromium.org/2084153003/diff/80001/net/ssl/ssl_client_auth...
net/ssl/ssl_client_auth_cache_unittest.cc:122:
ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem"));
Any reason for changing the test pattern here? (i.e. re-import "ok_cert.pem"
here).
Looks like all the test cares about is that cert1 and cert2 be different
pointers, not data.

I don't really care either way, just reviewing the intent of this change.

[Ryan Sleevi, 2016-06-22 17:31:46]

https://codereview.chromium.org/2084153003/diff/80001/net/ssl/ssl_client_auth...
File net/ssl/ssl_client_auth_cache_unittest.cc (right):

https://codereview.chromium.org/2084153003/diff/80001/net/ssl/ssl_client_auth...
net/ssl/ssl_client_auth_cache_unittest.cc:122:
ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem"));
On 2016/06/22 16:51:08, eroman wrote:
> Any reason for changing the test pattern here? (i.e. re-import "ok_cert.pem"
> here).
> Looks like all the test cares about is that cert1 and cert2 be different
> pointers, not data.

I'm not sure I follow. Were you suggesting to reimport ok_cert.pem? Not sure why
the comment here vs say line 55-61

If I reused ok_cert.pem, I would have had to sprinkle in assertions that
cert1.get() != cert2.get() and that seemed unnecessary to add new preconditions.

[mmenke, 2016-06-22 17:33:59]

content/browser/loader LGTM, rubberstamp (Not sure if you already have reviewer
coverage of that file)

[eroman, 2016-06-22 17:38:21]

https://codereview.chromium.org/2084153003/diff/80001/net/ssl/ssl_client_auth...
File net/ssl/ssl_client_auth_cache_unittest.cc (right):

https://codereview.chromium.org/2084153003/diff/80001/net/ssl/ssl_client_auth...
net/ssl/ssl_client_auth_cache_unittest.cc:122:
ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem"));
On 2016/06/22 17:31:45, Ryan Sleevi wrote:
> On 2016/06/22 16:51:08, eroman wrote:
> > Any reason for changing the test pattern here? (i.e. re-import "ok_cert.pem"
> > here).
> > Looks like all the test cares about is that cert1 and cert2 be different
> > pointers, not data.
> 
> I'm not sure I follow. Were you suggesting to reimport ok_cert.pem? Not sure
why
> the comment here vs say line 55-61

55-61 isn't changing the code pattern, whereas this is.

> 
> If I reused ok_cert.pem, I would have had to sprinkle in assertions that
> cert1.get() != cert2.get() and that seemed unnecessary to add new
preconditions.

[Ryan Sleevi, 2016-06-22 20:27:33]

https://codereview.chromium.org/2084153003/diff/80001/net/ssl/ssl_client_auth...
File net/ssl/ssl_client_auth_cache_unittest.cc (right):

https://codereview.chromium.org/2084153003/diff/80001/net/ssl/ssl_client_auth...
net/ssl/ssl_client_auth_cache_unittest.cc:122:
ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem"));
On 2016/06/22 17:38:21, eroman wrote:
> 55-61 isn't changing the code pattern, whereas this is.

I suspect I'm not understanding what you mean by "changing the code pattern". As
best I can understand, you're talking about cert2 having the same input
parameters ("foo", "CA", etc) with the expectation of returning a different
cert, and assuming that was somehow intentional or meaningful. Not really, no -
it was intended that they be 'different' certs (and X509Certificate() has always
obeyed that, as is required by a ctor). However,
X509Certificate::CreateFrom(...) has historically not obeyed that, and that the
other constructor was intentionally private because of this.

That is to say, this change reflects exactly what the test was trying to do and
testing, even if it didn't change "foo" to "foo2", so no, it's not a change in
code pattern.

[Ryan Sleevi, 2016-06-22 20:29:01]

rsleevi@chromium.org changed reviewers:
+ maruel@chromium.org

[Ryan Sleevi, 2016-06-22 20:29:01]

maruel: For isolate changes

[M-A Ruel, 2016-06-22 20:29:51]

oh no! Not another layering violation of unit test data!!



lgtm

[Ryan Sleevi, 2016-06-22 20:36:28]

Description was changed from

==========
Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended
for test-only code that allowed minting a fake
certificate. However, this is a frequent source of test
bugs, because these certificates violate one of
X509Certificate's core guarantees - that there's always
an associated OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either
imports a test certificate directly or mints them on the
fly, as appropriate.

BUG=526899
==========

to

==========
Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended
for test-only code that allowed minting a fake
certificate. However, this is a frequent source of test
bugs, because these certificates violate one of
X509Certificate's core guarantees - that there's always
an associated OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either
imports a test certificate directly or mints them on the
fly, as appropriate.

BUG=526899
TBR=rdevlin.cronin@chromium.org
==========

[Ryan Sleevi, 2016-06-22 20:36:28]

rsleevi@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Ryan Sleevi, 2016-06-22 20:36:35]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-22 20:36:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, wez@chromium.org, thakis@chromium.org,
mmenke@chromium.org, eroman@chromium.org, maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2084153003/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-06-22 20:37:37]

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

[commit-bot: I haz the power, 2016-06-22 22:11:01]

Description was changed from

==========
Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended
for test-only code that allowed minting a fake
certificate. However, this is a frequent source of test
bugs, because these certificates violate one of
X509Certificate's core guarantees - that there's always
an associated OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either
imports a test certificate directly or mints them on the
fly, as appropriate.

BUG=526899
TBR=rdevlin.cronin@chromium.org
==========

to

==========
Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended
for test-only code that allowed minting a fake
certificate. However, this is a frequent source of test
bugs, because these certificates violate one of
X509Certificate's core guarantees - that there's always
an associated OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either
imports a test certificate directly or mints them on the
fly, as appropriate.

BUG=526899
TBR=rdevlin.cronin@chromium.org
==========

[commit-bot: I haz the power, 2016-06-22 22:11:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-22 22:14:09]

Description was changed from

==========
Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended
for test-only code that allowed minting a fake
certificate. However, this is a frequent source of test
bugs, because these certificates violate one of
X509Certificate's core guarantees - that there's always
an associated OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either
imports a test certificate directly or mints them on the
fly, as appropriate.

BUG=526899
TBR=rdevlin.cronin@chromium.org
==========

to

==========
Remove the test-only X509Certificate constructor

net::X509Certificate had a constructor that was intended
for test-only code that allowed minting a fake
certificate. However, this is a frequent source of test
bugs, because these certificates violate one of
X509Certificate's core guarantees - that there's always
an associated OSCertHandle for an X509Certificate.

This removes the ctor and all usages of it. Code either
imports a test certificate directly or mints them on the
fly, as appropriate.

BUG=526899
TBR=rdevlin.cronin@chromium.org

Committed: https://crrev.com/f300d683274a040d68a367aeed7aecbdc3a611a8
Cr-Commit-Position: refs/heads/master@{#401430}
==========

[commit-bot: I haz the power, 2016-06-22 22:14:12]

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