[Breaking] OAuth2 Authorization Server implementation, Separate OpenID and OAuth2 configs, OAuth2 Metadata over gRPC #minor

[EngHabu]

Signed-off-by: Haytham Abuelfutuh haytham@afutuh.com

TL;DR

This PR completes the implementation of OAuth2 and OpenID Connected started in Admin. The notable changes are:

1. Implement OAuth2 Authorization Server so Admin can issue OAuth2 for PKCE and Client_Credentials flows.
2. Separate OpenID Connect and OAuth2 configs to make it configurable to work with pure OpenID Connect services (e.g. Google Idp) as well as more advanced ones (e.g. KeyCloak and Okta).
3. Breaking: Parts of the prior Auth config section have been moved up into a separate config section.
4. Refactoring of the auth package to make it standalone and reusable in other services in flyte.
5. Added a command line to issue and store secrets to make it easier to turn on Auth in basic scenarios.
6. Overhaul auth config to simplify required fields for enabling auth.

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Tracking Issue

flyteorg/flyte#925

[pmahindrakar-oss]

The else can be eliminated and L30 can be removed from the if block.

[pmahindrakar-oss]

I am assuming this would be fed in to the app through a secrets engine and not be in the code

[EngHabu]

to flytepropeller for example? yes, I'm going to add it to the sandbox deployment so that flytepropeller has it already deployed... and then in the guide we should tell people how to change it...

[pmahindrakar-oss]

Needs offline scope for refresh token to work

[pmahindrakar-oss]

There is one refresh token issue which we spoke about where the expiry is incorrect.
This is log from the flytectl app where it refreshes the token but gets incorrect expiry for new one.

got a response from the refresh grant for old expiry 2021-04-09 18:39:35.990869 +0530 IST with new expiry 0001-01-01 00:00:00 +0000 UTC

Other than that i see my changes working from flytectl.

[kumare3]

@EngHabu something seems off here, this is a huge PR, Does every open source product have to implement this? Should this be a separate service?

[tnsetting]

One question here: If we have a none flyteconsole web app which wants to access flyteadmin, does it require still require a cookie in the header, or we can pass the id/access token in authorization from web app directly which is similar to the grpc one?

[EngHabu]

It can pass access tokens it acquires through OAuth2 2 or 3 legged flows....

On Mon, Apr 19, 2021 at 4:37 AM tnsetting ***@***.***> wrote: One question here: If we have a none flyteconsole web app which wants to access flyteadmin, does it require still require a cookie in the header, or we can pass the id/access token in authorization from web app directly which is similar to the grpc one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#168 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGUF4SCHSXWFIRUPXMK23TJQIX7ANCNFSM4ZYKG5RA> .

-- Thanks, Haytham Abuelfutuh.

[tnsetting]

It can pass access tokens it acquires through OAuth2 2 or 3 legged flows....
On Mon, Apr 19, 2021 at 4:37 AM tnsetting @.***> wrote: One question here: If we have a none flyteconsole web app which wants to access flyteadmin, does it require still require a cookie in the header, or we can pass the id/access token in authorization from web app directly which is similar to the grpc one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#168 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGUF4SCHSXWFIRUPXMK23TJQIX7ANCNFSM4ZYKG5RA .
-- Thanks, Haytham Abuelfutuh.

@EngHabu Thanks for your reply. I just check a bit about the ID token and access token. In our case we want to use the ID token since the ID token contains user information and we need this information. The other issue with the google access token is that google access token only has two parts and does not comply with JWT token format.

[tnsetting]

@EngHabu Also we want to try this PR out, but is it possible to provide a sample configuration for GCP so we can try it as the config section for auth has been changed a lot? Thanks.

[EngHabu]

@tnsetting, please checkout the new docs here With an example for Google IdP

[tnsetting]

In case of using google as the idp, the link https://accounts.google.com/.well-known/oauth-authorization-server does not exist so the getJwksForIssuer is failing when configured the baseUrl as https://accounts.google.com. Can we fall back to use OIdCMetadataEndpoint when this is failing? So this link does exist https://accounts.google.com/.well-known/openid-configuration

[EngHabu]

Google can't be used as the external OAuth2 Authorization Server... their server only protects other Google Endpoints. You can setup/use GCP Identity Platform instead (I do not have instructions for how to do that)...
If you follow the guide here, it takes you through setting up Google Idp just for OpenID Connect while continuing to use FlyteAdmin's own certs to act as the OAuth2 Authorization Server

[tnsetting]

We actually need to use Google as authentication server because of gcloud service accounts. What want to do for authentication is just to verify who is actually who based on ID token. Regarding authorisation, when we know what the user it is we can fetch custom roles from google (it is possible to add custom role) and implement a very thin layer of access control. In this way we don't need have a separate service to store access policies.

[EngHabu]

That's exactly what the guide takes you through... setting up OpenID Connect with Google IdP... have you tried that?
Let's chat over slack (@haytham) and I would be happy to help you debug and get it up and running

[tnsetting]

Thanks. I managed to get it running but it required me to make some code changes (hack). I will create a PR for these changes.

[tnsetting]

Suggested change

	if claims.Audience[0] != expectedAudience {
	if !strings.Contains(expectedAudience, claims.Audience[0]) {

It looks the code will always prepend http/https to the url but in our case the audience[0] does not have that prefix

[EngHabu]

What do you use as an Authorization Server? Okta? KeyCloak? if so, you can configure them to issue tokens with a specific audience...

[codecov]

Codecov Report

Merging #168 (03fde57) into master (63b7676) will decrease coverage by 2.14%.
The diff coverage is 34.81%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   63.93%   61.79%   -2.15%     
==========================================
  Files         105      120      +15     
  Lines        7353     8087     +734     
==========================================
+ Hits         4701     4997     +296     
- Misses       2072     2463     +391     
- Partials      580      627      +47     

Flag	Coverage Δ
unittests	61.79% <34.81%> (-2.15%)	⬇️

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

Impacted Files	Coverage Δ
auth/auth_context.go	0.00% <0.00%> (ø)
auth/config/authorizationservertype_enumer.go	0.00% <0.00%> (ø)
auth/config/third_party_config.go	0.00% <0.00%> (ø)
auth/identity_context.go	0.00% <0.00%> (ø)
auth/init_secrets.go	0.00% <0.00%> (ø)
auth/token.go	0.00% <0.00%> (ø)
auth/user_info_provider.go	0.00% <0.00%> (ø)
pkg/config/config.go	14.28% <0.00%> (-2.39%)	⬇️
auth/cookie_manager.go	48.86% <11.11%> (ø)
auth/handlers.go	15.16% <15.16%> (ø)
... and 36 more

---

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 63b7676...03fde57. Read the comment docs.