file.mkdir applying incorrect permissions

[slai]

Original thread: https://groups.google.com/forum/#!topic/salt-users/ozjebLCvv04

If I run the following command on a system where none of the directories exist except the first directory (the number of subdirectories is irrelevant)...

salt somehost.somedomain file.mkdir /opt/myspecialapp/data/v9.0/data mode=700 

I end up with each of the directories (except the first because it already exists) with 0457 permissions. This isn't right (obviously) and breaks things spectacularly as the owner of the directory is not able to traverse into it!

Here's are some tests with different mode combinations -

GOOD

linux:~ # salt-call file.mkdir /opt/blah/blah1/blah2
[INFO    ] Configuration file path: /etc/salt/minion
local:
    None
linux:~ # tree -p /opt/blah
/opt/blah
└── [drwxr-xr-x]  blah1
    └── [drwxr-xr-x]  blah2

linux:~ # salt-call file.mkdir /opt/foo/blah1/blah2 mode=755
[INFO    ] Configuration file path: /etc/salt/minion
local:
    None
linux:~ # tree -p /opt/foo
/opt/foo
└── [drwxr-xr-x]  blah1
    └── [drwxr-xr-x]  blah2

BAD

linux:~ # salt-call file.mkdir /opt/meh/blah1/blah2 mode=700
[INFO    ] Configuration file path: /etc/salt/minion
local:
    None
linux:~ # tree -p /opt/meh
/opt/meh
└── [dr--r-xrwx]  blah1
    └── [dr--r-xrwx]  blah2

linux:~ # salt-call file.mkdir /opt/bar/blah1/blah2 mode=750
[INFO    ] Configuration file path: /etc/salt/minion
local:
    None
linux:~ # tree -p /opt/bar
/opt/bar
└── [drwxr-xr-x]  blah1
    └── [drwxr-xr-x]  blah2

Notice how mode 755 (also the default mode) works fine, but mode 700 or 750 doesn't.

This problem can be reproduced on Salt 0.15.9 and Salt 0.16.0.

[slai]

I did some digging into the source code, specifically modules/file.py, and I found the following lines (unnecessary comments removed for brevity) -

def mkdir(dir_path, user=None, group=None, mode=None): 
    directory = os.path.normpath(dir_path) 

    if not os.path.isdir(directory): 
        # turn on the executable bits for user, group and others. 
        # Note: the special bits are set to 0. 
        if mode: 
            mode = int(str(mode)[-3:], 8) | 0111 

        makedirs_perms(directory, user, group, mode) 

Once the mode has been manipulated to flick on the executable bits, it is left as an int and passed to makedirs_perms which eventually calls check_perms, which then calls config.manage_mode.

config.manage_mode however, doesn't know anything about how to handle mode values as ints. It simply stringifies the int (as base-10) and pads it with zeroes so it is 3 characters long. Therefore the mode value, which is 457 as an int (base-10), becomes '457' as a string which is not the mode we want to apply. Instead, the mode value should be '711' (i.e. the base-10 int 457 in base-8 representation).

There are a couple of ways to fix this -

1. In file.mkdir, convert the mode int back into a string before passing it to makedirs_perms using -

   mode = oct(mode) 

... after the line that flicks on the executable bits.

1. Modify config.manage_mode so it can handle mode values as ints, e.g. add this before the return statement -

   import numbers 
   if isinstance(mode, numbers.Integral): 
       mode = oct(mode) 

I'm not sure which is the better solution, particularly as I don't know if there's a standard in how mode values are represented inside these modules. I can also see someone coming along down the track wondering why their mode isn't being applied because it is the base-10 literal of the base-8 value, e.g. why doesn't mode=int(700) work (most likely when the int conversion is implicitly performed elsewhere).

I hope this makes sense - I had to read it over a couple of times to make sure I wasn't mixing up the bases and even now I'm not sure. Doesn't help I don't know the proper words for describing numbers, bases and their representations!

[slai]

P.S. The reason why mode=755 'works' is because it is the default on Linux systems and because when it is interpreted as base-8 (i.e. 493 in base-10), it becomes an invalid value to pass to chmod. The makedirs_perms function doesn't bother checking the return value of check_perms (the method that attempts to change permissions) and instead just thinks everything's fine.

It might be worth checking the return value of check_perms in makedirs_perms (and also makedirs_perms in mkdir) as well so these hidden errors aren't swallowed. I see this being something that should be done in a few other places too. I'm happy to work towards a patch if someone can let me know what the best course of action is.

[terminalmage]

We've tried to ensure that file modes are not interpreted as octals, it looks like this particular instance has slipped through however. Thanks for the report.

[terminalmage]

Should be fixed in #6046.

[slai]

I'm not sure removing that chunk of code is a good idea - what happens when the file.managed state is used with makedirs: True? Where are the dir permissions calculated for that? (I had a look and got lost trying to find where makedirs is utilised - will have another look when I'm not so tired :)

Also, here's another case of octal-specific code causing issues - https://github.com/saltstack/salt/blob/develop/salt/states/file.py#L1233

Finally, I still think it is a good idea for makedirs_perms to interpret the response from check_perms instead of blindly assuming it all worked. I'll try to get a patch in for this soon.