search

spf13 / cobra

A Commander for modern Go CLI interactions
https://cobra.dev
Apache License 2.0
38.47k stars 2.86k forks source link

CLA bot requires to broad permissions #1555

Open WhyNotHugo opened 2 years ago

WhyNotHugo commented 2 years ago

When opening a PR, I'm prompted to sign a DCO.

There's a couple of issues with this:

  1. The bot is a "CLA" assistant, and announced in PRs that contributors have signed a CLA for the code submitted, which is untruthful; the text presented by the bot is A DCO, which is a completely different type of document. I'm not very comfortable with a bot requesting that I accept one document, but publicly announcing that I've accepted something entirely different to which I definitely don't agree (the DCO exists precisely because of how how bad CLAs are):

image

  1. The bot requires access to all my repositories, including private repositories. That's too broad of a permissions scope (you'll understand that there's many projects for which I cannot/will not grant access just for this).

image

Maybe you want to consider this integration instead, which is purpose designed for DCO checks? It's less ambiguous: https://github.com/apps/dco

(Context: #1530)

jpmcb commented 2 years ago

I like your suggestion of using the DCO GitHub app. And I believe you're right; there's no reason to give such wide, sweeping permissions to the CLA bot.

@spf13 - can you chime in here? And if you are ok with it, can you add the GitHub DCO app? (fyi - I don't have permissions to add the app to the repo)

github-actions[bot] commented 2 years ago

This issue is being marked as stale due to a long period of inactivity

spf13 commented 2 years ago

Sorry, I'm just seeing this. This is stale pretty quickly..

I don't have any issue with this. I've never heard of the DCO app though. I'll need to do some research on it.

On Sun, Feb 6, 2022 at 7:05 PM github-actions[bot] @.***> wrote:

This issue is being marked as stale due to a long period of inactivity

— Reply to this email directly, view it on GitHub https://github.com/spf13/cobra/issues/1555#issuecomment-1030946580, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABKKZE7357MTKWR7YTFJM3UZ4EFLANCNFSM5JUHW62Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

WhyNotHugo commented 2 years ago

Thanks! :)

jpmcb commented 2 years ago

@spf13 - I've done a bit more research and the CLA's text we are pointing to is a DCO, so it's not actually a valid CLA. I talked with @geekygirldawn, head of OSS at VMware, and the CLA as is appears to be invalid.

We should remove it as soon as possible in favor of a DCO check. I again recommend we install GitHub's build in DCO bot. (I cannot do that since I don't have ownership permissions on this repository).

spf13 commented 2 years ago

@jpmcb I've just installed DCO bot for the cobra-cli repo.

I'm a bit puzzled by your comment though. The CLA assistant and the DCO bot are using the exact same language. In either case the individual is agreeing to the Developer Certificate of Origin. I don't see how changing the application would make a difference here. As far as I can tell, both DCO bot and CLA assistant are 3rd party applications.

Partially due to my employment and Google's usage of Cobra I reviewed this process with Google's lead open source counsel and they actually recommended the current approach. I'm pretty confident it's sound.

I'm not against changing it and @WhyNotHugo 's reasoning against it is pretty sound. CLA assistant is pretty broadly reaching, just not clear how from a legal standpoint, it really is changing anything.

geekygirldawn commented 2 years ago

I'm not a lawyer, so maybe I'm missing some legal nuance, but I can say that in my 20+ years of open source, I don't think I've ever seen the DCO implemented in this way :)

The difference between a CLA and DCO is mostly one of scope although there are other legal differences.

I'd recommend switching to the DCO bot, especially given that the CLA Assistant is asking for overreaching access to people's accounts.

spf13 commented 2 years ago

Thanks for sharing your thoughts and expertise.

My understanding is that the DCO is a CLA, just a much lighter one than most. By definition a CLA requires an individual to certify the origin and right to contribute and that the content contributed will have the given license applied (that’s the CLA part).

Most corporate CLAs are CLAs and also copyright assignment agreements as well but rarely called that.

And while some implement it on every commit, the language in the DCO is not scoped to a commit but to an individual. It’s a developer certificate of origin, not a commit certificate of origin.

“By making a contribution to this project, I certify that…”

This language is clear that the individual certifies that any and all future contributions made from by the individual to the project from the moment they agree meet the criteria set forth.

In the case of cla assistant the individual accepts this explicitly via an online form and SSO and then makes contributions. We have a record of all individuals who certified in a database and it green lights individuals future contributions when they are in the database.

In the case of DCO bot they have no such database so they need to re-certify on each commit via a signed commit.

From a legal standpoint both are equally valid. Though I would question the signing approach a bit as the individual is never actually presented the text they are agreeing to. It’s more like signing a blank sheet of paper and stapling it to the agreement. It’s a bit weaker I think as it’s implicit and not explicit.

I do like the simplicity of the DCObot and how the record is stored as part of the commit and not in a 3rd party database. If this was available when I started these projects nearly a decade ago I’d have used it.

The challenge now is how to switch given that the database exists already and how to store that in a way that validates all that has happened prior to now.

jpmcb commented 2 years ago

Also not a lawyer 😂

I also really like DCObot's simplicity. But I'm unaware of a way to move over existing entries or the history. Maybe this is one of those "start over" scenarios for DCO?

spf13 commented 2 years ago

Ok. A short update. The more I look into this the more interesting it becomes.

My last update cited a concern with DCOBot in how the signing happens independently of the DCO. In fact, there is no indication to the individual doing the signing of the commit that the DCO exists at all or that they are agreeing to it.

Since signing commits is considered a best practice for git and not an action limited to connoting acceptance of an agreement and since the signing happens away from the language of the DCO, I'm quite concerned that the DCO bot would be legally very weak. I discussed this with a lawyer and they felt even stronger than I did citing that there's no legal precedence for signing commits as acknowledging acceptance of an agreement and that even if there was, the fact that the signing happens removed from the agreement makes it an inferred agreement which is very weak.

To state it another way, it's quite trivial for someone to sign the commit without acknowledging they are agreeing to anything... in fact, that seems to be a common path.

I'll do more diligence around this with legal counsel who has deeper experience in open source law, but at the present time, I really think that it would be quite dangerous to migrate to DCOBot for this or any other project.

jpmcb commented 2 years ago

To state it another way, it's quite trivial for someone to sign the commit without acknowledging they are agreeing to anything... in fact, that seems to be a common path.

Hmmm fascinating, I hadn't considered that ...

I'll leave this up to your discretion since the code / repo is under your personal page, but I'm glad we're taking a deeper look.

spf13 commented 2 years ago

Ok. I've done further diligence on this. Here's my findings.

  1. CLA Assistant only requires limited permissions to sign. The permissions referenced here are admin permissions. See https://github.com/cla-assistant/cla-assistant/issues/566
  2. The DCO is a CLA and the way we are using it here is perfectly acceptable.
  3. Using DCOBot on it's own puts a project in a very weak position from a legal standpoint. There's no connection between signing a commit and accepting the DCO other than the readme. It's entirely possible to sign a commit, which is a best practice, never read or even acknowledge there even is a DCO requirement.
  4. The strongest practice is to have an individual sign a CLA such as the https://www.apache.org/licenses/cla-corporate.txt and also use DCOBot or similar to certify each individual commit.

I'm not sure why CLA Assistant prompted @WhyNotHugo for such broad permissions but presuming CLA assistant works as they claim, I think we're ok here.

WhyNotHugo commented 2 years ago

The fact that the bot announces one agreement publicly, but the text shown afterwards is another one still sounds a bit weird. Especially given the fact that the text of the document is not publicly accessible.

The DCO is a CLA and the way we are using it here is perfectly acceptable.

The DCO is not a CLA. It does not grant any rights. It merely states that one owns the rights to a contribution, and that it is covered under an open source licence.

See https://opensource.com/article/18/3/cla-vs-dco-whats-difference

It's entirely possible to sign a commit, which is a best practice, never read or even acknowledge there even is a DCO requirement.

I think it would be weird for someone to sign, but later claim they didn't know what they were signing. It's entirely possible for someone to accept the text of the CLABot without reading it too.

CLA Assistant only requires limited permissions to sign. The permissions referenced here are admin permissions

It's possible that it's been fixed since, which would cover the biggest issue here.

I'm willing to give it a shot, but I honestly don't recall how to trigger the bot's signing process.

jpmcb commented 2 years ago

the text of the document is not publicly accessible.

This is something we should remedy. Let's add the CLA text to the CONTRIBUTING.md doc.

spf13 commented 2 years ago

@WhyNotHugo I think the way to trigger it is to go to the PR and there should be a link to sign the CLA and trigger the workflow.