Move privileged flag to yaml file

[bradrydzewski]

I would like to move the Privileged field to the Docker section of the yaml file:

docker:
  privileged: true
  net: host

And then change the Privileged field to Trusted:

type Repo struct {
-   Privileged  bool   `meddler:"repo_privileged"   json:"privileged"`
+   Trusted     bool   `meddler:"repo_privileged"   json:"trusted"`

Note that for compatibility reasons we should leave the database field alone (as repo_privileged) to avoid complicating our migrations.

The reasons for this change is that we are adding features (like Volume support) that require elevated privileges in order to enable. Enabling volumes, however, does not mean that someone wants to run the build in a Docker container with privileged mode. This is why I think it makes sense to start differentiating between trusted and privileged

[dave-tucker]

TL;DR: I don't think flags to elevate container permissions should be present in the YAML unless they can be encrypted.
Trust should be applied to a build not a repo (unless the flag says ALL builds are trusted).

---

I'm not sure I like exposing any of the Docker privileges in the YAML file.
I'd be concerned about somebody submitting a PR that changed the permissions and used this to compromise the host system. root in the container has pretty much the same permissions as root on your Drone server.

Yes, it's possible (like is done today) to not build PR's with these Docker privileges... but consider the case where I have tests that need elevated privileges, to access Netlink or iptables for example.
In order to keep our master branch ready to ship, we need to build every PR.
But If we build every PR, we'll expose out system to unnecessary risk.
We're in a Catch 22 situation.

My suggestion would be to use the finer grained permissions available in Docker - --cap-add, --cap-drop, and --device - which are much safer. I'm of the opinion that containers booted using these permissions should be safe enough to be used for PR's but it's up to the administrator to define what level of risk they are willing to take (this is where trust comes in).

As such, I'm 👍 for separating between trusted and privileged. I would however argue that I don't think trust is as binary as true|false when applied to a Repo. I think that the concept of trusted should be applied to a Build and based on something like #838 that would enable us to work out our trust level from the remote's permissions and/or via system wide matrix (which is also configurable).

An example policy might be:

• Don't trust a build with where the container will run with --privileged=true unless the user has Admin permission on ACME Corp GitHub organization.
• Trust all builds that will run with --cap-add=NET_ADMIN when not --net=host
• Unconditionally trust all builds that are for Private Repo's etc..

Having these in a policy will remove the assumptions on trust that are in the code today and place it all under the users's control.

If we want to allow all configuration to live in the YAML, allowing certain parts of the YAML to be encrypted with a repo-specific token would reduce the risk of unwanted tampering (making it safer to apply trust to all PR's for a given repo)

e.g

cat << EOF > foo.yml
privileged: true
net: host
volume: {{VOLUME_NAME}}:/opt/bar
EOF

drone encrypt my_repo foo.yml 
f3d7f8a9930dabf485fce74ffaad6941a554c182

Then this could be used as follows in .drone.yml

---
docker:
  encrypted: f3d7f8a9930dabf485fce74ffaad6941a554c182

Assuming the file is decrypted before it's variables are injected, you can still use private variables to capture any host specifics.

[bradrydzewski]

Thanks for weighing in. Some great points.

If we want to allow all configuration to live in the YAML, allowing certain parts of the YAML to be encrypted with a repo-specific token would reduce the risk of unwanted tampering

What about diffing the YAML files? We could compare the YAML to the prior run and halt the build for manual review and execution if changes were made. In fact, we would probably do this for all non-admin pull requests to public repositories labeled as trusted (which I believe you suggested in another thread).

Encryption is another option. We initially avoided encryption in favor of private environment variables so that you could fork a repository and supply the same named, private variables. I'm not opposed to re-thinking this strategy.

[davidak]

diffing the YAML sounds good.

[daMupfel]

Just seen this now. I think diffing would actually be the cleanest solution! 👍

[fommil]

👍 but for the reason that I want to pass specific limitations rather than just "privileged for everything"

[Cadair]

diffing the YAML and then not allowing builds for untrusted users would be a great way to improve the security of the builds in public repos. It would also be useful if the diffing could be extended to other specified files, like upload scripts etc. although this is less important.

I think once you have the ability to stop builds from non-trusted users that change the code executed on the build server the system will become much more secure.

[shawnzhu]

does this have to happen in release 0.4.0 only? I can provide a patch to master right now just like the docker tty mode