ml-tooling / contaxy

MIT License
10 stars 8 forks source link

Access service of other user #9

Open JNKielmann opened 3 years ago

JNKielmann commented 3 years ago

Hi all,

Lately we have been doing some further testing of contaxy and I noticed a bug that allows a user to access any service regardless of permissions. Steps to reproduce:

I think the issue is in the nginx config (https://github.com/ml-tooling/contaxy/blob/main/docker/nginx/nginx.conf#L166). By modifying the url to point to the private project of User B, the token verification for the following permission is made "/projects/9xz28racfobhlrzqigcg18pg4/services/ctxy-p-6i2jix6d17pg61vun97kydah2-s-echo/access/80#read". This check succeeds as User B has access to the project and therefore access to all its services. However, it is not checked if the service being accessed is actually part of that project. As a potential fix of the nginx config we could extract the project id from the service name (everything between -p- and -s-) and use that for the permission check or check that it is the same as the project id in the URL.

What do you think?

Thanks, Jan

lukasmasuch commented 3 years ago

@JNKielmann

That's a good find. There is a missing part in the Nginx proxy which wasn't implemented yet :( Actually accessing internal services should be only allowed via a dedicated service access token (Cookie: service_proxy_token) which can be requested via the API (see here: https://github.com/ml-tooling/contaxy/blob/main/backend/src/contaxy/api/endpoints/deployment.py#L703). Requesting this token should be done automatically by Nginx.

Here are the original notes on that aspect:

image

In other words: If the service_proxy_token is not provided (as a cookie) or invalid (outdated) in the user request to a service, Nginx has to use the user token (ct_token) to request the service_proxy_token from the API endpoint (nginx does the API call). The API endpoint will do the required checks to find out if the user is actually allowed to access the service/port/path and return a short-lived token for the given combination (this will also check if the service is actually part of the given project). Nginx will set this service_proxy_token as a cookie and use it for all proxy requests to the given service (the ct_token is removed from the proxy request to prevent malicious activity of the underlying service with the user token).

Doing it this way has a lot of security benefits by still reducing the number of required checks and DB calls in the backend (token is automatically set as a cookie and we apply heavy caching).

The two missing parts are: Implementing the workflow of getting the Service Access Token in Nginx and finishing the backend API method to do the required checks (e.g. service part of the project).

lukasmasuch commented 3 years ago

As a potential fix of the nginx config we could extract the project id from the service name (everything between -p- and -s-) and use that for the permission check or check that it is the same as the project id in the URL.

This was the initial way we wanted to solve this, however, using a dedicated access token provides a lot of additional benefits, and we don't have to rely on a certain structure in the naming.

JNKielmann commented 3 years ago

Thanks for the explanation @LukasMasuch

I saw that there is already a concept of session tokens implemented in the Nginx proxy. As I understand it, a session JWT token is set as a cookie (ct_session_token) after the first access to a service. This session JWT token will only give access to this specific service and would reduce the number of DB calls to verify API tokens? Would this ct_session_token then be replaced by the service_proxy_token?

Moreover, I found this issue when implementing a workspace extension. This extension is started as a service in the global extension project. When a user calls the extension endpoint which creates a new workspace, the provided token of the user is used to create the workspace service via the contaxy API. If I understand you corrently, this would no longer be possible with the implementation of the service_proxy_token, right? The user token would no longer be passed to the extension service. Do you also already have a concept on how to handle that (a (extension) service that wants to make requests to the Contaxy API). Maybe when creating a service you could pass a list of permissions that the service will need (e.g. create services in a specific project) and during service creation a technical user with these permissions is created that is passed to the service?

raethlein commented 3 years ago

I think the behavior described by @LukasMasuch should already be implemented here. Though, currently nginx generates it's own session token (here) instead of using the mentioned API endpoint (https://github.com/ml-tooling/contaxy/blob/main/backend/src/contaxy/api/endpoints/deployment.py#L703). The service_proxy_token and the currently used name for it ct_session_token should be the same token. Maybe replacing that logic already solves the issue. Also, it looks like that currently the token can only be sent via cookie to nginx (and not via a custom header or the Authorization header). Further, currently the ct_token does not seem to be replaced by the ct_session_token, but rather set additionally. So, currently extensions can receive the full user token (ct_token) which allows them to make requests as the user. I think you are right that this would not be possible anymore in case only the ct_session_token would be forwarded to the extensions. The creation with a technical user sounds good, although that would limit the use-case an extension could cover. For example, creating an extension that lists certain services could be difficult as still the permissions of the actual user during runtime would need to be made. Another solution could be that an extension has its own API token and uses the received ct_session_token to get all of the user's permissions, although that would require to query the DB again. @LukasMasuch do you remember why the ct_token should be completely replaced with the ct_session_token?