search

lensesio / kafka-topics-ui

Web Tool for Kafka Topics |
https://lenses.io
877 stars 148 forks source link

Run container as non-root #135

Closed varyumin closed 6 years ago

varyumin commented 6 years ago

Hello! I’ve fixed up a recommendation related with running docker as non-root. Please, check it out. If there are any questions, feel free to comment.

This change is Reviewable

andmarios commented 6 years ago

Hi @varyumin. Thank you for your pull request.

Unfortunately, I cannot merge it as it introduces some bad practices and doesn't really solve the issue you describe. I know you put in a lot of work and I understand this can be frustrating, so I'll try to explain my decision as best as I can.

  1. This docker is 1 part of 3 dockers (kafka topics ui, schema registry ui, kafka connect ui) yet you made significant changes that alter the coding style. Objectively this is my problem, not yours, but it's a reason I can't accept the PR.
  2. You changed a very big portion of the docker and the scripts, whilst you could achieve exactly the same result in just 2 lines in the Dockerfile just before the ENTRYPOINT: 
    RUN chown -R 1000:1000 /caddy /kafka-topics-ui
USER 1000:1000
  3. You removed the WORKDIR directive and changed all paths to relative. These are there to make development easier and less error prone. Any reviewer can understand with just a glance where we are and which files we operate on.
  4. An especially bad practice is that whilst I use an explicit whitelist or deny policy for adding files to the docker, you replaced it with a blacklist or allow policy. By that I mean that I add every file needed for the docker explicitly, while you just add everything (ADD . /landoop) and blacklist some files in .dockerignore. Why this is bad? It makes easy to break production code. For example let's say we add a new file to the docker, yet forget to do git add. With your policy what will happen? The build locally will pass and work. The build in docker hub will pass too but the docker image will not work. So our users will get broken dockers! With the default whitelist policy I had, if we forgot to do git add, the build in docker hub would fail and the broken image would never be generated. Of course there are times it's preferrable to add a whole directory. In such cases, (a) you should add a subdirectory and never the docker root, (b) use COPY instead of ADD so you can distinguish easily between files and directories when needed.
  5. Whilst you describe the PR as run container as non-root what it really does, is run the container as the user 1000. We just switch the problem, not really fix it, which would require for the docker to be able to run with any user. Also please note that user 1000 is a common (the default actually) non-system user in any Linux distribution and thus a bad choice.
  6. Last, and this is not something I would reject the PR based on, but I would postpone the switch: you alter the docker image behaviour, breaking our contract with the users. There are many users that run the docker with --net=host, usually when they host REST Proxy and Kafka Topics UI on the same machine. For these users we added the PORT env var, which sets Caddy to bind to the desired port. By changing the user to 1000 (or any non-root user for that matter), you automatically restrict Caddy from binding to ports lower than 1024, thus breaking the deployment for any user that uses such a port.

I hope I didn't sound too harsh. I'll soon add the feature your request. Thanks again, Marios.