From 6c8da712dd25e411c7bba44867ef66dd4e5f9fb4 Mon Sep 17 00:00:00 2001 From: Yee Hing Tong Date: Fri, 13 Dec 2019 14:28:30 -0800 Subject: [PATCH] Add auth enforcement config options (#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 | 4 ++ flyteadmin/pkg/auth/config/config.go | 7 +++ flyteadmin/pkg/auth/handlers.go | 77 +++++++++++++++++----------- flyteadmin/pkg/auth/handlers_test.go | 34 +++++++++++- flyteadmin/pkg/auth/token.go | 19 +++++++ flyteadmin/pkg/config/config.go | 2 +- 6 files changed, 112 insertions(+), 31 deletions(-) diff --git a/flyteadmin/cmd/entrypoints/serve.go b/flyteadmin/cmd/entrypoints/serve.go index 4e39243b9f..d0abe1ab11 100644 --- a/flyteadmin/cmd/entrypoints/serve.go +++ b/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 diff --git a/flyteadmin/pkg/auth/config/config.go b/flyteadmin/pkg/auth/config/config.go index 589fefbef2..dde837a019 100644 --- a/flyteadmin/pkg/auth/config/config.go +++ b/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 { diff --git a/flyteadmin/pkg/auth/handlers.go b/flyteadmin/pkg/auth/handlers.go index 96ad91c024..b021c6b662 100644 --- a/flyteadmin/pkg/auth/handlers.go +++ b/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 diff --git a/flyteadmin/pkg/auth/handlers_test.go b/flyteadmin/pkg/auth/handlers_test.go index 12fe445336..abba3c4985 100644 --- a/flyteadmin/pkg/auth/handlers_test.go +++ b/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") diff --git a/flyteadmin/pkg/auth/token.go b/flyteadmin/pkg/auth/token.go index 9db07f0b1c..6eb7a41891 100644 --- a/flyteadmin/pkg/auth/token.go +++ b/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) +} diff --git a/flyteadmin/pkg/config/config.go b/flyteadmin/pkg/config/config.go index dcbc374ff4..d25640d81d 100644 --- a/flyteadmin/pkg/config/config.go +++ b/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", }, },