Check that all preloaded hostnames are in canonicalized form.

Check that all preloaded hostnames are in canonicalized form.

The hostnames in the preload list must be in their canonical form because they
are matched against canonicalized input by the Chromium code. Non-canonical
hostnames would never be matched.

This CL also removes one entry which is not in canonical form. The list already
contains entry in its canonical form.

BUG=677294
Review-Url: https://codereview.chromium.org/2906223002
Cr-Commit-Position: refs/heads/master@{#478369}
Committed: https://chromium.googlesource.com/chromium/src/+/e7c54372558c9bb3e5861096087685fd5b73f8ea

[martijnc, 2017-05-28 18:49:29]

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

[commit-bot: I haz the power, 2017-05-28 18:49:39]

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-05-28 19:52:05]

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

[commit-bot: I haz the power, 2017-05-28 19:52:06]

Dry run: This issue passed the CQ dry run.

[martijnc, 2017-05-28 19:59:06]

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

[martijnc, 2017-06-01 16:57:56]

Description was changed from

==========
Check that all preloaded hostnames are in canonicalized form.

BUG=677294
==========

to

==========
Check that all preloaded hostnames are in canonicalized form.

The hostnames in the preload list must be in their canonical form because they
are matched against canonicalized input by the Chromium code. Non-canonical
hostnames would never be matched.

This CL also removes one entry which is not in canonical form. The list already
contains entry in its canonical form.

BUG=677294
==========

[martijnc, 2017-06-01 17:02:34]

Hi, can you review this patch?

The check is based on net::IsCanonicalizedHostCompliant().

Thanks!

[Ryan Sleevi, 2017-06-01 18:20:23]

Could you explain why you didn't/couldn't reuse that method?

[martijnc, 2017-06-01 18:52:30]

On 2017/06/01 at 18:20:23, rsleevi wrote:
> Could you explain why you didn't/couldn't reuse that method?

It is part of the //net component which now has a dependency on the generator,
using the method in the generator adds a dependency from the generator on //net.
This results in a circular dependency between //net and the generator.

This is also the reason the generator uses BoringSSL's X509 code and not the
ASN.1 methods in net/cert. Because //net is one component I haven't been able to
make the generator depend on specific parts like net/base or net/cert.

[Ryan Sleevi, 2017-06-01 18:55:33]

On 2017/06/01 18:52:30, martijnc wrote:
> On 2017/06/01 at 18:20:23, rsleevi wrote:
> > Could you explain why you didn't/couldn't reuse that method?
> 
> It is part of the //net component which now has a dependency on the generator,
> using the method in the generator adds a dependency from the generator on
//net.
> This results in a circular dependency between //net and the generator.
> 
> This is also the reason the generator uses BoringSSL's X509 code and not the
> ASN.1 methods in net/cert. Because //net is one component I haven't been able
to
> make the generator depend on specific parts like net/base or net/cert.

Ah. I see. If //net was more modular, we'd be able to reuse it. I think ideally,
we'd want to do that - where the (above //net) consumers can pick up
//net+preload, the preload generator can depend on //net "real", and only the
preload bits depend on the preload generator.

[martijnc, 2017-06-02 20:52:31]

On 2017/06/01 at 18:55:33, rsleevi wrote:
> On 2017/06/01 18:52:30, martijnc wrote:
> > On 2017/06/01 at 18:20:23, rsleevi wrote:
> > > Could you explain why you didn't/couldn't reuse that method?
> > 
> > It is part of the //net component which now has a dependency on the
generator,
> > using the method in the generator adds a dependency from the generator on
//net.
> > This results in a circular dependency between //net and the generator.
> > 
> > This is also the reason the generator uses BoringSSL's X509 code and not the
> > ASN.1 methods in net/cert. Because //net is one component I haven't been
able to
> > make the generator depend on specific parts like net/base or net/cert.
> 
> Ah. I see. If //net was more modular, we'd be able to reuse it. I think
ideally, we'd want to do that - where the (above //net) consumers can pick up
//net+preload, the preload generator can depend on //net "real", and only the
preload bits depend on the preload generator.

Not quite sure how to proceed with this.

There was a bug at some point about splitting up the //net target but it has
since been closed.

To fix the specific issue here, splitting of the preload code from
TransportSecurityState would be sufficient I think but that means adding more
complexity to an already complex object (the //net+preload target still needs a
way to plug the preload list back in). TransportSecurityState seems to have
become a complicated thing from both a code and policy perspective (e.g.
https://crbug.com/603597).

Should the issues in https://crbug.com/620928 be addressed before making more
changes to the preload code & generator? It looks like that will be a long and
complicated process though.

Anyway, this CL isn't urgent. It's a sanity check to avoid unused entries ending
up in the list and the likely source of the one invalid entry has been fixed in
the preload site. I can hold off landing this to avoid duplicating the code.

[Ryan Sleevi, 2017-06-08 19:54:23]

Let's go ahead and land this, and separately figure out how we can sort out the
dependencies a bit more, so that we can align implementations

LGTM

[martijnc, 2017-06-09 17:48:37]

The CQ bit was checked by martijn@martijnc.be

[commit-bot: I haz the power, 2017-06-09 17:49:00]

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-09 19:27:39]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1497030517137220, "parent_rev":
"1fc3b248c69ca7c5d02ee21616cab2211f1a301f", "commit_rev":
"e7c54372558c9bb3e5861096087685fd5b73f8ea"}

[commit-bot: I haz the power, 2017-06-09 19:27:48]

Description was changed from

==========
Check that all preloaded hostnames are in canonicalized form.

The hostnames in the preload list must be in their canonical form because they
are matched against canonicalized input by the Chromium code. Non-canonical
hostnames would never be matched.

This CL also removes one entry which is not in canonical form. The list already
contains entry in its canonical form.

BUG=677294
==========

to

==========
Check that all preloaded hostnames are in canonicalized form.

The hostnames in the preload list must be in their canonical form because they
are matched against canonicalized input by the Chromium code. Non-canonical
hostnames would never be matched.

This CL also removes one entry which is not in canonical form. The list already
contains entry in its canonical form.

BUG=677294

Review-Url: https://codereview.chromium.org/2906223002
Cr-Commit-Position: refs/heads/master@{#478369}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7c54372558c9bb3e58610960876...
==========

[commit-bot: I haz the power, 2017-06-09 19:27:50]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/e7c54372558c9bb3e58610960876...