OAuth2 support for flyte-cli and SDK engine #23

katrogan · 2019-10-11T23:47:22Z

This change adds authentication support for flyte-cli and the pyflyte CLIs.

New authorization code

Specifically this change introduces an AuthorizationClient which implements the PKCE authorization flow for untrusted clients. This client handles requesting an initial access token, spinning up a callback server to receive the access token and using that to retrieve an authorization code. The client also handles refreshing expired authorization tokens.

This change also includes a lightweight DiscoveryClient for retrieving authorization endpoint metadata defined in the OAuth 2.0 Authorization Server Metadata draft document.

An authorization client singleton is lazily initialized for use by flyte-cli.

Pyflyte changes (basic auth)

Requests an authorization token using a username and password.

Flyte-cli changes (standard auth)

Requests an authorization token using the PKCE flow.

Raw client changes

Wraps RPC calls to flyteadmin in a retry handler that initiates the appropriate authentication flow defined in the flytekit config in response to HTTP 401 unauthorized response codes.

flytekit/clis/auth/auth.py

flytekit/clients/raw.py

wild-endeavor · 2019-11-06T02:13:20Z

flytekit/clis/flyte_cli/main.py

 from flytekit.clis.helpers import construct_literal_map_from_variable_map as _construct_literal_map_from_variable_map, \
    construct_literal_map_from_parameter_map as _construct_literal_map_from_parameter_map, \
    parse_args_into_dict as _parse_args_into_dict, str2bool as _str2bool
 from flytekit.common import utils as _utils, launch_plan as _launch_plan_common
 from flytekit.common.core import identifier as _identifier
 from flytekit.common.types import helpers as _type_helpers
 from flytekit.common.utils import load_proto_from_file as _load_proto_from_file
-from flytekit.configuration import platform as _platform_config
+from flytekit.configuration import creds as _creds_config, platform as _platform_config


so I think we should keep the underlying config objects regardless (because they'll be useful in pyflyte), but currently none of these files iirc are used in flyte-cli. That is, the flytekit.config style files are never read by flyte-cli, because it's assumed that users of flyte-cli might not have any flyte repos checked out at all - they may only be an administrator. Auth has a fair amount to config though, it's pretty awkward to pass in everything as command line args. What does @matthewphsmith think?

I think it should definitely be env var based. You can use the configuration package though to make things easier since the objects are env var aware.

I think making it env var based makes it difficult to set-up for an end-user. I'd prefer to have a config that can be easily cloned and written, but I'm happy to move it to a separate directory if it makes sense organizationally.

Haytham's argument was that there is a .aws/config and a .kube/config and a .gitconfig. Why can't there be a .flyte/config? I think it's cleaner than having to rely on five or six environment variables. What's the downside that you're worried about Matt?

I think we can definitely have .flyte/config The only thing is then we need to provide config modification commands (but with click opening up the editor is simple)
Another option is to have .flyte/config as an option otherwise explicitly specify all the args

matthewphsmith · 2019-11-11T19:57:04Z

flytekit/clis/auth/auth.py

+        from http import client as _StatusCodes
+    except ImportError:  # Python 2
+        import httplib as _StatusCodes
+from BaseHTTPServer import HTTPServer as _HTTPServer, BaseHTTPRequestHandler as _BaseHTTPRequestHandler


nit: let's import the module. it makes refactoring easier and keeps the namespace cleaner.

import BaseHTTPServer as _base_http_server Later... _base_http_server.HTTPServer

same for urlparse below

but it makes it harder to jump into the definition of the underlying object. and the ide shows everything as yellow.

hm actually i can still jump around in my ide to the underlying object with refactoring

flytekit/clis/auth/auth.py

flytekit/configuration/platform.py

matthewphsmith · 2019-11-11T20:18:14Z

flytekit/configuration/creds.py

+
+from flytekit.configuration import common as _config_common
+
+DISCOVERY_ENDPOINT = _config_common.FlyteStringConfigurationEntry('credentials', 'discovery_endpoint', default=None)


are all these env vars going to be auto-set via the auth flow? or would a user ever specify any of this? If yes, can you let me know about a few of those cases to help me understand?

These are specific to the hosted Flyte platform - so they can't be set. For example the discovery endpoint is configured in admin. The native Okta app at Lyft we're testing with has a unique client id and a hardcoded, pre-configured redirect URI. The authorization metadata key is a departure we have to make from the standard protocol because Envoy actually behaves funkily if we use the default value.

kumare3 · 2019-11-12T20:13:15Z

flytekit/clis/flyte_cli/main.py

+    if not _platform_config.AUTH.get():
+        # nothing to do
+        return None
+    access_token = _keyring.get_password(_metadata, _metadata_access_token)


That is awesome!

kumare3 · 2019-11-12T20:14:52Z

I think before an approval we should either demonstrate the auth flow?

…ry endpoint

katrogan · 2019-11-13T18:48:33Z

flytekit/configuration/creds.py

 """
-This is the redirect uri registered with the app which handles authorization for a Flyte deployment.
+This is the callback uri registered with the app which handles authorization for a Flyte deployment.
+Please note the hardcoded port number. Ideally we would not do this, but some IDPs do not allow wildcards for


Haytham suggested that we implement logic to either accept a wildcard port or a list of ports (i.e. if we register a bunch of hard-coded ports with the IDP.) That way we have some flexibility with choosing an available system port. I'm happy to save that for the next PR though and leave this comment in for now

…deleted, only using for debugging at the moment

katrogan · 2019-11-15T21:52:15Z

flytekit/clients/raw.py



 def _handle_rpc_error(fn):
+    @_try_three_times


hey @matthewphsmith we introduced this to make sure we don't get stuck in a loop if authentication is persistently failing...but i'm not sure that this is the prettiest code 🙃 let us know if a better way stands out

the code's been refactored but i think the original comment still stands

…that if the host is specified in the user's ~/.flyte/config file, you don't need to specify it in the flyte-cli command

requirements.txt

matthewphsmith · 2019-12-03T01:53:12Z

flytekit/clis/flyte_cli/example.config

+auth=true
+
+[credentials]
+discovery_endpoint=http://corp.idp.com/.well-known/oauth-authorization-server


Can you explain the use case a bit as to why a user would specify the discovery and redirect endpoint? Wouldn’t that config be based server side?

Just to clarify, this discovery endpoint should be pointed at flyte.corp.net or something... and that should redirect to whatever IDP corp.net is using. Or it can return the values itself without redirecting, but the point is, you only hard-code the main flyte server. Thinking about it, maybe we should nix this @katrogan and just assume the extension off of the platform URL. That's probably the cleanest thing to do.

The redirect endpoint is configured in the IDP. It's a hardcoded fully-specified URL. This means that the callback url (including the port) that the local server that's stood up at has to match exactly what the IDP has. This information is not returned by the metadata/discovery endpoint. Why i'm not sure... it may be a security concern.

Ideally, the metadata endpoint would be less restrictive and allow us to return more things. Because of the limitations of the RFC, we also need to hardcode this in for example: https://github.com/lyft/flyteexamples/pull/97/files#diff-2e41be19539d0ba6fda0e5ce814a9123R22. This is the scope that will be requested by pyflyte when it makes the token request. Again, like the callback url, it has to match a hardcoded string set within the IDP, otherwise it'll fail. It doesn't really offer anything, but we have to have it.

Re: assuming the extension - I'm happy to do this if we can safely assume this is the major convention (which I guess sounds like it is?)

yup https://tools.ietf.org/html/rfc8414#section-3

Hmm very interesting...well seems like you have a handle on it, so I defer to whatever you think is best though I am biased to less config wherever possible.

flytekit/clients/raw.py

matthewphsmith · 2019-12-03T02:10:53Z

flytekit/clis/sdk_in_container/basic_auth.py

+_utf_8 = 'utf-8'
+
+
+class FlyteAuthenticationException(_FlyteException):


NIt: should we define this in flyte.exceptions.user?

@wild-endeavor can we use https://github.com/lyft/flytekit/pull/23/files#diff-7fbb98dd27586954c518f84994ed987cR77?

flytekit/clis/flyte_cli/main.py

making a waterfall for how configuration settings are looked up from the environment variable

flytekit/clis/sdk_in_container/basic_auth.py

jonathanburns · 2019-12-06T09:38:54Z

flytekit/clients/raw.py

+    :return:
+    """
+    auth_endpoints = _credentials_access.get_authorization_endpoints()
+    token_endpoint = auth_endpoints.token_endpoint


Guessing you'll have to do something here to support relative URLs.

Not sure what you mean?

if the token endpoint is "/token", the code doesn't work, I think

jonathanburns · 2019-12-06T19:45:13Z

Any chance you can ask for password over stdin if password is required but not password is passed? Something like this:

https://github.com/lyft/flytekit/pull/56/files#diff-39ff0a124da92894da1f07bd5ce13c67R46-R47

already done

This change adds authentication support for flyte-cli and the pyflyte CLIs. # New authorization code Specifically this change introduces an **AuthorizationClient** which implements the [PKCE authorization flow](https://www.oauth.com/oauth2-servers/pkce/authorization-code-exchange/) for untrusted clients. This client handles requesting an initial access token, spinning up a callback server to receive the access token and using that to retrieve an authorization code. The client also handles refreshing expired authorization tokens. This change also includes a lightweight **DiscoveryClient** for retrieving authorization endpoint metadata defined in the [OAuth 2.0 Authorization Server Metadata](https://tools.ietf.org/id/draft-ietf-oauth-discovery-08.html) draft document. An authorization client singleton is lazily initialized for use by flyte-cli. # Pyflyte changes (basic auth) Requests an authorization token using a username and password. # Flyte-cli changes (standard auth) Requests an authorization token using the PKCE flow. # Raw client changes Wraps RPC calls to flyteadmin in a retry handler that initiates the appropriate authentication flow defined in the flytekit config in response to `HTTP 401 unauthorized` response codes.

WIP pkce auth flow

d0715bb

wild-endeavor reviewed Oct 22, 2019

View reviewed changes

flytekit/clis/auth/auth.py Outdated Show resolved Hide resolved

katrogan added 3 commits October 22, 2019 17:13

Add discovery

f576129

pass authentication metadata in gRPC calls

c7eba16

add credential type, misc clean-up

1bda5da

matthewphsmith reviewed Nov 6, 2019

View reviewed changes

flytekit/clients/raw.py Outdated Show resolved Hide resolved

wild-endeavor reviewed Nov 6, 2019

View reviewed changes

matthewphsmith reviewed Nov 11, 2019

View reviewed changes

flytekit/clis/auth/auth.py Outdated Show resolved Hide resolved

matthewphsmith reviewed Nov 11, 2019

View reviewed changes

flytekit/configuration/platform.py Show resolved Hide resolved

matthewphsmith reviewed Nov 11, 2019

View reviewed changes

nits

cc2517c

katrogan requested a review from kumare3 as a code owner November 12, 2019 19:16

kumare3 reviewed Nov 12, 2019

View reviewed changes

removing a redirect follow, comments, adding a sample default discove…

2a350c7

…ry endpoint

katrogan commented Nov 13, 2019

View reviewed changes

katrogan and others added 8 commits November 13, 2019 14:48

checkpoint

1cb1620

proper sequence of events

2b9d178

adding config file read, sample config file, and a test that will be …

1cc9999

…deleted, only using for debugging at the moment

pulling and resolving conflicts

89dcb1e

let ur browser handle redirects

1cbcf3a

pull out urlencode separately, doesn't work for python3

6ccb327

fixing python3 import, changing keyring var names

58f97e9

some super pythonic code for handling token refresh

986d49b

katrogan commented Nov 15, 2019

View reviewed changes

katrogan added 4 commits November 15, 2019 16:41

use retry library

ff842e2

customizable rpc error callback fn

2928417

configurable refresh handlers

e50df71

expose set_access_token

41c6c2b

katrogan and others added 4 commits November 27, 2019 10:47

nevermind

bd7324e

one more revert

7004c59

change handling around default home directory config file loading so …

244d032

…that if the host is specified in the user's ~/.flyte/config file, you don't need to specify it in the flyte-cli command

Merge branch 'pkce-auth' of github.com:lyft/flytekit into pkce-auth

48f74fe

matthewphsmith previously requested changes Dec 3, 2019

View reviewed changes

katrogan and others added 11 commits December 4, 2019 14:53

address review comments

067a604

nits

23a14c8

Env var (from pkce-auth pr) option 2 (#64)

a52ba07

making a waterfall for how configuration settings are looked up from the environment variable

remove no longer necessary location backup for credentials secret

7f54611

merge master and resolve conflict

dcd7d34

use the real auth exception class

053e03e

get_discovery_endpoint changes

0dc3186

cleanup keyring code

946a058

Merge branch 'pkce-auth' of github.com:lyft/flytekit into pkce-auth

a940a0c

add expiration handling

d200b55

bump flytekit version

feb403e

jonathanburns reviewed Dec 6, 2019

View reviewed changes

flytekit/clis/sdk_in_container/basic_auth.py Show resolved Hide resolved

jonathanburns reviewed Dec 6, 2019

View reviewed changes

no need to force twice, now that it's been added to the base client

fc56ce6

wild-endeavor previously approved these changes Dec 6, 2019

View reviewed changes

use non-beta release

bd9bbb6

katrogan dismissed wild-endeavor’s stale review via bd9bbb6 December 6, 2019 19:39

wild-endeavor approved these changes Dec 6, 2019

View reviewed changes

wild-endeavor merged commit 439a36d into master Dec 6, 2019

katrogan mentioned this pull request Dec 6, 2019

Implement pkce compliant flow in cli flyteorg/flyte#102

Closed

honnix mentioned this pull request Jul 26, 2023

[Housekeeping] flytekit unnecessarily depends on responses lib flyteorg/flyte#3895

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAuth2 support for flyte-cli and SDK engine #23

OAuth2 support for flyte-cli and SDK engine #23

katrogan commented Oct 11, 2019 •

edited

Loading

wild-endeavor Nov 6, 2019

matthewphsmith Nov 11, 2019

katrogan Nov 11, 2019

wild-endeavor Nov 12, 2019

kumare3 Nov 12, 2019

matthewphsmith Nov 11, 2019

matthewphsmith Nov 11, 2019

wild-endeavor Nov 12, 2019 •

edited

Loading

katrogan Nov 12, 2019

matthewphsmith Nov 11, 2019

katrogan Nov 12, 2019

kumare3 Nov 12, 2019

kumare3 commented Nov 12, 2019

katrogan Nov 13, 2019

katrogan Nov 15, 2019

katrogan Nov 22, 2019

matthewphsmith Dec 3, 2019

wild-endeavor Dec 3, 2019

katrogan Dec 4, 2019

wild-endeavor Dec 4, 2019

matthewphsmith Dec 5, 2019

matthewphsmith Dec 3, 2019

katrogan Dec 4, 2019

jonathanburns Dec 6, 2019

wild-endeavor Dec 6, 2019

jonathanburns Dec 6, 2019

jonathanburns commented Dec 6, 2019


		from flytekit.configuration import common as _config_common

		DISCOVERY_ENDPOINT = _config_common.FlyteStringConfigurationEntry('credentials', 'discovery_endpoint', default=None)

		_utf_8 = 'utf-8'


		class FlyteAuthenticationException(_FlyteException):

OAuth2 support for flyte-cli and SDK engine #23

OAuth2 support for flyte-cli and SDK engine #23

Conversation

katrogan commented Oct 11, 2019 • edited Loading

New authorization code

Pyflyte changes (basic auth)

Flyte-cli changes (standard auth)

Raw client changes

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

wild-endeavor Nov 12, 2019 • edited Loading

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

kumare3 commented Nov 12, 2019

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

jonathanburns commented Dec 6, 2019

katrogan commented Oct 11, 2019 •

edited

Loading

wild-endeavor Nov 12, 2019 •

edited

Loading