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

feat: upgrade oidc-authservice #2150

Merged

Conversation

ittus
Copy link
Contributor

@ittus ittus commented Feb 26, 2022

Which issue is resolved by this Pull Request:
Upgrade oidc-authservice to gcr.io/arrikto/kubeflow/oidc-authservice:e236439

  • Support OIDC ID Token authentication
  • Support ServiceAccount Token authentication

Description of your changes:

  • Upgrade oidc-authservice to new docker image
  • Add rbac to allow calling tokenreviews API

Additional information

@google-cla
Copy link

google-cla bot commented Feb 26, 2022

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

For more information, open the CLA check for this pull request.

@aws-kf-ci-bot
Copy link

Hi @ittus. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@ittus ittus force-pushed the feat/upgrade-oidc-authservice branch from aa561e6 to ed63abb Compare February 26, 2022 10:25
Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ok-to-test

@juliusvonkohout
Copy link
Member

@ittus could you elaborate a bit more on the changes?

"Support OIDC ID Token authentication" Does that mean we can use serviceaccounts not just for the pipelines API, but for all istio secured stuff, e.g. kfserving, seldon, Kserve etc?
"Support ServiceAccount Token authentication" How exactly does that work? Is it enough to have the default-editor serviceaccount in a Jupyterlab?

@ittus
Copy link
Contributor Author

ittus commented Mar 2, 2022

Hi @juliusvonkohout ,
The purpose is that we can use OIDC IdToken or serviceAccount token to authenticate in oidc-authservice. The upstream action can be calling KFServing API.

Currently, it's only available with session authentication

In the new version of oidc-authservice, it has 3 authentications: sessions, idtoken, and k8sauthentication.
The authenticate method will check one by one until it passed.

I checked with KFServing, ServiceAccount token and IdToken can be used to call serving API along with session

@juliusvonkohout
Copy link
Member

Can we use it then to authenticate from outside AND inside of the cluster with an extracted serviceaccounttoken from default-editor? I need something like this to trigger pipelines automatically without manual login.

@ittus
Copy link
Contributor Author

ittus commented Mar 2, 2022

Currently, I'm using it to invoke KFServing outside of cluster. Not test with pipeline yet.

@juliusvonkohout
Copy link
Member

Currently, I'm using it to invoke KFServing outside of cluster. Not test with pipeline yet.

@ittus Thank you very much, then this is a very important feature.

@kimwnasptd is this possible for 1.5 ?

@davidspek this feature might also be interesting for you.

@snmohan83
Copy link

@ittus Do you have an example implementation of how you are using either the ID token or service account to call KFserving API ?

@ittus
Copy link
Contributor Author

ittus commented Mar 10, 2022

@snmohan83 I implemented this in a private project.
Do you have a suggestion on how to create an example for this easily for you to check?

@snmohan83
Copy link

@snmohan83 I implemented this in a private project. Do you have a suggestion on how to create an example for this easily for you to check?

@ittus I would like to use API calls similar to the example used here: https://github.com/kserve/kserve/tree/master/docs/samples/istio-dex#run-a-prediction where a session_token can be obtained that can be used in the application.

@juliusvonkohout
Copy link
Member

@snmohan83 I implemented this in a private project. Do you have a suggestion on how to create an example for this easily for you to check?

@ittus I would like to use API calls similar to the example used here: https://github.com/kserve/kserve/tree/master/docs/samples/istio-dex#run-a-prediction where a session_token can be obtained that can be used in the application.

@ittus maybe you have a similar example to share. Are you also using it with other dex authentication providers like keycloak and gitlab?

@ittus ittus force-pushed the feat/upgrade-oidc-authservice branch from 8b20726 to 3c94d21 Compare May 11, 2022 12:16
@juliusvonkohout
Copy link
Member

@kimwnasptd Can we get this into Kubeflow 1.6?

@MatthiasCarnein
Copy link

MatthiasCarnein commented Jul 9, 2022

Updating the oidc-authservice was also discussed for the release of Kubeflow 1.3 but could not be included because it would break the logout button of the centraldashboard. Is this still an issue? #1714 (comment)

@juliusvonkohout
Copy link
Member

It will also be interesting to see how it works with the new DEX version 2.31.2 #2243 (comment)

@QipengZhou
Copy link

Updating the oidc-authservice was also discussed for the release of Kubeflow 1.3 but could not be included because it would break the logout button of the centraldashboard. Is this still an issue? #1714 (comment)

Logout button is still an issue.

@juliusvonkohout
Copy link
Member

Updating the oidc-authservice was also discussed for the release of Kubeflow 1.3 but could not be included because it would break the logout button of the centraldashboard. Is this still an issue? #1714 (comment)

Logout button is still an issue.

Is that also the case for the new DEX version 2.31.2 in Kubeflow 1.6 #2243 (comment) ?

@natalian98
Copy link

Hi Team, did you have a chance to test this upgrade?
We have tested the new oidc image with dex 2.31.2 in charmed kubeflow, but we're getting troubles with accessing the dashboard (403 authorization error). Are you aware of sth in the image impacting istio?
The previous image gcr.io/arrikto/kubeflow/oidc-authservice:fef11c3 works correctly with dex 2.31.2, but the logout button is still an issue @juliusvonkohout.
I'd appreciate your feedback, thanks!

@alembiewski
Copy link
Member

alembiewski commented Aug 8, 2022

I opened a PR to make the logout button work with the updated version of the authservice, feel free to review and provide feedback

@juliusvonkohout
Copy link
Member

@kimwnasptd any hope that it goes into 1.6 with the fixes from @alembiewski ?

@droctothorpe
Copy link

droctothorpe commented Aug 10, 2022

We also had to diverge from the upstream manifests since they're using an outdated version of oidc-authservice that doesn't support opaque tokens. The opaque token pattern lets us use the KFP SDK from outside of the cluster by consuming the auth tokens in our kubeconfig.

@dbg-raghulkrishna
Copy link

+1 I would also like this to be implemented in 1.6

@kimwnasptd
Copy link
Member

/ok-to-test

@manolis-andr @athamark could you help with this review to update the AuthService manifests? Let's also have a follow up PR afterwards to have an OWNERS file in that component.

@athamark
Copy link

athamark commented Feb 3, 2023

Hello everyone, sorry for the delay. I am picking this up. In the comments below I will try address the following topics:

  1. What does the new e236439 image of oidc-authservice offer to the Kubeflow users?
  2. Review the proposed changes of this PR.
  3. What else do we need for this effort to conclude?
  4. A brief overview of new features that AuthService offers but this image does not include.

@athamark
Copy link

athamark commented Feb 3, 2023

1. What does the proposed e236439 image of oidc-authservice offer to the Kubeflow users?

The transition proposed with this PR corresponds to ~50 commits. I will try to highlight the most important KF-visible aspects of these changes:

  1. we have introduced two new authentication methods (kubernetes and ID token) and we have factored out the session authentication method. This will allow authenticating Kubeflow users to perform requests with one of the three supported authentication methods. Note that for the two new methods AuthService examines the Bearer token included in the Authorization header of the request.
  2. we have iterated over the oidc-authservice code to resolve some known bugs. For example, oidc-authservice failed to accept an external Identity Provider that does not support the revocation of the access token. As stated in the RFC 7009 (https://www.rfc-editor.org/rfc/rfc7009#section-2):

    Implementations MUST support the revocation of refresh tokens and
    SHOULD support the revocation of access tokens
    We have made sure that oidc-authservice is compliant on this front.

@athamark
Copy link

athamark commented Feb 3, 2023

2. Review the proposed changes of this PR.

All the changes proposed for the:

  • common/dex/base/config-map.yaml
  • common/oidc-authservice/base/params.env
  • common/oidc-authservice/base/kustomization.yaml
  • common/oidc-authservice/base/rbac.yaml
  • common/oidc-authservice/base/statefulset.yaml

make sense. I do not see anything that needs to be corrected. Good job @ittus!

Just one minor comment regarding the REDIRECT_URL:
Keep in mind that the e236439 image does not handle gracefully the REDIRECT_URL configuration. Admins may initialize it but it won't affect the behavior of oidc-authservice as intended. We resolved this issue with this PR: arrikto/oidc-authservice#99.

Since this behavior was part of the 6ac9400 image of oidc-authservice which we currently support for Kubeflow, we should not block on this. But let's expose this so that the community is aware of it.

@athamark
Copy link

athamark commented Feb 3, 2023

3. What else do we need for this effort to conclude?

As mentioned above in this conversation, we can not merge this effort without updating the CentralDashboard. See here:

I tested the e236439 image with the current CentralDashboard to verify the above claims. Indeed we need to make some modifications on the CentralDashboard to avoid the problem with the logout button.

For this, I am in sync with @kimwnasptd and we will comment on the respective effort to highlight minor fixes that we should make.
In short, we should first take care of the logout button and then we should proceed with merging this PR. @kimwnasptd I suggest that we start reviewing the kubeflow/kubeflow#6609.

@athamark
Copy link

athamark commented Feb 3, 2023

4. A brief overview of new features that AuthService offers but this image does not include.

Later oidc-authservice image will:

  1. include slight improvements regarding the log messages.
  2. support both bool and string values for the email_verified standard claim. Case is that some Identity Providers (e.g. AWS Cognito) deviate from this spec (https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims) and return string values instead (email_verified attribute is a string amazon-archives/amazon-cognito-identity-js#436, User info fails to unmarshal when email_verified is a string arrikto/oidc-authservice#63).
  3. include a caching mechanism and other small changes for boosting the authentication performance.
  4. include two new authentication methods for both JWT access tokens, and opaque access tokens.
  5. give the option of integrating with an external authorizer (right now the only authorization check that oidc-authservice offers is group-based).
  6. support redis session store, admins will be able to select between the currently supported boltDB session store and the redis session store.

@jbottum
Copy link

jbottum commented Feb 3, 2023

@yuzisun @yubozhao this might be interesting to inference systems that integrate with Kubeflow

@kimwnasptd
Copy link
Member

Thanks for the thorough review and context @athamark! I'll move on and merge this PR.

The follow up step is to work on kubeflow/kubeflow#6609 and ensure it can use a dynamic link for the logout URL. So the first RC will have a bug with the logout but we'll fix it during feature freeze. I'll open an issue so that we can track this.

Thank you for the work @ittus!

/lgtm
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ittus, 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:

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

@google-oss-prow google-oss-prow bot merged commit a114d58 into kubeflow:master Feb 6, 2023
@annajung annajung mentioned this pull request Feb 9, 2023
kevin85421 pushed a commit to juliusvonkohout/manifests that referenced this pull request Feb 28, 2023
* feat: upgrade oidc-authservice

* Update common/oidc-authservice/base/params.env
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.