mkdir: do not return errors for incorrect directory modes #29
Conversation
There was a problem hiding this comment.
As for vendoring, it's not really needed. Modern go can do fine without (it will download the missing modules from the proxy.golang.org to $GOMODCACHE, when needed, if they are not yet there).
Having vendor has its pros and contras; I haven't yet grasped which way is better, but it is definitely not a must have.
|
Yeah, I might just drop it for now. We need |
cyphar
force-pushed
the
mkdirall-mode-warning
branch
2 times, most recently
from
62a2e20
to
974dfae
Compare
cyphar
force-pushed
the
mkdirall-mode-warning
branch
from
974dfae
to
fdd4788
Compare
|
Should it be a warning or info @kolyshkin? |
cyphar
force-pushed
the
mkdirall-mode-warning
branch
from
fdd4788
to
d30a2f2
Compare
Either way is fine with me. Info is one level below Warning (and just above Debug), and the default level is Warning. Meaning, if you want it to show by default, use That's from the top of my head and need to be checked. |
|
Default logrus includes output for FWIW, after speaking to @rata and @alban I am convinced these checks don't defend against any practical attack so it might make sense to just drop these in the future. The owner and mode checks are also going to be wrong for |
cyphar
force-pushed
the
mkdirall-mode-warning
branch
3 times, most recently
from
9cb300b
to
370bf46
Compare
We've had several examples of unexpected semantics with how modes are calculated, and there will likely be many more in the future. In addition, mounting filesystems like vfat with mount options that mess with ownership (like "uid=1234,gid=5678,umask=0") will result in unexpected behaviour that would be very difficult to emulate. To avoid further regressions, just remove the checks entirely. In theory we could switch to adding warnings, but there's no real benefit IMHO. The semantics of MkdirAll are quite loose already so arguably there is no practical difference between re-using a directory that already existed and being tricked into opening an intermediate directory you didn't create. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Some pseudofilesystems (like cgroupfs) create non-empty directories, so this check is kind of questionable if someone tries to use MkdirAll on those filesystems. The semantics of MkdirAll already allow us to re-use a non-empty directory, so this check arguably didn't buy us anything anyway. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
cyphar
force-pushed
the
mkdirall-mode-warning
branch
from
370bf46
to
92b699d
Compare
|
I ended up dropping the warnings since it's not clear anyone would care anyway, and tools that don't use logrus would get warnings they can't practically block. |
We've had several examples of unexpected semantics with how modes are
calculated, and there will likely be many more in the future. To avoid
further regressions, we should just downgrade the error to a warning.
However, we can keep the owner checks (those -- hopefully -- shouldn't
have too many weird semantics).
See opencontainers/runc#4408
Signed-off-by: Aleksa Sarai cyphar@cyphar.com