Project

General

Profile

Bug #10312

"Download files from the shared folder" behaving badly?

Added by Hamlyn Mootoo 7 months ago. Updated 5 months ago.

Status:
Released
Priority:
N/A
Category:
Techniques
Target version:
Target version (plugin):
Severity:
Critical - prevents main use of Rudder | no workaround | data loss | security
User visibility:
Effort required:
Priority:
0

Description

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.

downloadfile.JPG (56 KB) Hamlyn Mootoo, 2017-03-01 13:14


Related issues

Related to Rudder - User story #10214: More consistant naming of techniques Released
Related to Rudder - Bug #10377: When copying files, digest comparison uses ctime when types are different. New
Duplicated by Rudder - Bug #4334: Technique "Download From A Shared Folder": copy files to directory destination doesn't work Rejected

Associated revisions

Revision 4fb58eb6
Added by Alexis MOUSSET 7 months ago

Fixes #10312: \"Download files from the shared folder\" behaving badly?

History

#1 Updated by Vincent MEMBRÉ 7 months ago

  • Target version changed from 4.1.0~rc1 to 4.1.0

#2 Updated by Benoît PECCATTE 7 months ago

The destination must be the absolute file path, not the destination directory.
If you say /tmp, /tmp is considered to be the destination "file".

#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

#4 Updated by Hamlyn Mootoo 7 months ago

Also, when a trailing slash was put in (clearly indicating the intent to put the file into a directory), it behaved the same way, namely overwriting (but luckily renaming first) the directory.

#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).

#6 Updated by Jonathan CLARKE 7 months ago

  • Category changed from Agent to Techniques

#7 Updated by François ARMAND 7 months ago

#8 Updated by Jonathan CLARKE 7 months ago

  • Severity set to Critical - prevents main use of Rudder | no workaround | data loss | security

#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

When the dir directory exists on the destination:

Source Destination Comparison Purge Recursion Outcome
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

With mtime:

 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'

#12 Updated by Alexis MOUSSET 7 months ago

From move_obstruction doc:

If we have promised to make file X a link, but it already exists as a file, or vice-versa, or if a file is blocking the creation of a directory, then normally CFEngine will report an error.

#13 Updated by Alexis MOUSSET 7 months ago

We can:

  • If (the destination exists and is a directory) and (the destination does not end with a "/") => Add a "/" at the end of destination.

This allows to fail is the source is a file, and avoid removing the destination.

#14 Updated by Alexis MOUSSET 7 months ago

  • Category changed from Agent to Techniques

#15 Updated by Alexis MOUSSET 7 months ago

  • Status changed from New to In progress

#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

#17 Updated by Alexis MOUSSET 7 months ago

  • Related to Bug #10377: When copying files, digest comparison uses ctime when types are different. added

#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

#20 Updated by François ARMAND 6 months ago

  • Duplicated by Bug #4334: Technique "Download From A Shared Folder": copy files to directory destination doesn't work added

#21 Updated by Vincent MEMBRÉ 5 months ago

  • Status changed from Pending release to Released
  • Priority set to 0

This bug has been fixed in Rudder 3.1.19, 4.0.4 and 4.1.1 which were released today.

Also available in: Atom PDF