feat(backend): Support authentication with ServiceAccountTokens. Part of #5138 #5286

elikatsis · 2021-03-11T12:34:14Z

Description of your changes:
The changes are described in detail in this comment: #5138 (comment)

In a nutshell:

We make the authentication mechanisms extendable
We extend the authentication mechanisms with an authenticator for ServiceAccountTokens
The aforementioned authenticator expects ServiceAccountTokens
- in a bearer token header: Authorization: Bearer <token>
- with ml-pipeline as audience

/cc @Bobgy
/cc @yanniszark
/cc @StefanoFioravanzo
/assign @elikatsis

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

Do you want this pull request (PR) cherry-picked into the current release branch?

Learn more about cherry-picking updates into the release branch.

Bobgy · 2021-03-16T11:27:28Z

You also need to update ~/manifests/kustomize/base/installs/multi-user/api-service/cluster-role.yaml to include API server role changes for multi-user mode.

(being able to update it in the same PR thanks to @yanniszark's work for moving them here)

Bobgy

Thank you for the super useful contribution that many users are hoping for!

I'm leaving some early comments before I finish reading all the code. I'll try to finish review tomorrow.
Note, most of them are nit pickings, so feel free to discuss about them. I don't think they are blockers.

backend/src/apiserver/server/util.go

backend/src/apiserver/server/experiment_server_test.go

backend/src/apiserver/common/const.go

backend/src/apiserver/auth/auth_util.go

elikatsis

@Bobgy thank you for your comments. They are to the point.
I've answered to each one of them and I'll push a few extra commits on top

backend/src/apiserver/server/experiment_server_test.go

backend/src/apiserver/auth/auth_util.go

backend/src/apiserver/common/const.go

backend/src/apiserver/server/util.go

elikatsis · 2021-03-17T18:03:38Z

@Bobgy I rebased on top of latest master (there were some conflicts after the merge of #5231) adding extra commits to address your comments

Bobgy

Thank you! I've went through the PR, it's very well structured and well written.
I feel pretty confident about it, except for some very minor nitpickings

backend/src/apiserver/auth/auth.go

google-oss-robot · 2021-03-18T08:27:51Z

@Bobgy: GitHub didn't allow me to request PR reviews from the following users: elikatsis.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

I'm not very familiar with concurrent programming in golang. Can this be a race condition?
e.g. the server runs as many go routines, and there can be two incoming requests triggering this method at the same time.

/cc @capri-xiyue @elikatsis

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.

Bobgy · 2021-03-18T08:28:06Z

Can you please resolve another conflict?

Bobgy · 2021-03-18T08:31:09Z

/lgtm

Introduce common utils for client initialization to factor out common code. This is a step towards fulfilling kubeflow#4738.

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

Split the file into: * auth.go: contains the main entrance from the outside of the package * util.go: contains all utility functions used inside

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.

Have the HTTPHeaderAuthenticator first followed by the TokenReviewAuthenticator

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.

Bobgy · 2021-03-18T12:58:35Z

/lgtm
Thanks! I feel this is ready, but I'd want to wait for @capri-xiyue's review too.
I think this can catch up with the release cut.

capri-xiyue · 2021-03-18T19:07:37Z

/lgtm

Bobgy · 2021-03-19T02:42:56Z

/approve
@elikatsis thank you for the great contribution!

google-oss-robot · 2021-03-19T02:43:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, elikatsis

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:

~~backend/OWNERS~~ [Bobgy]
~~manifests/kustomize/OWNERS~~ [Bobgy]

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

elikatsis · 2021-03-19T10:22:18Z

@Bobgy and thank you for the quick and detailed review! I think the code is very improved now.

google-oss-robot assigned elikatsis Mar 11, 2021

google-oss-robot requested review from Bobgy, StefanoFioravanzo and yanniszark March 11, 2021 12:34

google-oss-robot added the size/XXL label Mar 11, 2021

google-cla bot added the cla: yes label Mar 11, 2021

This was referenced Mar 11, 2021

Add authentication with ServiceAccountToken #5138

Closed

Katib x Kale x Multi-User Kubeflow doesn't support userid injection kubeflow/katib#1454

Closed

Bobgy reviewed Mar 16, 2021

View reviewed changes

elikatsis commented Mar 17, 2021

View reviewed changes

elikatsis force-pushed the feature-backend-authenticators branch from 0f55e91 to 2a8ca8a Compare March 17, 2021 18:02

Bobgy reviewed Mar 18, 2021

View reviewed changes

backend/src/apiserver/auth/auth.go Outdated Show resolved Hide resolved

backend/src/apiserver/auth/auth.go Outdated Show resolved Hide resolved

google-oss-robot requested a review from capri-xiyue March 18, 2021 08:27

google-oss-robot assigned Bobgy Mar 18, 2021

google-oss-robot added the lgtm label Mar 18, 2021

elikatsis force-pushed the feature-backend-authenticators branch from ac4936f to 5ef17ea Compare March 18, 2021 09:30

google-oss-robot removed the lgtm label Mar 18, 2021

elikatsis added 7 commits March 18, 2021 11:30

Introduce kubernetes client utils

3236948

Introduce common utils for client initialization to factor out common code. This is a step towards fulfilling kubeflow#4738.

Use common util to initialize k8s clientset

41e950b

Introduce TokenReview client and fake ones

c043de4

Extend ResourceManager with a TokenReview client

fcac9f8

Extend FakeClientManager with a fake TokenReview Client

7b2b451

Introduce authentication utils

e1ec878

Introduce HTTP header authenticator

3f0b2e1

elikatsis added 15 commits March 18, 2021 11:30

Introduce constants for the TokenReview authentication

77cc6df

Introduce TokenReview authenticator

6485de3

Extend authenticators with a TokenReview one

fa3c151

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

Add tests for auth_util

059d570

Add tests for HTTPHeaderAuthenticator

16fc740

Update server tests based on the new authentication API

40acf1f

Remove old tests and unused code

f508e66

Add tests for TokenReviewAuthenticator

01f1faa

Add server tests with unauthenticated requests

66decb2

manifests: Allow KFP API server to create TokenReviews

00f76c5

auth: Split 'auth_util.go' into two parts

f61c73c

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

b282e63

Allow configuring audience with an environment variable

c70ab9b

Rename IsRequestAuthenticated -> AuthenticateRequest

7e8fce4

elikatsis force-pushed the feature-backend-authenticators branch from 5ef17ea to 3dc13dc Compare March 18, 2021 09:31

elikatsis added 2 commits March 18, 2021 12:17

Change authenticators order

9310e1a

Have the HTTPHeaderAuthenticator first followed by the TokenReviewAuthenticator

Move authenticators to a ResourceManager property

b350d93

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.

google-oss-robot added the lgtm label Mar 18, 2021

google-oss-robot assigned capri-xiyue Mar 18, 2021

google-oss-robot added the approved label Mar 19, 2021

google-oss-robot merged commit 5db66e0 into kubeflow:master Mar 19, 2021

elikatsis deleted the feature-backend-authenticators branch March 19, 2021 10:22

juliusvonkohout mentioned this pull request Jun 3, 2022

feat(backend): authorize readArtifacts and ReportMetrics endpoints #7819

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): Support authentication with ServiceAccountTokens. Part of #5138 #5286

feat(backend): Support authentication with ServiceAccountTokens. Part of #5138 #5286

elikatsis commented Mar 11, 2021

Bobgy commented Mar 16, 2021 •

edited

Loading

Bobgy left a comment

elikatsis left a comment

elikatsis commented Mar 17, 2021

Bobgy left a comment

google-oss-robot commented Mar 18, 2021

Bobgy commented Mar 18, 2021

Bobgy commented Mar 18, 2021

Bobgy commented Mar 18, 2021

capri-xiyue commented Mar 18, 2021

Bobgy commented Mar 19, 2021

google-oss-robot commented Mar 19, 2021

elikatsis commented Mar 19, 2021

feat(backend): Support authentication with ServiceAccountTokens. Part of #5138 #5286

feat(backend): Support authentication with ServiceAccountTokens. Part of #5138 #5286

Conversation

elikatsis commented Mar 11, 2021

Bobgy commented Mar 16, 2021 • edited Loading

Bobgy left a comment

Choose a reason for hiding this comment

elikatsis left a comment

Choose a reason for hiding this comment

elikatsis commented Mar 17, 2021

Bobgy left a comment

Choose a reason for hiding this comment

google-oss-robot commented Mar 18, 2021

Bobgy commented Mar 18, 2021

Bobgy commented Mar 18, 2021

Bobgy commented Mar 18, 2021

capri-xiyue commented Mar 18, 2021

Bobgy commented Mar 19, 2021

google-oss-robot commented Mar 19, 2021

elikatsis commented Mar 19, 2021

Bobgy commented Mar 16, 2021 •

edited

Loading