ANC installations relocatable

[ashutom]

This PR resolves the issue #283.
This PR Make ANC installations relocatable and add support for ANC's new 1.5.0+ packaging layout

Technical Details :

1. Relocatable tar installs (ANC_INSTALL_PATH)

• New optional ANC_INSTALL_PATH config tag lets tar releases extract to a user-chosen prefix; blank/absent falls back to the legacy /opt/amdtools. Supports {home}/~ expansion to the runner user's home.
• Applies to tar releases only — deb/rpm packages carry predefined absolute locations in their archives and ignore the key.
• resolve_anc_install_prefix / resolve_anc_paths thread the per-install prefix through version check, install verification, group run, and readiness.
• Relocated tar installs rewrite content YAML exe_path entries from /opt/amdtools to the chosen prefix, leaving relative/system paths intact.
• Auto-sudo: extraction skips sudo when the target prefix is user-writable.

2. 1.5.0+ direct packaging support

• detect_package_flavour now returns (pkg_type, is_direct). Legacy token forms (-- / -.) are matched before the direct extension fallback so a legacy tar is never mistaken for a direct
  one.
• Direct installers: deb (dpkg -i) / rpm (dnf install) download-and-install; tar untars straight into the relocatable prefix and rewrites exe_path.
• exe_path validation is shared between legacy and direct tar installers.

3. Centralized install-path resolution

• resolve_anc_install_location probes the node for anc.py by URL flavour, logs and caches the resolved path for the session, and fails clearly when absent. ensure_anc_ready populates it; run_anc_groups
  builds its anc.py -g command from the cached, node-verified path.

4. Fail-fast config guard

• check_version_matches_url aborts the run before contacting any node when anc_version disagrees with the version parsed from anc_release_url (both packaging generations). Wired into
  cvs/tests/anc/conftest.py config validation.

All the UTs are passing with this change.

[solaiys]

Line-specific notes on the one blocking issue are inline below; a plain-language summary is in the PR comment.

[solaiys]

Review summary: one blocking issue

The relocatable-install work reads well overall. In particular I checked and am happy with: the legacy -<flavour>- token being matched before the direct extension fallback (so a legacy ...-tar-linux-x64.tar.gz is never mistaken for a direct tar), the fail-fast version guard being wired into ANC config validation so a version/URL mismatch aborts before any node is contacted, and the new session-scoped install-path cache, which introduces no concurrency risk (single writer, no new threads or background jobs).

The one blocking issue: the new ANC_INSTALL_PATH config value is interpolated into remote shell commands without ever passing the _assert_shell_safe guard.

In plain terms: anc_release_url gets validated at that boundary, but ANC_INSTALL_PATH does not, and resolve_anc_install_prefix only applies expanduser/abspath — neither of which strips shell metacharacters. So whatever the user types into that config key is handed to the shell verbatim on every node, in commands that mostly run under sudo.

Two concrete consequences:

• A path containing a quote (e.g. /home/o'brien/anc) breaks the command's quoting and fails across the whole cluster with a confusing remote parse error, instead of the clear fail-fast message the guard was written to produce.
• One sink is double-quoted rather than single-quoted, so it misbehaves without any quote character at all: a $(...) there is command-substituted, and a stray $VAR expands to empty — which means a rm -rf can silently delete the wrong path.

The fix is small, but needs both halves:

1. Add the key to the guard: _assert_shell_safe(anc_cfg, ("anc_release_url", ANC_INSTALL_PATH_KEY)).
2. Harden it, since the guard currently rejects only ' — also reject $, backtick, backslash and newline, or validate the resolved prefix against a safe-path allowlist. Single-quoting the rm -rf argument closes that specific sink.

I've left inline comments on the exact lines (the guard, the prefix resolution, and the three interpolation sites, including the duplicated one in the direct-tar installer) so the line references are attached to the code rather than listed here. Nothing else is blocking.

[solaiys]

one more IMPORTANT change is needed in the full ANC code:
There are places you have used /tmp/ locations and paths to create folders and files.
It seems I missed this in the initial PR#269.

Please change it to /tmp/{user-id}/ ( if it is in config files , {user-id} will be auto-resolved)
Else, inside the code, you can change the path to be /tmp/current-user-id/ ( current user-id shuold be resolved dynamically) .

Reason:
when another user run the same ANC test on the same node, if those /tmp/* folders or files exists under your userid ownership, it will throw the permission error for the other user.

[ashutom]

one more IMPORTANT change is needed in the full ANC code: There are places you have used /tmp/ locations and paths to create folders and files. It seems I missed this in the initial PR#269.

Please change it to /tmp/{user-id}/ ( if it is in config files , {user-id} will be auto-resolved) Else, inside the code, you can change the path to be /tmp/current-user-id/ ( current user-id shuold be resolved dynamically) .

Reason: when another user run the same ANC test on the same node, if those /tmp/* folders or files exists under your userid ownership, it will throw the permission error for the other user.

Fixed.
All remote temp paths are now namespaced under /tmp/ (username from the cluster file): the validate_exe_paths.py upload, the log tarball in _pull_log_dir, and the quiet-mode run-output file — each with a mkdir -p first. So a second user running ANC on the same node no longer collides with another user's /tmp/* ownership. I used the SSH username rather than the {user-id} config token since these paths are built in code, not config, but the effect is the same per-user isolation you described.

[ashutom]

Review summary: one blocking issue

The relocatable-install work reads well overall. In particular I checked and am happy with: the legacy -<flavour>- token being matched before the direct extension fallback (so a legacy ...-tar-linux-x64.tar.gz is never mistaken for a direct tar), the fail-fast version guard being wired into ANC config validation so a version/URL mismatch aborts before any node is contacted, and the new session-scoped install-path cache, which introduces no concurrency risk (single writer, no new threads or background jobs).

The one blocking issue: the new ANC_INSTALL_PATH config value is interpolated into remote shell commands without ever passing the _assert_shell_safe guard.

In plain terms: anc_release_url gets validated at that boundary, but ANC_INSTALL_PATH does not, and resolve_anc_install_prefix only applies expanduser/abspath — neither of which strips shell metacharacters. So whatever the user types into that config key is handed to the shell verbatim on every node, in commands that mostly run under sudo.

Two concrete consequences:

* A path containing a quote (e.g. `/home/o'brien/anc`) breaks the command's quoting and fails across the whole cluster with a confusing remote parse error, instead of the clear fail-fast message the guard was written to produce.

* One sink is **double-quoted** rather than single-quoted, so it misbehaves without any quote character at all: a `$(...)` there is command-substituted, and a stray `$VAR` expands to empty — which means a `rm -rf` can silently delete the **wrong** path.

The fix is small, but needs both halves:

1. Add the key to the guard: `_assert_shell_safe(anc_cfg, ("anc_release_url", ANC_INSTALL_PATH_KEY))`.

2. Harden it, since the guard currently rejects only `'` — also reject `$`, backtick, backslash and newline, or validate the resolved prefix against a safe-path allowlist. Single-quoting the `rm -rf` argument closes that specific sink.

I've left inline comments on the exact lines (the guard, the prefix resolution, and the three interpolation sites, including the duplicated one in the direct-tar installer) so the line references are attached to the code rather than listed here. Nothing else is blocking.

Both halves are in d0335091.

1. ANC_INSTALL_PATH is now passed through _assert_shell_safe;
2. the guard is hardened to reject ', ", `, $, , newline.

Beyond that I closed the related sinks you pointed to inline: single-quoted, the rm -rf prefix in both installers, escaped the sed-replacement metacharacters (\ & #) in the exe_path rewrite, and — found while hardening — validated the cluster username (it reaches sudo chown but never went through the config guard) and namespaced remote /tmp paths per user. All fail-fast checks are consolidated into validate_anc_config so a bad prefix/URL/username aborts cleanly before any node is contacted, rather than as a mid-run traceback.

[solaiys]

Re-review @ d0335091 — the blocking issue is properly fixed

I re-traced ANC_INSTALL_PATH from config parsing to every remote shell sink, and the hardening is more thorough than what I asked for. One small documentation inconsistency this PR introduces is worth a fix before merge; nothing else is blocking.

The shell-injection blocker is closed, on both halves

The guard (anc_lib.py:416) now rejects ', ", backtick, $, \ and newline, so the double-quoted sink that worried me most is no longer reachable with a $(...) or a bare $VAR.

Coverage is better than the two-line fix I suggested. Rather than only adding the key to the install_anc call, validation moved to a single choke point in resolve_anc_install_prefix (:719). I checked that this actually holds: ANC_INSTALL_PATH_KEY is read in exactly one place in the whole file (:717), and every consumer — install, group run, version check — reaches it through resolve_anc_paths. The extra _assert_shell_safe(anc_cfg, ("anc_release_url", ANC_INSTALL_PATH_KEY)) at :869 is then belt-and-braces rather than the load-bearing check. That's the right shape; it can't be bypassed by a future caller that forgets the guard.

The root check is sharper than my suggestion. I proposed rejecting == "/"; the implementation tests not resolved.strip("/") (:726) with a comment explaining that abspath preserves a POSIX-special leading //, so // and //x/.. are also filesystem root. My version would have missed those.

Both rm -rf loops are fixed, legacy (:1275) and direct-tar (:1417), and the fix is layered rather than relying on the guard alone:

        f"echo \"  {prefix}/$name\"; $SUDO rm -rf '{prefix}'/\"$name\"; done; "

The prefix is single-quoted (and ' is now rejected upstream), and $name can't escape the prefix through three independent filters: awk -F/ '{print $1}' keeps only the first path component, grep -vE '^([.]{1,2})?$' drops ./../empty, and a case "$name" in */*|.|..) continue guard repeats the check in the loop. So a malicious or corrupt archive can't redirect the delete.

The sed sinks are escaped, at :1249 and :1396, via _sed_replacement_safe. The escape order is correct — backslash first, then & and the # delimiter — so the added escapes aren't themselves re-escaped.

The two issues you found while hardening are real ones

The cluster username reaching sudo chown without ever passing the config guard was a genuine gap I missed; validate_cluster_username rejecting whitespace as well as the shell set is the right call, since a space would have split into two chown arguments. And consolidating everything into validate_anc_config, wired into the conftest at conftest.py:101, means a bad prefix, URL or username now aborts before any node is contacted instead of surfacing as a mid-run traceback.

My /tmp request is addressed

All three sites are namespaced under /tmp/<user> — the validate_exe_paths.py upload (:1319), the log tarball (:1717), and the quiet-mode run output (:2120) — each with its own mkdir -p first, and each sanitised to a single path component. Using the SSH username instead of the {user-id} config token is fine; these paths are built in code, and the isolation is what I was after. The log-tarball case is handled correctly too: the archive is created by root inside the user-owned directory and then chowned back so SFTP can pull it.

Checked and clear

• The <changeme> migration doesn't regress the placeholder check. This one needed care, because the removed find_replace_me_placeholders deliberately skipped _comment* keys and required an exact match, whereas the standard resolver (utils_lib.py:293) does a substring match and skips nothing — so a comment merely mentioning the placeholder would hard-exit a correctly-filled config. I walked the shipped config: exactly two strings contain <changeme> (anc.anc_release_url, anc.log_folder_path, both intended), and the two comments that discuss it deliberately write "changeme" without the angle brackets. No false positives.
• Tests. 86 new tests, all passing, and they cover the security fixes specifically rather than just the happy path (TestTarCleanupFiltersDotDot, TestValidateClusterUsername, the shell-safe cases). Full suite goes 484 → 570 with no new failures.
• Lint. ruff format --check and ruff check clean on all three Python files, pylint 10.00/10 on the logging gate.
• Concurrency, re-checked after the refactor: still a single writer to the module-level cache, no threads or background jobs introduced.

One thing to fix before merge (docs, not code)

This PR adds three README lines that tell users to write REPLACE_ME, while the same PR deletes the code that recognised it:

        "anc_release_url": "REPLACE_ME",
        "log_folder_path": "REPLACE_ME",
`anc_release_url` and `log_folder_path` ship as the `REPLACE_ME` sentinel and

The shipped config now uses <changeme>, and REPLACE_ME_SENTINEL is gone, so nothing recognises that string any more. Reading the code paths: a literal REPLACE_ME passes the non-blank check in validate_anc_config, passes _prefix_problem, and then _resolve_prefix returns it as a plain relative path — so an operator who follows the README gets ANC trying to download REPLACE_ME as a URL and writing logs to a relative directory, rather than the clean "replace the placeholder" message the old sentinel produced. It's a three-line docs edit (REPLACE_ME → <changeme> in the example block and the sentence at line 145), but worth doing in this PR since this PR is what created the mismatch.

Minor, entirely optional: _assert_shell_safe runs on the raw value before expanduser, so in principle a home directory containing a metacharacter could reintroduce one after validation. Pathological and out of scope for the config-is-trusted threat model this guard operates in — noting it only for completeness.

Aside from the README lines, this looks good to me — the security fix is well-constructed and the tests back it up.