Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159103006:
Add policy provider that would filter extensions/apps allowed on the (Closed)
|
Created:
4 years, 5 months ago by Denis Kuznetsov (DE-MUC) Modified:
4 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd policy provider that would filter extensions/apps allowed on the
sign-in screen.
BUG=626342
Committed: https://crrev.com/5876956faa38aef22e53b4a8712d5a87c4980578
Cr-Commit-Position: refs/heads/master@{#431874}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 22
Patch Set 4 : Addressed first set of comments #
Total comments: 38
Patch Set 5 : Use features instead of manual parsing #Patch Set 6 : Address some nits #
Total comments: 10
Patch Set 7 : Addressed review comments, single behavior feature now #
Total comments: 8
Patch Set 8 : Merged #Patch Set 9 : Addressed comments #
Total comments: 6
Patch Set 10 : addressed comments #Patch Set 11 : Added tests #
Total comments: 14
Patch Set 12 : Addressed comments #
Total comments: 21
Patch Set 13 : Add a way to bypass checks for tests #
Total comments: 17
Patch Set 14 : Address comments, fix tests #Patch Set 15 : Fix typo #Messages
Total messages: 49 (16 generated)
Description was changed from ========== Add policy provider that would filter extensions/apps allowed on the sign-in screen. BUG=626342 ========== to ========== Add policy provider that would filter extensions/apps allowed on the sign-in screen. BUG=626342 ==========
antrim@chromium.org changed reviewers: + asargent@chromium.org, emaxx@chromium.org
https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:39: "storage", You should use APIPermission::ID here instead of strings, and rename to something like kSafePermissions. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:45: const char* const kSafePermissionDicts[] = { Same thing here, although I wonder if you should even leave this in if we aren't using it yet? https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:50: // Checks if given this provider applies to given extension. grammar nit: "if given this provider applies to given" maybe you meant "Checks if this provider applies to given extension." ? https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:59: // Checks if given this provider applies to given extension. same grammar nit here https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:64: LOG(ERROR) << extension->id() https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... "Remove most logging calls before checking in. Unless you're adding temporary logging to track down a specific bug, and you have a plan for how to collect the logged data from user machines, you should generally not add logging statements. For the rare case when logging needs to stay in the codebase for a while, prefer DVLOG(1) to other logging methods. This avoids bloating the release executable and in debug can be selectively enabled at runtime by command-line arguments " Do you really need this logging? (here and elsewhere) https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:136: } The code in the above couple of blocks seems a little too tightly coupled to the implementation details of how Extension objects get their permissions from a manifest. What I think would be better is if you instead used the permissions_data() accessor on the Extension and then called PermissionsData::HasAPIPermission with the APIPermission::ID from your list of allowed permissions from above in the file. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:156: if (it.key() != "background") { I have a similar concern here about this being too tightly coupled to the implementation details of a manifest. What is it you are trying to actually prevent? I.e. what is it in the "app" section of an app's manifest leads to giving it an ability that you're trying to prevent here? If we don't already have a way for you to get that information in a structured way (without having to try and re-parse the manifest) from an Extension object or its permissions_data(), perhaps we can add a way. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:166: int version; Instead of examining the dictionary value from the manifest to determine this, you should instead just call: extension->manifest()->GetManifestVersion() https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.h (right): https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.h:23: #endif // defined(OS_CHROMEOS) I've been trying to have us move away from using #ifdef's to separate chromeos parts of code (since that can make things pretty hard to read) in favor of other approaches like using delegate interfaces, subclasses, listeners, etc. As an example, in this CL from a while back: https://codereview.chromium.org/1869483002/diff/1/chrome/browser/extensions/e... instead of adding more #ifdef chromeos blocks, we instead added a generic interface: https://codereview.chromium.org/1890163004 I think we should try and do something similar here.
https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:39: "storage", On 2016/07/22 00:13:29, Antony Sargent wrote: > You should use APIPermission::ID here instead of strings, and rename to > something like kSafePermissions. See reasoning below. I agree that we'd better to have constants (for static typing), but direct enum-based approach will not work (see below), and there is no easy way to get string based on enum. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:50: // Checks if given this provider applies to given extension. On 2016/07/22 00:13:28, Antony Sargent wrote: > grammar nit: "if given this provider applies to given" > > maybe you meant "Checks if this provider applies to given extension." ? > Done. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:59: // Checks if given this provider applies to given extension. On 2016/07/22 00:13:29, Antony Sargent wrote: > same grammar nit here Done. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:64: LOG(ERROR) << extension->id() On 2016/07/22 00:13:28, Antony Sargent wrote: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > "Remove most logging calls before checking in. Unless you're adding temporary > logging to track down a specific bug, and you have a plan for how to collect the > logged data from user machines, you should generally not add logging statements. > > For the rare case when logging needs to stay in the codebase for a while, prefer > DVLOG(1) to other logging methods. This avoids bloating the release executable > and in debug can be selectively enabled at runtime by command-line arguments > " > > Do you really need this logging? (here and elsewhere) Yes. Because with ChromeOS login screen the extension developer will not have much debugging capabilities, logs are one of the very few. So we want to make it clear for developer what could block the extension - that's the reason for extensive logging, and using ERROR level. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:136: } On 2016/07/22 00:13:28, Antony Sargent wrote: > The code in the above couple of blocks seems a little too tightly coupled to the > implementation details of how Extension objects get their permissions from a > manifest. What I think would be better is if you instead used the > permissions_data() accessor on the Extension and then called > PermissionsData::HasAPIPermission with the APIPermission::ID from your list of > allowed permissions from above in the file. We do not want to "allow app if it has permission". We want to "block all apps that have any permissions other than above". And PermissionsData does not allow iterating over all permissions - only checks if it has one. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:156: if (it.key() != "background") { On 2016/07/22 00:13:28, Antony Sargent wrote: > I have a similar concern here about this being too tightly coupled to the > implementation details of a manifest. What is it you are trying to actually > prevent? We want to allow 3rd-party apps on chromeos sign-in screen to enable hardware token-based flow in SAML signin process (e.g. login by smartcards). But we want to limit these app as hard as possible so that such apps will not spoil sign-in UI nor will lower the sign-in flow security. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:166: int version; On 2016/07/22 00:13:29, Antony Sargent wrote: > Instead of examining the dictionary value from the manifest to determine this, > you should instead just call: > > extension->manifest()->GetManifestVersion() Done. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.h (right): https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.h:23: #endif // defined(OS_CHROMEOS) On 2016/07/22 00:13:29, Antony Sargent wrote: > I've been trying to have us move away from using #ifdef's to separate chromeos > parts of code (since that can make things pretty hard to read) in favor of other > approaches like using delegate interfaces, subclasses, listeners, etc. > > As an example, in this CL from a while back: > > https://codereview.chromium.org/1869483002/diff/1/chrome/browser/extensions/e... > > instead of adding more #ifdef chromeos blocks, we instead added a generic > interface: > > https://codereview.chromium.org/1890163004 > > I think we should try and do something similar here. That was just a move from extension_system.h. I agree that adding new #ifdefs complicates the code, but this CL only adds some code inside already-existing CHROMEOS ifdefs. And while your suggestion seems very reasonable, I believe that should be the scope of separate CL (as it would also affect existing code, like DeviceLocalAccountManagementPolicyProvider).
https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:64: LOG(ERROR) << extension->id() On 2016/07/22 14:37:14, Denis Kuznetsov (DE-MUC) wrote: > On 2016/07/22 00:13:28, Antony Sargent wrote: > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > "Remove most logging calls before checking in. Unless you're adding temporary > > logging to track down a specific bug, and you have a plan for how to collect > the > > logged data from user machines, you should generally not add logging > statements. > > > > For the rare case when logging needs to stay in the codebase for a while, > prefer > > DVLOG(1) to other logging methods. This avoids bloating the release executable > > and in debug can be selectively enabled at runtime by command-line arguments > > " > > > > Do you really need this logging? (here and elsewhere) > > Yes. Because with ChromeOS login screen the extension developer will not have > much debugging capabilities, logs are one of the very few. So we want to make it > clear for developer what could block the extension - that's the reason for > extensive logging, and using ERROR level. Ok, that makes sense. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:136: } On 2016/07/22 14:37:14, Denis Kuznetsov (DE-MUC) wrote: > On 2016/07/22 00:13:28, Antony Sargent wrote: > > The code in the above couple of blocks seems a little too tightly coupled to > the > > implementation details of how Extension objects get their permissions from a > > manifest. What I think would be better is if you instead used the > > permissions_data() accessor on the Extension and then called > > PermissionsData::HasAPIPermission with the APIPermission::ID from your list of > > allowed permissions from above in the file. > > We do not want to "allow app if it has permission". We want to "block all apps > that have any permissions other than above". > And PermissionsData does not allow iterating over all permissions - only checks > if it has one. Do you just care about API permissions or other types of permissions like host access or content scripts? If you just care about API permissions, you can get the PermissionsData for the extension, take the union of the active and withheld API PermissionSet's, then take the difference of that against a manually constructed set of PermissionSet's that you want to allow and see if the result is empty. If that's still not enough, we can add additional structured access via APIs, as I really don't like the idea of having code outside the extensions system using direct string-based access against the raw manifest DictionaryValue. https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.h (right): https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.h:23: #endif // defined(OS_CHROMEOS) On 2016/07/22 14:37:14, Denis Kuznetsov (DE-MUC) wrote: > On 2016/07/22 00:13:29, Antony Sargent wrote: > > I've been trying to have us move away from using #ifdef's to separate chromeos > > parts of code (since that can make things pretty hard to read) in favor of > other > > approaches like using delegate interfaces, subclasses, listeners, etc. > > > > As an example, in this CL from a while back: > > > > > https://codereview.chromium.org/1869483002/diff/1/chrome/browser/extensions/e... > > > > instead of adding more #ifdef chromeos blocks, we instead added a generic > > interface: > > > > https://codereview.chromium.org/1890163004 > > > > I think we should try and do something similar here. > > That was just a move from extension_system.h. > I agree that adding new #ifdefs complicates the code, but this CL only adds some > code inside already-existing CHROMEOS ifdefs. > And while your suggestion seems very reasonable, I believe that should be the > scope of separate CL (as it would also affect existing code, like > DeviceLocalAccountManagementPolicyProvider). It would be fine to do this in a separate CL, but I'd like to see that land before this one.
https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:136: } On 2016/07/25 21:27:01, Antony Sargent wrote: > On 2016/07/22 14:37:14, Denis Kuznetsov (DE-MUC) wrote: > > On 2016/07/22 00:13:28, Antony Sargent wrote: > > > The code in the above couple of blocks seems a little too tightly coupled to > > the > > > implementation details of how Extension objects get their permissions from a > > > manifest. What I think would be better is if you instead used the > > > permissions_data() accessor on the Extension and then called > > > PermissionsData::HasAPIPermission with the APIPermission::ID from your list > of > > > allowed permissions from above in the file. > > > > We do not want to "allow app if it has permission". We want to "block all apps > > that have any permissions other than above". > > And PermissionsData does not allow iterating over all permissions - only > checks > > if it has one. > > Do you just care about API permissions or other types of permissions like host > access or content scripts? > > If you just care about API permissions, you can get the PermissionsData for the > extension, take the union of the active and withheld API PermissionSet's, then > take the difference of that against a manually constructed set of > PermissionSet's that you want to allow and see if the result is empty. > > If that's still not enough, we can add additional structured access via APIs, as > I really don't like the idea of having code outside the extensions system using > direct string-based access against the raw manifest DictionaryValue. I would agree that the low-level parsing should ideally be kept separate from this logic. On the other side, the resulting solution should leave no gray areas (about whose security it's difficult to reason) and, ideally, sustain against adding new permissions and new manifest keys without risks of opening the security gaps. Looking through the app manifest documentation, the following keys look dangerous and probably have to be prohibited in our case (note that this list is the very rough estimation): "bluetooth" (this one is actually under discussions), "commands", "event_rules", "file_handlers", "nacl_modules", "url_handlers", "usb_printers", "usb_printers". This diverse (and yet incomplete) list explains why the way of explicit whitelisting looked more robust to us. Or, Antony, do you maybe see an option to make such kind of whitelisting through some structured API? https://codereview.chromium.org/2159103006/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2159103006/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:4127: + <message name="IDS_EXTENSION_CANT_INSTALL_ON_LOGIN_SCREEN" desc="Error message when the administrator tries to force-install through policy an extension that is not allowed on a local screen."> nit: s/a local screen/the login screen/ https://codereview.chromium.org/2159103006/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:4127: + <message name="IDS_EXTENSION_CANT_INSTALL_ON_LOGIN_SCREEN" desc="Error message when the administrator tries to force-install through policy an extension that is not allowed on a local screen."> In one places in the CL the "signin screen" term is used, in the other - the "login screen" term. It would make sense to use some consistent name. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:28: const char* const kWhitelistedApps[]{ nit: Please use some consistent style, either " = {" (like with the next constant) or "{". https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:29: "kmendfapggjehodndflmmgagdbamhnfd", // Gnubby component extension. Is this needed? The code below seems to allow the component extensions. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:30: "beknehfpfkghjoafdifaflglpjkojoco", // Gnubby app Is it confirmed that this app is normally installed and running on the signin screen? I may be wrong, but I thought that for gnubby authentication only the component extension is required, while the app is only for the user sessions (and provides more functionality like UI for updating the key, etc.). https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:31: "ljoammodoonkhnehlncldjelhidljdpi", // Genius app (help) This and the next one also seem to be the component extensions, which shouldn't be required to listed here. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:37: "usb", nit: Please sort the list lexicographically. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:44: // for permission strings (and for more documentation). nit: s/ (and for more documentation)// (there's no documentation for kSafePermissionStrings). https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:46: // No dictionary permissions are allowed at the moment. Actually, we already need "usbDevices" dictionary permission for the Smart Card Connector app (id khpfeaanjngmcnplbdlpegiifgpfgdco). It's closely related to the "usb" permission: https://developer.chrome.com/apps/app_usb#manifest https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:52: if (extension->location() == extensions::Manifest::COMPONENT) { Shouldn't EXTERNAL_COMPONENT be also specified here? According to its documentation, it also corresponds to the component extensions (in the sense that it also represents an internal piece of Chrome). BTW, there's a function Manifest::IsComponentLocation. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:56: return true; Not sure, but it probably also makes sense to always allow the case of COMMAND_LINE. This may be useful for the development purposes. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:59: // Checks if this provider applies to given extension. nit: Please fix the comment. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:68: if (extension->location() == extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD || IMO, the logic would be simpler if you would negate the condition here. Because currently it's a bit difficult to read this function and understand which cases lead to "true" and which to "false". https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:68: if (extension->location() == extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD || BTW, there's a function Manifest::IsPolicyLocation. (Though not insisting, as some may consider explicit enum values to be more readable.) https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:73: << " location is prohibited: " << extension->location(); I haven't found operator<< for Manifest::Location. So if this prints its numeric value, then it's not very good log message. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:78: bool ArrayContains(const char* const char_array[], const std::string& entry) { You may consider passing the array as a reference with a template length parameter. This would allow to rid of these sentinel nullptrs, and use the STL algorithm instead of the raw loop. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:87: // Returns true for platform apps that are considered safe for Public Sessions, nit: s/Public Sessions/the login screen/ (or signin screen). https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:89: // contained in the |kSafeManifestEntries| whitelist and all permissions to be nit: Remove references to kSafeManifestEntries. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:144: return false; Shouldn't this be "safe = false", as in other similar cases in this function? https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:156: if (it.key() != "background") { This check would fail for the current versions of our example login screen apps (Smart Card Connector app khpfeaanjngmcnplbdlpegiifgpfgdco and Charismathics Smart Card extension haeblkpifdemlfnkogkipmghfcbonief): they also contain the "persistent" key (which is actually redundant in these cases and doesn't change any default behavior, but this still shows that the check is too restrictive). https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:173: // Everything else is a warning. Looking at our examples of the login screen apps, looks like this would produce too much useless warnings: "name", "short_name", "description", "version", "minimum_chrome_version", "default_locale", "icons", "storage", "update_url". https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.h (right): https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: s/2013/2016/ https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.h:8: #include "base/compiler_specific.h" nit: Please fix the includes in this and other new files. E.g. here seems that "base/compiler_specific.h" and "chrome/browser/chromeos/policy/device_local_account.h" are not actually used, and <string>, "extensions/common/extension.h", "base/strings/string16.h" are missing. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:132: // We can not perform check for Signin Profile here, as it would result in If the profile creation is really the only reason for doing this, then what about implementing this check for signin profile more accurately, e.g. by comparing the profile path? And such helper function could be put into profile_helper.h.
https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:136: } On 2016/07/26 02:55:30, emaxx wrote: > On 2016/07/25 21:27:01, Antony Sargent wrote: > > On 2016/07/22 14:37:14, Denis Kuznetsov (DE-MUC) wrote: > > > On 2016/07/22 00:13:28, Antony Sargent wrote: > > > > The code in the above couple of blocks seems a little too tightly coupled > to > > > the > > > > implementation details of how Extension objects get their permissions from > a > > > > manifest. What I think would be better is if you instead used the > > > > permissions_data() accessor on the Extension and then called > > > > PermissionsData::HasAPIPermission with the APIPermission::ID from your > list > > of > > > > allowed permissions from above in the file. > > > > > > We do not want to "allow app if it has permission". We want to "block all > apps > > > that have any permissions other than above". > > > And PermissionsData does not allow iterating over all permissions - only > > checks > > > if it has one. > > > > Do you just care about API permissions or other types of permissions like host > > access or content scripts? > > > > If you just care about API permissions, you can get the PermissionsData for > the > > extension, take the union of the active and withheld API PermissionSet's, then > > take the difference of that against a manually constructed set of > > PermissionSet's that you want to allow and see if the result is empty. > > > > If that's still not enough, we can add additional structured access via APIs, > as > > I really don't like the idea of having code outside the extensions system > using > > direct string-based access against the raw manifest DictionaryValue. > > I would agree that the low-level parsing should ideally be kept separate from > this logic. > > On the other side, the resulting solution should leave no gray areas (about > whose security it's difficult to reason) and, ideally, sustain against adding > new permissions and new manifest keys without risks of opening the security > gaps. > > Looking through the app manifest documentation, the following keys look > dangerous and probably have to be prohibited in our case (note that this list is > the very rough estimation): "bluetooth" (this one is actually under > discussions), "commands", "event_rules", "file_handlers", "nacl_modules", > "url_handlers", "usb_printers", "usb_printers". > > This diverse (and yet incomplete) list explains why the way of explicit > whitelisting looked more robust to us. > > Or, Antony, do you maybe see an option to make such kind of whitelisting through > some structured API? Yes, one of the reasons I'm arguing for structured API access instead of unstructured manifest DictionaryValue manual parsing is precisely that we sometimes have introduced new "features" via top-level manifest keys instead of permissions entries, such as the examples you cite. On the other hand, sometimes we introduce new manifest keys that would have no security implications, but could become required always or for certain kinds of extensions/apps (eg "manifest_version", or "requirements"). I'd much rather that we have an API somewhere in the extensions system itself that lets you assert "the app is only allowed to do x,y,z" that is close to where we'd be making these changes, so that it can evolve without the code in this file having to know about manifest keys. If it's not just API permissions but more generally "extensions system features" that you're concerned with, we already have a pretty flexible declarative system for describing these and limiting access to them in various ways. See the _*features.json files in chrome/common/extensions/api/ and extensions/common/api/, which let us do things like restrict access based on extension type, javascript context type, extension id (whitelist), etc. It could make a lot of sense to add a new type of restriction, something like "allowed_on_signin_screen": true, and probably we'd want to implement your whitelist that way too.
On 2016/07/27 21:00:14, Antony Sargent wrote: > > Yes, one of the reasons I'm arguing for structured API access instead of > unstructured manifest DictionaryValue manual parsing is precisely that we > sometimes have introduced new "features" via top-level manifest keys instead of > permissions entries, such as the examples you cite. On the other hand, sometimes > we introduce new manifest keys that would have no security implications, but > could become required always or for certain kinds of extensions/apps (eg > "manifest_version", or "requirements"). I'd much rather that we have an API > somewhere in the extensions system itself that lets you assert "the app is only > allowed to do x,y,z" that is close to where we'd be making these changes, so > that it can evolve without the code in this file having to know about manifest > keys. > > If it's not just API permissions but more generally "extensions system features" > that you're concerned with, we already have a pretty flexible declarative system > for describing these and limiting access to them in various ways. See the > _*features.json files in chrome/common/extensions/api/ and > extensions/common/api/, which let us do things like restrict access based on > extension type, javascript context type, extension id (whitelist), etc. It could > make a lot of sense to add a new type of restriction, something like > "allowed_on_signin_screen": true, and probably we'd want to implement your > whitelist that way too. Ping so that this CL is easier to find.
Please take a look again. This CL was rewritten to use Features approach in extensions. Now it does not cover API usage checks, that would be done in a separate CL. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:28: const char* const kWhitelistedApps[]{ On 2016/07/26 02:55:31, emaxx wrote: > nit: Please use some consistent style, either " = {" (like with the next > constant) or "{". Gone. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:29: "kmendfapggjehodndflmmgagdbamhnfd", // Gnubby component extension. On 2016/07/26 02:55:31, emaxx wrote: > Is this needed? The code below seems to allow the component extensions. Gone. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:30: "beknehfpfkghjoafdifaflglpjkojoco", // Gnubby app On 2016/07/26 02:55:31, emaxx wrote: > Is it confirmed that this app is normally installed and running on the signin > screen? > I may be wrong, but I thought that for gnubby authentication only the component > extension is required, while the app is only for the user sessions (and provides > more functionality like UI for updating the key, etc.). Gone. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:31: "ljoammodoonkhnehlncldjelhidljdpi", // Genius app (help) On 2016/07/26 02:55:31, emaxx wrote: > This and the next one also seem to be the component extensions, which shouldn't > be required to listed here. Gone. But we explicitly block all component extensions on login screen, so we want to whitelist these ones. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:37: "usb", On 2016/07/26 02:55:31, emaxx wrote: > nit: Please sort the list lexicographically. Gone. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:44: // for permission strings (and for more documentation). On 2016/07/26 02:55:32, emaxx wrote: > nit: s/ (and for more documentation)// > (there's no documentation for kSafePermissionStrings). Here and below : Gone. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:46: // No dictionary permissions are allowed at the moment. On 2016/07/26 02:55:31, emaxx wrote: > Actually, we already need "usbDevices" dictionary permission for the Smart Card > Connector app (id khpfeaanjngmcnplbdlpegiifgpfgdco). It's closely related to the > "usb" permission: https://developer.chrome.com/apps/app_usb#manifest Acknowledged. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:52: if (extension->location() == extensions::Manifest::COMPONENT) { On 2016/07/26 02:55:31, emaxx wrote: > Shouldn't EXTERNAL_COMPONENT be also specified here? > According to its documentation, it also corresponds to the component extensions > (in the sense that it also represents an internal piece of Chrome). > > BTW, there's a function Manifest::IsComponentLocation. Acknowledged. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:56: return true; On 2016/07/26 02:55:31, emaxx wrote: > Not sure, but it probably also makes sense to always allow the case of > COMMAND_LINE. This may be useful for the development purposes. Acknowledged. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:59: // Checks if this provider applies to given extension. On 2016/07/26 02:55:31, emaxx wrote: > nit: Please fix the comment. Acknowledged. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.h (right): https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/07/26 02:55:32, emaxx wrote: > nit: s/2013/2016/ Done. I believe at some point there was (or it was just an idea?) a script that updated year upon the commit of modified file. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.h:8: #include "base/compiler_specific.h" On 2016/07/26 02:55:32, emaxx wrote: > nit: Please fix the includes in this and other new files. > E.g. here seems that "base/compiler_specific.h" and > "chrome/browser/chromeos/policy/device_local_account.h" are not actually used, > and <string>, "extensions/common/extension.h", "base/strings/string16.h" are > missing. Done. https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:132: // We can not perform check for Signin Profile here, as it would result in On 2016/07/26 02:55:32, emaxx wrote: > If the profile creation is really the only reason for doing this, then what > about implementing this check for signin profile more accurately, e.g. by > comparing the profile path? And such helper function could be put into > profile_helper.h. I prefer to have explicit check on instance. For example, path check would work for both signin profile, and it's parent (e.g. non-incognito profile). And we want to enable extensions only for non-incognito profile.
https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:31: "ljoammodoonkhnehlncldjelhidljdpi", // Genius app (help) On 2016/08/29 15:14:40, Denis Kuznetsov (DE-MUC) wrote: > On 2016/07/26 02:55:31, emaxx wrote: > > This and the next one also seem to be the component extensions, which > shouldn't > > be required to listed here. > Gone. > But we explicitly block all component extensions on login screen, so we want to > whitelist these ones. Isn't it possible to allow all component extensions in SigninScreenPolicyProvider? Tracking the component extensions in a separate whitelist may be bad when someone who doesn't know about these login-screen special whitelists adds a new component extension into Chrome. https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:49: extensions::BehaviorFeature::kSigninScreen); Shouldn't kSigninScreenWhitelist be used here? And, generally, I don't get why two different features kSigninScreen and kSigninScreenWhitelist are necessary. Would it be possible to put the whitelist into the same kSigninScreen feature? https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:54: return false; The error message should be probably also set here. https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:59: LOG(ERROR) << "Extension whitelisted : " << extension->id(); I guess this shouldn't be an ERROR message. https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:67: LOG(ERROR) << "Extension allowed : " << extension->id() << " / " And this also looks like logging left from the debug phase. https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:132: // We can not perform check for Signin Profile here, as it would result in nit: Seems that the indentation was lost here.
https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:49: extensions::BehaviorFeature::kSigninScreen); On 2016/08/30 13:01:41, emaxx wrote: > Shouldn't kSigninScreenWhitelist be used here? Ah, yes. > And, generally, I don't get why two different features kSigninScreen and > kSigninScreenWhitelist are necessary. Would it be possible to put the whitelist > into the same kSigninScreen feature? No, it is not possible - if you specify whitelist it would only match extensions with specified IDs, even if they satisfy rest of the conditions. https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:54: return false; On 2016/08/30 13:01:41, emaxx wrote: > The error message should be probably also set here. Done. https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:59: LOG(ERROR) << "Extension whitelisted : " << extension->id(); On 2016/08/30 13:01:42, emaxx wrote: > I guess this shouldn't be an ERROR message. Done. https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:67: LOG(ERROR) << "Extension allowed : " << extension->id() << " / " On 2016/08/30 13:01:41, emaxx wrote: > And this also looks like logging left from the debug phase. Done. https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:132: // We can not perform check for Signin Profile here, as it would result in On 2016/08/30 13:01:42, emaxx wrote: > nit: Seems that the indentation was lost here. Done.
https://codereview.chromium.org/2159103006/diff/120001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2159103006/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:4138: + <message name="IDS_EXTENSION_CANT_INSTALL_ON_LOGIN_SCREEN" desc="Error message when the administrator tries to force-install through policy an extension that is not allowed on a local screen."> nit: s/local screen/login screen/ (or signin screen) https://codereview.chromium.org/2159103006/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:4138: + <message name="IDS_EXTENSION_CANT_INSTALL_ON_LOGIN_SCREEN" desc="Error message when the administrator tries to force-install through policy an extension that is not allowed on a local screen."> Do you think it would make sense to do s/LOGIN_SCREEN/SIGNIN_SCREEN/ for consistency? All other code in this CL uses this word. https://codereview.chromium.org/2159103006/diff/120001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/120001/extensions/common/api/... extensions/common/api/_behavior_features.json:55: "platforms": ["chromeos"], Shouldn't this go into the first "branch" of the signin_screen feature too? I think here it only applies to the whitelisted extensions. https://codereview.chromium.org/2159103006/diff/120001/extensions/common/api/... extensions/common/api/_behavior_features.json:56: "whitelist": [ Shouldn't also the "component_extensions_auto_granted":false property be specified, if the intention is to disallow some of the component extensions? https://codereview.chromium.org/2159103006/diff/120001/extensions/common/api/... extensions/common/api/_behavior_features.json:58: "6F9E349A0561C78A0D3F41496FE521C5151C7F71", // Gnubby app I think I may already asked this in one of the previous iterations, but is Gnubby app really useful on the login screen? Isn't it that only the Gnubby extension is necessary for performing the authentication?
achuith@chromium.org changed reviewers: + achuith@chromium.org
https://codereview.chromium.org/2159103006/diff/120001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2159103006/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:4138: + <message name="IDS_EXTENSION_CANT_INSTALL_ON_LOGIN_SCREEN" desc="Error message when the administrator tries to force-install through policy an extension that is not allowed on a local screen."> On 2016/09/05 13:58:20, emaxx wrote: > Do you think it would make sense to do s/LOGIN_SCREEN/SIGNIN_SCREEN/ for > consistency? All other code in this CL uses this word. Done. https://codereview.chromium.org/2159103006/diff/120001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/120001/extensions/common/api/... extensions/common/api/_behavior_features.json:55: "platforms": ["chromeos"], On 2016/09/05 13:58:20, emaxx wrote: > Shouldn't this go into the first "branch" of the signin_screen feature too? I > think here it only applies to the whitelisted extensions. Done. https://codereview.chromium.org/2159103006/diff/120001/extensions/common/api/... extensions/common/api/_behavior_features.json:56: "whitelist": [ On 2016/09/05 13:58:20, emaxx wrote: > Shouldn't also the "component_extensions_auto_granted":false property be > specified, if the intention is to disallow some of the component extensions? Done.
Denis, I pointed out some time ago that some code had landed which contains quite a similar functionality: "session_types" extension feature property: http://crrev.com/2255613003 Did you consider extending this property to support our sign-in screen limitation?
On 2016/10/31 18:47:39, emaxx wrote: > Denis, I pointed out some time ago that some code had landed which contains > quite a similar functionality: "session_types" extension feature property: > http://crrev.com/2255613003 > Did you consider extending this property to support our sign-in screen > limitation? Yes, that can be integrated to our scheme. I will integrate that to this CL.
On 2016/11/03 17:09:43, Denis Kuznetsov (DE-MUC) wrote: > On 2016/10/31 18:47:39, emaxx wrote: > > Denis, I pointed out some time ago that some code had landed which contains > > quite a similar functionality: "session_types" extension feature property: > > http://crrev.com/2255613003 > > Did you consider extending this property to support our sign-in screen > > limitation? > > Yes, that can be integrated to our scheme. I will integrate that to this CL. After studying that CL it makes less sense, as we need extensions not only on login screen, but on a lock screen also. And it will not work nice to have different "session types" (lock and user session) simultaneously, while the setting is based on a global variable.
LGTM from my perspective. Though would be great if some tests would be added for the new policy provider.
Things seem fine overall with a few minor things - I would like to see some tests though. https://codereview.chromium.org/2159103006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:52: VLOG(0) << "Extension allowed : " << extension->id() << " / " nit: (this line and below) Generally leaving logging statements like this in is discouraged. See the logging section of the style guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... For chromeos, turning on VLOG output at runtime on release builds is impossible (at least last I heard), and sometimes administrators need to be able to see diagnostic information to understand certain behaviors of chrome or diagnose problems, so sometimes an exception to the general rule is made, and we allow specific LOG statements if there is a high probability they will be looked at ("just in case they might be useful" is typically not sufficient though). https://codereview.chromium.org/2159103006/diff/160001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/160001/extensions/common/api/... extensions/common/api/_behavior_features.json:65: "E24F1786D842E91E74C27929B0B3715A4689A473", // Gnubby component extension. nit: is this a spurious period? (it looks like it might push the entire line over 80 chars, but hard to tell in codereview interface) https://codereview.chromium.org/2159103006/diff/160001/extensions/common/api/... extensions/common/api/_behavior_features.json:69: ] FYI, these conditions are logically OR'ed together, so this is saying that this feature is allowed if either: -We're on chromeos dev channel with any policy force-install (except remote desktop) OR -We're on chromeos stable channel and the extension matches one of the entries in the whitelist Was that what you intended? BTW, if it wasn't, see this documentation the Devlin recently wrote up: https://chromium.googlesource.com/chromium/src/+/master/chrome/common/extensi...
https://codereview.chromium.org/2159103006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:52: VLOG(0) << "Extension allowed : " << extension->id() << " / " On 2016/11/09 05:50:43, Antony Sargent wrote: > nit: (this line and below) Generally leaving logging statements like this in is > discouraged. See the logging section of the style guide: > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > For chromeos, turning on VLOG output at runtime on release builds is impossible > (at least last I heard), and sometimes administrators need to be able to see > diagnostic information to understand certain behaviors of chrome or diagnose > problems, so sometimes an exception to the general rule is made, and we allow > specific LOG statements if there is a high probability they will be looked at > ("just in case they might be useful" is typically not sufficient though). The intent was to provide external extension developers with meaningful message in case when something goes wrong. As this particular situation is not easy to reproduce (this code is triggered only on ChromeOS and only on the login screen) I prefer verbosity. I will change logging level to debug for failures and remove logging for success case. https://codereview.chromium.org/2159103006/diff/160001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/160001/extensions/common/api/... extensions/common/api/_behavior_features.json:65: "E24F1786D842E91E74C27929B0B3715A4689A473", // Gnubby component extension. On 2016/11/09 05:50:43, Antony Sargent wrote: > nit: is this a spurious period? (it looks like it might push the entire line > over 80 chars, but hard to tell in codereview interface) Done. https://codereview.chromium.org/2159103006/diff/160001/extensions/common/api/... extensions/common/api/_behavior_features.json:69: ] On 2016/11/09 05:50:43, Antony Sargent wrote: > FYI, these conditions are logically OR'ed together, so this is saying that this > feature is allowed if either: > > -We're on chromeos dev channel with any policy force-install (except remote > desktop) > > OR > > -We're on chromeos stable channel and the extension matches one of the entries > in the whitelist > > Was that what you intended? > > BTW, if it wasn't, see this documentation the Devlin recently wrote up: > > https://chromium.googlesource.com/chromium/src/+/master/chrome/common/extensi... Yes, current intent is to enable it on non-stable channels with less restrictions so that early adopters can try their own extensions, but have only explicitly whitelisted extensions on stable channel.
added tests
asargent@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
lgtm, but I'm adding Devlin to have him take a look at the features changes because he's most familiar with that area of extensions code and I've gotten tripped up a few times in the past. https://codereview.chromium.org/2159103006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:54: LOG(WARNING) << "Extension disabled : " << extension->id() << " / " optional suggestion: perhaps saying something like "Denying load of extension " ... fits better with this being an implementation of "UserMayLoad", as opposed to "disabled". https://codereview.chromium.org/2159103006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:59: << " is not defined"; Do we ever expect feature to be null for legitimate reasons? If not, maybe this should just be a CHECK(feature) up above instead. https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:54: // Allow developers to test new feature. grammar nit: "feature" -> "features" https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:59: "min_manifest_version": 2, FYI, I don't think we really support installing extensions/apps/themes with manifest version 1 anymore, so you don't need to bother including this min_manifest_version key/val. https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:74: // Explicitly whitelist extensions that should always be available on a singin screen. typo: "sing" -> "sign" https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:77: "whitelist": [ Did you intend for component_extensions_auto_granted to be true in this block? (that's the default if not specified, I believe).
https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:50: "signin_screen": [ { How do we anticipate using this? Will it eventually be open to arbitrary apps/extensions, or will this always be whitelist only?
https://codereview.chromium.org/2159103006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:54: LOG(WARNING) << "Extension disabled : " << extension->id() << " / " On 2016/11/09 21:59:48, Antony Sargent wrote: > optional suggestion: perhaps saying something like "Denying load of extension " > ... fits better with this being an implementation of "UserMayLoad", as opposed > to "disabled". > Done. https://codereview.chromium.org/2159103006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:59: << " is not defined"; On 2016/11/09 21:59:48, Antony Sargent wrote: > Do we ever expect feature to be null for legitimate reasons? If not, maybe this > should just be a CHECK(feature) up above instead. Done. https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:50: "signin_screen": [ { On 2016/11/09 22:07:43, Devlin (slow) wrote: > How do we anticipate using this? Will it eventually be open to arbitrary > apps/extensions, or will this always be whitelist only? It will be open to all extensions installed via policy that match some extra criteria (e.g. requested permissions) once we implement more in-depth defense mechanisms. https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:54: // Allow developers to test new feature. On 2016/11/09 21:59:48, Antony Sargent wrote: > grammar nit: "feature" -> "features" Done. https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:59: "min_manifest_version": 2, On 2016/11/09 21:59:49, Antony Sargent wrote: > FYI, I don't think we really support installing extensions/apps/themes with > manifest version 1 anymore, so you don't need to bother including this > min_manifest_version key/val. Done. https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:74: // Explicitly whitelist extensions that should always be available on a singin screen. On 2016/11/09 21:59:49, Antony Sargent wrote: > typo: "sing" -> "sign" Done. https://codereview.chromium.org/2159103006/diff/200001/extensions/common/api/... extensions/common/api/_behavior_features.json:77: "whitelist": [ On 2016/11/09 21:59:49, Antony Sargent wrote: > Did you intend for component_extensions_auto_granted to be true in this block? > (that's the default if not specified, I believe). Good catch, thanks.
still lgtm https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:133: // We can not perform check for Signin Profile here, as it would result in nit: The comment indentation seems to be wrong. https://codereview.chromium.org/2159103006/diff/220001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/220001/extensions/common/api/... extensions/common/api/_behavior_features.json:69: "EC3DE21E048B67319893889529354DFBFA96FD23" // Smart Card Connector As you started adding real-world apps, please add the second (and the last) app that was planned to be whitelisted in the first version: "CSSI Smart Card Middleware" (ID "haeblkpifdemlfnkogkipmghfcbonief") https://codereview.chromium.org/2159103006/diff/220001/extensions/common/api/... extensions/common/api/_behavior_features.json:72: // Explicitly whitelist extensions that should always be available Can the "location" key be also specified here? These all, as far as I understand, are component extensions. I'm not sure how easy could it be to make some third-party extension have the ID hash from this list, but the additional level of defense shouldn't be excessive.
lgtm https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:50: if (availability.is_available()) { nit: don't need {} https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc (right): https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc:58: TEST_F(SigninScreenPolicyProviderTest, AllowPolicyExtensionOnDev) { Let's add some descriptive comments to each of these tests https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc:81: CreateTestApp("beknehfpfkghjoafdifaflglpjkojoco" /* gnubby */, maybe pull these constants out and name them? https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc:92: "khpfeaanjngmcnplbdlpegiifgpfgdco" /* smart card connector */, ditto https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:145: // Lazy creation of SigninScreenPolicyProvider nit: period at end of comment https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:147: if (profile_ == Why not chromeos::ProfileHelper::IsSigninProfile(profile_) https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:158: if (signin_screen_policy_provider_) { nit: don't need {}
https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:50: if (availability.is_available()) { On 2016/11/11 00:36:28, achuithb wrote: > nit: don't need {} Done. https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc (right): https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc:58: TEST_F(SigninScreenPolicyProviderTest, AllowPolicyExtensionOnDev) { On 2016/11/11 00:36:28, achuithb wrote: > Let's add some descriptive comments to each of these tests Done. https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc:81: CreateTestApp("beknehfpfkghjoafdifaflglpjkojoco" /* gnubby */, On 2016/11/11 00:36:28, achuithb wrote: > maybe pull these constants out and name them? Done. https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc:92: "khpfeaanjngmcnplbdlpegiifgpfgdco" /* smart card connector */, On 2016/11/11 00:36:28, achuithb wrote: > ditto Done. https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:133: // We can not perform check for Signin Profile here, as it would result in On 2016/11/10 14:44:32, emaxx wrote: > nit: The comment indentation seems to be wrong. Done. https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:145: // Lazy creation of SigninScreenPolicyProvider On 2016/11/11 00:36:28, achuithb wrote: > nit: period at end of comment Done. https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:147: if (profile_ == On 2016/11/11 00:36:29, achuithb wrote: > Why not chromeos::ProfileHelper::IsSigninProfile(profile_) Because signin profile is the off-the-record profile for the "sign-in profile parent", and we need to install the provider for the "sign-in profile parent" (note the GetOriginalProfile()). https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:158: if (signin_screen_policy_provider_) { On 2016/11/11 00:36:28, achuithb wrote: > nit: don't need {} Done. https://codereview.chromium.org/2159103006/diff/220001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/220001/extensions/common/api/... extensions/common/api/_behavior_features.json:69: "EC3DE21E048B67319893889529354DFBFA96FD23" // Smart Card Connector On 2016/11/10 14:44:32, emaxx wrote: > As you started adding real-world apps, please add the second (and the last) app > that was planned to be whitelisted in the first version: > "CSSI Smart Card Middleware" (ID "haeblkpifdemlfnkogkipmghfcbonief") Done. https://codereview.chromium.org/2159103006/diff/220001/extensions/common/api/... extensions/common/api/_behavior_features.json:72: // Explicitly whitelist extensions that should always be available On 2016/11/10 14:44:32, emaxx wrote: > Can the "location" key be also specified here? > These all, as far as I understand, are component extensions. > > I'm not sure how easy could it be to make some third-party extension have the ID > hash from this list, but the additional level of defense shouldn't be excessive. Done.
https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:133: // We can not perform check for Signin Profile here, as it would result in On 2016/11/11 15:50:17, Denis Kuznetsov (DE-MUC) wrote: > On 2016/11/10 14:44:32, emaxx wrote: > > nit: The comment indentation seems to be wrong. > > Done. It's still indented with 0 spaces AFAICS. https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:74: original_flag_ = g_bypass_checks_for_testing; nit: Transform into member initializer. https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.h (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.h:35: class ScopedSigninScreenPolicyProviderDisabler { Can this be suffixed with "ForTesting", to ensure that it won't be used for anything outside tests? Or there's no such hook for the whole classes? https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: s/2013/2016/ https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:136: void TearDownOnMainThread() override { Is this actually needed? https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:282: std::unique_ptr<chromeos::ScopedSigninScreenPolicyProviderDisabler> disabler_; Maybe change this member to have a more descriptive name? https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:319: disabler_ = If both test subclasses need this disabler, then why not moving it into the common base SigninExtensionsDeviceCloudPolicyBrowserTestBase? Also the disabler_ member could marked as private then.
lgtm lgtm assuming https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.h (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.h:35: class ScopedSigninScreenPolicyProviderDisabler { Drive by - we could also just have a method that returns a base::AutoReset<bool> here, i.e. base::AutoReset<bool> GetScopedSigninScreenPolicyProviderDisabler() { return base::AutoReset<bool>(&g_bypass_checks_for_testing, true); } https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:148: chromeos::ProfileHelper::GetSigninProfile()->GetOriginalProfile()) { nit: prefer Profile::IsSameProfile() https://codereview.chromium.org/2159103006/diff/240001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/240001/extensions/common/api/... extensions/common/api/_behavior_features.json:55: "channel": "dev", Note: we have seen malware that modifies the perceived channel of chrome. Is this a security risk?
On 2016/11/11 16:35:08, Devlin (slow) wrote: > lgtm > > lgtm assuming Gah, hit the button by accident. Meant "assuming https://codereview.chromium.org/2159103006/diff/240001/extensions/common/api/... isn't a problem".
https://codereview.chromium.org/2159103006/diff/240001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/240001/extensions/common/api/... extensions/common/api/_behavior_features.json:55: "channel": "dev", On 2016/11/11 16:35:08, Devlin (slow) wrote: > Note: we have seen malware that modifies the perceived channel of chrome. Is > this a security risk? Given that this the platforms field only contains chromeos here, I don't think we need to worry. (I don't think there even is a signin screen on non-chromeos ; the idea behind this CL is to limit the kinds of extensions that can run on the signin screen)
https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc:74: original_flag_ = g_bypass_checks_for_testing; On 2016/11/11 16:11:37, emaxx wrote: > nit: Transform into member initializer. Acknowledged. https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/signin_screen_policy_provider.h (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/signin_screen_policy_provider.h:35: class ScopedSigninScreenPolicyProviderDisabler { On 2016/11/11 16:35:08, Devlin (slow) wrote: > Drive by - we could also just have a method that returns a base::AutoReset<bool> > here, i.e. > base::AutoReset<bool> GetScopedSigninScreenPolicyProviderDisabler() { > return base::AutoReset<bool>(&g_bypass_checks_for_testing, true); > } Done. https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:136: void TearDownOnMainThread() override { On 2016/11/11 16:11:37, emaxx wrote: > Is this actually needed? Done. https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:282: std::unique_ptr<chromeos::ScopedSigninScreenPolicyProviderDisabler> disabler_; On 2016/11/11 16:11:37, emaxx wrote: > Maybe change this member to have a more descriptive name? Done. https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:319: disabler_ = On 2016/11/11 16:11:37, emaxx wrote: > If both test subclasses need this disabler, then why not moving it into the > common base SigninExtensionsDeviceCloudPolicyBrowserTestBase? Also the disabler_ > member could marked as private then. Acknowledged. https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2159103006/diff/240001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:148: chromeos::ProfileHelper::GetSigninProfile()->GetOriginalProfile()) { On 2016/11/11 16:35:08, Devlin (slow) wrote: > nit: prefer Profile::IsSameProfile() Acknowledged. https://codereview.chromium.org/2159103006/diff/240001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/2159103006/diff/240001/extensions/common/api/... extensions/common/api/_behavior_features.json:55: "channel": "dev", On 2016/11/11 17:37:13, Antony Sargent wrote: > On 2016/11/11 16:35:08, Devlin (slow) wrote: > > Note: we have seen malware that modifies the perceived channel of chrome. Is > > this a security risk? > > Given that this the platforms field only contains chromeos here, I don't think > we need to worry. (I don't think there even is a signin screen on non-chromeos ; > the idea behind this CL is to limit the kinds of extensions that can run on the > signin screen) Right. And there are extra checks - type of the extension, installation location, etc. There will be more checks (e.g. requested permissions) in upcoming CLs.
The CQ bit was checked by antrim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by antrim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by antrim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asargent@chromium.org, achuith@chromium.org, emaxx@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2159103006/#ps280001 (title: "Fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add policy provider that would filter extensions/apps allowed on the sign-in screen. BUG=626342 ========== to ========== Add policy provider that would filter extensions/apps allowed on the sign-in screen. BUG=626342 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add policy provider that would filter extensions/apps allowed on the sign-in screen. BUG=626342 ========== to ========== Add policy provider that would filter extensions/apps allowed on the sign-in screen. BUG=626342 Committed: https://crrev.com/5876956faa38aef22e53b4a8712d5a87c4980578 Cr-Commit-Position: refs/heads/master@{#431874} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/5876956faa38aef22e53b4a8712d5a87c4980578 Cr-Commit-Position: refs/heads/master@{#431874} |
