ChromeOS: Implement CaptivePortalAuthenticationIgnoresProxy policy.

chrome/common/pref_names.cc

Index: chrome/common/pref_names.cc
diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
index 0467d5a38034b1494498ebde830b931859c1f441..d42aa8577d046e6c55460404c8ab1cafdced598d 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -839,6 +839,12 @@ const char kTouchVirtualKeyboardEnabled[] = "ui.touch_virtual_keyboard_enabled";
 // enabled.
 const char kWakeOnWiFiEnabled[] = "settings.internet.wake_on_wifi";
 
+// This is the policy CaptivePortalAuthenticationIgnoresProxy that allows to
+// open Captive Portal authentication pages in a separate window under

[bartfab (slow), 2015/01/14 11:00:07]

Nit 1: s/Captive Portal/captive portal/ (for consistency, you use lowercase
below)
Nit 2: s/under/under a/

[Alexander Alekseev, 2015/01/16 22:51:48]

Done.

+// temporary incognito profile ("signin profile" is used for this purpose),

[bartfab (slow), 2015/01/14 11:00:07]

Do you use the signin profile directly or an incognito profile tied to the
signin profile? In either case, there could be unintended side effects:

1) Incognito profiles are not 100% separated from the underlying profiles.

2) If the same user runs into multiple captive portals in a row (e.g. a user who
roams around a lot), cookies set by one captive portal will still be around when
the user visits the next profile. It may make sense to clean up the profile
whenever captive portal authentication is done.

3) You are using the signin profile as a convenient way to grab a profile not
tied to the current user's settings. What if someone else has the same idea when
implementing a completely unrelated feature? The two features may end up using
the same profile without knowing about each other, potentially leaking data
between their two use cases.

4) The signin profile is not tied to the current user's policy. This may allow
the user to e.g. click through from the captive portal page to so some page that
policy would normally disallow access to.

[Alexander Alekseev, 2015/01/16 22:51:48]

ProfileHelper::GetSigninProfile() returns "profile used to show Signin screen,
Gaia screen, Lock screen (easy unlock, etc...)", which is incognito profile.

This way we are currently using it to show web pages in some special cases,
clearing it in between.
What side-effects do you expect here?
It is already cleared. See
NetworkPortalNotificationController::OnDialogDestroyed();
Signin profile is currently used as "temporary profile" clearing it when needed.
If someone is unaware of this scenario, his new code would break.
Yes. That is why it is disabled by default for enterprise users.

[bartfab (slow), 2015/01/21 10:09:56]

Defense in depth and good security practice: I am not sure what is not fully
separated between regular and icognito profiles right now. More importantly, I
cannot be sure what will and will not be separated in the future. If anything
slips into the sign-in profile, it will persist onto the login screen. This is a
security risk we need to evaluate before landing this feature.
Excellent. Thanks for confirming.
What if the two features try to use temporary profiles concurrently?
Please document this caveat in policy_templates.json. Better be explicit about
this than to cause surprises.

+// which allows to bypass proxy for captive portal authentication.

[bartfab (slow), 2015/01/14 11:00:07]

Nit: s/proxy/the user's proxy/

[Alexander Alekseev, 2015/01/16 22:51:48]

Done.

+const char kCaptivePortalAuthenticationIgnoresProxy[] =
+    "proxy.captive_portal_ignores_proxy";
 #endif  // defined(OS_CHROMEOS)
 
 // The disabled messages in IPC logging.