__file: mktemp -u checks for perms #332

New Issue

ander · 2022-04-12T06:22:19Z

ander commented

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

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

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

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

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

ander commented

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

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

__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

steven commented

2022-05-02 21:32:13 +00:00

The regressions should be fixed as of 6c8c692a22

The regressions should be fixed as of https://code.ungleich.ch/ungleich-public/cdist/commit/6c8c692a22c886bf82b18f51a133f88a8ab547b7

Sign in to join this conversation.