Add unittests for certificate and file parsing.

Add unittests for the certificate and file parsing logic in the transport
security state generator.

BUG=595493
Review-Url: https://codereview.chromium.org/2793823002
Cr-Commit-Position: refs/heads/master@{#461900}
Committed: https://chromium.googlesource.com/chromium/src/+/304189c3d5089ead05a2dfb3360e141d2b92eda0

[martijnc, 2017-04-02 19:28:16]

Patchset #1 (id:1) has been deleted

[martijnc, 2017-04-02 19:35:35]

martijn@martijnc.be changed reviewers:
+ rsleevi@chromium.org

[martijnc, 2017-04-02 19:36:08]

Description was changed from

==========
Add unittests for certificate and file parsing.

BUG=595493
==========

to

==========
Add unittests for the certificate and file parsing logic in the transport
security state generator.
==========

[martijnc, 2017-04-02 19:37:55]

Description was changed from

==========
Add unittests for the certificate and file parsing logic in the transport
security state generator.
==========

to

==========
Add unittests for the certificate and file parsing logic in the transport
security state generator.

BUG=595493
==========

[martijnc, 2017-04-03 18:57:49]

These are the last pieces from https://codereview.chromium.org/2660793002.

The functions that do the input file parsing (both the json and pins files) are
moved out of the main cc file. There are no code/logic changes in those
functions.

The only new additions are the tests.

Can you review? Thanks!

[Ryan Sleevi, 2017-04-03 19:06:09]

Thanks for sending this out. Unfortunately, there's a few other things going on
and I'm not going to be able to review this in a timely fashion (e.g. this week,
likely), so it would be better to find another //net/OWNER, preferably one who
has also worked on these files :)

Thanks! :)

[martijnc, 2017-04-03 19:30:12]

martijn@martijnc.be changed reviewers:
+ davidben@chromium.org
- rsleevi@chromium.org

[martijnc, 2017-04-03 19:30:12]

davidben: I think you are familiar with the preload code, can you review this
patch?

[davidben, 2017-04-03 20:53:02]

Thanks! Minor nits but otherwise looks good.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
File net/tools/transport_security_state_generator/cert_util_unittest.cc (right):

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:138:
GetX509CertificateFromPEM(std::string(kInvalidPublicKeyPEM)));
I don't believe you need the std::string calls here. (It'd even be slightly more
efficient since the base::StringPiece will save you an allocation.) Ditto in the
tests below.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:145: cert1 =
GetX509CertificateFromPEM(std::string(kSelfSignedWithCommonNamePEM));
Nit: bssl::UniquePtr<X509> cert1 = ....;

(Fold the two lines together.)

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:155:
bssl::UniquePtr<X509> cert2;
Ditto.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:198: cert1 =
GetX509CertificateFromPEM(std::string(kSelfSignedWithCommonNamePEM));
Ditto re folding lines together.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:208:
GetX509CertificateFromPEM(std::string(kSelfSignedWithoutCommonNamePEM));
Ditto.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:217: cert3 =
GetX509CertificateFromPEM(std::string(kSelfSignedWithoutSubject));
Ditto.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
File net/tools/transport_security_state_generator/input_file_parsers.h (right):

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers.h:23: bool
ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets);
Doesn't really matter, but base::StringPiece is basically strictly preferable to
a std::string const-ref.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers.h:31: bool
ParseJSON(const std::string& json,
Ditto.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
File net/tools/transport_security_state_generator/input_file_parsers_unittest.cc
(right):

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers_unittest.cc:46:
"      \"include_subdomains_for_expect_staple\": true"
Think it's worth including non-include_subdomains variants of HSTS, HPKP, and
Expect-Staple?

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers_unittest.cc:75:
EXPECT_EQ("hsts.example.com", entry->hostname);
EXPECT_TRUE(entry->include_subdomains)?

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers_unittest.cc:86:
EXPECT_EQ("hpkp.example.com", entry->hostname);
EXPECT_FALSE(entry->include_subdomains)? (Ditto for the others below.)

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers_unittest.cc:312:
}
Nit: newline above this and a "// namespace" comment.

[martijnc, 2017-04-04 17:56:24]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 17:56:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[martijnc, 2017-04-04 18:02:28]

Patchset #2 (id:40001) has been deleted

[martijnc, 2017-04-04 18:14:19]

Patchset #2 (id:60001) has been deleted

[martijnc, 2017-04-04 18:21:59]

Patchset #2 (id:80001) has been deleted

[martijnc, 2017-04-04 18:22:22]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 18:23:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[martijnc, 2017-04-04 18:31:10]

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
File net/tools/transport_security_state_generator/cert_util_unittest.cc (right):

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:138:
GetX509CertificateFromPEM(std::string(kInvalidPublicKeyPEM)));
On 2017/04/03 at 20:53:02, davidben wrote:
> I don't believe you need the std::string calls here. (It'd even be slightly
more efficient since the base::StringPiece will save you an allocation.) Ditto
in the tests below.

Done.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:145: cert1 =
GetX509CertificateFromPEM(std::string(kSelfSignedWithCommonNamePEM));
On 2017/04/03 at 20:53:02, davidben wrote:
> Nit: bssl::UniquePtr<X509> cert1 = ....;
> 
> (Fold the two lines together.)

Done.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:155:
bssl::UniquePtr<X509> cert2;
On 2017/04/03 at 20:53:02, davidben wrote:
> Ditto.

Done.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:198: cert1 =
GetX509CertificateFromPEM(std::string(kSelfSignedWithCommonNamePEM));
On 2017/04/03 at 20:53:02, davidben wrote:
> Ditto re folding lines together.

Done.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:208:
GetX509CertificateFromPEM(std::string(kSelfSignedWithoutCommonNamePEM));
On 2017/04/03 at 20:53:01, davidben wrote:
> Ditto.

Done.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/cert_util_unittest.cc:217: cert3 =
GetX509CertificateFromPEM(std::string(kSelfSignedWithoutSubject));
On 2017/04/03 at 20:53:01, davidben wrote:
> Ditto.

Done.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
File net/tools/transport_security_state_generator/input_file_parsers.h (right):

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers.h:23: bool
ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets);
On 2017/04/03 at 20:53:02, davidben wrote:
> Doesn't really matter, but base::StringPiece is basically strictly preferable
to a std::string const-ref.

Done.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers.h:31: bool
ParseJSON(const std::string& json,
On 2017/04/03 at 20:53:02, davidben wrote:
> Ditto.

Done.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
File net/tools/transport_security_state_generator/input_file_parsers_unittest.cc
(right):

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers_unittest.cc:46:
"      \"include_subdomains_for_expect_staple\": true"
On 2017/04/03 at 20:53:02, davidben wrote:
> Think it's worth including non-include_subdomains variants of HSTS, HPKP, and
Expect-Staple?

Added.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers_unittest.cc:75:
EXPECT_EQ("hsts.example.com", entry->hostname);
On 2017/04/03 at 20:53:02, davidben wrote:
> EXPECT_TRUE(entry->include_subdomains)?

Added.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers_unittest.cc:86:
EXPECT_EQ("hpkp.example.com", entry->hostname);
On 2017/04/03 at 20:53:02, davidben wrote:
> EXPECT_FALSE(entry->include_subdomains)? (Ditto for the others below.)

Added.

https://codereview.chromium.org/2793823002/diff/20001/net/tools/transport_sec...
net/tools/transport_security_state_generator/input_file_parsers_unittest.cc:312:
}
On 2017/04/03 at 20:53:02, davidben wrote:
> Nit: newline above this and a "// namespace" comment.

Done.

[commit-bot: I haz the power, 2017-04-04 20:32:27]

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

[commit-bot: I haz the power, 2017-04-04 20:32:30]

Dry run: This issue passed the CQ dry run.

[davidben, 2017-04-04 23:35:31]

The CQ bit was checked by davidben@chromium.org

[davidben, 2017-04-04 23:35:32]

lgtm

https://codereview.chromium.org/2793823002/diff/100001/net/tools/transport_se...
File net/tools/transport_security_state_generator/input_file_parsers.cc (right):

https://codereview.chromium.org/2793823002/diff/100001/net/tools/transport_se...
net/tools/transport_security_state_generator/input_file_parsers.cc:181:
certs_input, "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL)) {
Oh, my bad! It didn't occur to me that you'd have to redo the istreamstream
stuff. Ah well, this works. Thanks!

[commit-bot: I haz the power, 2017-04-04 23:36:29]

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-04-04 23:44:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491348931681320,
"parent_rev": "22fecb03c22982be2034c89bbb1cf1833bd4e330", "commit_rev":
"304189c3d5089ead05a2dfb3360e141d2b92eda0"}

[commit-bot: I haz the power, 2017-04-04 23:46:01]

Description was changed from

==========
Add unittests for the certificate and file parsing logic in the transport
security state generator.

BUG=595493
==========

to

==========
Add unittests for the certificate and file parsing logic in the transport
security state generator.

BUG=595493

Review-Url: https://codereview.chromium.org/2793823002
Cr-Commit-Position: refs/heads/master@{#461900}
Committed:
https://chromium.googlesource.com/chromium/src/+/304189c3d5089ead05a2dfb3360e...
==========

[commit-bot: I haz the power, 2017-04-04 23:46:02]

Committed patchset #2 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/304189c3d5089ead05a2dfb3360e...