Add log rotation for AZNFS logs and make the log directory configurable

[rajasi3010]

Problem

aznfs.log and the per-mount turbo*.log files are never deleted, so on a long lived machine they grow until they fill the disk, which takes down everything on the box and not just AZNFS. The log location was also hardcoded, so logs could not be moved to another filesystem.

Rotation was always intended. nfsv3mountscript.sh already carried this note:

# We append to the logfile w/o any support for re-opening log file for log rotation. Use copytruncate option in logrotate config.

The config was simply never written. This PR adds it.

Logs produced by AZNFS, and their retention

Log	Path	Before	After
Mount helper + watchdog	<logdir>/aznfs.log	never deleted, unbounded	rotated
Turbo client (per mount)	<logdir>/turbo<mountpoint>.log	never deleted, unbounded	rotated
stunnel (per storage IP)	/etc/stunnel/microsoft/aznfs/nfsv4_fileShare/logs/	deleted on unmount	unchanged
systemd journal	journald	journald limits	unchanged

Only the first two grew unbounded and those are what this PR fixes. stunnel logs are already removed by cleanup_stunnel_files() together with their .conf and .pid, and are deliberately left alone since rotating them could orphan files that the cleanup matches by exact name.

Changes

New src/aznfs.logrotate, a template installed to /opt/microsoft/aznfs/ and expanded into /etc/logrotate.d/aznfs:

size 100M, rotate 7, compress, delaycompress, missingok, notifempty, copytruncate

• copytruncate is required because aznfsclient holds its log open for the life of the mount and cannot reopen it after a rename.
• Rotation is size based with no daily/weekly. These are diagnostic logs, so the retained history should scale with how much is logged rather than with elapsed time. logrotate only runs daily either way, so a time based policy adds no disk safety, it only caps the history at rotate days. Note that size makes logrotate ignore time directives, so the two must not be combined. Azure's own waagent and cloud-init are likewise pure size based.

Configurable log directory. AZNFS_LOGDIR in /opt/microsoft/aznfs/data/config, overridable per invocation with the env variable of the same name, defaulting to the previous location so existing installs are unaffected. Turbo logs follow it and AZNFSC_LOGDIR still takes precedence for them.

The value must be an absolute path built from safe characters. & and | would corrupt the generated config via sed, * and ? would turn the log paths into globs matching unrelated files, and a relative path would depend on the caller's cwd. Those, along with a directory that cannot be created or is not writable, fall back to the default with a warning rather than failing the mount.

The generated config is regenerated only when the log directory changes, keyed off an # AZNFS_LOGDIR: marker, so a locally edited rotation policy survives mounts and package upgrades.

Packaging. The repo has two packaging scripts, package.sh and generate_package.sh, and the Azure build pipeline runs the latter. Both now stage the template, for deb, rpm and tarball as applicable. The config is generated at install time so rotation applies from the first boot, logrotate is added to all three Requires: variants including azurelinux, and /etc/logrotate.d/aznfs is removed on uninstall.

Hardening. mount.aznfs is installed setuid root and execs the mount script with the caller's environment, so honouring a caller supplied log directory there would let an unprivileged user make root create and write files at a path of their choosing. It now drops AZNFS_LOGDIR and AZNFSC_LOGDIR alongside the BASH_ENV and LD_PRELOAD it already dropped. The log directory is therefore administrator controlled, through the root owned config file. AZNFSC_LOGDIR predates this PR but is the same class of issue.

Behaviour when the log directory is changed

Only the newly configured directory is rotated. Long running processes resolve their log file once at startup, so the watchdog services have to be restarted. AZNFS prints this on the first mount after the change:

AZNFS log directory changed to '/var/log/aznfs'.
Restart the watchdog services so that they log there too:
    sudo systemctl restart aznfswatchdog aznfswatchdogv4
Logs from before the change remain in '/opt/microsoft/aznfs/data' and are not removed automatically.

Logs left in the old directory are not moved or deleted. Since rotation is size based, a log that stops growing never crosses the threshold again, so those files stay until they are removed by hand. They do not grow, so they are not a risk of filling the disk.

Testing

testing/test_logrotate.sh, 388 sandboxed unit tests that need neither root nor a mount and never touch real system paths. They cover path resolution and validation, config parsing, config generation and idempotency, log directory changes, that every packaging path ships the template, that the setuid mount helper drops the log directory overrides, and the real logrotate behaviour, including copytruncate against a live writer verified by inode, and retention actually deleting aged out rotations. The retention test drives six real size triggered rotations with each generation tagged by a distinct digit, and asserts that only rotate files are kept, that the older content is gone from disk, and that the total size stays bounded.

testing/test_logrotate_e2e.sh, 31 checks against a real Azure Files NFSv4.1 mount on a machine with AZNFS installed. It backs up and restores everything it touches, including the logs, and removes its backup only once the restore has completed cleanly.

Both suites were run against this branch. The unit suite has 388 cases and enforces that floor, so a run that silently loses tests fails rather than passes: as an unprivileged user 380 pass and 8 skip. As root more cases skip, because root ignores the permission bits several of them depend on, for the same 388 total. The E2E ran twice: 29 passed / 0 failed / 4 skipped against a live NFSv4.1 share, and 33 passed / 0 failed / 0 skipped against a live NFSv3 Turbo share, which is what retires the four Turbo skips. They have not been run on main or release/3.0.

The guards were mutation tested, each one was verified to fail when the code it protects is deliberately broken. That is what the suite is worth, rather than the pass count: three checks were found during review to be passing vacuously and were rewritten.

Known gaps

• Turbo log rotation is now tested live. A premium block blob account with NFSv3 enabled was created, mounted with -o vers=3,turbo, and the E2E run against it: the four Turbo assertions passed rather than skipped. The rotation check appends to the real aznfsclient log, rotates it with the live policy, and asserts the inode is unchanged, which is what lets the client keep writing through its open descriptor. What it still does not do is observe aznfsclient itself writing after the truncation: the inode check is the mechanism that permits it, not a direct observation of it. Running against a real Turbo mount also found a bug in the E2E's own mount point guard, which allowed only nfs/nfs4/aznfs and refused fuse.aznfsclient; nothing sandboxed could have caught that.
• The RPM %post scriptlet is now extracted and executed by the unit suite, the same way the deb postinst already was, so a shell error in it fails the tests. rpm's macro expansion has since been checked with a real rpmbuild: a bare %u in a scriptlet body is rewritten when a macro named u is defined, which would make aznfs_safe_logdir reject every directory, so the spec's stat and printf formats are escaped as %% and verified to reach the installed script as %. The deb copy stays unescaped, since nothing collapses %% there. Still not done: no package is built or installed by the tests, so this is verified at the scriptlet level rather than through a real rpm -i.
• Validated on Ubuntu 24.04 (bash 5.2, logrotate 3.21, rpm 4.18.2) and RHEL 10.2 (bash 5.2.26, logrotate 3.22, rpm 4.19.1.1, SELinux enforcing), each as root and as an unprivileged user. On RHEL the %post scriptlet was built into an rpm and aznfs_safe_logdir extracted back out of the package and run: it accepts a safe directory and refuses a world writable one, and the stat/printf formats survive a build with u, G and a all defined as hostile macros. logrotate accepted every directive in the shipped template and rotating under SELinux enforcing produced no new AVC denials. The other supported distros (SLES 15, CentOS, Rocky, Ubuntu 18/20/22) are untested. The log directory rule is permission driven, so its verdict on /var/log legitimately differs by distro: Debian ships it root:syslog 0775 and it is refused as a log directory, RHEL ships it root:root 0755 and it is accepted. A distro shipping /var/log group writable by a group other than root or syslog would refuse the recommended /var/log/aznfs; none of the supported ones is known to, but that has only been confirmed on the two tested.
• The logrotate scriptlet has been installed by a real rpm -i on RHEL 10.2. The functions are copied verbatim out of the spec into a separate probe package, so rpm's macro expansion, its shell, its $1 and its failure handling all apply: the policy is created at install time, is valid to logrotate, has no unsubstituted placeholder, honours a configured directory with size 25M / rotate 4, and a world writable directory neither breaks the install nor gets rotated. What is still not installed is the full package: the systemd units, stunnel directories and the Turbo binary are excluded from the probe so it cannot disturb a real installation, and building them needs the C++ build. dpkg -i has since been run too, on Ubuntu 24.04, with the same probe approach: the deb postinst is built from the verbatim aznfs_safe_logdir and install_logrotate_config, so dpkg's shell, its $1 and its failure handling all apply. The policy is created at install time with no unsubstituted placeholder, honours a configured directory with size 2M / rotate 3, and the log directory is created root:root 0755. What is still untested on either packaging format is replacing a real installation: the probe carries no systemd units, stunnel directories or Turbo binary, and the box used has a live NFS mount on it that an upgrade would disturb.
• Rotation and retention are now demonstrated end to end on two distros, against the policy the product's own ensure_logrotate_config() generates at its shipped size 100M / rotate 7. Nine rotations are driven by genuinely exceeding the threshold with a plain logrotate run, and each generation is tagged with a distinct byte so survivors can be identified by content. Rotations accumulate 1 to 7, stay at 7 through generations 8 and 9, and the survivors are exactly generations 3 to 9 with 1 and 2 gone from disk: deleted, not renamed. copytruncate keeps the inode and a descriptor opened before the rotation stays valid, which is what lets aznfsclient keep writing. Identical results on Ubuntu 24.04 (logrotate 3.21) and RHEL 10.2 (logrotate 3.22, gzip 1.13, SELinux enforcing, as root with the policy's su root root intact and no AVC denials). A negative control confirms an undersized log is not rotated, so the threshold really is the trigger, and the retention assertion requires the survivors to be exactly the newest rotate generations rather than merely that the old ones are absent, so an empty directory cannot satisfy it. The README's disk budget of AZNFS_LOGSIZE x (AZNFS_LOGCOUNT + 2) was checked against this: the measured worst case is x (AZNFS_LOGCOUNT + 1), with the extra multiple covering the copytruncate copy, so the documented figure is conservative rather than wrong.
• The distro's own schedule is now exercised rather than assumed. After the real dpkg -i above, rotation is driven by systemctl start logrotate.service, the unit the daily timer activates, which runs /usr/sbin/logrotate /etc/logrotate.conf against the shared /var/lib/logrotate/status. An oversized log rotates through that path, the inode is preserved by copytruncate, the rotation is recorded in the shared state file, and retention caps at rotate across repeated service runs. A small log survives a service run, so the size rule and not the schedule is what fires. What remains unexercised is the timer firing on its own clock: the unit is started explicitly rather than waited for.
• The log directory check exists in four hand-maintained copies, which has produced four separate drift bugs so far. The guard previously compared only the deb and rpm copies: scripts/aznfs_install.sh was outside it entirely, which is why a umask fix had to be applied to three files by hand, and safe_logdir() in common.sh, the copy that decides at mount time, was compared to nothing. All four are now checked and are currently in sync. Both new guards are mutation tested: drifting aznfs_install.sh alone fails, and changing the group allowlist in common.sh alone fails. Install time and runtime disagreeing is how the installer creates a directory the mount then refuses, which is the bug class this catches.
• Review found a privilege escalation in the policy generator and it is fixed here. The staged file was created by redirecting onto the predictable /etc/.aznfs-logrotate.tmp.$$. mount.aznfs.c has no umask call, so the setuid helper runs with the caller's umask: under umask 000 root created that file 0666. An unprivileged caller cannot create files in /etc, but does not need to, only write to the one root just made, before the chmod and mv installed it as a logrotate config that root then executes directives from. It is now staged with mktemp, which is 0600 whatever the umask, in common.sh and both packaging copies. Measured: redirection gives 666 under umask 000 and 644 under umask 022, mktemp gives 600 in both cases.
• The umask fix applied earlier was incomplete: the guarded mkdir got its chmod 0755 but the fallback to the default directory did not, in all three packaging files. A fresh install under umask 0002 that fell back would create a 0775 directory the runtime check then refuses. The original test only matched the guarded mkdir, so it stayed green; it now fails on any mkdir not paired with a chmod. This is the fifth bug from these four copies, and the first the equality-based drift guard could not have caught, because the fix was applied consistently and incompletely.
• Two further hardening changes came out of review. The log directory was created with mkdir -p, which succeeds on an entry that already exists and follows it, so a member of a group that can write the parent could plant a symlink at the target between validation and creation. Parents now use -p and the final component does not, in all four copies, so a planted entry fails with EEXIST. Measured: mkdir -p on a planted symlink succeeds, plain mkdir reports File exists. Separately mount.aznfs.c never reset umask, so every file the script created as root took the caller's; it now calls umask(0022) before execv, asserted by running the real binary under umask 000 and reading back what the exec'd script sees.
• Open question for maintainers. A residual race remains and cannot be closed in shell: once the directory is validated and created, a member of an allowlisted group can still rename it and substitute a symlink before root writes, because the check and the write are separate syscalls and shell has no O_NOFOLLOW. The only complete fix is to stop treating group-writable parents as safe, which would refuse /var/log/aznfs on Debian and Ubuntu where /var/log is root:syslog 0775 — the directory this README recommends. Left as-is pending a decision, rather than changing documented behaviour silently.
• Paths containing spaces are rejected by design. Supporting them would require quoting many pre-existing unquoted usages across the mount scripts, which is out of scope here.
• testing/*.sh are repo only developer tooling, neither packaging script ships them.
• The log directory rule accepts a group writable parent when the group is a system group, because /var/log is root:syslog 0775 and refusing it made the documented AZNFS_LOGDIR=/var/log/aznfs silently fall back. This was verified against a sandbox parent owned by a system group, not against a real /var/log with a live mount.

[Copilot]

Copilot review overview

🟡 Changes recommended

Packaging paths, log-directory fallback handling, and E2E state restoration have unresolved issues.

Review effort: Lite
Findings: 3 · 2 · 1

Open (6)

• Stage logrotate template in all Azure packaging paths · New
• Stage logrotate template before Debian post-install chmod · New
• Stage logrotate template for Azure RPM builds · New
• Preserve configured log directory when runtime override is invalid · New
• Preserve pre-existing alternate log directory during test cleanup · New
• Update rotation-scope comment to match configured log directory · New

What changed in this PR

Adds configurable AZNFS/Turbo log directories and size-based log rotation, with packaging, documentation, and test coverage.

Changes:

• Validates AZNFS_LOGDIR with fallback behavior.
• Generates compressed, copy-truncated rotation rules.
• Updates installers, packages, documentation, and unit/E2E tests.

File	Description
testing/​test_logrotate.sh	Sandboxed unit and rotation tests
testing/​test_logrotate_e2e.sh	Live-mount end-to-end rotation tests
src/​nfsv3mountscript.sh	Configurable Turbo log directory
src/​aznfs.logrotate	Logrotate policy template
scripts/​aznfs_install.sh	Installer log-path handling
README.md	Logging and rotation documentation
packaging/​aznfs/​RPM/​aznfs.spec	RPM packaging and rotation setup
packaging/​aznfs/​DEBIAN/​postrm	Rotation configuration cleanup
packaging/​aznfs/​DEBIAN/​postinst	Debian rotation setup
packaging/​aznfs/​DEBIAN/​control	Logrotate dependency
package.sh	Template inclusion in packages
lib/​common.sh	Log-path resolution and rotation generation

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical setuid-controlled path write risk and several packaging/configuration defects remain unresolved.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Prevent setuid path from honoring unsafe log directory overrides · New
• Preserve valid AZNFS_LOGDIR overrides when config is invalid · New

Resolved since last review (6)

• Stage logrotate template for Azure RPM builds
• Stage logrotate template before Debian post-install chmod
• Stage logrotate template in all Azure packaging paths
• Preserve pre-existing alternate log directory during test cleanup
• Preserve configured log directory when runtime override is invalid
• Update rotation-scope comment to match configured log directory

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate issues affect cleanup safety, configuration generation, path validation, and test portability.

Review effort: Lite
Findings: 1 · 2

Open (3)

• Detect dangling symlinks before removing alternate log paths · New
• Render logrotate config atomically to prevent truncation · New
• Render logrotate config atomically to prevent truncation · New

Resolved since last review (2)

• Prevent setuid path from honoring unsafe log directory overrides
• Preserve valid AZNFS_LOGDIR overrides when config is invalid

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate issues remain in runtime validation, failure handling, and E2E test safety.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Make logrotate setup failures non-fatal with set -e enabled · New
• Use private temporary files in the root-run E2E test · New
• Prevent inherited bad_logdir from triggering false fallback · New

Resolved since last review (3)

• Detect dangling symlinks before removing alternate log paths
• Render logrotate config atomically to prevent truncation
• Render logrotate config atomically to prevent truncation

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved moderate test coverage and cleanup issues remain.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Clean up temporary directories on early exits · New
• Correctly describe delayed compression of newest rotation · New

Resolved since last review (3)

• Use private temporary files in the root-run E2E test
• Make logrotate setup failures non-fatal with set -e enabled
• Prevent inherited bad_logdir from triggering false fallback

[Copilot]

Copilot review overview

🟡 Changes recommended

Address the stale Turbo log policy and the three E2E cleanup, interruption, and permission-restoration issues.

Review effort: Lite
Findings: 1

Open (1)

• Old Turbo client logs lose coverage after directory changes · New

Resolved since last review (2)

• Clean up temporary directories on early exits
• Correctly describe delayed compression of newest rotation

[Copilot]

Copilot review overview

🟡 Changes recommended

Fix the invalid configured-directory fallback and make --repair-logs preserve legitimate rotated logs.

Review effort: Lite
Findings: 1

Open (1)

• Repair mode deletes valid rotated logs · New

Resolved since last review (1)

• Old Turbo client logs lose coverage after directory changes

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical E2E safety gaps and inconsistent regular-file validation remain unresolved.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Prevent root E2E tests from modifying live installation · New
• Validate missing log directories before restore · New
• Restore state after partial deployment failure · New

Resolved since last review (6)

• Symlink race in deployment directory creation
• mkdir -p follows attacker-inserted symlinks
• RPM scriptlet uses Bash syntax without Bash interpreter
• mkdir -p follows attacker-inserted symlinks
• Symlink race in recursive log directory creation
• Unchecked deployment failures mark partial deployment complete

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved validation, tarball installation, and test-suite issues remain, including a critical self-check failure.

Review effort: Lite
Findings: 1

Open (1)

• Fix mktemp self-check pattern matching · New

Resolved since last review (3)

• Validate missing log directories before restore
• Prevent root E2E tests from modifying live installation
• Restore state after partial deployment failure

[Copilot]

Copilot review overview

🟡 Changes recommended

The E2E alternate-path test is not exercising the intended behavior, and valid placeholder-containing paths can generate incorrect rotation policies.

Review effort: Lite
Findings: 1

Open (1)

• E2E alternate log directory under /tmp is rejected by safe_logdir · New

Resolved since last review (1)

• Fix mktemp self-check pattern matching

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved issues remain in tarball installation, directory permissions, CI coverage, and E2E validation.

Review effort: Lite
Findings: 2

Open (2)

• Document all configuration changes that regenerate the policy · New
• Document all policy regeneration marker inputs · New

Resolved since last review (1)

• E2E alternate log directory under /tmp is rejected by safe_logdir

[Copilot]

Copilot review overview

🟡 Changes recommended

Address dangling symlink handling and generate the rotation policy for tarball installations.

Review effort: Lite
Findings: 1

Open (1)

• Render logrotate policy during tarball installation · New

Resolved since last review (2)

• Document all policy regeneration marker inputs
• Document all configuration changes that regenerate the policy

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved log-directory fallback, packaging error handling, and E2E reliability issues remain.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Render logrotate policy during tarball installation

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical log-directory trust validation issues and additional test and cleanup defects remain unresolved.

Review effort: Lite
Findings: 4 · 1

Open (5)

• Use an explicit trusted-group policy for symlink protection · New
• Use an explicit trusted-group policy for symlink protection · New
• Use an explicit trusted-group policy for symlink protection · New
• Use an explicit trusted-group policy for symlink protection · New
• Separate write_config and assertion commands · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

Fix the skipped sticky-parent assertion and stop watchdog services before restoring E2E logs.

Review effort: Lite
Findings: 4 · 1

Open (5)

• Use an explicit trusted-group policy for symlink protection
• Use an explicit trusted-group policy for symlink protection
• Use an explicit trusted-group policy for symlink protection
• Use an explicit trusted-group policy for symlink protection
• Separate write_config and assertion commands

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical path-safety races and log-rotation generation and fallback issues block approval.

Review effort: Lite
Findings: 4

Open (4)

• Group-writable ancestors enable log path replacement race · New
• Trusted group-writable ancestors enable symlink race · New
• Trusted group-writable ancestors enable symlink race · New
• User-controlled HOME enables root E2E scratch path attack · New

Resolved since last review (5)

• Use an explicit trusted-group policy for symlink protection
• Use an explicit trusted-group policy for symlink protection
• Use an explicit trusted-group policy for symlink protection
• Use an explicit trusted-group policy for symlink protection
• Separate write_config and assertion commands

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical log-symlink safety findings and fallback-handling issues must be addressed before approval.

Review effort: Lite
Findings: 4

Open (4)

• Reject symlinked log paths before writability checks · New
• Reject symlinked log paths in RPM install checks · New
• Reject symlinked log paths in DEB install checks · New
• Reject symlinked log files before accepting log directory · New

Resolved since last review (4)

• User-controlled HOME enables root E2E scratch path attack
• Trusted group-writable ancestors enable symlink race
• Trusted group-writable ancestors enable symlink race
• Group-writable ancestors enable log path replacement race

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical validation and RPM scriptlet issues remain, along with moderate runtime, installer, Turbo, and E2E findings.

Review effort: Lite
Findings: 5 · 3

Open (8)

• Prevent repeated-slash log paths from hanging validation · New
• Prevent repeated-slash paths from hanging package configuration · New
• Escape RPM format percent signs in scriptlets · New
• Prevent repeated-slash paths from hanging installation · New
• Check dangling symlinks before touching installer log paths · New
• Reject directory targets in generated logrotate policy · New
• Reject directory log targets in RPM policy generation · New
• Validate the computed Turbo logfile before mounting · New

Resolved since last review (4)

• Reject symlinked log files before accepting log directory
• Reject symlinked log paths in DEB install checks
• Reject symlinked log paths in RPM install checks
• Reject symlinked log paths before writability checks

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical fallback and normalized-path validation issues could still permit unsafe symlink-following log writes.

Review effort: Lite
Findings: 5

Open (5)

• Validate fallback log path before use · New
• Revalidate default log before installer touches it · New
• Normalize log directory before symlink validation · New
• Validate fallback per-mount log before assignment · New
• Escape RPM format percent signs in scriptlets

Resolved since last review (7)

• Check dangling symlinks before touching installer log paths
• Prevent repeated-slash paths from hanging installation
• Prevent repeated-slash paths from hanging package configuration
• Prevent repeated-slash log paths from hanging validation
• Validate the computed Turbo logfile before mounting
• Reject directory log targets in RPM policy generation
• Reject directory targets in generated logrotate policy

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved log-directory safety and rotation fallback issues remain, along with a test-count documentation mismatch.

Review effort: Lite
Findings: 4 · 1

Open (5)

• Normalize log directory before symlink validation
• Revalidate default log before installer touches it
• Validate fallback log path before use
• Escape RPM format percent signs in scriptlets
• Invalid logdir fallback bypasses configured log rotation · New

Resolved since last review (1)

• Validate fallback per-mount log before assignment

[Copilot]

Copilot review overview

🟡 Changes recommended

Fix the configured-directory fallback mismatch and the undefined test assertion helper.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Escape RPM format percent signs in scriptlets
• Define missing assert_rc helper in test script · New

Resolved since last review (4)

• Normalize log directory before symlink validation
• Revalidate default log before installer touches it
• Validate fallback log path before use
• Invalid logdir fallback bypasses configured log rotation

[Copilot]

Copilot review overview

🔵 Needs a closer look

Fix fallback rotation state and warn when installer settings are rejected.

Review effort: Lite
Findings: 1

Open (1)

• Escape RPM format percent signs in scriptlets

Resolved since last review (1)

• Define missing assert_rc helper in test script

[Copilot]

Copilot review overview

🔵 Needs a closer look

Two moderate security and installation issues, plus one documentation nit, remain unresolved.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Escape RPM format percent signs in scriptlets

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical E2E mount-target safety and moderate validation/cleanup issues remain unresolved.

Review effort: Lite
Findings: 1

Open (1)

• Prevent unmounting dangerous or unrelated mount points · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved test-validity defects and a critical E2E log-directory validation gap remain.

Review effort: Lite
Findings: 1 · 1 · 1

Open (3)

• Default log path bypasses safety validation · New
• Test count floor exceeds actual executed checks · New
• Document logging-related mount failure exception · New

Resolved since last review (1)

• Prevent unmounting dangerous or unrelated mount points

[Copilot]

Copilot review overview

🔵 Needs a closer look

Moderate fallback-path safety issues and broken test coverage remain unresolved.

Review effort: Lite
Findings: None

Resolved since last review (3)

• Default log path bypasses safety validation
• Test count floor exceeds actual executed checks
• Document logging-related mount failure exception

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical root-equivalent log-path validation bypasses and additional E2E safety and coverage issues remain unresolved.

Review effort: Lite
Findings: 4 · 2

Open (6)

• Reject root-equivalent log paths with dot components · New
• Package validation accepts root-equivalent log paths · New
• RPM validation accepts root-equivalent root log paths · New
• Installer path check accepts canonical root-equivalent paths · New
• Clean up backups when deployment never begins · New
• Use the active log directory for Turbo log checks · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect test reliability, path safety, and log cleanup safety.

Review effort: Lite
Findings: 2 · 2

Open (4)

• Correct test count threshold · New
• Avoid deleting unrelated concurrently created logs · New
• Reject dot components in mount paths · New
• Use the active log directory for Turbo log checks

Resolved since last review (5)

• Installer path check accepts canonical root-equivalent paths
• RPM validation accepts root-equivalent root log paths
• Package validation accepts root-equivalent log paths
• Reject root-equivalent log paths with dot components
• Clean up backups when deployment never begins

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect test validity, directory creation, and log-directory security.

Review effort: Lite
Findings: 1 · 3

Open (4)

• Count skipped logrotate assertions to avoid false failures · New
• Create configured log directories with mode 0755 · New
• Create configured log directories with mode 0755 · New
• Create configured log directories with mode 0755 · New

Resolved since last review (4)

• Avoid deleting unrelated concurrently created logs
• Correct test count threshold
• Reject dot components in mount paths
• Use the active log directory for Turbo log checks

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved path-permission, tarball installation-order, CI coverage, and E2E validation issues remain.

Review effort: Lite
Findings: 1

Open (1)

• Documented test count conflicts with enforced minimum · New

Resolved since last review (4)

• Count skipped logrotate assertions to avoid false failures
• Create configured log directories with mode 0755
• Create configured log directories with mode 0755
• Create configured log directories with mode 0755

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 3 · 2

Open (6)

• The mount-point validation guards ./.. and a denylist, but it does not reject symlinks in the… · New
• When falling back to the default log directory, the installer creates it with mkdir -p but does… · New
• $mount_point is expanded unquoted inside echo, which can lead to word-splitting and glob… · New
• nok() only prints a single optional detail argument ($2), but several call sites pass 3… · New
• This comment line is duplicated inline, which looks accidental and makes the test harder to read.… · New
• Documented test count conflicts with enforced minimum

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate review findings remain, including unsafe temporary-file handling and configuration/test correctness issues.

Review effort: Lite
Findings: 3 · 4 · 1

Open (8)

• Caller umask enables tampering with privileged temporary config · New
• sudo HOME allows unprivileged sandbox replacement · New
• The mount-point validation guards ./.. and a denylist, but it does not reject symlinks in the…
• Forced rotation fails to verify size-based logrotate selection · New
• nok() only prints a single optional detail argument ($2), but several call sites pass 3…
• $mount_point is expanded unquoted inside echo, which can lead to word-splitting and glob…
• When falling back to the default log directory, the installer creates it with mkdir -p but does…
• This comment line is duplicated inline, which looks accidental and makes the test harder to read.…

Resolved since last review (1)

• Documented test count conflicts with enforced minimum

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved privileged log-directory symlink races and rotation tests that can report false positives block approval.

Review effort: Lite
Findings: 2 · 2

Open (4)

• Prevent setuid symlink race in install-time path validation · New
• Handle whitespace safely in mount-point component checks · New
• Avoid stale rotation files masking logrotate failure · New
• Prevent stale Turbo rotation files causing false-positive tests · New

Resolved since last review (8)

• sudo HOME allows unprivileged sandbox replacement
• Caller umask enables tampering with privileged temporary config
• The mount-point validation guards ./.. and a denylist, but it does not reject symlinks in the…
• Forced rotation fails to verify size-based logrotate selection
• nok() only prints a single optional detail argument ($2), but several call sites pass 3…
• $mount_point is expanded unquoted inside echo, which can lead to word-splitting and glob…
• When falling back to the default log directory, the installer creates it with mkdir -p but does…
• This comment line is duplicated inline, which looks accidental and makes the test harder to read.…

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved path-safety, installer error-handling, and E2E test-isolation issues remain.

Review effort: Lite
Findings: 1

Open (1)

• Mount-point check allows component replacement before destructive operations · New

Resolved since last review (4)

• Handle whitespace safely in mount-point component checks
• Prevent setuid symlink race in install-time path validation
• Prevent stale Turbo rotation files causing false-positive tests
• Avoid stale rotation files masking logrotate failure