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
mark commented 2022-04-03 09:20:24 +00:00
Contributor
Copy link

I ran into a probably niche case with the file type. When configuring files related to
ssh. I saw this in two cases:

  • hosts.allow/deny and related filters
  • ssh related file for the non root user cdist logs in as

The root cause seems to be that the upload of a new or modified file is executed through the code generated in gencode-local. This creates commands to generate a unique remote filename, uploads the file and moves it into the final location.
But file attributes are put in place in gencode-remote, which will always run in a seperate ssh session. Any configuration done impacting the ssh connection required can make for a situation where the configuration run is aborted and the target is left in a partially unreachable state for ssh.

The only option I could come up with to mitigate this, is to move all code that determines the changes that need to be done to the file on the target to the gencode-local manifest and generate a command sequence that is run through a single ssh session.

I've tested this and it seems to work with the testcases and production manifests I am running.

I'm not sure if this is the best solution for this problem or that I'm overlooking some other option.

Regards,

Mark

I ran into a probably niche case with the file type. When configuring files related to ssh. I saw this in two cases: * hosts.allow/deny and related filters * ssh related file for the non root user cdist logs in as The root cause seems to be that the upload of a new or modified file is executed through the code generated in gencode-local. This creates commands to generate a unique remote filename, uploads the file and moves it into the final location. But file attributes are put in place in gencode-remote, which will always run in a seperate ssh session. Any configuration done impacting the ssh connection required can make for a situation where the configuration run is aborted and the target is left in a partially unreachable state for ssh. The only option I could come up with to mitigate this, is to move all code that determines the changes that need to be done to the file on the target to the gencode-local manifest and generate a command sequence that is run through a single ssh session. I've tested this and it seems to work with the testcases and production manifests I am running. I'm not sure if this is the best solution for this problem or that I'm overlooking some other option. Regards, Mark
mark added 1 commit 2022-04-03 09:20:25 +00:00 
file attribute change more atomic (in 1 ssh command). This fixes the problem
when distributing files related to ssh access on the host (for example files
in ~/.ssh or /etc/hosts.allow|deny).
nico commented 2022-04-03 10:19:02 +00:00
Owner
Copy link

Hey @mark! Thanks for the PR.

I am not fully getting the impact just yet, but moving the whole logic into __gencode_local as a starter "feels" wrong (doesn't mean your solution is right).

Can you describe the practical implications of the non-atomicity that you are experiencing?

  • What's your manifest?
  • How does it abort?
  • What are the resulting permissions/incorrect settings?

The __file type is indeed a bit special, as it uses most features of cdist and from the get-go is the most intensive type :-)

Hey @mark! Thanks for the PR. I am not fully getting the impact just yet, but moving the whole logic into `__gencode_local` as a starter "feels" wrong (doesn't mean your solution is right). Can you describe the practical implications of the non-atomicity that you are experiencing? * What's your manifest? * How does it abort? * What are the resulting permissions/incorrect settings? The `__file` type is indeed a bit special, as it uses most features of cdist and from the get-go is the most intensive type :-)
mark commented 2022-04-03 10:44:55 +00:00
Author
Contributor
Copy link

Hi Nico,

No problem, I'll describe the case in more detail. I ran into this issue recently when deploying an filter script which is used to geoblock ssh sessions through hosts.allow.

For context, the hosts.allow:

sshd: ALL: aclexec /usr/local/bin/geofilter %a

And hosts.deny:

sshd: ALL

The filter script sometimes gets updates, so this is deployed with cdist (the hosts.allow/deny also, but that is a one time thing, they don't change, so it isn't relevant). I've made a simple init manifest to demonstrate:

cat /tmp/conf/init
_file /usr/local/bin/geofilter \
   --source "$files/geoip/geofilter" \
   --mode 0750 --owner root --group root --state present

So, starting out with a state of the client that works, the filter file looks like this on the client:

ls -al /usr/local/bin/geofilter
-rwxr-x--- 1 root root 219 Apr  2 00:38 /usr/local/bin/geofilter

Now I run the type (only thing in the manifest) from the cdist server (after having made a modification to the filter script ofcourse):

cdist config -i /tmp/conf/init -l-1 -v --remote-exec ssh --remote-copy 'scp -q' -v shell
INFO: shell: Starting configuration run
INFO: shell: Processing __file/usr/local/bin/geofilter
ERROR: shell: ssh shell mkdir -p /var/lib/cdist/object/__file/usr/local/bin/geofilter/.cdist-fjw53oye: ['ssh', 'shell', 'mkdir', '-p', '/var/lib/cdist/object/__file/usr/local/bin/geofilter/.cdist-fjw53oye']

Error processing object '__file/usr/local/bin/geofilter'
========================================================
name: __file/usr/local/bin/geofilter
path: /tmp/tmpnd5i1l76/2591c98b70119fe624898b1e424b5e91/data/object/__file/usr/local/bin/geofilter/.cdist-fjw53oye
source: /tmp/conf/init
type: /home/cdist/cdist/cdist/conf/type/__file


ERROR: cdist: Failed to configure the following hosts: shell

Then, looking on the client, the geofilter file looks as follows:

ls -al /usr/local/bin/geofilter
-rw------- 1 root staff 227 Apr  3 12:39 /usr/local/bin/geofilter

From what I see what goes wrong in this specific case is the file is copied to the client in the gencode-local. After the copy it is moved into it's final destination. This step is done in 1 ssh command. Then the code generated by gencode-remote is ran. These are the commands to change the file's attributes. Those are set in a new ssh command. But the ssh command will now fail, because geofilter is not executable yet.

There is another case with wich I ran into the same problem, but I think this (hopefully) illustrates what I ran into.

Again, not sure the way I tried to fix it is the right one, but I couldn't think of anything else to make the move to the final destination and the attribute change more atomic..

Regards,

Mark

Hi Nico, No problem, I'll describe the case in more detail. I ran into this issue recently when deploying an filter script which is used to geoblock ssh sessions through hosts.allow. For context, the hosts.allow: ``` sshd: ALL: aclexec /usr/local/bin/geofilter %a ``` And hosts.deny: ``` sshd: ALL ``` The filter script sometimes gets updates, so this is deployed with cdist (the hosts.allow/deny also, but that is a one time thing, they don't change, so it isn't relevant). I've made a simple init manifest to demonstrate: ``` cat /tmp/conf/init _file /usr/local/bin/geofilter \ --source "$files/geoip/geofilter" \ --mode 0750 --owner root --group root --state present ``` So, starting out with a state of the client that works, the filter file looks like this on the client: ``` ls -al /usr/local/bin/geofilter -rwxr-x--- 1 root root 219 Apr 2 00:38 /usr/local/bin/geofilter ``` Now I run the type (only thing in the manifest) from the cdist server (after having made a modification to the filter script ofcourse): ``` cdist config -i /tmp/conf/init -l-1 -v --remote-exec ssh --remote-copy 'scp -q' -v shell INFO: shell: Starting configuration run INFO: shell: Processing __file/usr/local/bin/geofilter ERROR: shell: ssh shell mkdir -p /var/lib/cdist/object/__file/usr/local/bin/geofilter/.cdist-fjw53oye: ['ssh', 'shell', 'mkdir', '-p', '/var/lib/cdist/object/__file/usr/local/bin/geofilter/.cdist-fjw53oye'] Error processing object '__file/usr/local/bin/geofilter' ======================================================== name: __file/usr/local/bin/geofilter path: /tmp/tmpnd5i1l76/2591c98b70119fe624898b1e424b5e91/data/object/__file/usr/local/bin/geofilter/.cdist-fjw53oye source: /tmp/conf/init type: /home/cdist/cdist/cdist/conf/type/__file ERROR: cdist: Failed to configure the following hosts: shell ``` Then, looking on the client, the geofilter file looks as follows: ``` ls -al /usr/local/bin/geofilter -rw------- 1 root staff 227 Apr 3 12:39 /usr/local/bin/geofilter ``` From what I see what goes wrong in this specific case is the file is copied to the client in the gencode-local. After the copy it is moved into it's final destination. This step is done in 1 ssh command. Then the code generated by gencode-remote is ran. These are the commands to change the file's attributes. Those are set in a new ssh command. But the ssh command will now fail, because geofilter is not executable yet. There is another case with wich I ran into the same problem, but I think this (hopefully) illustrates what I ran into. Again, not sure the way I tried to fix it is the right one, but I couldn't think of anything else to make the move to the final destination and the attribute change more atomic.. Regards, Mark
nico commented 2022-04-04 13:42:58 +00:00
Owner
Copy link

Thanks a lot for the insight, @mark! And I get the problem and rephrasing it in my own words for a later reference:

  • One object is modifying the behaviour of sshd in code-local
  • cdist fails to continue, because new connections cannot be established anymore

Good news is, we probably don't need a patch at all (or at least not to solve your problem):

  • cdist usually uses a control socket (see the default remote exec/copy)
  • If your custom remote-exec/copy also uses the control socket, no new ssh connection will be established

Now there are two things that we might still want to patch:

  • First, we should probably document that the ssh connection is supposed to last during one run ("expected behaviour of remote-exec/copy")
  • Second we might still consider a patch to the file type to not create transient incorrect permission files

We do have another criteria to watch:

  • Types should only generate code, if necessary

My first idea was to shift the mv into code-remote and executing the permissions change before the move.

Another direction would be to include more into gencode-local. Not exactly sure what's the best way atm, will probably need a couple of days to think about it.

Thanks a lot for the insight, @mark! And I get the problem and rephrasing it in my own words for a later reference: * One object is modifying the behaviour of sshd in code-local * cdist fails to continue, because new connections cannot be established anymore Good news is, we probably don't need a patch at all (or at least not to solve your problem): * cdist usually uses a control socket (see the default remote exec/copy) * If your custom remote-exec/copy also uses the control socket, no new ssh connection will be established Now there are two things that we might still want to patch: * First, we should probably document that the ssh connection is supposed to last during one run ("expected behaviour of remote-exec/copy") * Second we might still consider a patch to the __file__ type to not create transient incorrect permission files We do have another criteria to watch: * Types should only generate code, if necessary My first idea was to shift the `mv` into code-remote and executing the permissions change before the move. Another direction would be to include more into gencode-local. Not exactly sure what's the best way atm, will probably need a couple of days to think about it.
mark commented 2022-04-06 05:04:03 +00:00
Author
Contributor
Copy link

Hi Nico,

As far as I know I don't have any specific changes active regadring the use of control sockets on the cdist server. I will look into that to see if there is something wrong there, because if new connections through the control socket isn't checked against hosts.allow anymore this shouldn't happen.

For me the problem isn't as much that file creates a new file with the wrong permissions, it is the split that occurs between gencode-local and gencode-remote. In my opinion the way gencode-local is used within the file type is wrong. It makes sense to have the remote copy there. But command run on the target should not be in gencode-local. But that is not an option if you want to find a safe, temporary filename on the target to place the file to be transferred. That is where the problem originates. The filename is determined through mktemp in gencode-local and needs to be used there to copy the file to remote. But as that filename is stored in the environment it is only available in the scope of the commands run from gencode-local. And thus you need to place the move to the final destination on the target there (which is again a command run from gencode-local on the remote).

Putting the move in gencode-remote would be the complete solution, but how do you generate a safe, temporary filename where the file to be copied can be placed that is available for gencode-local and gencode-remote?

I thought about creating one always with an explorer and always removing it even if it isn't used. But that is just very ugly in my opinion..

Regards,

Mark

Hi Nico, As far as I know I don't have any specific changes active regadring the use of control sockets on the cdist server. I will look into that to see if there is something wrong there, because if new connections through the control socket isn't checked against hosts.allow anymore this shouldn't happen. For me the problem isn't as much that file creates a new file with the wrong permissions, it is the split that occurs between gencode-local and gencode-remote. In my opinion the way gencode-local is used within the file type is wrong. It makes sense to have the remote copy there. But command run on the target should not be in gencode-local. But that is not an option if you want to find a safe, temporary filename on the target to place the file to be transferred. That is where the problem originates. The filename is determined through mktemp in gencode-local and needs to be used there to copy the file to remote. But as that filename is stored in the environment it is only available in the scope of the commands run from gencode-local. And thus you need to place the move to the final destination on the target there (which is again a command run from gencode-local on the remote). Putting the move in gencode-remote would be the complete solution, but how do you generate a safe, temporary filename where the file to be copied can be placed that is available for gencode-local and gencode-remote? I thought about creating one always with an explorer and always removing it even if it isn't used. But that is just very ugly in my opinion.. Regards, Mark
nico commented 2022-04-06 05:34:05 +00:00
Owner
Copy link

Hey Mark,

I was thinking in a similar direction, that the mv should just be on "on the other side".

One problem that we have as a base is, where to temporarily copy the file over and do we actually need a copy?

In theory we could copy the file inside /var/lib/cdist, into the object folder. This might be a bit tricky in terms of space requirements, but would give a secure (/var/lib/cdist is cdist only writable by us) destination with predictable naming.

I think in the beginning of cdist some people had specific mount options for /var, which made this tricky, but I might also be mistaken on that one and /var/lib/cdist might be an option.

Another approach I was thinking about is generating the filename like mktemp -u, however that one is not secure.

Then again, code-local could in theory write the filename into the object directory on the other side and thus code-remote could actually

  • change permissions first
  • then mv the file

So far I'd say that could be the best solution.

pinging another oldtimer @steven, he might remember why we opted for mktemp in the first place.

Hey Mark, I was thinking in a similar direction, that the mv should just be on "on the other side". One problem that we have as a base is, where to temporarily copy the file over and do we actually need a copy? In theory we could copy the file inside /var/lib/cdist, into the object folder. This might be a bit tricky in terms of space requirements, but would give a secure (/var/lib/cdist is cdist only writable by us) destination with predictable naming. I think in the beginning of cdist some people had specific mount options for /var, which made this tricky, but I might also be mistaken on that one and /var/lib/cdist might be an option. Another approach I was thinking about is generating the filename like `mktemp -u`, however that one is not secure. Then again, `code-local` could in theory write the **filename** into the object directory on the other side and thus `code-remote` could actually * change permissions first * then mv the file So far I'd say that could be the best solution. pinging another oldtimer @steven, he might remember why we opted for mktemp in the first place.
mark commented 2022-04-06 09:23:53 +00:00
Author
Contributor
Copy link

Hi Nico,

Writing the filename to some place that is also accessable for the remote side would be a great way out of this!

But from what I understood of the documentation is that the code generated by gencode-local and gencode-remote has read-only access to information referenced by $__object and $__object_id. The unique filename should be generated on the remote side. This sounds like the best solution, as it will then be generated at the same location as the original file. So any problems like filesystem sizes etc. should not be relevant (or at least expected, as that is the filesystem the modification is made). It isn't possible to generated the unique filename during the runtime of gencode-local, so it must be in the generated code. And then it can't communicate the resulting filename to the code generated by gencode-remote..

So you would want to have something that executes before gencode-local and gencode-remote that provides the unique filename on the remote side. And I think the only option there is the explorer. But that is pretty ugly imho if the explorer actually also generates the file (empty) on the filesystem, because that would always require cleanup. If the explorer only generates a safe filename for later use, that would be less of an issue I think. But that is ofcourse less safe (but pretty simple to implement ;) ).

Regards,

Mark

Hi Nico, Writing the filename to some place that is also accessable for the remote side would be a great way out of this! But from what I understood of the documentation is that the code generated by gencode-local and gencode-remote has read-only access to information referenced by $__object and $__object_id. The unique filename should be generated on the remote side. This sounds like the best solution, as it will then be generated at the same location as the original file. So any problems like filesystem sizes etc. should not be relevant (or at least expected, as that is the filesystem the modification is made). It isn't possible to generated the unique filename during the runtime of gencode-local, so it must be in the generated code. And then it can't communicate the resulting filename to the code generated by gencode-remote.. So you would want to have something that executes before gencode-local and gencode-remote that provides the unique filename on the remote side. And I think the only option there is the explorer. But that is pretty ugly imho if the explorer actually also generates the file (empty) on the filesystem, because that would always require cleanup. If the explorer only generates a safe filename for later use, that would be less of an issue I think. But that is ofcourse less safe (but pretty simple to implement ;) ). Regards, Mark
nico commented 2022-04-06 09:43:13 +00:00
Owner
Copy link

If I am not mistaken and the cdist-reference so far agrees with be, the $__object variable is available for code:

__object
    Directory that contains the current object.

    Available for: type manifest, type explorer, type gencode and code scripts.

See https://www.cdi.st/manual/latest/cdist-reference.html for details.

Do you want to give it a spin with adding something on the line of

echo mkdir -p \$__object/files
echo \$filename_from_mktemp > \$__object/files/filename

and then in gencode-remote

mv \$(cat \$__object/files/filename) $final_destination

?

If I am not mistaken and the cdist-reference so far agrees with be, the `$__object` variable is available for code: ``` __object Directory that contains the current object. Available for: type manifest, type explorer, type gencode and code scripts. ``` See https://www.cdi.st/manual/latest/cdist-reference.html for details. Do you want to give it a spin with adding something on the line of ``` echo mkdir -p \$__object/files echo \$filename_from_mktemp > \$__object/files/filename ``` and then in gencode-remote ``` mv \$(cat \$__object/files/filename) $final_destination ``` ?
mark commented 2022-04-06 10:22:35 +00:00
Author
Contributor
Copy link

Hi Nico,

Did a quick test, but when executing the output from gencode-remote I get:

code-remote:stderr
------------------
cat: /var/lib/cdist/object/__file/usr/local/bin/geofilter/.cdist-cpnrkpc5/files/filename: No such file or directory

From what I read in the documentation I assumed this would not work because of the statement:

15.20. Variable access from the generated scripts

In the generated scripts, you have access to the following cdist variables

    __object

    __object_id

but only for read operations, means there is no back copy of this files after the script execution.

Assuming I did the test correctly, it seems to match the documentation (always good ;) ). This would be a nice way to solve it..

Regards,

Mark

Hi Nico, Did a quick test, but when executing the output from gencode-remote I get: ``` code-remote:stderr ------------------ cat: /var/lib/cdist/object/__file/usr/local/bin/geofilter/.cdist-cpnrkpc5/files/filename: No such file or directory ``` From what I read in the documentation I assumed this would not work because of the statement: ``` 15.20. Variable access from the generated scripts In the generated scripts, you have access to the following cdist variables __object __object_id but only for read operations, means there is no back copy of this files after the script execution. ``` Assuming I did the test correctly, it seems to match the documentation (always good ;) ). This would be a nice way to solve it.. Regards, Mark
nico commented 2022-04-06 13:05:02 +00:00
Owner
Copy link

You are right, we did not really consider the IPC between code-local and code-remote on the remote side.

I tested using a copy of the __file type:

destination="/$__object_id"
source="$(cat "$__object/parameter/source")"

cat <<DONE | tee /tmp/grr
destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")"
$__remote_exec $__target_host "mkdir -p \$__object/files; echo \$destination_upload > \$__object/files/filename; echo \$__object > /tmp/zz"
$__remote_copy "$source" "${__target_host}:\$destination_upload"
DONE

resulting into

root@nico-vm:~# cat /tmp/zz 
/tmp/tmp1uzvquxe/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-6dnmmln3

Using

echo __file_plus /tmp/moo --source /etc/passwd --mode 0740 |   cdist config -i -  -vvvv  --color never nico-vm.schottelius.org

for debugging. We see the old logic and order:

  • generating code (local, remote)
  • executing code (local, remote)

code-local and code-remove

DEBUG: nico-vm.schottelius.org: Generating code for __file_plus/tmp/moo
DEBUG: nico-vm.schottelius.org: /tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/conf/type/__file_plus/gencode-local is executable, running it
TRACE: nico-vm.schottelius.org: Local run: ['/home/nico/vcs/cdist-workdir/cdist/cdist/conf/type/__file_plus/gencode-local']
TRACE: nico-vm.schottelius.org: Command: ['/home/nico/vcs/cdist-workdir/cdist/cdist/conf/type/__file_plus/gencode-local']; Local stderr: 
DEBUG: nico-vm.schottelius.org: /tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/conf/type/__file_plus/gencode-remote is executable, running it
TRACE: nico-vm.schottelius.org: Local run: ['/home/nico/vcs/cdist-workdir/cdist/cdist/conf/type/__file_plus/gencode-remote']
TRACE: nico-vm.schottelius.org: Command: ['/home/nico/vcs/cdist-workdir/cdist/cdist/conf/type/__file_plus/gencode-remote']; Local stderr: 
INFO: nico-vm.schottelius.org: Processing __file_plus/tmp/moo
TRACE: nico-vm.schottelius.org: Executing local code for __file_plus/tmp/moo
DEBUG: nico-vm.schottelius.org: /tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-local is NOT executable, running it with /bin/sh -e
TRACE: nico-vm.schottelius.org: Local run: ['/bin/sh', '-e', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-local']
TRACE: nico-vm.schottelius.org: Command: ['/bin/sh', '-e', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-local']; Local stderr: 
TRACE: nico-vm.schottelius.org: Command: ['/bin/sh', '-e', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-local']; Local stdout: 
TRACE: nico-vm.schottelius.org: Executing remote code for __file_plus/tmp/moo
TRACE: nico-vm.schottelius.org: Remote mkdir: /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58
TRACE: nico-vm.schottelius.org: Remote run: ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', 'mkdir', '-p', '/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58']
TRACE: nico-vm.schottelius.org: Command: ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', 'mkdir', '-p', '/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58']; Remote stderr: 
TRACE: nico-vm.schottelius.org: Command: ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', 'mkdir', '-p', '/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58']; Remote stdout: 
TRACE: nico-vm.schottelius.org: Remote transfer: /tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote -> /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote
TRACE: nico-vm.schottelius.org: Remote run: ['scp', '-o', 'User=root', '-q', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote', 'nico-vm.schottelius.org:/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote']
TRACE: nico-vm.schottelius.org: Command: ['scp', '-o', 'User=root', '-q', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote', 'nico-vm.schottelius.org:/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote']; Remote stderr: 
TRACE: nico-vm.schottelius.org: Command: ['scp', '-o', 'User=root', '-q', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote', 'nico-vm.schottelius.org:/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote']; Remote stdout: 
TRACE: nico-vm.schottelius.org: Remote run: ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', "/bin/sh -c ' export __object=/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58;  export __object_id=tmp/moo;/bin/sh -e /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote'"]
ERROR: nico-vm.schottelius.org: ssh -o User=root -o ControlPath=/tmp/tmp9v1i230q/s -o ControlMaster=auto -o ControlPersist=2h nico-vm.schottelius.org /bin/sh -c ' export __object=/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58;  export __object_id=tmp/moo;/bin/sh -e /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote': ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', "/bin/sh -c ' export __object=/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58;  export __object_id=tmp/moo;/bin/sh -e /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote'"]

Using gencode-local and remote with

echo "echo Object: \$__object > /tmp/local_object"


echo "echo Object: \$__object > /tmp/remote_object"

shows we have different paths references to it:

root@nico-vm:~# cat /tmp/remote_object 
Object: /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-l3bwcfru
root@nico-vm:~# 

[14:53] bridge:~% cat /tmp/local_object 
Object: /tmp/tmpz_g7bxi4/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-l3bwcfru

And in between the code-local execution and the code-remote execution is no synchronisation between the $__object directory anymore.

So long talk, what would probably make sense is to add a reference to the remote path of the object to gencode-local as well as code-local, probably naming it __object_remote.

You are right, we did not really consider the IPC between code-local and code-remote on the remote side. I tested using a copy of the `__file` type: ``` destination="/$__object_id" source="$(cat "$__object/parameter/source")" cat <<DONE | tee /tmp/grr destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")" $__remote_exec $__target_host "mkdir -p \$__object/files; echo \$destination_upload > \$__object/files/filename; echo \$__object > /tmp/zz" $__remote_copy "$source" "${__target_host}:\$destination_upload" DONE ``` resulting into ``` root@nico-vm:~# cat /tmp/zz /tmp/tmp1uzvquxe/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-6dnmmln3 ``` Using ``` echo __file_plus /tmp/moo --source /etc/passwd --mode 0740 | cdist config -i - -vvvv --color never nico-vm.schottelius.org ``` for debugging. We see the old logic and order: * generating code (local, remote) * executing code (local, remote) code-local and code-remove ``` DEBUG: nico-vm.schottelius.org: Generating code for __file_plus/tmp/moo DEBUG: nico-vm.schottelius.org: /tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/conf/type/__file_plus/gencode-local is executable, running it TRACE: nico-vm.schottelius.org: Local run: ['/home/nico/vcs/cdist-workdir/cdist/cdist/conf/type/__file_plus/gencode-local'] TRACE: nico-vm.schottelius.org: Command: ['/home/nico/vcs/cdist-workdir/cdist/cdist/conf/type/__file_plus/gencode-local']; Local stderr: DEBUG: nico-vm.schottelius.org: /tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/conf/type/__file_plus/gencode-remote is executable, running it TRACE: nico-vm.schottelius.org: Local run: ['/home/nico/vcs/cdist-workdir/cdist/cdist/conf/type/__file_plus/gencode-remote'] TRACE: nico-vm.schottelius.org: Command: ['/home/nico/vcs/cdist-workdir/cdist/cdist/conf/type/__file_plus/gencode-remote']; Local stderr: INFO: nico-vm.schottelius.org: Processing __file_plus/tmp/moo TRACE: nico-vm.schottelius.org: Executing local code for __file_plus/tmp/moo DEBUG: nico-vm.schottelius.org: /tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-local is NOT executable, running it with /bin/sh -e TRACE: nico-vm.schottelius.org: Local run: ['/bin/sh', '-e', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-local'] TRACE: nico-vm.schottelius.org: Command: ['/bin/sh', '-e', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-local']; Local stderr: TRACE: nico-vm.schottelius.org: Command: ['/bin/sh', '-e', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-local']; Local stdout: TRACE: nico-vm.schottelius.org: Executing remote code for __file_plus/tmp/moo TRACE: nico-vm.schottelius.org: Remote mkdir: /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58 TRACE: nico-vm.schottelius.org: Remote run: ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', 'mkdir', '-p', '/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58'] TRACE: nico-vm.schottelius.org: Command: ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', 'mkdir', '-p', '/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58']; Remote stderr: TRACE: nico-vm.schottelius.org: Command: ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', 'mkdir', '-p', '/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58']; Remote stdout: TRACE: nico-vm.schottelius.org: Remote transfer: /tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote -> /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote TRACE: nico-vm.schottelius.org: Remote run: ['scp', '-o', 'User=root', '-q', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote', 'nico-vm.schottelius.org:/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote'] TRACE: nico-vm.schottelius.org: Command: ['scp', '-o', 'User=root', '-q', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote', 'nico-vm.schottelius.org:/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote']; Remote stderr: TRACE: nico-vm.schottelius.org: Command: ['scp', '-o', 'User=root', '-q', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', '/tmp/tmp2u1cqjxd/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote', 'nico-vm.schottelius.org:/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote']; Remote stdout: TRACE: nico-vm.schottelius.org: Remote run: ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', "/bin/sh -c ' export __object=/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58; export __object_id=tmp/moo;/bin/sh -e /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote'"] ERROR: nico-vm.schottelius.org: ssh -o User=root -o ControlPath=/tmp/tmp9v1i230q/s -o ControlMaster=auto -o ControlPersist=2h nico-vm.schottelius.org /bin/sh -c ' export __object=/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58; export __object_id=tmp/moo;/bin/sh -e /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote': ['ssh', '-o', 'User=root', '-o', 'ControlPath=/tmp/tmp9v1i230q/s', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=2h', 'nico-vm.schottelius.org', "/bin/sh -c ' export __object=/var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58; export __object_id=tmp/moo;/bin/sh -e /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-k77r9h58/code-remote'"] ``` Using gencode-local and remote with ``` echo "echo Object: \$__object > /tmp/local_object" ``` ``` echo "echo Object: \$__object > /tmp/remote_object" ``` shows we have different paths references to it: ``` root@nico-vm:~# cat /tmp/remote_object Object: /var/lib/cdist/object/__file_plus/tmp/moo/.cdist-l3bwcfru root@nico-vm:~# ``` ``` [14:53] bridge:~% cat /tmp/local_object Object: /tmp/tmpz_g7bxi4/02a3bca3fd4aac868088879a5ec03871/data/object/__file_plus/tmp/moo/.cdist-l3bwcfru ``` And in between the code-local execution and the code-remote execution is no synchronisation between the `$__object` directory anymore. So long talk, what would probably make sense is to add a reference to the remote path of the object to gencode-local as well as code-local, probably naming it `__object_remote`.
mark commented 2022-04-06 13:45:31 +00:00
Author
Contributor
Copy link

Hi Nico,

That sounds like a good solution. Would maken for a nice fix for this problem, and also the possibility to clean up the __file type, moving the mv from gencode-local to gencode-remote.

Regards,

Mark

Hi Nico, That sounds like a good solution. Would maken for a nice fix for this problem, and also the possibility to clean up the __file type, moving the mv from gencode-local to gencode-remote. Regards, Mark
steven commented 2022-04-06 14:09:44 +00:00
Collaborator
Copy link

Re IPC, this would work, not?

[16:06:13] eos:__file% git d gencode-local 
diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local
index 231b6927..ba2a8ab1 100755
--- a/cdist/conf/type/__file/gencode-local
+++ b/cdist/conf/type/__file/gencode-local
@@ -92,6 +92,7 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then
       tempfile_template="${destination}.cdist.XXXXXXXXXX"
       cat << DONE
 destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")"
+echo "\$destination_upload" > "$__object/files/destination-upload"
 DONE
       if [ "$upload_file" ]; then
          echo upload >> "$__messages_out"
[16:06:16] eos:__file%

And then in __file/gencode-remote: (pseudo-code):

destination="/$__object_id"
destination_upload=$(cat "$__object/files/destination-upload")"

cat << DONE
rm -rf "$destination"
mv "$destination_upload" "$destination"
DONE
Re IPC, this would work, not? ``` [16:06:13] eos:__file% git d gencode-local diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local index 231b6927..ba2a8ab1 100755 --- a/cdist/conf/type/__file/gencode-local +++ b/cdist/conf/type/__file/gencode-local @@ -92,6 +92,7 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then tempfile_template="${destination}.cdist.XXXXXXXXXX" cat << DONE destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")" +echo "\$destination_upload" > "$__object/files/destination-upload" DONE if [ "$upload_file" ]; then echo upload >> "$__messages_out" [16:06:16] eos:__file% ``` And then in __file/gencode-remote: (pseudo-code): ``` destination="/$__object_id" destination_upload=$(cat "$__object/files/destination-upload")" cat << DONE rm -rf "$destination" mv "$destination_upload" "$destination" DONE ```
steven commented 2022-04-06 14:11:37 +00:00
Collaborator
Copy link

Otherwise, could we maybe (ab|re)use __cdist_object_marker to have a name that we know both in gencode-local and in gencode-remote?

e.g. __file/gencode-local:

destination_upload="${destination}.${__cdist_object_marker}"

e.g. __file/gencode-remote: (pseudo-code)

chmod ... "${destination}.${__cdist_object_marker}"
chown ... "${destination}.${__cdist_object_marker}"
mv "${destination}.${__cdist_object_marker}" "${destination}"

Just thinking out loud.

Otherwise, could we maybe (ab|re)use `__cdist_object_marker` to have a name that we know both in gencode-local and in gencode-remote? e.g. __file/gencode-local: ``` destination_upload="${destination}.${__cdist_object_marker}" ``` e.g. __file/gencode-remote: (pseudo-code) ``` chmod ... "${destination}.${__cdist_object_marker}" chown ... "${destination}.${__cdist_object_marker}" mv "${destination}.${__cdist_object_marker}" "${destination}" ``` Just thinking out loud.
mark commented 2022-04-06 14:52:41 +00:00
Author
Contributor
Copy link

Hi Steven,

Tried the changes with __cdist_object_marker, and that seems to work fine! I guess it is a bit less secure then running mktemp on the destination as this way you assume there is no (important) file on the remote which has the same name as the destinationfile with the generated object marker, but I would find that a very reasonable assumption.

BTW, did do the mv first in the gencode-remote before the chown to prevent issue's with changing only ownership/modes for an already existing file. I don't think that should be an issue, as all generated code from gencode-remote is run in a single ssh session, correct?

Regards,

Mark

Hi Steven, Tried the changes with __cdist_object_marker, and that seems to work fine! I guess it is a bit less secure then running mktemp on the destination as this way you assume there is no (important) file on the remote which has the same name as the destinationfile with the generated object marker, but I would find that a very reasonable assumption. BTW, did do the mv first in the gencode-remote before the chown to prevent issue's with changing only ownership/modes for an already existing file. I don't think that should be an issue, as all generated code from gencode-remote is run in a single ssh session, correct? Regards, Mark
steven commented 2022-04-06 15:22:36 +00:00
Collaborator
Copy link

Re IPC, this would work, not?

[16:06:13] eos:__file% git d gencode-local 
diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local
index 231b6927..ba2a8ab1 100755
--- a/cdist/conf/type/__file/gencode-local
+++ b/cdist/conf/type/__file/gencode-local
@@ -92,6 +92,7 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then
       tempfile_template="${destination}.cdist.XXXXXXXXXX"
       cat << DONE
 destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")"
+echo "\$destination_upload" > "$__object/files/destination-upload"
 DONE
       if [ "$upload_file" ]; then
          echo upload >> "$__messages_out"
[16:06:16] eos:__file%

And then in __file/gencode-remote: (pseudo-code):

destination="/$__object_id"
destination_upload=$(cat "$__object/files/destination-upload")"

cat << DONE
rm -rf "$destination"
mv "$destination_upload" "$destination"
DONE

Just noticed that this can not work because gencode-remote is executed before code-local.

> Re IPC, this would work, not? > > ``` > [16:06:13] eos:__file% git d gencode-local > diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local > index 231b6927..ba2a8ab1 100755 > --- a/cdist/conf/type/__file/gencode-local > +++ b/cdist/conf/type/__file/gencode-local > @@ -92,6 +92,7 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then > tempfile_template="${destination}.cdist.XXXXXXXXXX" > cat << DONE > destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")" > +echo "\$destination_upload" > "$__object/files/destination-upload" > DONE > if [ "$upload_file" ]; then > echo upload >> "$__messages_out" > [16:06:16] eos:__file% > ``` > > And then in __file/gencode-remote: (pseudo-code): > > ``` > destination="/$__object_id" > destination_upload=$(cat "$__object/files/destination-upload")" > > cat << DONE > rm -rf "$destination" > mv "$destination_upload" "$destination" > DONE > ``` Just noticed that this can not work because gencode-remote is executed before code-local.
steven commented 2022-04-06 15:25:48 +00:00
Collaborator
Copy link

Hi Steven,

Tried the changes with __cdist_object_marker, and that seems to work fine! I guess it is a bit less secure then running mktemp on the destination as this way you assume there is no (important) file on the remote which has the same name as the destinationfile with the generated object marker, but I would find that a very reasonable assumption.

We could off course check ourself if the assumed upload destination already exists and error out before uploading. Then I'd say we're about as safe (or unsafe) as we were before.

BTW, did do the mv first in the gencode-remote before the chown to prevent issue's with changing only ownership/modes for an already existing file. I don't think that should be an issue, as all generated code from gencode-remote is run in a single ssh session, correct?

Regards,

Mark

> Hi Steven, > > Tried the changes with __cdist_object_marker, and that seems to work fine! I guess it is a bit less secure then running mktemp on the destination as this way you assume there is no (important) file on the remote which has the same name as the destinationfile with the generated object marker, but I would find that a very reasonable assumption. > We could off course check ourself if the assumed upload destination already exists and error out before uploading. Then I'd say we're about as safe (or unsafe) as we were before. > BTW, did do the mv first in the gencode-remote before the chown to prevent issue's with changing only ownership/modes for an already existing file. I don't think that should be an issue, as all generated code from gencode-remote is run in a single ssh session, correct? > > Regards, > > Mark
mark commented 2022-04-06 17:29:24 +00:00
Author
Contributor
Copy link

Hi Steven,

We could off course check ourself if the assumed upload destination already exists and error out before uploading. Then I'd say we're about as safe (or unsafe) as we were before.

Good point. Tested this. Initially I tried to do it by adding an explorer to the type. I think that would be the nicest place to check, but it seems that __cdist_object_marker is not (yet) available during the explorer stage.

So I reintroduced it through a $__remote_exec in the gencode-local, which seems to work fine, but is a bit "unclean" I think.

Regards,

Mark

Hi Steven, > We could off course check ourself if the assumed upload destination already exists and error out before uploading. Then I'd say we're about as safe (or unsafe) as we were before. Good point. Tested this. Initially I tried to do it by adding an explorer to the type. I think that would be the nicest place to check, but it seems that __cdist_object_marker is not (yet) available during the explorer stage. So I reintroduced it through a $__remote_exec in the gencode-local, which seems to work fine, but is a bit "unclean" I think. Regards, Mark
steven commented 2022-04-06 19:24:13 +00:00
Collaborator
Copy link

Why unclean? Something like this would be pretty ok I think:

[21:20:54] eos:__file% git d . | cat
diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local
index 231b6927..7c4a066c 100755
--- a/cdist/conf/type/__file/gencode-local
+++ b/cdist/conf/type/__file/gencode-local
@@ -89,10 +89,17 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then
       touch "$__object/files/set-attributes"
 
       # upload file to temp location
-      tempfile_template="${destination}.cdist.XXXXXXXXXX"
+      destination_upload="${destination}${__cdist_object_marker}"
       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
+}
 DONE
+      # Tell gencode-remote that it has to move our file to its
+      # final destination.
+      touch "$__object/files/file-uploaded"
+
       if [ "$upload_file" ]; then
          echo upload >> "$__messages_out"
          # IPv6 fix
@@ -103,12 +110,8 @@ DONE
              my_target_host="${__target_host}"
          fi
          cat << DONE
-$__remote_copy "$source" "${my_target_host}:\$destination_upload"
+$__remote_copy "$source" "${my_target_host}:${destination_upload}"
 DONE
       fi
-# move uploaded file into place
-cat << DONE
-$__remote_exec $__target_host "rm -rf \"$destination\"; mv \"\$destination_upload\" \"$destination\""
-DONE
    fi
 fi
diff --git a/cdist/conf/type/__file/gencode-remote b/cdist/conf/type/__file/gencode-remote
index f7a528fd..f256b66e 100755
--- a/cdist/conf/type/__file/gencode-remote
+++ b/cdist/conf/type/__file/gencode-remote
@@ -62,6 +62,11 @@ set_mode() {
 
 case "$state_should" in
     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
         #  clearing S_ISUID and S_ISGID bits (see chown(2))
         for attribute in group owner mode; do
[21:20:56] eos:__file%
Why unclean? Something like this would be pretty ok I think: ``` [21:20:54] eos:__file% git d . | cat diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local index 231b6927..7c4a066c 100755 --- a/cdist/conf/type/__file/gencode-local +++ b/cdist/conf/type/__file/gencode-local @@ -89,10 +89,17 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then touch "$__object/files/set-attributes" # upload file to temp location - tempfile_template="${destination}.cdist.XXXXXXXXXX" + destination_upload="${destination}${__cdist_object_marker}" 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 +} DONE + # Tell gencode-remote that it has to move our file to its + # final destination. + touch "$__object/files/file-uploaded" + if [ "$upload_file" ]; then echo upload >> "$__messages_out" # IPv6 fix @@ -103,12 +110,8 @@ DONE my_target_host="${__target_host}" fi cat << DONE -$__remote_copy "$source" "${my_target_host}:\$destination_upload" +$__remote_copy "$source" "${my_target_host}:${destination_upload}" DONE fi -# move uploaded file into place -cat << DONE -$__remote_exec $__target_host "rm -rf \"$destination\"; mv \"\$destination_upload\" \"$destination\"" -DONE fi fi diff --git a/cdist/conf/type/__file/gencode-remote b/cdist/conf/type/__file/gencode-remote index f7a528fd..f256b66e 100755 --- a/cdist/conf/type/__file/gencode-remote +++ b/cdist/conf/type/__file/gencode-remote @@ -62,6 +62,11 @@ set_mode() { case "$state_should" in 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 # clearing S_ISUID and S_ISGID bits (see chown(2)) for attribute in group owner mode; do [21:20:56] eos:__file% ```
steven commented 2022-04-06 19:25:58 +00:00
Collaborator
Copy link

Good point. Tested this. Initially I tried to do it by adding an explorer to the type. I think that would be the nicest place to check, but it seems that __cdist_object_marker is not (yet) available during the explorer stage.

An explorer is the wrong tool here. I think the time between running the explorer and the file uploading is to long.

> Good point. Tested this. Initially I tried to do it by adding an explorer to the type. I think that would be the nicest place to check, but it seems that __cdist_object_marker is not (yet) available during the explorer stage. An explorer is the wrong tool here. I think the time between running the explorer and the file uploading is to long.
steven commented 2022-04-06 19:49:54 +00:00
Collaborator
Copy link

Oops, forgot something:

Why unclean? Something like this would be pretty ok I think:

[21:20:54] eos:__file% git d . | cat
diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local
index 231b6927..7c4a066c 100755
--- a/cdist/conf/type/__file/gencode-local
+++ b/cdist/conf/type/__file/gencode-local
@@ -89,10 +89,17 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then
       touch "$__object/files/set-attributes"
 
       # upload file to temp location
-      tempfile_template="${destination}.cdist.XXXXXXXXXX"
+      destination_upload="${destination}${__cdist_object_marker}"
       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
+}

Should probably be something like this instead:

$__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"
}
Oops, forgot something: > Why unclean? Something like this would be pretty ok I think: > > ``` > [21:20:54] eos:__file% git d . | cat > diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local > index 231b6927..7c4a066c 100755 > --- a/cdist/conf/type/__file/gencode-local > +++ b/cdist/conf/type/__file/gencode-local > @@ -89,10 +89,17 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then > touch "$__object/files/set-attributes" > > # upload file to temp location > - tempfile_template="${destination}.cdist.XXXXXXXXXX" > + destination_upload="${destination}${__cdist_object_marker}" > 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 > +} Should probably be something like this instead: ``` $__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" } ```
mark commented 2022-04-07 04:14:06 +00:00
Author
Contributor
Copy link

Hi Steven,

Why unclean? Something like this would be pretty ok I think:

Because gencode-local now generates code that runs commands on the target, which should be done by gencode-remote.

The solution works fine, have not seen the previous lockout behaviour :)

Regards,

Mark

Hi Steven, > Why unclean? Something like this would be pretty ok I think: Because gencode-local now generates code that runs commands on the target, which should be done by gencode-remote. The solution works fine, have not seen the previous lockout behaviour :) Regards, Mark
steven commented 2022-04-07 05:58:07 +00:00
Collaborator
Copy link

Hi Steven,

Why unclean? Something like this would be pretty ok I think:

Because gencode-local now generates code that runs commands on the target, which should be done by gencode-remote.

True. But in this case there's just no other way.
The only alternative would be to execute code in gencode-* instead of code-* which would be even worse.

The solution works fine, have not seen the previous lockout behaviour :)

Cool!
Can you wrap it up and either re-use (force push to) this PR or create a new one so we can drive this one home?

Thanks!

> Hi Steven, > > > Why unclean? Something like this would be pretty ok I think: > > Because gencode-local now generates code that runs commands on the target, which should be done by gencode-remote. > True. But in this case there's just no other way. The only alternative would be to execute code in gencode-* instead of code-* which would be even worse. > The solution works fine, have not seen the previous lockout behaviour :) Cool! Can you wrap it up and either re-use (force push to) this PR or create a new one so we can drive this one home? Thanks!
mark added 1 commit 2022-04-07 07:08:49 +00:00 
change (credits to Steven Armstrong).
mark commented 2022-04-07 07:14:06 +00:00
Author
Contributor
Copy link

Hi Steven,

True. But in this case there's just no other way.
The only alternative would be to execute code in gencode-* instead of code-* which would be even worse.

Completely agree, that would be ugly :)

I implemented the diffs you proposed and did some retesting with my test manifest. All my tests execute ok (except the one with a space in the filename, but that one also failed on the original __file implementation, have to look into that further).

Cool!
Can you wrap it up and either re-use (force push to) this PR or create a new one so we can drive this one home?

I've commited the changes to git. I see that gitea updated the files in this PR, is that sufficient, or do I need to make some additional changes (not too familiair with gitea and updating a PR ;) ).

Regards,

Mark

Hi Steven, > True. But in this case there's just no other way. > The only alternative would be to execute code in gencode-* instead of code-* which would be even worse. Completely agree, that would be ugly :) I implemented the diffs you proposed and did some retesting with my test manifest. All my tests execute ok (except the one with a space in the filename, but that one also failed on the original __file implementation, have to look into that further). > Cool! > Can you wrap it up and either re-use (force push to) this PR or create a new one so we can drive this one home? I've commited the changes to git. I see that gitea updated the files in this PR, is that sufficient, or do I need to make some additional changes (not too familiair with gitea and updating a PR ;) ). Regards, Mark
steven commented 2022-04-07 07:15:25 +00:00
Collaborator
Copy link

Thanks Mark!

Can you please rebase on master (unless it did not change) and squash those commits so we have a cleaner history?

Then I'm all +1.

Cheers,
Steven

Thanks Mark! Can you please rebase on master (unless it did not change) and squash those commits so we have a cleaner history? Then I'm all +1. Cheers, Steven
mark force-pushed __file-atomic-attributes from ea4a2aac2d to 30193bdae0 2022-04-07 07:29:31 +00:00 Compare
nico reviewed 2022-04-07 07:30:02 +00:00
cdist/conf/type/__file/gencode-local
@ -95,0 +96,4 @@
exit 1
} || {
# Put a towel in place.
$__remote_exec $__target_host "umask 077; touch \"$destination_upload\""
nico commented 2022-04-07 07:30:02 +00:00
Owner
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
Collaborator
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
Owner
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
nico marked this conversation as resolved
nico commented 2022-04-07 07:35:55 +00:00
Owner
Copy link

Guys,

I think we are having a security problem here.

Let's say we __file /some/dir/foo and a user has write access to /some/dir and the user can run ps on the target system.

Thus the user (=attacker) knows our suffix by means of ps. She can monitor when we test for it (wait...) and afterwards create /some/dir/foo.oursuffix/ and thus make us copy into a directory.

Then until we do the mv, the directory can be replaced by a file of choice by the attacker.

Now, the question is whether this is a real problem or not, because it assumes that an attacker has write access to the directory we want to place a file in. If that is already the case, she can probably also delete our newly created file anyway.

So maybe this is actually a non-problem, but would like to shortly hear your thoughts on it, too.

Guys, I think we are having a security problem here. Let's say we `__file /some/dir/foo` and a user has write access to /some/dir and the user can run `ps` on the target system. Thus the user (=attacker) knows our suffix by means of ps. She can monitor when we test for it (wait...) and afterwards create /some/dir/foo.oursuffix/ and thus make us copy into a directory. Then until we do the mv, the directory can be replaced by a file of choice by the attacker. Now, the question is whether this is a real problem or not, because it assumes that an attacker has write access to the directory we want to place a file in. If that is already the case, she can probably also delete our newly created file anyway. So maybe this is actually a non-problem, but would like to shortly hear your thoughts on it, too.
steven commented 2022-04-07 08:07:06 +00:00
Collaborator
Copy link

Guys,

I think we are having a security problem here.

Let's say we __file /some/dir/foo and a user has write access to /some/dir and the user can run ps on the target system.

Thus the user (=attacker) knows our suffix by means of ps. She can monitor when we test for it (wait...) and afterwards create /some/dir/foo.oursuffix/ and thus make us copy into a directory.

Then until we do the mv, the directory can be replaced by a file of choice by the attacker.

Now, the question is whether this is a real problem or not, because it assumes that an attacker has write access to the directory we want to place a file in. If that is already the case, she can probably also delete our newly created file anyway.

So maybe this is actually a non-problem, but would like to shortly hear your thoughts on it, too.

I don't think this problem is solvable given the current implementation of cdist.

Maybe using __cdist_object_marker makes this worse, as it's already visible long in ps on the target long before it's first used by __file. So an attacker has more time.

Instead of (re|ab)using __cdist_object_marker we could:

Generate a random file name in gencode-local (NOT code-local).
Store it in __object/files/file-name or something.
Grab that name in gencode-remote (NOT core-remote).

But this soon ends up in bikesheding as the only way to solve this for good would be ditch ssh/shell and use some rpc or the like that does not leek this information in the env or in the process tree.

> Guys, > > I think we are having a security problem here. > > Let's say we `__file /some/dir/foo` and a user has write access to /some/dir and the user can run `ps` on the target system. > > Thus the user (=attacker) knows our suffix by means of ps. She can monitor when we test for it (wait...) and afterwards create /some/dir/foo.oursuffix/ and thus make us copy into a directory. > > Then until we do the mv, the directory can be replaced by a file of choice by the attacker. > > Now, the question is whether this is a real problem or not, because it assumes that an attacker has write access to the directory we want to place a file in. If that is already the case, she can probably also delete our newly created file anyway. > > So maybe this is actually a non-problem, but would like to shortly hear your thoughts on it, too. I don't think this problem is solvable given the current implementation of cdist. Maybe using __cdist_object_marker makes this worse, as it's already visible long in ps on the target long before it's first used by __file. So an attacker has more time. Instead of (re|ab)using __cdist_object_marker we could: Generate a random file name in gencode-local (NOT code-local). Store it in __object/files/file-name or something. Grab that name in gencode-remote (NOT core-remote). But this soon ends up in bikesheding as the only way to solve this for good would be ditch ssh/shell and use some rpc or the like that does not leek this information in the env or in the process tree.
mark commented 2022-04-07 09:19:53 +00:00
Author
Contributor
Copy link

Hi Nico,

Let's say we __file /some/dir/foo and a user has write access to /some/dir and the user can run ps on the target system.

Thus the user (=attacker) knows our suffix by means of ps. She can monitor when we test for it (wait...) and afterwards create /some/dir/foo.oursuffix/ and thus make us copy into a directory.

Then until we do the mv, the directory can be replaced by a file of choice by the attacker.

Now, the question is whether this is a real problem or not, because it assumes that an attacker has write access to the directory we want to place a file in. If that is already the case, she can probably also delete our newly created file anyway.

So maybe this is actually a non-problem, but would like to shortly hear your thoughts on it, too.

Depending on the target (its security profile) and the type of file, an option would be to just overwrite the existing file. This would mitigate this issue. But it would not be a global solution. The behaviour could be easily selectable by an extra boolean option to the __file type.

Regards,

Mark

Hi Nico, > Let's say we `__file /some/dir/foo` and a user has write access to /some/dir and the user can run `ps` on the target system. > > Thus the user (=attacker) knows our suffix by means of ps. She can monitor when we test for it (wait...) and afterwards create /some/dir/foo.oursuffix/ and thus make us copy into a directory. > > Then until we do the mv, the directory can be replaced by a file of choice by the attacker. > > Now, the question is whether this is a real problem or not, because it assumes that an attacker has write access to the directory we want to place a file in. If that is already the case, she can probably also delete our newly created file anyway. > > So maybe this is actually a non-problem, but would like to shortly hear your thoughts on it, too. Depending on the target (its security profile) and the type of file, an option would be to just overwrite the existing file. This would mitigate this issue. But it would not be a global solution. The behaviour could be easily selectable by an extra boolean option to the __file type. Regards, Mark
mark commented 2022-04-10 08:05:23 +00:00
Author
Contributor
Copy link

Are there any changes you want me to make or test for this pull request? Assuming it is acceptable security wise?

Regards,

Mark

Are there any changes you want me to make or test for this pull request? Assuming it is acceptable security wise? Regards, Mark
steven commented 2022-04-10 11:06:33 +00:00
Collaborator
Copy link

I think if we do it like this, we are as safe as we can be.

[13:00:54] eos:__file% git d . | cat
diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local
index 231b6927..99750648 100755
--- a/cdist/conf/type/__file/gencode-local
+++ b/cdist/conf/type/__file/gencode-local
@@ -89,10 +89,19 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then
       touch "$__object/files/set-attributes"
 
       # upload file to temp location
-      tempfile_template="${destination}.cdist.XXXXXXXXXX"
+      destination_upload="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")"
       cat << DONE
-destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")"
+$__remote_exec $__target_host "umask 777; test -e $destination_upload && exit 1 || touch $destination_upload" || {
+   echo "Refusing to upload file to existing destination: $destination_upload" >&2
+   exit 1
+}
 DONE
+      # Tell gencode-remote that it has to move our file to its
+      # final destination.
+      touch "$__object/files/file-uploaded"
+      # Tell gencode-remote to where we uploaded the file.
+      echo "$destination_upload" > "$__object/files/destination-upload"
+
       if [ "$upload_file" ]; then
          echo upload >> "$__messages_out"
          # IPv6 fix
@@ -103,12 +112,8 @@ DONE
              my_target_host="${__target_host}"
          fi
          cat << DONE
-$__remote_copy "$source" "${my_target_host}:\$destination_upload"
+$__remote_copy "$source" "${my_target_host}:${destination_upload}"
 DONE
       fi
-# move uploaded file into place
-cat << DONE
-$__remote_exec $__target_host "rm -rf \"$destination\"; mv \"\$destination_upload\" \"$destination\""
-DONE
    fi
 fi
diff --git a/cdist/conf/type/__file/gencode-remote b/cdist/conf/type/__file/gencode-remote
index f7a528fd..d00a46c9 100755
--- a/cdist/conf/type/__file/gencode-remote
+++ b/cdist/conf/type/__file/gencode-remote
@@ -62,6 +62,12 @@ set_mode() {
 
 case "$state_should" in
     present|exists)
+        if [ -f "$__object/files/file-uploaded" ]; then
+            destination_upload="$(cat "$__object/files/destination-upload")"
+            # move uploaded file into place
+            printf 'rm -rf "%s"\n' "$destination"
+            printf 'mv "%s" "%s"\n' "$destination_upload" "$destination"
+        fi
         # Note: Mode - needs to happen last as a chown/chgrp can alter mode by
         #  clearing S_ISUID and S_ISGID bits (see chown(2))
         for attribute in group owner mode; do
[13:01:05] eos:__file%

If I'm not mistaken an attacker has no way/time to predict or react.

@nico RFC, let's get this PR in asap.

I think if we do it like this, we are as safe as we can be. ``` [13:00:54] eos:__file% git d . | cat diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local index 231b6927..99750648 100755 --- a/cdist/conf/type/__file/gencode-local +++ b/cdist/conf/type/__file/gencode-local @@ -89,10 +89,19 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then touch "$__object/files/set-attributes" # upload file to temp location - tempfile_template="${destination}.cdist.XXXXXXXXXX" + destination_upload="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")" cat << DONE -destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")" +$__remote_exec $__target_host "umask 777; test -e $destination_upload && exit 1 || touch $destination_upload" || { + echo "Refusing to upload file to existing destination: $destination_upload" >&2 + exit 1 +} DONE + # Tell gencode-remote that it has to move our file to its + # final destination. + touch "$__object/files/file-uploaded" + # Tell gencode-remote to where we uploaded the file. + echo "$destination_upload" > "$__object/files/destination-upload" + if [ "$upload_file" ]; then echo upload >> "$__messages_out" # IPv6 fix @@ -103,12 +112,8 @@ DONE my_target_host="${__target_host}" fi cat << DONE -$__remote_copy "$source" "${my_target_host}:\$destination_upload" +$__remote_copy "$source" "${my_target_host}:${destination_upload}" DONE fi -# move uploaded file into place -cat << DONE -$__remote_exec $__target_host "rm -rf \"$destination\"; mv \"\$destination_upload\" \"$destination\"" -DONE fi fi diff --git a/cdist/conf/type/__file/gencode-remote b/cdist/conf/type/__file/gencode-remote index f7a528fd..d00a46c9 100755 --- a/cdist/conf/type/__file/gencode-remote +++ b/cdist/conf/type/__file/gencode-remote @@ -62,6 +62,12 @@ set_mode() { case "$state_should" in present|exists) + if [ -f "$__object/files/file-uploaded" ]; then + destination_upload="$(cat "$__object/files/destination-upload")" + # move uploaded file into place + printf 'rm -rf "%s"\n' "$destination" + printf 'mv "%s" "%s"\n' "$destination_upload" "$destination" + fi # Note: Mode - needs to happen last as a chown/chgrp can alter mode by # clearing S_ISUID and S_ISGID bits (see chown(2)) for attribute in group owner mode; do [13:01:05] eos:__file% ``` If I'm not mistaken an attacker has no way/time to predict or react. @nico RFC, let's get this PR in asap.
nico commented 2022-04-10 12:15:36 +00:00
Owner
Copy link

LGTM - please go ahead. I'd suggest we make a major release out of this one, as changes on the file type affect basically everyone. So if we screwed something up, we have at least an indicator for everyone.

Also, 7.0.0 is a very nice release name.

LGTM - please go ahead. I'd suggest we make a major release out of this one, as changes on the file type affect basically everyone. So if we screwed something up, we have at least an indicator for everyone. Also, 7.0.0 is a very nice release name.
matze commented 2022-04-10 14:09:06 +00:00
First-time contributor
Copy link

After reading through the comments and start skimming through in the middle cause of the heavy discussion happen there - it's really hard to follow.

Reading the first part, I would suggest using a predictive filename (so it won't leave orphanded files on aborts, like it's done in __download) and do the move in code-remote. Maybe the move happens after permissions are correctly set to the tempfile.

As mentioned, it's hard to follow the discussion of which higher scince/magic blocked simple solutions, and looking at stevens latest patch looks very clunky for the cdist approch.

Targeting the security considerations, we create a root file and override a root- or user-file. The attacer must be root (where we already lost the war) or the file owner/group, which don't lead to privileage escalation (at minimum when we change permissions before the move). He just can do what he already can. And if you are worried about moving files to a directory destination, please use mv -T. If moving files with cdist is a problem, we have a big one for more cdist types that should be handled separately.

In the end, I just wan't to call for a simple and cdist-elegant solution, what Nico and Steven already used to block other MR's. Please elaborate the current state/problem and why an simple and cdist-like solution is (maybe) impossible. Or maybe going back to start and try a simpler solution.

After reading through the comments and start skimming through in the middle cause of the heavy discussion happen there - it's really hard to follow. Reading the first part, I would suggest using a predictive filename (so it won't leave orphanded files on aborts, like it's done in `__download`) and do the move in `code-remote`. Maybe the move happens after permissions are correctly set to the tempfile. As mentioned, it's hard to follow the discussion of which higher scince/magic blocked simple solutions, and looking at stevens latest patch looks very clunky for the cdist approch. Targeting the security considerations, we create a root file and override a root- or user-file. The attacer must be root (where we already lost the war) or the file owner/group, which don't lead to privileage escalation (at minimum when we change permissions before the move). He just can do what he already can. And if you are worried about moving files to a directory destination, please use `mv -T`. If moving files with cdist is a problem, we have a big one for more cdist types that should be handled separately. **In the end, I just wan't to call for a simple and cdist-elegant solution**, what Nico and Steven already used to block other MR's. Please elaborate the current state/problem and why an simple and cdist-like solution is (maybe) impossible. Or maybe going back to start and try a simpler solution.
steven commented 2022-04-10 15:10:09 +00:00
Collaborator
Copy link

@matze tl;dr:
the problem that @mark is having is that he is defining a __file with cdist which
is used by his ssh config.

The current implentation of cdist's __file type needs 2 ssh connections to

  1. upload the file and move it into place (code-local)
  2. set the files attributes (code-remote)

The second ssh connection fails for him, because the first one uploaded a new file but did not set the attributes. e.g. the file is there but not executable.

The proposed patch fixes that by making all code that has effects on the original file happen in a single ssh connection.

@matze tl;dr: the problem that @mark is having is that he is defining a __file with cdist which is used by his ssh config. The current implentation of cdist's __file type needs 2 ssh connections to 1) upload the file and move it into place (code-local) 2) set the files attributes (code-remote) The second ssh connection fails for him, because the first one uploaded a new file but did not set the attributes. e.g. the file is there but not executable. The proposed patch fixes that by making all code that has effects on the original file happen in a single ssh connection.
steven commented 2022-04-10 15:26:23 +00:00
Collaborator
Copy link

This is the cleanest/best solution I can think of.

  • upload file to tmp location
  • set attributes on the uploaded file
  • move uploaded file to its final destination

The one thing I'm not 100% sure of is:
Do we have to delete the existing file before moving or
does mv -f handle any possible case for us.

[17:22:32] eos:__file% git d . | cat   
diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local
index 231b6927..9c84c855 100755
--- a/cdist/conf/type/__file/gencode-local
+++ b/cdist/conf/type/__file/gencode-local
@@ -89,10 +89,17 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then
       touch "$__object/files/set-attributes"
 
       # upload file to temp location
-      tempfile_template="${destination}.cdist.XXXXXXXXXX"
+      upload_destination="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")"
       cat << DONE
-destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")"
+$__remote_exec $__target_host "umask 777; test -e $upload_destination && exit 1 || touch $upload_destination" || {
+   echo "Refusing to upload file to existing destination: $upload_destination" >&2
+   exit 1
+}
 DONE
+      # Tell gencode-remote to where we uploaded the file so
+      # it can move it to its final destination.
+      echo "$upload_destination" > "$__object/files/upload-destination"
+
       if [ "$upload_file" ]; then
          echo upload >> "$__messages_out"
          # IPv6 fix
@@ -103,12 +110,8 @@ DONE
              my_target_host="${__target_host}"
          fi
          cat << DONE
-$__remote_copy "$source" "${my_target_host}:\$destination_upload"
+$__remote_copy "$source" "${my_target_host}:${upload_destination}"
 DONE
       fi
-# move uploaded file into place
-cat << DONE
-$__remote_exec $__target_host "rm -rf \"$destination\"; mv \"\$destination_upload\" \"$destination\""
-DONE
    fi
 fi
diff --git a/cdist/conf/type/__file/gencode-remote b/cdist/conf/type/__file/gencode-remote
index f7a528fd..ab3f27b6 100755
--- a/cdist/conf/type/__file/gencode-remote
+++ b/cdist/conf/type/__file/gencode-remote
@@ -62,6 +62,13 @@ set_mode() {
 
 case "$state_should" in
     present|exists)
+        if [ -f "$__object/files/upload-destination" ]; then
+            # We change attributes of the new/uploaded file before moving it
+            # to it's final destination.
+            final_destination="$destination"
+            # Note: changing 'global' variable here.
+            destination="$(cat "$__object/files/upload-destination")"
+        fi
         # Note: Mode - needs to happen last as a chown/chgrp can alter mode by
         #  clearing S_ISUID and S_ISGID bits (see chown(2))
         for attribute in group owner mode; do
@@ -81,6 +88,12 @@ case "$state_should" in
                 fi
             fi
         done
+        if [ -f "$__object/files/upload-destination" ]; then
+            # move uploaded file into place
+            # FIXME: are there any use cases that require the file to be deleted first?
+            #printf 'rm -rf "%s"\n' "$destination"
+            printf 'mv -T -f "%s" "%s"\n' "$destination" "$final_destination"
+        fi
         if [ -f "$__object/files/set-attributes" ]; then
             # set-attributes is created if file is created or uploaded in gencode-local
             fire_onchange=1
[17:22:33] eos:__file%
This is the cleanest/best solution I can think of. - upload file to tmp location - set attributes on the **uploaded file** - move uploaded file to its final destination The one thing I'm not 100% sure of is: Do we have to delete the existing file before moving or does `mv -f` handle any possible case for us. ``` [17:22:32] eos:__file% git d . | cat diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local index 231b6927..9c84c855 100755 --- a/cdist/conf/type/__file/gencode-local +++ b/cdist/conf/type/__file/gencode-local @@ -89,10 +89,17 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then touch "$__object/files/set-attributes" # upload file to temp location - tempfile_template="${destination}.cdist.XXXXXXXXXX" + upload_destination="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")" cat << DONE -destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")" +$__remote_exec $__target_host "umask 777; test -e $upload_destination && exit 1 || touch $upload_destination" || { + echo "Refusing to upload file to existing destination: $upload_destination" >&2 + exit 1 +} DONE + # Tell gencode-remote to where we uploaded the file so + # it can move it to its final destination. + echo "$upload_destination" > "$__object/files/upload-destination" + if [ "$upload_file" ]; then echo upload >> "$__messages_out" # IPv6 fix @@ -103,12 +110,8 @@ DONE my_target_host="${__target_host}" fi cat << DONE -$__remote_copy "$source" "${my_target_host}:\$destination_upload" +$__remote_copy "$source" "${my_target_host}:${upload_destination}" DONE fi -# move uploaded file into place -cat << DONE -$__remote_exec $__target_host "rm -rf \"$destination\"; mv \"\$destination_upload\" \"$destination\"" -DONE fi fi diff --git a/cdist/conf/type/__file/gencode-remote b/cdist/conf/type/__file/gencode-remote index f7a528fd..ab3f27b6 100755 --- a/cdist/conf/type/__file/gencode-remote +++ b/cdist/conf/type/__file/gencode-remote @@ -62,6 +62,13 @@ set_mode() { case "$state_should" in present|exists) + if [ -f "$__object/files/upload-destination" ]; then + # We change attributes of the new/uploaded file before moving it + # to it's final destination. + final_destination="$destination" + # Note: changing 'global' variable here. + destination="$(cat "$__object/files/upload-destination")" + fi # Note: Mode - needs to happen last as a chown/chgrp can alter mode by # clearing S_ISUID and S_ISGID bits (see chown(2)) for attribute in group owner mode; do @@ -81,6 +88,12 @@ case "$state_should" in fi fi done + if [ -f "$__object/files/upload-destination" ]; then + # move uploaded file into place + # FIXME: are there any use cases that require the file to be deleted first? + #printf 'rm -rf "%s"\n' "$destination" + printf 'mv -T -f "%s" "%s"\n' "$destination" "$final_destination" + fi if [ -f "$__object/files/set-attributes" ]; then # set-attributes is created if file is created or uploaded in gencode-local fire_onchange=1 [17:22:33] eos:__file% ```
matze commented 2022-04-10 16:18:44 +00:00
First-time contributor
Copy link

I've got those points, too. It's not hard to understand the real problem, but the big discussion in the middle of this PR. About security and predictive filenames, that it's posible to modify things in code-local and code-remote, ... (it was the point I started to skip through)

Since all times (at least I think), __file uses a random tempfile. Lately, I've wrote a patch for __download in https://github.com/cdist-community/cdist-conf/pull/28 to change tempfile behaviour. I've discovered, that it uses a single predictive tempfile and adopted my change to do the same. What I've found out, is that you can get a lot of orphanded tempfiles if the type will fail. Separating changes into code-local and code-remote, this error point will grow, because it will be more likely a paralell execution can abort the __file processes, too (if I understand the cdist paralism correctly).

Because of the following reasons ..

  1. to avoid orphanded tempfiles
  2. to easy predict the temp filename
  3. where the predict is possible as the object ID garantees there will be no duplicate tempfiles

.. I propose to make a determant predictive tempfile like ${__object_id}__file_tmp which solves the ugly $__remote_exec in gencode-local with the use of dry-run tempfiles (mktemp -u) shared to the gencode-remote and instead provide a pretty and simple solution.

The move of the tempfile to the real destination should happen after all file attribute changes happend, so it's non-interruptive to any application that relies on the file. If the file at the end is really moved by rm -rf $dst; mv $tmp $dst or with a single mv command doesn't really matter.

In the great disscussion of this MR, there might was a security consideration against such a predictive filename, which should be checked against if this is a realy problem for this suggestion.

I've got those points, too. It's not hard to understand the real problem, but the big discussion in the middle of this PR. About security and predictive filenames, that it's posible to modify things in `code-local` and `code-remote`, ... (*it was the point I started to skip through*) Since all times (at least I think), `__file` uses a random tempfile. Lately, I've wrote a patch for `__download` in [https://github.com/cdist-community/cdist-conf/pull/28](cdist-community#28) to change tempfile behaviour. I've discovered, that it uses a single predictive tempfile and adopted my change to do the same. What I've found out, is that you can get a lot of orphanded tempfiles if the type will fail. Separating changes into `code-local` and `code-remote`, this error point will grow, because it will be more likely a paralell execution can abort the `__file` processes, too (if I understand the cdist paralism correctly). Because of the following reasons .. 1. to avoid orphanded tempfiles 2. to easy predict the temp filename 3. where the predict is possible as the object ID garantees there will be no duplicate tempfiles .. I propose to make a determant predictive tempfile like `${__object_id}__file_tmp` which solves the ugly `$__remote_exec` in `gencode-local` with the use of dry-run tempfiles (`mktemp -u`) shared to the `gencode-remote` and instead provide a pretty and simple solution. The move of the tempfile to the real destination should happen after all file attribute changes happend, so it's non-interruptive to any application that relies on the file. If the file at the end is really moved by `rm -rf $dst; mv $tmp $dst` or with a single `mv` command doesn't really matter. In the great disscussion of this MR, there might was a security consideration against such a predictive filename, which should be checked against if this is a realy problem for this suggestion.
mark commented 2022-04-10 17:29:27 +00:00
Author
Contributor
Copy link

Sorry for the late response.

I think any security concerns are related to the temporary filename that is
going to be used would already exist on the target (like Nico's example of
creating a directory).

The current approach is to use mktemp to generate a safe filename and reserving
it on the filesystem, negating this problem, but not being able to pass the
filename from code generated by gencode-local to code from gencode-remote.

The other is to not check the target system and append something to the
filename in order to make it unique. The last one is a game of chance.

So I think it would come down to how much randomness do we add to the filename
in order to make it unique enough to withstand an attach. With (I think) at
least 255 characters possible in a filename, it should be possible to generate
something quite unique? Append a 50 character suffix to a file (with
a-zA-Z0-9) should already give about 300bits of uniqueness. Which is pretty
overkill IMHO..

As long as the uniqueness is not generated (and not visible until file
creation) on the target, I think that should be pretty ok.

And that was (as far as I can see) basically what the last patch of Steven
did, using mktemp on the server side in order to generate the file. From
a security perspective it might be better not to check the upload destination,
that could give away the filename. Just upload it through the code generated
by gencode-local. As long as the filename used in gencode-local can be passed
to gencode-remote, so it can change the attributes and move it in place it
should be ok.

Not checking the destination of the filename with the very random name already
exists would (for me) be a very acceptable risk. Chances of that happening are
pretty remote.

Regards,

Mark

Sorry for the late response. I think any security concerns are related to the temporary filename that is going to be used would already exist on the target (like Nico's example of creating a directory). The current approach is to use mktemp to generate a safe filename and reserving it on the filesystem, negating this problem, but not being able to pass the filename from code generated by gencode-local to code from gencode-remote. The other is to not check the target system and append something to the filename in order to make it unique. The last one is a game of chance. So I think it would come down to how much randomness do we add to the filename in order to make it unique enough to withstand an attach. With (I think) at least 255 characters possible in a filename, it should be possible to generate something quite unique? Append a 50 character suffix to a file (with a-zA-Z0-9) should already give about 300bits of uniqueness. Which is pretty overkill IMHO.. As long as the uniqueness is not generated (and not visible until file creation) on the target, I think that should be pretty ok. And that was (as far as I can see) basically what the last patch of Steven did, using mktemp on the server side in order to generate the file. From a security perspective it might be better not to check the upload destination, that could give away the filename. Just upload it through the code generated by gencode-local. As long as the filename used in gencode-local can be passed to gencode-remote, so it can change the attributes and move it in place it should be ok. Not checking the destination of the filename with the very random name already exists would (for me) be a very acceptable risk. Chances of that happening are pretty remote. Regards, Mark
steven commented 2022-04-10 19:31:03 +00:00
Collaborator
Copy link

@matze

# ls -al /etc/something-important
-r-------- 1 root root 15 Apr 10 21:00 /etc/something-important
# cat /etc/something-important
important-file
#

% mkdir /tmp/test
% ls -al /tmp/test
total 0
drwxrwxr-x  2 sar  sar   40 Apr 10 21:05 .
drwxrwxrwt 25 root root 560 Apr 10 21:05 ..
%

% touch /tmp/test/my-file
% sudo touch /tmp/test/my-file__file_tmp

% ls -al /tmp/test/
total 0
drwxrwxr-x  2 sar  sar   80 Apr 10 21:07 .
drwxrwxrwt 25 root root 560 Apr 10 21:05 ..
-rw-rw-r--  1 sar  sar    0 Apr 10 21:05 my-file
-rw-r--r--  1 root root   0 Apr 10 21:07 my-file__file_tmp

% whoami
sar
% rm -f /tmp/test/my-file__file_tmp
% ln -s /etc/something-important /tmp/test/my-file__file_tmp

% ls -al /tmp/test
total 0
drwxrwxr-x  2 sar  sar   80 Apr 10 21:10 .
drwxrwxrwt 25 root root 560 Apr 10 21:05 ..
-rw-rw-r--  1 sar  sar    0 Apr 10 21:05 my-file
lrwxrwxrwx  1 sar  sar   24 Apr 10 21:10 my-file__file_tmp -> /etc/something-important
%

now cdist comes and uploads the changed my-file to your predictive filename

# echo "not what you want" > /tmp/test/my-file__file_tmp
# cat /etc/something-important
not what you want
#
@matze ``` # ls -al /etc/something-important -r-------- 1 root root 15 Apr 10 21:00 /etc/something-important # cat /etc/something-important important-file # ``` ``` % mkdir /tmp/test % ls -al /tmp/test total 0 drwxrwxr-x 2 sar sar 40 Apr 10 21:05 . drwxrwxrwt 25 root root 560 Apr 10 21:05 .. % % touch /tmp/test/my-file % sudo touch /tmp/test/my-file__file_tmp % ls -al /tmp/test/ total 0 drwxrwxr-x 2 sar sar 80 Apr 10 21:07 . drwxrwxrwt 25 root root 560 Apr 10 21:05 .. -rw-rw-r-- 1 sar sar 0 Apr 10 21:05 my-file -rw-r--r-- 1 root root 0 Apr 10 21:07 my-file__file_tmp % whoami sar % rm -f /tmp/test/my-file__file_tmp % ln -s /etc/something-important /tmp/test/my-file__file_tmp % ls -al /tmp/test total 0 drwxrwxr-x 2 sar sar 80 Apr 10 21:10 . drwxrwxrwt 25 root root 560 Apr 10 21:05 .. -rw-rw-r-- 1 sar sar 0 Apr 10 21:05 my-file lrwxrwxrwx 1 sar sar 24 Apr 10 21:10 my-file__file_tmp -> /etc/something-important % ``` now cdist comes and uploads the changed my-file to your predictive filename ``` # echo "not what you want" > /tmp/test/my-file__file_tmp # cat /etc/something-important not what you want # ```
steven commented 2022-04-10 20:31:43 +00:00
Collaborator
Copy link

I'll use this for a while to see if there are any side effects.

bdc8e64df0

I'll use this for a while to see if there are any side effects. https://github.com/asteven/cdist/commit/bdc8e64df0acd2b88623550492c8c338d7479793
matze commented 2022-04-10 20:43:45 +00:00
First-time contributor
Copy link

thanks Mark, currently thinking about how this can be a problem
also, thanks Steven, I've saw your comment while writing mine (and already found this, too)

TL;DR

I've wrote some thoughs about the security problem that wasn't really described at the time I've started this writing. At the end, I've added some - in my mind - prettier solution, which might be used cdist-globaly.

Important sections you shouldn't miss:

  • security conclusion
  • solution compling with cdist principles
  • alternative cdist-wide solution

But it's now getting late and I should go to bed (didn't expect it would need that long) - good night :-)

possible security problems

Remembering the security findings in please, the most exploiting way would be still only to interfer, but not to actively exploit something. If you can predict the filename of the uploaded file, you can create a symlink to mostly any location on the filesystem. As root writes this file, it will almost work every time. Also the file will be in the mode 0600, so no other user can read the file. Concratulation, you did a DOS by overwriting a maybe important file with some crap (as you can't control the user-defined content). Only in edge-cases, you can really use the file redirection to do some evil. That it will gain broad usage, it must be possible with a public core-type.

As Nico and Mark said, with prior knowleage of the filename, you can create a folder with the excat same name. scp or sftp will copy the file inside the folder all the times. In an atomar call (doing nothing else than a single remote copy), you can't influence anything - it will copy the file inside the folder every time. After this, permission changes are applied to the folder, but not to the file, means the file stay root with mode 0600. The attacker can't write or even read the file. The directory will be moved to the destination. Program which wan't to read this file fails. DOS. Nothing more.

Last option works if the attacker has permission for the folder. Then, he can move or even delete the temp file or even the original file. Linux permissions and nothing special.

security conclusion

That said, only the symlink thing can theoretical be used for exploits if you know the content that will be copied over and if it can be even used for it (file is still 0600, so not read- or writeable). Else you can only interrupt the file process so it won't be able to automaticly restore. The DOS maybe even just happen only after a service reload/restart.

To perform all the DOSes, the attacker needs a user which can write to the folder. Then, it should be able to write to or move the file he needs to by himself. Only the exploit with symlinks explicitly requires cdist to work, which is the only convincing argument I've found. Protecting against the other problems isn't worth it, as it makes no difference if cdist is involved with.

how to avoid

How to avoid this exploit? The current live __file implementation uses both:

  1. using a random file which is hard to guess or to bruteforce (tempfile to copy to)
  2. directly before the move/copy, clear the path (move to destination)

Noting that there are doubts in this MR that 1. isn't really save because of the "long" time between the tempfile is created and it will be copied over.

An easy way of avoiding the above problem is to use a tool that is aware of this. This is nice, because we will never get the level of an atomar filesystem action as mkdir has and is often used for locks, but it will get better. Instead of relying on the shell and kernel that it forks the two commands directly after, it's only one fork. Not perfect, but theoretical less effort/time inbetween these checks. I've wrote to much, because now tested that cp or mv will never write to the symlink destination, rather replacing it. Still though about mv -T which won't override folders ...

Symlink problems happen with scp; both with the old SCP and the new sftp protocol. This means, the previously suggested predictive filenames from me means death. At least when you assosiate DOS = death. No, serious, I know understand why they shouldn't be used now. This also means @ander needs to refactor his __download and co.

But now coming back utilising the shortest time for changes: I've noticed if we use sftp instead of scp in command line mode, we can batch things in a single command. I'm pretty sure we can script this with passing the commands to stdin. Then, we can script a rm pretty close to an put, utilise the mv, or something similar. Even things need to go over the network, it is better than simply doing a $__remote_exec and then a $__remote_copy or soemthing similar ...

other funny things we can do (but better not)

Which should work, too, is to use piping from ssh. Like this:

# code-local

tmp=${dst}__file_tmp
cat $local | $__remote_exec "umask 066; rm -rf $tmp ; cat > $tmp"

Will work, and at least I think ssh piping should be POSIX-compatible, but might be an hard abuse of it ...

The other thinks are HERE-documents, like this:

# code-remote

tmp=${dst}__file_tmp
umask 066
rm -rf $tmp
cat > $tmp <<FILE
$(cat $magic_local)
FILE

I don't propose this seriously, but it came to my mind, too. If special characters are making trouble in the HERE-document, we can use base64 or some accient email transcoding which served the same usecase (to embed binary content into ascii emails, which usually increased size by 20% :P). Don't think we can solve it with POSIX compatibility.

The idea was to have more control writing it with piping. It's still vurnable to a symlink cause of the write, but the timeframe should be low. As indicated by the title, we should better skip them. Others will do better ;-)

solution compling with cdist principles

After all this wall of text in this MR disscussion, I will finally present the way I've found that should comply (after all the info I've got by writing this comment): Copy the file with an hardly predictive file name so nobody whats to trap us with a symlink. Then move the file to somewhat predictive. As mv overrides symlinks instead of writing to, this will be save. Then, we can chill and wait till an other script (code-remote in this case) picks up the file with no real effort guessing the name. And a move in the same directory doesn't really cost anything.

Example (you know how I mean the pseudo script):

# code-local

random=$(mktemp -u cdist.XXXXXXXXXXXXXXXXXXXX)  # can be longer, just need to be random
remote=$(dirname $dst)/$random

$__remote_copy $local $remote
$__remote_exec mv $remote ${dst}__file_tmp

# code-remote

# modify ${dst}__file_tmp
mv ${dst}__file_tmp $dst

alternative cdist-wide solution

The above solution will work based on unpredictability, just like Mark suggested it and was already in discussion. But it will require to change every other type utilising $__remote_copy. Idealy, they shoud use the __file type, but others are around, too, like __download and more. By writing the sftp section, I thought about such a general script replacing $__remote_copy.

This means a script utilising sftp with an fallback for scp. If scp is required, we can make the same as with sftp, which doesn't really drop the security level.

The only problem is, that the logic is very general - but we make life difficult when somebody whats to use his own $__remote_copy with the same security. Maybe stacking of different $__remote_copy's - Ander would like it combine it with the sudo $__remote_copy.

thanks Mark, currently thinking about how this can be a problem also, thanks Steven, I've saw your comment while writing mine (and already found this, too) ## TL;DR I've wrote some thoughs about the security problem that wasn't really described at the time I've started this writing. At the end, I've added some - in my mind - prettier solution, which might be used cdist-globaly. **Important sections you shouldn't miss:** - security conclusion - solution compling with cdist principles - alternative cdist-wide solution But it's now getting late and I should go to bed (didn't expect it would need that long) - good night :-) ## possible security problems Remembering the [security findings in `please`](https://seclists.org/oss-sec/2021/q2/152), the most exploiting way would be still only to interfer, but not to actively exploit something. If you can predict the filename of the uploaded file, you can create a symlink to mostly any location on the filesystem. As `root` writes this file, it will almost work every time. Also the file will be in the mode `0600`, so no other user can read the file. Concratulation, you did a DOS by overwriting a maybe important file with some crap (as you can't control the user-defined content). Only in edge-cases, you can really use the file redirection to do some evil. That it will gain broad usage, it must be possible with a public core-type. As Nico and Mark said, with prior knowleage of the filename, you can create a folder with the excat same name. `scp` or `sftp` will copy the file inside the folder all the times. In an atomar call (doing nothing else than a single remote copy), you can't influence anything - it will copy the file inside the folder every time. After this, permission changes are applied to the folder, but not to the file, means the file stay `root` with mode `0600`. The attacker can't write or even read the file. The directory will be moved to the destination. Program which wan't to read this file fails. DOS. Nothing more. Last option works if the attacker has permission for the folder. Then, he can move or even delete the temp file or even the original file. Linux permissions and nothing special. ## security conclusion That said, only the symlink thing can theoretical be used for exploits if you know the content that will be copied over and if it can be even used for it (file is still `0600`, so not read- or writeable). Else you can only interrupt the file process so it won't be able to automaticly restore. The DOS maybe even just happen only after a service reload/restart. To perform all the DOSes, the attacker needs a user which can write to the folder. Then, it should be able to write to or move the file he needs to by himself. **Only the exploit with symlinks explicitly requires cdist to work**, which is the only convincing argument I've found. Protecting against the other problems isn't worth it, as it makes no difference if cdist is involved with. ## how to avoid How to avoid this exploit? The current live `__file` implementation uses both: 1. using a random file which is hard to guess or to bruteforce *(tempfile to copy to)* 2. directly before the move/copy, clear the path *(move to destination)* *Noting that there are doubts in this MR that 1. isn't really save because of the "long" time between the tempfile is created and it will be copied over.* An easy way of avoiding the above problem is to use a tool that is aware of this. This is nice, because we will never get the level of an atomar filesystem action as `mkdir` has and is often used for locks, but it will get better. Instead of relying on the shell and kernel that it forks the two commands directly after, it's only one fork. Not perfect, but theoretical less effort/time inbetween these checks. I've wrote to much, because now tested that `cp` or `mv` will never write to the symlink destination, rather replacing it. Still though about `mv -T` which won't override folders ... Symlink problems happen with `scp`; both with the old `SCP` and the new `sftp` protocol. This means, the previously suggested predictive filenames from me means death. At least when you assosiate DOS = death. No, serious, I know understand why they shouldn't be used now. This also means @ander needs to refactor his `__download` and co. But now coming back utilising the shortest time for changes: I've noticed if we use `sftp` instead of `scp` in command line mode, we can batch things in a single command. I'm pretty sure we can script this with passing the commands to `stdin`. Then, we can script a `rm` pretty close to an `put`, utilise the `mv`, or something similar. Even things need to go over the network, it is better than simply doing a `$__remote_exec` and then a `$__remote_copy` or soemthing similar ... ## other funny things we can do (but better not) Which should work, too, is to use piping from `ssh`. Like this: ```sh # code-local tmp=${dst}__file_tmp cat $local | $__remote_exec "umask 066; rm -rf $tmp ; cat > $tmp" ``` Will work, and at least I think ssh piping should be POSIX-compatible, but might be an hard abuse of it ... The other thinks are HERE-documents, like this: ```sh # code-remote tmp=${dst}__file_tmp umask 066 rm -rf $tmp cat > $tmp <<FILE $(cat $magic_local) FILE ``` I don't propose this seriously, but it came to my mind, too. If special characters are making trouble in the HERE-document, we can use `base64` or some accient email transcoding which served the same usecase (to embed binary content into ascii emails, which usually increased size by 20% :P). Don't think we can solve it with POSIX compatibility. The idea was to have more control writing it with piping. It's still vurnable to a symlink cause of the write, but the timeframe should be low. As indicated by the title, we should better skip them. Others will do better ;-) ## solution compling with cdist principles After all this wall of text in this MR disscussion, I will finally present the way I've found that should comply (after all the info I've got by writing this comment): Copy the file with an hardly predictive file name so nobody whats to trap us with a symlink. Then move the file to somewhat predictive. As `mv` overrides symlinks instead of writing to, this will be save. Then, we can chill and wait till an other script (`code-remote` in this case) picks up the file with no real effort guessing the name. And a move in the same directory doesn't really cost anything. Example (you know how I mean the pseudo script): ```sh # code-local random=$(mktemp -u cdist.XXXXXXXXXXXXXXXXXXXX) # can be longer, just need to be random remote=$(dirname $dst)/$random $__remote_copy $local $remote $__remote_exec mv $remote ${dst}__file_tmp ``` ```sh # code-remote # modify ${dst}__file_tmp mv ${dst}__file_tmp $dst ``` ## alternative cdist-wide solution The above solution will work based on unpredictability, just like Mark suggested it and was already in discussion. But it will require to change every other type utilising `$__remote_copy`. Idealy, they shoud use the `__file` type, but others are around, too, like `__download` and more. By writing the `sftp` section, I thought about such a general script replacing `$__remote_copy`. This means a script utilising `sftp` with an fallback for `scp`. If `scp` is required, we can make the same as with `sftp`, which doesn't really drop the security level. The only problem is, that the logic is very general - but we make life difficult when somebody whats to use his own `$__remote_copy` with the same security. Maybe stacking of different `$__remote_copy`'s - Ander would like it combine it with the sudo `$__remote_copy`.
nico commented 2022-04-10 21:14:14 +00:00
Owner
Copy link

@matze, I think there is one important point to highlight: the attack for predictable names only applies to directories that are user writable. Typically mktemp and friends have to deal with /tmp.

cdist is not affected by this in probably most, if not virtually all cases, for multiple reasons:

  • People don't use CM to manage tmp directories
  • If it's not a temp directory (47xx permissions), then it must be user owned or ACLs in places - in that case the user can do anything anyway

My main motivation was to raise awareness, but there is not much practical need to change behaviour for writing files/directories/symlinks to directories that are not user writable anyway.

@matze, I think there is one important point to highlight: the attack for predictable names only applies to directories that are **user writable**. Typically mktemp and friends have to deal with /tmp. **cdist** is **not affected by this** in probably most, if not virtually all cases, for multiple reasons: * People don't use CM to manage tmp directories * If it's not a temp directory (47xx permissions), then it must be user owned or ACLs in places - in that case the user can do anything anyway My main motivation was to raise awareness, but there is not much practical need to change behaviour for writing files/directories/symlinks to directories that are not user writable anyway.
steven commented 2022-04-10 22:07:01 +00:00
Collaborator
Copy link

Given that scp is deprecated anyway I'm all open to look at sftp at some point. But not now.

This PR/issue was to discuss and fix the following problem:

The current implentation of cdist's __file type needs 2 ssh connections to

  • upload the file and move it into place (code-local)
  • set the files attributes (code-remote)

There are cases where using 2 ssh connections causes problems.

We've implemented a fix that addresses this by making all changes in a more
atomic way within a single ssh connection.

Given that [scp is deprecated anyway](https://lwn.net/Articles/835962/) I'm all open to look at sftp at some point. But not now. This PR/issue was to discuss and fix the following problem: The current implentation of cdist's __file type needs 2 ssh connections to - upload the file and move it into place (code-local) - set the files attributes (code-remote) There are cases where using 2 ssh connections causes problems. We've implemented a fix that addresses this by making all changes in a more atomic way within a single ssh connection.
steven closed this pull request 2022-04-10 22:07:01 +00:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bugfix

   
cleanup

   
discussion

   
documentation

   
doing

   
done

   
feature

   
improvement

   
packaging

   
Stale

   
testing

   
TODO
No labels
bugfix
cleanup
discussion
documentation
doing
done
feature
improvement
packaging
Stale
testing
TODO
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: ungleich-public/cdist#331
No description provided.
Delete branch "(deleted):__file-atomic-attributes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?