Run app with non-root user

[TheAssassin]

Running applications in Docker as root user opens various attack vectors, as Docker's "isolation" isn't meant to be the only safe guard against adversaries. It should be considered a "last resort". It's best practice to use a non-root user wherever possible.

This commit changes back to Alpine as a base, as it provides the adduser tool via busybox. It increases the image size by a few MiB (2-3, currently). One could optimize the image size by writing a custom /etc/passwd, but this seems quite unnecessary, especially given that Alpine is the base of so many other images.

Fixes #100.

[aspacca]

@TheAssassin having an alpine based image running a static binary as non-root user being less open to various attack vector srather than having a scratch image run as non-root user on the host system seems rather counter-intuitive.
can you explain why it should be safer?

[TheAssassin]

The key term is "privilege escalation". Running as root is far more dangerous than having some tools in the userland.

All attacks that manage to escape out of a Docker container have one requirement: root. If you have root, you can do a lot more stuff than you can do as a normal, unprivileged user: run syscalls to mount things, for instance, overwrite stuff on block devices, ...
Of course, modern systems have mitigation techniques in place. Docker containers are usually run with only a minimal set of cgroups capabilities (e.g., SYS_ADMIN is usually not permitted for good reasons), and there are sandboxing tools like AppArmor that also restrict the processes. All these, however, are prone to errors: configuration mistakes, bugs in the code, there's a lot that could go wrong. In the end, they're just meant as safety guards, and the more the better. Relying only on those is, however, not a good idea.

The worst I can think of with adding some userland tools is that, if a user manages to escape the Go process, e.g., through some shellcode (it doesn't use any form of libc, which already closes some attack vectors), or some bug in the Go compiler, they're left with just a busybox instance, a bunch of libraries and nothing else, as an unprivileged user. No matter what, they can't run privileged operations like mounting devices.

Millions of containers use Alpine as the base, and I am not aware of a big wave of attacks on those. Of course, that doesn't have to mean anything. But I also can't think of an attack vector that opens because some tools are laying around on the filesystem. I consider root access significantly more dangerous than busybox. And I'm convinced the security community agrees there.

[TheAssassin]

By the way, I already offered a way to implement #100 without using Alpine, by just writing the passwd file ourselves. It's possible, but I don't see any real gain over just using Alpine as the base and using its tools, as explained in the commit message. If you feel better, though, we can also do that.

[aspacca]

By the way, I already offered a way to implement #100 without using Alpine, by just writing the passwd file ourselves

this could solve an issue with https://github.com/dutchcoders/transfer.sh/pull/345/files#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R21

unless the uid and gid of the newly created user are deterministic an user running the docker image and wanting to use a docker volume on the host for local storage cannot know what uid/gid give ownership to for the directory on the host.

writing the passwd file should solve the issue.

correct me if I'm wrong

[TheAssassin]

unless the uid and gid of the newly created user are deterministic

Good catch and good question. In my experience in the past years, the first system user to be created is always UID 100. The group assigned is nogroup. We can hardcode those values, if you want to, but I don't think it'll be necessary. Writing the file directly won't be any easier than adding -u <id>. That should not be the criterion you use to decide that question.

In fact, this directly brings us to the next point: we need to ensure that the files of people who upgrade the Docker container will be accessible. I'm sure it'd been better to have that fixed in 2017 rather than now. I'm not sure what the best solution is there, really. I guess it's merely a matter of documentation, but it isn't very user friendly. Suggestions welcome.

[aspacca]

we need to ensure that the files of people who upgrade the Docker container will be accessible

we cannot ensure this, the only thing we can do is to add some documentation/breaking update notes on the readme and in the release page. local storage set explicitely 700 for dir and 600 files permissions, so it will break for sure.

as for scratch vs alpine: 2-3MB more is 30-40% bigger image. I would go for scratch, there's also the consideration to reduce even further the attack surface

please let me know if it's fine for you

[mckaygerhard]

i guess security is not considering here @aspacca .. from scratch is just upstream withour any attention and sync from others, each piece of the base are independient of the other, using a base like alpine of whatever is far better cos is specialiced in that objetive.. is pretty clear and makes sense for any 4 finger of head!

using a better "from" will solve those stupid "breaks" .. @TheAssassin has a good point here .. and the future will give us the reason..

[aspacca]

@mckaygerhard either be based on alpine or scratch, running the transfer.sh binary inside the docker as an user instead of root will break the current installation with local storage

[TheAssassin]

30-40% bigger image

First, this isn't quite true. The image is composed of multiple layers, one being the Alpine base layer. As mentioned in one of my previous comments, this base layer is in very widespread use, so the chance is high that it'll be used by any other image as well, and thus won't occupy additional space.
Also, who won't have the 2-3 MiB on their servers? This is a storage system, and I'm sure many many people use the local provider. Even just a few pictures are a lot larger than the base image. I don't think this is going to be an issue at all. It smells like premature optimization.

@mckaygerhard either be based on alpine or scratch, running the transfer.sh binary inside the docker as an user instead of root will break the current installation with local storage

True, and that's an issue. I was thinking about adding a startup script that ensures the correct permissions, then launches the daemon with the right permissions...

[mckaygerhard]

err pardon me, but as @TheAssassin saids, is not breaking.. cos we can made pre-scripts to ensure correct permissions.. in fact linux from scratch has more "tune up" non standar scripts rather any other.. so.. i guess you are wrong XD

[aspacca]

It smells like premature optimization.

it would be premature optimization if we are going to build now the docker image for the first time.
we already have scratch based image, and moving to alpine is going to increase the size. assuming that alpine layer is already present in the image cache on the machine that will pull the image is just an assumption, and I won't go through that.

I was thinking about adding a startup script that ensures the correct permissions, then launches the daemon with the right permissions...

this script cannot run that as root, otherwise it won't be able to ensures the correct permissions, so in the end we are letting back from the window what we are trying to get out of the door.
also I don't think that a docker image should take care of such task. if this is the option I would rather keep the dockerfile as it is and providing guidelines on how to run the container as non-root user on the host machine.

[TheAssassin]

so in the end we are letting back from the window what we are trying to get out of the door.

That's not the case. The actual process is still run as non-root. The shell script doesn't open ports or anything. In fact, you don't even have to keep the shell session open, you can just exec the Go stuff once you're done dropping privileges.

also I don't think that a docker image should take care of such task

Of course it should. Easy and flawless upgrading is a core task of good Docker images. Database upgrades are also done by the init scripts in good containers. Automate what can be automated safely, reducing the risk that the user may screw up, and saving their own time.

if this is the option I would rather keep the dockerfile as it is and providing guidelines on how to run the container as non-root user on the host machine.

Do you realize that this requires more than just adding --uid to the run command? A named volume's ownership is, upon the first mount, impacted by the USER command in the Dockerfile. You will have to, like, sudo chown the actual volume path on the host machine. Plus, it's a nonstandard setup.

I see a lot of reluctance on your end with regards to this pull request. Please decide if you want to improve the overall security by not running the process as root or not. Only then it makes sense to talk about issues like upgrading or maintaining a static user ID for the system user. All the facts have been presented.

[aspacca]

I see a lot of reluctance on your end with regards to this pull request. Please decide if you want to improve the overall security by not running the process as root or not

I have no reluctance, I just set the conditions for how it should be done: scratch based image, no permissions change script.

[lkubb]

More an FYI since I'm mostly a Github lurker: I needed this functionality as well and patched it in with the scratch based image. The UID/GID are set at build time though. I intended it to be modifiable at the container creation stage, but that needs some logic before launching transfersh, which is hard for me to do without a shell (script) and some additional binaries (i.e. chown, su_exec/gosu/sue).

lkubb@d9fefd5

[aspacca]

@lkubb , your solution is exactly what I proposed.

maybe just using build arguments as PUID/GUID, or higher random ones

the only problem I can see in making it upstream is that it can break existing installation, where mounted path is already owned by root.

do you have any suggestion how to avoid/mitigate this issue?

[lkubb]

Thanks for looking over my commit! I've been thinking about this problem longer than I wanted tbh, tried different methods. My conclusion was that the only way without disrupting existing installations and without adding additional binaries would be to keep building with the root user by default, but allow to specify non-root mode at build time with --build-arg RUNAS=something. My Dockerfile now builds like the default one, except when you explicitly specify you want another user (setting RUNAS to anything works and you can request another ID like before): lkubb@78e236d [Building it for both cases doesn't take much longer since the ARGs are referenced after the compilation.]

I would document this feature for users that need it, possibly provide a sample docker-compose.yml as well. If you want to go further, maybe provide something like dutchcoders/transfer.sh:noroot on Docker Hub and steer new users to it. Existing ones that want to switch just need a single, but manual sudo chown -R $UID:$GID ./data on the host. I'm happy to make a pull request, if desired.

[aspacca]

@lkubb I think your solution is very valuable, also providing a noroot tagged image is a good idea.

I'd say we finally found a solution to this issue :)

correct me if I'm wrong, here RUNAS is mandatory:
lkubb@78e236d#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R33

so in the non breaking build I have to pass root

but then we will end up executing this if branch: lkubb@78e236d#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R22

[lkubb]

@aspacca Thank you, glad to hear. :)

In the meantime, I tried to build it on a different ("virgin") system, first without providing --build-arg. This surfaced another issue I was actually expecting (no files matching the glob /tmp/useradd/*), but that didn't happen before somehow. This is fixed now in lkubb@54e125f (adding a placeholder file)

COPY --from=build --chown=${RUNAS} runs just fine without providing a value in the argument, same as USER ${RUNAS}. Both default to root, at least on the four systems I tried it with. It feels a little bit hacky, but works well.

My docker-compose.yml:

---
version: '3.8'

services:
  transfer:
    build:
        args: {}
#            RUNAS: transfersh
#            PUID: 5001
#            PGID: 5001
        context: ./transfer.sh
    hostname: transfer
    environment:
        - PROVIDER=local
        - BASEDIR=/tmp
    ports:
      - "1234:8080"
    expose:
      - "8080"

first build with the above docker-compose.yml, second build with commented out args:

$ git clone https://github.com/lkubb/transfer.sh
$ docker-compose up -d
Creating network "test_default" with the default driver
Building transfer
Step 1/18 : ARG GO_VERSION=1.16
Step 2/18 : FROM golang:${GO_VERSION}-alpine as build
1.16-alpine: Pulling from library/golang
a0d0a0d46f8b: Pull complete
31adcdaf11c8: Pull complete
b8b176561691: Pull complete
23d1c9dd6ab7: Pull complete
36803fd6ed32: Pull complete
Digest: sha256:78181bcf43be59a818e23095f21b3818f456895c3f1f2daaabdfd1af75cedd1f
Status: Downloaded newer image for golang:1.16-alpine
 ---> 1b35785aa3c4
Step 3/18 : RUN apk add git musl-dev
[...]
Step 14/18 : COPY --from=build --chown=${RUNAS}  /go/bin/transfersh /go/bin/transfersh
 ---> 52dcabc2af45
Step 15/18 : COPY --from=build /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
 ---> ea5dc64d26ba
Step 16/18 : USER ${RUNAS}
 ---> Running in 4b2d8756aad6
Removing intermediate container 4b2d8756aad6
 ---> d53b24877c39
Step 17/18 : ENTRYPOINT ["/go/bin/transfersh", "--listener", ":8080"]
 ---> Running in 104bb487a1f4
Removing intermediate container 104bb487a1f4
 ---> 760ee3ad7929
Step 18/18 : EXPOSE 8080
 ---> Running in b200625a2964
Removing intermediate container b200625a2964
 ---> 7dd773bdd234

Successfully built 7dd773bdd234
Successfully tagged test_transfer:latest
WARNING: Image for service transfer was built because it did not already exist. To rebuild this image you must use `docker-compose build` or `docker-compose up --build`.
Creating test_transfer_1 ... done
$ docker-compose top
test_transfer_1
UID     PID    PPID    C   STIME   TTY     TIME                     CMD
---------------------------------------------------------------------------------------
root   20721   20701   0   18:36   ?     00:00:00   /go/bin/transfersh --listener :8080
$ vim docker-compose.yml
$ docker-compose up -d --build
Building transfer
Step 1/18 : ARG GO_VERSION=1.16
Step 2/18 : FROM golang:${GO_VERSION}-alpine as build
 ---> 1b35785aa3c4
Step 3/18 : RUN apk add git musl-dev
 ---> Using cache
 ---> 3645c13f2ffa
[...]
Step 14/18 : COPY --from=build --chown=${RUNAS}  /go/bin/transfersh /go/bin/transfersh
 ---> 3ea81e980658
Step 15/18 : COPY --from=build /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
 ---> b8d8ff3c9678
Step 16/18 : USER ${RUNAS}
 ---> Running in 3b9a99503cea
Removing intermediate container 3b9a99503cea
 ---> 6a13ed7763e8
Step 17/18 : ENTRYPOINT ["/go/bin/transfersh", "--listener", ":8080"]
 ---> Running in f1f29b24b49d
Removing intermediate container f1f29b24b49d
 ---> 2d6ae386dd65
Step 18/18 : EXPOSE 8080
 ---> Running in c153f0868bef
Removing intermediate container c153f0868bef
 ---> fb4ce9bb8f00

Successfully built fb4ce9bb8f00
Successfully tagged test_transfer:latest
Recreating test_transfer_1 ... done
$ docker-compose top
test_transfer_1
UID     PID    PPID    C   STIME   TTY     TIME                     CMD
---------------------------------------------------------------------------------------
5001   21197   21174   2   18:37   ?     00:00:00   /go/bin/transfersh --listener :8080

[aspacca]

actual fix in #418