Skip to content
This repository was archived by the owner on Jan 24, 2024. It is now read-only.

SEP 19: Improve running salts under user privileges/accounts #35

Closed
wants to merge 7 commits into from
Closed

SEP 19: Improve running salts under user privileges/accounts #35

wants to merge 7 commits into from

Conversation

The-Loeki
Copy link
Contributor

Request for Comments ;)

Aims

  • Salt daemons (minion excluded?) running default under system-user credentials
  • Salt CLI tools use XDG/other relevant standard locations

References

@The-Loeki The-Loeki requested a review from a team as a code owner September 19, 2020 09:13
@The-Loeki The-Loeki requested review from Ch3LL and removed request for a team September 19, 2020 09:13
@ghost ghost requested a review from waynew September 19, 2020 09:13
@sagetherage
Copy link
Contributor

Thank you, @The-Loeki for submitting a Salt Enhancement Proposal! I apologies for not greeting you sooner and welcoming the SEP. I will have a Core Team member review this more thoroughly in the next week.

@sagetherage sagetherage added Draft Initial Status Final Comment Period Speak now or forever hold your peace. and removed Draft Initial Status labels Feb 5, 2021
Copy link

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ma all in favor of running Salt without root privileges, especially after the pita it was getting parts working on Debian/Ubuntu during packaging with gpg signing. However I fear the move to XDG, given past experience with 'everyone was moving to HFS' back in the late 90's / early 2000's and that didn't go has well as planned.

I think this SEP as it stands as two parts that are unrelated: namely running Salt as non-root, and changing the location of Salt files, (configuration, daemons, etc.)

Additionally there is the urge to make things convenient for Linux based systems, but Salt runs on more than Linux, and those other platforms should be considered too, noting mention of BSD, but Salt is currently running on Arista and Juniper routers natively, and these can constrain locations.

I am in favor of making Salt function better as a non-root user, concerned about layout changes, and with the move and direction to self-contained tiamat (use of /opt/saltstack/salt directories), and use of pop, if layout changes would be applicable.

@sagetherage
Copy link
Contributor

I ma all in favor of running Salt without root privileges, especially after the pita it was getting parts working on Debian/Ubuntu during packaging with gpg signing. However I fear the move to XDG, given past experience with 'everyone was moving to HFS' back in the late 90's / early 2000's and that didn't go has well as planned.

I think this SEP as it stands as two parts that are unrelated: namely running Salt as non-root, and changing the location of Salt files, (configuration, daemons, etc.)

Additionally there is the urge to make things convenient for Linux based systems, but Salt runs on more than Linux, and those other platforms should be considered too, noting mention of BSD, but Salt is currently running on Arista and Juniper routers natively, and these can constrain locations.

I am in favor of making Salt function better as a non-root user, concerned about layout changes, and with the move and direction to self-contained tiamat (use of /opt/saltstack/salt directories), and use of pop, if layout changes would be applicable.

these should be added to risks and/or things to detail in the implementation @The-Loeki
@dmurphy18 we are looking for rough consensus and in implementation this can be worked out, but I would like to add it to the PR.

@sagetherage
Copy link
Contributor

After discussing this SEP with the Core Team we would like to separate out the changing the location of Salt files, (configuration, daemons, etc.) into a separate SEP. @The-Loeki

@barneysowood
Copy link
Contributor

Very much in favour of making it the default to run Salt as a non-root user. I think as suggested separating out the issue of changing config file locations into another SEP is the right thing to do too.

I think we should further clarify/reduce the aim of this to be

  • Running the salt-master and salt-api (if used) running under unprivileged accounts
  • Allowing users to run the cli clients (salt, salt-run, salt-cloud, salt-cp, salt-key) as non root users

I think it's reasonable to exclude the salt-minion (and salt-call) as normally they're going to require running as root in order to make system level changes.

In order to allow users to easily use the cli clients (without requiring the use of sudo or similar) we could:

  • Add a group on package install (call it salt-users for example)
  • Make salt config files and directories readable by that group
  • Possibly make /var/log/salt writable (and maybe setgid) by that group

Users could then be given access to salt by adding them to the salt-users group.

There's probably an argument for also setting up a series of additional groups, with default config in publisher_acl for say "salt-superusers", "salt-admins" with different levels of privilege to encourage users assign finer grained access.. obviously they'd still need to be in the salt-users group too.

On the shared log, it maybe be relatively easy to split out where the cli clients and the daemons log to by changing logger settings.

Final thing on PAM and external auth - a fairly simple way to maintain existing functionality (at least on most Linux distros) would be to add the user that the salt daemons run as to the shadow group. Packages could default to not doing that and have it selected at install time (eg w/debconf or similar). It's not ideal as it does mean the salt daemon user can read /etc/shadow if compromised, but that's a lot better than being able to run anything as root due to shell injection.

@udf2457
Copy link

udf2457 commented Apr 1, 2021

I think it's reasonable to exclude the salt-minion (and salt-call) as normally they're going to require running as root in order to make system level changes.

I disagree strongly here.

There should be segregation of duties.

Yes, system level changes needs root. No debate about that.

But salt-minion also receives orders from the master, and it also processes its own config files locally.

There is a strong case (perhaps especially for the network facing part of salt-minion) for many parts of salt-minion to not have root !

Only the parts of salt-minion that actually need root should have it.

If you want inspiration, look at the way postfix does privilege seperation. It has root for things like port binding <1024, but runs non-privileged for 99% of other things.

@waynew
Copy link
Contributor

waynew commented Apr 1, 2021

That's actually a really good point. It should be "fairly trivial" to have a root-runner process that is small and its entire job is simply running commands that actually need the escalated privileges 🤔

@barneysowood
Copy link
Contributor

I think it's reasonable to exclude the salt-minion (and salt-call) as normally they're going to require running as root in order to make system level changes.

I disagree strongly here.

There should be segregation of duties.

Yes, system level changes needs root. No debate about that.

But salt-minion also receives orders from the master, and it also processes its own config files locally.

It receives orders from the master that can instruct it to make changes as root. If the master is compromised, even if you separated the minion into an unprivileged part that talked to the master and a privileged part that ran modules and states, a compromised master would still be able to control the privileged part of the minion.

There is a strong case (perhaps especially for the network facing part of salt-minion) for many parts of salt-minion to not have root !

If the minion was listening and potentially accepting incoming connections from unknown sources, there would be a strong argument for this. As it is the minion doesn't do that, it connects to the master. If your master is compromised or an attacker is able to impersonate your master sufficiently for the minion to connect to it, the privilege separation probably isn't going to help much.

Only the parts of salt-minion that actually need root should have it.

Absolutely agree this would be a great thing to aim for. However, I also think it's going to take a very large amount of effort to do that and it will also add a massive amount of complexity.

I think given limited resources (which is definitely what the project has) it would be better to focus initially on securing those components that are exposed to potentially untrusted clients connecting and don't actually have any real need to run as root (salt-master and salt-api). I think that's going to give the largest gains in terms of improving the default security posture of a salt installation.

If you want inspiration, look at the way postfix does privilege seperation. It has root for things like port binding <1024, but runs non-privileged for 99% of other things.

Yep, postfix does privilege separation very well - but it's trying to solve a different problem. In order to function as an MTA it needs to be able to accept connections from anywhere and deal with malicious traffic whilst still handling legitimate but untrusted clients. Furthermore it needs to then handle and parse untrusted, potentially incorrect and generally malformed data and try and deliver it safely. Given those requirements, the complexity of the design, the engineering required and the various tradeoffs that causes are definitely worth it.

@udf2457
Copy link

udf2457 commented May 6, 2021

Absolutely agree this would be a great thing to aim for. However, I also think it's going to take a very large amount of effort to do that and it will also add a massive amount of complexity.

I think given limited resources (which is definitely what the project has) it would be better to focus initially on securing those components that are exposed to potentially untrusted clients connecting and don't actually have any real need to run as root (salt-master and salt-api). I think that's going to give the largest gains in terms of improving the default security posture of a salt installation.

I'm really not sure it would add "a massive amount of complexity".

Nor is anybody saying you have to do it all overnight.

Surely you could add the necessary framework to the minion codebase and then "upgrade" the various state processes etc. as time went by ?

As attacks on systems get more and more advanced, simply sitting by and saying "its too hard to make it more secure" is not really an excuse.

@barneysowood
Copy link
Contributor

@sagetherage - should this still be open? You merged the updated version, although that needs a further update to move it to the right place and update the status, assuming it should have been merged?

@dkacar-oradian
Copy link

I have a completely different use case. This SEP and and all comments are centered about security issues when running master and minion. But I have a hazy idea about a different kind of system: two masters somewhere and two minions per host, one for each master.
The first of those would be for OS configuration, so running under root. Or, if you manage to pull this off with privilege separation and all, maybe running under that.
The second pair (probably salt + idem) would be running under unprivileged user and it would not be used to configure stuff for which root privileges are needed.

That should give me two important benefits: security and parallelism.

In my use case the sets of people who would use these services are different. Even if the requests are coming from some complication, like Jenkins server which makes salt API calls to the master which then makes a call to the minion, the two sets can be using different Jenkins servers (or whatever). So I hope to gain more security by separating humans who need root privileges from those who do not.
Even if you manage to implement all this in a reasonable amount of time you're still going to have security bugs in the implementation, so I don't have much trust in the whole thing for at least several years. And I'd rather have some security features elsewhere in addition to what salt can provide.

Root master/minion wouldn't be running things in parallel, but unprivileged minion would. That means I'd have to implement locking myself in some manner, but that's fine. The important thing is that these two shouldn't be blocking one another.
Unprivileged minion should also be a bit more privileged than a single user, for example it should be able to create (chown, probably) files as users from a small, limited set. I need this on Linux only and I hope I could pull that trick by running the minion in a separate user namespace where in-namespace root user can do that. But again, that's my problem, not yours.

The only thing I'd like to have from you is not to write code which would prevent this kind of configuration. For example, minion startup procedure shouldn't depend on some grain which absolutely requires root privilege. And all files, directories and other resources used for minion's internal operation should be configurable. Ideally, there would be a single documentation source which would list all configuration directives necessary to run two separate minions on a single host.

It will be at least a year before I'd be able to seriously start working on this, so I don't have much more details at this time.

@waynew
Copy link
Contributor

waynew commented Oct 6, 2021

But I have a hazy idea about a different kind of system: two masters somewhere and two minions per host, one for each master. The first of those would be for OS configuration, so running under root. Or, if you manage to pull this off with privilege separation and all, maybe running under that. The second pair (probably salt + idem) would be running under unprivileged user and it would not be used to configure stuff for which root privileges are needed.

FWIW that's actually mostly possible today. Obviously they'll have to run under separate ports, but it's possible to just straight up run Salt as non-root. Granted, there are several bits of Salt today that are expecting root so you may find some issues there but I actually run Salt as non-root all the time when I'm debugging various things in Salt.

Of course that's completely running as non-root, as opposed to partially running as root, which is the focus of this SEP 🙃

@waynew
Copy link
Contributor

waynew commented Jan 5, 2022

Apologies for the long hiatus, we're working to refocus on getting SEPs through the process.


To summarize things so far:

There have been several implementation detail suggestions, but overall there has been no disagreement that it's a good idea to be able to make Salt run as non-root. At this point, it's ready for a core team vote for approval.

(From what I can tell, requested updates were made in #55, on approval this PR should simply be closed+marked accepted)

@barneysowood
Copy link
Contributor

(From what I can tell, requested updates were made in #55, on approval this PR should simply be closed+marked accepted)

@waynew - that's correct, that's my updated version, not sure why it was merged.

@waynew
Copy link
Contributor

waynew commented Mar 10, 2022

Pretty sure it was an accident 😅

@viq
Copy link

viq commented Apr 17, 2022

I finally sat down to write down my experiences making salt-master run as non-root user. You'll see I named user and group _salt because that's the convention on OpenBSD, where back in 2014 I helped make it work, and I've been running my masters that way since...

Permissions

This mostly means write access to directories, or even just specific files, either via regular permissions or ACLs. This may mean giving access to the group. Remember that to list files, an execute permission is necessary on the directory.

  • /etc/salt/master and /etc/salt/master.d/
    • read-only is sufficient. You don't need to do anything if those files are world-readable.
    • You may want to do something like 640 root:_salt for files that contain secrets
  • /etc/salt/pki/master/
  • /etc/salt/gpgkeys/ (or otherwise configured gpg_keydir)
  • /var/cache/salt/master/
  • /var/log/salt/master
    • in my case /var/log/salt/ is 750 root:_salt

Running commands

To avoid commands clobbering permissions, make sure to run all commands as your dedicated salt user. A shell alias can help with that. All my commands are prefixed with sudo -u _salt -H, making it e.g. sudo -u _salt -H -E salt-run cache.clear_git_lock gitfs type=update

Example configurations

Here are some example configurations on various distributions

OpenBSD

You don't need to do anything, it does that out of the box :)

Archlinux

Dedicated user and group:

# grep salt passwd group 
passwd:_salt:x:10722:10722:SaltStack Daemon:/var/salt:/sbin/nologin
group:_salt:x:10722:

In /etc/salt/master set it to run as that user:

user: _salt

Set permissions as described above.

NixOS

This shows my NixOS config, creating a system user, adding additional packages to salt's environment, and passing whole configuration to salt-master. The important parts for basic usage are users.users._salt and services.salt.master.configuration = { user = “_salt”; };, and then you need to ensure that /var/cache/salt/master exists with proper permissions. PKI dir by default will get created under /var/lib/salt, so most likely you don't need to worry about that.

{ config, lib, pkgs, modulesPath, ... }:

{
  users = {
    groups = {
      _salt = {
        gid = 10722;
      };
    };
    users = {
            _salt = {
              isSystemUser = true;
              uid = 10722;
              group = "_salt";
              home = "/var/lib/salt";
              shell = "/run/current-system/sw/bin/nologin";
              description = "SaltStack Daemon";
            };
  };
  nixpkgs.config.packageOverrides = pkgs: {
    salt = pkgs.salt.override { extraInputs = [
      pkgs.gnupg
      pkgs.libssh2
      pkgs.pass
      pkgs.python3.pkgs.jmespath
      pkgs.python3.pkgs.pygit2
      pkgs.python3.pkgs.gitdb
      pkgs.python3.pkgs.python-gnupg
    ]; };
  };
  services = {
    salt ={
      master = {
        enable = true;
        configuration = {
          user = "_salt";
          cli_summary = true;
          state_output = "changes";
          ping_on_rotate = true;
          keysize = 4096;
          presence_events = true;
          state_events = true;
          peer = {
            ".*" = [ "x509.sign_remote_certificate" ];
            "some.server.local" = [
              "grains.get"
              "network.ipaddrs"
            ];
          };
          reactor = [
            {"salt/auth" = [
              "salt://reactor/ntpdsync.sls"
              "salt://reactor/start-notify.sls"
            ];}
            {"salt/presence/change" = [ "salt://reactor/presence-changes.sls" ];}
            {"salt/engines/hook/prometheus/alertmanager" = [ "salt://reactor/prometheus.sls" ];}
            {"updates/obsd/*" = [ "salt://reactor/upgrades-notify.sls" ];}
          ];
          fileserver_backend = [ "git" "roots" ];
          file_roots = {
            base = [
              "/srv/salt"
            ];
          };
          thorium_roots = {
            base = [ "/srv/thorium" ];
          };
          gitfs_provider = "pygit2";
          gitfs_user = "git";
          gitfs_pubkey = "/var/lib/salt/.ssh/id_ed25519.pub";
          gitfs_privkey = "/var/lib/salt/.ssh/id_ed25519";
          state_top_saltenv = "base";
          gitfs_remotes = [ "ssh://[email protected]/salt-roots.git" ];
          gpg_keydir = "/var/lib/salt/gpgkeys";
          "ext_pillar" = [
              {
                 "git" = [
                       {"master ssh://[email protected]/salt-pillars" = [
                          {"user" = "git";}
                          {"pubkey" = "/var/lib/salt/.ssh/id_ed25519.pub";}
                          {"privkey" = "/var/lib/salt/.ssh/id_ed25519";}
                       ];}
                 ];
              }
           ];
        };
      };
    };
}

@viq
Copy link

viq commented Apr 17, 2022

I think it's reasonable to exclude the salt-minion (and salt-call) as normally they're going to require running as root in order to make system level changes.

I disagree strongly here.
There should be segregation of duties.
Yes, system level changes needs root. No debate about that.
But salt-minion also receives orders from the master, and it also processes its own config files locally.

It receives orders from the master that can instruct it to make changes as root. If the master is compromised, even if you separated the minion into an unprivileged part that talked to the master and a privileged part that ran modules and states, a compromised master would still be able to control the privileged part of the minion.

There is a strong case (perhaps especially for the network facing part of salt-minion) for many parts of salt-minion to not have root !

If the minion was listening and potentially accepting incoming connections from unknown sources, there would be a strong argument for this. As it is the minion doesn't do that, it connects to the master. If your master is compromised or an attacker is able to impersonate your master sufficiently for the minion to connect to it, the privilege separation probably isn't going to help much.

Reviving an old part of discussion - recent vulnerabilities have shown, that attackers on the network can do unpleasant things to a minion without (fully) impersonating master. Processing at least first level of communications in an unprivileged process would further increase difficulty of abusing the processes.

@barneysowood
Copy link
Contributor

@s0undt3ch - just to note that the updated version of this was already merged (probably by mistake) in #55 so it's likely to conflict. I think the version I proposed and waas merged is correct, although it should move to the "accepted" dir.

@s0undt3ch
Copy link

Oh. Woops.

@The-Loeki The-Loeki closed this by deleting the head repository Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Final Comment Period Speak now or forever hold your peace.
Projects
None yet
Development

Successfully merging this pull request may close these issues.