Mount only the docker.sock file instead of the whole root path of the host about ksniff HOT 12 CLOSED

Hello @bostrt just kindly remind you that I've created the PR, thanks!

from ksniff.

@bostrt great! I would come out with the PR soon, thanks!

from ksniff.

Hi @namco1992 thanks for opening this issue! There's a few things to unravel here..

When the pod is created, it mounts the whole / host path under the /host path of the pod.

The last ksniff release shipped with support for multiple container runtimes. Right now, just Docker and CRI-O, but I'm looking into containerd as well (k3s, microk8s). I think there's some room for improvement in terms of selectively mounting what is needed for ksniff to function and I'm open to suggestions. However, we must keep in mind that using ksniff in privileged mode (-p) means the user would still have privilege to create a Pod / host mount or create a new container via the selectively mounted Docket socket that uses / host mount. So there's honestly no protection to be added to ksniff but I think we could improve it's "appearance" from the code for what that's worth.

However, when the Service Account Admission Manager came into the play, it will mount the token under /var/run/secrets/kubernetes.io/serviceaccount, and both the /var/run and /host/var/run is a symlink to /run, the /host/var/run/docker.sock is gone and docker won't be able to connect to the docker daemon.

I'm having trouble understanding how the Service Account Admission Manager comes into play. I've not had any issues with it in OpenShift 3.11 (k8s 1.11), OpenShift 4.5 (k8s 1.18), and vanilla Kubernetes 1.19.2.

Can you elaborate on what you are seeing there? Its possible that you are seeing the issue being patched through #86.

from ksniff.

Hello @bostrt Thank you for the reply!

For the first question, I believe that even though the user should understand the risk of using the privileged mode, ksniff should only mount the minimum path it needs. For example, for docker runtime, this should be sufficient:

spec:
  containers:
    ...
    volumeMounts:
    - mountPath: /var/run/docker.sock
      name: docker-sock
      readOnly: true
  ...
  volumes:
  - hostPath:
      path: /var/run/docker.sock
      type: ""
    name: docker-sock

For the second question, although I'm not 100% sure but I would like to give my 2 cents:

1. ksniff mount the / hostPath to /host in the container, there is /host/var/run which is a symlink to /run;
2. Service Account Admission Manager mounts /var/run/secrets/kubernetes.io/serviceaccount, and again /var/run is a symlink to /run too, so the /run is overwritten;
3. From now on you won't be able to see the /var/run from the host (which should be accessible via /host/var/run). Instead, you only see the overwritten /run path which contains only /var/run/secrets/kubernetes.io/serviceaccount.

This is what I observed from my tests, and correct me if I'm wrong pls.

Then we come back to the fix #86, I think instead of guessing the file location, we should mount the docker.sock file as the above example shows and everything should work as expected. I still stick to my view that mounting the whole root directory from hostPath is not ideal and could lead to side effects.

I'm not sure why you can't replicate the issue, perhaps the Service Account Admission Manager is not enabled by default on the platforms you tried. I tested on GKE and this issue #82 mentioned it's using AKE, so I suppose it's common within the cloud providers.

I would like to create a PR for this if you think it's a reasonable approach. Thanks!

from ksniff.

Sorry for the delay. Regarding your first point. I completely agree. It is feasible to have ksniff to selectively mount but we must keep issues like #86 in mind. That's something I didn't consider in the original implementation.

Regarding the second issue, I think I understand it now but want to ask one more question to hopefully solidify it:

In your test environments, does the symlink look like this: /var/run -> /run or does it look like this: /var/run -> ../run? In other words, is the symlink absolute or relative path?

In my test environments, its relative which may explain why I'm not having the same issues. Unfortunately, I do not have access to GKE or AKE for testing ksniff :(

from ksniff.

In your test environments, does the symlink look like this: /var/run -> /run or does it look like this: /var/run -> ../run? In other words, is the symlink absolute or relative path?

I think this is it! I just compared RHEL/Fedora and Ubuntu/Debian container images and they do indeed symlink /var/run differently! Interesting difference and likely a cause of problems when mounting all of /.

I would certainly welcome a PR resolving this and happy to review it.

from ksniff.

Yes indeed, today I also learned something new, thanks! I will create a PR then.

from ksniff.

@bostrt IMO #86 is trying to fix it from the facade behavior instead of the root cause. I believe that if we mount the sock file precisely it shouldn't need to "guess" the sock file path.

I propose to make 2 changes:

1. Have a default socket path for each runtime (e.g. /var/run/docker.sock for docker, /run/containerd/containerd.sock for containerd, etc.)
2. Accept an argument for passing in the socket path to support the scenario that the socket path is different from the default path (e.g. the DOCKER_SOCKET is changed to another path in the docker conf)

However, this might be a conflict with #86, and potentially conflict with your work to support containerd runtime. What do you think?

from ksniff.

@namco1992 Agreed. We didn't arrive to a real root cause during conversation for the changes in #86.

1. Have a default socket path for each runtime (e.g. /var/run/docker.sock for docker, /run/containerd/containerd.sock for containerd, etc.)
2. Accept an argument for passing in the socket path to support the scenario that the socket path is different from the default path (e.g. the DOCKER_SOCKET is changed to another path in the docker conf)

Yes and yes, I think both of these are the best options. We briefly touched on this in #82 but unknowingly dismissed it since the root cause wasn't really known.

However, this might be a conflict with #86, and potentially conflict with your work to support containerd runtime. What do you think?

I'll update it asking to hold (or just close the PR) the changes per this conversation.

from ksniff.

hi @namco1992! I wanted to follow-up and see if you have made any progress on a PR.

from ksniff.

Hello @bostrt sorry I was occupied with work stuffs recently, I'm in the middle of it, should be able to submit the PR on Wednesday (UTC+8).

from ksniff.

thanks @namco1992! we'll have to await @eldadru to review and merge if everything checks out.

from ksniff.