util: fix GetRuntimeDir permission check #947
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@baude @rhatdan @edsantiago PTAL We need this for now until we can use containers/podman#13389 |
|
@giuseppe be advised this is now low priority. Current plan is to revert the commit that caused this. Ref: containers/podman#13391 (main) |
giuseppe
force-pushed
the
xdg-runtime-dir-accept-711
branch
from
4dcbf0f
to
c6dff20
Compare
Do not require 0700 permissions for the runtime directory but accept anything that is writeable by the user. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
xdg-runtime-dir-accept-711
branch
from
c6dff20
to
0431f9b
Compare
| @@ -19,6 +19,12 @@ var ( | |||
| rootlessRuntimeDir string | |||
| ) | |||
|
|
|||
| // isWriteableOnlyByOwner checks that the specified permission mask allows write | |||
| // access only to the owner. | |||
| func isWriteableOnlyByOwner(perm os.FileMode) bool { | |||
There was a problem hiding this comment.
I find the function name misleading: yes, it checks WritableOnlyByOwner but also checks that owner has rx. Unfortunately I don't know the context for checking that so I don't know when if or how user perms would not be rwx, or why there's a need to check.
As best I can tell from the original commit 4cc4c7137d29111fe6ccca526bc11d435fa20dd7, I believe the intention behind this check was more "does owner have rwx" than "is it locked down". If so, a name like ownerHasFullAccess() might be more descriptive - but I really don't know. @giuseppe this was your code from the beginning, though: what was your intention then (July 2018)? Can you (or anyone) find a less misleading name for this check?
There was a problem hiding this comment.
Oh wow, github didn't linkify the commit hash. Try this instead: #1048
There was a problem hiding this comment.
Wow, another failure to linkify. This? containers/podman#1048
|
I don't believe we care about the ownership of the directory from Podmans point of view other then the user can use it. |
Do not require 0700 permissions for the runtime directory but accept
anything that is writeable by the user.
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com