proposal: Uniform way for handling JWTs

[kimwnasptd]

Continuing from the discussion in #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]

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

[kromanow94]

Hey, this is the continuation of my thoughts from #2747 (comment).

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]

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

[kromanow94]

May I suggest to drop the requirement to allow access only from Istio IngressGateway? AFAIR, this was a security concern from the past so the requests are always going through the EnvoyFilter. We don't use EnvoyFilter anymore and limiting access to requests only going through Istio IngressGateway doesn't bring any more security at this point and just makes the setup more complicated in my opinion.

If we only allow traffic to backends that have a trusted principal in requests (for the principal to be trusted and have it available to use in AuthorizationPolicy, it must be configured through the RequestAuthentication), there is no risk of trusting JWT/Issuer from unknown source.

[juliusvonkohout]

" AuthorizationPolicy objects of components, for allowing access from Istio IngressGateway, must be adjusted to enforce the presence of a JWT"

[kromanow94]

Or maybe

"AuthorizationPolicy objects of KF Components have to be adjusted to allow access only when a trusted JWT is present in the Authorization: Bearer <Token> header. We no longer have to enforce access only through istio-ingressgateway."

[kromanow94]

So this rule allows accessing the specific endpoint only if the request went through the istio-ingressgateway and contains a trusted principal.

Can we just leave it like in the suggestion below? It would lower the coupling with this specific istio gateway.

Arguments in favor:

• the name istio-ingressgateway-service-account couples the kubeflow deployment with istio gateway deployed from operator/istioctl, because istio gateway installed with Helm uses different SA (it should be also possible to use different gateway name and/or SA name if required)
• it would allow OOB secure access from more gateways/istio LBs to access the service without any additional configuration
• if some platform uses yet another LB (like ALB in AWS; istio still have to be deployed in-cluster), it's also possible to define a rule where the traffic goes directly to the endpoint (here jupyter-web-app) and omit the istio-ingressgateway
• I don't see any security risks in dropping this requirement of access only through the istio-ingressgateway because the traffic must be authorized first to access either way

This is only for the reason of flexibility and ease of adoption.

Suggested change

	- from:
	- source:
	principals:
	- cluster.local/ns/istio-system/sa/istio-ingressgateway-service-account
	requestPrincipals: # new! Require JWT
	- '*'
	- from:
	- source:
	requestPrincipals:
	- '*'

[kimwnasptd]

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

[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?

[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.

[juliusvonkohout]

will it remove the bearer token header with the JWT before forwarding the request to the backend or will the backend recieve the userid header and the bearer token?

"The recommended way is to use requestPrincipals: [""], as the Istio docs suggest, to accept only requests that have a JWT. The recommended way is to use requestPrincipals: [""], as the Istio docs suggest, to accept only requests that have a ?valid? JWT." later on suggests that the JWT has to be forwarded as well.

[kimwnasptd]

No no the JWT will stay in the Authorization header, and the mesh will add the headers as an extra

[kromanow94]

@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]

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]

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

[juliusvonkohout]

@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]

/approve

[google-oss-prow]

[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 [juliusvonkohout]

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