Only import certificates with Web trust from ONC if the user is managed and matches the enterprise …

Only import certificates with Web trust from ONC if the user is managed and matches the enterprise domain of the device.

BUG=chromium-os:33879
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155390

[Joao da Silva, 2012-08-27 14:27:25]

Please review, thanks!

@mnissler: overall thing, as discussed by email

@gspencer: ONC parser changes, does this make sense to you?

[Greg Spencer (Chromium), 2012-08-27 17:39:28]

ONC parser changes LGTM.

[Mattias Nissler (ping if slow), 2012-08-28 11:57:56]

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:333: void
OncNetworkParser::AllowWebTrustFromPolicy() {
Make this a regular setter? When I read the declaration, I wasn't sure what it
would do, just setting a flag or actually attempting to set the web trust?

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:865: return NULL;
I don't think this is a good idea, as it'll abort the certificate import process
(correct me if I'm wrong). That means that you'll essentially break ONC
configuration that specifies this on non-enrolled (and foreign) devices. I guess
the only chance we have here is to not set the web trust bit and silently bail
out.

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/policy/netwo...
File chrome/browser/policy/network_configuration_updater.h (right):

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/policy/netwo...
chrome/browser/policy/network_configuration_updater.h:34: chromeos::UserManager*
user_manager);
These are some heavy-weight dependencies (and indeed, we're cyclic due to
BrowserPolicyConnector). Would it be possible to just put a boolean flag into
this class that's updated appropriately when the user logs in?

[Joao da Silva, 2012-09-03 15:24:32]

@mnissler: thanks for the review! PTAL

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:333: void
OncNetworkParser::AllowWebTrustFromPolicy() {
On 2012/08/28 11:57:56, Mattias Nissler wrote:
> Make this a regular setter? When I read the declaration, I wasn't sure what it
> would do, just setting a flag or actually attempting to set the web trust?

Turned into a setter.

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:865: return NULL;
On 2012/08/28 11:57:56, Mattias Nissler wrote:
> I don't think this is a good idea, as it'll abort the certificate import
process
> (correct me if I'm wrong). That means that you'll essentially break ONC
> configuration that specifies this on non-enrolled (and foreign) devices. I
guess
> the only chance we have here is to not set the web trust bit and silently bail
> out.

You're right, thanks! This is now dropping the web trust bit only.

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/policy/netwo...
File chrome/browser/policy/network_configuration_updater.h (right):

http://codereview.chromium.org/10868076/diff/3002/chrome/browser/policy/netwo...
chrome/browser/policy/network_configuration_updater.h:34: chromeos::UserManager*
user_manager);
On 2012/08/28 11:57:56, Mattias Nissler wrote:
> These are some heavy-weight dependencies (and indeed, we're cyclic due to
> BrowserPolicyConnector). Would it be possible to just put a boolean flag into
> this class that's updated appropriately when the user logs in?

Done, moved the logic to chrome_browser_main_chromeos.cc.

[Mattias Nissler (ping if slow), 2012-09-04 09:57:04]

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:418: void
ChromeBrowserMainPartsChromeos::PostProfileInit() {
Does this also run for the login profile? I guess not, in which case the code
below will never get a logged-in user and therefore fail.

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:446:
!user_manager->IsLoggedInAsGuest() &&
I guess we should rather check of regular logins to avoid retail and other
upcoming stuff?

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:955: if ((onc_source_ ==
NetworkUIData::ONC_SOURCE_DEVICE_POLICY ||
only for onc_source_ == USER_POLICY

[Joao da Silva, 2012-09-04 12:52:55]

The ownership of NetworkConfigurationUpdater has changed, see inline. PTAL.

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:418: void
ChromeBrowserMainPartsChromeos::PostProfileInit() {
On 2012/09/04 09:57:04, Mattias Nissler wrote:
> Does this also run for the login profile? I guess not, in which case the code
> below will never get a logged-in user and therefore fail.

Yikes, of course. This means that the NetworkConfigurationUpdater has to be
created before sign-in (for device-level ONC before sign-in), and after sign-in
it has to receive an updated flag (once we can determine if the user is managed
or not). It also means that it can't be owned from here, since the instance
isn't exposed anywhere.

I've moved ownership of the NCU to the BrowserPolicyConnector for this reason,
and update the flag from there after the user signs in.

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:446:
!user_manager->IsLoggedInAsGuest() &&
On 2012/09/04 09:57:04, Mattias Nissler wrote:
> I guess we should rather check of regular logins to avoid retail and other
> upcoming stuff?

Yes, that's more future-proof :-) With the NCU in the BPC this isn't an issue,
since InitializeUserPolicy() is only called for "real" users.

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10868076/diff/20001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:955: if ((onc_source_ ==
NetworkUIData::ONC_SOURCE_DEVICE_POLICY ||
On 2012/09/04 09:57:04, Mattias Nissler wrote:
> only for onc_source_ == USER_POLICY

Done. For DEVICE_POLICY this is now bailing out without importing the
certificate.

http://codereview.chromium.org/10868076/diff/19003/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (left):

http://codereview.chromium.org/10868076/diff/19003/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:566:
network_config_updater_.reset();
This isn't an issue when the NCU is owned by the BPC, since the BPC is owned by
g_browser_process.

[Mattias Nissler (ping if slow), 2012-09-05 11:19:05]

LGTM

http://codereview.chromium.org/10868076/diff/29001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/10868076/diff/29001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:185: GetUserAffiliation(email)
== USER_AFFILIATION_MANAGED;
Ah, I guess I should have refrained from publishing that hacked-up CL back then
;)

http://codereview.chromium.org/10868076/diff/29001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/10868076/diff/29001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:226:
scoped_ptr<NetworkConfigurationUpdater> network_configuration_updater_;
As discussed offline, we probably want to separate out the policy enforcement
bits to a new kitchen sink class in an effort to keep responsibilities clear.
Let's do that in a separate effort though.

[commit-bot: I haz the power, 2012-09-05 11:27:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10868076/29001

[commit-bot: I haz the power, 2012-09-05 11:27:17]

Presubmit check for 10868076-29001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui/net_internals
    chrome/browser/chromeos/cros
    chrome/browser/chromeos
    chrome/browser/chromeos/login

Presubmit checks took 2.1s to calculate.

[Joao da Silva, 2012-09-05 11:36:34]

@nkostylev: please OWNER check chromeos/
@jhawkins: please OWNER check ui/

Thanks!

[James Hawkins, 2012-09-05 13:48:07]

ui lgtm

[Nikita (slow), 2012-09-05 17:44:57]

lgtm but you still need someone to cover  chrome/browser/chromeos/cros

[Joao da Silva, 2012-09-05 19:26:59]

@satorux: can you do an OWNER check on chromeos/cros? Thanks!

[satorux1, 2012-09-06 17:53:27]

cros/ LGTM

[commit-bot: I haz the power, 2012-09-06 18:43:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10868076/29001

[commit-bot: I haz the power, 2012-09-07 04:32:41]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_chromeos, revision is 155251, job name
was 10868076-29001 (retry) (retry) (previous was lost) (previous was lost).

[commit-bot: I haz the power, 2012-09-07 08:36:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10868076/31003

[commit-bot: I haz the power, 2012-09-07 10:37:11]

Try job failure for 10868076-31003 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...