Allow to call getCurrentUser and oAuth API with machine token #16383
Conversation
vinokurig
requested review from
l0rd,
mshaposhnik,
nickboldt and
rhopp
as code owners
che-bot
added
status/code-review
|
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) |
| .toInstance(new MachineAuthenticatedResource("oauth", "token")); | ||
| .toInstance( | ||
| new MachineAuthenticatedResource( | ||
| "oauth", "token", "authenticate", "callback", "getRegisteredAuthenticators")); |
There was a problem hiding this comment.
The token endpoint means that we're allowing any plugin or custom code in the workspace to impersonate the current user and talk to the Che API. This basically means that you can for example access other workspaces from the current workspace, create new workspaces, delete workspaces, etc...
I would call that a security issue, because it is conceivable to imagine that a user might import an "infected" devfile that would contain some code that could read the machine token from the env and wreck havoc in all the user workspaces - reading secret data, delete stuff, DDoS the cluster with hundreds of workspace start requests, etc.
There was a problem hiding this comment.
The token method is already opened. Actually I don't understand how oAuth API can influence the che API. It allows to get the GitHub or OpenShift token, but that was discussed earlier.
cc @skabashnyuk
There was a problem hiding this comment.
To be fair, I misunderstood the purpose of the API 🙄 But I still think this is a dangerous thing. If for example the user authenticated with OpenShift authenticator as himself and the workspace is able to get hold of the openshift token (which I think it would with this change?), then the workspace can also go to keycloak and use the openshift token to get access token for the Che API. From there we're at a place I described in the above comment.
Maybe I'm wrong (I'm fairly new to this auth business to be fair) but giving the whole of workspace (which can come from various sources) is a dangerous thing. If nothing else it opens up the door to phishing attacks (please import this devfile with a component with only a slightly misspelled image name, nothing wrong can happen...).
|
I agree with @metlos POV. We are making machine token too powerful. My concerns are similar to the single I want to let @benoitf @l0rd @davidfestal decide is that acceptable. |
|
@skabashnyuk @metlos do we have other options? The goal is to preconfigure Anyway on VS Code, not Che, extensions have access to local filesystem and k8s tokens. I don't think it's considered as a VS Code vulnerability but it requires that users should install only extensions that they trust. For Che it should be the same. |
We understand the goal, it's understandable and logical. We just have concerns in the way how it is going to be implemented. |
|
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
| .toInstance( | ||
| new MachineAuthenticatedResource( | ||
| "oauth", "token", "authenticate", "callback", "getRegisteredAuthenticators")); | ||
| .toInstance(new MachineAuthenticatedResource("oauth", "token")); |
There was a problem hiding this comment.
.toInstance(new MachineAuthenticatedResource("oauth", "token")); this line can be removed too?
|
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
What does this PR do?
Allow to get current user with machine token; Allow to call other oAuth service API with machine token. This is needed for github and openShift authentication to work in multi-user mode.
What issues does this PR fix or reference?
#15261
Release Notes
Docs PR