diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index dd9930a5..d2602ad7 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -308,9 +308,7 @@ func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, } // 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("", requestedNamespace, workspaces...); err != nil { + if err := validateWorkspaceRequest("", workspaces...); err != nil { return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) } @@ -333,9 +331,7 @@ func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, work } // 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 { + if err := validateWorkspaceRequest(workspaceName, *workspace); err != nil { return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) } @@ -828,9 +824,8 @@ func replaceTokenInWebsocketRequest(req *http.Request, newToken string) { } // 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 { +// If `requestedWorkspace` is empty, then the home workspace (the one with `status.Type` set to `home`) is assumed. +func validateWorkspaceRequest(requestedWorkspace string, workspaces ...toolchainv1alpha1.Workspace) error { // check workspace access isHomeWSRequested := requestedWorkspace == "" @@ -845,32 +840,5 @@ func validateWorkspaceRequest(requestedWorkspace, requestedNamespace string, wor return fmt.Errorf("access to workspace '%s' is forbidden", requestedWorkspace) } - // check namespace access - if requestedNamespace != "" { - allowedNamespace := false - namespaces := workspaces[allowedWorkspace].Status.Namespaces - for _, ns := range namespaces { - if ns.Name == requestedNamespace { - allowedNamespace = true - break - } - } - if !allowedNamespace { - return fmt.Errorf("access to namespace '%s' in workspace '%s' is forbidden", requestedNamespace, workspaces[allowedWorkspace].Name) - } - } return nil } - -func namespaceFromCtx(ctx echo.Context) string { - path := ctx.Request().URL.Path - if strings.Index(path, "/namespaces/") > 0 { - segments := strings.Split(path, "/") - for i, segment := range segments { - if segment == "namespaces" && i+1 < len(segments) { - return segments[i+1] - } - } - } - return "" -} diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index c2d5513f..aeaf32fa 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -283,73 +283,103 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr }, // 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", + // When alice requests the list of pods in a namespace which does not belong to the alice's workspace + // Then the proxy does forward the request anyway. + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API always server returns OK + "plain http request as permitted user to namespace outside of private workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"alice"}, + "X-SSO-User": {"username-" + alice.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsInNamespaceRequestURL("alice-private", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // 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", + // When smith requests the list of pods in a namespace which does not belong to the workspace smith-community + // Then the proxy does forward the request anyway. + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API server returns OK + "plain http request as owner to namespace outside of community workspace": { + 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: podsInNamespaceRequestURL("smith-community", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // 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", + // When alice requests the list of pods in a namespace which does not belong to the smith's workspace + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API server returns OK + "plain http request as community user to namespace outside of community workspace": { + 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: podsInNamespaceRequestURL("smith-community", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // 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", + // When bob requests the list of pods in a namespace which does not belong to the smith's workspace + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API server returns OK + "plain http request as unsigned user to namespace outside of community workspace": { + 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: podsInNamespaceRequestURL("smith-community", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // 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", + // When john requests the list of pods in a namespace which does not belong to the smith's workspace + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API server returns OK + "plain http request as notReadyUser to namespace outside community workspace": { + 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: podsInNamespaceRequestURL("smith-community", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // 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 + // 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": { diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index d638a274..8d87df7d 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -702,29 +702,43 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { 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": { + "request to namespace which does not belong to implicit workspace is still proxied OK": { + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. ProxyRequestPaths: map[string]string{ - "not existing namespace": "http://localhost:8081/api/namespaces/not-existing-namespace/pods", + "not existing namespace": "http://localhost:8081/api/namespaces/namespace-outside-of-workspace/pods", }, ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ - "Authorization": {"Bearer clusterSAToken"}, + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith2"}, }, - ExpectedResponse: ptr("invalid workspace request: access to namespace 'not-existing-namespace' in workspace 'mycoolworkspace' is forbidden"), - ExpectedProxyResponseStatus: http.StatusForbidden, + ExpectedProxyResponseHeaders: map[string][]string{ + "Access-Control-Allow-Origin": {"*"}, + "Access-Control-Allow-Credentials": {"true"}, + "Access-Control-Expose-Headers": {"Content-Length, Content-Encoding, Authorization"}, + "Vary": {"Origin"}, + }, + ExpectedProxyResponseStatus: http.StatusOK, }, - "unauthorized if namespace does not exist in explicit workspace": { + "request to namespace which does not belong to explicit workspace is still proxied OK": { ProxyRequestPaths: map[string]string{ - "not existing namespace": "http://localhost:8081/workspaces/mycoolworkspace/api/namespaces/not-existing-namespace/pods", + "not existing namespace": "http://localhost:8081/workspaces/mycoolworkspace/api/namespaces/namespace-outside-of-workspace/pods", }, ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ - "Authorization": {"Bearer clusterSAToken"}, + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith2"}, }, - ExpectedResponse: ptr("invalid workspace request: access to namespace 'not-existing-namespace' in workspace 'mycoolworkspace' is forbidden"), - ExpectedProxyResponseStatus: http.StatusForbidden, + ExpectedProxyResponseHeaders: map[string][]string{ + "Access-Control-Allow-Origin": {"*"}, + "Access-Control-Allow-Credentials": {"true"}, + "Access-Control-Expose-Headers": {"Content-Length, Content-Encoding, Authorization"}, + "Vary": {"Origin"}, + }, + ExpectedProxyResponseStatus: http.StatusOK, }, } @@ -1109,13 +1123,11 @@ func (s *TestProxySuite) TestGetWorkspaceContext() { func (s *TestProxySuite) TestValidateWorkspaceRequest() { tests := map[string]struct { requestedWorkspace string - requestedNamespace string workspaces []toolchainv1alpha1.Workspace expectedErr string }{ "valid workspace request": { requestedWorkspace: "myworkspace", - requestedNamespace: "ns-dev", workspaces: []toolchainv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{ Name: "myworkspace", @@ -1141,7 +1153,6 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { }, "valid home workspace request": { requestedWorkspace: "", // home workspace is default when no workspace is specified - requestedNamespace: "test-1234", workspaces: []toolchainv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{ Name: "homews", @@ -1157,7 +1168,6 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { }, "workspace not allowed": { requestedWorkspace: "notexist", - requestedNamespace: "myns", workspaces: []toolchainv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{ Name: "myworkspace", @@ -1170,42 +1180,11 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { }}, expectedErr: "access to workspace 'notexist' is forbidden", }, - "namespace not allowed": { - requestedWorkspace: "myworkspace", - requestedNamespace: "notexist", - workspaces: []toolchainv1alpha1.Workspace{{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myworkspace", - }, - Status: toolchainv1alpha1.WorkspaceStatus{ - Namespaces: []toolchainv1alpha1.SpaceNamespace{ - {Name: "ns-dev"}, - }, - }, - }}, - expectedErr: "access to namespace 'notexist' in workspace 'myworkspace' is forbidden", - }, - "namespace not allowed for home workspace": { - requestedWorkspace: "", // home workspace is default when no workspace is specified - requestedNamespace: "myns", - workspaces: []toolchainv1alpha1.Workspace{{ - ObjectMeta: metav1.ObjectMeta{ - Name: "homews", - }, - Status: toolchainv1alpha1.WorkspaceStatus{ - Type: "home", // home workspace - Namespaces: []toolchainv1alpha1.SpaceNamespace{ - {Name: "test-1234"}, // namespace does not match the requested one - }, - }, - }}, - expectedErr: "access to namespace 'myns' in workspace 'homews' is forbidden", - }, } 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.workspaces...) if tc.expectedErr == "" { require.NoError(t, err) } else {