Profile controller and KFAM allow unauthenticated in-cluster traffic

[johnhoman]

This is similar to this kerve issue kserve/kserve#2197 (comment). Users can be impersonated without authentication by attaching the header kubeflow-userid: with a valid profile contributor name to a request going to any namespace within the mesh.

(base) jovyan@tensorflow-0:~$ curl -sS -D - http://jupyter-scipy.kubeflow-user-example-com.svc.cluster.local/notebook/kubeflow-user-example-com/jupyter-scipy/api/sessions -o /dev/null
HTTP/1.1 403 Forbidden
content-length: 19
content-type: text/plain
date: Tue, 07 Mar 2023 04:02:17 GMT
server: envoy
x-envoy-upstream-service-time: 4

(base) jovyan@tensorflow-0:~$ curl -H "kubeflow-userid: user@example.com" -sS -D - http://jupyter-scipy.kubeflow-user-example-com.svc.cluster.local/notebook/kubeflow-user-example-com/jupyter-scipy/api/sessions -o /dev/null
HTTP/1.1 200 OK
server: envoy
content-type: application/json
date: Tue, 07 Mar 2023 04:03:22 GMT
x-content-type-options: nosniff
content-security-policy: frame-ancestors 'self'; report-uri /notebook/kubeflow-user-example-com/jupyter-scipy/api/security/csp-report; default-src 'none'
access-control-allow-origin: *
etag: "97d170e1550eee4afc0af065b78cda302a97674c"
content-length: 2
x-envoy-upstream-service-time: 106

Adding this change resolves that

kubectl patch deployment profiles-deployment -n kubeflow --type=json -p='[{"op": "replace", "path": "/spec/template/spec/containers/1/image", "value": "profile-controller:v1.7.0-rc.1-dirty"}]'
deployment.apps/profiles-deployment patched

➜  ~ kubectl get authorizationpolicies/ns-owner-access-istio -n kubeflow-user-example-com -o json | jq '.spec.rules[0]'

{
  "from": [
    {
      "source": {
        "principals": [
          "cluster.local/ns/istio-system/sa/istio-ingressgateway-service-account"
        ]
      }
    }
  ],
  "when": [
    {
      "key": "request.headers[kubeflow-userid]",
      "values": [
        "user@example.com"
      ]
    }
  ]
}

(base) jovyan@tensorflow-0:~$ curl -sS -D - http://jupyter-scipy.kubeflow-user-example-com.svc.cluster.local/notebook/kubeflow-user-example-com/jupyter-scipy/api/sessions -o /dev/null
HTTP/1.1 403 Forbidden
content-length: 19
content-type: text/plain
date: Tue, 07 Mar 2023 04:26:19 GMT
server: envoy
x-envoy-upstream-service-time: 32

(base) jovyan@tensorflow-0:~$ curl -H "kubeflow-userid: user@example.com" -sS -D - http://jupyter-scipy.kubeflow-user-example-com.svc.cluster.local/notebook/kubeflow-user-example-com/jupyter-scipy/api/sessions -o /dev/null
HTTP/1.1 403 Forbidden
content-length: 19
content-type: text/plain
date: Tue, 07 Mar 2023 04:26:22 GMT
server: envoy
x-envoy-upstream-service-time: 9

The authorization policy created by KFAM also has the same issue.

(base) jovyan@tensorflow-0:~$ curl -H "kubeflow-userid: jack@example.com" -sS -D - http://jupyter-scipy.kubeflow-user-example-com.svc.cluster.local/notebook/kubeflow-user-example-com/jupyter-scipy/api/sessions -o /dev/null | head -n 1
HTTP/1.1 200 OK

[juliusvonkohout]

are you using Kubeflow 1.7 ?
Did you really test accessing other namespaces? I think you need to add more documentation.

[TobiasGoerke]

Can confirm: spoofing the kubeflow-userid is easily done when there are no network policies in place.

I currently cannot test the PR but restricting the Authorization Policy to the istio SA looks promising.

Thanks!

[johnhoman]

are you using Kubeflow 1.7 ? Did you really test accessing other namespaces? I think you need to add more documentation.

Sorry, I thought the examples were straight forward enough, but now I'm realizing that is not the case.

When KFAM adds contributors to a profile, it creates an AuthorizationPolicy. That authorization policy has a single rule that looks something like

spec:
  rules:
  - when:
    - key: request.headers[kubeflow-userid]
       values:
       - user@example.com

For traffic coming through the ingress gateway, that's fine because the kubeflow-userid header is set by the auth chain. Internal traffic doesn't go through the ingress gateway, so any request with the kubeflow-userid header set to a namespace contributor will be allowed through.

This is a curl request from a notebook to a notebook in another namespace.

(base) jovyan@tensorflow-0:~$ curl -sS -D - http://jupyter-scipy.kubeflow-user-example-com.svc.cluster.local/notebook/kubeflow-user-example-com/jupyter-scipy/api/sessions -o /dev/null
HTTP/1.1 403 Forbidden
content-length: 19
content-type: text/plain
date: Tue, 07 Mar 2023 04:02:17 GMT
server: envoy
x-envoy-upstream-service-time: 4

# adding header `kubeflow-user=user@example.com`
(base) jovyan@tensorflow-0:~$ curl -H "kubeflow-userid: user@example.com" -sS -D - http://jupyter-scipy.kubeflow-user-example-com.svc.cluster.local/notebook/kubeflow-user-example-com/jupyter-scipy/api/sessions -o /dev/null
HTTP/1.1 200 OK
server: envoy
content-type: application/json
date: Tue, 07 Mar 2023 04:03:22 GMT
x-content-type-options: nosniff
content-security-policy: frame-ancestors 'self'; report-uri /notebook/kubeflow-user-example-com/jupyter-scipy/api/security/csp-report; default-src 'none'
access-control-allow-origin: *
etag: "97d170e1550eee4afc0af065b78cda302a97674c"
content-length: 2
x-envoy-upstream-service-time: 106

If the rules that allow traffic based on kubeflow-userid require that it comes from the ingress gateway, then the problem goes away.

spec:
  rules:
  - when:
    - key: request.headers[kubeflow-userid]
       values:
       - user@example.com
    from:
    - source:
        principals: ["cluster.local/ns/istio-system/sa/istio-ingressgateway-service-account"]

This is how it works on master today and is only a problem for in cluster traffic.

[juliusvonkohout]

Yes, that is the same way it is done here https://github.com/kubeflow/manifests/blob/b1e29829b0d13d5493c8a3f5f720f87547a8abe2/apps/pipeline/upstream/base/installs/multi-user/istio-authorization-config.yaml#L28

Only the istio-ingressgateway and some other serviceaccounts in the kubeflow namespace have verified headers. You still have to guess or have seen the notebook and contributor name but @kimwnasptd @akgraner this is security relevant.

@johnhoman If you are interested in fixing security issues are you willing to help with the following two security issues?

[juliusvonkohout]

@johnhoman can you provide a public image for testing?

[juliusvonkohout]

/lgtm

[johnhoman]

@johnhoman can you provide a public image for testing?

Sorry, must have missed this. Here are public images rebuilt from this commit

jackhoman/kubeflow-profile-controller:e205339e
jackhoman/kubeflow-kfam:e205339e

[kimwnasptd]

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• components/access-management/OWNERS [kimwnasptd]
• components/profile-controller/OWNERS [kimwnasptd]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[thesuperzapper]

@kimwnasptd I see that we have hard-coded the istio gateway's namespace as istio-sytem and its Pod ServiceAccount as istio-ingressgateway-service-account, this will make it impossible to use in many companies which don't use these.

Similar to how we allow configuring this with the ISTIO_GATEWAY env-var in the notebook controller.

[kimwnasptd]

@thesuperzapper indeed, let's have the same configuration everywhere for allowing different Istio handlings

[thesuperzapper]

@kimwnasptd we either need to revert this ASAP, because it will break Kubeflow as it currently stands.

Specifically, we need to:

1. Allow users to specify the correct ServiceAccount for their Istio Gateway:
   • These configs must be added to both controllers:
     • ISTIO_GATEWAY_NAMESPACE
     • ISTIO_GATEWAY_SA
   • These configs are used to set the source.principals of the rule:
     • cluster.local/ns/{ISTIO_GATEWAY_NAMESPACE}/sa/{ISTIO_GATEWAY_SA}
2. Add a source.principals for the Kubeflow Pipelines UI:
   • (if we don't add this, KFP won't be able to access ml-pipeline-ui-artifact pods in each profile namespace)
   • cluster.local/ns/kubeflow/sa/ml-pipeline-ui

[thesuperzapper]

Also, another option is to use a single ISTIO_GATEWAY_PRINCIPAL config, rather than the two (ISTIO_GATEWAY_NAMESPACE and ISTIO_GATEWAY_SA)

Which would have a value like: cluster.local/ns/{ISTIO_GATEWAY_NAMESPACE}/sa/{ISTIO_GATEWAY_SA} (and would enable support for clusters that don't use cluster.local).

[kimwnasptd]

Yep, that would be the most dynamic of all. I'll send a PR