I wonder if drone project would accept a global flag feature that disables PR builds from forked repos?
We use drone (v0.8.x) to build and test many of our open-source repositories, however we do not want drone triggering builds from forked PRs due to security concerns, mainly around the fact that it is very easy to make changes to ci scripts or drone.yml and gain access further into our systems or drone secrets.
There is also an option in the user interface (repository settings) to enable protected mode. This will block all pull requests that modify the yaml pending approval from a repository admin.
That certainly helps, but does not prevent one from changing a script ci.sh or similar that .drone.yml runs.
The logic for blocking pull requests (or any hooks for that matter) is pluggable. You can specify an endpoint that should be used to determine the gating logic [1]. You can then implement the SenderAllowed endpoint [2] that can either accept or reject the hook.
This gives you a bit more control over the logic and could allow you to simply block all pull requests by default, block all pull requests by outside contributors, or you could even augment the drone functionality and check to see if certain scripts (other than the .drone.yml) have been modified.
I think in the end this will be a bit more robust than a flag, so perhaps we can work together to flush out the design of the gating feature and get it productionalized and documented.
[1] https://github.com/drone/drone/blob/956418cae9bef51fb5855c6a7bd881ceeb4fa801/cmd/drone-server/server.go#L139:L143
[2] https://github.com/drone/drone/blob/master/plugins/sender/plugin.go
This is interesting! Thanks for sharing. I’ll look into this.
@bradrydzewski I have started looking into implementing a gating service to deny forked RPs, but if I understand correctly, there is no way to enforce that ALL repos have to be gated? There is a per-repo flag that controls whether a repo is gated or not?
What I am trying to achieve is to disable building forked PRs globally. Gating service sounds like a nice idea, but is there a way to enforce it globally?
Another issue that I found is that currently drone has no idea whether a build is for a forked repo or not.
A POST /senders/... contains model.Config{} and model.Build{} which do not seem to contain any reference whether it’s a forked repo, so using gating service does not help.
I think model.Build{} needs to include Fork bool field first. Ideas?
Another issue that I found is that currently drone has no idea whether a build is for a forked repo or not.
I would recommend using the GitHub API in your custom plugin / gating code to fetch additional metadata.
Wanted to bump this up - does anyone have a solution for this? IMO it’s a pretty critical feature to have for OSS repos using Drone, and is almost a dealbreaker. The Protected option is somewhat useful, but what’s going to stop a bad actor from, for instance, changing a unit test to fail with an error containing process.env.AWS_SECRET_ACCESS_KEY?
For reference, CircleCI gets it right:
By default, CircleCI does not build PRs from forked repositories.
and
Running an unrestricted build in a parent repository can be dangerous. Projects often contain sensitive information, and this information is freely available to anyone who can push code that triggers a build.
By default, CircleCI does not pass secrets to builds from forked PRs for open source projects and hides four types of configuration data:
- Environment variables set through the application.
- Deployment keys and user keys.
- Passphraseless private SSH keys you have added to CircleCI to access arbitrary hosts during a build.
- AWS permissions and configuration files.
but what’s going to stop a bad actor from, for instance, changing a unit test to fail with an error containing
process.env.AWS_SECRET_ACCESS_KEY?
Secrets are not exposed to pull requests by default, which prevents this exact scenario.
I think calling this a dealbreaker for all open source projects is a bit exaggerated. We use drone for hundreds of open source projects and have never needed to restrict builds based on whether or not the pull request is coming from a fork. We limit secrets to push and tag events (the default behavior) to mitigate the attack vector you described.
The signature can be an effective tool when you need to expose secrets to pull request for use with plugins, which is the most common use case (send slack notifications, upload reports, etc). The signature is typically very effective for these use cases because an attacker would need to change the yaml in order to expose the secret, which would cause signature verification to fail.
There are of course some gaps in our approach. One example use case would be running unit or integration tests against a third party service that requires an API key, but only wanting to expose this secret to intra-repository pull requests. Lack of support for this use case could be problematic for some open source projects, and is something we will consider for future releases, but should not be a dealbreaker for most projects.
Thanks for the quick response, and sorry if I wasn’t clear - I meant dealbreaker in the context of a project I was working on and considering open sourcing.
That being said, I actually missed the part in the documentation that mentions that secrets are not exposed for pull request events, which coupled with the Protected bit addresses the scenario I was concerned about. 
You’re right that there is a gap for running credentialed tasks against pull requests, but IMO that’s something that affects a very limited set of projects. In any case, good to have this thread updated with a solution.
If you would like to execute builds on PRs, but force a contributor to approve the build first, check out the following extension:
