Prevent Kubeflow Pipelines from pulling arbitrary images

[ca-scribner]

Currently this code run on our notebook servers successfully pulls an arbitrary image:

BASE_IMAGE = "scribby182/demo-kfp-pipeline-authoring:latest"

def concat_string(a, b) -> str:
    return f"({a} | {b})"

concat_string = func_to_container_op(concat_string,
                                               base_image=BASE_IMAGE
                                               )

@dsl.pipeline(
    name="Pipeline that pulls arbitrary image",
)
def pipeline(str1, str2, str3):
    concat_result_1 = concat_string_component(str1, str2)
    concat_result_2 = concat_string_component(concat_result_1.output, str3)

kfp.Client().create_run_from_pipeline_func(
    pipeline,
    arguments={'str1': 'String 1', 'str2': 'String 2', 'str3': 'String 3'},
    experiment_name=experiment_name
)

Need to prevent this (ideally enforcing the same or similar whitelist as done with kubeflow in general?). @brendangadd, @blairdrummond knew the details of what we've done there better.

If possible, including tensorflow/tensorflow:1.13.2-py3 in the whitelist would be helpful as func_to_container_op() defaults to use that image if base_image is not specified

[brendangadd]

@zachomedia: I remember reading your OPA whitelist for images. It would prevent this type of image pull from happening, and I thought it had a cluster-wide scope. Any insight into how these images can sneak into pipelines?

[zachomedia]

@brendangadd There are two things going on about this:

1. We have an issue where Azure's Kubernetes Service where is randomly removing the Gatekeeper custom resource definitions - we have open support ticket with them about this.
2. The kubeflow namespace was accidentally whitelisted from enforcement, I'm working on clearing that from the whitelist and then the image restriction will be enforced

[blairdrummond]

Is this still open?

[ca-scribner]

Yes. I just tried pulling from an arbitrary container and it still does it successfully.

[blairdrummond]

CC @justbert @zachomedia ?

[justbert]

@ca-scribner You made a pod with an arbitrary container?

The policy is still there but the CRDs are gone again. @zachomedia is waiting on a ticket from Microsoft.

[justbert]

@ca-scribner Fixed! I redeployed the ConstraintTemplates. Hopefully, Microsoft gets back to us with why they keep on killing them!

[justbert]

Assigned to @zachomedia so that we can coordinate the lifecycle of this ticket with the Azure Support ticket.

[ca-scribner]

@justbert from your side does it still look fixed? I just tried again now and can still run arbitrary containers.

I also made sure this was a new container just in case it was using a cached version. Definitely pulled a new container from my dockerhub and ran fine.

[justbert]

@ca-scribner Was the image scribby182/demo-kfp-pipeline-authoring:v2?

The infrastructure to deny this IS still in place.

[ca-scribner]

Yes that’s the container. I could also pull a generic python:buster (or whatever it’s called) successfully, but wondered if that might be on a whitelist.

…

On Wed, May 27, 2020 at 12:04 Justin Bertrand ***@***.***> wrote: @ca-scribner <https://github.com/ca-scribner> Was the image scribby182/demo-kfp-pipeline-authoring:v2? The infrastructure to deny this IS still in place. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALPFPIZXU6XOBZPI5O4PPI3RTU2Y3ANCNFSM4M73D4FA> .

[justbert]

Ok, so it SAYS that it denied the image pull...

Whitelist is here

[justbert]

Looks like a control-plane label was added to the kubeflow namespace again. Removing it worked got the GateKeeper policy to be enforced once again.

Checking to see where it's being repopulated from.

Thanks @ca-scribner for testing!

[justbert]

From @zachomedia
Issue is with kfctl continually readding the control-plane label to the namespace. Others are having similar issues to ours.

We may need to open up a discussion with the Kubeflow team to see if there could be a way to rectify this conflict.

[sylus]

@justbert we going to use a manual patch to fix this? Then can get this one closed ^_^

[justbert]

We could! Could we add a kustomize at the end of the CI to remove it?