[PATCH] Improve permission checks in prosodyctl

[Paul Tiedtke]

Hey guys,

 

I already submitted this change a few month ago but my thread never appeared. MattJ couldn’t find it as well – so we decided to try it again.

 

My patch will improve the permission checks to check for all user and directory permissions. And will really solve the following issue.

https://issues.prosody.im/1075

 

Best,

Paul (sapkra)

 

[Matthew Wild]

Hi Paul,

Thanks for the patch, and sorry it's taken a while to get to review it.

As it stands, the patch changes the current behaviour in a negative
way. I guess your intention is to allow the target directory to be not
owned by the prosody user (I'm curious why).

The current code checks that the directory is owned by prosody,
writable by prosody, and also that it is *not* writable by any other
user.

The current proposed patch removes the first and last checks, and just
ensures it is writable by the current user. This also makes the
warning message incorrect.

And for the larger picture, there are some related issues:

- https://issues.prosody.im/1217

- https://issues.prosody.im/999

We have been discussing various solutions to make the feature more
robust and less surprising (including removing some features if
needed).

I'm curious to hear your thoughts, and exactly how you're using it
that makes the current code inadequate. That will help with figuring
out the best solution.

Regards,
Matthew

[paul]

Hey Matt,

thanks for reviewing it.

I'm running prosody on OpenShift (a Kubernetes distribution by RedHat) which is running the containers with arbitrary user IDs because of security reasons.

So it's impossible to have a prosody user inside a container but the arbitrary users are always member of the root group so my code will check if the current group of the user is able to write to the directory or if the directory is even writable by every user.

If you want you can remove the last check but then the error message would be wrong because the user could write to this directory. So there has to be a second warning for it which tells the user that the directory should not be writable by every user.

Here are the docs:

https://docs.openshift.com/container-platform/3.11/creating_images/guidelines.html#openshift-specific-guidelines

Additionally I've found a typo in my patch. The warning message has to be: "The directory "..cert_basedir.." is not writable by this user"