Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oauth2-proxy with istio mesh config and m2m bearer tokens #2544

Merged
merged 57 commits into from
Feb 28, 2024

Conversation

kromanow94
Copy link
Contributor

@kromanow94 kromanow94 commented Oct 3, 2023

Which issue is resolved by this Pull Request:
Resolves #2516, #2517

Description of your changes:
Changes are based on discussion and description available here:

TL;DR:

  • Drop EnvoyFilter in oauth2-proxy kustomization
  • Use Istio envoyExtAuthzHttp instead of EnvoyFilter
  • Enable m2m bearer tokens
  • Use kustomize components for configuration tailoring
  • Enable scenario with in-cluster Kubernetes OIDC served behind self-signed certificates
  • Explain new auth routine
  • Use oauth2-proxy in logout routine

Testing setup:
In common/oidc-client/oauth2-proxy/README.md are instructions on how to enable usage of oauth2-proxy. This covers the example scenario with Kubeflow deployed on kind, which enables configuration for in-cluster OIDC behind self-signed certificates.

To verify if oauth2-proxy is working for standard user login and m2m tokens:

  • try to login to Kubeflow with example user/password. After that, you should be able to see:
    • logged in user is user@example.com
    • there is a cookie named oauth2_proxy_kubeflow in your browser
  • create a Pod with curl image and try calling istio-ingressgateway with Authorization Bearer Token from Service Account:
    [user@example ~]$ kubectl run -ti --rm --image curlimages/curl curl --command -- sh
    ~ $ curl istio-ingressgateway.istio-system/api/workgroup/exists -H "Authorization: Bearer $(cat /run/secrets/kubernetes.io/serviceaccount/token)"
    {"hasAuth":true,"user":"system:serviceaccount:default:default","hasWorkgroup":false,"registrationFlowAllowed":false}

Comments:
@axel7083 , I'd appreciate your feedback and it would be great if you could test this setup

@juliusvonkohout , I know the file structure looks a little bit different but I thought it makes a little bit more sense. That said, I'm open for discussion and we can change the file structure.

From implementation perspective everything is in place. The only missing parts are adding some more README, probably doing some clean up and addressing feedback.

I'd appreciate review.


Discovered missing points

  • enable oauth2-proxy as default
  • enable m2m by default
  • enable self-signed certs by default
  • write e2e tests for m2m - Add e2e github actions #2417
    • list pipeline runs
    • list experiments in a namespace
    • starting a pipeline - good to have if time allows, might require considerate amount of work
    • test kserve (create new e2e github workflow based on the current kserve test)
      • use only the scenario with standard istio installation (not cni)
  • HA for oauth2-proxy with multiple replicas
  • describe oauth2-proxy upgrade instructions

@google-cla
Copy link

google-cla bot commented Oct 3, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@kromanow94 kromanow94 changed the title oauth2-proxy with istio mesh config and m2m bearer tokens - #2517, #2516 oauth2-proxy with istio mesh config and m2m bearer tokens Oct 3, 2023
@juliusvonkohout
Copy link
Member

@kromanow, i can take a look at this next week. Please sign the CLA as well.

@google-oss-prow google-oss-prow bot added size/XXL and removed size/XL labels Oct 4, 2023
@kromanow94
Copy link
Contributor Author

@juliusvonkohout I signed the CLA.

@juliusvonkohout
Copy link
Member

/lgtm

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Oct 9, 2023

I think wemight be missing tests. For example https://github.com/kubeflow/manifests/blob/master/tests/gh-actions/install_knative-cni.sh and https://github.com/kubeflow/manifests/blob/master/tests/gh-actions/install_istio-cni.sh could be extended with oauth 2.

Maybe - ../../../../oidc-client/oauth2-proxy/components/istio-external-auth is already enough, but i have to check again.

@kromanow94
Copy link
Contributor Author

@juliusvonkohout thanks for the review!

About the tests - good point. I'm not sure about the "could be extended with oauth2" part. You mean not to create new tests for oauth2-proxy but just extend the ones for knative and istio-cni?

I think until there is support for oidc-authservice, I'd suggest having those as a separate tests. But, if you think extending the current ones is good enough - that's fine with me.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Oct 9, 2023

@juliusvonkohout thanks for the review!

About the tests - good point. I'm not sure about the "could be extended with oauth2" part. You mean not to create new tests for oauth2-proxy but just extend the ones for knative and istio-cni?

I think until there is support for oidc-authservice, I'd suggest having those as a separate tests. But, if you think extending the current ones is good enough - that's fine with me.

For the time being extending them might be enough. @kimwnasptd what do you think?
I think we should at least test the human session and the m2m authentication in the github workflow, because this is really crucial functionality.

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
use latest in deployment spec

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
…unt token

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
@kromanow94
Copy link
Contributor Author

From my perspective all that we've agreed on having in this PR is ready.

@juliusvonkohout
Copy link
Member

Amazing improvement
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout, kromanow94

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:

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

@juliusvonkohout
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Feb 28, 2024
@google-oss-prow google-oss-prow bot merged commit 506789a into kubeflow:master Feb 28, 2024
10 checks passed
@kromanow94
Copy link
Contributor Author

Hurray!

Many thanks for all the support! ❤️🍀

@deepakdeore2004
Copy link

which kubeflow version will have this change included?

@kromanow94
Copy link
Contributor Author

@deepakdeore2004 officially it will be included in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants