Safe copying of files from local to remote

[sideeffect42]

In cdist-ungleich an interesting discussion has come up in #331:
how can we safely and atomically transfer file from the local machine to the remote machine?

The scope of this problem is not limited to the __file type. It also applies to other types in cdist-conf, e.g. __config_file, __dot_file, __staged_file (via __file), __download and possibly others.

I think the discussion is worth pursuing as a properly designed solution will benefit all cdist users (who doesn't use __file?).

Requirements to a solution:

1. use non-predictable temporary file names (no race conditions),
2. should not fail if temporary directory is size constrained,
3. must not leave the system in a broken state if the destination has insufficient space for the file,
4. must not leave temp files in $destination (think of applications using include * configuration),
5. replacement of the $destination must be atomic (in the sense of made in one SSH connection).

(Please extend if I forgot something.)

Proposals:

Common to all proposals is the division between code-local and code-remote: code-local copies the file to a remote temp location, code-remote moves the remote temp file to $destination and ensures the attributes are set correctly.

Copy to $TMPDIR using mktemp(1), then move to $destination.

This is how __file has worked in the past, albeit using mktemp -u.

pros: uses non-predictable file names (if mktemp is used on the target), no temp files in $destination.
cons: fails if the file is large and $TMPDIR is size constrained (e.g. tmpfs), may leave the system in a broken state if insufficient space in $destination.

Copy to remote ${__object}/files/tempfile, then move to $destination.

This solution uses completely predictable file names, but I don't think this is a problem because cdist's run directory is root-owned and 0700.
Also the remote $__object directory is used for a single cdist run only, so there shouldn't be any collisions possible.

pros: no temp files in $destination.
cons: may fail if cdist's run directory is space-constained, may leave the system in a broken state if insufficient space in $destination.

Copy to $destination.tmp.XXXXXX, then move to $destination.

Idea:
In code-local:

1. Allocate a random temp file in $destination on the target safely, using mktemp,
2. copy file to allocated temp file,
3. store temporary file name in remote's $__object.

Then in code-remote:

1. Set file attributes,
2. move from $destination.tmp.XXXXXX (as stored in $__object) to $destination.

pros: non-predictable temp names, does not use $TMPDIR, does not fail if $destination has insufficient space (can't copy file in the first place)
cons: may leave temp files in $destination if code-* fails for some reason.

Decisions

1. Can we rely on mktemp(1)?
   cdist is supposed to only rely on POSIX features and mktemp(1) isn't defined by POSIX.
   Furthermore mktemp(1) has no standardized interface. An invocation that works fine on Linux may fail or produce an unecpected result on e.g. OpenBSD.

   Or could we hack our own mktemp using /dev/random, tr and set -C?

[4nd3r]

This reminds me an old idea that cdist should have lib for reusable functions. For example $__files/lib.sh (shipped with cdist-conf) which can be sourced in types.

[sideeffect42]

Yup, "the library" would also be very useful here. (I resurrected the issue → skonfig/skonfig#17)

[4nd3r]

See my idea: https://code.ungleich.ch/ungleich-public/cdist/issues/332#issuecomment-14780

Would that help? IMHO it's better than using mktemp everywhere.

[sideeffect42]

Am I overlooking something? This is exactly what __cdist_object_marker is defined as.

That doesn't resolve the attack vector nico mentioned, though.

1. Even if the suffix is random that does not guarantee that no such file already exists at $destination.
2. The random ID (whatever its name is) is already known at object creation, thus long before the tempfile is pushed to the target.
   This gives an attacker plenty of time to create said directory, wait for the file to be uploaded, move the directory away and put the malicious file there.

[sideeffect42]

Providing our own mktemp(1) implementation should be doable.
cf. mktemp.awk

[4nd3r]

Create a temporary file or directory, safely, and print its name.

What does the "safely" imply here? Imho it's not securely. AFAIK mktemp only purpose is to avoid collision and not protect temp files from attacker. Tho it uses perms 600, which is nice to have. If attacker can write somewhere they should not, then the game is already lost, no matter what tricks we pull.

IMHO most reasonable thing is to use what Nico proposed in https://code.ungleich.ch/ungleich-public/cdist/issues/332#issuecomment-14777

[sideeffect42]

Before we go into the mktemp topic too deeply, we first need to define which strategy we want to follow.
Maybe we don't necessarily need secure file names.

As far as security of mktemp goes. Since cdist runs commands as root on the target and files are created with a 077 umask, the only user that could possibly modify the temp file would be root.
If root can modify the file that's fine, because root can modify all of the files anyway.

What is important is that non-privileged users cannot in any way modify the temp file.

Nico's proposal using __cdist_object_marker is even more insecure than mktemp -u is, because it doesn't even try to check if the file already exists.

[4nd3r]

Does it need to be checked? Something like /etc/something/bla.conf.cdist-k9rqtk_z?

Tho, some leftovers explorer would be nice, e.g. remove all files with .cdist-XXXX suffx in dest dir.

[sideeffect42]

Does it need to be checked? Something like /etc/something/bla.conf.cdist-k9rqtk_z?

A proper solution does check for the existence of a temp file. Of course one can always argue with percentages and say that it'll be fine in almost all cases, but you could do the same for mktemp(1), too.
Why even check? If the suffix is random enough, no one will have a file with the same random suffix.
I'm pretty sure the developers of mktemp had their reasons to still include the check.

IMO this case is frequent enough to invest the time into a proper solution.

Tho, some leftovers explorer would be nice, e.g. remove all files with .cdist-XXXX suffx in dest dir.

We need a solution for leftovers (cf. 4. in "Requirements to a solution"), but please not an explorer!
Explorers must be read-only.

I was thinking about whether we could trap EXIT for this, but this would probably not work if the connection was lost during an scp copy.
(What does scp do if the connection drops in the middle of the transfer?)

[4nd3r]

Explorers must be read-only.

Yes, explorers discover first and... :)

__clean_path in manifest and done.

[Matze1224]

Tho, some leftovers explorer would be nice, e.g. remove all files with .cdist-XXXX suffx in dest dir.

I keep my opinion to remove all cdist temp files before the $__remote_copy instead of a check.

But btw. I don't even know if I commited too much into the discussion, only disturbed them or was simply ignored. But after some flawed proposals, this where the parts I've finally came up with. Shouldn't start to get off-topic here :P

Edit: After looking into cdist-ungleich's master, it looks like he took my last comment at heart - maybe. The fixes from Steven except the newest one (abbc7dfc37, 1 day ago) are already commited. Still uses mktemp for local RNG, but have a rm before the final move. Only an exact remove - could be used for cleanup if you change it to a wildcard-match (but don't say this gonna be easy in the scope of gencode-remote).