search

tianon / gosu

Simple Go-based setuid+setgid+setgroups+exec
Apache License 2.0
4.71k stars 320 forks source link

Shouldn't "empty user" be disallowed? #18

Closed ionelmc closed 6 years ago

ionelmc commented 8 years ago

Hey,

I'm doing a port of this sorely need tool to Python here and I was wondering why are these usecases allowed: https://github.com/tianon/gosu/blob/master/Dockerfile.test#L50-L51

Also, I've looked quite a bit to su-exec (I actually started porting that) and I'm puzzled as to why it fallback to current user if input is invalid: https://github.com/ncopa/su-exec/blob/master/su-exec.c#L27-L28

tianon commented 8 years ago

Yeah, it's a little odd, but it's not explicitly disallowed by Docker:

$ docker run --rm --user '' debian:sid id
uid=0(root) gid=0(root) groups=0(root)

I believe the rationale there is that the syntax is really [user[:group]], so both are technically optional and default to whatever the "current" values are.

ionelmc commented 8 years ago

Hm interesting. ":mygrp" does indeed make sense tho I wonder if this shouldn't break away from docker's handling. If you use '' as the user then maybe there's a problem with the inputs and you should get an error. What do you think?

tianon commented 8 years ago

If "gosu" is setuid (which I don't recommend, but it's a valid use case), then the "empty" specification makes some sense IMO -- I'm really wary of diverging from Docker's behavior here, especially since one of our main benefits is that we directly use the same library Docker does for parsing the values

ionelmc commented 8 years ago

So I was thinking about this from the perspective that the container is ran as root (it's default and you only need gosu when you need to do root stuff). So from that perspective it's undesirable to allow silent treatment of user mistakes.

Anyway, I don't want to change behavior in gosu, I only want to see if you agree in principle with this idea (I'm a bit undecided about what to do in my port).

tianon commented 6 years ago

Closing given the same rationale as my comment in https://github.com/tianon/gosu/issues/33#issuecomment-430037353, which I'm quoting here for posterity/completeness:

... this is essentially by design -- see https://github.com/tianon/gosu/blob/fd64f11edb4fa2423859d615aa8783df62a4be40/Dockerfile.test#L25-L67 for our (pretty extensive) test suite of edge cases.

I agree that this would be a good idea to document better, but this is fundamentally Docker (and at this point even runc specific) functionality, so I think the associated documentation belongs in one of those upstream projects (our README.md here explicitly calls out that our syntax is that of Docker's own docker run --user flag, using the very same implementation referenced directly from runc's code).