shill scripts: too much shell in verified mode |
||||||
Issue descriptionff_debug is a shell script that wraps a debugd dbus API and provides some CLI parsing. it is called by some test code and by crosh. the trouble is that it's a shell script that runs as chronos currently and does parsing (in shell) of user input's. we could address this by adding a new debugd dbus API to run the command under the debugd user, but now we're sending a dbus message to debugd so that it can fork a child shell script to process a CLI to send a dbus message to shill. that's kind of complicated. for user's needs, the existing network_diag & network_logging commands should be sufficient to set up the shill debug settings. that means we can delete the ff_debug command from crosh. but there are a few calls to ff_debug in crosh itself. we can convert those to make the dbus request directly, and do so in a form that the dbus endpoint expects. finally, for test/devs, we can move ff_debug to test images under /usr/local.
,
Sep 19 2017
Not sure if the QA team uses it for testing, so +harpreet fyi.
I'd agree with cernekee@ that the ability to configure logging of individual shill modules and debug level in verified mode is quite useful to diagnose issues in the field. But we may want to revisit how the functionality (especially the user input parsing part) is exposed as vapier@ pointed out. Would moving most of the input processing from ff_debug to shill's {Get,Set}Debug{Tags,Level} API be good enough?
On a related note, we should probably invest more in chrome://net-internals/#chromeos for developers in long run. AFAICT, `network_logging` shares quite some similarity with the debug settings under chrome://net-internals/#chromeos.
,
Sep 19 2017
while the dbus endpoint needs to do validation itself regardless, i would hesitate recommending we expand the dbus endpoints to accept the fancier formats (what ff_debug allows with its inline +/- syntax). the dbus API should be simple/stupid. if we changed the ff_debug crosh interface to also be stupid, that would work. basically leave it up to the user to add/delete/merge active tag settings. there are similar concerns with all these glorified dbus wrappers: - ff_debug - wpa_debug - modem - set_apn - set_arpgw - set_wake_on_lan - network_diag - set_cellular_ppp - connectivity i guess what we really want to do is create a debugd endpoint where we can specify the command from a hard-coded whitelist (e.g. "wpa_debug"), drop to an unprivileged user (e.g. "shill-commands"?), and let that do all the parsing/dbus requests. the requirement would be that all of its communication would be via dbus. it wouldn't be allowed to go poking any files directly.
,
Sep 19 2017
IMHO, most of these commands should really be migrated to proper UI components or to one of those chrome:// pages. A near term fix may be to migrate some of them to debugd helpers. Perhaps debugd could provide a generic interface for crosh to invoke whitelisted debug commands in a sandboxed manner.
,
Sep 19 2017
sorry, i of course agree with putting a proper UI here and killing all the crosh interfaces (and the UI would be based on the dbus API rather than running shell scripts which would get us back into this problem). but i'd like to a near term mitigation as i don't expect the custom Chrome pages/updates to arrive any time soon.
,
Sep 19 2017
Most of our testing uses test images, but as cernekee@ pointed, this is useful to set log levels, when using verified mode.
,
Sep 20 2017
i've largely implemented this now, so i guess i'll take the bug ;)
,
Sep 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/def6d325ea04dd57a8049f45b900068962b7c732 commit def6d325ea04dd57a8049f45b900068962b7c732 Author: Mike Frysinger <vapier@chromium.org> Date: Wed Sep 20 12:50:22 2017 shill: ff_debug/wpa_debug: add missing shebang BUG= chromium:766379 TEST=running scripts still work Change-Id: I4520cec0b4133c40f250d2a63ad194fc5a9a9535 Reviewed-on: https://chromium-review.googlesource.com/674823 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Ben Chan <benchan@chromium.org> [modify] https://crrev.com/def6d325ea04dd57a8049f45b900068962b7c732/bin/wpa_debug [modify] https://crrev.com/def6d325ea04dd57a8049f45b900068962b7c732/bin/ff_debug
,
Sep 20 2017
OK, stack is up: https://chromium-review.googlesource.com/q/topic:%2522shill-scripts-depriv%2522+status:open this basically moves all the scripts to running under the new "shill-scripts" which won't have access to chronos paths. there aren't any other sandboxing tech leveraged (yet?) as the debugd C++ API here is pretty basic in general. will need to look at improving this as i think we can enable most things for these scripts.
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/eclass-overlay/+/4dba0bc38765ac43515545846ac52a0a6bec1a0a commit 4dba0bc38765ac43515545846ac52a0a6bec1a0a Author: Mike Frysinger <vapier@chromium.org> Date: Fri Sep 22 13:10:12 2017 shill-scripts: new accounts for running helper scripts under BUG= chromium:766379 TEST=scripts can run under these new accounts OK Change-Id: Ia34eed41c001e1f4ebb7b5ed19a1bbb8826144d4 Reviewed-on: https://chromium-review.googlesource.com/675726 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> [add] https://crrev.com/4dba0bc38765ac43515545846ac52a0a6bec1a0a/profiles/base/accounts/user/shill-scripts [add] https://crrev.com/4dba0bc38765ac43515545846ac52a0a6bec1a0a/profiles/base/accounts/group/shill-scripts
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/f88af718b66a6cb601138001cee2b105e93e06a0 commit f88af718b66a6cb601138001cee2b105e93e06a0 Author: Mike Frysinger <vapier@chromium.org> Date: Fri Sep 22 13:10:12 2017 shill: create new shill-scripts accts The helper scripts we install will use these. BUG= chromium:766379 TEST=scripts can run under these new accounts OK CQ-DEPEND=CL:675726 Change-Id: I0abdd7fd9f7e35b73d1a1941d7c44b573ce99903 Reviewed-on: https://chromium-review.googlesource.com/675824 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> [modify] https://crrev.com/f88af718b66a6cb601138001cee2b105e93e06a0/chromeos-base/shill/shill-9999.ebuild
,
Sep 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/1250df1e08dcc920b16f913e2a332fcb4ba057ce commit 1250df1e08dcc920b16f913e2a332fcb4ba057ce Author: Mike Frysinger <vapier@chromium.org> Date: Sat Sep 23 02:53:55 2017 debugd: simplify CreateProcess defaults BUG= chromium:766379 TEST=precq passes Change-Id: Ifbec7d12347192c58400135d51f9d8dad47e1735 Reviewed-on: https://chromium-review.googlesource.com/675826 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Ben Chan <benchan@chromium.org> [modify] https://crrev.com/1250df1e08dcc920b16f913e2a332fcb4ba057ce/debugd/src/subprocess_tool.h [modify] https://crrev.com/1250df1e08dcc920b16f913e2a332fcb4ba057ce/debugd/src/ping_tool.cc [modify] https://crrev.com/1250df1e08dcc920b16f913e2a332fcb4ba057ce/debugd/src/subprocess_tool.cc [modify] https://crrev.com/1250df1e08dcc920b16f913e2a332fcb4ba057ce/debugd/src/tracepath_tool.cc
,
Sep 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/6742dbfc1855a32aff23b0c3dd7cdb4eddf77e11 commit 6742dbfc1855a32aff23b0c3dd7cdb4eddf77e11 Author: Mike Frysinger <vapier@chromium.org> Date: Tue Sep 26 20:36:04 2017 shill: move help text from crosh to respective programs Rather than maintain duplicate display settings in two places, move them all to the programs so crosh can leverage them. BUG= chromium:766379 TEST=--help in crosh is still useful Change-Id: Ica43db4279f98ae9830ff8e606cd3577b0186502 Reviewed-on: https://chromium-review.googlesource.com/679320 Commit-Ready: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Brian Norris <briannorris@chromium.org> [modify] https://crrev.com/6742dbfc1855a32aff23b0c3dd7cdb4eddf77e11/bin/set_apn [modify] https://crrev.com/6742dbfc1855a32aff23b0c3dd7cdb4eddf77e11/bin/set_wake_on_lan [modify] https://crrev.com/6742dbfc1855a32aff23b0c3dd7cdb4eddf77e11/bin/set_cellular_ppp [modify] https://crrev.com/6742dbfc1855a32aff23b0c3dd7cdb4eddf77e11/bin/set_arpgw
,
Sep 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/57a957e8112a42ce0cf16fbe94f6a482b937db42 commit 57a957e8112a42ce0cf16fbe94f6a482b937db42 Author: Brian Norris <briannorris@chromium.org> Date: Tue Sep 26 20:36:04 2017 shill: support --help for set_{arpgw,wake_on_lan} These might get plumbed through to crosh, so might as well support a direct --help, instead of just an implicit "help" when you get the command wrong. BUG= chromium:766379 TEST=try out getting/setting state, with good and bad options; try `--help` Change-Id: I0871df9cdc522dd1ae5dc98b78e5b084ceff2d98 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/681619 Reviewed-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/57a957e8112a42ce0cf16fbe94f6a482b937db42/bin/set_wake_on_lan [modify] https://crrev.com/57a957e8112a42ce0cf16fbe94f6a482b937db42/bin/set_arpgw
,
Sep 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/da0fc3fa80a1f284674537f4cd5faa746293620b commit da0fc3fa80a1f284674537f4cd5faa746293620b Author: Mike Frysinger <vapier@chromium.org> Date: Wed Sep 27 23:29:29 2017 shill: allow dbus access from shill-scripts We're moving crosh from running the shill scripts directly as the chronos user to indirectly via debugd (which will run them under the shill-scripts accounts). They need access to these dbus methods to work, so update the whitelist to allow shill-scripts. BUG= chromium:766379 TEST=scripts can run under these new accounts OK CQ-DEPEND=CL:675824 Change-Id: I735e326f94536e7e2016bbbb302b089bd8bf6e3e Reviewed-on: https://chromium-review.googlesource.com/675730 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> [modify] https://crrev.com/da0fc3fa80a1f284674537f4cd5faa746293620b/shims/org.chromium.flimflam.conf
,
Sep 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/f9da3d359931723a523fc36086918e2431002085 commit f9da3d359931723a523fc36086918e2431002085 Author: Mike Frysinger <vapier@chromium.org> Date: Thu Sep 28 06:52:12 2017 debugd: add callbacks for shill helper scripts Lets run all the shill helper scripts under a dedicated shill-scripts account via debugd so we can sandbox them. All of their work is done via dbus commands anyways (other than network_diag, but we'll address that further later on). BUG= chromium:766379 TEST=running from crosh works CQ-DEPEND=CL:675730 Change-Id: I7bfb0180f0d770b14bbc673ed1eff2864382d441 Reviewed-on: https://chromium-review.googlesource.com/675733 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Ben Chan <benchan@chromium.org> [modify] https://crrev.com/f9da3d359931723a523fc36086918e2431002085/debugd/src/debugd_dbus_adaptor.cc [modify] https://crrev.com/f9da3d359931723a523fc36086918e2431002085/debugd/src/subprocess_tool.cc [modify] https://crrev.com/f9da3d359931723a523fc36086918e2431002085/debugd/src/debugd_dbus_adaptor.h [add] https://crrev.com/f9da3d359931723a523fc36086918e2431002085/debugd/src/shill_scripts_tool.h [modify] https://crrev.com/f9da3d359931723a523fc36086918e2431002085/debugd/dbus_bindings/org.chromium.debugd.xml [modify] https://crrev.com/f9da3d359931723a523fc36086918e2431002085/debugd/src/subprocess_tool.h [add] https://crrev.com/f9da3d359931723a523fc36086918e2431002085/debugd/src/shill_scripts_tool.cc [modify] https://crrev.com/f9da3d359931723a523fc36086918e2431002085/debugd/debugd.gyp
,
Sep 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/cbee82a480c3df85a44019c7b46a9c0add709a66 commit cbee82a480c3df85a44019c7b46a9c0add709a66 Author: Mike Frysinger <vapier@chromium.org> Date: Thu Sep 28 06:52:12 2017 debugd: update MM debug levels to match crosh The network_logging crosh helper tweaks the modem manager log levels based on the device, so update debugd to match. BUG= chromium:766379 TEST=precq passes Change-Id: Id6393873b59eef3786d2fe6cf1aa76f03ea42901 Reviewed-on: https://chromium-review.googlesource.com/688574 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Brian Norris <briannorris@chromium.org> [modify] https://crrev.com/cbee82a480c3df85a44019c7b46a9c0add709a66/debugd/src/debug_mode_tool.cc
,
Sep 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/4058b14066fd3428fbb0ea61d1646a3868765c43 commit 4058b14066fd3428fbb0ea61d1646a3868765c43 Author: Mike Frysinger <vapier@chromium.org> Date: Thu Sep 28 09:04:05 2017 crosh: drop network_logging helper This functionality should be handled entirely by debugd/net-internals now, so drop it and point people to it. BUG= chromium:766379 TEST=read the message Change-Id: I4c0b4f2e27ffcac6135f60a72b86a6becf61b931 Reviewed-on: https://chromium-review.googlesource.com/688038 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Brian Norris <briannorris@chromium.org> [modify] https://crrev.com/4058b14066fd3428fbb0ea61d1646a3868765c43/crosh/crosh
,
Sep 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/27ce068399563ccc48efa99cd9ff64ecd8add507 commit 27ce068399563ccc48efa99cd9ff64ecd8add507 Author: Mike Frysinger <vapier@chromium.org> Date: Thu Sep 28 09:04:04 2017 debugd: update wifi debug levels to match crosh The network_logging crosh helper tweaks the wifi debug settings based on the device, so update debugd to match. BUG= chromium:766379 TEST=precq passes Change-Id: Ife8cfd82320ccff2a454ebf214f08cd0af924c9c Reviewed-on: https://chromium-review.googlesource.com/688575 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Brian Norris <briannorris@chromium.org> [modify] https://crrev.com/27ce068399563ccc48efa99cd9ff64ecd8add507/debugd/src/debugd_dbus_adaptor.cc [modify] https://crrev.com/27ce068399563ccc48efa99cd9ff64ecd8add507/debugd/src/debugd_dbus_adaptor.h [modify] https://crrev.com/27ce068399563ccc48efa99cd9ff64ecd8add507/debugd/src/debug_mode_tool.cc [modify] https://crrev.com/27ce068399563ccc48efa99cd9ff64ecd8add507/debugd/dbus_bindings/org.chromium.debugd.xml [delete] https://crrev.com/649e41b3b51bca53deea670b8d3909e7bb04871f/debugd/src/wifi_debug_tool.h [modify] https://crrev.com/27ce068399563ccc48efa99cd9ff64ecd8add507/debugd/debugd.gyp [delete] https://crrev.com/649e41b3b51bca53deea670b8d3909e7bb04871f/debugd/src/wifi_debug_tool.cc
,
Sep 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/03b54c868e08e0839d0aa78c4b157890f8b98bd5 commit 03b54c868e08e0839d0aa78c4b157890f8b98bd5 Author: Mike Frysinger <vapier@chromium.org> Date: Fri Sep 29 06:26:29 2017 crosh: switch shill commands to run via debugd The majority of the commands just make dbus requests and don't need access to local resources. Only stand out is the network_diag util as it writes its log output to the Downloads folder. Move that out of the script and into crosh so we can preserve behavior. BUG= chromium:766379 TEST=running from crosh works Change-Id: I8405d4c8470e2a41b62c5b20ede620274aad3ba0 Reviewed-on: https://chromium-review.googlesource.com/675734 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Ben Chan <benchan@chromium.org> [modify] https://crrev.com/03b54c868e08e0839d0aa78c4b157890f8b98bd5/crosh/network_diag [modify] https://crrev.com/03b54c868e08e0839d0aa78c4b157890f8b98bd5/crosh/crosh
,
Sep 30 2017
shill scripts should all be run via debugd and in a more restrictive environment. the jail itself isn't as good as it should be, but that'll wait on the libminijail cleanup.
,
Sep 13
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by cernekee@chromium.org
, Sep 18 2017