-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add authentication with ServiceAccountToken #5138
Comments
Thank you for the proposal! I'd love to see it getting it upstreamed too. It's a common request in #4440. |
Hello! I'll provide an update here as I'll be pushing a PR covering the backend part very soon. As mentioned in the first comment, we are adding a new authentication method: authentication using ServiceAccountTokens. How will this ServiceAccountToken find its way in the requests?
What does the authentication cycle of the backend look like?
How does the ServiceAccountToken authenticator work?
Useful links:
How does the client find a ServiceAccountToken to use?Kubernetes has built-in ways to project tokens with specific audience for the ServiceAccount of a pod. The KFP client should have a seamless way to
The token has an expiration time, however the kubelet makes sure to refresh this token before it expires. This last part is also relevant to the discussion of #4683 Useful links: |
@elikatsis @yanniszark If any help is needed so this can be added for 1.3 please let me know. I think a large portion of the community has been waiting for this a while now and it would be great to have it included in 1.3. |
/assign @elikatsis |
@davidspek thanks for volunteering! Before I open the client PR I'll present some implementation details (we've described an overview in the comment above) As mentioned in #4683 (comment), we want to have a generic way to provide credentials to the client. We will be using a Requirements
Information about the Kubernetes Configuration object
[Expanding (1)] As shown in this source, by providing a
[Expanding (3)] The
[Expanding (2)] The ConclusionsTo sum up, what we need to do is:
So, for this case (authentication with ServiceAccountTokens), we need to
Design decisions
How to set up the pod to authenticate against KFPWe (Arrikto) have been using a apiVersion: kubeflow.org/v1alpha1
kind: PodDefault
metadata:
name: access-ml-pipeline
spec:
desc: Allow access to Kubeflow Pipelines
selector:
matchLabels:
access-ml-pipeline: "true"
volumeMounts:
- mountPath: /var/run/secrets/ml-pipeline
name: volume-ml-pipeline-token
readOnly: true
volumes:
- name: volume-ml-pipeline-token
projected:
sources:
- serviceAccountToken:
path: token
expirationSeconds: 7200
audience: ml-pipeline
env:
- name: ML_PIPELINE_SA_TOKEN_PATH
value: /var/run/secrets/ml-pipeline/token # this is dependent on the volume mount path and SAT path |
@elikatsis Thanks for the detailed post. I will look at it more closely tomorrow and do my best to help test the PRs. |
Part of kubeflow#5138 This is a subclass of TokenCredentials and implements the logic of retrieving a service account token provided via a ProjectedVolume. The 'get_token()' method reads and doesn't store the token as the kubelet is refreshing it quite often. Relevant docs: https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#service-account-token-volume-projection
Hi @elikatsis! Thank you for the detailed design and PRs! However, despite that, I'm a little concerned that the design was only made public 5 days before Kubeflow 1.3 feature cut date -- March 15th. I think we agreed early on the rough direction, that was a good heads up, but it's not possible to discuss this fairly complex feature design thoroughly within 5 days. If we commit to shipping this in KF 1.3, we can only rush to a decision. Besides that an important dependency (important in the terms of making zero-config default better experience) on |
@Bobgy thanks for putting time on this!
Your concerns are totally valid and understandable. We agree it is very close to the first RC and this may be a bit pressing.
Indeed, this is an advanced feature. However, most of the changes we had already discussed due to the joint talk you had with @yanniszark. That's why we expect the backend changes to be unsurprising. Note that all of the changes are extensions to existing functionality and are not removing or changing any old behavior.
We agree, but it's not necessary to have a zero-config issue before the RC. We can still use the alternative of some config, if we want. To sum up: yes, we are very close to the RC (also take into consideration that cutting a release was pushed one week), but let's do our best and see if we can make it! Many users rely on it. If we don't make it, it's ok! |
@elikatsis thanks I just realized the RC cut delay, I'm glad we get some more breath on this feature.
Makes sense, so I'd frame the discussion around common things we agree on, would you mind splitting your PR as smaller ones , so that we can approve the ones we fully agree on right now first for the RC? (For clarification, I don't mean to ask you to split right now, but rather during review if we see parts that everyone agrees on, we can split them out for a quick merge.) and I've got very good context on the backend part based on previous discussion with Yannis, I think we can get them merged. The only part I have concerns is the user facing interface to add service account tokens. What do you think about letting KFP api server inject projected service account token to every KFP pod? I don't think that raises more security risk (because service account tokens are already available there), nor is there chance to break existing components. Pros -- we do not need PodDefault there, so one less dependency. e.g. I guess we can configure https://argoproj.github.io/argo-workflows/default-workflow-specs/ with a global |
Extend the authenticators which the KFP apiserver applies on a request with a TokenReview authenticator. This authenticator expects a ServiceAccountToken in a header with the format: 'Authorization: Bearer <token>' Part of kubeflow#5138
For clarification, I'm prioritizing reviewing the backend PR, because it's a blocker of release. The SDK PR can be released after Kubeflow release, because users can easily upgrade the SDK at any time, and there's very little coupling to the server. |
Extend the authenticators which the KFP apiserver applies on a request with a TokenReview authenticator. This authenticator expects a ServiceAccountToken in a header with the format: 'Authorization: Bearer <token>' Part of kubeflow#5138
… of #5138 (#5286) * Introduce kubernetes client utils Introduce common utils for client initialization to factor out common code. This is a step towards fulfilling #4738. * Use common util to initialize k8s clientset * Introduce TokenReview client and fake ones * Extend ResourceManager with a TokenReview client * Extend FakeClientManager with a fake TokenReview Client * Introduce authentication utils * Introduce HTTP header authenticator * Initialize Kubeflow-UserID header authenticator * Refactor getUserIdentity() to use auth_util * Move getting user identity logic to resource manager Have the resource manager authenticate the request. In following commits we will be extending the authentication methods to use, among others, Kubernetes clients. Thus, we move the logic to the resource manager to benefit from the clients kept its context. * Introduce constants for the TokenReview authentication * Introduce TokenReview authenticator * Extend authenticators with a TokenReview one Extend the authenticators which the KFP apiserver applies on a request with a TokenReview authenticator. This authenticator expects a ServiceAccountToken in a header with the format: 'Authorization: Bearer <token>' Part of #5138 * Add tests for auth_util * Add tests for HTTPHeaderAuthenticator * Update server tests based on the new authentication API * Remove old tests and unused code * Add tests for TokenReviewAuthenticator * Add server tests with unauthenticated requests * manifests: Allow KFP API server to create TokenReviews * auth: Split 'auth_util.go' into two parts Split the file into: * auth.go: contains the main entrance from the outside of the package * util.go: contains all utility functions used inside * Change token review audience variable and value * Allow configuring audience with an environment variable * Rename IsRequestAuthenticated -> AuthenticateRequest * Don't use AuthenticateRequest method in tests Instead of using AuthenticateRequest to retrieve the user from the request and then use it for the expected values, allocate a variable for the username in the request and use that in the expected values. This ensures we don't hide potential errors of AuthenticateRequest. * Change authenticators order Have the HTTPHeaderAuthenticator first followed by the TokenReviewAuthenticator * Move authenticators to a ResourceManager property To avoid potential race conditions when initializing the Authenticators variable, we move authenticators to a ResourceManager property and initialize it along with the initialization of the manager.
I had totally missed these comments 😱
We've merged the backend now, I hope you are good with this and did not hesitate asking me to split some commits. Next time feel free to explicitly ask for things like that during the review!
These sound like very good ideas. However, maybe we want an explicit way to declare something like "allow this pod to have access to KFP, but not this one". We will iterate on these ideas internally and come back to it! |
@elikatsis Does it make sense to integrate the PodDefault you shared above with the notebook controller to make the user experience more seamless? I believe this would be the best way to solve this long standing issue. |
No worries, the backend PR LGTM. I was mostly talking about concerns for the sdk PR.
I'd prefer adhering to the standard RBAC model. Each Pod has access to a service account, while we add RBAC rules to control what one service account can do. I worry the addition of choosing which pods can have access to KFP api is introducing an unnecessary abstract layer. |
@elikatsis @yanniszark I just recall a separate concern with current implementations. When we support using service account token to authenticate in KFP api server, we need to configure Istio authorization rules to allow all types of access. However, that seems to break the security assumption when using HTTP user id header to authenticate -- only requests from istio ingress gateway are allowed to access KFP api server. How do you overcome this problem? Per previous discussion in #4440 (comment), we should implement ability to parse the special Istio header X-Forwarded-Client-Cert, so that KFP API server can know which requests come from Istio ingress gateway. and this part is currently missing in the implementations |
First #6537 should be merged. Then you could easily alter the two following kustomization patch files. I use them currently to add stuff for istio-cni on Openshift, but you can easily add a poddefault too. This is the official way that is used to prepare a user namespace in kubeflow. Metacontroller monitors the namespace and creates the resources gathered from pipelines-profile-controller sync.py. E.g. it adds the serviceaccounts and visualization server to each namespace and in the future it should also add a poddefault. |
Please merge and you will have automatic authentication by default @bartgras feel free to test it yourself |
Sorry for the delay. There are three options as commented by me in the specific issues.
|
@thesuperzapper which of the three options above sound sensible to you? Or do you have another Idea? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stalebot this is still relevant |
Just installed KubeFlow on AWS EKS few days ago. Used aws kfl manifests from: All in all it works, so I created a new jupiter notebooks server.
Error message:
I also tried:
Error:
Also tried:
Error message in this case:
When I was creating the jupiter server I checked config to give server access to pipelines, and in notebooks this code:
prints token data Any ideas what I can do to reach pipelines service from notebooks? |
After kfp lib update it works fine, exact code that works for me is the variant without anything really special:
So all in all for me it was:
|
Hi, do I understand correctly that solution discussed in this issue solves only authentication inside the cluster and using only K8s service account? Is there any plan or issue related to authentication using cloud-specific service account/service principal/etc (in my case GCP)? Current solution with only interactive and only user-based authentication is highly not sufficient (https://www.kubeflow.org/docs/distributions/gke/pipelines/authentication-sdk/). |
@tomaszstachera check out the new oidc-authservice |
/cc @chensun |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I think this is solved with our new oauth2-proxy and token based authentication. There is a small new bug kubeflow/manifests#2747 but that will be fixed for 1.9. So we support heterogeneous ways of authenticating. /close |
@juliusvonkohout: Closing this issue. In response to this:
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-sigs/prow repository. |
Problem Statement
Clients in various namespaces (e.g., Notebooks) need to access the Pipelines API. However, there is currently no way for these clients to authenticate to the Pipelines API:
#4440
#4733
In-cluster clients need a way to authenticate to the KFP API Server.
Proposed Solution
The correct way to do this is by using audience-scoped ServiceAccountTokens. In Arrikto's Kubeflow distribution, we have been successfully using this method for a long time, in numerous customer environments. We want to upstream this solution so the whole community can benefit as well, since we see this is an issue many users bump into.
Changes need to happen in 2 places:
/assign @yanniszark
cc @Bobgy
The text was updated successfully, but these errors were encountered: