Add enterprise policy to exempt hosts from Certificate Transparency

Add and connect an Enterprise Policy for whitelisting
hosts as exempt from Certificate Transparency policy.

This introduces a policy
(CertificateTransparencyEnforcementDisabledForUrls) that
allows exempting certain hostnames from the Certificate
Transparency requirements. Some CAs, such as Symantec and
CNNIC at present, are required to disclose their
certificates via CT in order to have them trusted; any
certificate not disclosed is not trusted.

However, to accomodate some enterprise users who have the
capability to manage Chromium consumers, but cannot
manage other certificate-consuming systems on their
network, and which need certificates from these CAs, and
which claim that they cannot have these hosts disclosed
publicly (e.g. "topsecret.internal.example.com"), this
provides a policy mechanism to allow those hosts to be
exempted from CT requirement.

This is not a blanket policy for general hosts on the
Internet; in general, all certificates from these CAs
must conform, unless the device is enterprise managed.

Whether or not this policy ends up being temporary or not
depends on the IETF and CA community, and whether or not
a suitable technical means of redaction can be devised
which allows redaction (e.g. "?.?.example.com") to be
safely performed. For now and the foreseeable future,
redaction is not viable for Chromium, so the enterprise
policy is offered as an alternative.

BUG=620178
TBR=atwilson@chromium.org

Committed: https://crrev.com/96356f8d5e565b402c85b3a4c4a58d58fb594dbd
Cr-Commit-Position: refs/heads/master@{#403125}

[Ryan Sleevi, 2016-06-27 21:52:15]

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

[Ryan Sleevi, 2016-06-27 21:52:15]

Dominic, Matt: Would you two do the high-level review first?

Matt: To make sure I've got the ProfileIOData glue and ownership lifetimes
correct)

Dominic: I'm re-using url_matcher here, but it does feel somewhat heavyweight
for my use case (which only cares about host name || subdomain matches). There's
a feeling here that there's room to optimize for my use case (e.g. a simple trie
or DAFSA on the reversed DNS, as we do for the public suffix list), but given
that the inputs are expected to be on the order of O(10-50) for those that set
it, and O(0) in the 99.% case, that didn't seem terribly useful.

The biggest assumption of the design constraints is that the construction of the
URLMatcher is done on the IOThread rather than forced to a worker - again,
because of the above design factors. I can rework that, but that just seemed
overkill.

[mmenke, 2016-06-27 22:11:57]

Lifetime looks reasonable (To the extent that anything in ProfileIOData looks
reasonable)

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
File components/certificate_transparency/ct_policy_manager.h (right):

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.h:17: class
SequencedTaskRunner;
Should this be included explicitly?  Seems like CTPolicyManager's prototype
requires we know about its increment method, though I'm honestly not sure.

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.h:26: class
CTPolicyManager {
Should document this class, and its lifetime and threading expectations.  Fine
to create on one thread, destroy on another, I assume?

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.h:32: explicit
CTPolicyManager(
nit:  Explicit not needed.

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.h:37: void Shutdown();
As used in ProfileIOData, its delegate on the UIThread is used after Shutdown is
called...  If that's fine, should say so.

[mmenke, 2016-06-27 22:14:53]

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
File components/certificate_transparency/ct_policy_manager.cc (right):

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.cc:67:
url_matcher::URLMatcherConditionSet::ID id_;
Should this be called something like next_id_?  At least it doesn't look like
the ID of the CTPolicyManager itself.

[Ryan Sleevi, 2016-06-27 22:49:19]

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
File components/certificate_transparency/ct_policy_manager.h (right):

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.h:17: class
SequencedTaskRunner;
On 2016/06/27 22:11:57, mmenke wrote:
> Should this be included explicitly?  Seems like CTPolicyManager's prototype
> requires we know about its increment method, though I'm honestly not sure.

It doesn't need to be (only at call-site; you can actually pass through the
objects if you have something that returns a scoped_refptr<> because of RVO)

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.h:26: class
CTPolicyManager {
On 2016/06/27 22:11:57, mmenke wrote:
> Should document this class, and its lifetime and threading expectations.  Fine
> to create on one thread, destroy on another, I assume?

Yeah; it's created on UI, shut down on UI, deleted on IO AIUI - does that match?

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.h:37: void Shutdown();
On 2016/06/27 22:11:56, mmenke wrote:
> As used in ProfileIOData, its delegate on the UIThread is used after Shutdown
is
> called...  If that's fine, should say so.

I'm perhaps not sure I understood your comment.

I think you're saying it should be clearer that:
1) Shutdown is called on the UI thread
2) the RequireCTDelegate from GetDelegate() is safe to be used after Shutdown,
but it doesn't receive any more updates

Is that correct?

[Ryan Sleevi, 2016-06-28 04:24:46]

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

[Ryan Sleevi, 2016-06-28 04:24:47]

Adding Drew now that I have unittests; still curious for the design feedback
from Dominic, but making sure all my Is are dotted and Ts crossed from Drew.

This is gated on the UI review of the error page, understandably - but Matt, can
you review the profiles stuff officially, Drew, can you review policy, and
Dominic, can you review the new files in //components/certificate_transparency
for how they use the URLMatcher & friends

[Ryan Sleevi, 2016-06-28 04:46:30]

Description was changed from

==========
Connect the enterprise policy to exempt hosts from CT to the prefs system

This connects the enterprise policy
CertificateTransparencyEnforcementDisabledForUrls to the preference
certificate_transparency.excluded_hosts, and has that preference cause
a TransportSecurityState::RequireCTDelegate to exclude hosts matching
those host patterns from being subjected to any CT policies that would
cause connection failures (advisory evaluation is still performed for
all hosts)

BUG=620178
==========

to

==========
Add and connect an Enterprise Policy for whitelisting hosts as exempt from
Certificate Transparency policy.

This introduces a policy ( CertificateTransparencyEnforcementDisabledForUrls )
that allows exempting certain hostnames from the Certificate Transparency
requirements. Some CAs, such as Symantec and CNNIC at present, are required to
disclose their certificates via CT in order to have them trusted; any
certificate not disclosed is not trusted.

However, to accomodate some enterprise users who have the capability to manage
Chromium consumers, but cannot manage other certificate-consuming systems on
their network, and which need certificates from these CAs, and which claim that
they cannot have these hosts disclosed publicly (e.g.
"topsecret.internal.example.com"), this provides a policy mechanism to allow
those hosts to be exempted from CT requirement.

This is not a blanket policy for general hosts on the Internet; in general, all
certificates from these CAs must conform, unless the device is enterprise
managed.

Whether or not this policy ends up being temporary or not depends on the IETF
and CA community, and whether or not a suitable technical means of redaction can
be devised which allows redaction (e.g. "?.?.example.com") to be safely
performed. For now and the foreseeable future, redaction is not viable for
Chromium, so the enterprise policy is offered as an alternative.

BUG=620178
==========

[Ryan Sleevi, 2016-06-28 04:47:43]

Description was changed from

==========
Add and connect an Enterprise Policy for whitelisting hosts as exempt from
Certificate Transparency policy.

This introduces a policy ( CertificateTransparencyEnforcementDisabledForUrls )
that allows exempting certain hostnames from the Certificate Transparency
requirements. Some CAs, such as Symantec and CNNIC at present, are required to
disclose their certificates via CT in order to have them trusted; any
certificate not disclosed is not trusted.

However, to accomodate some enterprise users who have the capability to manage
Chromium consumers, but cannot manage other certificate-consuming systems on
their network, and which need certificates from these CAs, and which claim that
they cannot have these hosts disclosed publicly (e.g.
"topsecret.internal.example.com"), this provides a policy mechanism to allow
those hosts to be exempted from CT requirement.

This is not a blanket policy for general hosts on the Internet; in general, all
certificates from these CAs must conform, unless the device is enterprise
managed.

Whether or not this policy ends up being temporary or not depends on the IETF
and CA community, and whether or not a suitable technical means of redaction can
be devised which allows redaction (e.g. "?.?.example.com") to be safely
performed. For now and the foreseeable future, redaction is not viable for
Chromium, so the enterprise policy is offered as an alternative.

BUG=620178
==========

to

==========
Add and connect an Enterprise Policy for whitelisting
hosts as exempt from Certificate Transparency policy.

This introduces a policy
(CertificateTransparencyEnforcementDisabledForUrls) that
allows exempting certain hostnames from the Certificate
Transparency requirements. Some CAs, such as Symantec and
CNNIC at present, are required to disclose their
certificates via CT in order to have them trusted; any
certificate not disclosed is not trusted.

However, to accomodate some enterprise users who have the
capability to manage Chromium consumers, but cannot
manage other certificate-consuming systems on their
network, and which need certificates from these CAs, and
which claim that they cannot have these hosts disclosed
publicly (e.g. "topsecret.internal.example.com"), this
provides a policy mechanism to allow those hosts to be
exempted from CT requirement.

This is not a blanket policy for general hosts on the
Internet; in general, all certificates from these CAs
must conform, unless the device is enterprise managed.

Whether or not this policy ends up being temporary or not
depends on the IETF and CA community, and whether or not
a suitable technical means of redaction can be devised
which allows redaction (e.g. "?.?.example.com") to be
safely performed. For now and the foreseeable future,
redaction is not viable for Chromium, so the enterprise
policy is offered as an alternative.

BUG=620178
==========

[Ryan Sleevi, 2016-06-28 04:47:54]

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

[commit-bot: I haz the power, 2016-06-28 04:48:05]

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, 2016-06-28 04:49:48]

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

[commit-bot: I haz the power, 2016-06-28 04:49:49]

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

[battre, 2016-06-28 08:33:14]

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
File components/certificate_transparency/ct_policy_manager.cc (right):

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:38: //
RequireCTDelegate implementation
add: to be called on the |network_task_runner_|?

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:112: }
Do you expect this to be faster than a simple lookup? filters_ is a map that
uses the matching_ids as keys, right?

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
File components/certificate_transparency/ct_policy_manager_unittest.cc (right):

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/ct_policy_manager_unittest.cc:133: // The
new preferences should take effect.
This comment needs to be updated.

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/ct_policy_manager_unittest.cc:135:
delegate->IsCTRequiredForHost("google.com"));
Add comment that "*" and "https://" should be ignored. Therefore, google.com
does not match?

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
File components/certificate_transparency/pref_names.h (right):

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/pref_names.h:13: extern const char
kCTRequiredHosts[];
Why do you add this if it is not controllable by policy?

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/pref_names.h:18: extern const char
kCTExcludedHosts[];
Do you want to add a comment which one wins in case both match?

https://codereview.chromium.org/2102783003/diff/120001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/2102783003/diff/120001/net/http/transport_sec...
net/http/transport_security_state.cc:43: int g_ct_required_for_testing = 0;
Can you please document the values?

https://codereview.chromium.org/2102783003/diff/120001/net/http/transport_sec...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2102783003/diff/120001/net/http/transport_sec...
net/http/transport_security_state.h:447: static void
SetShouldRequireCTForTesting(bool* required);
Why don't you just take a boolean and either document that this defaults to
false or have a static void ResetShouldRequireCTForTesting();?

[mmenke, 2016-06-28 12:38:00]

profiles/ LGTM

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
File components/certificate_transparency/ct_policy_manager.h (right):

https://codereview.chromium.org/2102783003/diff/1/components/certificate_tran...
components/certificate_transparency/ct_policy_manager.h:37: void Shutdown();
On 2016/06/27 22:49:19, Ryan Sleevi wrote:
> On 2016/06/27 22:11:56, mmenke wrote:
> > As used in ProfileIOData, its delegate on the UIThread is used after
Shutdown
> is
> > called...  If that's fine, should say so.
> 
> I'm perhaps not sure I understood your comment.
> 
> I think you're saying it should be clearer that:
> 1) Shutdown is called on the UI thread
> 2) the RequireCTDelegate from GetDelegate() is safe to be used after Shutdown,
> but it doesn't receive any more updates
> 
> Is that correct?

Right, I just wanted enough usage comments here that it can be verified
ProfileIOData's use of this class is correct, without delving into the
implementation of the class (It's not complicated, but API docs should be
sufficient to use a class correctly).

The comments you added look to cover this (I consider reviewing your interface
docs here part of reviewing the ProfileIOData changes, since their correctness
depends on them).

[Ryan Sleevi, 2016-06-28 16:41:28]

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
File components/certificate_transparency/ct_policy_manager.cc (right):

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:112: }
On 2016/06/28 08:33:13, battre wrote:
> Do you expect this to be faster than a simple lookup? filters_ is a map that
> uses the matching_ids as keys, right?

Yes, lookups would be O(matching_ids.size() log filters_.size()), with limited
cache locality (because of the binary search doing random accesses across
potentially all the data set). This makes it O(matching_ids.size() +
filters_.size()) in the worst case, with better cache locality (because it's
linearly iterating through from start).

Just a habit that whenever I have two sorted sets (in this case, matching_ids_
and filters_ are both sorted on ID).

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
File components/certificate_transparency/pref_names.h (right):

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/pref_names.h:13: extern const char
kCTRequiredHosts[];
On 2016/06/28 08:33:13, battre wrote:
> Why do you add this if it is not controllable by policy?

Honestly? Mostly for testing purposes (it was easier to test with this). I'd
like to expose it via policy, but I was happy to wait for M-54 to actually add
that bit.

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/pref_names.h:18: extern const char
kCTExcludedHosts[];
On 2016/06/28 08:33:13, battre wrote:
> Do you want to add a comment which one wins in case both match?

Should that be with the Pref? I felt like the pref itself doesn't make any
commitment, it's the thing that glues multiple prefs together (the
CTPolicyManager)

https://codereview.chromium.org/2102783003/diff/120001/net/http/transport_sec...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2102783003/diff/120001/net/http/transport_sec...
net/http/transport_security_state.h:447: static void
SetShouldRequireCTForTesting(bool* required);
On 2016/06/28 08:33:14, battre wrote:
> Why don't you just take a boolean and either document that this defaults to
> false or have a static void ResetShouldRequireCTForTesting();?

Because it won't default to false shortly (but a more complicated policy, per
the commit description), and the implementation of Set/Reset would have been as
complex (because it still has to deal with the tristate - yes, no, default)

[battre, 2016-06-29 09:14:43]

lgtm

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
File components/certificate_transparency/ct_policy_manager.cc (right):

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:112: }
On 2016/06/28 16:41:27, Ryan Sleevi wrote:
> On 2016/06/28 08:33:13, battre wrote:
> > Do you expect this to be faster than a simple lookup? filters_ is a map that
> > uses the matching_ids as keys, right?
> 
> Yes, lookups would be O(matching_ids.size() log filters_.size()), with limited
> cache locality (because of the binary search doing random accesses across
> potentially all the data set). This makes it O(matching_ids.size() +
> filters_.size()) in the worst case, with better cache locality (because it's
> linearly iterating through from start).
> 
> Just a habit that whenever I have two sorted sets (in this case, matching_ids_
> and filters_ are both sorted on ID).

I think that it does not make any practical difference here, so do what you
prefer (I was expecting similar runtime but simpler code). Just be careful to
not follow the general habit if the sets are of significantly different size. I
don't think that you get much locality from a red/black tree traversal.

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
File components/certificate_transparency/pref_names.h (right):

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/pref_names.h:13: extern const char
kCTRequiredHosts[];
On 2016/06/28 16:41:27, Ryan Sleevi wrote:
> On 2016/06/28 08:33:13, battre wrote:
> > Why do you add this if it is not controllable by policy?
> 
> Honestly? Mostly for testing purposes (it was easier to test with this). I'd
> like to expose it via policy, but I was happy to wait for M-54 to actually add
> that bit.

Acknowledged.

https://codereview.chromium.org/2102783003/diff/120001/components/certificate...
components/certificate_transparency/pref_names.h:18: extern const char
kCTExcludedHosts[];
On 2016/06/28 16:41:27, Ryan Sleevi wrote:
> On 2016/06/28 08:33:13, battre wrote:
> > Do you want to add a comment which one wins in case both match?
> 
> Should that be with the Pref? I felt like the pref itself doesn't make any
> commitment, it's the thing that glues multiple prefs together (the
> CTPolicyManager)

Hm. It's just the place I would look for this information. Up to  you.

https://codereview.chromium.org/2102783003/diff/120001/net/http/transport_sec...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2102783003/diff/120001/net/http/transport_sec...
net/http/transport_security_state.h:447: static void
SetShouldRequireCTForTesting(bool* required);
On 2016/06/28 16:41:27, Ryan Sleevi wrote:
> On 2016/06/28 08:33:14, battre wrote:
> > Why don't you just take a boolean and either document that this defaults to
> > false or have a static void ResetShouldRequireCTForTesting();?
> 
> Because it won't default to false shortly (but a more complicated policy, per
> the commit description), and the implementation of Set/Reset would have been
as
> complex (because it still has to deal with the tristate - yes, no, default)

Acknowledged.

[Ryan Sleevi, 2016-06-30 00:17:23]

rsleevi@chromium.org changed reviewers:
+ isherman@chromium.org, pkasting@chromium.org

[Ryan Sleevi, 2016-06-30 00:17:24]

pkasting: Need your OWNERS approval for the DEPS addition to url_formatter (for
segmenting user-entered URL patterns)

isherman: For the histograms.xml change (should be rubberstamp-y; just a new
enterprise policy)

Carrying over Drew's LG from https://codereview.chromium.org/2087743002/ on the
policy & language

[Ilya Sherman, 2016-06-30 00:20:07]

histograms.xml lgtm

[Ryan Sleevi, 2016-06-30 00:20:26]

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

[commit-bot: I haz the power, 2016-06-30 00:21:16]

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, 2016-06-30 00:25:08]

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

[commit-bot: I haz the power, 2016-06-30 00:25:09]

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

[Ryan Sleevi, 2016-06-30 00:34:51]

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

[commit-bot: I haz the power, 2016-06-30 00:35:24]

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, 2016-06-30 01:05:25]

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

[commit-bot: I haz the power, 2016-06-30 01:05:27]

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 Sleevi, 2016-06-30 01:32:57]

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

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

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

[Ryan Sleevi, 2016-06-30 01:57:43]

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

[commit-bot: I haz the power, 2016-06-30 02:03:34]

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

[commit-bot: I haz the power, 2016-06-30 02:03:36]

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_...)

[eroman, 2016-06-30 02:24:36]

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
File components/certificate_transparency/ct_policy_manager.cc (right):

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:85:
base::Owned(required_hosts->CreateDeepCopy().release()),
How about:

 base::Passed(required_hosts->CreateDeepCopy())

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:86:
base::Owned(excluded_hosts->CreateDeepCopy().release())));
ditto

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:102:
std::map<url_matcher::URLMatcherConditionSet::ID, Filter>::const_iterator it =
nit: auto ?

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:124: void
CTPolicyManager::CTDelegate::Update(base::ListValue* required_hosts,
can these ptrs be const? (throughout)

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:158: std::string
lc_host = base::ToLowerASCII(
Is this required?
Does condition_factory->CreateHost*Condition() require normalized hostnames?

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:164: // A leading dot
means exact match and to not match subdomains.
This is the opposite of what I would have expected from other hostname filter
schemes (like proxy bypass rules)....
But I see that is the Blacklist URL format

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:175: if
(host_info.family == url::CanonHostInfo::NEUTRAL) {
The comment above makes it sound like this is only supposed to be done for
non-IP addresses, in which case host_info.IsIPAddress() would be more clear.

Probably better as-is though, since the BROKEN class shouldn't be matching
subdomains.
(In fact should it be matching anything at all?)

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:194: ?
condition_factory->CreateHostSuffixCondition(lc_host)
What happens if lc_host is "*" at this point?
(which could happen if the input was ".*").

Does it barf on this, or does this effectively give a way to do wildcards...

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:196:
conditions->push_back(
I don't think I quite grok how the URLMatcher stuff is supposed to work.

So there is a vector of Condition sets, where each set just has 1 entry...
because all conditions in a set need to be matched?

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:207: return
!lhs.match_subdomains;  // Prefer the more explicit policy.
side-comment:  In practice this is probably redundant with the check below,
since the "exact match" filter has a dot prepended to, so the "host_length" will
be greater.
Although I agree it is clearer to explicitly list this as written.
... Also probably has edge case if hostnames are allowed to have trailing dots

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
File components/certificate_transparency/ct_policy_manager.h (right):

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.h:50: // Returns a
RequireCTDelegate() that responds based on the policies set
Why parens on RequiredCTDelegate() ?

https://codereview.chromium.org/2102783003/diff/220001/components/certificate...
File components/certificate_transparency/ct_policy_manager_unittest.cc (right):

https://codereview.chromium.org/2102783003/diff/220001/components/certificate...
components/certificate_transparency/ct_policy_manager_unittest.cc:93:
delegate->IsCTRequiredForHost("google.com"));
Are there any tests with:
  * non-lowercased hostname
  * trailing dot
  * IPv6 address
  * IPv4 address

[eroman, 2016-06-30 02:31:56]

https://codereview.chromium.org/2102783003/diff/240001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2102783003/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:7932: 'name':
'CertificateTransparencyEnforcementDisabledForUrls',
Why name this "Urls" rather than "Hosts" ?

The preference names it "Hosts", and that seems more in line with how it works.

(It only accepts host filter expressions, not full-blown URL ones).

https://codereview.chromium.org/2102783003/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:7955: A URL pattern is
formatted according to
https://www.chromium.org/administrators/url-blacklist-filter-format. However,
because certificates are valid for a given hostname independent of the scheme,
port, or path, only the hostname portion of the URL is considered. Wildcard
hosts are not supported.
Is wildcard allowed anywhere in the expression ?
i.e. google.*.com ?

Also I think it would be much simpler to just explain the actual restricted
grammar rather than as a restricted version of url-blacklist-filter format
(since the majority of it doesn't apply).

[Ryan Sleevi, 2016-06-30 02:39:44]

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
File components/certificate_transparency/ct_policy_manager.cc (right):

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:86:
base::Owned(excluded_hosts->CreateDeepCopy().release())));
On 2016/06/30 02:24:36, eroman wrote:
> ditto

Unfortunately, base::Passed() (and std::unique_ptr) don't define unwrap
semantics for methods that expect a naked pointer (because there's no implicit
operator*, only get). base::Owned() does, but it requires a naked pointer to
bind - it won't take a move-only variable.

To change to base::Owned() here (or really, dropping it entirely, because
CallbackTraits are smart enough to std::move to the callback) means changing
Update to take ownership, and that wasn't a necessary API contract needed.

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:102:
std::map<url_matcher::URLMatcherConditionSet::ID, Filter>::const_iterator it =
On 2016/06/30 02:24:36, eroman wrote:
> nit: auto ?

Auto binds to the non-const iterator (until C++14's .cbegin())

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:124: void
CTPolicyManager::CTDelegate::Update(base::ListValue* required_hosts,
On 2016/06/30 02:24:36, eroman wrote:
> can these ptrs be const? (throughout)

Unfortunately, the callback helpers get ornery about this (see above)

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:158: std::string
lc_host = base::ToLowerASCII(
On 2016/06/30 02:24:36, eroman wrote:
> Is this required?
> Does condition_factory->CreateHost*Condition() require normalized hostnames?

From what I could tell, implicitly it does - unfortunately, that's not well
documented.

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:194: ?
condition_factory->CreateHostSuffixCondition(lc_host)
On 2016/06/30 02:24:36, eroman wrote:
> What happens if lc_host is "*" at this point?
> (which could happen if the input was ".*").
> 
> Does it barf on this, or does this effectively give a way to do wildcards...

It does nothing, actually - it's just a 'bad filter' that will never match.

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:207: return
!lhs.match_subdomains;  // Prefer the more explicit policy.
On 2016/06/30 02:24:36, eroman wrote:
> side-comment:  In practice this is probably redundant with the check below,
> since the "exact match" filter has a dot prepended to, so the "host_length"
will
> be greater.
> Although I agree it is clearer to explicitly list this as written.
> ... Also probably has edge case if hostnames are allowed to have trailing dots

Yup, I noticed this too. Here, I'm cargo-culting URLBlacklist, Extensions, and
Content settings - which all lacked comments regarding how this precedence is
worked out. But the unittests yield the right thing, and this is something to
dive into more as a cleanup activity I hope to take on (relating to URLBlacklist
as well)

https://codereview.chromium.org/2102783003/diff/240001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2102783003/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:7932: 'name':
'CertificateTransparencyEnforcementDisabledForUrls',
On 2016/06/30 02:31:56, eroman wrote:
> Why name this "Urls" rather than "Hosts" ?
> 
> The preference names it "Hosts", and that seems more in line with how it
works.
> 
> (It only accepts host filter expressions, not full-blown URL ones).

This was also touched on with policy folks (see comments below), but the term
'hosts' in our policy is already overloaded for another context (you can see the
'hosts' usage), so while I had initially considered it, considering this is
user-facing, it seemed more consistent to document it as URLs as the format,
even if we only use the hostname from the URL.

Using 'hosts' would have introduced yet-another-syntax here with the expression
language, and even moreso related to segmenting and canonicalization & fixups...
so I chose to go consistency w/ the other URL based policies because that's at
least "understood"

https://codereview.chromium.org/2102783003/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:7955: A URL pattern is
formatted according to
https://www.chromium.org/administrators/url-blacklist-filter-format. However,
because certificates are valid for a given hostname independent of the scheme,
port, or path, only the hostname portion of the URL is considered. Wildcard
hosts are not supported.
On 2016/06/30 02:31:56, eroman wrote:
> Is wildcard allowed anywhere in the expression ?
> i.e. http://google.*.com ?

No. That's covered in the url-blacklist-filter-format doc.

> Also I think it would be much simpler to just explain the actual restricted
> grammar rather than as a restricted version of url-blacklist-filter format
> (since the majority of it doesn't apply).

Right, this is why I had double-checked with policy folks, because I could go
either way, but they were happy with it, so I figure they're the ones most
concerned.

[Ryan Sleevi, 2016-06-30 02:57:41]

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

[commit-bot: I haz the power, 2016-06-30 02:57:56]

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, 2016-06-30 03:13:45]

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

[commit-bot: I haz the power, 2016-06-30 03:13:47]

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

[Ryan Sleevi, 2016-06-30 03:45:08]

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

[commit-bot: I haz the power, 2016-06-30 03:45:28]

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, 2016-06-30 04:49:57]

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

[commit-bot: I haz the power, 2016-06-30 04:49:59]

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

[Peter Kasting, 2016-06-30 06:19:51]

RS LGTM

[Ryan Sleevi, 2016-06-30 07:42:33]

Missed Eric's remark about additional tests.

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
File components/certificate_transparency/ct_policy_manager.cc (right):

https://codereview.chromium.org/2102783003/diff/140001/components/certificate...
components/certificate_transparency/ct_policy_manager.cc:196:
conditions->push_back(
On 2016/06/30 02:24:36, eroman wrote:
> I don't think I quite grok how the URLMatcher stuff is supposed to work.
> 
> So there is a vector of Condition sets, where each set just has 1 entry...
> because all conditions in a set need to be matched?

Correct

https://codereview.chromium.org/2102783003/diff/220001/components/certificate...
File components/certificate_transparency/ct_policy_manager_unittest.cc (right):

https://codereview.chromium.org/2102783003/diff/220001/components/certificate...
components/certificate_transparency/ct_policy_manager_unittest.cc:93:
delegate->IsCTRequiredForHost("google.com"));
On 2016/06/30 02:24:36, eroman wrote:
> Are there any tests with:
>   * non-lowercased hostname
>   * trailing dot
>   * IPv6 address
>   * IPv4 address

Beyond the existing URLMatcher code? No.

non-lowercased hostname is part of the API contract (of TransportSecurityState),
even though it's not well-documented as such (TSS already makes assumptions
about canonicalized hostnames internally, albeit somewhat inconsistently)

trailing dot is treated the same as in all the other subsystems (meaning not a
match; you'd have to add both), and the IP cases should never be triggered by
this code (that is, the only use case for excluding hosts from disclosure when
it's required is if you believe there's something secret going on, but issuing
to IANA reserved IPs is verboeten, and so issuing to public IPs *is* a matter of
a public record and the CA should be required to disclose).

[Ryan Sleevi, 2016-06-30 07:45:24]

Description was changed from

==========
Add and connect an Enterprise Policy for whitelisting
hosts as exempt from Certificate Transparency policy.

This introduces a policy
(CertificateTransparencyEnforcementDisabledForUrls) that
allows exempting certain hostnames from the Certificate
Transparency requirements. Some CAs, such as Symantec and
CNNIC at present, are required to disclose their
certificates via CT in order to have them trusted; any
certificate not disclosed is not trusted.

However, to accomodate some enterprise users who have the
capability to manage Chromium consumers, but cannot
manage other certificate-consuming systems on their
network, and which need certificates from these CAs, and
which claim that they cannot have these hosts disclosed
publicly (e.g. "topsecret.internal.example.com"), this
provides a policy mechanism to allow those hosts to be
exempted from CT requirement.

This is not a blanket policy for general hosts on the
Internet; in general, all certificates from these CAs
must conform, unless the device is enterprise managed.

Whether or not this policy ends up being temporary or not
depends on the IETF and CA community, and whether or not
a suitable technical means of redaction can be devised
which allows redaction (e.g. "?.?.example.com") to be
safely performed. For now and the foreseeable future,
redaction is not viable for Chromium, so the enterprise
policy is offered as an alternative.

BUG=620178
==========

to

==========
Add and connect an Enterprise Policy for whitelisting
hosts as exempt from Certificate Transparency policy.

This introduces a policy
(CertificateTransparencyEnforcementDisabledForUrls) that
allows exempting certain hostnames from the Certificate
Transparency requirements. Some CAs, such as Symantec and
CNNIC at present, are required to disclose their
certificates via CT in order to have them trusted; any
certificate not disclosed is not trusted.

However, to accomodate some enterprise users who have the
capability to manage Chromium consumers, but cannot
manage other certificate-consuming systems on their
network, and which need certificates from these CAs, and
which claim that they cannot have these hosts disclosed
publicly (e.g. "topsecret.internal.example.com"), this
provides a policy mechanism to allow those hosts to be
exempted from CT requirement.

This is not a blanket policy for general hosts on the
Internet; in general, all certificates from these CAs
must conform, unless the device is enterprise managed.

Whether or not this policy ends up being temporary or not
depends on the IETF and CA community, and whether or not
a suitable technical means of redaction can be devised
which allows redaction (e.g. "?.?.example.com") to be
safely performed. For now and the foreseeable future,
redaction is not viable for Chromium, so the enterprise
policy is offered as an alternative.

BUG=620178
TBR=atwilson@chromium.org
==========

[Ryan Sleevi, 2016-06-30 07:46:47]

TBRing atwilson, since the files he owned he previously LGTM'd on
https://codereview.chromium.org/2087743002/ (combined into this CL to make sure
we've got full E2E tests)

[Ryan Sleevi, 2016-06-30 07:49:46]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-30 07:49:46]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
isherman@chromium.org, battre@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2102783003/#ps300001
(title: "Comment tweak to remove ()")

[commit-bot: I haz the power, 2016-06-30 07:50:09]

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, 2016-06-30 09:03:22]

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

[commit-bot: I haz the power, 2016-06-30 09:03:24]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-06-30 09:03:30]

Description was changed from

==========
Add and connect an Enterprise Policy for whitelisting
hosts as exempt from Certificate Transparency policy.

This introduces a policy
(CertificateTransparencyEnforcementDisabledForUrls) that
allows exempting certain hostnames from the Certificate
Transparency requirements. Some CAs, such as Symantec and
CNNIC at present, are required to disclose their
certificates via CT in order to have them trusted; any
certificate not disclosed is not trusted.

However, to accomodate some enterprise users who have the
capability to manage Chromium consumers, but cannot
manage other certificate-consuming systems on their
network, and which need certificates from these CAs, and
which claim that they cannot have these hosts disclosed
publicly (e.g. "topsecret.internal.example.com"), this
provides a policy mechanism to allow those hosts to be
exempted from CT requirement.

This is not a blanket policy for general hosts on the
Internet; in general, all certificates from these CAs
must conform, unless the device is enterprise managed.

Whether or not this policy ends up being temporary or not
depends on the IETF and CA community, and whether or not
a suitable technical means of redaction can be devised
which allows redaction (e.g. "?.?.example.com") to be
safely performed. For now and the foreseeable future,
redaction is not viable for Chromium, so the enterprise
policy is offered as an alternative.

BUG=620178
TBR=atwilson@chromium.org
==========

to

==========
Add and connect an Enterprise Policy for whitelisting
hosts as exempt from Certificate Transparency policy.

This introduces a policy
(CertificateTransparencyEnforcementDisabledForUrls) that
allows exempting certain hostnames from the Certificate
Transparency requirements. Some CAs, such as Symantec and
CNNIC at present, are required to disclose their
certificates via CT in order to have them trusted; any
certificate not disclosed is not trusted.

However, to accomodate some enterprise users who have the
capability to manage Chromium consumers, but cannot
manage other certificate-consuming systems on their
network, and which need certificates from these CAs, and
which claim that they cannot have these hosts disclosed
publicly (e.g. "topsecret.internal.example.com"), this
provides a policy mechanism to allow those hosts to be
exempted from CT requirement.

This is not a blanket policy for general hosts on the
Internet; in general, all certificates from these CAs
must conform, unless the device is enterprise managed.

Whether or not this policy ends up being temporary or not
depends on the IETF and CA community, and whether or not
a suitable technical means of redaction can be devised
which allows redaction (e.g. "?.?.example.com") to be
safely performed. For now and the foreseeable future,
redaction is not viable for Chromium, so the enterprise
policy is offered as an alternative.

BUG=620178
TBR=atwilson@chromium.org

Committed: https://crrev.com/96356f8d5e565b402c85b3a4c4a58d58fb594dbd
Cr-Commit-Position: refs/heads/master@{#403125}
==========

[commit-bot: I haz the power, 2016-06-30 09:03:33]

Patchset 16 (id:??) landed as
https://crrev.com/96356f8d5e565b402c85b3a4c4a58d58fb594dbd
Cr-Commit-Position: refs/heads/master@{#403125}

[eroman, 2016-06-30 20:14:42]

lgtm