Repository navigation
Conversation
Over ssh the server side of a transfer leaves no per-file record unless the client asks for one with --log-file, so an rrsync admin has no reliable way to know what a restricted key uploaded, downloaded or deleted. The new -log FILE option passes --log-file=FILE to the server rsync, which writes its default "%i %n%L" line for every file, plus the start and summary lines, with a timestamp and pid. FILE is made absolute like DIR, since rsync opens it after the chdir into DIR. The client's own --log-file is refused while -log is in force, the same way -ro refuses it. Add rrsync-log_test.py, which checks the per-file lines of a push and the refusal of a client --log-file only when -log is given.
The client can plant symlinks inside DIR, and rsync opens the log by name, so a log there could be redirected to any file the user can write. Refuse a FILE whose path contains a symlink, or whose existing ancestors include DIR, compared by device and inode so that a bind mount of DIR is caught as well. Extend rrsync-log_test.py with a FILE inside DIR, one behind a symlink that leads through it, and one in a bind mount of it (where unshare can create a user namespace).
Symlinks outside DIR, such as /var -> /private/var on macOS, are fine: rrsync resolves FILE, checks that DIR is not among the resolved ancestors and hands rsync the resolved path. With DIR / no FILE is outside it, so -log is refused there.
steadytao
left a comment
steadytao
left a comment
There was a problem hiding this comment.
Overall -log still is not a reliable audit record -- rsync continues after failing to open --log-file and later write failures are not propagated. Either make the option explicitly best-effort in the documentation and PR description or add fail-closed logging semantics. The PR description also still says paths inside DIR are allowed.
rrsync now follows FILE one component at a time and refuses it if any step enters DIR, since from there the client decides where the path leads even when the final path is outside DIR. It also opens FILE before running rsync, so a log that cannot be written stops the transfer, and hands rsync the open file where it already pins paths. Write errors after that remain rsync's --log-file behaviour, as the man page now says.
rrsync now opens FILE itself before running rsync and refuses the transfer if it cannot, then hands rsync the open file ( |
|
Still has mistakes. I will work on this. |
Which mistakes ? I can fix if you tell me what's wrong |
I appreciate the offer. I only meant that I will take this pass to avoid more back-and-forth over a few connected details. I will share the resulting diff and explain the changes once it is ready. |
Thanks. For context, this modification is crucial for my use case: I use rrsync in a shared hosting infrastructure to allow users to write via rsync into their specific site container directories. The absence of a transfer log has been a major gap - both for me as an administrator lacking an audit trail, and for clients who need to verify whether automated or manual transfers completed successfully. |
|
Fixed path handling around symlinks and |
|
@seks99x would appreciate a second pass here, if you have the time |
|
Oh yeah ofc I have it on my radar. Will look today. |
| } | ||
|
|
||
| /* A log's ownership check must not inherit file-list confinement. */ | ||
| int open_log_file(const char *path, int flags, mode_t mode) |
There was a problem hiding this comment.
Adding enforce_root introduces a third mechanism controlling root confinement alongside files_from (filesfrom_owner_walk_active()) and operator_path_resolve. Having three overlapping flags makes the confinement logic inconsistent and harder to reason about or debug later.
Instead of adding a new parameter to ona_open(), it would be cleaner to handle this inside open_log_file() by temporarily setting confine_root = NULL (or files_from = NULL) as exception only for log-files when used with files-from (filesfrom_owner_walk_active returns 1) , then restoring them right after ( same as operator_path_resolve style) .
Note that zeroing confine_root will cause pseudo-paths (Test 2) to fail, which is actually expected. I was already planning to open a separate PR to allow trusted pseudo-paths under --confine-root when operator_path_resolve == 0 (for --log-file, --files-from, etc.), since rejecting them unconditionally on if (confine_root) is a separate bug we need to fix anyway.
There was a problem hiding this comment.
@steadytao the pseudo-paths fix are covered here #1169
Related to #452
Adds
-log FILEso an administrator can force server-side transfer logging independently of the client. Uploads, downloads and deletions use rsync's existing log format. The client's own--log-fileis refused while this option is active.The log must be a regular file outside the restricted directory. Symlinks are resolved before
..and must be owned by root or the user running rrsync. Administrators must also keep hard-link and bind-mount aliases out of the client's reach because those cannot always be detected.rrsync checks that the log can be opened before starting the transfer. Rsync then opens and writes it. Logging remains best-effort, so clients should use rsync's exit status to determine whether a transfer succeeded.
The existing per-connection
rrsync.logis unchanged.