Merge pull request #5 from nebari-dev/search_for_all_volumeMounts

add additional test data, check for volumeMounts anywhere in Workflow…

nebari_workflow_controller/app.py

@@ -77,19 +77,47 @@ def base_return_response(allowed, apiVersion, request_uid, message=None):
 
 
 def find_invalid_volume_mount(
-    container, volume_name_pvc_name_map, allowed_pvc_sub_paths_iterable
+    volume_mounts, volume_name_pvc_name_map, allowed_pvc_sub_paths_iterable
 ):
     # verify only allowed volume_mounts were mounted
-    for volume_mount in container.get("volumeMounts", {}):
+    for volume_mount in volume_mounts:
         if volume_mount["name"] in volume_name_pvc_name_map:
             for allowed_pvc, allowed_sub_paths in allowed_pvc_sub_paths_iterable:
                 if volume_name_pvc_name_map[volume_mount["name"]] == allowed_pvc:
-                    if volume_mount.get("subPath", "") not in allowed_sub_paths:
-                        denyReason = f"Workflow attempts to mount disallowed subPath: {volume_mount}. Allowed subPaths are: {allowed_sub_paths}."
+                    if (
+                        sub_path := volume_mount.get("subPath", "")
+                    ) not in allowed_sub_paths:
+                        denyReason = f"Workflow attempts to mount disallowed subPath: {sub_path}. Allowed subPaths are: {allowed_sub_paths}."
                         logger.info(denyReason)
                         return denyReason
 
 
+def check_for_invalid_volume_mounts(
+    dict_or_list, volume_name_pvc_name_map, allowed_pvc_sub_paths_iterable
+):
+    """Recursively check for invalid volume mounts"""
+    if isinstance(dict_or_list, dict):
+        for key, value in dict_or_list.items():
+            if key == "volumeMounts":
+                if denyReason := find_invalid_volume_mount(
+                    value,
+                    volume_name_pvc_name_map,
+                    allowed_pvc_sub_paths_iterable,
+                ):
+                    return denyReason
+            elif isinstance(value, (list, dict)):
+                if found_invalid_volume_mount := check_for_invalid_volume_mounts(
+                    value, volume_name_pvc_name_map, allowed_pvc_sub_paths_iterable
+                ):
+                    return found_invalid_volume_mount
+    elif isinstance(dict_or_list, list):
+        for item in dict_or_list:
+            if found_invalid_volume_mount := check_for_invalid_volume_mounts(
+                item, volume_name_pvc_name_map, allowed_pvc_sub_paths_iterable
+            ):
+                return found_invalid_volume_mount
+
+
 @app.post("/validate")
 def admission_controller(request=Body(...)):
     keycloak_user = get_keycloak_user_info(request)
@@ -115,7 +143,7 @@ def admission_controller(request=Body(...)):
         )
     )
 
-    # verify only allowed volumes were mounted
+    # verify only allowed pvcs were attached as volumes
     volume_name_pvc_name_map = {}
     for volume in (
         request.get("request", {}).get("object", {}).get("spec", {}).get("volumes", {})
@@ -132,21 +160,11 @@ def admission_controller(request=Body(...)):
                     "persistentVolumeClaim"
                 ]["claimName"]
 
-    for template in request["request"]["object"]["spec"]["templates"]:
-        # verify container
-        if denyReason := find_invalid_volume_mount(
-            template["container"],
-            volume_name_pvc_name_map,
-            allowed_pvc_sub_paths_iterable,
-        ):
-            return return_response(False, message=denyReason)
-
-        # verify initContainers
-        for initContainer in template.get("initContainers", {}):
-            if denyReason := find_invalid_volume_mount(
-                initContainer, volume_name_pvc_name_map, allowed_pvc_sub_paths_iterable
-            ):
-                return return_response(False, message=denyReason)
+    # verify only allowed subPaths were mounted
+    if denyReason := check_for_invalid_volume_mounts(
+        request, volume_name_pvc_name_map, allowed_pvc_sub_paths_iterable
+    ):
+        return return_response(False, message=denyReason)
 
     logger.info(
         f"Allowing workflow to be created: {request['request']['object']['metadata']['name']}"

tests/test_app.py

@@ -1,3 +1,5 @@
+from pathlib import Path
+
 import pytest
 import yaml
 
@@ -7,30 +9,15 @@
 
 @pytest.mark.parametrize(
     "request_file,allowed",
-    # [
-    # 	(str(p), True) for p in Path('./tests/test_data/requests/pass').glob('*.yaml')
-    # ] + [
-    # 	(str(p), False) for p in Path('./tests/test_data/requests/fail').glob('*.yaml')
-    # ]
-    [
-        ("tests/test_data/requests/pass/browser_hello_world.yaml", True),
-        ("tests/test_data/requests/pass/argo_cli_hello_world.yaml", True),
-        ("tests/test_data/requests/pass/jupyterlab_pod.yaml", True),
-        ("tests/test_data/requests/pass/kubectl_malicious.yaml", True),
-        ("tests/test_data/requests/fail/initContainer_empty_subPath.yaml", False),
-        ("tests/test_data/requests/fail/container_empty_subPath.yaml", False),
-        ("tests/test_data/requests/fail/disallowed_volume.yaml", False),
-        ("tests/test_data/requests/fail/container_disallowed_file_mount.yaml", False),
-        (
-            "tests/test_data/requests/fail/initContainer_disallowed_conda_mount.yaml",
-            False,
-        ),
-        ("tests/test_data/requests/fail/container_disallowed_conda_mount.yaml", False),
-        (
-            "tests/test_data/requests/fail/initContainer_disallowed_file_mount.yaml",
-            False,
-        ),
-    ],
+    sorted(
+        [(str(p), True) for p in Path("./tests/test_data/requests/pass").glob("*.yaml")]
+    )
+    + sorted(
+        [
+            (str(p), False)
+            for p in Path("./tests/test_data/requests/fail").glob("*.yaml")
+        ]
+    ),
 )
 def test_admission_controller(mocker, request_file, allowed):
     mocker.patch(

tests/test_data/README.md

@@ -0,0 +1,3 @@
+A lot of the requess came from https://github.com/argoproj/argo-workflows/tree/master/examples.
+
+Then modified with convert_workflows.py.

tests/test_data/convert_workflows.py

@@ -0,0 +1,58 @@
+from pathlib import Path
+
+import yaml
+
+preamble = """
+apiVersion: admission.k8s.io/v1
+kind: AdmissionReview
+request:
+    dryRun: false
+    kind:
+        group: argoproj.io
+        kind: Workflow
+        version: v1alpha1
+    name: hello-world
+    namespace: dev
+    operation: CREATE
+    options:
+        apiVersion: meta.k8s.io/v1
+        kind: CreateOptions
+    requestKind:
+        group: argoproj.io
+        kind: Workflow
+        version: v1alpha1
+    requestResource:
+        group: argoproj.io
+        resource: workflows
+        version: v1alpha1
+    resource:
+        group: argoproj.io
+        resource: workflows
+        version: v1alpha1
+    uid: c1bba5c6-2189-41ff-9487-be504c04487b
+    userInfo:
+        groups:
+        - system:serviceaccounts
+        - system:serviceaccounts:dev
+        - system:authenticated
+        uid: eac0d7ab-af84-4c3f-a5fd-71845ff9e8c9
+        username: system:serviceaccount:dev:argo-admin
+"""
+
+new_request = yaml.load(preamble, Loader=yaml.FullLoader)
+files = Path("./requests/pass").glob("*.yaml")
+
+for request_file in files:
+    with open(request_file, "r") as f:
+        try:
+            request = yaml.load(f, Loader=yaml.FullLoader)
+        except Exception:
+            print(str(request_file))
+            breakpoint()
+    if request["kind"] == "Workflow":
+        if value := request["metadata"].get("generateName"):
+            request["metadata"]["name"] = value
+            request["metadata"].pop("generateName")
+        new_request["request"]["object"] = request
+        with open(request_file, "w") as f:
+            yaml.dump(new_request, f)

tests/test_data/requests/fail/container_disallowed_conda_mount.yaml

@@ -78,7 +78,7 @@ request:
                     -   mountPath: /home/conda/global
                         name: conda-store
                         subPath: global
-                    -   mountPath: /home/conda/super-admin
+                    -   mountPath: /home/conda/super-admin  # disallowed
                         name: conda-store
                         subPath: super-admin
                     -   mountPath: /home/conda/analyst

tests/test_data/requests/fail/volumes-existing.yaml

@@ -0,0 +1,74 @@
+apiVersion: admission.k8s.io/v1
+kind: AdmissionReview
+request:
+  dryRun: false
+  kind:
+    group: argoproj.io
+    kind: Workflow
+    version: v1alpha1
+  name: hello-world
+  namespace: dev
+  object:
+    apiVersion: argoproj.io/v1alpha1
+    kind: Workflow
+    metadata:
+      name: volumes-existing-
+    spec:
+      entrypoint: volumes-existing-example
+      templates:
+      - name: volumes-existing-example
+        steps:
+        - - name: generate
+            template: append-to-accesslog
+        - - name: print
+            template: print-accesslog
+      - container:
+          args:
+          - 'echo accessed at: $(date) | tee -a /mnt/vol/accesslog'
+          command:
+          - sh
+          - -c
+          image: alpine:latest
+          volumeMounts:
+          - mountPath: /mnt/vol
+            name: workdir
+        name: append-to-accesslog
+      - container:
+          args:
+          - echo 'Volume access log:'; cat /mnt/vol/accesslog
+          command:
+          - sh
+          - -c
+          image: alpine:latest
+          volumeMounts:
+          - mountPath: /mnt/vol
+            name: workdir
+        name: print-accesslog
+      volumes:
+      - name: workdir
+        persistentVolumeClaim:
+          claimName: my-existing-volume
+  operation: CREATE
+  options:
+    apiVersion: meta.k8s.io/v1
+    kind: CreateOptions
+  requestKind:
+    group: argoproj.io
+    kind: Workflow
+    version: v1alpha1
+  requestResource:
+    group: argoproj.io
+    resource: workflows
+    version: v1alpha1
+  resource:
+    group: argoproj.io
+    resource: workflows
+    version: v1alpha1
+  uid: c1bba5c6-2189-41ff-9487-be504c04487b
+  userInfo:
+    groups:
+    - system:serviceaccounts
+    - system:serviceaccounts:dev
+    - system:authenticated
+    uid: eac0d7ab-af84-4c3f-a5fd-71845ff9e8c9
+    username: system:serviceaccount:dev:argo-admin

tests/test_data/requests/pass/archive-location.yaml

@@ -0,0 +1,51 @@
+apiVersion: admission.k8s.io/v1
+kind: AdmissionReview
+request:
+  dryRun: false
+  kind:
+    group: argoproj.io
+    kind: Workflow
+    version: v1alpha1
+  name: hello-world
+  namespace: dev
+  object:
+    apiVersion: argoproj.io/v1alpha1
+    kind: Workflow
+    metadata:
+      name: archive-location-
+    spec:
+      entrypoint: whalesay
+      templates:
+      - archiveLocation:
+          archiveLogs: true
+        container:
+          args:
+          - hello world
+          command:
+          - cowsay
+          image: docker/whalesay:latest
+        name: whalesay
+  operation: CREATE
+  options:
+    apiVersion: meta.k8s.io/v1
+    kind: CreateOptions
+  requestKind:
+    group: argoproj.io
+    kind: Workflow
+    version: v1alpha1
+  requestResource:
+    group: argoproj.io
+    resource: workflows
+    version: v1alpha1
+  resource:
+    group: argoproj.io
+    resource: workflows
+    version: v1alpha1
+  uid: c1bba5c6-2189-41ff-9487-be504c04487b
+  userInfo:
+    groups:
+    - system:serviceaccounts
+    - system:serviceaccounts:dev
+    - system:authenticated
+    uid: eac0d7ab-af84-4c3f-a5fd-71845ff9e8c9
+    username: system:serviceaccount:dev:argo-admin

tests/test_data/requests/pass/arguments-artifacts.yaml

@@ -0,0 +1,60 @@
+apiVersion: admission.k8s.io/v1
+kind: AdmissionReview
+request:
+  dryRun: false
+  kind:
+    group: argoproj.io
+    kind: Workflow
+    version: v1alpha1
+  name: hello-world
+  namespace: dev
+  object:
+    apiVersion: argoproj.io/v1alpha1
+    kind: Workflow
+    metadata:
+      name: arguments-artifacts-
+    spec:
+      arguments:
+        artifacts:
+        - http:
+            url: https://storage.googleapis.com/kubernetes-release/release/v1.8.0/bin/linux/amd64/kubectl
+          name: kubectl
+      entrypoint: kubectl-input-artifact
+      templates:
+      - container:
+          args:
+          - kubectl version
+          command:
+          - sh
+          - -c
+          image: debian:9.4
+        inputs:
+          artifacts:
+          - mode: 493
+            name: kubectl
+            path: /usr/local/bin/kubectl
+        name: kubectl-input-artifact
+  operation: CREATE
+  options:
+    apiVersion: meta.k8s.io/v1
+    kind: CreateOptions
+  requestKind:
+    group: argoproj.io
+    kind: Workflow
+    version: v1alpha1
+  requestResource:
+    group: argoproj.io
+    resource: workflows
+    version: v1alpha1
+  resource:
+    group: argoproj.io
+    resource: workflows
+    version: v1alpha1
+  uid: c1bba5c6-2189-41ff-9487-be504c04487b
+  userInfo:
+    groups:
+    - system:serviceaccounts
+    - system:serviceaccounts:dev
+    - system:authenticated
+    uid: eac0d7ab-af84-4c3f-a5fd-71845ff9e8c9
+    username: system:serviceaccount:dev:argo-admin