From 283ead386168dcd62dacee56893299407da283ba Mon Sep 17 00:00:00 2001 From: Francesco Ilario <filario@redhat.com> Date: Thu, 26 Sep 2024 18:42:12 +0200 Subject: [PATCH] feat: add Public-Viewer support (#443) This commit adds the notion of Public-Viewer to the Proxy as described in [JIRA ASC-532](https://issues.redhat.com/browse/ASC-532). --------- Signed-off-by: Francesco Ilario <filario@redhat.com> Co-authored-by: Alexey Kazakov <alkazako@redhat.com> Co-authored-by: Francisc Munteanu <fmuntean@redhat.com> --- pkg/proxy/proxy.go | 244 +++++++++++++++-- pkg/proxy/proxy_community_test.go | 422 ++++++++++++++++++++++++++++++ pkg/proxy/proxy_test.go | 209 +++++++++++---- test/fake/informer.go | 16 ++ 4 files changed, 816 insertions(+), 75 deletions(-) create mode 100644 pkg/proxy/proxy_community_test.go diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 2b7e469b..08ded564 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -26,6 +26,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy/access" "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" + "github.com/codeready-toolchain/registration-service/pkg/signup" commoncluster "github.com/codeready-toolchain/toolchain-common/pkg/cluster" "github.com/labstack/echo/v4" "github.com/labstack/echo/v4/middleware" @@ -119,6 +120,7 @@ func (p *Proxy) StartProxy(port string) *http.Server { } }, p.ensureUserIsNotBanned(), + p.addPublicViewerContext(), ) // middleware after routing @@ -142,6 +144,7 @@ func (p *Proxy) StartProxy(port string) *http.Server { // Space lister routes wg.GET("/:workspace", handlers.HandleSpaceGetRequest(p.spaceLister, p.getMembersFunc)) wg.GET("", handlers.HandleSpaceListRequest(p.spaceLister)) + router.GET(proxyHealthEndpoint, p.health) // SSO routes. Used by web login (oc login -w). // Here is the expected flow for the "oc login -w" command: @@ -269,6 +272,7 @@ func (p *Proxy) health(ctx echo.Context) error { } func (p *Proxy) processRequest(ctx echo.Context) (string, *access.ClusterAccess, error) { + // retrieve required information from the HTTP request userID, _ := ctx.Get(context.SubKey).(string) username, _ := ctx.Get(context.UsernameKey).(string) proxyPluginName, workspaceName, err := getWorkspaceContext(ctx.Request()) @@ -276,40 +280,203 @@ func (p *Proxy) processRequest(ctx echo.Context) (string, *access.ClusterAccess, return "", nil, crterrors.NewBadRequest("unable to get workspace context", err.Error()) } - ctx.Set(context.WorkspaceKey, workspaceName) // set workspace context for logging - cluster, err := p.app.MemberClusterService().GetClusterAccess(userID, username, workspaceName, proxyPluginName, false) - if err != nil { - return "", nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) - } + // set workspace context for logging + ctx.Set(context.WorkspaceKey, workspaceName) - // before proxying the request, verify that the user has a spacebinding for the workspace and that the namespace (if any) belongs to the workspace - var workspaces []toolchainv1alpha1.Workspace - if workspaceName != "" { - // when a workspace name was provided - // validate that the user has access to the workspace by getting all spacebindings recursively, starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. - workspace, err := handlers.GetUserWorkspace(ctx, p.spaceLister, workspaceName) + // if the target workspace is NOT explicitly declared in the HTTP request, + // process the request against the user's home workspace + if workspaceName == "" { + cluster, err := p.processHomeWorkspaceRequest(ctx, userID, username, proxyPluginName) if err != nil { - return "", nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) - } - if workspace == nil { - // not found - return "", nil, crterrors.NewForbiddenError("invalid workspace request", fmt.Sprintf("access to workspace '%s' is forbidden", workspaceName)) - } - // workspace was found means we can forward the request - workspaces = []toolchainv1alpha1.Workspace{*workspace} - } else { - // list all workspaces - workspaces, err = handlers.ListUserWorkspaces(ctx, p.spaceLister) - if err != nil { - return "", nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) + return "", nil, err } + return proxyPluginName, cluster, nil + } + + // if the target workspace is explicitly declared in the HTTP request, + // process the request against the declared workspace + cluster, err := p.processWorkspaceRequest(ctx, userID, username, workspaceName, proxyPluginName) + if err != nil { + return "", nil, err + } + return proxyPluginName, cluster, nil +} + +// processHomeWorkspaceRequest process an HTTP Request targeting the user's home workspace. +func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, proxyPluginName string) (*access.ClusterAccess, error) { + // retrieves the ClusterAccess for the user and their home workspace + cluster, err := p.app.MemberClusterService().GetClusterAccess(userID, username, "", proxyPluginName, false) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) } + + // list all workspaces the user has access to + workspaces, err := handlers.ListUserWorkspaces(ctx, p.spaceLister) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) + } + + // check whether the user has access to the home workspace + // and whether the requestedNamespace -if any- exists in the workspace. requestedNamespace := namespaceFromCtx(ctx) - if err := validateWorkspaceRequest(workspaceName, requestedNamespace, workspaces); err != nil { - return "", nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) + if err := validateWorkspaceRequest("", requestedNamespace, workspaces...); err != nil { + return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) } - return proxyPluginName, cluster, nil + // return the cluster access + return cluster, nil +} + +// processWorkspaceRequest process an HTTP Request targeting a specific workspace. +func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, workspaceName, proxyPluginName string) (*access.ClusterAccess, error) { + // check that the user is provisioned and the space exists. + // if the PublicViewer support is enabled, user check is skipped. + if err := p.checkUserIsProvisionedAndSpaceExists(ctx, userID, username, workspaceName); err != nil { + return nil, err + } + + // retrieve the requested Workspace with SpaceBindings + workspace, err := p.getUserWorkspaceWithBindings(ctx, workspaceName) + if err != nil { + return nil, err + } + + // check whether the user has access to the workspace + // and whether the requestedNamespace -if any- exists in the workspace. + requestedNamespace := namespaceFromCtx(ctx) + if err := validateWorkspaceRequest(workspaceName, requestedNamespace, *workspace); err != nil { + return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) + } + + // retrieve the ClusterAccess for the user and the target workspace + return p.getClusterAccess(ctx, userID, username, proxyPluginName, workspace) +} + +// checkUserIsProvisionedAndSpaceExists checks that the user is provisioned and the Space exists. +// If the PublicViewer support is enabled, User check is skipped. +func (p *Proxy) checkUserIsProvisionedAndSpaceExists(ctx echo.Context, userID, username, workspaceName string) error { + if err := p.checkUserIsProvisioned(ctx, userID, username); err != nil { + return crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) + } + if err := p.checkSpaceExists(workspaceName); err != nil { + return crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) + } + return nil +} + +// checkSpaceExists checks whether the Space exists. +func (p *Proxy) checkSpaceExists(workspaceName string) error { + if _, err := p.app.InformerService().GetSpace(workspaceName); err != nil { + // log the actual error but do not return it so that it doesn't reveal information about a space that may not belong to the requestor + log.Errorf(nil, err, "requested space '%s' does not exist", workspaceName) + return fmt.Errorf("access to workspace '%s' is forbidden", workspaceName) + } + return nil +} + +// checkUserIsProvisioned checks whether the user is Approved, if they are not an error is returned. +// If public-viewer is enabled, user validation is skipped. +func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string) error { + // skip if public-viewer is enabled: read-only operations on community workspaces are always permitted. + if context.IsPublicViewerEnabled(ctx) { + return nil + } + + // retrieve the UserSignup for the requesting user. + // + // UserSignup complete status is not checked, since it might cause the proxy blocking the request + // and returning an error when quick transitions from ready to provisioning are happening. + userSignup, err := p.app.SignupService().GetSignupFromInformer(nil, userID, username, false) + if err != nil { + return err + } + + // if the UserSignup is nil or has NOT the CompliantUsername set, + // it means that MUR was NOT created and useraccount is NOT provisioned yet + if userSignup == nil || userSignup.CompliantUsername == "" { + cause := errs.New("user is not provisioned (yet)") + log.Error(nil, cause, fmt.Sprintf("signup object: %+v", userSignup)) + return cause + } + return nil +} + +// getClusterAccess retrieves the access to the cluster hosting the requested workspace, +// if the user has access to it. +// Access can be either direct (a SpaceBinding linking the user to the workspace exists) +// or community (a SpaceBinding linking the PublicViewer user to the workspace exists). +func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { + // retrieve cluster access as requesting user or PublicViewer + cluster, err := p.getClusterAccessAsUserOrPublicViewer(ctx, userID, username, proxyPluginName, workspace) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) + } + return cluster, nil +} + +// getClusterAccessAsUserOrPublicViewer if the requesting user exists and has direct access to the workspace, +// this function returns the ClusterAccess impersonating the requesting user. +// If PublicViewer support is enabled and PublicViewer user has access to the workspace, +// this function returns the ClusterAccess impersonating the PublicViewer user. +// If requesting user does not exists and PublicViewer is disabled or does not have access to the workspace, +// this function returns an error. +func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { + // retrieve the requesting user's UserSignup + userSignup, err := p.app.SignupService().GetSignupFromInformer(nil, userID, username, false) + if err != nil { + log.Error(nil, err, fmt.Sprintf("error retrieving user signup for userID '%s' and username '%s'", userID, username)) + return nil, crterrors.NewInternalError(errs.New("unable to get user info"), "error retrieving user") + } + + // proceed as PublicViewer if the feature is enabled and userSignup is nil + publicViewerEnabled := context.IsPublicViewerEnabled(ctx) + if publicViewerEnabled && !userHasDirectAccess(userSignup, workspace) { + return p.app.MemberClusterService().GetClusterAccess( + toolchainv1alpha1.KubesawAuthenticatedUsername, + toolchainv1alpha1.KubesawAuthenticatedUsername, + workspace.Name, + proxyPluginName, + publicViewerEnabled) + } + + // otherwise retrieve the ClusterAccess for the cluster hosting the workspace and the given user. + return p.app.MemberClusterService().GetClusterAccess(userID, username, workspace.Name, proxyPluginName, publicViewerEnabled) +} + +// userHasDirectAccess checks if an UserSignup has access to a workspace. +// Workspace's bindings are obtained from its `status.bindings` property. +func userHasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspace) bool { + if signup == nil { + return false + } + + return userHasBinding(signup.CompliantUsername, workspace) +} + +func userHasBinding(username string, workspace *toolchainv1alpha1.Workspace) bool { + for _, b := range workspace.Status.Bindings { + if b.MasterUserRecord == username { + return true + } + } + return false + +} + +// getUserWorkspaceWithBindings retrieves the workspace with the SpaceBindings if the requesting user has access to it. +// User access to the Workspace is checked by getting all spacebindings recursively, +// starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. +func (p *Proxy) getUserWorkspaceWithBindings(ctx echo.Context, workspaceName string) (*toolchainv1alpha1.Workspace, error) { + workspace, err := handlers.GetUserWorkspaceWithBindings(ctx, p.spaceLister, workspaceName, p.getMembersFunc) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) + } + if workspace == nil { + // not found + return nil, crterrors.NewForbiddenError("invalid workspace request", fmt.Sprintf("access to workspace '%s' is forbidden", workspaceName)) + } + // workspace was found means we can forward the request + return workspace, nil } func (p *Proxy) handleRequestAndRedirect(ctx echo.Context) error { @@ -409,6 +576,18 @@ func (p *Proxy) addUserContext() echo.MiddlewareFunc { } } +// addPublicViewerContext updates echo.Context with the configuration's PublicViewerEnabled value. +func (p *Proxy) addPublicViewerContext() echo.MiddlewareFunc { + return func(next echo.HandlerFunc) echo.HandlerFunc { + return func(ctx echo.Context) error { + publicViewerEnabled := configuration.GetRegistrationServiceConfig().PublicViewerEnabled() + ctx.Set(context.PublicViewerEnabled, publicViewerEnabled) + + return next(ctx) + } + } +} + // ensureUserIsNotBanned rejects the request if the user is banned. // This Middleware requires the context to contain the email of the user, // so it needs to be executed after the `addUserContext` Middleware. @@ -503,11 +682,17 @@ func extractUserToken(req *http.Request) (string, error) { func (p *Proxy) newReverseProxy(ctx echo.Context, target *access.ClusterAccess, isPlugin bool) *httputil.ReverseProxy { req := ctx.Request() targetQuery := target.APIURL().RawQuery + username, _ := ctx.Get(context.UsernameKey).(string) + // set username in context for logging purposes + ctx.Set(context.ImpersonateUser, target.Username()) + director := func(req *http.Request) { origin := req.URL.String() req.URL.Scheme = target.APIURL().Scheme req.URL.Host = target.APIURL().Host req.URL.Path = singleJoiningSlash(target.APIURL().Path, req.URL.Path) + req.Header.Set("X-SSO-User", username) + if isPlugin { // for non k8s clients testing, like vanilla http clients accessing plugin proxy flows, testing has proven that the request // host needs to be updated in addition to the URL in order to have the reverse proxy contact the openshift @@ -671,7 +856,10 @@ func replaceTokenInWebsocketRequest(req *http.Request, newToken string) { req.Header.Set(ph, strings.Join(protocols, ",")) } -func validateWorkspaceRequest(requestedWorkspace, requestedNamespace string, workspaces []toolchainv1alpha1.Workspace) error { +// validateWorkspaceRequest checks whether the requested workspace is in the list of workspaces the user has visibility on (retrieved via the spaceLister). +// If `requestedWorkspace` is zero, this function looks for the home workspace (the one with `status.Type` set to `home`). +// If `requestedNamespace` is NOT zero, this function checks if the namespace exists in the workspace. +func validateWorkspaceRequest(requestedWorkspace, requestedNamespace string, workspaces ...toolchainv1alpha1.Workspace) error { // check workspace access isHomeWSRequested := requestedWorkspace == "" diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go new file mode 100644 index 00000000..5274bd75 --- /dev/null +++ b/pkg/proxy/proxy_community_test.go @@ -0,0 +1,422 @@ +package proxy + +import ( + "fmt" + "net/http" + "net/http/httptest" + "time" + + appservice "github.com/codeready-toolchain/registration-service/pkg/application/service" + "github.com/codeready-toolchain/registration-service/pkg/auth" + infservice "github.com/codeready-toolchain/registration-service/pkg/informers/service" + "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" + "github.com/codeready-toolchain/registration-service/pkg/signup" + "github.com/codeready-toolchain/registration-service/test/fake" + commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" + authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" + "github.com/google/uuid" + "go.uber.org/atomic" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func (s *TestProxySuite) TestProxyCommunityEnabled() { + // given + + port := "30456" + + env := s.DefaultConfig().Environment() + defer s.SetConfig(testconfig.RegistrationService(). + Environment(env)) + s.SetConfig(testconfig.RegistrationService(). + Environment(string(testconfig.E2E))) // We use e2e-test environment just to be able to re-use token generation + _, err := auth.InitializeDefaultTokenParser() + require.NoError(s.T(), err) + + for _, environment := range []testconfig.EnvName{testconfig.E2E, testconfig.Dev, testconfig.Prod} { + s.Run("for environment "+string(environment), func() { + // spin up proxy + s.SetConfig( + testconfig.RegistrationService().Environment(string(environment)), + testconfig.PublicViewerConfig(true), + ) + fakeApp := &fake.ProxyFakeApp{} + p, server := s.spinUpProxy(fakeApp, port) + defer func() { + _ = server.Close() + }() + + // wait for proxy to be alive + s.Run("is alive", func() { + s.waitForProxyToBeAlive(port) + }) + s.Run("health check ok", func() { + s.checkProxyIsHealthy(port) + }) + + // run community tests + s.checkProxyCommunityOK(fakeApp, p, port) + }) + } +} + +func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Proxy, port string) { + podsRequestURL := func(workspace string) string { + return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/pods", port, workspace) + } + + podsInNamespaceRequestURL := func(workspace string, namespace string) string { + return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/namespaces/%s/pods", port, workspace, namespace) + } + + s.Run("successfully proxy", func() { + // user with public workspace + smith := uuid.New() + // user with private workspace + alice := uuid.New() + // unsigned user + bob := uuid.New() + // not ready + john := uuid.New() + // banned user + eve, eveEmail := uuid.New(), "eve@somecorp.com" + + // Start the member-2 API Server + httpTestServerResponse := "my response" + testServer := httptest.NewServer(nil) + defer testServer.Close() + + // initialize SignupService + signupService := fake.NewSignupService( + fake.Signup(smith.String(), &signup.Signup{ + Name: "smith", + APIEndpoint: testServer.URL, + ClusterName: "member-2", + CompliantUsername: "smith", + Username: "smith@", + Status: signup.Status{ + Ready: true, + }, + }), + fake.Signup(alice.String(), &signup.Signup{ + Name: "alice", + APIEndpoint: testServer.URL, + ClusterName: "member-2", + CompliantUsername: "alice", + Username: "alice@", + Status: signup.Status{ + Ready: true, + }, + }), + fake.Signup(john.String(), &signup.Signup{ + Name: "john", + CompliantUsername: "john", + Username: "john@", + Status: signup.Status{ + Ready: false, + }, + }), + fake.Signup(eve.String(), &signup.Signup{ + Name: "eve", + CompliantUsername: "eve", + Username: "eve@", + Status: signup.Status{ + Ready: false, + Reason: toolchainv1alpha1.UserSignupUserBannedReason, + }, + }), + ) + + // init fakeClient + cli := commontest.NewFakeClient(s.T(), + fake.NewSpace("smith-community", "member-2", "smith"), + fake.NewSpace("alice-private", "member-2", "alice"), + fake.NewSpaceBinding("smith-community-smith", "smith", "smith-community", "admin"), + fake.NewSpaceBinding("smith-community-publicviewer", toolchainv1alpha1.KubesawAuthenticatedUsername, "smith-community", "viewer"), + fake.NewSpaceBinding("alice-default", "alice", "alice-private", "admin"), + fake.NewBannedUser("eve", eveEmail), + fake.NewBase1NSTemplateTier(), + ) + + // configure informer + inf := infservice.NewInformerService(cli, commontest.HostOperatorNs) + + // configure Application + fakeApp.Err = nil + fakeApp.InformerServiceMock = inf + fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(testServer.URL) + fakeApp.SignupServiceMock = signupService + + s.Application.MockSignupService(signupService) + s.Application.MockInformerService(inf) + + // configure proxy + p.spaceLister = &handlers.SpaceLister{ + GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, + GetInformerServiceFunc: func() appservice.InformerService { return inf }, + ProxyMetrics: p.metrics, + } + + // run test cases + tests := map[string]struct { + ProxyRequestMethod string + ProxyRequestHeaders http.Header + ExpectedAPIServerRequestHeaders http.Header + ExpectedProxyResponseStatus int + RequestPath string + ExpectedResponse string + }{ + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When smith requests the list of pods in workspace smith-community + // Then the request is forwarded from the proxy + // And the request impersonates smith + // And the request's X-SSO-User Header is set to smith's ID + // And the request is successful + "plain http actual request as community space owner": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith"}, + "X-SSO-User": {"username-" + smith.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsRequestURL("smith-community"), + ExpectedResponse: httpTestServerResponse, + }, + // Given The not ready user john exists + // When john requests the list of pods in workspace smith-community + // Then the request is forwarded from the proxy + // And the request impersonates john + // And the request's X-SSO-User Header is set to john's ID + // And the request is successful + "plain http actual request as notReadyUser": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(john)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + john.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsRequestURL("smith-community"), + ExpectedResponse: httpTestServerResponse, + }, + // Given The not signed up user bob exists + // When bob requests the list of pods in workspace smith-community + // Then the request is forwarded from the proxy + // And the request impersonates bob + // And the request's X-SSO-User Header is set to bob's ID + // And the request is successful + "plain http actual request as not signed up user": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(bob)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + bob.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsRequestURL("smith-community"), + ExpectedResponse: httpTestServerResponse, + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // And a user named alice exists + // And smith's smith-community is not directly shared with alice + // When alice requests the list of pods in workspace smith-community + // Then the request is forwarded from the proxy + // And the request impersonates the PublicViewer + // And the request's X-SSO-User Header is set to alice's ID + // And the request is successful + "plain http actual request as community user": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + alice.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsRequestURL("smith-community"), + ExpectedResponse: httpTestServerResponse, + }, + // Given user alice exists + // And alice owns a private workspace + // When smith requests the list of pods in alice's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http actual request as non-owner to private workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsRequestURL("alice-private"), + ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", + }, + // Given banned user eve exists + // And user smith exists + // And smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When eve requests the list of pods in smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http actual request as banned user to community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(eve, authsupport.WithEmailClaim(eveEmail))}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsRequestURL("smith-community"), + ExpectedResponse: "user access is forbidden: user access is forbidden", + }, + // Given banned user eve exist + // And user alice exists + // And alice owns a private workspace + // When eve requests the list of pods in alice's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http actual request as banned user to private workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(eve, authsupport.WithEmailClaim(eveEmail))}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsRequestURL("alice-private"), + ExpectedResponse: "user access is forbidden: user access is forbidden", + }, + // Given user alice exists + // And alice owns a private workspace + // When alice requests the list of pods in a non existing namespace in alice's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http request as owner to not existing namespace in private workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("alice-private", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'alice-private' is forbidden", + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When smith requests the list of pods in a non existing namespace in workspace smith-community + // Then the request is forwarded from the proxy + // And the request impersonates smith + // And the request's X-SSO-User Header is set to smith's ID + // And the request is successful + "plain http request as owner to not existing namespace in community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // And user alice exists + // When alice requests the list of pods in a non existing namespace in smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http request as community user to not existing namespace in community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When bob requests the list of pods in a non existing namespace in smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http request as unsigned user to not existing namespace in community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(bob)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // And not ready user john exists + // When john requests the list of pods in a non existing namespace in smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http request as notReadyUser to not existing namespace in community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(john)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + }, + // Given banned user eve exists + // And user smith exists + // And smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When eve requests the list of pods in a non existing namespace smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http actual request as banned user to not existing namespace community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(eve, authsupport.WithEmailClaim(eveEmail))}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "user access is forbidden: user access is forbidden", + }, + } + + for k, tc := range tests { + s.Run(k, func() { + testServerInvoked := atomic.NewBool(false) + + // given + testServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + v := testServerInvoked.Swap(true) + require.False(s.T(), v, "expected handler to be invoked just one time") + + w.Header().Set("Content-Type", "application/json") + // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier + w.Header().Set("Access-Control-Allow-Origin", "dummy") + w.WriteHeader(http.StatusOK) + _, err := w.Write([]byte(httpTestServerResponse)) + require.NoError(s.T(), err) + for hk, hv := range tc.ExpectedAPIServerRequestHeaders { + require.Len(s.T(), r.Header.Values(hk), len(hv)) + for i := range hv { + assert.Equal(s.T(), hv[i], r.Header.Values(hk)[i], "header %s", hk) + } + } + }) + + // prepare request + req, err := http.NewRequest(tc.ProxyRequestMethod, tc.RequestPath, nil) + require.NoError(s.T(), err) + require.NotNil(s.T(), req) + + for hk, hv := range tc.ProxyRequestHeaders { + for _, v := range hv { + req.Header.Add(hk, v) + } + } + + // when + client := http.Client{Timeout: 3 * time.Second} + resp, err := client.Do(req) + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), resp) + defer resp.Body.Close() + assert.Equal(s.T(), tc.ExpectedProxyResponseStatus, resp.StatusCode) + s.assertResponseBody(resp, tc.ExpectedResponse) + + forwardExpected := len(tc.ExpectedAPIServerRequestHeaders) > 0 + requestForwarded := testServerInvoked.Load() + require.Equal(s.T(), + forwardExpected, requestForwarded, + "expecting call forward to be %v, got %v", forwardExpected, requestForwarded, + ) + }) + } + }) +} diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 6bd9ad6c..cb1aa373 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -28,6 +28,8 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/gin-gonic/gin" + "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" "k8s.io/client-go/kubernetes/scheme" @@ -38,7 +40,6 @@ import ( authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" - "github.com/google/uuid" routev1 "github.com/openshift/api/route/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -117,46 +118,11 @@ func (s *TestProxySuite) TestProxy() { _ = server.Close() }() - // Wait up to N seconds for the Proxy server to start - ready := false - sec := 10 - for i := 0; i < sec; i++ { - log.Println("Checking if Proxy is started...") - req, err := http.NewRequest("GET", "http://localhost:8081/api/mycoolworkspace/pods", nil) - require.NoError(s.T(), err) - require.NotNil(s.T(), req) - resp, err := http.DefaultClient.Do(req) - if err != nil { - time.Sleep(time.Second) - continue - } - _, _ = io.Copy(io.Discard, resp.Body) - _ = resp.Body.Close() - if resp.StatusCode != http.StatusUnauthorized { - // The server may be running but still not fully ready to accept requests - time.Sleep(time.Second) - continue - } - // Server is up and running! - ready = true - break - } - require.True(s.T(), ready, "Proxy is not ready after %d seconds", sec) - + s.Run("is alive", func() { + s.waitForProxyToBeAlive(DefaultPort) + }) s.Run("health check ok", func() { - req, err := http.NewRequest("GET", "http://localhost:8081/proxyhealth", nil) - require.NoError(s.T(), err) - require.NotNil(s.T(), req) - - // when - resp, err := http.DefaultClient.Do(req) - - // then - require.NoError(s.T(), err) - require.NotNil(s.T(), resp) - defer resp.Body.Close() - assert.Equal(s.T(), http.StatusOK, resp.StatusCode) - s.assertResponseBody(resp, `{"alive": true}`) + s.checkProxyIsHealthy(DefaultPort) }) s.checkPlainHTTPErrors(fakeApp) @@ -167,6 +133,62 @@ func (s *TestProxySuite) TestProxy() { } } +func (s *TestProxySuite) spinUpProxy(fakeApp *fake.ProxyFakeApp, port string) (*Proxy, *http.Server) { + proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) + p, err := newProxyWithClusterClient( + fakeApp, nil, proxyMetrics, proxytest.NewGetMembersFunc(commontest.NewFakeClient(s.T()))) + require.NoError(s.T(), err) + + server := p.StartProxy(port) + require.NotNil(s.T(), server) + + return p, server +} + +func (s *TestProxySuite) waitForProxyToBeAlive(port string) { + // Wait up to N seconds for the Proxy server to start + ready := false + sec := 10 + for i := 0; i < sec; i++ { + log.Println("Checking if Proxy is started...") + req, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%s/api/mycoolworkspace/pods", port), nil) + require.NoError(s.T(), err) + require.NotNil(s.T(), req) + resp, err := http.DefaultClient.Do(req) + if err != nil { + time.Sleep(time.Second) + continue + } + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized { + // The server may be running but still not fully ready to accept requests + time.Sleep(time.Second) + continue + } + // Server is up and running! + ready = true + break + } + require.True(s.T(), ready, "Proxy is not ready after %d seconds", sec) +} + +func (s *TestProxySuite) checkProxyIsHealthy(port string) { + req, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%s/proxyhealth", port), nil) + require.NoError(s.T(), err) + require.NotNil(s.T(), req) + + // when + resp, err := http.DefaultClient.Do(req) + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), resp) + defer resp.Body.Close() + assert.Equal(s.T(), http.StatusOK, resp.StatusCode) + s.assertResponseBody(resp, `{"alive": true}`) +} + func (s *TestProxySuite) checkPlainHTTPErrors(fakeApp *fake.ProxyFakeApp) { s.Run("plain http error", func() { s.Run("unauthorized if no token present", func() { @@ -218,6 +240,23 @@ func (s *TestProxySuite) checkPlainHTTPErrors(fakeApp *fake.ProxyFakeApp) { s.assertResponseBody(resp, "invalid bearer token: unable to extract userID from token: token does not comply to expected claims: subject missing") }) + s.Run("unauthorized if can't extract email from a valid token", func() { + // when + req, err := http.NewRequest("GET", "http://localhost:8081/api/mycoolworkspace/pods", nil) + require.NoError(s.T(), err) + require.NotNil(s.T(), req) + userID := uuid.New() + req.Header.Set("Authorization", "Bearer "+s.token(userID, authsupport.WithEmailClaim(""))) + resp, err := http.DefaultClient.Do(req) + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), resp) + defer resp.Body.Close() + assert.Equal(s.T(), http.StatusUnauthorized, resp.StatusCode) + s.assertResponseBody(resp, "invalid bearer token: unable to extract userID from token: token does not comply to expected claims: email missing") + }) + s.Run("unauthorized if workspace context is invalid", func() { // when req := s.request() @@ -476,11 +515,15 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { tests := map[string]struct { ProxyRequestMethod string + ProxyRequestPaths map[string]string ProxyRequestHeaders http.Header ExpectedAPIServerRequestHeaders http.Header ExpectedProxyResponseHeaders http.Header ExpectedProxyResponseStatus int Standalone bool // If true then the request is not expected to be forwarded to the kube api server + + OverrideGetSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) + ExpectedResponse *string }{ "plain http cors preflight request with no request method": { ProxyRequestMethod: "OPTIONS", @@ -569,6 +612,16 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { }, ExpectedProxyResponseStatus: http.StatusOK, }, + "proxy plain http actual request as not provisioned user": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(uuid.New())}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith3"}, + }, + ExpectedResponse: ptr("unable to get target cluster: user is not provisioned (yet)"), + ExpectedProxyResponseStatus: http.StatusInternalServerError, + }, "proxy plain http actual request": { ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, @@ -627,6 +680,54 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { }, ExpectedProxyResponseStatus: http.StatusOK, }, + "error retrieving user workspaces": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + }, + ExpectedProxyResponseStatus: http.StatusInternalServerError, + OverrideGetSignupFunc: func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { + return nil, fmt.Errorf("test error") + }, + ExpectedResponse: ptr("unable to retrieve user workspaces: test error"), + }, + "unauthorized if workspace not exists": { + ProxyRequestPaths: map[string]string{ + "not existing workspace namespace": "http://localhost:8081/workspaces/not-existing-workspace/api/namespaces/not-existing-namespace/pods", + }, + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + }, + ExpectedResponse: ptr("unable to get target cluster: access to workspace 'not-existing-workspace' is forbidden"), + ExpectedProxyResponseStatus: http.StatusInternalServerError, + }, + "unauthorized if namespace does not exist in implicit workspace": { + ProxyRequestPaths: map[string]string{ + "not existing namespace": "http://localhost:8081/api/namespaces/not-existing-namespace/pods", + }, + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + }, + ExpectedResponse: ptr("invalid workspace request: access to namespace 'not-existing-namespace' in workspace 'mycoolworkspace' is forbidden"), + ExpectedProxyResponseStatus: http.StatusForbidden, + }, + "unauthorized if namespace does not exist in explicit workspace": { + ProxyRequestPaths: map[string]string{ + "not existing namespace": "http://localhost:8081/workspaces/mycoolworkspace/api/namespaces/not-existing-namespace/pods", + }, + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + }, + ExpectedResponse: ptr("invalid workspace request: access to namespace 'not-existing-namespace' in workspace 'mycoolworkspace' is forbidden"), + ExpectedProxyResponseStatus: http.StatusForbidden, + }, } rejectedHeaders := []headerToAdd{ @@ -645,6 +746,14 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { for k, tc := range tests { s.Run(k, func() { + paths := tc.ProxyRequestPaths + if len(paths) == 0 { + paths = map[string]string{ + "default workspace": "http://localhost:8081/api/mycoolworkspace/pods", + "workspace context": "http://localhost:8081/workspaces/mycoolworkspace/api/mycoolworkspace/pods", + "proxy plugin context": "http://localhost:8081/plugins/myplugin/workspaces/mycoolworkspace/api/mycoolworkspace/pods", + } + } for _, firstHeader := range rejectedHeaders { rejectedHeadersToAdd := []headerToAdd{firstHeader} @@ -653,11 +762,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { // Test each request using both the default workspace URL and a URL that uses the // workspace context. Both should yield the same results. - for workspaceContext, reqPath := range map[string]string{ - "default workspace": "http://localhost:8081/api/mycoolworkspace/pods", - "workspace context": "http://localhost:8081/workspaces/mycoolworkspace/api/mycoolworkspace/pods", - "proxy plugin context": "http://localhost:8081/plugins/myplugin/workspaces/mycoolworkspace/api/mycoolworkspace/pods", - } { + for workspaceContext, reqPath := range paths { s.Run(workspaceContext, func() { // given req, err := http.NewRequest(tc.ProxyRequestMethod, reqPath, nil) @@ -748,6 +853,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { s.Application.MockInformerService(inf) fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(testServer.URL) + fakeApp.InformerServiceMock = inf p.spaceLister = &handlers.SpaceLister{ GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, @@ -756,6 +862,9 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { }, ProxyMetrics: p.metrics, } + if tc.OverrideGetSignupFunc != nil { + p.spaceLister.GetSignupFunc = tc.OverrideGetSignupFunc + } } // when @@ -767,7 +876,9 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { require.NotNil(s.T(), resp) defer resp.Body.Close() assert.Equal(s.T(), tc.ExpectedProxyResponseStatus, resp.StatusCode) - if !tc.Standalone { + if tc.ExpectedResponse != nil { + s.assertResponseBody(resp, *tc.ExpectedResponse) + } else if !tc.Standalone { s.assertResponseBody(resp, "my response") } for hk, hv := range tc.ExpectedProxyResponseHeaders { @@ -855,6 +966,10 @@ var noCORSHeaders = map[string][]string{ "Vary": {}, } +func ptr[T any](t T) *T { + return &t +} + func upgradeToWebsocket(req *http.Request) { req.Header.Set("Connection", "upgrade") req.Header.Set("Upgrade", "websocket") @@ -1100,7 +1215,7 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { for k, tc := range tests { s.T().Run(k, func(t *testing.T) { - err := validateWorkspaceRequest(tc.requestedWorkspace, tc.requestedNamespace, tc.workspaces) + err := validateWorkspaceRequest(tc.requestedWorkspace, tc.requestedNamespace, tc.workspaces...) if tc.expectedErr == "" { require.NoError(t, err) } else { diff --git a/test/fake/informer.go b/test/fake/informer.go index dbbb1fa6..241f1cf4 100644 --- a/test/fake/informer.go +++ b/test/fake/informer.go @@ -3,6 +3,7 @@ package fake import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/registration-service/pkg/configuration" + "github.com/codeready-toolchain/toolchain-common/pkg/hash" "github.com/codeready-toolchain/toolchain-common/pkg/test" spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -100,3 +101,18 @@ func NewMasterUserRecord(name string) *toolchainv1alpha1.MasterUserRecord { }, } } + +func NewBannedUser(name, email string) *toolchainv1alpha1.BannedUser { + return &toolchainv1alpha1.BannedUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: configuration.Namespace(), + Labels: map[string]string{ + toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString(email), + }, + }, + Spec: toolchainv1alpha1.BannedUserSpec{ + Email: email, + }, + } +}