ungleich-public/cdist
ungleich-public/cdist
7
4
Fork 10
Code Issues 58 Pull requests 15 Projects Releases Wiki Activity

Make file attribute changes more atomic #331

Closed
mark wants to merge 1 commit from (deleted):__file-atomic-attributes into master
pull from: (deleted):__file-atomic-attributes
merge into: ungleich-public:master
Conversation 39 Commits 1 Files changed 2 +18 -7
2 changed files with 18 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
cdist/conf/type/__file/gencode-local
View file

@ -89,10 +89,20 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then
touch "$__object/files/set-attributes" touch "$__object/files/set-attributes"
# upload file to temp location # upload file to temp location
tempfile_template="${destination}.cdist.XXXXXXXXXX" destination_upload="${destination}${__cdist_object_marker}"
cat << DONE cat << DONE
destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")" $__remote_exec $__target_host test -e "$destination_upload" && {
echo "Refusing to upload file to existing destination: $destination_upload" >&2
exit 1
} || {
# Put a towel in place.
$__remote_exec $__target_host "umask 077; touch \"$destination_upload\""
nico marked this conversation as resolved
nico commented 2022-04-07 07:30:02 +00:00
Review
Copy link

Just wondering, are we adding a behaviour change here? I.e. before we unconditionally deleted the file/directory/socket/whatever. Now we fail if it exists?

NVM, this is just the temporary location. This still has a race condition, addressing this in a bigger comment.

Just wondering, are we adding a behaviour change here? I.e. before we unconditionally deleted the file/directory/socket/whatever. Now we fail if it exists? NVM, this is just the temporary location. This still has a race condition, addressing this in a bigger comment.
steven commented 2022-04-07 07:47:11 +00:00
Review
Copy link

Before we used mktemp which would have also failed if it could not have created a file, not?
Of course the chance of mktemp failing is like zero, with at least 3 X's.

Before we used mktemp which would have also failed if it could not have created a file, not? Of course the chance of mktemp failing is like zero, with at least 3 X's.
nico commented 2022-04-07 07:58:49 +00:00
Review
Copy link

mktemp works differently. What we are doing now is similar to mktemp -u.

Again, whether this is an actual problem, is a different question.

The typical issue mktemp is trying to solve:

  • /tmp is 4777 or similar - everybody can write to it
  • mktemp needs to ensure that nobody is putting a symlink or directory to the temporary file that mktemp guarantees to be safe

What we do is:

  • Deploy files to specific locations

The question we need to answer is:

  • Are we doing it in a secure enough way that does not allow an attacker to tamper with it
mktemp works differently. What we are doing now is similar to `mktemp -u`. Again, whether this is an actual problem, is a different question. The typical issue mktemp is trying to solve: * /tmp is 4777 or similar - everybody can write to it * mktemp needs to ensure that nobody is putting a symlink or directory to the temporary file that mktemp guarantees to be safe What we do is: * Deploy files to specific locations The question we need to answer is: * Are we doing it in a secure enough way that does not allow an attacker to tamper with it
}
DONE DONE
# Tell gencode-remote that it has to move our file to its
# final destination.
touch "$__object/files/file-uploaded"
if [ "$upload_file" ]; then if [ "$upload_file" ]; then
echo upload >> "$__messages_out" echo upload >> "$__messages_out"
# IPv6 fix # IPv6 fix
@ -103,12 +113,8 @@ DONE
my_target_host="${__target_host}" my_target_host="${__target_host}"
fi fi
cat << DONE cat << DONE
$__remote_copy "$source" "${my_target_host}:\$destination_upload" $__remote_copy "$source" "${my_target_host}:${destination_upload}"
DONE DONE
fi fi
# move uploaded file into place
cat << DONE
$__remote_exec $__target_host "rm -rf \"$destination\"; mv \"\$destination_upload\" \"$destination\""
DONE
fi fi
fi fi

5
cdist/conf/type/__file/gencode-remote
View file

@ -62,6 +62,11 @@ set_mode() {
case "$state_should" in case "$state_should" in
present|exists) present|exists)
if [ -f "$__object/files/file-uploaded" ]; then
# move uploaded file into place
printf 'rm -rf "%s"\n' "$destination"
printf 'mv "%s" "%s"\n' "${destination}${__cdist_object_marker}" "$destination"
fi
# Note: Mode - needs to happen last as a chown/chgrp can alter mode by # Note: Mode - needs to happen last as a chown/chgrp can alter mode by
# clearing S_ISUID and S_ISGID bits (see chown(2)) # clearing S_ISUID and S_ISGID bits (see chown(2))
for attribute in group owner mode; do for attribute in group owner mode; do