Enable policy-imported root CA certificates for kiosk and AD sessions

Enable policy-imported root CA certificates for kiosk and AD sessions

This CL enables root CA certificates pushed through user ONC policy for
kiosk and AD sessions. They are not enabled for public sessions.

BUG=715460
Review-Url: https://codereview.chromium.org/2862003002
Cr-Commit-Position: refs/heads/master@{#472901}
Committed: https://chromium.googlesource.com/chromium/src/+/9258f1b1b8e234f289aaf7bd2031c208ecca58ce

[pmarko, 2017-05-04 19:18:54]

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

[pmarko, 2017-05-04 19:18:55]

@Drew:
Do you think we should add a test for this?
We could separate this logic into a static method and unit test that with all
possible user types or something like that.

Actually letting it create a UserNetworkConfigurationUpdater and examining the
result seems like overkill because we'd need to add a query method for
allow_trusted_certificates_from_policy_ in that class and pass in fake objects
etc.

[Andrew T Wilson (Slow), 2017-05-05 12:50:28]

lgtm

[Andrew T Wilson (Slow), 2017-05-05 12:52:41]

On 2017/05/05 12:50:28, Andrew T Wilson (Slow) wrote:
> lgtm

Re: tests - the only test that I think would be useful would be a browser test
that tries to inject a root cert into various types of sessions (regular_user,
kiosk, public sessions) and then checks if the cert is applied. How hard is it
to see if a given cert is part of the trust root set?

If this test is easy/trivial, we should add it, otherwise it's fine as-is.

[pmarko, 2017-05-16 09:58:07]

On 2017/05/05 12:52:41, Andrew T Wilson (Slow) wrote:
> On 2017/05/05 12:50:28, Andrew T Wilson (Slow) wrote:
> > lgtm
> 
> Re: tests - the only test that I think would be useful would be a browser test
> that tries to inject a root cert into various types of sessions (regular_user,
> kiosk, public sessions) and then checks if the cert is applied. How hard is it
> to see if a given cert is part of the trust root set?
> 
> If this test is easy/trivial, we should add it, otherwise it's fine as-is.

I've finally added browsertests for regular user (can use policy certs) and
public session (can not use policy certs), punted kiosk for now because I'd
probably have to set up a fake session manager to get the device policy for
auto-login working. Keeping that on my list.

PTAL.

[pmarko, 2017-05-17 14:36:41]

The CQ bit was checked by pmarko@chromium.org

[pmarko, 2017-05-17 14:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2862003002/#ps20001
(title: "Added browsertests.")

[commit-bot: I haz the power, 2017-05-17 14:37:07]

CQ is trying da patch.

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

[Andrew T Wilson (Slow), 2017-05-17 15:03:55]

I think I asked the question about guest mode before, but already forgot the
answer - maybe a comment is valuable there?

https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:77:
user->GetType() != user_manager::USER_TYPE_PUBLIC_ACCOUNT;
To be clear - what behavior does this expose for guest sessions?

[pmarko, 2017-05-17 15:18:07]

Hm, good point! IIRC, there's no way to get user policy onto guest sessions
(right?) so it's not really relevant, see comment on the file.

https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:77:
user->GetType() != user_manager::USER_TYPE_PUBLIC_ACCOUNT;
On 2017/05/17 15:03:54, Andrew T Wilson (Slow) wrote:
> To be clear - what behavior does this expose for guest sessions?

This is only relevant for user policy, and guest sessions don't have user
policy.
Do you think a comment is enough here or should we exclude USER_GUEST just to be
sure?

[Andrew T Wilson (Slow), 2017-05-17 15:48:03]

On 2017/05/17 15:18:07, pmarko wrote:
> Hm, good point! IIRC, there's no way to get user policy onto guest sessions
> (right?) so it's not really relevant, see comment on the file.
> 
>
https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
> File
> chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
>
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:77:
> user->GetType() != user_manager::USER_TYPE_PUBLIC_ACCOUNT;
> On 2017/05/17 15:03:54, Andrew T Wilson (Slow) wrote:
> > To be clear - what behavior does this expose for guest sessions?
> 
> This is only relevant for user policy, and guest sessions don't have user
> policy.
> Do you think a comment is enough here or should we exclude USER_GUEST just to
be
> sure?

Probably a DCHECK_NE(type, USER_GUEST) to show intent is good enough.

[pmarko, 2017-05-17 15:58:07]

On 2017/05/17 15:48:03, Andrew T Wilson (Slow) wrote:
> On 2017/05/17 15:18:07, pmarko wrote:
> > Hm, good point! IIRC, there's no way to get user policy onto guest sessions
> > (right?) so it's not really relevant, see comment on the file.
> > 
> >
>
https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
> > File
> > chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
> >
>
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:77:
> > user->GetType() != user_manager::USER_TYPE_PUBLIC_ACCOUNT;
> > On 2017/05/17 15:03:54, Andrew T Wilson (Slow) wrote:
> > > To be clear - what behavior does this expose for guest sessions?
> > 
> > This is only relevant for user policy, and guest sessions don't have user
> > policy.
> > Do you think a comment is enough here or should we exclude USER_GUEST just
to
> be
> > sure?
> 
> Probably a DCHECK_NE(type, USER_GUEST) to show intent is good enough.

Good idea. Will do that in a followup CL along with any browsertest related
comments, as the CQ is already running.

[commit-bot: I haz the power, 2017-05-17 18:01:58]

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

[commit-bot: I haz the power, 2017-05-17 18:01:58]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pmarko, 2017-05-18 11:26:13]

On 2017/05/17 15:58:07, pmarko wrote:
> On 2017/05/17 15:48:03, Andrew T Wilson (Slow) wrote:
> > On 2017/05/17 15:18:07, pmarko wrote:
> > > Hm, good point! IIRC, there's no way to get user policy onto guest
sessions
> > > (right?) so it's not really relevant, see comment on the file.
> > > 
> > >
> >
>
https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
> > > File
> > >
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2862003002/diff/20001/chrome/browser/chromeos...
> > >
> >
>
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:77:
> > > user->GetType() != user_manager::USER_TYPE_PUBLIC_ACCOUNT;
> > > On 2017/05/17 15:03:54, Andrew T Wilson (Slow) wrote:
> > > > To be clear - what behavior does this expose for guest sessions?
> > > 
> > > This is only relevant for user policy, and guest sessions don't have user
> > > policy.
> > > Do you think a comment is enough here or should we exclude USER_GUEST just
> to
> > be
> > > sure?
> > 
> > Probably a DCHECK_NE(type, USER_GUEST) to show intent is good enough.
> 
> Good idea. Will do that in a followup CL along with any browsertest related
> comments, as the CQ is already running.

Turns out I can't guarantee that this class won't be created for a guest profile
(even though the guest profile will never get user policy), so I've added the !=
USER_TYPE_GUEST check just to go sure.
Deatil: profile_io_data.cc calls PolicyCertServiceFactory::CreateForProfile
which calls UserNetworkConfigurationUpdaterFactory::GetForProfile.

Also, moved the logic to a separate method because it looks better IMO.

[pmarko, 2017-05-18 17:42:03]

The CQ bit was checked by pmarko@chromium.org

[pmarko, 2017-05-18 17:42:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2862003002/#ps40001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2017-05-18 17:42:58]

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-18 19:29:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495129323709580,
"parent_rev": "f073ac02784b6a426a86e20c4623c09ae2a211ba", "commit_rev":
"9258f1b1b8e234f289aaf7bd2031c208ecca58ce"}

[commit-bot: I haz the power, 2017-05-18 19:29:54]

Description was changed from

==========
Enable policy-imported root CA certificates for kiosk and AD sessions

This CL enables root CA certificates pushed through user ONC policy for
kiosk and AD sessions. They are not enabled for public sessions.

BUG=715460
==========

to

==========
Enable policy-imported root CA certificates for kiosk and AD sessions

This CL enables root CA certificates pushed through user ONC policy for
kiosk and AD sessions. They are not enabled for public sessions.

BUG=715460

Review-Url: https://codereview.chromium.org/2862003002
Cr-Commit-Position: refs/heads/master@{#472901}
Committed:
https://chromium.googlesource.com/chromium/src/+/9258f1b1b8e234f289aaf7bd2031...
==========

[commit-bot: I haz the power, 2017-05-18 19:30:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9258f1b1b8e234f289aaf7bd2031...