Conversation
There was a problem hiding this comment.
Hello friend!
I appreciate the consideration and willingness to help immensely. For that reason, I have spent a great deal of time reviewing your changes and addressing my areas of concern with examples, alternatives, and linked documentation.
Ultimately and unfortunately, I can not merge this as is given the level of vulnerabilities it introduces. The ability to let discopanel admins define their own custom containers and runtime scripts is something I considered at one point, but eventually decided against for the mentioned security implications.
Consider that discopanel itself is often run directly on a host server (or in a container), with privileged read/write access to the docker socket which inherently grants complete control over every other container on that host - it's a very easily deployed reverse shell for a bad actor - unless we defend against it.
Perhaps there is a way to safely conform to this, but I imagine it would require a lengthy and difficult number of engineering sessions to complete. I know this is probably not the response you were hoping for, and for that I am sorry. Please read my comments and let me know if you have any questions, I am happy to help.
UPDATE/EDIT: @Wolfhound905 After speaking with peers (the mods), they had the idea that we should revisit this once oauth/oidc is fully and completely implemented. I think that would be the perfect time to introduce something like this (after a little polishing up) as a opt-in feature, scoped specifically to admin roles.
internal/docker/client.go
Outdated
| c.log.Info("Generating init wrapper script with %d commands", len(initCommands)) | ||
|
|
||
| // Create wrapper script content | ||
| script := "#!/bin/bash\n" |
There was a problem hiding this comment.
I am going to assume this init wrapper is being used for all custom docker images and the user-provided "init commands", right?
Starting with this entrypoint, we are making an assumption that the docker image has bash installed here at /bin/bash. There are a very large number of publicly available docker images that don't have bash installed at all. The "modern" docker image doesn't even have sh / or any command line shell binary installed - these are called "distroless" containers - usually designed for security and to mitigate operational overhead that a full-fat OS usually comes with. Check it out: here
Additionally, if we were to permit the bash assumption (which we probably shouldnt), the correct approach for the shebang would be #!/usr/bin/env bash. The reason for this is because env is far more commonly at /usr/bin/env than bash is at /bin/bash, so passing "bash" to env provides the installed location of bash if it exists. Portability is a must when supporting user provided args.
internal/docker/client.go
Outdated
|
|
||
| // Create wrapper script content | ||
| script := "#!/bin/bash\n" | ||
| script += "set -e # Exit on first error\n" |
There was a problem hiding this comment.
Should we assume that a user wants to exit on first error?
Catching error code in the pipe below seems generally applicable here though, as long as we make use of it.
| if (overrides.capAdd && overrides.capAdd.length > 0) count++; | ||
| if (overrides.capDrop && overrides.capDrop.length > 0) count++; | ||
| if (overrides.devices && overrides.devices.length > 0) count++; | ||
| if (overrides) { |
There was a problem hiding this comment.
Your front end implementation is largely pretty good after a quick read. Some things here and there where some existing patterns/helpers couldve been utilized, but for a first contribution to the front end, id say it's better than expected!
That being said, I think ive illustrated some non-ui changes above that marginalize the front end changes at this moment in time. So for the sake of our collective time, I won't be reviewing the ui changes until the back end concerns are addressed.
|
@nickheyer Thanks for the thorough review, appreciate you taking the time. You caught some legitimate issues and I've addressed most of them in the latest commit. On the security concern though, I want to flag something important. You're right that custom images add a new attack surface, that's the real risk here. But init commands alone can already be done through the entrypoint field without any restrictions:
I've restricted both custom images and init commands to admin-only with audit logging. So if arbitrary command execution is a concern, waiting for OAuth/OIDC doesn't solve the entrypoint issue. |
You are totally correct, the entry-point definition is currently a potential vulnerability in the docker overrides. It's even possible that you've made that less of problem locking it behind admin role too! However, "fire needs fuel" and by vetting/limiting the image/container choices that a custom entry-point would run in, we can mitigate some of that fuel. itzg/minecraft is what discopanel is specifically tailored to support. In other words, we at least know where the fires are. As previously discussed though, this isn't a "No we won't merge this" - it's really just a "Let's lock down auth with a rock solid Oauth implementation first, then circle back". Auth is really only as trustworthy as it's track record, for which Discopanel's auth layer has almost no track record. Once we overcome that, we can really expose any level of insecure functionality as long as it's within a user's permissible actions / scope. |
2 participants
This PR adds more control for the user in terms of docker container stuffs. I needed a way to use this git based backup plugin. but since it runs as a plugin in the container, i saw no way of setting up the ssh keys. Well with this PR you can now add runtime commands for any container. Because of the immense control this provides, it is locked behind auth. an admin account must be made.
Custom Image Gallery
Init Commands Gallery