KFAM is unprotected and any user can manage all profiles / namespaces of other users.

[juliusvonkohout]

/kind bug

Kubeflow version: 1.4
KFAM is completely unprotected.

Any user can just start a Jupyterlab and add itself to other namespaces , delete profiles / namespaces and create new ones according to https://github.com/kubeflow/kubeflow/tree/master/components/access-management#apis.
Some examples:
curl -X POST http://profiles-kfam.kubeflow.svc.cluster.local:8081/kfam/v1/profiles -H 'Content-Type: applica-tion/json' -d '{"metadata":{"name":"dev-exploit"},"spec":{"owner":{"kind":"User","name":"dev-1@kubeflow.org"}}}'
curl/wget http://profiles-kfam.kubeflow.svc.cluster.local:8081/kfam/v1/bindings

With the following change i get the desired 403 errors from inside and outside of the mesh (metacontroller, jupyterlab) Connecting to profiles-kfam.kubeflow.svc.cluster.local:8081 (172.30.217.39:8081) wget: server returned error: HTTP/1.1 403 Forbidden

apiVersion: security.istio.io/v1beta1
kind: AuthorizationPolicy
metadata:
  name: kfam
  namespace: kubeflow
  labels:
    kustomize.component: profiles
spec:
  action: ALLOW
  rules:
  - from:
    - source:
        principals: [cluster.local/ns/kubeflow/sa/centraldashboard]
    to:
    - operation:
        methods: ["GET"]
    - operation:
        methods: ["POST"]
    - operation:
        methods: ["DELETE"]
  selector:
    matchLabels:
      kustomize.component: profiles

and we need to set sidecar.istio.io/inject: 'true' for profiles-deployment and central-dashboard via overlys:

kind: Deployment
apiVersion: apps/v1
metadata:
  name: centraldashboard
  namespace: kubeflow
spec:
  template:
    metadata:
      annotations:
        sidecar.istio.io/inject: 'true'
---
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    kustomize.component: profiles
  name: profiles-deployment
  namespace: kubeflow
spec:
  replicas: 1
  selector:
    matchLabels:
      kustomize.component: profiles
  template:
    metadata:
      annotations:
        sidecar.istio.io/inject: "true"

I can create pull requests for all problems @thesuperzapper @Bobgy

It is also sensible to add a networpolicy that in general protects the kubeflow namespace. In my instance i even protect the user namespaces with networkpolices.

kind: NetworkPolicy
apiVersion: networking.k8s.io/v1
metadata:
  name: kubeflow
  namespace: kubeflow
spec:
  podSelector:
    matchExpressions:
      - key: app
        operator: NotIn
        values:
          - minio        # artifact storage
          - ml-pipeline  # pipeline api server
          - poddefaults  # mutating webhook 
          - cache-server # mutating webhook
      - key: component
        operator: NotIn
        values:
          - metadata-grpc-server # metadata server
      - key: katib.kubeflow.org/component
        operator: NotIn
        values:
          - controller # katib mutating webhook to add metrics logger
          - db-manager # the metrics loggers write directly to this database
      - key: control-plane
        operator: NotIn
        values:
          - kfserving-controller-manager # mutating webhook
          - seldon-controller-manager # validating webhook
  ingress:
    - from:
        - namespaceSelector: # istio-system is essential
            matchExpressions:
              - key: kubernetes.io/metadata.name
                operator: In
                values:
                  - istio-system
        - podSelector: {} # allow all pods from the same namespace
  policyTypes:
    - Ingress

kubernetes.io/metadata.name cannot be changed even by cluster-admins. So it is a safe identifier for the istio-system namespace

Another approach would be to use multiple policies:

1. deny all ingress except from istio-system and kubeflow
2. allow profile namespaces (app.kubernetes.io/part-of=kubeflow-profile) to ingress app=minio,ml-pipeline,poddefaults,cache-server and component=metadata-grpc-server

We need to trust the other kubeflow components anyway, so it does not make sense to block profiles-deployment (KFAM) separately.
Anyone controlling its own namespace can add the label app.kubernetes.io/part-of=kubeflow-profile so the two separate rules do not improve security.

I also recommend a profile namespace networkpolicy to the pipelines-profile-controller sync.py

kind: NetworkPolicy
apiVersion: networking.k8s.io/v1
metadata:
  name: allow-non-profile-namespaces
spec:
  podSelector: {}
  ingress:
    - from:
        - namespaceSelector:
            matchExpressions:
              - key: kubernetes.io/metadata.name
                operator: In
                values:
                  - kubeflow
                  - istio-system # needed for jupyterlabs
                  - knative-serving # mutating webhooks
                  - knative-eventing # mutating webhook
        - podSelector: {}
  policyTypes:
    - Ingress

[jbottum]

/area jupyter
/priority p1
@kubeflow/wg-notebooks-leads

[thesuperzapper]

@juliusvonkohout thanks for raising this, but please note that this issue is actually related to how distributions are deploying Kubeflow (and configuring Istio).

The primary reason that most distros have this issue, is because the example in kubeflow/manifests effectively disables all of Istio's security systems.

---

In my mind, the best way to secure Kubeflow's APIs / UIs is to use Istio rather than NetworkPolicies.

Primarily what should happen:

1. All APIs & UIs should only accept traffic from the istio-ingressgateway Pods:
   • (Or whatever your cluster called the inbound traffic gateway)
   • (Can be done with AuthorizationPolicy)
2. The KUBEFLOW_USERID should not be blindly trusted, but verified against the JWT from the auth provider
   • (Must be done with some really complex EnvoyFilter and corresponding AuthorizationPolicy configs)

[kimwnasptd]

@juliusvonkohout thanks for raising this issue. Indeed, protecting KFAM with access only from the CentralDashboard is something we should do to avoid this issue. I agree with @thesuperzapper that we should rely on Istio's AuthorizationPolicies for this, and not have a NetworkPolicy for now.

@juliusvonkohout could you provide a PR with your mentioned AuthorizationPolicy and changes to enable the sidecars?

And also as a more general note, I really want to completely remove KFAM. It adds an abstraction on top of K8s RBAC that we should aim to avoid, and simplify. There are some techincal questions there on how the CentralDashboard can know which users have access to which namespace, but let's tackle this in an issue of its own.

[juliusvonkohout]

@kimwnasptd yes, as soon as I am back from vacation. Network policies are an additional protection layer on the kubernetes level if istio fails. They protect almost all services, not just KFAM. Are you sure that you do not want them?

[kimwnasptd]

Awesome, thanks @juliusvonkohout!

Are you sure that you do not want them?

I don't dismiss the idea, but I want to look into NetworkPolicies a little bit more first. From your description above I understand we can achieve the same thing with AuthorizationPolicies, but now only with K8s constructs.

But I don't want to block us from securing KFAM, that's why I want to propose to merge the AuthorizationPolicy as a first step and then get back and evaluate how to use NetworkPolicies as well.

This could also be a very useful piece of documentation to add in the website as well, on how to secure Kubeflow's services.

[juliusvonkohout]

@juliusvonkohout thanks for raising this, but please note that this issue is actually related to how distributions are deploying Kubeflow (and configuring Istio).

The primary reason that most distros have this issue, is because the example in kubeflow/manifests effectively disables all of Istio's security systems.

In my mind, the best way to secure Kubeflow's APIs / UIs is to use Istio rather than NetworkPolicies.

Primarily what should happen:

1. All APIs & UIs should only accept traffic from the `istio-ingressgateway` Pods:
   
   * (Or whatever your cluster called the inbound traffic gateway)
   * (Can be done with `AuthorizationPolicy`)

2. The `KUBEFLOW_USERID` should not be blindly trusted, but verified against the JWT from the auth provider
   
   * (Must be done with some really complex `EnvoyFilter` and corresponding `AuthorizationPolicy` configs)

So by default istio in Kubeflow is a complete security wreck? We should fix this as soon as possible. Do you have some guideliens on how to achieve "The KUBEFLOW_USERID should not be blindly trusted, but verified against the JWT from the auth provider"

[juliusvonkohout]

Awesome, thanks @juliusvonkohout!

Are you sure that you do not want them?

I don't dismiss the idea, but I want to look into NetworkPolicies a little bit more first. From your description above I understand we can achieve the same thing with AuthorizationPolicies, but now only with K8s constructs.

But I don't want to block us from securing KFAM, that's why I want to propose to merge the AuthorizationPolicy as a first step and then get back and evaluate how to use NetworkPolicies as well.

This could also be a very useful piece of documentation to add in the website as well, on how to secure Kubeflow's services.

Alright,

so i will create two separate pull requests. One for Authorization policies and another for the additional security layer of networkpolicies. Networkpolices can secure all services, also those that do not use istio and they can be used to secure user namespaces too.

But the problem is that there are no manifests in kubeflow/kubeflow. So am i right that i have to change it in kubeflow/manifests 1. https://github.com/kubeflow/manifests/blob/master/apps/profiles/upstream/overlays/kubeflow/patches/kfam.yaml 2. https://github.com/kubeflow/manifests/tree/master/apps/centraldashboard/upstream ?

[juliusvonkohout]

@kimwnasptd @thesuperzapper i need the information requested above to create the pull requests for you.

[davidspek]

Funny how a PR to fix this issue was 4 months before this issue was raised and never looked at by @kubeflow/wg-notebooks-leads

[juliusvonkohout]

Funny how a PR to fix this issue was 4 months before this issue was raised and never looked at by @kubeflow/wg-notebooks-leads

That is totally crazy. Now i understand why you are forking it.

[juliusvonkohout]

@thesuperzapper @jbottum @kimwnasptd i strongly propose that issue and @davidspek s pull request for kubeflow 1.5 #6077
Kubeflow 1.5 release plan kubeflow/manifests#2098

[thesuperzapper]

Just wanted to say, I am fairly sure that PR #6077 will not fully address the issue, see #6077 (comment)

[juliusvonkohout]

Just wanted to say, I am fairly sure that PR #6077 will not fully address the issue, see #6077 (comment)

Multiple users report otherwise in the pull request. my networkpolicies are just an additional security layer.

[kimwnasptd]

The AuthorizationPolicies added by @davidspek's PR should add a solid first wall of protection.

@juliusvonkohout could you also open an issue in the kubeflow/manifests repo to track the addition of networkpolicies as an additional security layer. It should also just make it in time for the 1.5 release.

[juliusvonkohout]

The AuthorizationPolicies added by @davidspek's PR should add a solid first wall of protection.

@juliusvonkohout could you also open an issue in the kubeflow/manifests repo to track the addition of networkpolicies as an additional security layer. It should also just make it in time for the 1.5 release.

Yes I can. I have reworked them into multiple smaller ones. My question is where theyshould be added. Maybe just under apps/common/networkpolicies?

[juliusvonkohout]

if there is such a responsiveness problem, why do you not add me or @davidspek or soemone else with enough time as maintainer? You definitely need more people to handle the pull request stuff

[kimwnasptd]

I was waiting on an issue in the manifests repo to discuss the more global network policies, since this issue should be closed now that #6077 is merged.

Yes I can. I have reworked them into multiple smaller ones. My question is where theyshould be added. Maybe just under apps/common/networkpolicies?

I'm having mixed feelings regarding the network policies. My reasoning is that we are limiting the networking, at the K8s level, in order to enforce authorization for which Istio should be responsible.

I understand the benefit of having an extra security wall, but at the same time I don't like that we are augmenting two different places to achieve what should be implemented in the Istio authz level via Authorization Policies.

So because of the above, I'm leaning towards including the network policies under contrib/networkpolicies, as an extra option for users like the dex-auth, basic-auth and other optional extensions/apps.
https://github.com/kubeflow/manifests/tree/master/contrib

[juliusvonkohout]

kubeflow/manifests#2121 has been created with the networkpolicies

[kimwnasptd]

Thank you @juliusvonkohout! I'm moving on and closing this issue since it has serve it's technical purpose and we've ensured KFAM is protected with AuthorizationPolicies.

/close

[google-oss-prow]

@kimwnasptd: Closing this issue.

In response to this:

Thank you @juliusvonkohout! I'm moving on and closing this issue since it has serve it's technical purpose and we've ensured KFAM is protected with AuthorizationPolicies.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.