Unusual 777 directory mode in NewFilePersister in file.go

[setharnold]

https://github.com/Azure/azure-event-hubs-go/blob/15b4b52d1a10fe04eefcd452b6483880a4267b08/persist/file.go#L44

// NewFilePersister creates a FilePersister for saving to a given directory
func NewFilePersister(directory string) (*FilePersister, error) {
    err := os.MkdirAll(directory, 0777)
    return &FilePersister{
        directory: directory,
    }, err
}

Hello, mode 0777 is very rarely used; this is a very unusual directory mode, almost always a mistake.

01777 can be useful for directories like /tmp or /dev/shm; I don't off-hand see a usecase for 0777, though.

Is this a mistake? Should the sticky bit be set? Should the directories be created with more restrictive permissions?

Thanks

[richardpark-msft]

You mention /tmp and /dev/shm, so I imagine you're on a Linux/Unix system for this. In practice, the umask (defaults to 022) will scope the permissions more. So the MkdirAll() function above will end up creating a folder with drwxr-xr-x. It's still a little more broad than it'd have to be, but given there's no confidential data in a checkpoint store this will probably be okay for development.

If you still desire to have more tightly locked down permissions you can precreate the folder. If it already exists os.MkdirAll() will leave it as is, preserving the stricter permissions.

Closing this issue for now but please reopen if you want to discuss it more.

[setharnold]

Sorry for my slow reply. it's been busy.

I don't know the full context of how this function is used -- if this function is used a few times and we know the umask is set safely in each, then yeah, we can just leave this here.

If this function is called from enough places that it is difficult to inspect them all for a correct umask, then it might be nice to pick safer defaults here.

(I don't actually see a button to re-open the issue, sorry.)

Thanks

[richardpark-msft]

Just to be clear, the umask isn't optional - we don't apply it, it's just part of the system overall. So behavior should be consistent whereever os.MkdirAll is called (which is a standard library package).

[setharnold]

The trick is, some applications set umask to 0000 because that's what W. Richard Stevens said to do in daemonize(). Some applications leave it alone entirely, leaving it up to the admin to pick something. Systemd unit files default to 0022 (a nice, safe, choice), but sysvinit scripts just take whatever the admin's shell was using when the admin runs /etc/init.d/whatever restart. Gnome terminals started in Gnome use a systemd user service file to start up, and thus don't get the settings that might have been configured in the pam_env(7) service.

It goes on like this.

This is why I think relying upon the caller of the function having a safe umask isn't ideal. If this function is used once in one application that only ships with a systemd unit file it's probably fine. If this function is called through dozens of applications, some of which are user-started applications, some of which have sysvinit scripts, etc, it'd be better to be more consistent in how it's used.

Thanks