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

Update Salt Master configuration template #839

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

Conversation

bastian-src
Copy link
Contributor

→ This is more of an pro-active change. I don't quite know where exactly we run into an issue with cherrypy-API permissions. So, I need a second opinion on this.

→ This is motivated by the following change:

Since 3006, Salt installs with an explicit user that runs the Salt Master daemon in order to limit the daemon permissions. If that users differs from the user that we execute the salt ... commands with (when triggering Salt from Smart Proxy), we get some permission issues. More specifically, sudo -u foreman-proxy salt '*' test.ping runs into the following error:

[ERROR   ] Unable to connect to the salt master publisher at /var/run/salt/master
The salt master could not be contacted. Is master running?

When executing salt '*' test.ping, that comand wants to connect to the Salt Master whose PID is stored in /var/run/salt/master/*. But, with the 3006-explicit-user update, they limit access to the /var/run/salt folder to the same user which runs the Salt Master daemon.

Therefore, I see two options:

A) Make the Salt Master daemon run with the same user that we execute the salt ... commands with (currently implemented in this PR)
B) Take care that the foreman-proxy user has the necessary permissions/adapt the folder permissions that the foreman-proxy user can execute salt ... even if the daemon is started by a different user.

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've often wondered if we shouldn't have code to fully manage the whole salt server, just like we can set up and manage the the configuration.

What I mean by that is to simplify the procedure. https://docs.theforeman.org/nightly/Managing_Configurations_Salt/index-katello.html now documents a lot of manual steps and that's generally not something we want.

For example, https://docs.theforeman.org/nightly/Managing_Configurations_Salt/index-katello.html#Authenticating_Salt_Minions_Using_Host_Names_managing-configurations-salt tells you to add foreman-proxy to the root group. IMHO that's extremely dangerous.

https://docs.theforeman.org/nightly/Managing_Configurations_Salt/index-katello.html#Configuring_the_Salt_API_managing-configurations-salt is also completely wrong because it must use installer options to persist.

I wrote up https://github.com/theforeman/puppet-foreman_proxy/blob/master/examples/salt.pp and https://github.com/theforeman/puppet-foreman_proxy/blob/master/spec/acceptance/salt_spec.rb as end-to-end testing, but as you can see it's still extremely basic. It would be nice if we actually had testing that verified some end-to-end workflow.

@maximiliankolb tagging you in because the Salt documentation must be reviewed. It can't be right.

Comment on lines 48 to 49
- local
- local_async
Copy link
Member

Choose a reason for hiding this comment

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

Should this be configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll make it configurable.

Do you think tftp:packages (here) is a good example which I can use as a guide on how to add a List-paramter?

@maximiliankolb
Copy link
Contributor

Thanks for the ping. Making sure that the docs work after we've merged this is definitely on my plate!

@sbernhard Is there any use case to configure the interfaces? Or are these two simply required to make Salt+Foreman work?

docs: https://docs.saltproject.io/en/master/topics/netapi/netapi-enable-clients.html#enabling-netapi-client-interfaces

@maximiliankolb
Copy link
Contributor

@sbernhard Friendly reminder.

@bastian-src
Copy link
Contributor Author

Coming back to this, thanks @ekohl for the review and feedback!

I totally agree, we should re-think the user management and I think since Salt switches to a non-root user we can take this as reason to update ours too.

However, for this PR, I'd just update the master configuration. I'm currently testing which default config we should apply for netapi to guarantee that the salt-api works for Foreman.

@bastian-src bastian-src force-pushed the fix/salt_conf_user_and_netapi branch 7 times, most recently from e941111 to 6ec9558 Compare September 20, 2024 14:37
* Set user for running Salt Master service due to 3006 changes
  https://docs.saltproject.io/en/3006/topics/releases/3006.0.html#linux-packaging-salt-master-salt-user-and-group

Salt API

* Add netapi_enable_clients explicitly due to 3006 changes
  https://docs.saltproject.io/en/master/topics/netapi/netapi-enable-clients.html#select-client-interfaces-to-enable
* Add api_interfaces parameter to enable configuration of API
  accessibility. However, smart_proxy_salt utilizes only
  the "runner" API interface.
Copy link
Contributor

@sbernhard sbernhard left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants