Support Google OAuth2 and other OIdC providers (flyteorg#147)

* Move scopes to config Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * missed Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * wip Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Update handlers.go Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * go get propeller at v0.5.12 (flyteorg#146) Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Update to using latest flytepropeller v0.5.13 (flyteorg#148) Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Update propeller to 0.5.14 (flyteorg#149) * Update propeller * cleanup Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Filter executions by user (flyteorg#150) Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Update CI post migration Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Update codecov link Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Publish raw events (flyteorg#151) Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * fix test Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * wip Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Fix token retrieval from cookies Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Fix unit tests and lint Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Move to const Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Revert Auth config Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Revert kube config path Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Use access token when posting to IdP Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Ignore refresh token error Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Fix tests Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Expose openId metadata endpoint and expose scopes in /config endpoint Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * mod tidy Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * rename Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * lint Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * unit tests Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> Co-authored-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com> Co-authored-by: Katrina Rogan <katrogan9@gmail.com> Co-authored-by: tnsetting <38207208+tnsetting@users.noreply.github.com>
eapolinario · Mar 2, 2021 · 1948047 · 1948047
1 parent ee98c70
commit 1948047
Show file tree

Hide file tree

Showing 18 changed files with 257 additions and 173 deletions.
diff --git a/cmd/entrypoints/serve.go b/cmd/entrypoints/serve.go
@@ -153,8 +153,12 @@ func newHTTPServer(ctx context.Context, cfg *config.ServerConfig, authContext in
 		if authContext.GetUserInfoURL() != nil && authContext.GetUserInfoURL().String() != "" {
 			mux.HandleFunc("/me", auth.GetMeEndpointHandler(ctx, authContext))
 		}
+
+		// The metadata endpoint is an RFC-defined constant, but we need a leading / for the handler to pattern match correctly.
+		mux.HandleFunc(fmt.Sprintf("/%s", auth.OIdCMetadataEndpoint), auth.GetOIdCMetadataEndpointRedirectHandler(ctx, authContext))
+
 		// The metadata endpoint is an RFC-defined constant, but we need a leading / for the handler to pattern match correctly.
-		mux.HandleFunc(fmt.Sprintf("/%s", auth.MetadataEndpoint), auth.GetMetadataEndpointRedirectHandler(ctx, authContext))
+		mux.HandleFunc(fmt.Sprintf("/%s", auth.OAuth2MetadataEndpoint), auth.GetOAuth2MetadataEndpointRedirectHandler(ctx, authContext))
 
 		// This option translates HTTP authorization data (cookies) into a gRPC metadata field
 		gwmuxOptions = append(gwmuxOptions, runtime.WithMetadata(auth.GetHTTPRequestCookieToMetadataHandler(authContext)))

diff --git a/flyteadmin_config.yaml b/flyteadmin_config.yaml
@@ -8,10 +8,13 @@ server:
   grpcServerReflection: true
   security:
     secure: false
-    ssl:
-      certificateFile: "/path/to/server.pem"
-      keyFile: "/path/to/server.key"
     useAuth: false
+    allowCors: true
+    allowedOrigins:
+      # Accepting all domains for Sandbox installation
+      - "*"
+    allowedHeaders:
+      - "Content-Type"
     oauth:
       clientId: yourclientid
       clientSecretFile: "/path/to/oauth/secret"
@@ -73,7 +76,7 @@ notifications:
     accountId: "bar"
   emailer:
     subject: "Notice: Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\"."
-    sender:  "flyte-notifications@example.com"
+    sender: "flyte-notifications@example.com"
     body: >
       Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\". View details at
       <a href=\http://example.com/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}>

diff --git a/go.sum b/go.sum
diff --git a/pkg/auth/auth_context.go b/pkg/auth/auth_context.go
@@ -22,15 +22,16 @@ const (
 
 // Please see the comment on the corresponding AuthenticationContext for more information.
 type Context struct {
-	oauth2        *oauth2.Config
-	claims        config.Claims
-	cookieManager interfaces.CookieHandler
-	oidcProvider  *oidc.Provider
-	options       config.OAuthOptions
-	userInfoURL   *url.URL
-	baseURL       *url.URL
-	metadataURL   *url.URL
-	httpClient    *http.Client
+	oauth2            *oauth2.Config
+	claims            config.Claims
+	cookieManager     interfaces.CookieHandler
+	oidcProvider      *oidc.Provider
+	options           config.OAuthOptions
+	userInfoURL       *url.URL
+	baseURL           *url.URL
+	oauth2MetadataURL *url.URL
+	oidcMetadataURL   *url.URL
+	httpClient        *http.Client
 }
 
 func (c Context) OAuth2Config() *oauth2.Config {
@@ -65,8 +66,12 @@ func (c Context) GetBaseURL() *url.URL {
 	return c.baseURL
 }
 
-func (c Context) GetMetadataURL() *url.URL {
-	return c.metadataURL
+func (c Context) GetOAuth2MetadataURL() *url.URL {
+	return c.oauth2MetadataURL
+}
+
+func (c Context) GetOIdCMetadataURL() *url.URL {
+	return c.oidcMetadataURL
 }
 
 const (
@@ -85,30 +90,35 @@ func NewAuthenticationContext(ctx context.Context, options config.OAuthOptions)
 	if err != nil {
 		return Context{}, errors.Wrapf(ErrAuthContext, err, "Error creating OAuth2 library configuration")
 	}
+
 	result.oauth2 = &oauth2Config
 
 	// Construct the cookie manager object.
 	hashKeyBytes, err := ioutil.ReadFile(options.CookieHashKeyFile)
 	if err != nil {
 		return Context{}, errors.Wrapf(ErrConfigFileRead, err, "Could not read hash key file")
 	}
+
 	blockKeyBytes, err := ioutil.ReadFile(options.CookieBlockKeyFile)
 	if err != nil {
 		return Context{}, errors.Wrapf(ErrConfigFileRead, err, "Could not read block key file")
 	}
+
 	cookieManager, err := NewCookieManager(ctx, string(hashKeyBytes), string(blockKeyBytes))
 	if err != nil {
 		logger.Errorf(ctx, "Error creating cookie manager %s", err)
 		return Context{}, errors.Wrapf(ErrAuthContext, err, "Error creating cookie manager")
 	}
+
 	result.cookieManager = cookieManager
 
 	// Construct an oidc Provider, which needs its own http Client.
 	oidcCtx := oidc.ClientContext(ctx, &http.Client{})
 	provider, err := oidc.NewProvider(oidcCtx, options.Claims.Issuer)
 	if err != nil {
-		return Context{}, errors.Wrapf(ErrAuthContext, err, "Error creating oidc provider")
+		return Context{}, errors.Wrapf(ErrAuthContext, err, "Error creating oidc provider w/ issuer [%v]", options.Claims.Issuer)
 	}
+
 	result.oidcProvider = provider
 
 	// TODO: Convert all the URLs in this config to the config.URL type
@@ -120,16 +130,25 @@ func NewAuthenticationContext(ctx context.Context, options config.OAuthOptions)
 		logger.Errorf(ctx, "Error parsing base URL %s", err)
 		return Context{}, errors.Wrapf(ErrAuthContext, err, "Error parsing IDP base URL")
 	}
+
 	logger.Infof(ctx, "Base IDP URL is %s", base)
 	result.baseURL = base
 
-	metadataURL, err := url.Parse(MetadataEndpoint)
+	result.oauth2MetadataURL, err = url.Parse(OAuth2MetadataEndpoint)
 	if err != nil {
-		logger.Errorf(ctx, "Error parsing metadata URL %s", err)
+		logger.Errorf(ctx, "Error parsing oauth2 metadata URL %s", err)
 		return Context{}, errors.Wrapf(ErrAuthContext, err, "Error parsing metadata URL")
 	}
-	logger.Infof(ctx, "Metadata endpoint is %s", metadataURL)
-	result.metadataURL = metadataURL
+
+	logger.Infof(ctx, "Metadata endpoint is %s", result.oauth2MetadataURL)
+
+	result.oidcMetadataURL, err = url.Parse(OIdCMetadataEndpoint)
+	if err != nil {
+		logger.Errorf(ctx, "Error parsing oidc metadata URL %s", err)
+		return Context{}, errors.Wrapf(ErrAuthContext, err, "Error parsing metadata URL")
+	}
+
+	logger.Infof(ctx, "Metadata endpoint is %s", result.oidcMetadataURL)
 
 	// Construct the URL object for the user info endpoint if applicable
 	if options.IdpUserInfoEndpoint != "" {
@@ -158,14 +177,13 @@ func GetOauth2Config(options config.OAuthOptions) (oauth2.Config, error) {
 	if err != nil {
 		return oauth2.Config{}, err
 	}
+
 	secret := strings.TrimSuffix(string(secretBytes), "\n")
 	return oauth2.Config{
 		RedirectURL:  options.CallbackURL,
 		ClientID:     options.ClientID,
 		ClientSecret: secret,
-		// Offline access needs to be specified in order to return a refresh token in the exchange.
-		// TODO: Second parameter is IDP specific - move to config. Also handle case where a refresh token is not allowed
-		Scopes: []string{OidcScope, OfflineAccessType, ProfileScope},
+		Scopes:       options.Scopes,
 		Endpoint: oauth2.Endpoint{
 			AuthURL:  options.AuthorizeURL,
 			TokenURL: options.TokenURL,

diff --git a/pkg/auth/config/config.go b/pkg/auth/config/config.go
@@ -58,6 +58,11 @@ type OAuthOptions struct {
 	// into the realm of authorization rather than authentication.
 	DisableForHTTP bool `json:"disableForHttp"`
 	DisableForGrpc bool `json:"disableForGrpc"`
+
+	// Provides a list of scopes to request from the IDP when authenticating. Default value requests claims that should
+	// be supported by any OIdC server. Refer to https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims for
+	// a complete list. Other providers might support additional scopes that you can define in a config.
+	Scopes []string `json:"scopes"`
 }
 
 type Claims struct {

diff --git a/pkg/auth/constants.go b/pkg/auth/constants.go
@@ -3,15 +3,14 @@ package auth
 // OAuth2 Parameters
 const CsrfFormKey = "state"
 const AuthorizationResponseCodeType = "code"
-const OidcScope = "openid"
-const ProfileScope = "profile"
 const RefreshToken = "refresh_token"
 const DefaultAuthorizationHeader = "authorization"
 const BearerScheme = "Bearer"
 
 // https://tools.ietf.org/html/rfc8414
 // This should be defined without a leading slash. If there is one, the url library's ResolveReference will make it a root path
-const MetadataEndpoint = ".well-known/oauth-authorization-server"
+const OAuth2MetadataEndpoint = ".well-known/oauth-authorization-server"
 
-// IDP specific
-const OfflineAccessType = "offline_access"
+// https://openid.net/specs/openid-connect-discovery-1_0.html
+// This should be defined without a leading slash. If there is one, the url library's ResolveReference will make it a root path
+const OIdCMetadataEndpoint = ".well-known/openid-configuration"
diff --git a/pkg/auth/cookie.go b/pkg/auth/cookie.go
@@ -17,13 +17,18 @@ import (
 
 const (
 	// #nosec
-	accessTokenCookieName = "flyte_jwt"
+	accessTokenCookieName = "flyte_at"
 	// #nosec
-	refreshTokenCookieName = "flyte_refresh"
+	idTokenCookieName = "flyte_idt"
+	// #nosec
+	refreshTokenCookieName = "flyte_rt"
 	// #nosec
 	csrfStateCookieName = "flyte_csrf_state"
 	// #nosec
 	redirectURLCookieName = "flyte_redirect_location"
+
+	// #nosec
+	idTokenExtra = "id_token"
 )
 
 const (
@@ -54,6 +59,33 @@ func NewSecureCookie(cookieName, value string, hashKey, blockKey []byte) (http.C
 	return http.Cookie{}, errors.Wrapf(ErrSecureCookie, err, "Error creating secure cookie")
 }
 
+func retrieveSecureCookie(ctx context.Context, request *http.Request, cookieName string, hashKey, blockKey []byte) (string, error) {
+	cookie, err := request.Cookie(cookieName)
+	if err != nil {
+		logger.Infof(ctx, "Could not detect existing cookie [%v]. Error: %v", cookieName, err)
+		return "", errors.Wrapf(ErrTokenNil, err, "Failure to retrieve cookie [%v]", cookieName)
+	}
+
+	if cookie == nil {
+		logger.Infof(ctx, "Retrieved empty cookie [%v].", cookieName)
+		return "", errors.Errorf(ErrTokenNil, "Retrieved empty cookie [%v]", cookieName)
+	}
+
+	logger.Debugf(ctx, "Existing [%v] cookie found", cookieName)
+	token, err := ReadSecureCookie(ctx, *cookie, hashKey, blockKey)
+	if err != nil {
+		logger.Errorf(ctx, "Error reading existing secure cookie [%v]. Error: %s", cookieName, err)
+		return "", errors.Errorf(ErrTokenNil, "Error reading existing secure cookie [%v]. Error: %s", cookieName, err)
+	}
+
+	if len(token) == 0 {
+		logger.Errorf(ctx, "Read empty token from secure cookie [%v].", cookieName)
+		return "", errors.Errorf(ErrTokenNil, "Read empty token from secure cookie [%v].", cookieName)
+	}
+
+	return token, nil
+}
+
 func ReadSecureCookie(ctx context.Context, cookie http.Cookie, hashKey, blockKey []byte) (string, error) {
 	var s = securecookie.New(hashKey, blockKey)
 	var value string

diff --git a/pkg/auth/cookie_manager.go b/pkg/auth/cookie_manager.go
@@ -20,6 +20,8 @@ const (
 	ErrB64Decoding errors.ErrorCode = "BINARY_DECODING_FAILED"
 	// #nosec
 	ErrTokenNil errors.ErrorCode = "EMPTY_OAUTH_TOKEN"
+	// #nosec
+	ErrNoIDToken errors.ErrorCode = "NO_ID_TOKEN_IN_RESPONSE"
 )
 
 func NewCookieManager(ctx context.Context, hashKeyEncoded, blockKeyEncoded string) (CookieManager, error) {
@@ -29,6 +31,7 @@ func NewCookieManager(ctx context.Context, hashKeyEncoded, blockKeyEncoded strin
 	if err != nil {
 		return CookieManager{}, errors.Wrapf(ErrB64Decoding, err, "Error decoding hash key bytes")
 	}
+
 	blockKey, err := base64.RawStdEncoding.DecodeString(blockKeyEncoded)
 	if err != nil {
 		return CookieManager{}, errors.Wrapf(ErrB64Decoding, err, "Error decoding block key bytes")
@@ -41,32 +44,30 @@ func NewCookieManager(ctx context.Context, hashKeyEncoded, blockKeyEncoded strin
 }
 
 // TODO: Separate refresh token from access token, remove named returns, and use stdlib errors.
-func (c CookieManager) RetrieveTokenValues(ctx context.Context, request *http.Request) (accessToken string,
+// RetrieveTokenValues retrieves id, access and refresh tokens from cookies if they exist. The existence of a refresh token
+// in a cookie is optional and hence failure to find or read that cookie is tolerated. An error is returned in case of failure
+// to retrieve and read either the id or the access tokens.
+func (c CookieManager) RetrieveTokenValues(ctx context.Context, request *http.Request) (idToken, accessToken,
 	refreshToken string, err error) {
 
-	jwtCookie, err := request.Cookie(accessTokenCookieName)
-	if err != nil || jwtCookie == nil {
-		logger.Errorf(ctx, "Could not detect existing access token cookie")
-		return
-	}
-	logger.Debugf(ctx, "Existing JWT cookie found")
-	accessToken, err = ReadSecureCookie(ctx, *jwtCookie, c.hashKey, c.blockKey)
+	idToken, err = retrieveSecureCookie(ctx, request, idTokenCookieName, c.hashKey, c.blockKey)
 	if err != nil {
-		logger.Errorf(ctx, "Error reading existing secure JWT cookie %s", err)
-		return
+		return "", "", "", err
 	}
 
-	refreshCookie, err := request.Cookie(refreshTokenCookieName)
-	if err != nil || refreshCookie == nil {
-		logger.Debugf(ctx, "Could not detect existing access token cookie")
-		return
+	accessToken, err = retrieveSecureCookie(ctx, request, accessTokenCookieName, c.hashKey, c.blockKey)
+	if err != nil {
+		return "", "", "", err
 	}
-	logger.Debugf(ctx, "Existing refresh cookie found")
-	refreshToken, err = ReadSecureCookie(ctx, *refreshCookie, c.hashKey, c.blockKey)
+
+	refreshToken, err = retrieveSecureCookie(ctx, request, refreshTokenCookieName, c.hashKey, c.blockKey)
 	if err != nil {
-		logger.Errorf(ctx, "Error reading existing secure refresh cookie %s", err)
-		return
+		// Refresh tokens are optional. Depending on the auth url (IdP specific) we might or might not receive a refresh
+		// token. In case we do not, we will just have to redirect to IdP whenever access/id tokens expire.
+		logger.Infof(ctx, "Refresh token doesn't exist or failed to read it. Ignoring this error. Error: %v", err)
+		err = nil
 	}
+
 	return
 }
 
@@ -76,22 +77,37 @@ func (c CookieManager) SetTokenCookies(ctx context.Context, writer http.Response
 		return errors.Errorf(ErrTokenNil, "Attempting to set cookies with nil token")
 	}
 
-	jwtCookie, err := NewSecureCookie(accessTokenCookieName, token.AccessToken, c.hashKey, c.blockKey)
+	atCookie, err := NewSecureCookie(accessTokenCookieName, token.AccessToken, c.hashKey, c.blockKey)
 	if err != nil {
-		logger.Errorf(ctx, "Error generating encrypted JWT cookie %s", err)
+		logger.Errorf(ctx, "Error generating encrypted accesstoken cookie %s", err)
 		return err
 	}
-	http.SetCookie(writer, &jwtCookie)
+
+	http.SetCookie(writer, &atCookie)
+
+	if idTokenRaw, converted := token.Extra(idTokenExtra).(string); converted {
+		idCookie, err := NewSecureCookie(idTokenCookieName, idTokenRaw, c.hashKey, c.blockKey)
+		if err != nil {
+			logger.Errorf(ctx, "Error generating encrypted id token cookie %s", err)
+			return err
+		}
+
+		http.SetCookie(writer, &idCookie)
+	} else {
+		logger.Errorf(ctx, "Response does not contain an id_token.")
+		return errors.Errorf(ErrNoIDToken, "Response does not contain an id_token.")
+	}
 
 	// Set the refresh cookie if there is a refresh token
 	if token.RefreshToken != "" {
 		refreshCookie, err := NewSecureCookie(refreshTokenCookieName, token.RefreshToken, c.hashKey, c.blockKey)
 		if err != nil {
-			logger.Errorf(ctx, "Error generating encrypted refresh cookie %s", err)
+			logger.Errorf(ctx, "Error generating encrypted refresh token cookie %s", err)
 			return err
 		}
 		http.SetCookie(writer, &refreshCookie)
 	}
+
 	return nil
 }