Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop capabilities in Kubernetes DaemonSet example #3028

Merged
merged 2 commits into from
Mar 22, 2018

Conversation

unkaktus
Copy link

@unkaktus unkaktus commented Mar 15, 2018

What does this PR do?

Changes example of running DaemonSet on Kubernetes.
Makes Traefik run not in privileged mode but with only capability to bind to ports < 1024.

Motivation

Reducing attack surface.

The motivation behind privileged: true was to have ability for Traefik to bind on privileged ports. Though there is a more fine-grained mechanism to restrict containers capabilities. This is unnecessary for the container to run in privileged mode.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Probably there was another rationale behind running in privileged. If there is it should be added explicitly to the capability list.

@dtomcej
Copy link
Contributor

dtomcej commented Mar 16, 2018

I like where this is going, but there are a couple of issues I forsee:

  1. If we can only bind to ports <1024, 8080 is a common dashboard port, and we would cause issues there.
  2. This only works in linux. I am unsure if there are any other environments where k8s runs, but it is something to note
  3. I am not sure what the minimum version of k8s is that would allow for that spec. We could pose it as an alternative, for users that are running the latest k8s, and don't have an issue with (1) or (2).

@timoreimann thoughts?

@unkaktus
Copy link
Author

@dtomcej,

  1. It is not the only capability in general sense - it is only Linux capability. 🙂 It still can bind on non-privileged ports too.
    Actually everything works in privileged: false because there are many capabilities added by default.
  2. It also runs on Windows I guess... and I am pretty sure that in non-Linux deployments these Linux-specific things are just being ignored.
  3. This feature appears in at least Kubernetes v1.4 which is pretty old (https://kubernetes-v1-4.github.io/docs/api-reference/v1/definitions/#_v1_securitycontext). So I think that it safe to not make it an alternative.

@timoreimann
Copy link
Contributor

timoreimann commented Mar 16, 2018

I think we're good on 3. Although we have never formally defined which versions of Kubernetes we want to support with Traefik, I think it makes sense to match Kubernetes' rule to provide support for the previous two releases. In that sense, 1.4 has long dropped out.

Re: 2, at least the list of known limitations for Windows does not mention capabilities. Maybe it's obvious that it's not supported because they are Linux capabilities; then again, I don't know how Windows containers work under the hood. Mac doesn't support containers natively either, and yet it works because there's a virtualization layer somewhere. I know @vdemeester has approximately 42 dev machines sitting on his desk, maybe he can shed some light here. :-)

Re: 1: One thing we should do before merging is test if this works as expected against minikube or even better some real cluster. @nogoegst by any chance, have you tested this configuration already?

@unkaktus
Copy link
Author

@timoreimann, yes I've successfully tested it on bare-metal v1.9 cluster (Ubuntu 16.04 kernel). Though I think that it should be tested by someone else because it is a works-for-me now.

@dtomcej
Copy link
Contributor

dtomcej commented Mar 16, 2018

Well I'm on board, after doing some reading :)

And I fully appreciate the dropping capabilities from root -> just port binding abilities. 👍

@timoreimann timoreimann changed the title Drop capabilites in Kubernetes DaemonSet example Drop capabilities in Kubernetes DaemonSet example Mar 16, 2018
@nmengin nmengin added this to the 1.6 milestone Mar 20, 2018
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR!

Tested on Minikube, and it works for me.

But can you update the documentation too?

@timoreimann
Copy link
Contributor

@juliens do we actually have documentation in this regard?

@unkaktus
Copy link
Author

@timoreimann I guess that @juliens meant that there should be some documentation for this even though it is not there at the moment.

@timoreimann
Copy link
Contributor

Gotcha. If it means adding a quick sentence to the guide, I'm good with it.

@juliens
Copy link
Member

juliens commented Mar 21, 2018

In fact, I talk about update this part https://docs.traefik.io/user-guide/kubernetes/

@unkaktus
Copy link
Author

@juliens, yes and there is no line about why privileged is true. Albeit it says that DaemonSet can bind to privileged ports.

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
:shipit:

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 📖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants