Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow proxy to forward requests to namespaces outside of workspace #471

Merged
merged 13 commits into from
Oct 29, 2024
Merged

Allow proxy to forward requests to namespaces outside of workspace #471

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d1cf75b
Allow proxy to forward requests to namespaces outside of workspace
alexeykazakov Sep 30, 2024
6656eb2
linter
alexeykazakov Sep 30, 2024
228625c
Merge branch 'master' into proxy-namespace
mfrancisc Oct 1, 2024
c8cc345
Merge branch 'master' into proxy-namespace
alexeykazakov Oct 1, 2024
db076ea
Merge branch 'master' into proxy-namespace
alexeykazakov Oct 3, 2024
1fee789
Update pkg/proxy/proxy.go
alexeykazakov Oct 4, 2024
30aae25
Merge branch 'master' into proxy-namespace
alexeykazakov Oct 4, 2024
391224f
Update pkg/proxy/proxy_community_test.go
alexeykazakov Oct 4, 2024
d7f9901
Merge branch 'master' into proxy-namespace
alexeykazakov Oct 7, 2024
c557b41
Update pkg/proxy/proxy_community_test.go
alexeykazakov Oct 9, 2024
c6fd25a
Merge branch 'master' into proxy-namespace
alexeykazakov Oct 9, 2024
56f55b6
Merge branch 'master' into proxy-namespace
alexeykazakov Oct 13, 2024
1844bef
Merge branch 'master' into proxy-namespace
alexeykazakov Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 4 additions & 36 deletions pkg/proxy/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,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())
}

Expand All @@ -328,9 +326,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())
}

Expand Down Expand Up @@ -820,9 +816,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 == ""

Expand All @@ -837,32 +832,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 ""
}
126 changes: 78 additions & 48 deletions pkg/proxy/proxy_community_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 always server returns OK
"plain http request as owner to namespace outside of private workspace": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused as to what does 'owner' refer to here in the test description. To me, there is not much difference in the two statements below, wrt to the test:
plain http request as owner to namespace outside of private workspace v/s
plain http request to namespace outside of private workspace.
So either I'm missing the significance of owner here and we should add it here (maybe in the comments?) or we can make the test case description simpler and remove owner.
Let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comments preceeding it explain it pretty well. But yeah the 'owner' thing in the test description threw me off a bit as well.

What about plain http request as permitted user to namespace outside of private workspace? Is that accurate?

Copy link
Contributor Author

@alexeykazakov alexeykazakov Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced owner by permitted user in c557b41

alexeykazakov marked this conversation as resolved.
Show resolved Hide resolved
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": {
Expand Down
71 changes: 25 additions & 46 deletions pkg/proxy/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}

Expand Down Expand Up @@ -1118,13 +1132,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",
Expand All @@ -1150,7 +1162,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",
Expand All @@ -1166,7 +1177,6 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() {
},
"workspace not allowed": {
requestedWorkspace: "notexist",
requestedNamespace: "myns",
workspaces: []toolchainv1alpha1.Workspace{{
ObjectMeta: metav1.ObjectMeta{
Name: "myworkspace",
Expand All @@ -1179,42 +1189,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 {
Expand Down
Loading