Oauth support in flytectl #49

pmahindrakar-oss · 2021-04-03T14:43:10Z

Signed-off-by: Prafulla Mahindrakar [email protected]

TL;DR

Config for querying admin

admin:
  # For GRPC endpoints you might want to use dns:///flyte.myexample.com
  clientId: flytectl
  endpoint: dns:///demo.nuclyde.io
  authType: Pkce
logger:
  show-source: true
  level: 6

flytectl get project
Warn: No metricsProvider set for the workqueue
INFO[0000] Using config file: [config.yaml]             
INFO[0000] Config section [root] updated. No update handler registered.  src="viper.go:318"
INFO[0000] Config section [logger] updated. Firing updated event.  src="viper.go:320"
{"json":{"src":"viper.go:320"},"level":"info","msg":"Config section [admin] updated. Firing updated event.","ts":"2021-04-28T21:10:49+05:30"}
{"json":{"src":"viper.go:318"},"level":"info","msg":"Config section [storage] updated. No update handler registered.","ts":"2021-04-28T21:10:49+05:30"}
{"json":{"src":"viper.go:318"},"level":"info","msg":"Config section [adminutils] updated. No update handler registered.","ts":"2021-04-28T21:10:49+05:30"}
{"json":{"src":"client.go:182"},"level":"info","msg":"Instantiating a token source to authenticate against Admin, ID: flytectl","ts":"2021-04-28T21:10:49+05:30"}
{"json":{"src":"token_cache_with_keyring.go:59"},"level":"debug","msg":"fetched token from cache with expiry at 2021-04-29 03:03:19.535504 +0530 IST","ts":"2021-04-28T21:10:51+05:30"}
{"json":{"src":"auth_flow_orchestrator.go:35"},"level":"debug","msg":"got a response from the refresh grant for old expiry 2021-04-29 03:03:19.535504 +0530 IST with new expiry 2021-04-29 03:03:19.535504 +0530 IST","ts":"2021-04-28T21:10:51+05:30"}
{"json":{"src":"client.go:65"},"level":"info","msg":"Initialized Admin client","ts":"2021-04-28T21:10:51+05:30"}
{"json":{"src":"project.go:87"},"level":"debug","msg":"Retrieved 4 projects","ts":"2021-04-28T21:10:51+05:30"}
 --------------- --------------- ---------------------------------- 
| ID (4)        | NAME          | DESCRIPTION                      |
 --------------- --------------- ---------------------------------- 
| flytectldemo  | flytectldemo  | Some description for the project |
 --------------- --------------- ---------------------------------- 
| flyteexamples | flyteexamples | flyteexamples description        |
 --------------- --------------- ---------------------------------- 
| flytesnacks   | flytesnacks   | flytesnacks description          |
 --------------- --------------- ---------------------------------- 
| flytetester   | flytetester   | flytetester description          |
 --------------- --------------- ---------------------------------- 
4 rows

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#945

Follow-up issue

NA

pkg/auth/oauth2_client.go

codecov · 2021-04-10T05:10:26Z

Codecov Report

Merging #49 (ae896c1) into master (510e46b) will increase coverage by 0.32%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   50.69%   51.02%   +0.32%     
==========================================
  Files          40       42       +2     
  Lines        1150     1170      +20     
==========================================
+ Hits          583      597      +14     
- Misses        500      502       +2     
- Partials       67       71       +4

Flag	Coverage Δ
unittests	`51.02% <55.55%> (+0.32%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/get/project.go	`0.00% <ø> (ø)`
pkg/pkce/token_cache_keyring.go	`55.55% <55.55%> (ø)`
pkg/filesystemutils/file_system_utils.go	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 510e46b...ae896c1. Read the comment docs.

cmd/root.go

pkg/auth/oauth2_client.go

pmahindrakar-oss · 2021-04-27T13:46:49Z

pkg/auth/auth_flow_orchestrator.go

@@ -0,0 +1,145 @@
+package auth
+
+import (


More Unit tests remaining for this package and will be added soon.

EngHabu

Awesome! I have some questions about the token exchange stuff and potentially simplifying the code (that surrounds each api call)... otherwise looks ready (minus those unit tests you are already adding)... thank you!

EngHabu · 2021-04-27T20:12:35Z

cmd/create/execution.go

+		exec, err = cmdCtx.AdminClient().CreateExecution(_ctx, executionRequest, _callOptions...)
+		return err
+	}
+	err = auth.Do(ctx, cmdCtx.AuthClient(), grpcAPICall, callOptions)


Is there a way to simplify this?
Like here
You can build the authmetadataclient without initializing that singleton connection, then call FlyteClient() and if that returns then you can setup a PerCallRPCCredentials and set it as a dial option (instead of a call option) ... and therefore, you do not need to surround every single call to admin with this...

Done. Created new MR for this flyteorg/flyteidl#156

Will be updating this MR with just changes to flyteidl and couple of modules to be used. Rest there are no changes in flytectl side with this new movement

EngHabu · 2021-04-27T20:17:31Z

pkg/auth/admin_grpc_call_util.go

+func Do(ctx context.Context, authMetadataClient service.AuthMetadataServiceClient, grpcAPICallContext AdminGrpcAPICallContext, callOptions []grpc.CallOption) error {
+	// Fetch from the cache only when usAuth is enabled.
+	useAuth := admin.GetConfig(ctx).UseAuth
+	if useAuth {


I'm getting rid of this... if we can attempt to setup auth before we Dial(), I think that will simplify the experience a ton....

EngHabu · 2021-04-27T20:18:33Z

pkg/auth/auth_flow_orchestrator.go

+type TokenOrchestrator struct {
+}
+
+// This is a copy of oauth2.internal.tokenJSON as its not accesible outside.


nit:

Suggested change

// This is a copy of oauth2.internal.tokenJSON as its not accesible outside.

// This is a copy of oauth2.internal.tokenJSON as it's not accesible outside.

EngHabu · 2021-04-27T20:21:03Z

pkg/auth/auth_flow_orchestrator.go

+
+func (f TokenOrchestrator) RefreshTheToken(ctx context.Context, clientConf *oauth2.Config, token *oauth2.Token) *oauth2.Token {
+	// ClientSecret is empty here. Basic auth is only needed to refresh the token.
+	client := newBasicClient(clientConf.ClientID, clientConf.ClientSecret)


This is concerning.... public clients should not need or attempt to keep secrets.... if that means we can't refresh tokens, then so be it... we can make the default access token expire in 2 hours (or so) to give people a slightly improved experience...

Or is the idea that this flow will be used in CI/CD system instead?

Actually internally oauth libraries do the same when they refresh the token by sending an empty client secret. I have removed this logic and instead using

/ Token returns the current token if it's still valid, else will // refresh the current token (using r.Context for HTTP client // information) and return the new one. func (s *reuseTokenSource) Token() (*Token, error) {

This automatically refreshes the token if the access token is expired.

EngHabu · 2021-04-27T20:27:19Z

pkg/auth/auth_flow_orchestrator.go

+		"refresh_token": {token.RefreshToken},
+		"scope":         {"all", "offline"},
+	}
+	_, body, err := client.Post(clientConf.Endpoint.TokenURL, payload)


can't we use oauth2.Config to Exchange and refresh tokens?

Removed this logic now and uses

EngHabu · 2021-04-27T20:35:07Z

pkg/auth/handler_app_call_back.go

+			_, _ = rw.Write([]byte(fmt.Sprintf(`<p>Couldn't get access token due to error: %s</p>`, err.Error())))
+			return
+		}
+		_, _ = rw.Write([]byte(`<p>Cool! Your authentication was successful and you can close the window.<p>`))


If we want to go fancy, I would say we should do something like this: https://blakewilliams.me/posts/handling-macos-url-schemes-with-go
(which seems to be supported in OSX and windows... not sure about general Linux support) to silently go back to the app once the user is authenticated/authorized...

but maybe that's too fancy?

Looks cool . Let me add try this in a followup

Signed-off-by: Prafulla Mahindrakar <[email protected]>

Signed-off-by: Haytham Abuelfutuh <[email protected]> Signed-off-by: Prafulla Mahindrakar <[email protected]>

EngHabu reviewed Apr 3, 2021

View reviewed changes

pmahindrakar-oss requested a review from EngHabu April 5, 2021 12:33

pmahindrakar-oss changed the title ~~Draft changes for Oauth support in flytectl~~ wip: Draft changes for Oauth support in flytectl Apr 12, 2021

kumare3 reviewed Apr 14, 2021

View reviewed changes

cmd/root.go Outdated Show resolved Hide resolved

EngHabu reviewed Apr 22, 2021

View reviewed changes

pkg/auth/oauth2_client.go Outdated Show resolved Hide resolved

pmahindrakar-oss force-pushed the pmahindrakar/oauth branch 2 times, most recently from dc283a1 to 6644f0f Compare April 27, 2021 11:18

pmahindrakar-oss changed the title ~~wip: Draft changes for Oauth support in flytectl~~ Oauth support in flytectl Apr 27, 2021

pmahindrakar-oss marked this pull request as ready for review April 27, 2021 13:17

pmahindrakar-oss requested a review from wild-endeavor as a code owner April 27, 2021 13:17

pmahindrakar-oss commented Apr 27, 2021

View reviewed changes

EngHabu reviewed Apr 27, 2021

View reviewed changes

Using changed flyteidl which contains oauth changes

48cbd2a

Signed-off-by: Prafulla Mahindrakar <[email protected]>

pmahindrakar-oss force-pushed the pmahindrakar/oauth branch from a707cab to 48cbd2a Compare April 28, 2021 15:37

pmahindrakar-oss added 4 commits April 29, 2021 17:55

Added zlando keyring as an implementation of tokencache

f9fde2d

Signed-off-by: Prafulla Mahindrakar <[email protected]>

Fixed unit tests

3de966a

Signed-off-by: Prafulla Mahindrakar <[email protected]>

Fixed linter issues

d7746d8

Signed-off-by: Prafulla Mahindrakar <[email protected]>

go mod tidy

b72e204

Signed-off-by: Prafulla Mahindrakar <[email protected]>

EngHabu force-pushed the pmahindrakar/oauth branch from 3bbbd29 to 0f5043c Compare April 30, 2021 01:54

EngHabu previously approved these changes Apr 30, 2021

View reviewed changes

kumare3 self-requested a review May 2, 2021 21:30

kumare3 previously approved these changes May 2, 2021

View reviewed changes

EngHabu added 2 commits May 3, 2021 08:51

Update to latest flyteidl

c139447

Signed-off-by: Haytham Abuelfutuh <[email protected]> Signed-off-by: Prafulla Mahindrakar <[email protected]>

Update to released flyteidl

ae896c1

Signed-off-by: Haytham Abuelfutuh <[email protected]> Signed-off-by: Prafulla Mahindrakar <[email protected]>

pmahindrakar-oss dismissed stale reviews from kumare3 and EngHabu via ae896c1 May 3, 2021 03:21

pmahindrakar-oss force-pushed the pmahindrakar/oauth branch from 0f5043c to ae896c1 Compare May 3, 2021 03:21

kumare3 self-requested a review May 3, 2021 04:44

kumare3 approved these changes May 3, 2021

View reviewed changes

pmahindrakar-oss merged commit cf63c5c into master May 3, 2021

pmahindrakar-oss deleted the pmahindrakar/oauth branch May 3, 2021 04:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oauth support in flytectl #49

Oauth support in flytectl #49

pmahindrakar-oss commented Apr 3, 2021 •

edited

Loading

codecov bot commented Apr 10, 2021 •

edited

Loading

pmahindrakar-oss Apr 27, 2021

EngHabu left a comment

EngHabu Apr 27, 2021

pmahindrakar-oss Apr 28, 2021

pmahindrakar-oss Apr 28, 2021

EngHabu Apr 27, 2021

pmahindrakar-oss Apr 28, 2021

EngHabu Apr 27, 2021

pmahindrakar-oss Apr 28, 2021

EngHabu Apr 27, 2021

EngHabu Apr 27, 2021

pmahindrakar-oss Apr 28, 2021

EngHabu Apr 27, 2021

pmahindrakar-oss Apr 28, 2021

EngHabu Apr 27, 2021

pmahindrakar-oss May 3, 2021

	// This is a copy of oauth2.internal.tokenJSON as its not accesible outside.
	// This is a copy of oauth2.internal.tokenJSON as it's not accesible outside.

Oauth support in flytectl #49

Oauth support in flytectl #49

Conversation

pmahindrakar-oss commented Apr 3, 2021 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented Apr 10, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

EngHabu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmahindrakar-oss commented Apr 3, 2021 •

edited

Loading

codecov bot commented Apr 10, 2021 •

edited

Loading