clearpathrobotics / robot_upstart

ROS package of helper functions related to bringing up roslaunch on system startup.
BSD 3-Clause "New" or "Revised" License
196 stars 96 forks source link

Keep group permissions? #100

Open civerachb-cpr opened 3 years ago

civerachb-cpr commented 3 years ago

How difficult would it be to replace setuidgid with something else that would preserve group permissions? The removal of additional groups can cause headaches when dealing with some hardware.

For example, on Jetsons the GPU is assigned to the nvidia group. Or audio devices like USB microphones belong to the audio group. Rather than needing to create custom udev rules to work around these restrictions, and possibly open up security holes by allowing global r/w to devices that shouldn't necessarily be that accessible, it would be nice if the nodes could be launched with the group permissions intact.

mikepurvis commented 3 years ago

Yeah, this has been a longstanding issue, so much so that there's a specific note about dialout in the docs: http://docs.ros.org/en/melodic/api/robot_upstart/html/install.html#permissions

Note that with systemd units, you can have systemd take care of doing the permissions drop, and then groups are preserved, but that means that nothing else in the start script is able to use root either since you're already unprivileged at that point. The conventional systemd solution to this is to use the ExecStartPre directive to do anything which must be done as root, however there's nothing comparable in Upstart.

So I think if someone is interested in tackling this, the ideal thing to do would be to break up the start script into separate start and prestart scripts, where prestart expects to run as root, and start expects to run as non-root. And then in Upstart, make the script stanza of the job just usr/sbin/prestart; setuidgid xxx:yyy /usr/sbin/start.


As a less invasive option, there a number of alternatives to setuidgid which didn't exist or weren't as visible when the original robot_upstart was written based on setup scripts from PR2 and Turtlebot in the Ubuntu Lucid days.

rgariepy commented 3 years ago

I'll take 'papercuts old enough that Ryan tried to solve them' for $100, Alex... :)

civerachb-cpr commented 3 years ago

So I think if someone is interested in tackling this, the ideal thing to do would be to break up the start script into separate start and prestart scripts, where prestart expects to run as root, and start expects to run as non-root. And then in Upstart, make the script stanza of the job just usr/sbin/prestart; setuidgid xxx:yyy /usr/sbin/start.

I'm not sure that would actually solve the problem; in the example of audio devices or devices in the dialout and/or plugdev groups, the ROS nodes themselves should be started in such a way that those group permissions are respected. Assuming that /usr/sbin/start in your example would be responsible for actually starting the ROS nodes we've just moved the problem, not actually solved it.

Alternatively, there a number of alternatives to setuidgid which didn't exist or weren't as visible when the original robot_upstart was written based on setup scripts from PR2 and Turtlebot in the Ubuntu Lucid days.

Thanks for that link. I'll take a look and see if any of those appear as promising alternatives. If any of them do work out I'll submit a PR.

mikepurvis commented 3 years ago

ROS nodes themselves should be started in such a way that those group permissions are respected.

When you use User= in a systemd unit it pulls all the supplementary groups by default. See the documentation:

           If the User= setting is used the supplementary group list is
           initialized from the specified user's default group list, as
           defined in the system's user and group database. Additional
           groups may be configured through the SupplementaryGroups= setting
           (see below).

I also validated this on one of our OTTO robots by cloning the robot.service unit that's part of our internal robot-common-cfg package, and changing the ExecStart line to just print the groups. It does the right thing:

$ systemctl status mpurvis-test
● mpurvis-test.service - Clearpath common robot roslaunch
     Loaded: loaded (/lib/systemd/system/mpurvis-test.service; disabled; vendor preset: enabled)
     Active: inactive (dead)

Dec 17 16:17:12 robot-ph-xxxxxx systemd[1]: Started Clearpath common robot roslaunch.
Dec 17 16:17:12 robot-ph-xxxxxx groups[90953]: administrator dialout sudo video systemd-journal pulse-access
Dec 17 16:17:12 robot-ph-xxxxxx systemd[1]: mpurvis.service: Succeeded.

The setuidgid command is the source of the problem, because it's not equipped to look up on the system what the groups are supposed to be for the specified user; I think it was originally meant for specific-app cases like a webserver where your user:group is just nginx:nginx and you have no interaction-with or knowledge of any other groups on the system.

Systemd's implementation fixes that and does the right thing here.

civerachb-cpr commented 3 years ago

Good news: in some quick tests on a Ridgeback with a standard PC + a Jetson it appears replacing setuidgid with setpriv -reuid USER -regid USER --init-groups does work as-desired: the ROS nodes launch with the appropriate plugdev, dialout, audio, etc... group permissions.

The bad news is that setpriv isn't available on Ubuntu 16.04 (though you can manually download the package from 18.04's repos and install it). But given that Kinetic is going EOL soon that's likely not a big issue, and we can probably switch to setpriv for Melodic onward.

rcodddow commented 3 years ago

@mikepurvis I can confirm the above works on Ubuntu and (Nivida L4T) using melodic and noetic, it would be nice to have this fix included in the main branch. I can create a pull request if you would like.

setpriv --reuid $USER --regid $USER --init-groups roslaunch $LAUNCH_FILENAME &