[3006.x] Add SALT_ONEDIR_HARDEN opt-in for daemon writable-state isolation (#70198) - #70208
Open
dwoz wants to merge 15 commits into
Open
[3006.x] Add SALT_ONEDIR_HARDEN opt-in for daemon writable-state isolation (#70198)#70208dwoz wants to merge 15 commits into
dwoz wants to merge 15 commits into
Conversation
Introduce SALT_ONEDIR_HARDEN=1 as an opt-in packaging mode that
relocates each daemon's writable state under per-daemon
/var/lib/salt/<daemon>/{home,extras-<py>} directories so
/opt/saltstack/salt stays root:root 0755. On 3006.x the default is
UNSET (legacy chown -R salt /opt/saltstack/salt behavior preserved)
so existing LTS deployments continue to work without intervention.
The default flips to hardened on 3009.0 (saltstack#70199).
To opt in on 3006.x, set SALT_ONEDIR_HARDEN=1 in /etc/default/salt-setup
(DEB) or /etc/sysconfig/salt-minion-setup (RPM) before installing or
upgrading.
Runtime salt-pip and the onedir _salt_onedir_extras .pth hook honor
SALT_EXTRAS_DIR so packages installed via salt-pip continue to be
importable by the daemon after the packaging layer relocates the
extras tree. Postinst / posttrans scriptlets migrate any populated
legacy /opt/saltstack/salt/extras-<py>/ contents into the per-daemon
location once (when the operator opts in), idempotent no-op otherwise.
Sibling of saltstack#70199 (master) per the per-branch rollout in issue saltstack#70198.
Per-daemon fanout for the writable-dir existence/ownership checks (minion/master/syndic/api/cloud), belt-and-braces asserts for /opt/saltstack/salt files staying root-owned when hardening opt-in is selected, an explicit unset-vs-1 gate check that pins the 3006.x legacy default so an accidental default flip on LTS fails CI loud, and multi-role isolation verification that each daemon's /var/lib/salt/<daemon>/ is a distinct directory with no cross-daemon file visibility. Also includes a runtime contract test for salt-pip honoring SALT_EXTRAS_DIR. The _hardened_mode_selected() gate on 3006.x is opposite of master: here, SALT_ONEDIR_HARDEN unset skips the hardened-layout assertions (legacy is the default). Operators must explicitly export SALT_ONEDIR_HARDEN=1 for the hardened tests to run. Refs saltstack#70198.
New sibling directory tests/pytests/pkg/integration/onedir_harden_lifecycle/ covers what the session-scoped install_salt fixture can't on 3006.x: - SALT_ONEDIR_HARDEN unset (3006.x default): legacy chown layout, per-daemon /var/lib/salt/<daemon>/ dirs absent - SALT_ONEDIR_HARDEN=1 opt-in: hardened layout, /opt/saltstack/salt stays root-owned - Explicit SALT_HOME and SALT_EXTRAS_DIR overrides win over the HARDEN=1 opt-in (per-daemon dirs replaced by operator-chosen paths) - Upgrade migration: pre-populates /opt/saltstack/salt/extras-<py>/ with a marker under HARDEN unset, then reinstalls with HARDEN=1 and asserts marker moved into /var/lib/salt/<daemon>/extras-<py>/ - Idempotency: dpkg-reconfigure / dnf reinstall doesn't resurrect the legacy dir or duplicate the marker Mirrors the config_overrides/ destructive-fixture pattern (collection-skip on upgrade/downgrade/no-install passes, per-test apt/rpm purge, force-purge on postinst failure). 7 new tests. The "HARDEN=0 escape hatch" and "deprecation warning surfaces" tests from master are omitted because on 3006.x the default is already unset (no opt-out needed) and there is no deprecation warning (that lands with the default flip on 3009.0). Refs saltstack#70198.
…o cloud
The %posttrans scriptlet directives for master, syndic, and api were
indented four spaces:
%posttrans master
...
%posttrans syndic
...
%posttrans api
RPM only recognizes scriptlet section markers when they start at column
0. With leading whitespace, rpmbuild silently treats them as shell text
that belongs to the previous %posttrans cloud section. Result: the
per-daemon posttrans hooks for master, syndic, and api never ran on
any RPM-based install, so their hardened-layout directory creation and
legacy-extras migration silently did nothing.
Surfaced by test_upgrade_migration_moves_legacy_extras on Photon 4:
the fixture drops a marker into /opt/saltstack/salt/extras-<py>/ and
expects the hardened postinst to move it to /var/lib/salt/<daemon>/;
no daemon-side move happened because master/syndic/api posttrans were
dead code.
Dedent the three directives to column 0. No behavior change intended
other than actually running the scriptlets.
…hs for salt-pip The onedir packaging installs salt-pip at /opt/saltstack/salt/salt-pip (not .../bin/salt-pip). Every distro pkg install job hit: FileNotFoundError: [Errno 2] No such file or directory: '/opt/saltstack/salt/bin/salt-pip' Use install_salt.binary_paths['pip'] like the other pkg integration tests do (see test_pip.py, test_pip_upgrade.py). Stays correct across every distro's package layout.
…eeds test_mod_repo and test_mod_repo_no_file call aptpkg.mod_repo, which internally runs 'apt-get update'. That update surfaces any transient 404 on unrelated 3rd-party feeds in /etc/apt/sources.list.d/. The GitHub Actions Debian 11 image ships an /etc/apt/sources.list.d hashicorp entry pointing at 'bullseye'; Hashicorp dropped bullseye from apt.releases.hashicorp.com so the update aborts with: E: The repository 'https://apt.releases.hashicorp.com bullseye Release' no longer has a Release file. That's outside these tests' scope but wedges them red. Extend revert_repo_file to also move /etc/apt/sources.list.d/* aside for the duration of the test and restore on teardown, so a 3rd-party feed churn on someone else's mirror doesn't red our functional runs. Not a Salt bug in the module itself, but the fix belongs at the test fixture boundary so we stop chasing external mirror state.
twangboy
previously approved these changes
Sep 3, 2026
Six is bundled in the onedir at /opt/saltstack/salt/lib/python*/site-packages, so 'salt-pip install --no-deps six' returns 0 with 'Requirement already satisfied' and never writes to the SALT_EXTRAS_DIR --target override. The override dir is left empty and the test's post-install assertion fails. Switch to pep8 -- known-not-bundled per the existing test_pip.py::test_pip_install_extras which uses pep8 for exactly this reason. This makes the runtime SALT_EXTRAS_DIR contract actually observable end-to-end. Failed uniformly across every install/upgrade/ downgrade Test Package job on this PR.
Round 2's fix (246085c) quarantined sources.list.d 3rd-party feeds during the fixture-yield so mod_repo's internal apt-get update wouldn't trip a dead upstream mirror. But the teardown restored the quarantined entries FIRST and THEN called aptpkg.refresh_db() -- which re-hit the same 404 (apt.releases.hashicorp.com bullseye) and raised CommandExecutionError during teardown, making test_mod_repo report ERROR even though the test itself passed. Two changes: - Run the teardown refresh_db() while the quarantine is still in place, then restore. - Wrap it in try/except CommandExecutionError so first-party feed hiccups can't red the job either -- best-effort refresh, the test is done.
Downgrade jobs (--use-prev-version) run the test suite against the previously-released 3006.x version. That version predates the _resolve_extras_dir hook added to salt/scripts.py in this PR, so its salt-pip still hard-codes <relenv_root>/extras-<py> and ignores SALT_EXTRAS_DIR. The runtime contract this test verifies can't hold on the old code -- skip cleanly instead of masking as a real regression. Same install_salt.use_prev_version pattern already used by tests/pytests/pkg/integration/test_pip_urllib3_patch.py. Failing uniformly across every Linux Test Package downgrade job (AL2023, Debian 11-13, Photon 4/5, Rocky 8/9, Ubuntu 22.04/24.04).
test_upgrade_migration_moves_legacy_extras failed on Photon 4/5 install jobs while passing on every other RPM distro (Rocky 8/9, AL2023). The migration guard required [ -d "$SALT_EXTRAS_DIR" ] but that dir was only created inside the if [ $1 -gt 1 ]; else branch. When RPM reports $1 > 1 on what looks-like-an-install to us (e.g. a purge that leaves rpmdb metadata behind, which the fixture's _purge_salt_packages hits on Photon's tdnf shim), the install-time mkdir doesn't run, the migration guard fails on the -d check, and the legacy marker stays put. Fix: move install -d -o $SALT_USER -g $SALT_GROUP $SALT_EXTRAS_DIR inside the migration block itself, guarded on SALT_ONEDIR_HARDEN=1 + non-empty legacy source. Idempotent: if the dir already exists this is a no-op. Empty-target precondition still guards the mv so a re-run doesn't clobber existing per-daemon content. Applied to all 4 daemon posttrans blocks (cloud/master/syndic/api) which all had the same shape.
Two Photon-tdnf-specific bugs remained after round-4's migration-block fix: 1. test_upgrade_migration_moves_legacy_extras (Photon 4/5 install): The destructive-cycle fixture calls ``yum remove`` between installs. On Photon, ``yum`` is a tdnf shim that sometimes leaves rpmdb metadata behind for removed packages. The next install then reports ``$1 > 1`` in %posttrans scriptlets even though the fixture treats it as a fresh install. Round-4 patched the migration block to create ``$SALT_EXTRAS_DIR`` unconditionally, but the fresh-install ``chown -R salt /opt/saltstack/salt`` and per-daemon ``install -d`` still live in the ``if [ $1 -gt 1 ]; then :; else ... ; fi`` gate and never fire on Photon. Fix at the fixture: after ``yum remove``, run ``rpm -e --nodeps --allmatches --noscripts`` to force-drop the rpmdb entry so the next install sees ``$1 == 1``. 2. test_hardening_unset_matches_legacy_default (Photon 5 Arm64 upgrade): after an RPM upgrade on Photon, /opt/saltstack/salt gets reset to root ownership even though the previous package version had chowned it to salt. Rocky/AL2023/Debian/Ubuntu upgrades preserve the tree ownership; only Photon resets. The %posttrans ``$1 > 1`` gate then skips the corrective chown. Add a post-branch healer in %posttrans cloud/master/syndic/api/minion that re-chowns /opt/saltstack/salt back to $SALT_USER:$SALT_GROUP if it's currently root-owned AND SALT_ONEDIR_HARDEN != 1. Only fires when the tree is root-owned, so operator overrides to any non-root user are preserved. Idempotent no-op on distros that preserve ownership. Both fixes are additive; no existing branch semantics change. The healer is a corrective on top of the ``$1 > 1`` preserve-ownership policy, gated to only heal an accidental root-reset in the legacy layout.
The two-package npm install (``pm2@5.1.0`` + ``grunt@1.5.3`` from the public npm registry) occasionally exceeds pytest's default 90-second timeout under CI network jitter. Observed on Rocky Linux 9 functional zeromq split 3 in the run for PR saltstack#70208 while the same test passed green on the 3006.x nightly baseline the day before -- a flake, not a regression from this PR. The sibling ``test_removed_installed_cycle`` in the same file already carries ``@pytest.mark.timeout_unless_on_windows(120)`` for the exact same reason (single-package install of pm2). Give the two-package case 180s to leave headroom for the extra network round-trip. This hardens a legitimately-slow test against transient network jitter; it does not skip or paper over any real bug in either the NPM state module or this PR's onedir-harden changes (the test doesn't exercise either).
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
2 participants
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.
Summary
Sibling of #70199 (master) for 3006.x. Per issue #70198's per-branch rollout,
SALT_ONEDIR_HARDENdefault = unset on 3006.x (legacy blanket-chown of/opt/saltstack/salt/preserved). Operators opt in explicitly viaSALT_ONEDIR_HARDEN=1in/etc/default/salt-setupor/etc/sysconfig/salt-<daemon>-setupto get the hardened layout early on LTS.When
SALT_ONEDIR_HARDEN=1:SALT_HOMEdefault →/var/lib/salt/<daemon>/homeSALT_EXTRAS_DIRdefault →/var/lib/salt/<daemon>/extras-<py>/opt/saltstack/salt/staysroot:root 0755.SALT_ONEDIR_HARDEN=1, populated legacy/opt/saltstack/salt/extras-<py>/migrates to/var/lib/salt/<daemon>/extras-<py>/.Existing
SALT_HOME/SALT_EXTRAS_DIRoverrides continue to work.Default flips to
SALT_ONEDIR_HARDEN=1on 3009.0 (master, #70199).Fixes #70198 for 3006.x.
Test plan
test:fulllabel applied).