Add auth enforcement config options (flyteorg#43)

This is the last bit of auth work on the Admin side of things.  Before this PR, if you present Admin with a bad or expired token, you will get a 401.  However, if you present no token whatsoever, Admin will let the request go through.  This PR begins enforcement.

To help with migration, we are incrementally going to require auth.  We were originally going to use the gRPC endpoint name to decide whether or not to enforce things, but ultimately it made more sense to divide up by protocol - HTTP vs gRPC. Since most programmatic (more problematic) traffic goes through gRPC and only Flyte Console uses HTTP, we can start by just doing HTTP.

In order to do this, the code tags incoming HTTP requests because the token validation code is further downstream.

Also
* Adding the bit of code to look for bearer tokens in http headers as well as cookies to support use-cases where we use the HTTP Admin API.  Note however, that Admin will be deployed with a custom config so that the header you should send is `Flyte-Authorization` not the default `Authorization`, so the complete header would look something like `Flyte-Authorization: Bearer j.w.t`

flyteadmin/cmd/entrypoints/serve.go

@@ -147,6 +147,10 @@ func newHTTPServer(ctx context.Context, cfg *config.ServerConfig, authContext in
 
 		// This option translates HTTP authorization data (cookies) into a gRPC metadata field
 		gwmuxOptions = append(gwmuxOptions, runtime.WithMetadata(auth.GetHTTPRequestCookieToMetadataHandler(authContext)))
+
+		// In an attempt to be able to selectively enforce whether or not authentication is required, we're going to tag
+		// the requests that come from the HTTP gateway. See the enforceHttp/Grpc options for more information.
+		gwmuxOptions = append(gwmuxOptions, runtime.WithMetadata(auth.GetHTTPMetadataTaggingHandler(authContext)))
 	}
 
 	// Create the grpc-gateway server with the options specified

flyteadmin/pkg/auth/config/config.go

@@ -51,6 +51,13 @@ type OAuthOptions struct {
 	// name. Instead, there is a gRPC interceptor, GetAuthenticationCustomMetadataInterceptor, that will translate
 	// incoming metadata headers with this config setting's name, into that standard header
 	GrpcAuthorizationHeader string `json:"grpcAuthorizationHeader"`
+
+	// To help ease migration, it was helpful to be able to only selectively enforce authentication.  The
+	// dimension that made the most sense to cut by at time of writing is HTTP vs gRPC as the web UI mainly used HTTP
+	// and the backend used mostly gRPC.  Cutting by individual endpoints is another option but it possibly falls more
+	// into the realm of authorization rather than authentication.
+	DisableForHTTP bool `json:"disableForHttp"`
+	DisableForGrpc bool `json:"disableForGrpc"`
 }
 
 type Claims struct {

flyteadmin/pkg/auth/handlers.go

@@ -6,7 +6,8 @@ import (
 	"fmt"
 	"net/http"
 
-	grpcauth "github.com/grpc-ecosystem/go-grpc-middleware/auth"
+	"github.com/grpc-ecosystem/go-grpc-middleware/util/metautils"
+
 	"github.com/lyft/flyteadmin/pkg/auth/interfaces"
 	"github.com/lyft/flytestdlib/contextutils"
 	"github.com/lyft/flytestdlib/errors"
@@ -20,6 +21,8 @@ import (
 
 const (
 	RedirectURLParameter                   = "redirect_url"
+	FromHTTPKey                            = "from_http"
+	FromHTTPVal                            = "true"
 	bearerTokenContextKey contextutils.Key = "bearer"
 	PrincipalContextKey   contextutils.Key = "principal"
 )
@@ -149,38 +152,35 @@ func GetAuthenticationCustomMetadataInterceptor(authCtx interfaces.Authenticatio
 	}
 }
 
-// This function will only look for a token from the request metadata, verify it, and extract the user email if valid.
-// Unless there is an error, it will not return an unauthorized status. That is up to subsequent functions to decide,
-// based on configuration.  We don't want to require authentication for all endpoints.
+// This is the function that chooses to enforce or not enforce authentication. It will attempt to get the token
+// from the incoming context, validate it, and decide whether or not to let the request through.
 func GetAuthenticationInterceptor(authContext interfaces.AuthenticationContext) func(context.Context) (context.Context, error) {
 	return func(ctx context.Context) (context.Context, error) {
 		logger.Debugf(ctx, "Running authentication gRPC interceptor")
-		tokenStr, err := grpcauth.AuthFromMD(ctx, BearerScheme)
-		if err != nil {
-			logger.Debugf(ctx, "Could not retrieve bearer token from metadata %v", err)
-			return ctx, nil
-		}
 
-		// Currently auth is optional...
-		if tokenStr == "" {
-			logger.Debugf(ctx, "Bearer token is empty, skipping parsing")
-			return ctx, nil
-		}
+		fromHTTP := metautils.ExtractIncoming(ctx).Get(FromHTTPKey)
+		isFromHTTP := fromHTTP == FromHTTPVal
 
-		// ...however, if there _is_ a bearer token, but there are additional errors downstream, then we return an
-		// authentication error.
-		token, err := ParseAndValidate(ctx, authContext.Claims(), tokenStr, authContext.OidcProvider())
-		if err != nil {
-			return ctx, status.Errorf(codes.Unauthenticated, "could not parse token string into object: %s %s", tokenStr, err)
+		token, err := GetAndValidateTokenObjectFromContext(ctx, authContext.Claims(), authContext.OidcProvider())
+
+		// Only enforcement logic is present. The default case is to let things through.
+		if (isFromHTTP && !authContext.Options().DisableForHTTP) ||
+			(!isFromHTTP && !authContext.Options().DisableForGrpc) {
+			if err != nil {
+				return ctx, status.Errorf(codes.Unauthenticated, "token parse error %s", err)
+			}
+			if token == nil {
+				return ctx, status.Errorf(codes.Unauthenticated, "Token was nil after parsing")
+			} else if token.Subject == "" {
+				return ctx, status.Errorf(codes.Unauthenticated, "no email or empty email found")
+			}
 		}
-		if token == nil {
-			return ctx, status.Errorf(codes.Unauthenticated, "Token was nil after parsing %s", tokenStr)
-		} else if token.Subject == "" {
-			return ctx, status.Errorf(codes.Unauthenticated, "no email or empty email found")
-		} else {
-			newCtx := WithUserEmail(context.WithValue(ctx, bearerTokenContextKey, tokenStr), token.Subject)
+
+		if token != nil {
+			newCtx := WithUserEmail(context.WithValue(ctx, bearerTokenContextKey, token), token.Subject)
 			return newCtx, nil
 		}
+		return ctx, nil
 	}
 }
 
@@ -195,10 +195,22 @@ func WithUserEmail(ctx context.Context, email string) context.Context {
 // attached to the request, from which the token is extracted later for verification.
 func GetHTTPRequestCookieToMetadataHandler(authContext interfaces.AuthenticationContext) HTTPRequestToMetadataAnnotator {
 	return func(ctx context.Context, request *http.Request) metadata.MD {
-		// TODO: Add read from Authorization header first, using the custom header if necessary.
 		// TODO: Improve error handling
 		accessToken, _, _ := authContext.CookieManager().RetrieveTokenValues(ctx, request)
 		if accessToken == "" {
+
+			// If no token was found in the cookies, look for an authorization header, starting with a potentially
+			// custom header set in the Config object
+			if authContext.Options().HTTPAuthorizationHeader != "" {
+				header := authContext.Options().HTTPAuthorizationHeader
+				// TODO: There may be a potential issue here when running behind a service mesh that uses the default Authorization
+				//       header. The grpc-gateway code will automatically translate the 'Authorization' header into the appropriate
+				//       metadata object so if two different tokens are presented, one with the default name and one with the
+				//       custom name, AuthFromMD will find the wrong one.
+				return metadata.MD{
+					DefaultAuthorizationHeader: []string{request.Header.Get(header)},
+				}
+			}
 			logger.Infof(ctx, "Could not find access token cookie while requesting %s", request.RequestURI)
 			return nil
 		}
@@ -208,6 +220,16 @@ func GetHTTPRequestCookieToMetadataHandler(authContext interfaces.Authentication
 	}
 }
 
+// Intercepts the incoming HTTP requests and marks it as such so that the downstream code can use it to enforce auth.
+// See the enforceHTTP/Grpc options for more information.
+func GetHTTPMetadataTaggingHandler(authContext interfaces.AuthenticationContext) HTTPRequestToMetadataAnnotator {
+	return func(ctx context.Context, request *http.Request) metadata.MD {
+		return metadata.MD{
+			FromHTTPKey: []string{FromHTTPVal},
+		}
+	}
+}
+
 // TODO: Add this to the Admin service IDL in Flyte IDL so that this can be exposed from gRPC as well.
 // This returns a handler that will retrieve user info, from the OAuth2 authorization server.
 // See the OpenID Connect spec at https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse for more information.
@@ -263,6 +285,3 @@ func GetLogoutEndpointHandler(ctx context.Context, authCtx interfaces.Authentica
 		}
 	}
 }
-
-// These are here for CORS handling. Actual serving of the OPTIONS request will be done by the gorilla/handlers package
-type CorsHandlerDecorator func(http.Handler) http.Handler

flyteadmin/pkg/auth/handlers_test.go

@@ -7,6 +7,8 @@ import (
 	"net/url"
 	"strings"
 
+	"github.com/lyft/flyteadmin/pkg/auth/config"
+
 	"github.com/lyft/flyteadmin/pkg/auth/interfaces/mocks"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/oauth2"
@@ -37,7 +39,7 @@ func TestGetLoginHandler(t *testing.T) {
 	assert.True(t, strings.Contains(w.Header().Get("Set-Cookie"), "flyte_csrf_state="))
 }
 
-func TestGetHttpRequestCookieToMetadataHandler(t *testing.T) {
+func TestGetHTTPRequestCookieToMetadataHandler(t *testing.T) {
 	ctx := context.Background()
 	// These were generated for unit testing only.
 	hashKeyEncoded := "wG4pE1ccdw/pHZ2ml8wrD5VJkOtLPmBpWbKHmezWXktGaFbRoAhXidWs8OpbA3y7N8vyZhz1B1E37+tShWC7gA" //nolint:goconst
@@ -55,6 +57,36 @@ func TestGetHttpRequestCookieToMetadataHandler(t *testing.T) {
 	assert.Equal(t, "Bearer a.b.c", handler(ctx, req)["authorization"][0])
 }
 
+func TestGetHTTPMetadataTaggingHandler(t *testing.T) {
+	ctx := context.Background()
+	mockAuthCtx := mocks.AuthenticationContext{}
+	annotator := GetHTTPMetadataTaggingHandler(&mockAuthCtx)
+	request, err := http.NewRequest("GET", "/api", nil)
+	assert.NoError(t, err)
+	md := annotator(ctx, request)
+	assert.Equal(t, FromHTTPVal, md.Get(FromHTTPKey)[0])
+}
+
+func TestGetHTTPRequestCookieToMetadataHandler_CustomHeader(t *testing.T) {
+	ctx := context.Background()
+	// These were generated for unit testing only.
+	hashKeyEncoded := "wG4pE1ccdw/pHZ2ml8wrD5VJkOtLPmBpWbKHmezWXktGaFbRoAhXidWs8OpbA3y7N8vyZhz1B1E37+tShWC7gA" //nolint:goconst
+	blockKeyEncoded := "afyABVgGOvWJFxVyOvCWCupoTn6BkNl4SOHmahho16Q"                                           //nolint:goconst
+	cookieManager, err := NewCookieManager(ctx, hashKeyEncoded, blockKeyEncoded)
+	assert.NoError(t, err)
+	mockAuthCtx := mocks.AuthenticationContext{}
+	mockAuthCtx.On("CookieManager").Return(&cookieManager)
+	mockConfig := config.OAuthOptions{
+		HTTPAuthorizationHeader: "Custom-Header",
+	}
+	mockAuthCtx.On("Options").Return(mockConfig)
+	handler := GetHTTPRequestCookieToMetadataHandler(&mockAuthCtx)
+	req, err := http.NewRequest("GET", "/api/v1/projects", nil)
+	assert.NoError(t, err)
+	req.Header.Set("Custom-Header", "Bearer a.b.c")
+	assert.Equal(t, "Bearer a.b.c", handler(ctx, req)["authorization"][0])
+}
+
 func TestGetMetadataEndpointRedirectHandler(t *testing.T) {
 	ctx := context.Background()
 	baseURL, err := url.Parse("http://www.google.com")

flyteadmin/pkg/auth/token.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/coreos/go-oidc"
+	grpcauth "github.com/grpc-ecosystem/go-grpc-middleware/auth"
 	"github.com/lyft/flyteadmin/pkg/auth/config"
 	"github.com/lyft/flytestdlib/errors"
 	"github.com/lyft/flytestdlib/logger"
@@ -54,3 +55,21 @@ func ParseAndValidate(ctx context.Context, claims config.Claims, accessToken str
 	}
 	return idToken, nil
 }
+
+// This function attempts to extract a token from the context, and will then call the validation function, passing up
+// any errors.
+func GetAndValidateTokenObjectFromContext(ctx context.Context, claims config.Claims,
+	provider *oidc.Provider) (*oidc.IDToken, error) {
+
+	tokenStr, err := grpcauth.AuthFromMD(ctx, BearerScheme)
+	if err != nil {
+		logger.Debugf(ctx, "Could not retrieve bearer token from metadata %v", err)
+		return nil, errors.Wrapf(ErrJwtValidation, err, "Could not retrieve bearer token from metadata")
+	}
+	if tokenStr == "" {
+		logger.Debugf(ctx, "Found Bearer scheme but token was blank")
+		return nil, errors.Errorf(ErrJwtValidation, "Bearer token is blank")
+	}
+
+	return ParseAndValidate(ctx, claims, tokenStr, provider)
+}

flyteadmin/pkg/config/config.go

@@ -48,7 +48,7 @@ var defaultServerConfig = &ServerConfig{
 	Security: ServerSecurityOptions{
 		Oauth: config2.OAuthOptions{
 			// Please see the comments in this struct's definition for more information
-			HTTPAuthorizationHeader: "placeholder",
+			HTTPAuthorizationHeader: "flyte-authorization",
 			GrpcAuthorizationHeader: "flyte-authorization",
 		},
 	},