small Dockerfile improvements

[schue30]

This PR improves the following points in the Dockerfile:

• build exact git tag instead of the current master
• update of the current golang build environment from 1.9.2 to 1.11.4
• use scratch instead of an alpine image
• run container as user 1000 (instead of root)
• change listening ports from system ports 53, 80, 443 to 5353, 8080, 8443

WARNING - this PR contains breaking changes in the Dockerfile
The container that is started from the image that is built from this Dockerfile is running as user 1000 instead of root (0). As a non root user cannot (per default) listen on system ports (<1024), I had to change them to ones that are higher than 1024. In this case 5353, 8080.
Updates from older versions should not be a problem, as long as the user 1000 or the group 0 has read permissions on the config file, write permissions on the sqlite db file and the listening ports are higher than 1024.

[coveralls]

Coverage remained the same at 93.831% when pulling 6ada9dd on schue30:master into e1f1d6a on joohoi:master.

[joohoi]

Thanks for the PR! This looks like a good update, I'm just slightly hesitant to merge it straight away because of the backwards breaking changes. I'll test it throughly in the near future to be able to figure out a good way to inform the current users about the changes they need to make.

[Ajedi32]

FYI, this PR should also include changes to docker-compose.yml (at a minimum I believe the port mappings need to be updated).

[Ajedi32]

Also, this fixes #79

[schue30]

FYI, this PR should also include changes to docker-compose.yml (at a minimum I believe the port mappings need to be updated).

Yes, for sure. I will add the change for the docker-compose.yml.

[joohoi]

Ok, this PR looks good. I had to pick the Go base image line from this PR for the work done in #147 so there's a merge conflict for that line, I'm sorry.

In addition to the merge conflict resolution, I think we should update the README to point to the new port configuration in: https://github.com/joohoi/acme-dns#using-docker

[joohoi]

I stumbled upon two additional questions about the Dockerfile

[joohoi]

Ping @schue30 are you interested to continue working on this PR or can someone else continue from here on? I would love to hear your opinion especially about the volume change, I'm not a Docker expert by any means myself.

[schue30]

Pong @joohoi

[joohoi]

Thanks for all the work you've put into this @schue30 and @tcely ! Looks robust. I'm still wondering how to notify the current Docker users of acme-dns about the breaking changes. What's the usual process?

[Ajedi32]

What if the user with ID 1000 is already an existing user on the host system for whom it wouldn't be appropriate to grant read permissions on that config file? Is there a way to configure the container to use a different user id for the process? Currently it's possible to do this in the existing acme-dns container by setting e.g. user: 1005:1005 in docker-compose.yml. Would that cause any permission issues with the new container?

[tcely]

Yes, changing away from 1000:1000 would cause problems. Anything "inside" the container would be owned by the wrong user ID.

If you're concerned about the host system, put your docker data under a directory only root can access. That way your host system user won't be able to access any of it. Docker runs as root and can find the correct directory for the mapping, but the host system user can't get to the subdirectory.

[Ajedi32]

Regarding communication, normally I'd suggest a major version bump as a way of denoting that the changes could break stuff. In this case though it doesn't seem like there are any changes to acme-dns itself, just to the docker container. So I guess you could just bump the docker container version, but then it'd be out of sync with the acme-dns version. Hmm...

[tcely]

Thanks for all the work you've put into this @schue30 and @tcely ! Looks robust. I'm still wondering how to notify the current Docker users of acme-dns about the breaking changes. What's the usual process?

I'm not sure we can do any better than a note at the top of the README.md file.

[zokradonh]

I see that breaking changes stop this PR from being merged. There are ways to accomplish nearly the same without breaking changes.

1. File permissions problem: Start container as root with a new entrypoint script. The new entrypoint script ensures correct file permissions on every startup and then drops root with e.g. gosu. This is common practice in several Docker images.
2. Port problem: use old ports (80, etc) and add container capability NET_BIND_SERVICE so non-root processes can open lower ports in this container. (As explained in Docker security documentation)

// edit: fixed broken link.

[sbocinec]

Hey folks, i've just implemented my self-hosted acme-dns instance into production using the modified version of the Dockerfile: 43c4f1b

I don't want to create a PR yet as i see you are trying to solve similar docker image improvements here. My proposal is to use a multi stage docker build using the scratch as a final image. This has following features:

• resulting image contains only the acme-dns binary
• much more secure and decreased attack surface by using the scratch image
• backward compatible with the old image

This solves all the previous implementation discussions that should be rather solved by outside of the project:

• USER/uid discussion - you don't need to solve this for scratch images
• EXPOSE/VOLUME - those are exposed/mapped overriden at the container runtime and are implementation-specific. In the Dockerfile they have practically only informational purpose.
• config shouldn't be included in the docker image as the default one is not possible to use for the real implementation either

I think this can significantly your whole workflow.

[zokradonh]

Root in scratch image is fine for my level of paranoia.

Would make things much more simple.