Don't run invocation image as root #478
Conversation
internal/commands/cnab.go
Outdated
|
|
||
| func isDockerHostLocal(dockerEndpoint context.EndpointMetaBase) bool { | ||
| host := dockerEndpoint.Host | ||
| return host == "" || host == "unix:///var/run/docker.sock" || host == "npipe:////./pipe/docker_engine" |
There was a problem hiding this comment.
I think you should test for prefixes of unix:// or npipe:// rather than the full string since I could e.g. run dockerd with -H unix://my/socket (if I were feeling obtuse).
Codecov Report
@@ Coverage Diff @@
## master #478 +/- ##
==========================================
- Coverage 69.53% 69.52% -0.01%
==========================================
Files 51 50 -1
Lines 2629 2553 -76
==========================================
- Hits 1828 1775 -53
+ Misses 569 543 -26
- Partials 232 235 +3
Continue to review full report at Codecov.
|
internal/commands/cnab.go
Outdated
|
|
||
| host := dockerEndpoint.Host | ||
| if strings.HasPrefix(host, "unix://") { | ||
| socketPath = strings.TrimPrefix(host, "unix://") |
There was a problem hiding this comment.
is the lack of an npipe:// alternative expected here?
There was a problem hiding this comment.
I mixed up host and socketPath so I see now that npipe://foo as input does return {true, "/var/run/docker.sock"} as expected (not {true, "npipe://foo"} like I thought).
| }{ | ||
| { | ||
| name: "kubernetes-orchestrator", | ||
| targetContextName: "target-context", |
There was a problem hiding this comment.
I'm unsure what populates this context (or maybe I misunderstand how this works), I can't find it here and my grep in the existing source tree isn't showing it. In any case do we not need to be sure we test at least unix:// and tcp:// as well as perhaps some invalid syntax?
There was a problem hiding this comment.
These tests only check for the 2 hardcoded conditions (kubernetes target and default context). I've split out the socket path resolution into a helper method and added some tests for that. The e2e tests should ensure the rest works for both unix:// and tcp:// (@simonferquel please can you confirm this?)
| if err != nil { | ||
| return err | ||
| } | ||
| driverImpl, err := prepareDriver(dockerCli, bind) |
There was a problem hiding this comment.
I wonder if there is a way prepareDriver could handle the requiredClaimBindMount call intenrally (e.g. if it had a few more args) or if this pattern happens enough times to warrant a hlper?
There was a problem hiding this comment.
the reasoning behind this layout is the "install" command gets the target orchestrator from its opts while the other commands get it from the claim store
|
Note in order to fix the vendoring this PR also contains the changes in #482 |
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Testing with local and tcp hosts Signed-off-by: Nick Adcock <nick.adcock@docker.com>
Signed-off-by: Nick Adcock <nick.adcock@docker.com>
- What I did
Made the invocation image run as a non-root account except where root permissions are required.
Root permissions are required when running through a unix socket/named pipe rather than TCP. In this case the unix socket is mounted and the container is run as root.
- How I did it
Updated the invocation image's docker file to create a "cnab" system user and run as that by default.
On commands, check whether the target context/orchestrator requires the socket binding by checking the stack orchestrator and context metadata's endpoint. If the orchestrator is not kubernetes and the endpoint is empty, the unix socket, or windows named pipe, then the socket needs to be bound.
When preparing the driver, mount /var/run/docker.sock and override the cnab user with "0:0" if binding the local socket is required.
- How to verify it
E2E tests have been updated to test the docker app lifecycle with and without the bind mount.
- Notes
Because of issues with the handling of the default context the bind mount/root permissions are assumed to be required when on the default context.