Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #401240 +++ As part of implementing CLAs (see bug 401236), we will require a git trigger which will reject any contribution where the email address of the author field does not match a CLA on file, or a known committer. Furthermore, we need to verify that the contributor and/or the committer has Signed-off-by their contributions.
http://www.clahub.com/ Came across this link in a blog post that Chris A linked on twitter. http://julien.ponge.org/blog/in-defense-of-contributor-license-agreements/ As I've mentioned previously GitHub doesn’t provide a way to block a push since all the hooks they expose are post-push. CLAhub appears to be using the pull request as the opportunity to verify CLAs, we might be able to leverage this possibly for our CLAs or at least look at the workflow. Food for thought at least.
I've spent some time looking at the details of the github api and as Nathan determined, the pull request provides the only opportunity to do external validation. The mechanism used by clahub.com is close to what is needed. proposed approach ----------------- We create a web service that listens for a POST from github on a new pull request in a github-hosted project owned by the eclipse organization. Using the api, the service walks the list of commits in the pull request, building a list of authors. It then verifies that each has signed the CLA and builds a suitable comment/status so that the project owner can decide whether the merge should go ahead. steps: ----- - create application permission for webhook service in eclipse organization on github. The generated token will be used for authentication to manipulate the github api. - eclipse-maintained web service starts up and uses api to check and, if needed, create a web hook in each eclipse organization github project. The hook is set to fire on pull_request or status events (status determines the appearance and merge-ability of the pull request). http://developer.github.com/v3/repos/hooks/ - on a new pull request, get commits then get author of each commit http://developer.github.com/v3/pulls/ http://developer.github.com/v3/pulls/#list-commits-on-a-pull-request -verify CLA status of committers, matching on github == eclipse account email. - walk pull request status history to see if third-party applications (e.g. Travis CI) already have a non-success status. http://developer.github.com/v3/repos/statuses/#list-statuses-for-a-specific-ref - put a status on the pull request indicating success or failure with details link pointing back to eclipse cla url, taking care not to 'improve' the status of a pull request from the last step http://developer.github.com/v3/repos/statuses/#create-a-status tested so far (sandbox at https://github.com/zidge/hooktest): ------------------------------------------------------------ -application permission and authority to set status on commits withing a pull request -generic web hook create, update (PATCH) with pull_request and status event support -listening for web hook post -walking pull request commits and extracting authors -reading, creating and updating status -verified that commit to a commiter's fork by an unknown author that is part of the commiter's pull request is tracked through to pull request commits list (and so can be tested for cla status) problems: -------- -status is set on commits, not on pull requests. The entire pull request gets the last status set on any commit it contains. We therefore must always walk all commits for status. -other services use status and the state they set should be honoured, perhaps by passing through any link to a details page and explaining in our CLA status why the pull request is still not marked 'good to merge'. I will look into how other services (like Travis) handle this. to be tested: ------------ -integrating eclipse CLA badge display in github comment. -responding to status changes on pull request -overwrite and inclusion of third party status updates in pull request comment -sample php web service responding to hook questions: --------- -where can I find the code that serves the CLA badge? Is it part of a larger api? https://projects.eclipse.org/api/cla/image/ future ideas and options: ------------------------ -talk to github about pre-commit hooks to replicate the Gerrit mechanism for checking contributors. -create an eclipse-specific web hook in the https://github.com/github/github-services project to simplify and formalize hook configuration. If you look at https://github.com/github/github-services/tree/master/lib/services you'll see a long list of existing services performing various roles. The alternative is just to maintain the hook as a generic web hook with specific event support, however, a service specific hook can have a support url and can still be selected via api.
Created attachment 235312 [details] github pull request cla test success Shows how status update is reflected in the pull request view.
Created attachment 235313 [details] github pull request cla test failure Shows how a cla test failure would be reflected in the github pull request ui.
Zak, thanks for the analysis and the work done so far. While we review, perhaps Nathan can help us out with this question: (In reply to Zak James from comment #2) > questions: > --------- > -where can I find the code that serves the CLA badge? Is it part of a larger > api? > https://projects.eclipse.org/api/cla/image/ Since we use Drupal for the projects.eclipse.org site, it is possible that the code is not in an "open" repository. If that's the case, we can give you specific access to it in order to work on the Foundation's web api.
(In reply to Denis Roy from comment #5) > Zak, thanks for the analysis and the work done so far. While we review, > perhaps Nathan can help us out with this question: > > (In reply to Zak James from comment #2) > > questions: > > --------- > > -where can I find the code that serves the CLA badge? Is it part of a larger > > api? > > https://projects.eclipse.org/api/cla/image/ > > > Since we use Drupal for the projects.eclipse.org site, it is possible that > the code is not in an "open" repository. If that's the case, we can give > you specific access to it in order to work on the Foundation's web api. Indeed this code is located in our PMI repository on foundation. /gitroot/drupal/custom/pmi Keep in mind that this is a large repo that contains all of the PMI code base. Is there something particular you're interested in checking out, perhaps it makes sense to move this module out of this repository.
I don't see any immediate for me to have access to the repository. I was just interested to know if there is a service that can do the cla check or signed-off-by check on an email address and return text, xml or json instead of an image. I want to use it in a sample web service that will talk to a sandbox project I created on github.
> proposed approach > ----------------- > We create a web service that listens for a POST from github on a new pull > request in a github-hosted project owned by the eclipse organization. Using > the api, the service walks the list of commits in the pull request, building > a list of authors. It then verifies that each has signed the CLA and builds > a suitable comment/status so that the project owner can decide whether the > merge should go ahead. +1 Let's do it. One addition though: we must also verify that the Signed-off-by: is present, and that the email address matches that of the author. Please see: http://wiki.eclipse.org/Development_Resources/Contributing_via_Git#Signing_off_on_a_commit and: http://wiki.eclipse.org/Development_Resources/Handling_Git_Contributions#Overview > future ideas and options: > ------------------------ > -talk to github about pre-commit hooks to replicate the Gerrit mechanism for > checking contributors. Yes, please. The closer we can insert ourselves into the commit/push mechanism, the better we can validate.
(In reply to Zak James from comment #7) > I don't see any immediate for me to have access to the repository. I was > just interested to know if there is a service that can do the cla check or > signed-off-by check on an email address and return text, xml or json instead > of an image. I want to use it in a sample web service that will talk to a > sandbox project I created on github. Currently the only api exposed is that for the image. I can write a handler for this that i'd prefer to be in JSON this afternoon. Would a simple 0/1 or TRUE / FALSE be enough for your purposes if I were to provide a /api/cla/%emailaddress% url?
That would be great if it isn't a big deal. The perl script that Denis provided which does the Gerrit cla check talks to ldap so I'm not sure if that is the way to go for a final implementation. I'm open to suggestions.
Denis, with respect to Signed-off-by, I read through the wiki articles but I'm not clear on how to apply it in the case of a pull request workflow. Should each commit within the contribution be checked for the Signed-off-by footer or is it the merge commit that needs it?
(In reply to Zak James from comment #10) > That would be great if it isn't a big deal. The perl script that Denis > provided which does the Gerrit cla check talks to ldap so I'm not sure if > that is the way to go for a final implementation. I'm open to suggestions. Turns out I'd already written that function! Gotta love bonus code! Here's an example http://projects.eclipse.org/api/cla/validate/nathan@eclipse.org
(In reply to Zak James from comment #10) > The perl script that Denis > provided which does the Gerrit cla check talks to ldap The perl script has the advantage of running within our firewall. Web API is what we'll use externally. Thanks for asking. (In reply to Zak James from comment #11) > Denis, with respect to Signed-off-by, I read through the wiki articles but > I'm not clear on how to apply it in the case of a pull request workflow. > Should each commit within the contribution be checked for the Signed-off-by > footer or is it the merge commit that needs it? I'm hoping Wayne can help us interpret the Wiki: "Contributor creates a Git commit: Author email address must set to the address associated with the author's Eclipse Foundation account; ===> This may not apply on GitHub, since we can't reasonably expect all GitHub users to create an Eclipse Foundation Account The Contributor must signed-off-by the Contribution, indicating that they are in compliance with the Eclipse Foundation Contributor’s Certificate of Origin; ===> Although Jane GitHub is technically committing into her own GitHub repo, if she will be making a Pull Request to an Eclipse Foundation project, she needs to Sign-of-by her contribution to indicate that she is in compliance with the Eclipse Foundation Contributor’s Certificate of Origin. Eclipse Committers would certainly need to make this clear to/assist the would-be contributor. I am hoping that your hook can warn Committers that one or more commits in a pull request are not signed-off-by. The "Signed-off-by" credentials must match the author credentials; ===> This is the same at Eclipse.org. One contributor can not Signed-off-by someone else contribution. Additional authors may be listed by including "Also-by: {Author Name} <{email}>" entries in the commit comment; " Does that help at all?
(In reply to Denis Roy from comment #13) > ===> This may not apply on GitHub, since we can't reasonably expect all > GitHub users to create an Eclipse Foundation Account They'll need an EF account if they're going to sign a CLA. I'm not sure how much wiggle room we have here. I think that we just need to be able to map a contributor's identity to the CLA, not any specific email address. But I'll have to confirm.
(In reply to Denis Roy from comment #13) > (In reply to Zak James from comment #10) > > The perl script that Denis > > provided which does the Gerrit cla check talks to ldap > > The perl script has the advantage of running within our firewall. Web API > is what we'll use externally. Thanks for asking. > > > (In reply to Zak James from comment #11) > > Denis, with respect to Signed-off-by, I read through the wiki articles but > > I'm not clear on how to apply it in the case of a pull request workflow. > > Should each commit within the contribution be checked for the Signed-off-by > > footer or is it the merge commit that needs it? > > I'm hoping Wayne can help us interpret the Wiki: > > "Contributor creates a Git commit: > > Author email address must set to the address associated with the author's > Eclipse Foundation account; > > ===> This may not apply on GitHub, since we can't reasonably expect all > GitHub users to create an Eclipse Foundation Account > > The Contributor must signed-off-by the Contribution, indicating that they > are in compliance with the Eclipse Foundation Contributor’s Certificate of > Origin; > > ===> Although Jane GitHub is technically committing into her own GitHub > repo, if she will be making a Pull Request to an Eclipse Foundation project, > she needs to Sign-of-by her contribution to indicate that she is in > compliance with the Eclipse Foundation Contributor’s Certificate of Origin. > Eclipse Committers would certainly need to make this clear to/assist the > would-be contributor. I am hoping that your hook can warn Committers that > one or more commits in a pull request are not signed-off-by. > That is certainly possible using the same mechanism as we do for CLA compliance, just that the check can be done right in the web service because it just needs to verify that there is a Signed-off-by footer and that the commit author matches it. > > The "Signed-off-by" credentials must match the author credentials; > > ===> This is the same at Eclipse.org. One contributor can not Signed-off-by > someone else contribution. > > Additional authors may be listed by including "Also-by: {Author Name} > <{email}>" entries in the commit comment; > " > > Does that help at all? That clears up my understanding of everything except the role of the Also-by authors. Do we need to verify that they have signed the CLA? Are they committers or just other contributors to the relevant commit?
(In reply to Wayne Beaton from comment #14) > (In reply to Denis Roy from comment #13) > > ===> This may not apply on GitHub, since we can't reasonably expect all > > GitHub users to create an Eclipse Foundation Account > > They'll need an EF account if they're going to sign a CLA. That was what I was planning to accommodate. In the web service we can differentiate between the 2 failure modes and provide an explanation and a link to the relevant eclipse.org page: no EF account => link to EF sign-up EF account but no CLA => link to EF CLA form/documentation I guess I'm assuming we can test for EF account easily. > > I'm not sure how much wiggle room we have here. I think that we just need to > be able to map a contributor's identity to the CLA, not any specific email > address. But I'll have to confirm.
(In reply to Nathan Gervais from comment #12) > Here's an example > http://projects.eclipse.org/api/cla/validate/nathan@eclipse.org Nathan, can we have the same three states the CLA Image has, to cover the case where an account is not an account? "TRUE" - email in is a valid user and has CLA signed "FALSE" - email in is a valid user and has not a CLA signed "" - email in is not a valid user
Pulling in Thanh and Matthias in case they see something we're not.
(In reply to Denis Roy from comment #18) > Pulling in Thanh and Matthias in case they see something we're not. Did you check that you prevent to accept commits with a forged committer or author identity ? I tried and was able to push a change to github which uses mickey mouse as author and committer identity [1]. In this case GitHub flags this person as unverified. I also tried to impersonate a colleague (with his permission) who really has a github account, when pushing that to my repository GitHub flags this person as verified [2]. This means you need to prevent that someone can sneak in changes under the identity of another person or even a committer of the Eclipse project he tries to contribute to. [1] https://github.com/msohn/HelloWorld/commit/72ecdd59a7581178cf1f29d1754e48d92cf711df [2] https://github.com/msohn/HelloWorld/commit/67e49fccfd2c13c0261b7347275bb39eef0c56eb
(In reply to Denis Roy from comment #17) > (In reply to Nathan Gervais from comment #12) > > Here's an example > > http://projects.eclipse.org/api/cla/validate/nathan@eclipse.org > > Nathan, can we have the same three states the CLA Image has, to cover the > case where an account is not an account? > > "TRUE" - email in is a valid user and has CLA signed > "FALSE" - email in is a valid user and has not a CLA signed > "" - email in is not a valid user Sorry I didnt see this sooner, Outlook decided that this bug was too active and marked it as spam. Tri-State is now enabled for this api.
> I tried > > I also tried Matthias, have you installed Zak's hooks into your repositories?
There isn't anything to install, but we eventually can configure the hook to watch for any github event and do validation. I'm not sure yet how to avoid the spoofing that Matthias describes, but I'm looking into it and will report back here. Because we have only post-commit hooks available, I don't think it will be possible to reject a spoofed push or spoofed credentials within a pull request, but we will be able to add a comment and status that makes the person responsible for the merge aware that there is something wrong. (In reply to Denis Roy from comment #21) > > I tried > > > > I also tried > > Matthias, have you installed Zak's hooks into your repositories?
(In reply to Denis Roy from comment #21) > > I tried > > > > I also tried > > Matthias, have you installed Zak's hooks into your repositories? no, I did not install it. Can you enforce that contributors have installed it ?
(In reply to Zak James from comment #22) > There isn't anything to install, but we eventually can configure the hook to > watch for any github event and do validation. I'm not sure yet how to avoid > the spoofing that Matthias describes, but I'm looking into it and will > report back here. > > Because we have only post-commit hooks available, I don't think it will be > possible to reject a spoofed push or spoofed credentials within a pull > request, but we will be able to add a comment and status that makes the > person responsible for the merge aware that there is something wrong. maybe we could enforce that pull requests only bring in new commits from the user owning the respective fork
(In reply to Matthias Sohn from comment #23) > (In reply to Denis Roy from comment #21) > > > I tried > > > > > > I also tried > > > > Matthias, have you installed Zak's hooks into your repositories? > > no, I did not install it. Can you enforce that contributors have installed > it ? If the projects receiving commits and pull requests are under the github eclipse organization, we have the authorization we need to install the webhook via the API and I will provide a sample script for this. If we don't have that, we would need project owners to install the hook via github's services settings and also grant eclipse an auth token. Then, any pull requests and/or pushes would trigger action from our web service.
(In reply to Zak James from comment #25) > (In reply to Matthias Sohn from comment #23) > > (In reply to Denis Roy from comment #21) > > > > I tried > > > > > > > > I also tried > > > > > > Matthias, have you installed Zak's hooks into your repositories? > > > > no, I did not install it. Can you enforce that contributors have installed > > it ? > > If the projects receiving commits and pull requests are under the github > eclipse organization, we have the authorization we need to install the > webhook via the API and I will provide a sample script for this. If we don't > have that, we would need project owners to install the hook via github's > services settings and also grant eclipse an auth token. Then, any pull > requests and/or pushes would trigger action from our web service. but contributors will push changes to their own fork which is not under the github eclipse organization, right ?
(In reply to Matthias Sohn from comment #26) > (In reply to Zak James from comment #25) > > (In reply to Matthias Sohn from comment #23) > > > (In reply to Denis Roy from comment #21) > > > > > I tried > > > > > > > > > > I also tried > > > > > > > > Matthias, have you installed Zak's hooks into your repositories? > > > > > > no, I did not install it. Can you enforce that contributors have installed > > > it ? > > > > If the projects receiving commits and pull requests are under the github > > eclipse organization, we have the authorization we need to install the > > webhook via the API and I will provide a sample script for this. If we don't > > have that, we would need project owners to install the hook via github's > > services settings and also grant eclipse an auth token. Then, any pull > > requests and/or pushes would trigger action from our web service. > > but contributors will push changes to their own fork which is not under the > github eclipse organization, right ? Right, but then they create a pull request against the EF project and we have the opportunity to validate all those commits. What I'm working on determining at the moment is whether it's possible to detect the forged email at that stage via the github api.
(In reply to Zak James from comment #25) > If the projects receiving commits and pull requests are under the github > eclipse organization, Just to confirm: yes, all the "official" Eclipse GitHub repos will be under our organization. > we have the authorization we need to install the > webhook via the API So the hook must be applied to each repo? Just looking to confirm where it lives. > and I will provide a sample script for this. Great; thanks.
(In reply to Denis Roy from comment #28) > (In reply to Zak James from comment #25) > > If the projects receiving commits and pull requests are under the github > > eclipse organization, > > Just to confirm: yes, all the "official" Eclipse GitHub repos will be under > our organization. > > > we have the authorization we need to install the > > webhook via the API > > So the hook must be applied to each repo? Just looking to confirm where it > lives. Yes, the hook will need to be set on each repo. We can either get the list of repos under the EF organization via the api, or maintain a list of urls separately on the web service side. > > > and I will provide a sample script for this. > > Great; thanks.
(In reply to Zak James from comment #27) > (In reply to Matthias Sohn from comment #26) > > (In reply to Zak James from comment #25) > > > (In reply to Matthias Sohn from comment #23) > > > > (In reply to Denis Roy from comment #21) From my testing, it appears that the scenario that Matthias described, a third party forging signed-off-by and commit user and then having that commit become part of a pull request from a non-EF fork to the EF organization project) is undetectable. If the forged user/email is a valid github user everything looks ok in the pull request. The git solution to this problem is to sign the commit, including the commit message text, with a GPG key (git -S commit). That would mean we would need to collect and maintain a database of keys on the web service side to use to verify every commit in every pull request. If the commit author did not match the user associated with the stored key, we would reject the parent pull request. This requires quite a bit of infrastructure and precludes any code from non-EF members being contributed. I'm not really sure what to do on this front and would appreciate any suggestions. Commit security is dealt with in some detail in this often-referenced blog post: http://mikegerwitz.com/papers/git-horror-story.html
(In reply to Zak James from comment #30) > From my testing, it appears that the scenario that Matthias described, a > third party forging signed-off-by and commit user and then having that > commit become part of a pull request from a non-EF fork to the EF > organization project) is undetectable. If the forged user/email is a valid > github user everything looks ok in the pull request. At this point, I think we need to have a conversation about whether we want to protect against outright fraud.
(In reply to Zak James from comment #30) > (In reply to Zak James from comment #27) > > (In reply to Matthias Sohn from comment #26) > > > (In reply to Zak James from comment #25) > > > > (In reply to Matthias Sohn from comment #23) > > > > > (In reply to Denis Roy from comment #21) > > From my testing, it appears that the scenario that Matthias described, a > third party forging signed-off-by and commit user and then having that > commit become part of a pull request from a non-EF fork to the EF > organization project) is undetectable. If the forged user/email is a valid > github user everything looks ok in the pull request. but maybe we could prevent this by limiting pull requests to only contain changes from the person owning the repository to pull from. I.e. if Joe Developer owns http://github.com/joedev/egit we could enforce that his pull requests for pulling his contributions for egit only contain commits using his github identity (Joe Developer <joe.developer@test.mail> if that's his name and verified email registered at github) for both committer and author.
Now that Zak has implemented the hook, we need to work on the backends and place the hook on our Organization/repos.
(In reply to Denis Roy from comment #33) > Now that Zak has implemented the hook, we need to work on the backends and > place the hook on our Organization/repos. I've been doing some cleanup work but there are a few things that we need to finalize: 1. Should the pull request be automatically closed if there is a validation failure? Right now it just gets the merge warning. 2. Should owners and/or committers be emailed about validation problems? I've refactored the status handling code and improved the display of the status details (which works around a 140 char github limit on status messages) https://github.com/zidge/eclipse-webhook/commit/15a4d6543768a7a32cf375e978c4d614d12ee65b As we discussed, I also implemented a simple page displaying the github api access count. https://github.com/zidge/eclipse-webhook/commit/2e2868b0e3f7444f9d4605779f711f99a9a49ccf todo: include status history in status details page, implement outcome of 1. and 2.
As we discussed this morning, Zak will focus on making the error checking and overall functionality more robust. But we did get it working on dev.eclipse.org, so we're almost there. A few more TODOs were discussed as well: - we'd prefer the two environment vars were as config elements instead - temp file location should be a config. We don't allow writing to /tmp - we're expecting the "services" directory should be the only directory serving content. That will place everything else, including config files, out of the way. Once those four items are done, we'll call success on this. I will open up other bugs for additional features, and I'll make them all depend on this bug. (In reply to Zak James from comment #34) > 1. Should the pull request be automatically closed if there is a validation > failure? Right now it just gets the merge warning. The short answer was "no", but we may want to generate some email. More to come in other bugs. > 2. Should owners and/or committers be emailed about validation problems? As above. Nice work so far, Zak.
The hook itself is complete and has been tested to work. We just need to install it on our repos. This will be done shortly. https://github.com/eclipse/eclipse-webhook
The hook is installed on the GitHub repos that are currently the main dev repos for Eclipse projects. A cron job runs every day at 9:00am to install the hooks. We've also tested that the hook install process doesn't interfere with other hooks that may or may not be in place for the projects' needs. Mission accomplished! Thanks Zak for the great work on this.
