Conversation
…group (#238) ci: bump actions/checkout in the actions-updates group Bumps the actions-updates group with 1 update: [actions/checkout](https://github.com/actions/checkout). Updates `actions/checkout` from 6.0.3 to 7.0.0 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@df4cb1c...9c091bb) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 7.0.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-updates ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…laky test
TestCreateArchiveWithDifferentCompressionTypes/pigz failed intermittently
in CI with "archive/tar: write too long". The compression tests placed the
output archive inside the very tempDir they archived, so filepath.Walk
reached the archiver's own still-growing output file, snapshotted its size
into the tar header, then overran it as the (async, external) compressor
kept appending. The archiver correctly fails closed in that case
("archive incomplete"), but the scenario is self-inflicted and never occurs
in production, where the backup destination is always outside the source.
Route every compression test's output through a new archiveOutputPath
helper that allocates a separate t.TempDir(), eliminating the whole class
of latent flake across all 9 sites (only the pigz table subtest had tripped
CI, but the direct create*Archive tests shared the same pattern).
Verified: 60x all compression tests under heavy CPU load -> 0 failures
(was ~18/40 on pigz alone); full package + -race green; gofmt/vet clean.
#239) * fix(restore): clear chattr +i mount-guard fallback in --cleanup-guards During restore, when a storage mountpoint is offline, ProxSave applies a read-only bind-mount guard; if the bind mount fails it falls back to `chattr +i` on the mountpoint directory so PVE/PBS cannot silently write into the root filesystem. `--cleanup-guards` previously only undid the bind-mount guards: the immutable flag was left set, persists across reboots, and had to be cleared by hand. Record each chattr-guarded mountpoint in a host-side index (/var/lib/proxsave/guards/chattr-targets) and have --cleanup-guards reverse it: - record on a successful `chattr +i` at both apply sites (PBS + PVE), written atomically on the real filesystem, deduped, with strict target validation (datastore mount root only; reject newline/control chars). - clearImmutableGuards reads the index and runs `chattr -i`, but only on targets that are NOT currently mounted (a live mount shadows the flagged dir; clearing it would touch the wrong inode), mirroring how hidden bind-mount guards are left in place. Symlinks are resolved and the allowlist re-checked before clearing so a parent-component symlink cannot escape /mnt|/media|/run/media. - keep the guard directory + index while any immutable target is still pending (mounted/unresolved/failed) so a later run can finish the job; remove them only when nothing is pending and no bind-mount guards remain. - surface the skip-while-mounted case at Info level and point users at the manual recovery (lsattr -d / chattr -i) when the guard dir was deleted. PVE guard apply now uses the existing mountGuardMkdirAll / mountGuardIsPathOnRootFilesystem seams (default behavior unchanged) so the record wiring can be tested without a writable /mnt/pve. Adversarially reviewed (correctness/security/test-rigor), mutation-proven. * docs(restore): document --cleanup-guards clearing the chattr +i fallback Correct the mount-guard docs to match the new cleanup behavior: - distinguish the two guard types: a bind-mount guard is shadowed by a later real mount and discarded on reboot / by --cleanup-guards, whereas the `chattr +i` fallback sets an immutable flag on the directory inode that persists across reboots and is now recorded under /var/lib/proxsave/guards/chattr-targets for cleanup to reverse. - fix the "real mount overlays the guard" wording, which was only true for the bind-mount guard. - explain that --cleanup-guards clears recorded immutable flags only on not-currently-mounted targets, the unmount→cleanup→remount procedure for mounted ones, and the manual lsattr/chattr -i recovery when the guard directory was deleted by hand. - add a TROUBLESHOOTING entry for "mountpoint read-only after restore". Touches RESTORE_GUIDE.md, RESTORE_TECHNICAL.md, CLI_REFERENCE.md, TROUBLESHOOTING.md. * fix(orchestrator): resolve gosec G301/G304 on the chattr guard index Address the two github-advanced-security (gosec) findings on mount_guard_chattr_index.go structurally, without #nosec: - G301: tighten the guard base dir from 0o755 to 0o750. It is root-owned host state (the 0o600 index + bind-mount guard subdirs); nothing non-root needs to traverse it. - G304: read the index through an *os.Root on its directory (root.ReadFile(base)), mirroring readLockFileContent / normalizeTextFile. This confines the read to the guard directory at the syscall level so a symlink or `..` in the basename cannot redirect it, and removes the raw variable sink. Behavior is unchanged (missing/unreadable index -> nil).
The Exit Codes table listed 0-7 with wrong meanings; the code (internal/types/exit_codes.go) defines 0-15. Rewrite the table to match exactly (e.g. 3=environment, 5=storage, 7=permission, 14=security, 15=encryption). Also fix the cloud note: cloud storage is non-critical, so an upload failure does not abort with a storage error (5); it is recorded as a warning and the run exits non-zero (1), not 0.
Cloud upload verification only compared file size, although the docs promised SHA256 checksum validation and the .sha256 sidecar was uploaded but never used. VerifyUpload now escalates from the size pre-check to a real SHA256 comparison: it asks the remote for its hash via 'rclone hashsum sha256' and compares it against the checksum computed fresh from the local file (the exact uploaded bytes -- raw archive, bundle, or sidecar -- never metadata.Checksum, which is the inner-archive hash and would not match a bundle). Behavior is gated by CLOUD_VERIFY_CHECKSUM (default true). On backends without a native SHA256 (S3, B2, Wasabi, ...) it falls back to the size-only verdict, logged at debug (not warning, so a successful run is not turned into exit code 1); CLOUD_VERIFY_DOWNLOAD=true forces a download-and-hash on such backends. A genuine hash mismatch fails the upload; transport errors stay fatal while 'hash type not supported' degrades to the size-only fallback. Adds 'hashsum' to the rclone allowlist and a table-driven test suite covering match/mismatch/no-native-hash/download/transport-error/wrong-file/ malformed-hash, the full Store() path, and a regression lock-in that the no-native-hash fallback emits no WARNING. Docs and the env template updated to match.
The Environment Variables section showed CONFIG_FILE=/path ./build/proxsave, but no such env var is read: the config file location comes only from the -c/--config flag (internal/cli/args.go), and the env-override mechanism (loadEnvOverrides) runs after the file is already loaded, so it cannot select it. Replace the example with the real flag and a note. The neighboring DRY_RUN/DEBUG_LEVEL/USE_COLOR examples are genuine env overrides and stay.
…USER_HOMES toggle BACKUP_USER_HOMES was a ghost toggle: read only in config.go (default true), absent from the env template and all docs, so no real user could set it. User home directories are part of a system backup, so collection is now unconditional. The brickSystemUserHomes brick always calls collectUserHomes (the if c.config.BackupUserHomes guard is gone). The BackupUserHomes field is removed from config.Config and CollectorConfig, along with its copy in the orchestrator and its default in GetDefaultCollectorConfig. Existing exclusions are unchanged: the global blacklist/exclude patterns still apply, and .ssh is still gated by BACKUP_SSH_KEYS. The Validate() 'at least one backup option must be enabled' guard (and its hasCollectionOptionEnabled/collectionOptionFlags helpers) is removed: the system recipe always collects a baseline (kernel/hardware report info plus /home), so the precondition is no longer accurate. Tests: the empty-config assertion is inverted (an empty config is now valid), the user-homes table case is dropped, and TestUserHomesBrickCollectsUncondi- tionally locks in that the brick collects /home with no config gate.
…and docs CLOUD_UPLOAD_MODE fell back to sequential when the key was absent or blank, but the shipped template sets it to parallel and every doc states the default is parallel (with CLOUD_PARALLEL_MAX_JOBS=2). Align the code with the documented/shipped default: unset or blank now selects parallel; only an explicit sequential (or any other non-parallel value) selects sequential. Adds TestCloudUploadModeDefault covering absent/blank/explicit/unknown.
After /home collection became unconditional, the RunGoBackup end-to-end tests backed up the real root filesystem and pulled in the CI runner's /home/<user>/go module cache (~1.9 GiB), making the archive phase exceed the 2-minute timeout (the Race Detector job failed on TestRunGoBackupFallback- Compression). These tests previously avoided /home only because the struct-literal config left the old BackupUserHomes flag at its false zero value. Set SystemRootPrefix to an isolated temp dir in setSmallBackupTestConfig and in the dry-run/bundle RunGoBackup tests so collection walks a controlled root. go test -race -count=1 ./... now passes; the RunGoBackup tests drop from 120s+ to ~2s.
…functions restore.go was split from a ~1000-line monolith into ~30 focused files, but RESTORE_TECHNICAL.md still cited restore.go:NNN with line numbers that no longer exist in the 55-line stub. Replace the 22 stale file:line citations with file + function references (approach B: no line numbers, which rot on every refactor), e.g. extractRegularFile() -> restore_archive_entries.go, unmountEtcPVE() -> restore_services.go, createDecompressionReader() -> restore_decompression.go, the workflow phases -> restore_workflow_ui_*.go. Clarify the 'File: restore.go' section header to note restore.go is the entry stub and the workflow body lives in restore_workflow_ui_run.go. Accurate references (the file table row, the RunRestoreWorkflow entry) are unchanged.
BACKUP_PXAR_FILES was listed under 'Unchanged (same name)' as SAME, but the code reads it as a legacy fallback alias: config.go uses getBoolWithFallback(["PXAR_SCAN_ENABLE", "BACKUP_PXAR_FILES"], true), so PXAR_SCAN_ENABLE is the canonical name and BACKUP_PXAR_FILES is only accepted for backward compatibility. Move it to the Renamed section with the doc's own convention: RENAMED(PXAR_SCAN_ENABLE).
…plate and docs The code defaulted CLOUD_PARALLEL_VERIFICATION to false when the key was absent, but the shipped template sets it to true and every doc states the default is true. Align the code with the documented/shipped default (same pattern as the earlier CLOUD_UPLOAD_MODE fix). Adds TestCloudParallelVerifi- cationDefault (absent->true, explicit false->false, explicit true->true).
--new-install preserves build/, env/ and identity/ (cmd/proxsave/new_install.go newInstallPreservedEntries), but several places said only env/identity, omitting build -- including the shipped --help text. Correct the --help string (internal/cli/args.go) plus README, EXAMPLES, CLI_REFERENCE and TROUBLESHOOTING so every mention names build/env/identity.
CONFIGURATION.md listed the built-in defaults without psimon and as 'various regex patterns', and its example value presented non-default regexes as if they were defaults. The setting is additive (user entries are merged with built-in defaults). List the real defaults from config.go (incl. psimon and the ^kvm-pit/ and ^worker/...-drbd_as_pm- regexes), note the additive behavior, and make the example match the template. Also add psimon to the template summary.
The intro cited reference/env/backup.env as the legacy Bash source, but neither that file nor the reference/ directory exists in this repo (it referred to the old pre-migration project). Reword to describe the legacy Bash origin without a broken path.
…e' for build-from-source proxsave installs as root: the binary lives at /opt/proxsave/build/proxsave and the installer creates the PATH symlink /usr/local/bin/proxsave, so the canonical command for an installed user is just 'proxsave' (matching the tool's own 'Next steps' output). The docs used './build/proxsave' everywhere, which only works from the repo dir. Convert usage examples to 'proxsave', keeping './build/proxsave' only in genuine build-from-source/test sections (INSTALL Building-from-Source + wizard, CLOUD_STORAGE Testing, all of MIGRATION_GUIDE which is build-from-source only). Cron examples keep the absolute /opt/proxsave/build/proxsave path (cron needs an absolute path).
The release-assets list omitted SHA256SUMS.sig, but the install and upgrade flows refuse to proceed without a verifiable release signature (install.sh downloads+verifies it; upgrade.go does the same) and release.yml validates the signing key before publishing. Add it to the asset list with a mandatory note.
INSTALL.md and CLOUD_STORAGE.md stated a v1.50+ minimum while the README badge declares 1.60+. Align the stated minimum to v1.60+ everywhere (the 'Recommended: latest v1.65+' note is unchanged).
go.mod ships with the repo, so 'go mod init github.com/tis24dev/proxsave' fails
('go.mod already exists'). For 'cannot find main module' the fix is to run from
the project root; keep cd + go mod tidy + make build and correct the cause.
…ions claim parseRecipientString accepts ssh-ed25519/ssh-rsa public keys (agessh) for both AGE_RECIPIENT and AGE_RECIPIENT_FILE, but the docs only mentioned X25519 and passphrase recipients. Document SSH recipients. Also fix the overclaim 'Enforces 0700/0600': files are created 0700/0600 and the security check verifies/warns, auto-fixing only when AUTO_FIX_PERMISSIONS is enabled.
…tches) The documented snippet used strings.HasPrefix(target, "/etc/pve") (which would also match /etc/pvexyz) and showed 'return nil'. Replace it with the real logic from restore_archive_entries.go shouldSkipRestoreEntryTarget: early-return when the dest root isn't '/', then block exactly /etc/pve or anything under /etc/pve/, returning true,nil with the real warning text.
RESTORE_DIAGRAMS.md never mentioned the mount-guard subsystem. Add a section + mermaid flow: when a datastore mount is offline during PBS restore, the mount-point (on the root FS) is bind-mounted read-only (fallback chattr +i) to prevent writes from landing on / and creating a ghost datastore; on remount the real storage shadows the guard; --cleanup-guards removes them. References mount_guard.go / mount_guard_apply.go / guards_cleanup.go.
The shipped template (internal/config/templates/backup.env) is the source of truth for defaults, but four code getBool fallbacks (used when a key is absent) diverged from it. Align them so an absent key behaves like the template: ENABLE_DEDUPLICATION, ENABLE_PREFILTER, AUTO_FIX_PERMISSIONS false->true; BACKUP_CEPH_CONFIG true->false. Template-based installs ship all four keys so they are unaffected; only configs that omit a key change. Flip the config_test AUTO_FIX assertion and add TestOptimizationAndSecurityDefaults- MatchTemplate to lock the four defaults to the template.
…y Sections The 10-step 'Key Sections' list under RunRestoreWorkflow cited bare line ranges (e.g. '(Lines 28-66)') that pointed into the old monolithic restore.go and drifted on every edit. Replace each with the actual implementing function(s)+file, and reorder phases 2-4 to match real execution: PlanRestore/splitRestoreCategories (mode+plan build) runs before the cluster SAFE/RECOVERY prompt, and confirmRestorePlan is the final pre-write confirmation. Fix the phase 9 chain to runClusterSafeApply -> runSafeClusterApplyWithUI (the standalone runSafeClusterApply is the unused CLI wrapper). Relabel the 'Extract Engine' ASCII cell from restore.go to restore_archive* (extraction moved out of the stub).
…from RESTORE_TECHNICAL The Module Structure / Execution Flow / Implementation Details sections still described the pre-refactor architecture: ~28 source line-number citations (file.go:NNN and '(Lines NNN)'), several function names that no longer exist or were renamed (AllCategories, ConfirmRestorePlan, GetCategoriesForModeOrCustom, splitExportCategories, SelectAndPrepareBackup, DiscoverBackups, SelectSpecificBackup, DecryptIfNeeded), a phantom file (bash.go), a phantom helper (isSecurePath), the PreparedBackup/CleanupFunc type, and stale signatures (extractArchiveNative, extractSelectiveArchive, createDecompressionReader). Replace every line-number citation with a greppable function+file reference; correct phantom/renamed symbols to their real current names (GetAllCategories, ShowRestoreModeMenuWithReader/ShowCategorySelectionMenuWithReader, ConfirmRestoreOperationWithReader, preparedBundle, prepareRestoreBundleWithUI/ selectBackupCandidateWithUI/discoverBackupCandidates/preparePlainBundleCommon, verifyStagedArchiveIntegrity, standaloneClusterMode); fix the entry point to main_restore_decrypt.go; update stale code-block signatures to the real opts-struct/ctx forms; align the /etc/pve guard snippet to the real exact-or-prefix check; and relabel the path-traversal pseudocode as a simplified illustration of ensureRestoreTargetWithinRoot(). Verified by adversarial refuters against the code until HOLD.
…de allowlist escape) The restore-time PBS/PVE mount guards bind-mounted / chattr +i'd a guard target validated only by a string-prefix allowlist check (isConfirmableDatastoreMountRoot: /mnt, /media, /run/media) on a non-symlink-resolved path. A parent-component symlink (e.g. /mnt/data -> /etc) passed the prefix check, so the guard — and especially the reboot-surviving chattr +i fallback — could land outside the allowlist on a live OS directory. The cleanup path already resolved symlinks and re-checked the allowlist; the two apply paths did not (the divergence that caused this). Add a single shared helper resolveGuardTargetWithinAllowlist() (mount_guard.go) that resolves the target through symlinks (deepest-existing-ancestor walk for a not-yet-created mountpoint leaf) and re-checks the allowlist on the resolved path, and route all three sites through it: PBS apply (before MkdirAll), PVE apply (before MkdirAll), and cleanup (replacing the inline resolve+recheck; the cleanupResolveTarget seam is renamed resolveGuardTarget). On an escape or resolution error the apply paths skip fail-safe and never act outside the allowlist; on success they operate on and record the RESOLVED path so cleanup can match it. The deepest-ancestor resolution before MkdirAll also prevents MkdirAll from materializing directories through a pre-existing symlinked parent. Normal (no-symlink) targets are unchanged. Tests: unit table for the helper plus PBS+PVE integration cases for symlink escape (refused), allowlist->allowlist (acts on/records the resolved path), and generic resolve-error fail-safe. go test ./... and -race green.
… guard visibility The restore-time mount guards used a chattr +i immutable-flag fallback when the read-only bind-mount guard could not be created. That flag survived reboots and became a latent landmine: once the storage was later unmounted it silently re-blocked the mountpoint, and it could only be cleared while unmounted (a real mount on top shadows the inode). The guard's 'auto-restore on remount' was only overlay shadowing, never removal, so the chattr case left a persistent footgun. (c) Downgrade the fallback to WARN-ONLY at both apply sites (mount_guard_apply.go, pve_staged_apply.go): when the bind mount fails, log a loud, actionable warning and proceed without setting any persistent flag. This is safe because proxsave's own directory recreation is already skipped by the storage-mount preflight (shouldSkipUnmountedStorageMount), the config-only restore never extracts into datastore mountpoints, and PVE/PBS services are stopped during the guarded window; only external writers are unblocked in the rare bind-impossible case. (b) Add visibility: warn at restore start about any LEGACY chattr +i flags still recorded (warnLegacyImmutableGuards), and print a summary at the end of --cleanup-guards (bind-unmounted / guards-remaining / immutable-cleared / immutable-pending / guard-dir state). Backward-compat: the legacy-flag cleanup path (clearImmutableGuards + the chattr index) is retained, so hosts upgraded from a chattr-setting version are still remediated by --cleanup-guards. Docs (RESTORE_TECHNICAL/DIAGRAMS/GUIDE, CLI_REFERENCE, TROUBLESHOOTING) reframe chattr as legacy/removed. Tests updated to assert warn-only (no chattr, empty index) plus new reader/warner/summary tests; go test ./... and -race green. Built from 3 constructor analyses and validated by 4 adversarial refuters to HOLD.
The final command summary printed after every run (including --restore) did not mention --cleanup-guards, so an operator finishing a restore that applied offline mount guards had no on-screen pointer to the command that removes them. Add a concise entry right after --restore.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (48)
📝 WalkthroughWalkthroughThe PR replaces the ChangesCI & Release Pipeline
Mount Guards, Cloud Checksum, Collector/Config Changes, and Docs
Sequence Diagram(s)sequenceDiagram
participant Restore as restore_workflow_ui_plan
participant Apply as mount_guard_apply / pve_staged_apply
participant Resolver as resolveGuardTargetWithinAllowlist
participant BindMount as sys.Mount (bind)
participant Index as mount_guard_chattr_index
participant Cleanup as CleanupMountGuards
Restore->>Index: warnLegacyImmutableGuards (list persisted targets)
Restore->>Apply: maybeApplyPBS/PVEMountGuards
Apply->>Resolver: resolve symlinks + allowlist check
alt escape or resolve error
Resolver-->>Apply: ok=false
Apply->>Apply: skip guard (warn)
else ok
Apply->>BindMount: bind-mount read-only
alt bind success
BindMount-->>Apply: guard active
else bind fails
Apply->>Apply: warnOfflineTargetUnguarded (no chattr +i)
end
end
Note over Cleanup: operator runs proxsave --cleanup-guards
Cleanup->>Index: read chattr index
Cleanup->>Resolver: re-validate each entry
loop each target
alt currently mounted
Cleanup->>Cleanup: mark pending
else not mounted
Cleanup->>Cleanup: chattr -i (clear immutable flag)
end
end
Cleanup->>Cleanup: emit summary (cleared, pending, dir-removed)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1131
to
+1140
| // rclone may report that the backend cannot produce the requested hash; | ||
| // treat that as "no hash available" (size-only fallback) rather than a | ||
| // transport error. Require the word "hash" so genuine transport/auth | ||
| // failures (e.g. "tls: unsupported protocol version", "operation not | ||
| // supported in this region") stay fatal instead of silently downgrading. | ||
| msg := strings.ToLower(strings.TrimSpace(string(output))) | ||
| if strings.Contains(msg, "hash") && (strings.Contains(msg, "not supported") || strings.Contains(msg, "unsupported")) { | ||
| return "", false, nil | ||
| } | ||
| return "", false, fmt.Errorf("rclone hashsum failed: %w: %s", err, strings.TrimSpace(string(output))) |
There was a problem hiding this comment.
strings.Fields splits on all whitespace, so a remote path like dir/my archive.tar.zst produces ["<hash>", "dir/my", "archive.tar.zst"]. fields[len(fields)-1] is "archive.tar.zst", but want = path.Base("remote:dir/my archive.tar.zst") = "my archive.tar.zst". The two never match, so checksum verification is silently skipped and verifyRemoteChecksum falls back to the size-only result as if the backend had no native SHA256. The comment acknowledges "paths may contain spaces" but the field-split approach does not handle them. ProxSave's own archive names are machine-generated without spaces, so this is low-risk in practice — but worth noting in case operators configure a custom CLOUD_REMOTE_PATH with spaces.
Comment on lines
+84
to
+120
| // recordImmutableGuardTarget appends target to the immutable-guard index after a | ||
| // successful `chattr +i`, so CleanupMountGuards can later clear exactly the | ||
| // directories ProxSave itself made immutable. Best-effort: any failure is logged | ||
| // and swallowed so a recording problem never aborts the restore. The write is | ||
| // atomic and de-duplicated. | ||
| func recordImmutableGuardTarget(logger *logging.Logger, target string) { | ||
| clean, ok := isRecordableImmutableTarget(target) | ||
| if !ok { | ||
| if logger != nil { | ||
| logger.Warning("Guard chattr index: refusing to record unsafe immutable target %q", target) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // 0o750: the guard base dir is root-owned host state (it holds the 0o600 index | ||
| // and the bind-mount guard subdirs); nothing non-root needs to traverse it. | ||
| if err := os.MkdirAll(mountGuardBaseDir, 0o750); err != nil { | ||
| if logger != nil { | ||
| logger.Warning("Guard chattr index: unable to create %s: %v (cleanup will need a manual chattr -i %s)", mountGuardBaseDir, err, clean) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| indexPath := mountGuardChattrTargetsPath() | ||
| existing := readImmutableGuardIndex(indexPath) | ||
| for _, t := range existing { | ||
| if t == clean { | ||
| return // already recorded; keep the index stable | ||
| } | ||
| } | ||
|
|
||
| payload := strings.Join(append(existing, clean), "\n") + "\n" | ||
| if err := writeImmutableGuardIndex(indexPath, []byte(payload), 0o600); err != nil { | ||
| if logger != nil { | ||
| logger.Warning("Guard chattr index: unable to record %s in %s: %v (cleanup will need a manual chattr -i)", clean, indexPath, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
recordImmutableGuardTarget has no production call site
recordImmutableGuardTarget is called only in tests. Since this PR also removes the chattr +i apply step from both protectOfflineTarget (PBS) and maybeApplyPVEStorageMountGuardsFromStage (PVE), the index will never gain entries in production: warnLegacyImmutableGuards will always be a no-op and clearImmutableGuards will always find an empty/missing index for installs running v0.26.0 or later. Legacy users upgrading from a version that called chattr +i (v0.25.x and earlier) also won't have an index — those older versions predate the recording mechanism — and will still need to clear flags manually with chattr -i.
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.
Automated release PR for
v0.26.0.Greptile Summary
This release (v0.26.0) introduces two main features: SHA256 checksum verification of cloud uploads via
rclone hashsum(with a graceful size-only fallback when the backend lacks native SHA256), and a replacement of thechattr +iguard fallback with a warn-only path that no longer sets persistent immutable flags — plus a new index-backed cleanup mechanism (CleanupMountGuards) that can reverse any such flags set by older versions.VerifyUploadnow runs a two-stage check (size, thenrclone hashsum sha256); controlled byCLOUD_VERIFY_CHECKSUM(default true) andCLOUD_VERIFY_DOWNLOAD(default false for bandwidth control). A "hash not supported" rclone response silently degrades to size-only; genuine transport errors remain fatal.chattr +ibind-mount fallback is removed from both PBS and PVE guard paths; instead a clear warning is emitted.clearImmutableGuardsand a newchattr-targetsindex file provide automated cleanup of flags left by previous versions.resolveGuardTargetWithinAllowlistcloses a symlink-escape vector on both the apply and cleanup paths.ENABLE_DEDUPLICATION,ENABLE_PREFILTER,AUTO_FIX_PERMISSIONS,CLOUD_UPLOAD_MODE,CLOUD_PARALLEL_VERIFICATION,BACKUP_CEPH_CONFIG) are updated to match the shippedbackup.envtemplate;BACKUP_USER_HOMESis removed (user homes are now always collected).Confidence Score: 4/5
Safe to merge; the chattr removal and SHA256 verification are both fail-safe by design.
The two significant behavioral changes are well-contained: removing the chattr fallback only affects a secondary guard path (bind-mount is the primary), and the new SHA256 verification degrades gracefully to size-only rather than failing uploads. Default-value changes are correctly aligned with the shipped template that users already have. The one item worth a second look is
recordImmutableGuardTargetbeing wired to no production call site — the index-based cleanup is effectively a no-op in this release — and the silent size-only degradation inremoteSHA256when a remote filename contains spaces.internal/orchestrator/mount_guard_chattr_index.go (recordImmutableGuardTarget has no production call site) and internal/storage/cloud.go (remoteSHA256 basename matching silently falls back for filenames with spaces)
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant C as Caller participant VU as VerifyUpload participant VP as verifyPrimary/Alt (lsl) participant RC as verifyRemoteChecksum participant RH as remoteSHA256 participant GC as GenerateChecksum C->>VU: VerifyUpload(ctx, localFile, remoteFile) VU->>VP: size check (rclone lsl) VP-->>VU: ok / err alt size check fails VU-->>C: false / err else "CLOUD_VERIFY_CHECKSUM=false" VU-->>C: true, nil (size-only) else "CLOUD_VERIFY_CHECKSUM=true" VU->>RC: verifyRemoteChecksum(ctx, localFile, remoteFile) RC->>RH: "remoteSHA256(ctx, remoteFile, download=false)" RH-->>RC: "hash / ok=false (no native SHA256) / err" alt transport error RC-->>VU: false, err else "no native hash AND CLOUD_VERIFY_DOWNLOAD=true" RC->>RH: "remoteSHA256(ctx, remoteFile, download=true)" RH-->>RC: hash / ok / err end alt still no hash RC-->>VU: true, nil (size-only fallback, logged at Debug) else hash returned RC->>GC: GenerateChecksum(ctx, localFile) GC-->>RC: localHash / err alt hashes match RC-->>VU: true, nil else mismatch RC-->>VU: false, checksum mismatch end end VU-->>C: result end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant C as Caller participant VU as VerifyUpload participant VP as verifyPrimary/Alt (lsl) participant RC as verifyRemoteChecksum participant RH as remoteSHA256 participant GC as GenerateChecksum C->>VU: VerifyUpload(ctx, localFile, remoteFile) VU->>VP: size check (rclone lsl) VP-->>VU: ok / err alt size check fails VU-->>C: false / err else "CLOUD_VERIFY_CHECKSUM=false" VU-->>C: true, nil (size-only) else "CLOUD_VERIFY_CHECKSUM=true" VU->>RC: verifyRemoteChecksum(ctx, localFile, remoteFile) RC->>RH: "remoteSHA256(ctx, remoteFile, download=false)" RH-->>RC: "hash / ok=false (no native SHA256) / err" alt transport error RC-->>VU: false, err else "no native hash AND CLOUD_VERIFY_DOWNLOAD=true" RC->>RH: "remoteSHA256(ctx, remoteFile, download=true)" RH-->>RC: hash / ok / err end alt still no hash RC-->>VU: true, nil (size-only fallback, logged at Debug) else hash returned RC->>GC: GenerateChecksum(ctx, localFile) GC-->>RC: localHash / err alt hashes match RC-->>VU: true, nil else mismatch RC-->>VU: false, checksum mismatch end end VU-->>C: result endReviews (1): Last reviewed commit: "docs(cli): list --cleanup-guards in the ..." | Re-trigger Greptile
Summary by CodeRabbit
New Features
--cleanup-guardscommand to remove leftover restore mount guards.Bug Fixes
/homeis included consistently.Documentation