Kubernetes Security Assesment: evaluate kubeadm reset problems

[neolit123]

https://github.com/kubernetes/community/blob/master/wg-security-audit/findings/Kubernetes%20Final%20Report.pdf

search for:
Kubeadm performs potentially-dangerous reset operations
Finding ID: TOB-K8S-014

the TL;DR is that reset according to the finding performs a set of Unix-ism unsafe operations.

there are also comments about errors not being returned, but i personally slightly disagree with that because kubeadm reset is best-effort and should not error out on every small error it finds.

[neolit123]

i think we should remove the awk... and replace it with:

// unmountKubeletDirectory unmounts all paths that contain KubeletRunDirectory
func unmountKubeletDirectory() error {
	raw, err := ioutil.ReadFile("/proc/mounts")
	if err != nil {
		return err // fatal error
	}
	mounts := strings.Split(string(raw), "\n")
	for _, mount := range mounts {
		m := strings.Split(mount, " ")
		if len(m) < 2 {
			continue
		}
		if !strings.HasPrefix(m[1], constants.KubeletRunDirectory) {
			continue
		}
		if err := syscall.Unmount(m[1], syscall.MNT_FORCE); err != nil {
			// klog: print warning
		}
	}
	return nil
}

which only happens in a _linux.go file and the _windows.go file is a NOP.

if k8s.io/kubernetes/pkg/util/mount can be used for this and if it's moving to a new repo (i think it is) we can use it, but i rather not block on repos moving for 20 lines of code.
so we can go ahead and make this change and have a potential TODO for the repo.

there are also comments about errors not being returned, but i personally slightly disagree with that because kubeadm reset is best-effort and should not error out on every small error it finds.

i still believe this is true and reset is best effort, unless something odd is going on e.g. missing /proc/mounts.

[neolit123]

document in both the CLI of reset and the k8s.io docs that reset is best effort:
https://kubernetes.io/docs/reference/setup-tools/kubeadm/kubeadm-reset/

[stealthybox]

i think we should remove the awk... and replace it

+1
It's possible to write a more secure shell command using a for-loop instead of xargs.
The go solution is much better.

[0xmichalis]

there are also comments about errors not being returned, but i personally slightly disagree with that because kubeadm reset is best-effort and should not error out on every small error it finds.

At least make sure these errors are logged?

[neolit123]

At least make sure these errors are logged?

a quick check does not reveal any errors that are not reported as warnings.

[neolit123]

this is fixed.