Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add back check to enforce root/sudo startup. #1031

Open
wants to merge 1 commit into
base: develop-3.0
Choose a base branch
from

Conversation

martincox
Copy link
Contributor

I couldn't work out what the first check was being done for. Where we were checking for non-root startup and not su'ing to riak.

I don't think that has any impact? I've run both ways (service & sudo) startup, and there's no difference. Unless there's something obvious I'm missing?

@martincox
Copy link
Contributor Author

martincox commented Jul 15, 2020

I think we might have an issue with the service script - I don't think it actually runs the process as riak.

If I add in the --chuid flag to start-stop-daemon in the do_start function, with this change, the service won't start - because at the point where the /usr/sbin/riak script is run, it's now being run as riak and fails the root UID check.

@martincox
Copy link
Contributor Author

I don't know if I'm misinterpreting the switches for start-stop-daemon, but to me it reads like --user just performs a check, as opposed to starting the process as a designated user. I'm looking at this man page - http://manpages.ubuntu.com/manpages/trusty/man8/start-stop-daemon.8.html

But then looking at the previous build, with node_package, it's exactly the same - https://github.com/basho/node_package/blob/a541c79bb65fb4ac6f60197c333f841065d0aff6/priv/templates/deb/init.script#L48

I'm a bit confused.

@martincox
Copy link
Contributor Author

martincox commented Jul 15, 2020

node_package has a check_user function (https://github.com/basho/node_package/blob/a541c79bb65fb4ac6f60197c333f841065d0aff6/priv/base/env.sh#L223) in env.sh which will su to the {{runner_user}}.

So the service script has never run as the riak user, and the su is handled in the wrapper script - /usr/sbin/riak in our case.

In which case, the setup now, with this change, mimics what previous builds did (I think).

@martinsumner
Copy link
Contributor

From a debian perspective we wanted the service to run the script as riak, as if we ran the script as root and then su'd to riak as we ran riak - the service would only give root access to the PIDfile (as it had started the script as root), and so riak would not be able to write the PID in the pre-start hook as required by the service.

So the script is meant to be run either as riak, or as root. If it is run as riak (i.e. if called directly by someone running as riak, or run by the service) it shouldn't switch user, and if it has been started as a service systemd will now give write access for the pid file to the riak user. If it is run as root, it should switch user, but assume it is not systemd starting - and so create the PID folder and make sure the riak user has write access.

So the check I was expecting to be added was not to confirm the script was being run as root, but to confirm that it is either being run as riak or root - and not some other user.

@martincox
Copy link
Contributor Author

Yeah, so the first check in the script is to handle when it is being invoked by the riak user - but under the current configuration, that never gets called, so we are always su'ing.

In order for that to become effective, we have to modify the service script to run as the riak user, which as far as I'm aware, is done with the --chuid switch.

And then, we can modify the user check in the wrapper script to validate both root and riak users.

Does that sound correct to you?

@martinsumner
Copy link
Contributor

Sorry, this looks like it has been sitting with me. Did we talk about this offline?

I don't have an intuitive understanding if this is going to break how thinks work stopping starting with systemd using the riak user. I can try and experiment when I'm cutting 3.0.2.

@martinsumner martinsumner added the 3.0.2 to be included in release 3.0.2 label Dec 4, 2020
@Bob-The-Marauder
Copy link
Contributor

Previously, there was a security issue where the Riak user could edit Riak files and potentially inject malicious code that would be executed when they started Riak. There was later a fix included to restrict access to root only for all Riak files that could be abused like this. I am wondering whether this check is related to that in any way.

@martinsumner
Copy link
Contributor

@martincox, just experimented with the package and this will break riak with systemd on debian/ubuntu.

This will start riak as the riak user (i.e. run the riak script as the riak user), so even though it is systemd starting the service at the point of running usr/sbin/riak it is the riak user. I think getting systemd to start as root is a bad alternative from a security perspective.

Can we leave this out of 3.0.2?

@martincox
Copy link
Contributor Author

Yes, leave it out until I can find time to test / change it to work properly 👍

@martinsumner martinsumner removed the 3.0.2 to be included in release 3.0.2 label Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants