proposal: Uniform way for handling JWTs

kimwnasptd commented 3 months ago

Continuing from the discussion in https://github.com/kubeflow/manifests/issues/2747

After the oauth2-proxy PRs are merged we have made some changes on how JWTs are used from within the cluster (RequestAuthentication) and how headers are added from these tokens.

This lead to some issues in KFP, for which to resolve we'll need to have an agreed upon story on how to be working in the future with JWTs.

After agreeing on the above, this can also enable us to push the effort for supporting groups since we'll have an agreed upon way of working with identity information.

/cc @juliusvonkohout @kromanow94 @thesuperzapper @axel7083 cc @rimolive @DnPlas

juliusvonkohout commented 3 months ago

The general approach looks good, although i did not check the technical details yet.

kromanow94 commented 3 months ago

Hey, this is the continuation of my thoughts from https://github.com/kubeflow/manifests/issues/2747#issuecomment-2168641503.

In general, I like this approach. In the comment linked above, I mentioned that I'd personally vote for not removing implementation of JWTs in Kubeflow components but after some thoughts over the weekend, I don't emphasize that so much right now.

While I personally would love Kubeflow to be natively supporting JWTs, I understand that we have to take into account the balance between functionality and maintenance/development and offload whatever possible. With this in mind, if you decide to drop JWT implementation from kubeflow components and just rely on the kubeflow-userid and kubeflow-groups headers and enforce those through Istio, I'm ok with that and you have my support.

If possible, I'd suggest standardizing the auth headers to X-Auth-Request-Groups and X-Auth-Request-Email.

I also mentioned that I'd prefer to specify AuthorizationPolicy objects with custom action and oauth2-proxy as the provider for every component that's exposed to users (api-server, jupyter-web-app, central-dashboard, etc...), not just for the istio-ingressgateway, to bring security closer to the components.

While this would move the security layer closer to the components and allow increased flexibility in implementation of how the requests are routed and authorized to reach an endpoint, this is also not something I'm putting strong emphasis on. If you like that approach, I'd be happy to help with hands-on, but this is not something we have to have.
So, to put it short: from in-cluster perspective, using a pair of AuthorizationPolicy and RequestAuthentication would be enough and the requests wouldn't have to go trough ouath2-proxy.

And then to continue some other thoughts:

Since this proposal is suggesting removal of the JWT implementation from Kubeflow components, it also means that we don't need to check for pipelines.kubeflow.org audience and we don't have to add PodDefaults that adds the projected token.
- I suggest to drop the pipelines.kubeflow.org audience at some form either way so I like that.
I suggest dropping the istio-ingressgateway-service-account from the AuthorizationPolicices principal and just ensure that any principal is available through rules.from.source.requestPrincipals. This would also ease integration with externally installed Istio since the service account name for istio-ingressgateway depends on if you install through Helm Chart or the istio operator/istioctl.

kimwnasptd commented 3 months ago

Thanks for the review @kromanow94!

To avoid answering multiple parts in the same message, ending in big messages, could you provide also the above points as concrete comments on the proposal so we keep the discussions focused/separate?

https://github.com/kubeflow/manifests/pull/2748/files

kimwnasptd commented 3 months ago

Thank you so much for the review comments @kromanow94! I'll take a look on them today/tomorrow

kimwnasptd commented 3 months ago

@kromanow94 I've addressed the review comments, and only left the ones that propose the removal of the IngressGateway declaration in the AuthorizationPolicies.

I would like to avoid making this change in this PR. Mainly because this is a bigger discussion, to untangle the Istio IngressGateway, from the Kubeflow components. There are other places that are affected by this, like the AuthorizationPolicies created by Profiles (for allowing traffic for Notebooks and/or ISVCs).

So I'd prefer that we tackle that problem of its own, in a proposal that captures all the aspects rather than doing some changes here that are not fully spec-ed out for the overall effort.

WDYT?

juliusvonkohout commented 3 months ago

Given oauth2-proxy the information in https://www.kubeflow.org/docs/components/pipelines/user-guides/core-functions/connect-api/#full-kubeflow-subfrom-inside-clustersub is is really misleading.

kromanow94 commented 3 months ago

@kimwnasptd

@kromanow94 I've addressed the review comments, and only left the ones that propose the removal of the IngressGateway declaration in the AuthorizationPolicies.

I would like to avoid making this change in this PR. Mainly because this is a bigger discussion, to untangle the Istio IngressGateway, from the Kubeflow components. There are other places that are affected by this, like the AuthorizationPolicies created by Profiles (for allowing traffic for Notebooks and/or ISVCs).

So I'd prefer that we tackle that problem of its own, in a proposal that captures all the aspects rather than doing some changes here that are not fully spec-ed out for the overall effort.

WDYT?

Ok, this makes very much sense. Let's do it like that.

Also, I wasn't able to resolve the threads but I either left a comment or a thumbs up where I agree on the resolution.

@juliusvonkohout

Given oauth2-proxy the information in https://www.kubeflow.org/docs/components/pipelines/user-guides/core-functions/connect-api/#full-kubeflow-subfrom-inside-clustersub is is really misleading.

So I understand you suggest to update this docs with information on how to use the SA Token?

BTW, since we're moving around some stuff for authz, maybe you're interested in having a service that will manage programmatic login? This would be something that replaces the Full Kubeflow (from outside cluster) flow. We have this little component that works with a websocket, can redirect to the auth page, get the cookies or a token from login response and send back via websocket to the client. We can also have a call sometime next week so I can showcase how would that work.

juliusvonkohout commented 3 months ago

So I understand you suggest to update this docs with information on how to use the SA Token?

Yes :-) this session hack i snot what we should advertise.

kimwnasptd commented 3 months ago

@kromanow94 @juliusvonkohout I've done a pass and integrated your comments. Let me know if there's anything else

juliusvonkohout commented 3 months ago

@kromanow94 @juliusvonkohout I've done a pass and integrated your comments. Let me know if there's anything else

For me it is anyway a

/lgtm

for a long time (despite the "nits" :-D)

We can always reiterate and reiterating is easier if it is merged first. In the end it is a proposal that can change.

juliusvonkohout commented 3 months ago

/approve

google-oss-prow[bot] commented 3 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout

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: - ~~[OWNERS](https://github.com/kubeflow/manifests/blob/master/OWNERS)~~ [juliusvonkohout] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

kubeflow / manifests

proposal: Uniform way for handling JWTs #2748