Run coredns as non root. #5969
Run coredns as non root. #5969
Conversation
|
(closed and reopened to re-trigger the failed circleci test) |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #5969 +/- ##
==========================================
+ Coverage 55.70% 57.52% +1.82%
==========================================
Files 224 246 +22
Lines 10016 16058 +6042
==========================================
+ Hits 5579 9237 +3658
- Misses 3978 6259 +2281
- Partials 459 562 +103
... and 156 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: Vinayak Goyal <vinaygo@google.com>
|
Weird, CircleCI is having issues. |
|
/retest |
That doesn't work in this repo. The Circle CI failures are some kind of Circle CI infrastructure issue. FYI, the error doesnt go away after a successful run... but in this case, we can see that he kubernetes ci tests are not present, so Circle CI failed to start them. Please amend your commit and force push - that should re-run the tests. |
|
closing and re-opening to re-run circle ci tests. |
|
Guys, just to let you know: after upgrading, my CoreDNS Docker container broke because of this change. The command I was using to run the container was: Adding Not complaining tho 🙂 Just posting my experience here in the hope that it may be useful to someone |
You could also use Although I agree with you, that seems like an oversight for a minor version bump. |
|
This change is listed in the release notes. https://github.com/coredns/coredns/blob/master/notes/coredns-1.11.0.md |
Yes, but it doesn't mention anything about the implicit Likewise there was no discussion here about the breakage it introduced for those who already were using unprivileged ports and explicitly dropping the capability that is now mandatory for the binary to run regardless even if you don't bind to privileged ports. According to the capabilities man page, the For example: https://github.com/ik5/gocapng |
|
I think we should consider rolling this change back, since it's causing problems for people. |
Yes, please be consistent with the binary 🙏 If you want to address the non-root capability concern, do so properly as my last comment suggests:
Please revert the I looked into this and documented justification extensively at the time via the PR: #6320 I know it is very verbose, but I am confident it's the better approach. The only blocker was the kubernetes test failure, which I cannot afford to investigate. If you're able to implement the Go API instead, that would presumably fix it and benefit anyone wanting the same feature by default without the Docker image. |
That "proper" way clearly states the need to have setcap on the binary: https://github.com/ik5/gocapng?tab=readme-ov-file#building-requirements That's why dropping setcap is likely breaking Kubernetes. (After waiting for 5 minutes CircleCI isn't loading the log, so I can't be sure...) The correct fix using capabilities is to use ambient capabilities, but those aren't currently supported in Kubernetes: kubernetes/kubernetes#56374 (and likely won't be in the short term)... Probably the better fix is to do similar to Docker does as you point out, and set |
|
Apologies for the verbosity in response below, and my lack of k8s knowledge which probably shows below 😅 TL;DR: Better to switch back to
Original response (click to expand)
Thanks for clarifying that. I had a misunderstanding and for non-root you're right, but if switching back to how CoreDNS was distributed as an image before (which was fine IMO), the API can be used and I don't think the minor convenience should be favored from using it for non-root. Instead that should be clearly documented for the security conscious that insist on running it as non-root. I am of the impression that they're simply interested in doing it for the sake of it because they don't know any better and are overly cautious/paranoid, plus it's simpler for them than to be explicit about the security context needed. If they care enough about it, better to inform correct configuration via documentation instead. https://man7.org/linux/man-pages/man7/capabilities.7.html
Relying on The API in that context cannot manage the capabilities in the same way at runtime, unless a parent process with privileges is running it and then manages privileges for the child process. Services like Postfix commonly do this, but if you insist on running the container without root user, you're stuck with the capability-dumb approach via file attributes. --
This error is not helpful as shown in #6320 you get: Prior to the introduction of Outside of containerization, you'd often run CoreDNS as root yes? It's not distributed with the same capability-dumb approach outside of the Docker image and worked fine prior to introducing that into the image. The only reason it was merged was a vague request to support running as non-root. You could run as non-root and bind to an unprivileged port, no issues. It doesn't change the fact that the privileged port is being run from the host as root? If running a Docker container via a rootless daemon, then you could have root in the container and bind to A container exploit does not require root to break out, there's even less attack surface with For non-root user, the only reason the capability is required is to bind a port lower than 1024. This isn't an issue on Docker by default as I've illustrated in #6302 while elsewhere you should be capable of the same approach, setting the You can apply the
Yes, understood. It's because it's running unprivileged and relies on file attribute granted from root to permit the capability. Personally, I don't like how that also affects the binary when run as root, since being capability-dumb still incurs the early exit check with the unhelpful error.
Not available in Docker either AFAIK. However both allow you to control the capabilities from the context of the container as a whole. Since only one process is run this seems adequate. Those that are security conscious and really care about the additional security practices should be comfortable with the explicit approach as described earlier. As root, CoreDNS could drop all capabilities by default and only keep those that are necessary. Although I'm not sure how well that'd work due to the plugin system, they'd likewise have to express the capabilities they need. Alternatively, the capabilities can be checked, along with if running privileged or unprivileged, if CoreDNS should better communicate a failure and advise a user (eg: link to documentation, or suggest using
That's almost a decade old release series. Is there known deployments of CoreDNS with k8s on RHEL 7 currently? Is that really worth supporting, or just an unknown? RHEL 7 is presently in maintenance and soon to switch to extended support, but I imagine those unable to migrate to newer version of RHEL aren't as critical about security as they are other factors? They are likely able to afford the expertise to maintain their own customizations to support their needs, or are comfortable remaining on an older CoreDNS release (that problem will affect them elsewhere in that same sense, so it's not adding any new friction) A 3.x kernel is very old for running a containerized environment on. There's no cgroups v2 support, which is important for running Docker for some time IIRC, and I wouldn't be surprised if that applies to k8s. I really don't see RHEL 7 as a valid blocker? 🤷 UPDATE: Actually it looks like the main problem is just setting That would prevent the binary failing from a kernel check on effective capabilities being set too early (via |
1. Why is this pull request needed and what does it do?
To run coredns as nonroot using the nonroot distroless base image.
2. Which issues (if any) are related?
3. Which documentation changes (if any) need to be made?
4. Does this introduce a backward incompatible change or deprecation?
No