The Docker doc spells out that there are security concerns of adding a user to the docker group. Unfortunately, they do not precisely give what the concern is. I guess that is a “security-by-obscurity” approach trying to avoid bad things. Practice show this isn’t useful: the bad guys know anyways, and the casual user has a bad time understanding the actual risk involved.
It is considerable, so let me explain at least one risk (I have not tried exhaustively check security issues): The containers are usually defined to run as root user. This permits you to bypass permission checks on the host.
Let’s assume a $USER is inside the docker group and otherwise has just installed docker. So he can run
$ docker run -v/etc:/malicious -ti –rm alpine
# cd /malicious
# vi sudoers
…. edit, write …
As such, the user can modify system config that he could not access otherwise. It’s a real risk. If you have a one-person “personal” machine/VM where the user has sudo permissions in any case … I’d say it’s no real issue.
The story is a different one on e.g. a CI machine. It’s easy to inject bad code into public pull requests, and so it’ll run on the CI platform. Usually (before spectre/meltdown…), this was guarded by the (low) permissions of the CI worker user (if you run CI with a sudo-enabled user … nothing changed). When you enable it to use docker, you now get this new class of attack vector. Don’t get me wrong: I do NOT advocate against using docker in CI. Right the opposite, it’s an excellent tool there. I just want to make you aware that you need to consider and mitigate another attack vector.
Thanks to the new improved CI workflow, we do no longer manually need to do a final check of pull requests. I have used the new system for roughly two weeks now without any problems. Consequently, I have just removed the master-candiate branch from our git (with a backup “just in case” currently remaining in the adiscon git repository).
Anyone contributing, please check the CI status of your PRs, as we can only merge things that pass the CI run. Note, though, that there still is a very limited set of tests which may falsly fail. Their number is shrinking, and I usually catch these relatively shortly and restart them. If in doubt, please add a comment to the PR and I’ll investigate.
Roughly one and a half year ago we at the rsyslog project started to get serious with CI, that time with travis only. Kudos to Thomas D. “whissi” for suggesting this and helping us to setup the initial system. In aid of CI, we have changed to a purely Pull Request (PR) driven develpoment model, and have made great success with that.
Over time, we have added more CI ressources (thanks to Digital Ocean for capacity sponsorship!) and begun to use Buildbot to drive those. Buildbot is a great tool, and has helped us tremendously to further improve software quality. Unfortunately, though, it does not offer as close integration into (guthub) PRs as Travis does. This resulted in a workflow where we had all PRs initially checked by Travis and, if all went well, I manually merged them to master-candidate branch, which Buildbot monitored. In those infrequent cases where the buildbot tests detected problems, I needed to manually contact the PR submittors. This worked well, but required some effort on my part.
The past two week we designed and implemented a small script that integrates github with buildbot much like Travis does. In essence, a new PR (or an update to an existing one) now automatically initiates the buildbot build AND the result is shown right on github inside the PR. That’s pretty sweet as it a) keeps submittors informed of everything, b) provides even better coverage of multiple platfrom testing and c) saves me from a lot of manual labor. Note that at the moment we see some infrequent quirks from this system (like some buildbot slaves not reporting, probably due to temporary network issues), but it already works much better than the old manual system. Also, I still have the capability to check things manually if there is a quirk.
As a consequence, we will change the workflow once again, removing master-candidate branch from it. Now that each and every PR is checked with all checks we have, there is no need to have an interim step when finally merging.