Skip to content
This repository has been archived by the owner on Sep 22, 2023. It is now read-only.

Use more secure visudo example in comments #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DerVerruckteFuchs
Copy link

This is the original visudo example in algorand@.service:

## To allow the update script (which runs as the current user) to manipulate
## the algorand systemd service, the following lines need to be added to
## /etc/sudoers (using visudo):
##
##   algo ALL=(ALL) NOPASSWD: /usr/bin/systemctl start *
##   algo ALL=(ALL) NOPASSWD: /usr/bin/systemctl stop *
##   algo ALL=(ALL) NOPASSWD: /usr/bin/systemctl restart *
##   algo ALL=(ALL) NOPASSWD: /usr/bin/systemctl status *

This is pretty bad. I've tested this, and the wildcard lets me interact with ANY systemd service (with start, stop, restart, and status) as the algo user without a password. I also did NOT add the algo user to the sudoers file (as algo ALL=(ALL) ALL, or adding algo to the wheel group ). I was able to stop and start ip6tables.service with sudo /usr/bin/systemctl stop ip6tables and sudo /usr/bin/systemctl start ip6tables respectively. Note that you have to use sudo /usr/bin/systemctl and not just sudo systemctl. The former is marginally better than the latter.

The following is more secure:

## To allow the update script (which runs as the current user) to manipulate
## the algorand-update systemd service, the following lines need to be added to
## /etc/sudoers (using visudo):
##
##   algo ALL=(ALL) NOPASSWD: /usr/bin/systemctl start algorand@*
##   algo ALL=(ALL) NOPASSWD: /usr/bin/systemctl stop algorand@*
##   algo ALL=(ALL) NOPASSWD: /usr/bin/systemctl restart algorand@*
##   algo ALL=(ALL) NOPASSWD: /usr/bin/systemctl status algorand@*

You'll still get the wildcard behavior that's needed for user specified directories to work properly with the systemd service. However, it's restricted to the systemd service you needed wildcard'ed in the first place. The algo user is going to need to be in the sudoers file and use a password for any other service.

Comparing it to a firewall, it's the difference between opening all ports, vs. just the ones that need opened for a particular task.

@JasonWeathersby
Copy link
Contributor

Any chance you can resubmit the PR here? We have setup CLA in the pr process.

@DerVerruckteFuchs
Copy link
Author

After creating the PR's I've made, github notified me that I needed to accept the CLA before they could be merged. It was that yellow/orange dot that shows up next to my PRs in the PR tab for the repo (at least on my screen when I'm logged in), and it showed up around the "Review Required" stuff that shows up at the bottom. I made sure to read through it, and have already accepted it. Is that different from what's been setup for this repo?

I can resubmit if necessary, but as far as what github has shown me, the CLA has been taken care of.

@JasonWeathersby
Copy link
Contributor

Thanks. You do not need to do anything else. We will review the PR.

@zeldovich
Copy link

Seems fine, in the sense that it doesn't hurt, but also I don't think this really improves the state of security at all. The systemd service runs an algod binary out of the user-supplied instance path name, which means arbitrary code running as root.

The real answer would be to use Debian packages, so the updates happen through standard system tools and don't need the algo user to run systemctl commands. Debian packages are somewhere in the pipeline but aren't ready yet.

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


DerVerruckteFuchs seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants