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

Support user namespaces in partition check (1.2.1) #444

Merged
merged 2 commits into from
Sep 29, 2020
Merged

Support user namespaces in partition check (1.2.1) #444

merged 2 commits into from
Sep 29, 2020

Conversation

markdumay
Copy link
Contributor

The current partition check (1.2.1) does not adequately support user namespaces (see e.g. #332). When user namespaces are enabled, Docker creates a subfolder in the Docker root directory (defaults to /var/lib/docker). This subfolder is based on the user ID and group ID of the user dockermap. The current check with mountpoint -- "$(docker info -f '{{ .DockerRootDir }}')" fails in this case, as the Docker Root Dir returns both the root directory and the subfolder, e.g. /var/lib/docker/165536.165536/ instead of /var/lib/docker/.

The suggested code change identifies the partition the Docker Root Dir belongs to, using df instead of mountpoint. If this partition differs from the system's partition (identified by /), the test passes.

@konstruktoid
Copy link
Collaborator

konstruktoid commented Sep 28, 2020

Hi @markdumay and thanks for the PR. I like the solution but I don't think it will fix the problem, e.g if /var/lib/docker is "just" a directory mounted on /var and not a separate partition.

Example:

~$ cat docker_test.sh 
#!/bin/bash
  system_partition=$(df / --output=source 2> /dev/null | sed -n 2p)
  docker_partition=$(df "$(docker info -f '{{ .DockerRootDir }}')" --output=source 2> /dev/null | sed -n 2p)
  if [ "$system_partition" != "$docker_partition" ] && [ -n "$docker_partition" ] ; then
    echo "DockerRootDir is $(docker info -f '{{ .DockerRootDir }}')"
    echo "docker_partition is $docker_partition and system_partition is $system_partition"
    mountpoint -- "$(docker info -f '{{ .DockerRootDir }}')"
  fi
~$ sh docker_test.sh 
DockerRootDir is /boot/docker
docker_partition is /dev/sda1 and system_partition is /dev/mapper/lab--base--vg-root
/boot/docker is not a mountpoint

We would pass the test but flood the /boot partition.

~$ sudo rm /etc/docker/daemon.json 
~$ sudo systemctl stop docker
~$ sudo systemctl start docker
~$ sh docker_test.sh 
DockerRootDir is /var/lib/docker
docker_partition is /dev/sdb and system_partition is /dev/mapper/lab--base--vg-root
/var/lib/docker is a mountpoint
~$ grep docker /etc/fstab 
/dev/sdb /var/lib/docker/ ext4 defaults  0 0

@markdumay
Copy link
Contributor Author

markdumay commented Sep 28, 2020

hi @konstruktoid, thanks for your quick reply! You're right in pointing out /boot is also at risk. To extend on that, wouldn't it suffice to confirm the Docker root folder doesn't belong to either the partition for / or /boot?

For example, let's run lsblk:

~$ sudo lsblk -o NAME,FSTYPE,SIZE,MOUNTPOINT,LABEL

On my server, this returns:

NAME   FSTYPE     SIZE MOUNTPOINT                   LABEL
[...]
sda               200G                              
├─sda1 ext4       953M /boot                        
├─sda2 ext4     101.4G /                            
└─sda3 ext4      97.7G /data/docker                 

My suggestion is to test for partitions rather than mount points. In below example, all files and folders belong to the partition sda2 by default, with the exception of /boot and /data/docker. On my server, docker info -f '{{ .DockerRootDir }}' returns /data/docker/165536.165536 because of support for user namespaces. This is indeed not a mount point, however, it does belong to the same sda3 partition because its parent is a mount point.

The revised test function could look something like this:

# 1.2.1
check_1_2_1() {
  id_1_2_1="1.2.1"
  desc_1_2_1="Ensure a separate partition for containers has been created (Scored)"
  check_1_2_1="$id_1_2_1 - $desc_1_2_1"
  starttestjson "$id_1_2_1" "$desc_1_2_1"

  totalChecks=$((totalChecks + 1))
  local boot_partition=$(df /boot --output=source 2> /dev/null | sed -n 2p)
  local system_partition=$(df / --output=source 2> /dev/null | sed -n 2p)
  local docker_partition=$(df "$(docker info -f '{{ .DockerRootDir }}')" --output=source 2> /dev/null | sed -n 2p)
  if [ "$boot_partition" != "$docker_partition" ] && [ "$system_partition" != "$docker_partition" ] ; then
    pass "$check_1_2_1"
    resulttestjson "PASS"
    currentScore=$((currentScore + 1))
  else
    warn "$check_1_2_1"
    resulttestjson "WARN"
    currentScore=$((currentScore - 1))
  fi
}

@konstruktoid
Copy link
Collaborator

konstruktoid commented Sep 28, 2020

/boot was just an example, using the default /var/lib/docker would fill up /var containing logs et al. #332 brings this up.

Perhaps adding | sed 's/\/[0-9].*\.[0-9].*\/$//g' would work?

mountpoint -- "$(docker info -f '{{ .DockerRootDir }}' | sed 's/\/[0-9].*\.[0-9].*\/$//g')"

@markdumay
Copy link
Contributor Author

I'm not sure I follow what you mean? If /var/lib/docker and (any of its subfolders) were mounted to a different partition, it should pass the test, correct? To clarify, /var can be mounted to sda2 and /var/lib/docker can be mounted to sda3. In this case, the sda3 partition prevents any Docker files flooding partition sda2.

@konstruktoid
Copy link
Collaborator

This is correct, but I see you added boot_partition and if [ "$boot_partition" != "$docker_partition" ] which isn't relevant to the issue.

@markdumay
Copy link
Contributor Author

markdumay commented Sep 28, 2020 via email

@konstruktoid
Copy link
Collaborator

The CIS test asks a quite simple question: Is the Docker data directory placed on a separate partition?
However, when using user namespaces things get a bit more complicated.

I personally don't care which solution we use as long as it shows the correct result.

After you opened this PR, and with the following discussion, I just thought we all might be over-complicating things.
Why not just remove the added user namespaces directory and just test the non-namespace directory?

@markdumay
Copy link
Contributor Author

markdumay commented Sep 28, 2020 via email

@konstruktoid
Copy link
Collaborator

df "$(docker info -f '{{ .DockerRootDir }}' | sed 's/\/[0-9].*\.[0-9].*\/$//g')" --output=source but we still need to verify that it's a mounted partition.

grep "$(df "$(docker info -f '{{ .DockerRootDir }}' | sed 's/\/[0-9].*\.[0-9].*\/$//g')" --output=source)" /proc/mounts perhaps.

@markdumay
Copy link
Contributor Author

What about the following code example?

  1. We determine the Docker Root Dir as usual with docker info -f '{{ .DockerRootDir }}'
  2. We then test whether Docker has enabled user names spaces with docker info | grep -q userns
  3. We strip the last sub directory only if user name space are enabled with readlink -f "$docker_root_dir/.."
  4. We then continue with the original test
docker_root_dir=$(docker info -f '{{ .DockerRootDir }}')
if docker info | grep -q userns ; then
  echo "Userns enabled"
  docker_root_dir=$(readlink -f "$docker_root_dir/..")
else
  echo "Userns disabled"
fi

echo "$docker_root_dir"
if mountpoint -q -- "$docker_root_dir" >/dev/null 2>&1; then
  echo "PASS"
else
  echo "WARN"
fi

@konstruktoid
Copy link
Collaborator

Yeah, that looks good. Want to update your PR?

@markdumay
Copy link
Contributor Author

OK, the PR has been updated with the proposed code change.

@konstruktoid
Copy link
Collaborator

Thanks @markdumay!

@konstruktoid konstruktoid merged commit f3e9c79 into docker:master Sep 29, 2020
@markdumay
Copy link
Contributor Author

markdumay commented Sep 29, 2020 via email

@markdumay markdumay deleted the partition branch September 29, 2020 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants