OCA / oca-github-bot

The GitHub Bot of the Odoo Community Association (OCA)
MIT License
40 stars 57 forks source link

[RFC] automatically run workflow if PR seems safe #169

Closed legalsylvain closed 2 years ago

legalsylvain commented 2 years ago

Is your feature request related to a problem? Seeing this PR, the workflow is awaiting maintainers manual actions. On each new commit, maintainers should again confirm the workflow/ This is a security added by github to avoid malicious PR that could use github actions for unwanted actions. (mainly cryptocurrency mining if I understand correctly). The security is enabled for each PR made by a "newcomer" contributor. (First PR, AFAIU).

Describe the solution you'd like The bot could (maybe ?), (don't know if the github api exists for that purpose) :

Describe alternatives you've considered No alternative.

@pedrobaeza, @sbidoul : what do you think ?

thanks for your review.

StefanRijnhart commented 2 years ago

Great idea! Perhaps a PR can be considered safe if a CLA is found for the github user?

pedrobaeza commented 2 years ago

I see the concept correct, but I can't say how technically that can be done. It should be Stéphane who tells, and I'm not sure there's direct connection from bot to OCA instance.

sbidoul commented 2 years ago

Yes, this GitHub policy of having manual approval for each new contributor is a pain... They put that in place to counter crypto miners... sigh.

I think I've seen somewhere they have a mechanism to soften the pain, but I'm not sure and I've not investigated recently.

It would be a pity to have custom code to handle that. In the end, this issue is particularly acute in OCA because we don't have enough maintainers who care about PRs.

legalsylvain commented 2 years ago

Hi @StefanRijnhart and @pedrobaeza. CLA check is a good thing, bug I don't see the relation with the github workflows. Regarding CLA, there is a current PR but the objective is other : https://github.com/OCA/oca-github-bot/pull/31 (here is the message displayed if the CLA is not signed)

For the current topic, github introduced a security to prevent malicious code to be executed on the github CI. So, the objective is to make sure that the PR is safe, and if yes, to automatically run workflows. indeed, people who signed CLA can be considered as safe. (although ? ...) but people who didn't signed CLA can also make valid PR, and CI should not be blocked for a CLA problem (in my opinion). So, I think that we should ensure that the PR is safe, and that is a technical check. My first impression is that if the ".github" folder is not modified in the current PR, a user cannot introduce malicious code, and so we should run automatically the CI.

sbidoul commented 2 years ago

My first impression is that if the ".github" folder is not modified in the current PR, a user cannot introduce malicious code, and so we should run automatically the CI.

Technically, they could inject a crypto miner disguised as an Odoo addon :shrug:

pedrobaeza commented 2 years ago

Yes, I think we can't control such approval process, as it's GitHub who is putting for free their resources, and they don't want to be improperly used.

legalsylvain commented 2 years ago

I think I've seen somewhere they have a mechanism to soften the pain, but I'm not sure and I've not investigated recently.

AFAIK, there are 3 policies at the @oca organization :

  1. Require approval for first-time contributors who are new to GitHub
  2. (DEFAULT) Require approval for first-time contributors
  3. Require approval for all outside collaborators

Ref : https://docs.github.com/en/organizations/managing-organization-settings/disabling-or-limiting-github-actions-for-your-organization#configuring-required-approval-for-workflows-from-public-forks

Maybe, we could move from option 2 to option 1 ? that way, most of the PR should pass. I don't think option 2 is more secure than option 1. (non-bot Hackers generally begin to make trivial good PR, before making bad PR)

What is your opinion ?

(sorry, that question is not a "oca-github-bot" question).

pedrobaeza commented 2 years ago

But you are going off topic of the main question of this issue, that is the requirement of approving GitHub workflows. If they sign the CLA and are included in the contributors team by the script connected to OCA instance, they don't need such approval, so I think that's the way to go.

StefanRijnhart commented 2 years ago

@legalsylvain that option 1 in the Github organization configuration seems a better choice than having a CLA indeed!

sbidoul commented 2 years ago

I changed to option 1 OCA-wide. That should be reasonable enough.

sbidoul commented 2 years ago

Thanks for the investigation, Sylvain!

pedrobaeza commented 2 years ago

Sorry, I read badly. Thanks all for the heads up.

legalsylvain commented 2 years ago

Closing this issue. Let's see if the new setting is enough, or if we need extra bot actions to avoid manual workflow confirmation. Thanks !