Fix salt-master package upgrade resetting non-root user ownership (#68577)#69360
Open
dwoz wants to merge 2 commits into
Open
Fix salt-master package upgrade resetting non-root user ownership (#68577)#69360dwoz wants to merge 2 commits into
dwoz wants to merge 2 commits into
Conversation
added 2 commits
June 17, 2026 03:43
The Debian preinst and the rpm `%pre master` scriptlet decided which
user owns the salt-master state directories on upgrade by inspecting
filesystem ownership only: deb looked at `/run/salt-master.pid`, rpm
looked at `/run/salt/master`. Neither read `user:` from
`/etc/salt/master` or a drop-in under `/etc/salt/master.d/`. On
systemd-managed installs those paths can be root-owned (or the pid file
absent) between restarts, so the preinst's `db_set salt-master/user`
wrote the package default (`salt`) back into debconf and the subsequent
chowns clobbered the configured non-root user. The postinst then
re-chowned to whatever debconf reported.
Make both scriptlets read `user:` from `/etc/salt/master` and
`/etc/salt/master.d/*.conf` as the authoritative source and fall back
to filesystem ownership only when no user is configured. Drop the dead
`%global _MS_CUR_USER ...` lines from `%pre master` — `%global` is an
rpm parse-time directive and the macros referenced (`%{_MS_LCUR_USER}`)
were never defined as rpm macros, so the lines were inert.
Add a regression test at `tests/pytests/unit/pkg/test_master_scriptlets.py`
that runs the actual scriptlet shell code in a sandbox and asserts that
a `user: bob` config produces `CUR_USER=bob` for both deb and rpm.
Fixes saltstack#68577
rpmbuild parses macros even inside shell comments within scriptlet sections. The literal `%global _MS_CUR_USER ...` in the explanatory comment caused rpmbuild to try to define a macro at parse time, failing with 'Macro % has illegal name (%global)'. Escape with %% so the comment reads as a literal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Make the salt-master
.debpreinst and the rpm%pre masterscriptlethonor the
user:directive in/etc/salt/master(or a drop-in under/etc/salt/master.d/) when deciding which user owns the master statedirectories during an upgrade. Also remove dead
%global _MS_CUR_USER ...macro lines from
%pre master(rpm expands%globalat build time andthe referenced macros were undefined at that point).
What issues does this PR fix or reference?
Fixes #68577
Previous Behavior
The Debian preinst never read
/etc/salt/master; it inferred thetarget user from filesystem ownership of
/run/salt-master.pidandfell back to the package default
saltotherwise. The rpm%pre mastersimilarly only looked at
ls -dl /run/salt/master. On systemd-managedinstalls those paths can be root-owned (or the pid file absent) between
restarts, so the upgrade would
db_set salt-master/user saltand thesubsequent
chown -R salt:saltreset state directory ownership evenwhen
/etc/salt/masterclearly specified a non-root user. The postinstthen re-chowned to whatever debconf reported, so the misconfigured
state persisted.
New Behavior
Both scriptlets read
user:from/etc/salt/masterand/etc/salt/master.d/*.confas the authoritative source and usefilesystem ownership only as a fallback. The dead
%globallines areremoved from the spec.
A new regression test at
tests/pytests/unit/pkg/test_master_scriptlets.pyruns the actualscriptlet shell code in a sandbox and asserts that a
user: bobconfig produces
CUR_USER=bobfor both deb and rpm.This mirrors the salt-minion-side fix in #69351 / #68793. The
salt-syndic and salt-api preinst scripts have the same shape but are
out of scope here — the reporter's title is salt-master + salt-minion.
Merge requirements satisfied?
Commits signed with GPG?
No