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

Set group owner to foreman-proxy for Salt config #815

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

maximiliankolb
Copy link
Contributor

Fixes 4ea5cf8

@maximiliankolb maximiliankolb marked this pull request as ready for review October 6, 2023 15:18
@evgeni
Copy link
Member

evgeni commented Oct 6, 2023

Why would the proxy user need to be able to write that file?

@maximiliankolb
Copy link
Contributor Author

maximiliankolb commented Oct 6, 2023

I have an internal ticket that says Salt REX jobs do not work. A documented and tested workaround is running chown foreman-proxy:foreman-proxy /etc/salt/master.d/foreman.conf which I have tried to implement here. I also wanted to talk to @bastian-src because he initially implemented this.

Do you think this change is undesired in general, or should I fix the mode to make it read-only?

Also: My bad for opening such a bare-bone PR without any background info!

@evgeni
Copy link
Member

evgeni commented Oct 6, 2023

Can you share more info what is meant by "do not work"?

The contents of the file come from a template (https://github.com/theforeman/puppet-foreman_proxy/blob/master/templates/plugin/salt_master.conf.erb) and any changes to it would be overwritten at the next installer run anyway, so IMHO nobody (but the installer) should be writing it.

As for reading… Your change should not have any effect for the foreman-proxy user (as that can already read via the group ownership) or root (as they usually has CAP_DAC_OVERRIDE capability).

Unless… There is something that does a "is file owned by my user, error otherwise" check somewhere.

@evgeni
Copy link
Member

evgeni commented Oct 6, 2023

And to elaborate the current setting a bit: changing the file allows to reconfigure the salt master, which usually runs as root. If we allow the foreman-proxy user to do that, we will allow almost arbitrary escalation from that user to root in the case of a breach.
Setting the mode to 0440 would "fix" that (the owner of a file can't change it permissions w/o having write access to the directory containing the file), but I would like to understand the original ask first.

@maximiliankolb
Copy link
Contributor Author

I could not find out why the file is read when running Salt REX on Foreman. However, I investigated some more with @sbernhard together.

It looks like there is already a command to fix this, but it does not have a working default value yet. I have changed my PR to reflect that.

We have tested this on Foreman:

# foreman-installer --foreman-proxy-plugin-salt-group foreman-proxy
# ls -l /etc/salt/master.d/
total 4
-rw-r-----. 1 root foreman-proxy 1268  9. Okt 16:55 foreman.conf

@evgeni
Copy link
Member

evgeni commented Oct 9, 2023

I still don't understand the need for that, tho.

The only place I can see any code in smart_proxy_salt to use something from /etc/salt/master is https://github.com/theforeman/smart_proxy_salt/blob/master/sbin/upload-salt-reports and I'd expect that is executed as the same user as Salt and thus already having the right perms?

manifests/plugin/salt.pp Outdated Show resolved Hide resolved
manifests/plugin/salt.pp Outdated Show resolved Hide resolved
@bastian-src
Copy link
Contributor

I still don't understand the need for that, tho.

The only place I can see any code in smart_proxy_salt to use something from /etc/salt/master is https://github.com/theforeman/smart_proxy_salt/blob/master/sbin/upload-salt-reports and I'd expect that is executed as the same user as Salt and thus already having the right perms?

Hey! 👋 We just had a look at the problem.

The following error occurs, when executing a Salt REX task:

2023-10-11T13:55:30 [I|app|d5d6ffad]   Parameters: {"callback"=>{"task_id"=>"<some id>", "step_id"=>3}, "data"=>{"result"=>[{"output_type"=>"stdout", "output"=>"Usage: salt [options] '<target>' <function> [arguments]\r\n\r\nsalt: error: Failed to load configuration: [Errno 13] Permission denied: '/etc/salt/master.d/foreman.conf'\r\n", "timestamp"=>1697025329.559228}], "runner_id"=>"<some id>", "exit_status"=>1}, "task"=>{}}

The call origins here in smart_proxy_salt and I assume those runners are executed as foreman-proxy.

But, in order to run salt, the corresponding user must be able to read /etc/salt/master.d/foreman.conf.
Therefore, I would argue we make the group foreman-proxy by default or we set the config readable for others.

For example, Ansible has the following permissions on one of its configs:

-rw-r--r--. 1 root root 103 Sep 26 12:54 /etc/ansible/ansible.cfg

@evgeni what do you think?

@evgeni
Copy link
Member

evgeni commented Oct 11, 2023

LGTM!

Technically, you could also swap the

Optional[String[1]] $group = undef,

to a

String $group = $foreman_proxy::user,

I think I prefer the way it is now (as it keeps the "default" empty, and thus doesn't even store the value in the answers file), but wanted to point it out.

@ekohl opinions on the style?

@ekohl ekohl changed the title Fix file ownership for Salt config Set group owner to foreman-proxy for Salt config Oct 12, 2023
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I prefer this style.

@ekohl ekohl merged commit fbf3322 into theforeman:master Oct 12, 2023
7 checks passed
@evgeni evgeni added the Bug label Oct 12, 2023
@maximiliankolb maximiliankolb deleted the salt_config branch October 12, 2023 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants