Skip to content

Commit

Permalink
fix(security): remove mounting of docker.sock into worker job containers
Browse files Browse the repository at this point in the history
  • Loading branch information
@jkuri
jkuri committed Jun 16, 2021
1 parent fdfc4ff commit 1049f92
Showing 1 changed file with 0 additions and 1 deletion.
1 change: 0 additions & 1 deletion worker/docker/docker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ func createContainer(cli *client.Client, name, image, dir string, cmd []string,

mounts := []mount.Mount{
{Type: mount.TypeBind, Source: path.Join(dir), Target: "/build"},
{Type: mount.TypeBind, Source: "/var/run/docker.sock", Target: "/var/run/docker.sock"},
}

return cli.ContainerCreate(context.Background(), &container.Config{
Expand Down

6 comments on commit 1049f92

@inkvizitor68sl
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkuri
Hi. It is one of the basic features needed for modern CI to mount /var/run/docker.sock inside job container - otherway docker build/run/etc can't be executed inside CI jobs.
Usage of TCP docker socket is probably even less secure - and too much configuration needed.

Maybe it can be option with default false value?

@jkuri
Copy link
Contributor Author

@jkuri jkuri commented on 1049f92 Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is a security issue, that's why removed. But I agree there should be option on the UI where you can enable mounting /var/run/docker.sock for specific repository, but default as false value, just as @inkvizitor68sl said.

/cc @MrCyjaneK

@MrCyjaneK
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, I'll have some free time now, so I can sit on the fork and make that mount thing usable

@inkvizitor68sl
Copy link
Contributor

@inkvizitor68sl inkvizitor68sl commented on 1049f92 Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@jkuri @MrCyjaneK
Mhm. Not sure, that it is good for security to make that only a repo setting.

From ops point of view - it looks like worker setting in first order. If worker is isolated enough from other hosts so even root access here is not dangerous much - than we can enable this setting on this worker.
But if it is only repo setting - any CI admin can get root on random agent host, even if it is not acceptable for current CI infrastructure.

(Until worker tags not implemented - this settings have to be same across all agents ofc, because any job chooses random agent)

I have no real idea, how to make docker.sock secure enough - remote socket has same problems, but if it is misconfigured - security problem can become remotely executable =) Socket access means that someone with that access can run anything on host (and root commands via nsenter too), so it is actually question about dockerd and firewall/VLAN configuration, and it is really important to understand.

Also recomendation to try buildkit or faas-cli before using socket mounting is good.
But socket mount is still only way for CI tests, where jobs really runs bunch of containers to create "test env" in-place, if k8s (or similiar) not available.

@jkuri
Copy link
Contributor Author

@jkuri jkuri commented on 1049f92 Jun 23, 2021

Choose a reason for hiding this comment

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

@inkvizitor68sl can you define or specify requirements and features needed for worker tags in more detail please? Also configuration format and the requirements on the UI would be welcome. Then we can start working on that..

@MrCyjaneK
Copy link
Contributor

@MrCyjaneK MrCyjaneK commented on 1049f92 Jun 25, 2021 •
edited by jkuri
Loading

Choose a reason for hiding this comment

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

In my opinion there should be default tags based on

So we will be able to run a build on aarch64 linux machine with more than 8 cores, and more than 16GB of ram with some custom tag.

Also there should be an unique ID that will allow to assign one specific worker to a repository - then when for example docker.sock is mounted, and somebody submit a pr with exploit it will affect only one worker.

Please sign in to comment.