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

[BUG] dmidecode/smbios/virt-what should be called with sudo #59794

Open
johnnybubonic opened this issue Mar 13, 2021 · 6 comments
Open

[BUG] dmidecode/smbios/virt-what should be called with sudo #59794

johnnybubonic opened this issue Mar 13, 2021 · 6 comments
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@johnnybubonic
Copy link

Description
As mentioned as a solution in #6746 (#6746 (comment)) but seemingly ignored, dmidecode/smbios in salt/modules/smbios.py and dmidecode/virt-what in salt/grains/core.py should be invoked with sudo (if present on the system) by default.

(Note: in the following cases, I'll refer to dmidecode explicitly but the same holds for all three utilites in theory.)

For my use case, I run a master as a non-root user. As a result, e.g. dmidecode cannot be invoked (and with good reason, normally).

One can SUID dmidecode, but this (as is most times when making a binary SUID root) results in a flaw where data can be harvested from e.g. /etc/shadow by any user on the system (e.g. dmidecode -d /etc/shadow, for starters). Hardly a good idea.

As such, the only alternative is to invoke dmidecode with sudo. With the following line in a sudoers file:

<salt user> ALL = NOPASSWD:/usr/sbin/dmidecode <dmidecode args salt calls>

Salt master can be run as a normal/system user, with no affect to its running as root (sudo, to my knowledge, on every installation includes root ALL=(ALL) SETENV: ALL in the default configuration) and still have access to dmidecode.

Suggested Resolution

  • Have salt call the above tools (dmidecode, smbios, virt-what), which all require root privileges, with sudo (if sudo is found present on the system).
    ** As such, they will need to be invoked with the absolute path to match the sudo rule and to prevent another hole.
  • Document exact commands and arguments that Salt invokes for these programs in particular so a properly restricted sudoers entry can be created (and/or provide an example snippet of the entry).
@johnnybubonic johnnybubonic added Bug broken, incorrect, or confusing behavior needs-triage labels Mar 13, 2021
@danielrobbins
Copy link

Anything is possible, and this seems like a good idea. I'm going to do a more thorough investigation of this (look at code, get background info) and will follow-up on this issue. (I'm on the Salt core team.)

@johnnybubonic
Copy link
Author

Much appreciated, thank you!

@danielrobbins
Copy link

@johnnybubonic would you be able to attach a sanitized version of your master conf, as well as any logs or reproduction steps for situations where you encounter the master attempting to run these commands, if you have them available?

It would be helpful to have more detail on the specific scenarios where the salt master runs into issues when running as non-root, with some logs or reproduction steps. I could try to make this happen myself, but I figure you may be more familiar with the situations in which this occurs.

@johnnybubonic
Copy link
Author

@danielrobbins Sure. I use includes for my config, so /etc/salt/master is empty:

[root@hostname ~]# egrep -Ev '^\s*(#.*)?$' /etc/salt/master
[root@hostname ~]# 

But here's a concatenation of /etc/salt/master.d:

rest_cherrypy:
  port: 8000
  host: 127.0.0.1
  log_access_file: /var/log/salt/api.access.log
  log_error_file: /var/log/salt/api.error.log
  disable_ssl: True
  static: /srv/http/salt_api
external_auth:
  ldap:
    InfraAdmins%:
      - '*'
      - '@wheel'
      - '@runner'
      - '@jobs'
auth.ldap.server: ldapserver.domain.tld
auth.ldap.port: 389
auth.ldap.tls: False
auth.ldap.starttls: True
auth.ldap.scope: 2
auth.ldap.no_verify: False
auth.ldap.anonymous: False
auth.ldap.auth_by_group_membership_only: False
auth.ldap.basedn: 'dc=domain,dc=tld'
auth.ldap.binddn: cn={{ username }},ou=Staff,dc=domain,dc=tld
auth.ldap.groupou: 'Staff'
auth.ldap.groupclass: 'inetOrgPerson'
auth.ldap.groupattribute: 'memberOf'
auth.ldap.accountattributename: 'cn'
extension_modules: /usr/local/lib/tablesalt/lib/pillar
ext_pillar:
  - dyninv: foo
user: saltstack
log_level: debug
module_dirs: ['/usr/local/lib/tablesalt/lib']
extension_modules: /usr/local/lib/salt/lib
mysql:
  user: [REDACTED]
  pass: [REDACTED]
  db: [REDACTED]
interface: '::'
ipv6: True
show_jid: True
strip_colors: False
cli_summary: True
file_roots:
  base:
    - /srv/salt
    - /usr/local/lib/tablesalt/formulas/enabled
fileserver_backend:
  - roots
  - git
pillar_roots:
  base:
    - /srv/pillar
ext_pillar_first: True
keysize: 4096
sign_pub_messages: True
ssl:
  keyfile: /etc/letsencrypt/live/hostname.domain.tld/privkey.pem
  certfile: /etc/letsencrypt/live/hostname.domain.tld/fullchain.pem
  ssl_version: PROTOCOL_TLSv1_2
failhard: True
hash_type: sha512

Honestly, the only useful part there is probably line 32, user: saltstack.

I don't seem to actually be seeing it in logs (anymore), at least with 3002.5. Previously I was seeing it in the master's log (which I found exceptionally weird, since it was a master and I don't know why it'd even need to call dmidecode) but that doesn't seem to be happening anymore.

It's not a hindrance to me personally; I run my minions as root. But this would allow for more accurate grains on minions that may be restricted to non-root users.

On second thought, this may be a duplicate of #39184 - and PR #39504 would explain why I no longer see it in the log as I believe the messages are now suppressed.

@danielrobbins
Copy link

danielrobbins commented Mar 16, 2021

So if we unpack this a bit, I think we have a trifecta of issues here.

  1. The salt master running as non-root. This, it appears, is working fine as of 3002.5, and is not even printing out confusing error messages that it can't run dmidecode -- which it shouldn't really need to run. This was probably an error message spit out on startup of salt because the master does touch some state/execution module code as well. But based on your feedback, this appears to be 100% resolved.
  2. The salt minion running as non-root

So since it appears that 1 is resolved, let's take a look at the minion. Here we have a couple of possibilities:

  1. Running the minion as non-root, as a general idea. I think this is what you are referring to in this bug report (correct me if I'm wrong) -- sort of the idea that "I like to run my daemons as non-privileged users, and it should still just work."
  2. Running the minion as non-root, for specialized use cases.

So, depending on how we look at 'running minion as non-root', we have two different possible directions. I'll cover the last first, because it's easiest.

So, if you want to run the minion as non-root, this is certainly possible, and documented here, despite not as clearly as I would like: https://docs.saltproject.io/en/latest/ref/configuration/nonroot.html -- the short summary is you can run a minion as non-root. But, if you do so, you will not be able to use its admin functionality. Some people use the minion in this way -- typically they would have a custom application that they are managing via the minion, and they will ensure that whatever user the minion runs as is able to manage their custom app, or that if they run commands on the minion, they manually preface the command with 'sudo' and have /etc/sudoers set up appropriately. This isn't really a turn-key solution -- it requires explicit use of sudo -- but it does function as intended for these customers. We might say this is not really 'solving the bigger problem'.

That brings us to issue 1 -- the possibility of having a minion, just running it as a non-privileged user, and having it just work -- even for admin functionality that normally requires root access. This is more challenging, but still potentially a good idea. But then again, we need to dig deeper and explore what we mean here.

I think there are again two possible options:

  1. Implement some functionality for the minion to implement privilege separation, where it will start as root, but then drop privileges for its main loop. This would keep most of the code running as an unprivileged user, but it could communicate with a subprocess running as root and use this subprocess for performing admin functions. This solution would not need sudo, and may provide some security benefits.

  2. Have some configuration option, so you can run the minion as non-root, but then also tell it to use sudo for everything. Now we have even more options here:

a. A new configuration option to tell the minion to use 'sudo' to wrap every command. It is then up to the user to ensure that sudo is configured sufficiently on the minion to allow the commands that the minion needs to run to actually execute.

b. Use sudo, but in a generic way where it starts a shell, and thus all admin commands can be run -- as the minion can 'get' root access. This would be more convenient but I'm not sure if it really has any security benefit, since sudo would need to be 'wide open' for the minion.

Of all these options, the one that looks like it may be promising is 2a. -- a new configuration option to essentially wrap all minion executable calls with 'sudo', but then it's left to the user to ensure that each managed system has /etc/sudoers properly configured so that any required calls succeed without requiring authentication.

Implementing this shouldn't be too hard, in theory -- one challenge is that salt execution modules may not call commands on the minions with absolute paths. So we would need to attempt to safely determine the absolute path of some commands as we build the sudo command.

Let me know if this is what you are wanting.

@sagetherage sagetherage added this to the Approved milestone Mar 31, 2021
@sagetherage sagetherage added the severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around label Mar 31, 2021
@johnnybubonic
Copy link
Author

johnnybubonic commented Apr 12, 2021

Hello-

Apologies for the delay in my response.

The salt master running as non-root. This, it appears, is working fine as of 3002.5, and is not even printing out confusing error messages that it can't run dmidecode -- which it shouldn't really need to run. This was probably an error message spit out on startup of salt because the master does touch some state/execution module code as well. But based on your feedback, this appears to be 100% resolved.

Actually, I think this is where the issue is and why I couldn't find it in my logs - I was grepping for dmidecode, but it's virt-what that it was having issues with:

[ ~ ] # fgrep 'virt-what' /var/log/salt/master | tail
2021-04-12 10:32:43,861 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:33:44,409 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:34:45,001 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:35:45,512 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:36:46,024 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:37:46,569 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:38:47,111 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:39:47,650 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:40:48,203 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.
2021-04-12 10:41:48,827 [salt.loaded.int.grains.core:1141][INFO    ][4151431] Although 'virt-what' was found in path, the current user cannot execute it. Grains output might not be accurate.

My use-case is the master running as non-root. It seems the master does not invoke virt-what with sudo.

Running the minion as non-root, as a general idea. I think this is what you are referring to in this bug report (correct me if I'm wrong) -- sort of the idea that "I like to run my daemons as non-privileged users, and it should still just work."

This is inaccurate; my bug report was filed in the scope of master running as non-root. Since minions are the ones actually doing the heavy-lifting, one would of course presuppose they'd run into more permission issues. However, as such they extensively make use of sudo - it seems master is designed with the assumption it will be run as root, and as such it seems to invoke sudo much less if at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

No branches or pull requests

3 participants