Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1714473002:
Add usb_detachable_whitelist to device policy proto (Closed)
|
Created:
4 years, 10 months ago by Vincent Palatin Modified:
4 years, 9 months ago Reviewers:
bartfab (slow), Vincent Palatin, Mattias Nissler (ping if slow), Mark P, vpalatin, cschuet (SLOW) CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org, dskaram Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd usb_detachable_whitelist to device policy proto
The |usb_detachable_whitelist| setting contains the list of USB devices
(identified by their USB VID:PID pairs) which can be detached from the kernel
drivers in order to use them in web application.
The list is used by the permission_broker daemon.
BUG=chrome-os-partner:50249
TEST=none
Committed: https://crrev.com/1d50972a183cf34e6c1310d318f6af0981a52218
Cr-Commit-Position: refs/heads/master@{#380475}
Patch Set 1 #Patch Set 2 : update field name #
Total comments: 2
Patch Set 3 : Update the policy_templates.json file and the policy decoder code #
Total comments: 19
Patch Set 4 : fix BartFab comments on patchset 2 #Patch Set 5 : update decoder output #
Total comments: 2
Patch Set 6 : display VID:PID as dictionary, remove (maybe) unsupported hex number from JSON #Patch Set 7 : rebased / incremented IDs to account for intermediate commits #
Messages
Total messages: 41 (14 generated)
Description was changed from ========== Add usb_detachable_whitelist to device policy proto The |usb_detachable_whitelist| setting contains the list of USB devices (identified by their USB VID:PID pairs) which can be detached from the kernel drivers in order to use them in web application. The list is used by the permission_broker daemon. BUG=chrome-os-partner:50249 TEST=none ========== to ========== Add usb_detachable_whitelist to device policy proto The |usb_detachable_whitelist| setting contains the list of USB devices (identified by their USB VID:PID pairs) which can be detached from the kernel drivers in order to use them in web application. The list is used by the permission_broker daemon. BUG=chrome-os-partner:50249 TEST=none ==========
vpalatin@google.com changed reviewers: + bartfab@chromium.org, cschuet@chromium.org, mnissler@chromium.org
vpalatin@google.com changed reviewers: + vpalatin@google.com
ping anybody ?
IIUC, this proto will be consumed by a daemon, not by Chrome. However, you should still decode it in Chrome so that the policy values will show up on the chrome://policy page for debugging and transparency purposes. https://codereview.chromium.org/1714473002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/1714473002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:667: // drivers in order to use them in web application. Nit: s/application/applications/
One thing I forgot: If I understand correctly and the daemon reads the proto directly (bypassing Chrome), how does it verify that the policy is legit? We have signature checks on policy blobs. How does the daemon verify the signatures?
On 2016/02/29 12:23:14, bartfab (slow) wrote: > One thing I forgot: If I understand correctly and the daemon reads the proto > directly (bypassing Chrome), how does it verify that the policy is legit? We > have signature checks on policy blobs. How does the daemon verify the > signatures? It's going through libpolicy, which handles this: https://cs.corp.google.com/android/external/libbrillo/policy/device_policy_im...
On 2016/02/29 12:23:14, bartfab (slow) wrote: > One thing I forgot: If I understand correctly and the daemon reads the proto > directly (bypassing Chrome), how does it verify that the policy is legit? We > have signature checks on policy blobs. How does the daemon verify the > signatures? It's going through libpolicy, which handles this: https://cs.corp.google.com/android/external/libbrillo/policy/device_policy_im...
On 2016/02/29 12:35:24, Mattias (ping if no reply) wrote: > On 2016/02/29 12:23:14, bartfab (slow) wrote: > > One thing I forgot: If I understand correctly and the daemon reads the proto > > directly (bypassing Chrome), how does it verify that the policy is legit? We > > have signature checks on policy blobs. How does the daemon verify the > > signatures? > > It's going through libpolicy, which handles this: > https://cs.corp.google.com/android/external/libbrillo/policy/device_policy_im... Excellent. The only concern I have then is that this new policy should be listed in policy_templates.json and decoded by Chrome so that it shows up on the chrome://policy page, even if Chrome does not use the values for anything else directly.
Ok I will try to update the .json file and the policy decoder. I'm not too familiar with those, though. https://codereview.chromium.org/1714473002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/1714473002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:667: // drivers in order to use them in web application. On 2016/02/29 12:20:31, bartfab (slow) wrote: > Nit: s/application/applications/ Done.
https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8210: 'id': 317, I'm not sure what id I'm supposed to put here. I have put the highest one I found in the file + 1 ...
https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right): https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:834: uint16_t vid = entry->has_vendor_id() ? entry->vendor_id() : 0x0000; Nit: const. https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:835: uint16_t pid = entry->has_product_id() ? entry->product_id() : 0x0000; Nit: const. https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:837: new base::StringValue(base::StringPrintf("%04x:%04x", vid, pid))); Why not use a base::ListValue of two base::FundamentalValues here instead? It would avoid the (potentially lossy, if the value is out of range) conversion from int to 4-digit string. https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:840: POLICY_SCOPE_MACHINE, POLICY_SOURCE_CLOUD, whitelist, NULL); Nit: s/NULL/nullptr/ https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8210: 'id': 317, On 2016/03/01 01:54:23, vpalatin wrote: > I'm not sure what id I'm supposed to put here. > I have put the highest one I found in the file + 1 ... You did the right thing, but you could have just looked at line 140 to find the highest number used :). And while we are on the topic: Please update line 140. https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8211: 'caption': '''white list of USB detachable devices''', Nit: s/white list/Whitelist/ https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8212: 'tags': [], I am wondering whether this should be tagged full-admin-access and/or system-security. After all, the admin can use it to MitM e.g. a USB keyboard. https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8216: }, Note that you need to add this policy to histograms.xml as well.
https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right): https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:834: uint16_t vid = entry->has_vendor_id() ? entry->vendor_id() : 0x0000; On 2016/03/01 14:49:57, bartfab (slow) wrote: > Nit: const. Done. https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:835: uint16_t pid = entry->has_product_id() ? entry->product_id() : 0x0000; On 2016/03/01 14:49:57, bartfab (slow) wrote: > Nit: const. Done. https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:837: new base::StringValue(base::StringPrintf("%04x:%04x", vid, pid))); On 2016/03/01 14:49:57, bartfab (slow) wrote: > Why not use a base::ListValue of two base::FundamentalValues here instead? It > would avoid the (potentially lossy, if the value is out of range) conversion > from int to 4-digit string. That would look nicer but I really want to print this as a 'standard' VID:PID pair as other Linux tool (e.g. lsusb), if we get a set of integers in base 10, it's somewhat unusable without doing the conversion manually. https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:840: POLICY_SCOPE_MACHINE, POLICY_SOURCE_CLOUD, whitelist, NULL); On 2016/03/01 14:49:57, bartfab (slow) wrote: > Nit: s/NULL/nullptr/ Done. https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8210: 'id': 317, On 2016/03/01 14:49:57, bartfab (slow) wrote: > On 2016/03/01 01:54:23, vpalatin wrote: > > I'm not sure what id I'm supposed to put here. > > I have put the highest one I found in the file + 1 ... > > You did the right thing, but you could have just looked at line 140 to find the > highest number used :). And while we are on the topic: Please update line 140. Done. https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8211: 'caption': '''white list of USB detachable devices''', On 2016/03/01 14:49:57, bartfab (slow) wrote: > Nit: s/white list/Whitelist/ Done. https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8212: 'tags': [], On 2016/03/01 14:49:57, bartfab (slow) wrote: > I am wondering whether this should be tagged full-admin-access and/or > system-security. After all, the admin can use it to MitM e.g. a USB keyboard. 'system-security' definitely, I will add it. From the description of 'full-admin-access', I don't feel it's really the case. But feel free to insist, I will add it https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8216: }, On 2016/03/01 14:49:57, bartfab (slow) wrote: > Note that you need to add this policy to histograms.xml as well. I did not understand what I was supposed to do for this file. Digging into other CLs, I imagine I should add it to "EnterprisePolicies" enum even though it suggests it's generated from the .json file. is that it ?
https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right): https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:837: new base::StringValue(base::StringPrintf("%04x:%04x", vid, pid))); On 2016/03/01 17:49:01, vpalatin wrote: > On 2016/03/01 14:49:57, bartfab (slow) wrote: > > Why not use a base::ListValue of two base::FundamentalValues here instead? It > > would avoid the (potentially lossy, if the value is out of range) conversion > > from int to 4-digit string. > > That would look nicer but I really want to print this as a 'standard' VID:PID > pair as other Linux tool (e.g. lsusb), > if we get a set of integers in base 10, it's somewhat unusable without doing the > conversion manually. I think there is a compromise that can make both of us happy. The issue I have is that converting to |uint16_t| hides errors if the value is outside the expected range. I suggest two changes: 1) Switch |vid| and |pid| from |uint16_t| to |int32_t|. If the values are in the expected range, this will have no effect - StringPrintf() will still produce 4-digit output. But if the values are invalid (e.g. more than four digits or negative), we will be able to see it in the StringPrintf output. 2) Rather than combining |vid| and |pid| into one string, StringPrintf() them into separate strings. This way, each piece of information on the chrome://policy page maps directly to a policy proto field. This will make the information in chrome://policy slightly harder to consume (you have to concatenate the |vid| and |pid| in your head) but considering they are printed as hex digits with correct padding already, that should not be a big burden. https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8216: }, On 2016/03/01 17:49:01, vpalatin wrote: > On 2016/03/01 14:49:57, bartfab (slow) wrote: > > Note that you need to add this policy to histograms.xml as well. > > I did not understand what I was supposed to do for this file. > Digging into other CLs, I imagine I should add it to "EnterprisePolicies" enum > even though it suggests it's generated from the .json file. > is that it ? Yes, this is all you had to do. I was curious about the comment which suggests the list is auto-generated. As it turns out, there is a script: tools/metrics/histograms/update_policies.py However, I have to yet hear about anyone actually using it. We always update the file manually :).
On Wed, Mar 2, 2016 at 2:40 AM, <bartfab@chromium.org> wrote: > > > https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc > (right): > > > https://codereview.chromium.org/1714473002/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:837: > new base::StringValue(base::StringPrintf("%04x:%04x", vid, pid))); > On 2016/03/01 17:49:01, vpalatin wrote: > > On 2016/03/01 14:49:57, bartfab (slow) wrote: > > > Why not use a base::ListValue of two base::FundamentalValues here > instead? It > > > would avoid the (potentially lossy, if the value is out of range) > conversion > > > from int to 4-digit string. > > > > That would look nicer but I really want to print this as a 'standard' > VID:PID > > pair as other Linux tool (e.g. lsusb), > > if we get a set of integers in base 10, it's somewhat unusable without > doing the > > conversion manually. > > I think there is a compromise that can make both of us happy. The issue > I have is that converting to |uint16_t| hides errors if the value is > outside the expected range. I suggest two changes: > > 1) Switch |vid| and |pid| from |uint16_t| to |int32_t|. If the values > are in the expected range, this will have no effect - StringPrintf() > will still produce 4-digit output. But if the values are invalid (e.g. > more than four digits or negative), we will be able to see it in the > StringPrintf output. > > 2) Rather than combining |vid| and |pid| into one string, StringPrintf() > them into separate strings. This way, each piece of information on the > chrome://policy page maps directly to a policy proto field. This will > make the information in chrome://policy slightly harder to consume (you > have to concatenate the |vid| and |pid| in your head) but considering > they are printed as hex digits with correct padding already, that should > not be a big burden. > Ok I think I understand, I will do another version with a ListValue of 2 strings for VID and PID. > > > > https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... > File components/policy/resources/policy_templates.json (right): > > > https://codereview.chromium.org/1714473002/diff/40001/components/policy/resou... > components/policy/resources/policy_templates.json:8216: }, > On 2016/03/01 17:49:01, vpalatin wrote: > > On 2016/03/01 14:49:57, bartfab (slow) wrote: > > > Note that you need to add this policy to histograms.xml as well. > > > > I did not understand what I was supposed to do for this file. > > Digging into other CLs, I imagine I should add it to > "EnterprisePolicies" enum > > even though it suggests it's generated from the .json file. > > is that it ? > > Yes, this is all you had to do. > > I was curious about the comment which suggests the list is > auto-generated. As it turns out, there is a script: > > tools/metrics/histograms/update_policies.py > > However, I have to yet hear about anyone actually using it. We always > update the file manually :). > > https://codereview.chromium.org/1714473002/ > -- Vincent -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL
https://codereview.chromium.org/1714473002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right): https://codereview.chromium.org/1714473002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:835: const std::string vid = Sorry for the many nits: Rather than a magic value "<no VID>", the best way to indicate whether a value is present or not is a DictionaryValue instead of a ListValue: Use "vid"/"pid" as keys and leave out the entries if the vid/pid is unset.
https://codereview.chromium.org/1714473002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right): https://codereview.chromium.org/1714473002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:835: const std::string vid = On 2016/03/07 11:16:47, bartfab (slow) wrote: > Sorry for the many nits: Rather than a magic value "<no VID>", the best way to > indicate whether a value is present or not is a DictionaryValue instead of a > ListValue: Use "vid"/"pid" as keys and leave out the entries if the vid/pid is > unset. Done.
LGTM. Thanks for bearing through the reviews.
The CQ bit was checked by vpalatin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714473002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vpalatin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org Link to the patchset: https://codereview.chromium.org/1714473002/#ps120001 (title: "rebased / incremented IDs to account for intermediate commits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714473002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
vpalatin@google.com changed reviewers: + mpearson@chromium.org, vpalatin@chromium.org
Mark P, Can you review the histograms.xml change ?
histograms.xml lgtm
The CQ bit was checked by vpalatin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714473002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vpalatin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714473002/120001
Message was sent while issue was closed.
Description was changed from ========== Add usb_detachable_whitelist to device policy proto The |usb_detachable_whitelist| setting contains the list of USB devices (identified by their USB VID:PID pairs) which can be detached from the kernel drivers in order to use them in web application. The list is used by the permission_broker daemon. BUG=chrome-os-partner:50249 TEST=none ========== to ========== Add usb_detachable_whitelist to device policy proto The |usb_detachable_whitelist| setting contains the list of USB devices (identified by their USB VID:PID pairs) which can be detached from the kernel drivers in order to use them in web application. The list is used by the permission_broker daemon. BUG=chrome-os-partner:50249 TEST=none ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add usb_detachable_whitelist to device policy proto The |usb_detachable_whitelist| setting contains the list of USB devices (identified by their USB VID:PID pairs) which can be detached from the kernel drivers in order to use them in web application. The list is used by the permission_broker daemon. BUG=chrome-os-partner:50249 TEST=none ========== to ========== Add usb_detachable_whitelist to device policy proto The |usb_detachable_whitelist| setting contains the list of USB devices (identified by their USB VID:PID pairs) which can be detached from the kernel drivers in order to use them in web application. The list is used by the permission_broker daemon. BUG=chrome-os-partner:50249 TEST=none Committed: https://crrev.com/1d50972a183cf34e6c1310d318f6af0981a52218 Cr-Commit-Position: refs/heads/master@{#380475} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1d50972a183cf34e6c1310d318f6af0981a52218 Cr-Commit-Position: refs/heads/master@{#380475} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
