"Download files from the shared folder" behaving badly?
I was experimenting with this directive and uploaded a hosts file to the share. I then specified that this file should be downloaded to the /tmp directory (screenshot attached). The selected "recursion level of the copy" did not seem to matter, nor did putting a trailing slash after the destination ( as in "/tmp/"). What I got was the /tmp directory being renamed to /tmp_1488367735_Wed_Mar__1_06_28_59_2017.cfsaved and the hosts file being renamed to /tmp. On this server /tmp is not a separate partition.
Is this behavior expected? It seems a bit dangerous.
#3 Updated by Hamlyn Mootoo 7 months ago
While I understand this now, it is very easy to misinterpret, and getting /tmp renamed was not fun. As a suggestion I would change the wording of the this field to something like "Full path of destination file". This would be somewhat symmetrical with the "Path of file to be copied" field. I understand there is some help text when the mouse hovers over the question mark, but if other sys admins are anything like me, when we read "destination" for a file, we think "directory location" not full path, and would not think help is needed. The reason for this is that when copying a file from one place to another, unless you're renaming the file, not many admins would put in the full path for the destination. For example:
cp /etc/hosts /tmp
NOT cp /etc/hosts /tmp/hosts
#5 Updated by Jonathan CLARKE 7 months ago
- Tracker changed from User story to Bug
- Target version changed from 4.1.0 to 3.1.19
- Reproduced set to No
I agree with you 100%, Hamlyn. This is misleading, and you're not the first person to misunderstand this, far from it. Worse than misleading, this could cause data loss (if it wasn't /tmp but /etc, and the .cfsaved backup ended up getting removed... the consequences could be disastrous).
This needs to be reworked. I'm requalifying this as a bug - it needs fixing, at least by clearer wording, and better still by figuring out we probably don't ever want to replace a directory with a single file (for example).
#9 Updated by Alexis MOUSSET 7 months ago
- Category changed from Techniques to Agent
- Assignee set to Alexis MOUSSET
Besides the documentation issue, it seems to be a bug in CFEngine. We do not use move_obstructions in this technique, so we should not replace a link or a directory by a file. Furthermore, the behavior seems different according to the file comparison method.
#11 Updated by Alexis MOUSSET 7 months ago
dir directory exists on the destination:
|file||dir||digest||*||*||Does nothing, returns success or replaces the directory, depending on ctime!|
|file||dir/||digest||*||*||Does nothing, returns success or fails and does not replace the directory, depending on ctime!|
|file||dir||mtime||*||*||Replaces the directory|
|file||dir/||mtime||*||*||Creates dir.cfnew but fails and does no replace the directory|
The issue with checksum is:
verbose: Checksum comparison replaced by ctime: files not regular
verbose: File '/tmp/dir' copy_from '/tmp/file' verbose: Destination file '/tmp/dir' already exists verbose: Image file '/tmp/dir' out of date, should be copy of '/tmp/file' verbose: Copy of regular file succeeded '/tmp/file' to '/tmp/dir.cfnew' verbose: No depth search when copying '/tmp/dir.cfsaved' so purging does not apply info: Cannot move a directory to repository, leaving at '/tmp/dir.cfsaved' info: Updated '/tmp/dir' from source '/tmp/file' on 'localhost'
#16 Updated by Alexis MOUSSET 7 months ago
- Status changed from In progress to Pending technical review
- Assignee changed from Alexis MOUSSET to Nicolas CHARLES
- Pull Request set to https://github.com/Normation/rudder-techniques/pull/1124
#18 Updated by Alexis MOUSSET 7 months ago
The only case where a directory got replaced by a file in my tests were when the destination did not end with a "/".
The proposed fix is, when the target is an existing directory on the managed node, to enforce that the destination in the directive ends by a "/". This way, the directory will not be replaced, and an error will be reported.
#19 Updated by Alexis MOUSSET 6 months ago
- Status changed from Pending technical review to Pending release
Applied in changeset rudder-techniques|4fb58eb68699892f9821c0c1ef37980d0212286d.
#21 Updated by Vincent MEMBRÉ 5 months ago
- Status changed from Pending release to Released
- Priority set to 0