RFC: consider killing the signature #1935
Comments
|
cc @benschumacher who helped flush the original secret implementation. Wondering if you have any thoughts on this proposal. |
|
One issue with approval would be how do you know that the yaml hasn't changed from when it's approved? You'll likely want to keep a hash of the approved yaml to avoid someone getting approval and then making a malicious change. |
|
The only issue I have is not having an option from the CLI to bypass the requirement to interact with the webUI. Like what if I am making changes I know are good (tested locally with I'd like if I could still bless it from CLI if I know what I am doing is what I want to save having to do the above. But I do see the added value of not having CLI being a requirement and having a fallback to what you described above. |
|
We discussed this some internally with our security team, and the consensus was strongly in favor of the proposal (eliminating the signing). There are simply too many other ways to mess with a build, so the additional burden ends up not being desirable. |
|
@jmccann I think when you use the CLI, you can commit with |
|
My feedback for the RFC: No yaml cli signing
Re approval for yaml changes:
Re skip-verify flag:
Re secrets user interface:
Re Drone CLI optional:
I really need to spin up a 0.5 instance to try these new features to see how they work day-to-day so I can understand the benefits the new features introduce. Apologies in advance if I sound like a 0.4 stickler. |
|
We might want to require author whitelisting or explicit approval for all PR builds, regardless of whether the YAML is changed. There are a number of attack surfaces besides the YAML. This sort of thing is standard fare at Google and many other more public-facing-CI heavy orgs. Here is an example. Probably not appropriate for all projects and all orgs, but I feel like we'd want this more than only when the signature of the YAML changed. Using |
This certainly makes sense. This probably merits its own issue thread to collect use cases and requirements. Today we only check the yaml signature before exposing secrets, and this proposal continues this practice. So I think it is safe to say this proposal, if implemented, would not weaken security or introduce a new attack surface. So with that being said, I think these can be decoupled and implemented separately. I still think it is a great idea and would like to get the ball rolling on requirements gathering. |
|
Brad, after some thinking it is my hope this feature of gating the build until approval should be generalized to gating the build until some external action (webhook, ui, API).
This is a big request we get from our dev teams that use Drone, followed by automatic retries. This is one aspect Jenkins shines, and a counterpoint the devs bring up frequently when we ask them to switch
… On Feb 10, 2017, at 8:28 PM, Brad Rydzewski ***@***.***> wrote:
We might want to require author whitelisting or explicit approval for all PR builds, regardless of whether the YAML is changed. There are a number of attack surfaces besides the YAML.
This certainly makes sense. This probably merits its own issue thread to collect use cases and requirements.
Today we only check the yaml signature before exposing secrets, and this proposal continues this practice. So I think it is safe to say this proposal, if implemented, would not weaken security or introduce a new attack surface.
So with that being said, I think these can be decoupled and implemented separately. I still think it is a great idea and would like to get the ball rolling on requirements gathering.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@tonglil completely agree. The direction we are moving is to ship drone with basic, built-in implementations and allow them to be extended with restful plugins / webhooks |
|
Can you trust git though? Its too easy to fake the authorship of commits, for example this repo has a commit apparently created by @bradrydzewski https://github.com/tln/commit-test Possibly related, it would be really nice to be able to review and EDIT the drone.yml before a build. |
|
@tln the {
"action": "opened",
"number": 1,
"pull_request": {
...
"user": {
"login": "baxterthehacker",
"id": 6752317
},
},
"sender": {
"login": "baxterthehacker",
"id": 6752317
}
...
}What we are really trying to prevent here is a malicious author from sending a pull request to a public repository that attempts to expose secrets. So as long as we can get an accurate username for the user issuing the pull request, we should be ok. That being said, the This could be useful for gating and additional white-listing capabilities mentioned above by @tonglil and @gtaylor but would probably not be necessary to implement this RFC which focuses on pull request validation. We will need to research other platforms including GitLab, Gogs, Bitbucket, and Stash to see if they offer the equivalent meta-data in their payloads. {
...
"pusher": {
"name": "baxterthehacker",
"email": "baxterthehacker@users.noreply.github.com"
}
...
} |
|
If you step back to the original intent of YAML signing, I believe this is achieves a consistent outcome. One optimization that may address @jmccann's concern would be to allow admin checkins to bypass the double check, similar to how GitHub allows admins to bypass restricted branches, etc. As an aside: There's a classic "Vim vs. Emacs" or "command-line vs. GUI"-type, etc. conflict embedded within this thread, and I'm relatively indifferent to the outcome. If this eases adoption, and make the user experience better, and we are still providing the same relative security, then I think it works. |
Yea, that'd make me happy. I just want it to build right away if I'm "authorized" to make updates to the Not sure if it should be limited to only |
|
With so many other ways to cause harm, I wonder if it's worth dealing with YAML signing, period. A PR author whitelist or approval process seems to be standard fare in the wild for this reason. |
|
I was inclined to agree, but I'm not sure on retrospection. Projects like kubernetes that use author whitelist don't have an opportunity for the PR to leak out secrets. It's more so for code changes, which are more so reversible.
Are there projects that can print out secrets via PRs?
… On Feb 14, 2017, at 11:39 AM, Greg Taylor ***@***.***> wrote:
With so many other ways to cause harm, I wonder if it's worth dealing with YAML signing, period. A PR author whitelist or approval process seems to be standard fare in the wild for this reason.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
This flow isn't specific to Kubernetes, so I'm sure a sufficiently motivated person could find an example in the wild. The whitelist + explicit approval process mitigates a number of different attack vectors: secrets, code changes, Docker image changes, etc. The only change that can't hurt you is the one you don't merge or run! |
I wouldn't see it as a conflict. Instead of In regards to entering/storing secrets - I see this similar - any gui/interface the drone website exposes, can be easily used via the command line as well? |
|
For reference we use a whitelist approach for our CI system and it works well for us. https://ci.deis.io/ Our workflow is
Our biggest concern was for a random user coming in to overload our CI server and DDOSing good contributions from being made and racking up our AWS bill. Public secrets stored in CI are "ephemeral" in the sense that it's strictly used for testing purposes and we don't care too much if they're accidentally leaked because it's not a system that prod runs upon. CD operations is selectively more private and is not publicly exposed to developers unless they happen to be the ones touching the production machines. As far as security is concerned with this workflow, if a whitelisted user accidentally exposes CI creds, we roll out new ones, tell them everything is OK but please try not to do that again. If it happens to be intentional, new creds are rolled out and they're removed from the whitelist. I can see the concern if one were to be using Drone to deploy said changes directly to prod or something, but for our use case the attack surface is really small because of how we use CI. We just want to prevent the random malicious attacker from racking up our AWS bill, so a whitelist approach is what we settled on. We've been doing this for the past 4 years and so far have only had one issue in the past which was completely unintentional, so there's at least another data point that the whitelist approach is a good approach. |
|
This is my plan for an initial implementation ... When a hook is received Drone will evaluate the hook and decide if the build should execute or if approval is required. The default logic will be quite simple: if event == pull_request && yaml_changed == true && sender_has_write_access == false {
// approval required
} else {
// automatically approved
}The sole purpose of this logic is to replace the yaml signature requirement. It does not cover many of the very valid use cases discussed above to provide generic gating and approval workflows. I do believe that we can build this in such a way that a project can use the builtin logic (described above) or supply some sort of API endpoint that Drone will invoke to determine if approval is required. This endpoint would allow teams to define their own approval logic and workflows. I was planning on an So why make this pluggable and not build directly into drone? Well, because every team will have slightly different approval requirements, and I don't think it is wise at this time to try to build some crazy flexible workflow solution into Drone |
|
The initial implementation is merged into master. I will continue testing and refining the implementation over the next few days |
|
Would love to have a config option to remove that |
It does provides an important protection against a malicious pull request attempting to alter the yaml configuration file and expose secrets intended only for plugins. The below changes would be blocked and require approval, for example. pipeline:
build:
image: golang
commands:
- go build
+ - echo ${SECRET}
publish:
image: some/plugin
secret: ${SECRET}Ok, some more background for those not familiar with the history behind all of this The primary reason we implemented signing in the first place was to provide open source projects safety measures for exposing secrets to pull requests. This is not something Travis allows, for example, because secrets are injected into all steps as environment variables. This is something we can allow in Drone because secrets are not injected as environment variables; you have direct control over how secrets are exposed with interpolation. The check for yaml modifications is, as far as I know, the best way to prevent a malicious author from changing how secrets are interpolated in an effort to expose them. If it were not for this one specific use case (open source projects using secrets in pull requests) we probably wouldn't even need any of this. It might seem like a lot of effort and complexity just for this one use case. I think the overall impact is positive, however, because it has forced us to implement gating capabilities in Drone. I hope that we can expand these capabilities (via plugins) to enable some really cool workflows, similar to the kubernetes workflow.
If there are no secrets exposed to your pull request, or if the secrets all have Note that we'll also be making all of this pluggable, so you will be able to swap out the default implementation with your own blocking / gating rules custom to your team and workflow. |
|
Preventing changes to .drone.yml is good, but it's only so helpful if your step's command section executes something outside of the .drone.yml. They can just change the script that the step calls and achieve the same goal without changing the .drone.yml hash. |
|
@gtaylor yes true, this primarily protects secrets intended only for plugins. For pull requests, this probably ends up being your slack, gitter, coveralls token, etc. |
|
It's certainly some protection. I'm definitely not meaning to poopoo the development, but the running arbitrary code execution by untrusted users vector is super scary. If I don't modify .drone.yml but change the repo contents, I may be able to:
To sum up the gist of my point: This is one means of protection, but I wonder if a basic org/user whitelist + a simple manual approval step would make this hashing unnecessary. Approvals could even be handled through a button on a pending build in the Drone UI if we wanted to skip issue integration initially. |
|
@gtaylor I definitely agree with everything you are saying. There is a lot of history with secrets, and not everyone reading this thread will have this context. I just want to provide some context and insight into what the heck I was thinking with the initial implementation, while addressing your very valid points.
The initial goal was to remove the signature while still providing protection for open source projects exposing secrets to pull requests (this would otherwise impact me personally). I think the initial implementation achieves the goal of providing the same level of security as the prior implementation, but with less burden. Now that we have an initial implementation we can start iterating and improving
I am in absolute agreement. The introduction of gating and blocking builds gives us the ability to start protecting against these vectors, which was not previously possible. This mirrors the feedback from @bacongobbler further up in the thread.
This could definitely be the case. This is why I want to make the implementation pluggable. It gives everyone the flexility to create and hopefully share different strategies for gating. We can then take the best strategy and implement in Drone as the default. The reason I didn't take this approach with the initial implementation was that the requirements are not flushed out for how a whitelist would work and be managed, and I was skeptical that we could come up with an initial implementation generic enough to make everyone happy. My skepticism came from my work on LGTM where teams had some pretty complex workflow approval requirements The initial implementation does whitelist users with write or admin access to the repository. There is no solution for individuals with read access and community members, so there is definitely room for improvement here. I've created a new issue where we can discuss the whitelist more formally. See #1971
Yep, this is how the initial implementation works
|
|
moving further discussion to #1971 |
Signing the YAML is good for security but (like most security measures) is bad for usability. I would like to consider an alternate approach that improves usability without sacrificing security.
Problem
One issue with the signature is that it needs to be re-generated every time the YAML is changed and the committed to your revision control system. Many feel this adds burden to the developer workflow. They generally recognize the security benefits, but ask themselves if there is a better way.
Second is the fact that signing the YAML requires the command line utility. The command line utility is productive once you start using it, however, there are many individuals that want to use Drone but don't spend much time at the command line. Installing and learning a command line tool increases their learning curve.
Third is the fact that the signature is a major support burden. People skim over the documentation and wonder why their secrets don't work when the YAML signature fails or does not exist. Fixing this would probably increase my quality of life ~ 10%
Solution
When a user commits changes the YAML from the head and base branch are compared. If the YAML file changes and the user is not a member of the repository (write or admin access) the state of the build is marked as
approval_required: true. The build will not execute until it is approved by a project member.Drone should inform the project members that approval is required. Options include:
Clicking the link would bring the project member to the build in Drone with a button to approve the changes. If the project member presses the approve button, the username is recorded in the
approved_byfield, and theapproved_dateis set to the current timestamp.We should also record the
approved_shato prevent a pull request from being approved and then a hostile author from pushing an updated YAML intended to do damage or expose secrets (credit to @donny-dont). EDIT since each update to the pull request is a separate build number in drone, this might not be an issue, since subsequent YAML changes would require additional approvals.Secrets
This should not require any changes to secrets. Secrets can still be managed using the command line utility and user interface (coming soon) and can be limited to specific event types (e.g. don't expose for pull request). The skip-verify flag could also be used to instruct Drone to skip the approval process when the Yaml is changed.
Conclusion
I believe this proposal provides the same security as the existing solution while reducing the burden of project members constantly having to re-sign the yaml file. This should also reduce the learning curve by making the Drone CLI optional in the developer workflow.
cc @jmccann who tells me if my ideas for secrets are crazy. What do you think?
cc @tonglil whose comments encouraged me to look at the problem from a different angle and explore alternate solutions with a focus on usability.
The text was updated successfully, but these errors were encountered: