ungleich-public/cdist

Fork 10

__file: mktemp -u checks for perms #332

New issue

Open

opened 2022-04-12 06:22:19 +00:00 by ander · 6 comments

ander commented

2022-04-12 06:22:19 +00:00

Collaborator

introduced with 22039284f5 cc @steven

problem is in gencode-local with upload_destination="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")".

even if mktemp -u is dry-run, it checks for perms.

ander@m720q:~$ mktemp -u /etc/no-such-path.XXX; echo $?
/etc/no-such-path.aPm
0
ander@m720q:~$ mktemp -u /etc/ssl/private/key.XXX; echo $?
mktemp: failed to create file via template ‘/etc/ssl/private/key.XXX’: Permission denied
1

so, for example, if you want to set up something in restricted paths, running mktemp -u in local machine might result in error:

$ echo __file /root/.bashrc --source /home/ander/.bashrc | cdist config -i - localhost
INFO: [376436]: localhost: Starting configuration run
--8<--SNIP--8<--
gencode-local:stderr
--------------------
mktemp: failed to create file via template ‘/root/.bashrc.cdist.XXXXXXXXXX’: Permission denied

introduced with 22039284f5 cc @steven problem is in `gencode-local` with `upload_destination="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")"`. even if `mktemp -u` is dry-run, it checks for perms. ``` ander@m720q:~$ mktemp -u /etc/no-such-path.XXX; echo $? /etc/no-such-path.aPm 0 ander@m720q:~$ mktemp -u /etc/ssl/private/key.XXX; echo $? mktemp: failed to create file via template ‘/etc/ssl/private/key.XXX’: Permission denied 1 ``` so, for example, if you want to set up something in restricted paths, running `mktemp -u` in local machine might result in error: ``` $ echo __file /root/.bashrc --source /home/ander/.bashrc | cdist config -i - localhost INFO: [376436]: localhost: Starting configuration run --8<--SNIP--8<-- gencode-local:stderr -------------------- mktemp: failed to create file via template ‘/root/.bashrc.cdist.XXXXXXXXXX’: Permission denied ```

nico commented

2022-04-12 07:23:05 +00:00

Owner

Good catch, @ander!

I think we could easily replace

      # upload file to temp location
      upload_destination="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")"

with

      upload_destination="${destination}${__cdist_object_marker)"

But we will need to document the variable in our reference.

The method used for creating the suffix is the same idea as mktemp, just python's way.

I wonder if we will rename the variable beforehand so that it is not internal (__cdist prefix) anymore.

Good catch, @ander! I think we could easily replace ``` # upload file to temp location upload_destination="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")" ``` with ``` upload_destination="${destination}${__cdist_object_marker)" ``` But we will need to document the variable in our reference. The method used for creating the suffix is the same idea as mktemp, just python's way. I wonder if we will rename the variable beforehand so that it is not internal (`__cdist` prefix) anymore.

ander commented

2022-04-12 07:29:52 +00:00

Author

Collaborator

I don't understand how suffix change helps.

mktemp -u will still check if ${destination} is accessible (locally).

I don't understand how suffix change helps. `mktemp -u` will still check if `${destination}` is accessible (locally).

nico commented

2022-04-12 08:01:22 +00:00

Owner

The mktemp -u should not have been there, updated the comment.

ander commented

2022-04-12 08:31:00 +00:00

Author

Collaborator

OK, now it makes sense. And maybe it's time to have globally usable randomzied run id, which can be used for variety of things. Starting with temp file suffixes etc. For example $__run_id? Or per object randomized like $__object_run_id? Or both.

For example:

tmp_file="$destination.$__run_id"

Edit: okay, read in hurry, there's already __cdist_object_marker, but not very convenient name to use, but I can live with that. It's currently not available in explorer, which we should add, just in case we might need it there.

OK, now it makes sense. And maybe it's time to have globally usable randomzied run id, which can be used for variety of things. Starting with temp file suffixes etc. For example `$__run_id`? Or per object randomized like `$__object_run_id`? Or both. For example: ```sh tmp_file="$destination.$__run_id" ``` Edit: okay, read in hurry, there's already `__cdist_object_marker`, but not very convenient name to use, but I can live with that. It's currently not available in explorer, which we should add, just in case we might need it there.

ander commented

2022-05-02 18:34:59 +00:00

Author

Collaborator

__file: broken on macOS config hosts:
https://github.com/cdist-community/cdist-conf/issues/53

The latest versions of macOS mount / read-only:

$ mktemp -u /foo.XXXXXX
mktemp: mkstemp failed on /foo.lOUTpx: Read-only file system

@steven @nico

__file: broken on macOS config hosts: https://github.com/cdist-community/cdist-conf/issues/53 The latest versions of macOS mount / read-only: ``` $ mktemp -u /foo.XXXXXX mktemp: mkstemp failed on /foo.lOUTpx: Read-only file system ``` @steven @nico