From d1cf75b2dc88db64baed35b900d8ca654a0400a3 Mon Sep 17 00:00:00 2001 From: Alexey Kazakov Date: Mon, 30 Sep 2024 16:32:08 -0700 Subject: [PATCH 1/5] Allow proxy to forward requests to namespaces outside of workspace --- pkg/proxy/proxy.go | 27 +------ pkg/proxy/proxy_community_test.go | 126 ++++++++++++++++++------------ pkg/proxy/proxy_test.go | 71 ++++++----------- 3 files changed, 107 insertions(+), 117 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 08ded564..ae757ec6 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -317,9 +317,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()) } @@ -342,9 +340,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()) } @@ -857,9 +853,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 zero, 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 == "" @@ -874,20 +869,6 @@ 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 } diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 5274bd75..aa385bd4 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -287,73 +287,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 server returns OK + "plain http request as owner 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 cb1aa373..d14034b0 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -704,29 +704,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, }, } @@ -1119,13 +1133,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", @@ -1151,7 +1163,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", @@ -1167,7 +1178,6 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { }, "workspace not allowed": { requestedWorkspace: "notexist", - requestedNamespace: "myns", workspaces: []toolchainv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{ Name: "myworkspace", @@ -1180,42 +1190,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 { From 6656eb26cc203da123dd4a831d205d9974ccb9c8 Mon Sep 17 00:00:00 2001 From: Alexey Kazakov Date: Mon, 30 Sep 2024 16:42:19 -0700 Subject: [PATCH 2/5] linter --- pkg/proxy/proxy.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index ae757ec6..541112c7 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -871,16 +871,3 @@ func validateWorkspaceRequest(requestedWorkspace string, workspaces ...toolchain 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 "" -} From 1fee7891fd379747c3d368fc2f5893f8053d3612 Mon Sep 17 00:00:00 2001 From: Alexey Kazakov Date: Fri, 4 Oct 2024 13:54:31 -0700 Subject: [PATCH 3/5] Update pkg/proxy/proxy.go Co-authored-by: Kanika Rana <46766610+ranakan19@users.noreply.github.com> --- pkg/proxy/proxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 23348d72..e7817a4a 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -816,7 +816,7 @@ 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, then the home workspace (the one with `status.Type` set to `home`) is assumed. +// 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 == "" From 391224fadda8dfda0414013350f0391c088fb256 Mon Sep 17 00:00:00 2001 From: Alexey Kazakov Date: Fri, 4 Oct 2024 14:41:20 -0700 Subject: [PATCH 4/5] Update pkg/proxy/proxy_community_test.go Co-authored-by: Kanika Rana <46766610+ranakan19@users.noreply.github.com> --- pkg/proxy/proxy_community_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index aa385bd4..cd249940 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -291,7 +291,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // 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 + // Here the request is successful because the underlying mock target cluster API always server returns OK "plain http request as owner to namespace outside of private workspace": { ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, From c557b4141f7d1eac7a133795a92693d78301d220 Mon Sep 17 00:00:00 2001 From: Alexey Kazakov Date: Wed, 9 Oct 2024 16:22:14 -0700 Subject: [PATCH 5/5] Update pkg/proxy/proxy_community_test.go --- pkg/proxy/proxy_community_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index cd249940..8d7924c3 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -292,7 +292,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // 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 owner to namespace outside of private workspace": { + "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{