search

2i2c-org / infrastructure

Infrastructure for configuring and deploying our community JupyterHubs.
https://infrastructure.2i2c.org
BSD 3-Clause "New" or "Revised" License
103 stars 57 forks source link

Non-Admins can see shared-readwrite directory when they shouldn't #2946

Open sgibson91 opened 11 months ago

sgibson91 commented 11 months ago

Context

ref: https://2i2c.freshdesk.com/a/tickets/870

There seems to be a bug on the OHW hub where non-admin users can see the shared-readwrite folder which is supposed to be admin only. We know at least one non-admin who can see it cannot write to it, so if that is true across the board, damage is limited and we are just dealing with confused users. We establish "who is an admin" via some custom Kubespawner code. I think a lot of work has been going on over there recently, and I'm wondering if something could have broken/modified this functionality a bit? But I'm not sure how to go about digging into this one.

Proposal

No response

Updates and actions

No response

consideRatio commented 11 months ago

I've started looking into this and can and it doesn't look like its a security issue at least, its just a bad UX presenting an seeminlgy empty folder.

I tried starting a non-admin user's server and saw this:

image

The pod created by KubeSpawner revealed:

  # pod spec snippet
  volumes:
  - name: home
    persistentVolumeClaim:
      claimName: home-nfs

  # main container spec snippet
    volumeMounts:
    - mountPath: /home/jovyan
      name: home
      subPath: redacted-username
    - mountPath: /home/jovyan/shared
      name: home
      readOnly: true
      subPath: _shared

  # init container spec snippet
    volumeMounts:
    - mountPath: /home/jovyan
      name: home
      subPath: redacted-username
    - mountPath: /home/jovyan/shared-readwrite
      name: home
      subPath: _shared

  # init container's command
  - command:
    - sh
    - -c
    - 'id && chown 1000:1000 /home/jovyan && chown 1000:1000 /home/jovyan/shared-readwrite
      && ls -lhd /home/jovyan '
consideRatio commented 11 months ago

This UX bug is seen in other clusters as well, not just the 2i2c cluster's ohw hub with GKE v1.26, such as openscapes on EKS k8s 1.24.

Running the mount command from a terminal as a non-admin user showed this.

On 2i2c / ohw

10.234.45.250:/homes/homes/ohw/_shared on /home/jovyan/shared           type nfs (ro,noatime,vers=3,rsize=1048576,wsize=1048576,namlen=255,soft,proto=tcp,timeo=600,retrans=2,sec=sys,mountaddr=10.234.45.250,mountvers=3,mountport=2050,mountproto=tcp,local_lock=none,addr=10.234.45.250)
10.234.45.250:/homes/homes/ohw/_shared on /home/jovyan/shared-readwrite type nfs (rw,noatime,vers=3,rsize=1048576,wsize=1048576,namlen=255,soft,proto=tcp,timeo=600,retrans=2,sec=sys,mountaddr=10.234.45.250,mountvers=3,mountport=2050,mountproto=tcp,local_lock=none,addr=10.234.45.250)

On openscapes / prod

fs-b25253b5.efs.us-west-2.amazonaws.com:/prod/_shared on /home/jovyan/shared type nfs4 (rw,relatime,vers=4.1,rsize=1048576,wsize=1048576,namlen=255,soft,noresvport,proto=tcp,timeo=600,retrans=2,sec=sys,clientaddr=192.168.43.179,local_lock=none,addr=192.168.63.243)

Basehub config

jupyterhub:
  custom:
    auth:
      anonymizeUsername: false
    singleuser:
      extraPVCs: []
    singleuserAdmin:
      extraEnv: {}
      extraVolumeMounts:
        - name: home
          mountPath: /home/jovyan/shared-readwrite
          subPath: _shared

Basehub singleuser.initContainers

  singleuser:
    # Need to explicitly fix ownership here, as otherwise these directories will be owned
    # by root on most NFS filesystems - neither EFS nor Google Filestore support anonuid
    initContainers:
      - name: volume-mount-ownership-fix
        image: busybox
        command:
          [
            "sh",
            "-c",
            "id && chown 1000:1000 /home/jovyan && chown 1000:1000 /home/jovyan/shared-readwrite && ls -lhd /home/jovyan ",
          ]
        securityContext:
          runAsUser: 0
        volumeMounts:
          - name: home
            mountPath: /home/jovyan
            subPath: "{username}"
          # Here so we can chown it appropriately
          - name: home
            mountPath: /home/jovyan/shared-readwrite
            subPath: _shared
consideRatio commented 11 months ago

Problem understood

I think I understand this well enough now, but I'm not sure on the resolution.

It seems that when we've mounted something inside another mount, like we how we mount a users home folder to /home/jovyan and then mounted the shared folder to /home/jovyan/shared-readwrite, we end up with an empty directory in the users volume.

So in practice, the non-admin users see the shared-readwrite directory because its part of their home directory, as a remnant of just having declared the mount to /home/jovyan/shared-readwrite inside /home/jovyan once in an init container. I've verified it wasn't part of the chown step.

    initContainers:
      - name: volume-mount-ownership-fix
        image: busybox
        command:
          [
            "sh",
            "-c",
            "id && chown 1000:1000 /home/jovyan && chown 1000:1000 /home/jovyan/shared-readwrite && ls -lhd /home/jovyan ",
          ]
        securityContext:
          runAsUser: 0
        volumeMounts:
          - name: home
            mountPath: /home/jovyan
            subPath: "{username}"
          # Here so we can chown it appropriately
          - name: home
            mountPath: /home/jovyan/shared-readwrite
            subPath: _shared

Fix for new hubs

I think by an update like below, we should be fine. I've tested this manually on staging.2i2c.cloud from a state of having cleaned up the folders and started an admin and a non-admin user, where the admin user saw staging and staging-readwrite with the correct permissions to the folders, and the non-admin saw staging only with the correct permissions to the folder.

         command:
           [
             "sh",
             "-c",
-            "id && chown 1000:1000 /home/jovyan && chown 1000:1000 /home/jovyan/shared-readwrite && ls -lhd /home/jovyan ",
+            "id && chown 1000:1000 /home/jovyan && chown 1000:1000 /home/jovyan/shared && ls -lhd /home/jovyan 
",
           ]
         volumeMounts:
           - name: home
             mountPath: /home/jovyan
             subPath: "{username}"
           # Here so we can chown it appropriately
           - name: home
-             mountPath: /home/jovyan/shared-readwrite
+             mountPath: /home/jovyan/shared
             subPath: _shared

Cleanup for old hubs

I think what needs to be done is to visit all clusters hubs storage and iterate across all users to remove all empty shared-readwrite folders we find in their home directories.

consideRatio commented 4 months ago

Here is how we can empty shared-readwrite folders put inside users home directories using a script. Note that for admin users, this empty folder won't matter since they are hidden by getting a proper folder mounted on top of it, but non-admin users will be confused by seeing an empty folder in their home directory.

deployer exec homes <cluster name>
cd home

# the command below is quite safe to run I htink,
# because `rm -d` is used, it will only delete empty folders
ls -d */shared-readwrite | xargs -t -L1 -I {} rm -d {}
consideRatio commented 4 months ago

Action point

consideRatio commented 4 months ago

I made this a "High" priority because its a good time save to get it solved before more complaints come in about it - as each complaint leads to notable amount of communication work for us etc.