diff mbox series

.github: add github action to automate PR handling

Message ID 20240701024316.19646-1-zhangdongdong@eswincomputing.com
State Changes Requested
Headers show
Series .github: add github action to automate PR handling | expand

Commit Message

DongdongZhang July 1, 2024, 2:43 a.m. UTC 
From: Dongdong Zhang <zhangdongdong@eswincomputing.com>

This patch updates the Buildroot repository's GitHub configuration to better
manage incoming pull requests.

- Removed the existing pull request template that advised contributors to use
  the mailing list for patch submissions.
- Added a new GitHub Actions workflow (`repo-lockdown.yml`) to automatically
  handle new pull requests.

The new workflow:
- Triggers on new pull requests.
- Uses the `dessant/repo-lockdown` action to:
  - Comment on the pull request, guiding contributors to use the mailing list
    for patch submission.
  - Lock the pull request to prevent further discussion.
  - Close the pull request.

This change ensures that contributors are properly directed to the preferred
method of patch submission via the mailing list, maintaining consistency and
streamlining the review process.

Signed-off-by: Dongdong Zhang <zhangdongdong@eswincomputing.com>
---
 .github/pull_request_template.md    |  6 ------
 .github/workflows/repo-lockdown.yml | 23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)
 delete mode 100644 .github/pull_request_template.md
 create mode 100644 .github/workflows/repo-lockdown.yml

Comments

Arnout Vandecappelle July 1, 2024, 7:24 a.m. UTC | #1 
Hi Dongdong,

On 01/07/2024 04:43, zhangdongdong@eswincomputing.com wrote:
> From: Dongdong Zhang <zhangdongdong@eswincomputing.com>
> 
> This patch updates the Buildroot repository's GitHub configuration to better
> manage incoming pull requests.
> 
> - Removed the existing pull request template that advised contributors to use
>    the mailing list for patch submissions.
> - Added a new GitHub Actions workflow (`repo-lockdown.yml`) to automatically
>    handle new pull requests.
> 
> The new workflow:
> - Triggers on new pull requests.
> - Uses the `dessant/repo-lockdown` action to:
>    - Comment on the pull request, guiding contributors to use the mailing list
>      for patch submission.
>    - Lock the pull request to prevent further discussion.
>    - Close the pull request.

  Thank you very much for this patch - that looks like an excellent plan!

  I have one question though (since I'm not super familiar with how github 
workflows work). It is possible for people to modify the workflows yaml files 
and make a pull request that can do anything. This is a risk because those 
workflows may have access to project-specific resources and abuse that to do 
some things (although I'm not entirely sure what can be done, since I'm not 
sure, I prefer to block everything). Right now, we have turned on the option 
"Require approval for all outside collaborators". In other words, the workflow 
will not run until one of the maintainers approves it.

  Of course, that kind of defeats the purpose of this automatic closing of pull 
requests, because we still have to go to the github web interface to trigger the 
workflow...

  So, is there a way to make this actually automatic without risking abuse?

  And in fact, I have a similar question about the dessant/repo-lockdown 
workflow. Is there anything stopping that workflow provider from installing a 
backdoor in all the projects that use that workflow?

  Sorry if this sounds paranoid, but xz, you know...

  Regards,
  Arnout

> 
> This change ensures that contributors are properly directed to the preferred
> method of patch submission via the mailing list, maintaining consistency and
> streamlining the review process.
> 
> Signed-off-by: Dongdong Zhang <zhangdongdong@eswincomputing.com>
> ---
>   .github/pull_request_template.md    |  6 ------
>   .github/workflows/repo-lockdown.yml | 23 +++++++++++++++++++++++
>   2 files changed, 23 insertions(+), 6 deletions(-)
>   delete mode 100644 .github/pull_request_template.md
>   create mode 100644 .github/workflows/repo-lockdown.yml
> 
> diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
> deleted file mode 100644
> index 319e67d861..0000000000
> --- a/.github/pull_request_template.md
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -Please do not submit a Pull Request via GitHub. Buildroot makes use of a
> -[mailing list](http://lists.buildroot.org/mailman/listinfo/buildroot) for patch submission and review.
> -See [submitting your own patches](http://buildroot.org/manual.html#submitting-patches) for more info.
> -
> -Thanks for your help!
> -
> diff --git a/.github/workflows/repo-lockdown.yml b/.github/workflows/repo-lockdown.yml
> new file mode 100644
> index 0000000000..a5f8bd6a93
> --- /dev/null
> +++ b/.github/workflows/repo-lockdown.yml
> @@ -0,0 +1,23 @@
> +name: 'Repo Lockdown'
> +
> +on:
> +  pull_request_target:
> +    types: opened
> +
> +permissions:
> +  pull-requests: write
> +
> +jobs:
> +  action:
> +    runs-on: ubuntu-latest
> +    steps:
> +      - uses: dessant/repo-lockdown@v4
> +        with:
> +          pr-comment: |
> +            Please do not submit a Pull Request via GitHub. Buildroot makes use of a
> +            [mailing list](http://lists.buildroot.org/mailman/listinfo/buildroot) for patch submission and review.
> +            See [submitting your own patches](http://buildroot.org/manual.html#submitting-patches) for more info.
> +
> +            Thanks for your help!
> +          lock-pr: true
> +          close-pr: true
> \ No newline at end of file
Edgar Bonet July 1, 2024, 7:50 a.m. UTC | #2 
Hello!

Dongdong Zhang wrote:
> This patch [...] Removed the existing pull request template that
> advised contributors to use the mailing list for patch submissions.

What is the rationale for doing so? The sooner the contributor learns
about the procedure, the more convenient it is for them. Keeping this
template would then be friendlier to would-be contributors.

Of course, there is still the possibility that they do not read the
template, so there is value in having the workflow *in addition to* (not
instead of) the pull request template.

Regards,

Edgar Bonet.
Edgar Bonet July 1, 2024, 7:55 a.m. UTC | #3 
About the dessant/repo-lockdown workflow, Arnout Vandecappelle wrote:
> Is there anything stopping that workflow provider from installing a
> backdoor in all the projects that use that workflow?

No, there isn't. A common protection against such attacks is to not rely
on a tag like “v4”, but instead

```yaml
  uses: provider/action@<specific SHA1>
```

Cheers!

Edgar Bonet.
DongdongZhang July 3, 2024, 2:50 a.m. UTC | #4 
Hi Arnout,

Thank you for your feedback. I'll address your questions one by one.

1. How to prevent workflow abuse?

Since Buildroot on GitHub is merely a read-only mirror and we do not
plan to use complex workflows in the future, this workflow file should 
remain unchanged and thus should be safe. Additionally, GitHub provides 
some security measures. You can set the workflow's read/write permissions 
in Buildroot-Settings-Actions-General-Workflow permissions-Read repository 
contents and packages permissions. This way, the workflow will never 
write content to Buildroot.

2. Does enabling "Require approval for all outside collaborators" mean 
that maintainers need to manually trigger the workflow?

The official documentation for Workflow includes the following:

Source: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository

Note: Workflows triggered by pull_request_target events are run in the 
context of the base branch. Since the base branch is considered trusted, 
workflows triggered by these events will always run, regardless of 
approval settings. For more information about the pull_request_target 
event, see "Events that trigger workflows."

> > +on:
> > +  pull_request_target:
> > +    types: opened
> > +

In short, this means that when a pull_request_target event triggers, 
the workflow will run on the base branch. Since the base branch is 
trusted, workflows triggered by these events will always run, 
regardless of the approval settings. Therefore, even if 
"Require approval for all outside collaborators" is enabled, the 
workflow will still run on the base branch without requiring 
maintainers to manually trigger it. I have tested this process in a 
repository I created. After a colleague submitted a pull request, 
the workflow ran automatically.

3. How to prevent the repo-lockdown workflow provider from installing 
backdoors in all projects using repo-lockdown?

Your cautious approach is highly commendable. However, we need not 
worry about this issue. Firstly, repo-lockdown is a public repository, 
and everyone can review its source code: 
https://github.com/dessant/repo-lockdown. 
Secondly, the workflow runs on GitHub's servers, not locally, so 
there is no risk to our development machines. Most importantly, 
your concern is whether it will affect the Buildroot repository. 
This has already been addressed by setting the workflow permissions; 
it will not modify any content in the Buildroot repository.

Lastly, I would like to mention that the idea of adding repo-lockdown 
came from the QEMU community, which also uses this 
workflow: https://github.com/qemu/qemu. It has been running stably 
for three years. Therefore, I believe this workflow is safe.

I hope this clarifies everything. If you have any other questions, 
feel free to ask!

PS： My English is not very good, and some parts were machine-translated. 
Please forgive any inappropriate phrasing. Thank you.

Regards,
Dongdong
---

> -----原始邮件-----发件人:"Arnout Vandecappelle" <arnout@mind.be>发送时间:2024-07-01 15:24:21 (星期一)收件人:zhangdongdong@eswincomputing.com, buildroot@buildroot.org抄送:主题:Re: [Buildroot] [PATCH] .github: add github action to automate PR handling
> 
>   Hi Dongdong,
> 
> On 01/07/2024 04:43, zhangdongdong@eswincomputing.com wrote:
> > From: Dongdong Zhang <zhangdongdong@eswincomputing.com>
> > 
> > This patch updates the Buildroot repository's GitHub configuration to better
> > manage incoming pull requests.
> > 
> > - Removed the existing pull request template that advised contributors to use
> >    the mailing list for patch submissions.
> > - Added a new GitHub Actions workflow (`repo-lockdown.yml`) to automatically
> >    handle new pull requests.
> > 
> > The new workflow:
> > - Triggers on new pull requests.
> > - Uses the `dessant/repo-lockdown` action to:
> >    - Comment on the pull request, guiding contributors to use the mailing list
> >      for patch submission.
> >    - Lock the pull request to prevent further discussion.
> >    - Close the pull request.
> 
>   Thank you very much for this patch - that looks like an excellent plan!
> 
>   I have one question though (since I'm not super familiar with how github 
> workflows work). It is possible for people to modify the workflows yaml files 
> and make a pull request that can do anything. This is a risk because those 
> workflows may have access to project-specific resources and abuse that to do 
> some things (although I'm not entirely sure what can be done, since I'm not 
> sure, I prefer to block everything). Right now, we have turned on the option 
> "Require approval for all outside collaborators". In other words, the workflow 
> will not run until one of the maintainers approves it.
> 
>   Of course, that kind of defeats the purpose of this automatic closing of pull 
> requests, because we still have to go to the github web interface to trigger the 
> workflow...
> 
>   So, is there a way to make this actually automatic without risking abuse?
> 
>   And in fact, I have a similar question about the dessant/repo-lockdown 
> workflow. Is there anything stopping that workflow provider from installing a 
> backdoor in all the projects that use that workflow?
> 
>   Sorry if this sounds paranoid, but xz, you know...
> 
>   Regards,
>   Arnout
> 
> > 
> > This change ensures that contributors are properly directed to the preferred
> > method of patch submission via the mailing list, maintaining consistency and
> > streamlining the review process.
> > 
> > Signed-off-by: Dongdong Zhang <zhangdongdong@eswincomputing.com>
> > ---
> >   .github/pull_request_template.md    |  6 ------
> >   .github/workflows/repo-lockdown.yml | 23 +++++++++++++++++++++++
> >   2 files changed, 23 insertions(+), 6 deletions(-)
> >   delete mode 100644 .github/pull_request_template.md
> >   create mode 100644 .github/workflows/repo-lockdown.yml
> > 
> > diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
> > deleted file mode 100644
> > index 319e67d861..0000000000
> > --- a/.github/pull_request_template.md
> > +++ /dev/null
> > @@ -1,6 +0,0 @@
> > -Please do not submit a Pull Request via GitHub. Buildroot makes use of a
> > -[mailing list](http://lists.buildroot.org/mailman/listinfo/buildroot) for patch submission and review.
> > -See [submitting your own patches](http://buildroot.org/manual.html#submitting-patches) for more info.
> > -
> > -Thanks for your help!
> > -
> > diff --git a/.github/workflows/repo-lockdown.yml b/.github/workflows/repo-lockdown.yml
> > new file mode 100644
> > index 0000000000..a5f8bd6a93
> > --- /dev/null
> > +++ b/.github/workflows/repo-lockdown.yml
> > @@ -0,0 +1,23 @@
> > +name: 'Repo Lockdown'
> > +
> > +on:
> > +  pull_request_target:
> > +    types: opened
> > +
> > +permissions:
> > +  pull-requests: write
> > +
> > +jobs:
> > +  action:
> > +    runs-on: ubuntu-latest
> > +    steps:
> > +      - uses: dessant/repo-lockdown@v4
> > +        with:
> > +          pr-comment: |
> > +            Please do not submit a Pull Request via GitHub. Buildroot makes use of a
> > +            [mailing list](http://lists.buildroot.org/mailman/listinfo/buildroot) for patch submission and review.
> > +            See [submitting your own patches](http://buildroot.org/manual.html#submitting-patches) for more info.
> > +
> > +            Thanks for your help!
> > +          lock-pr: true
> > +          close-pr: true
> > \ No newline at end of file
DongdongZhang July 4, 2024, 9:43 a.m. UTC | #5 
Sorry, Edgar Bonet, I might not have received your emails due to the
company's email filtering. I only found your email today when I checked
the buildroot thread list, I apologize for that.

When I was adding this workflow, I felt that reminding developers twice
was a bit redundant. However, your suggestion is great, as the pull
request template can remind developers immediately, rather than waiting
until after the pull request is created. I will restore the pull request
template in the next version and keep it.

Thank you.

------------------------------------------------------
Hello!

Dongdong Zhang wrote:
> This patch [...] Removed the existing pull request template that
> advised contributors to use the mailing list for patch submissions.

What is the rationale for doing so? The sooner the contributor learns
about the procedure, the more convenient it is for them. Keeping this
template would then be friendlier to would-be contributors.

Of course, there is still the possibility that they do not read the
template, so there is value in having the workflow *in addition to* (not
instead of) the pull request template.

Regards,

Edgar Bonet.

> -----原始邮件-----发件人:zhangdongdong@eswincomputing.com发送时间:2024-07-01 10:43:16 (星期一)收件人:buildroot@buildroot.org抄送:zhangdongdong@eswincomputing.com主题:[PATCH] .github: add github action to automate PR handling
> 
> From: Dongdong Zhang <zhangdongdong@eswincomputing.com>
> 
> This patch updates the Buildroot repository's GitHub configuration to better
> manage incoming pull requests.
> 
> - Removed the existing pull request template that advised contributors to use
>   the mailing list for patch submissions.
> - Added a new GitHub Actions workflow (`repo-lockdown.yml`) to automatically
>   handle new pull requests.
> 
> The new workflow:
> - Triggers on new pull requests.
> - Uses the `dessant/repo-lockdown` action to:
>   - Comment on the pull request, guiding contributors to use the mailing list
>     for patch submission.
>   - Lock the pull request to prevent further discussion.
>   - Close the pull request.
> 
> This change ensures that contributors are properly directed to the preferred
> method of patch submission via the mailing list, maintaining consistency and
> streamlining the review process.
> 
> Signed-off-by: Dongdong Zhang <zhangdongdong@eswincomputing.com>
> ---
>  .github/pull_request_template.md    |  6 ------
>  .github/workflows/repo-lockdown.yml | 23 +++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 6 deletions(-)
>  delete mode 100644 .github/pull_request_template.md
>  create mode 100644 .github/workflows/repo-lockdown.yml
> 
> diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
> deleted file mode 100644
> index 319e67d861..0000000000
> --- a/.github/pull_request_template.md
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -Please do not submit a Pull Request via GitHub. Buildroot makes use of a
> -[mailing list](http://lists.buildroot.org/mailman/listinfo/buildroot) for patch submission and review.
> -See [submitting your own patches](http://buildroot.org/manual.html#submitting-patches) for more info.
> -
> -Thanks for your help!
> -
> diff --git a/.github/workflows/repo-lockdown.yml b/.github/workflows/repo-lockdown.yml
> new file mode 100644
> index 0000000000..a5f8bd6a93
> --- /dev/null
> +++ b/.github/workflows/repo-lockdown.yml
> @@ -0,0 +1,23 @@
> +name: 'Repo Lockdown'
> +
> +on:
> +  pull_request_target:
> +    types: opened
> +
> +permissions:
> +  pull-requests: write
> +
> +jobs:
> +  action:
> +    runs-on: ubuntu-latest
> +    steps:
> +      - uses: dessant/repo-lockdown@v4
> +        with:
> +          pr-comment: |
> +            Please do not submit a Pull Request via GitHub. Buildroot makes use of a
> +            [mailing list](http://lists.buildroot.org/mailman/listinfo/buildroot) for patch submission and review.
> +            See [submitting your own patches](http://buildroot.org/manual.html#submitting-patches) for more info.
> +
> +            Thanks for your help!
> +          lock-pr: true
> +          close-pr: true
> \ No newline at end of file
> -- 
> 2.17.1
diff mbox series

Patch

diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
deleted file mode 100644
index 319e67d861..0000000000
--- a/.github/pull_request_template.md
+++ /dev/null
@@ -1,6 +0,0 @@ 
-Please do not submit a Pull Request via GitHub. Buildroot makes use of a
-[mailing list](http://lists.buildroot.org/mailman/listinfo/buildroot) for patch submission and review.
-See [submitting your own patches](http://buildroot.org/manual.html#submitting-patches) for more info.
-
-Thanks for your help!
-
diff --git a/.github/workflows/repo-lockdown.yml b/.github/workflows/repo-lockdown.yml
new file mode 100644
index 0000000000..a5f8bd6a93
--- /dev/null
+++ b/.github/workflows/repo-lockdown.yml
@@ -0,0 +1,23 @@ 
+name: 'Repo Lockdown'
+
+on:
+  pull_request_target:
+    types: opened
+
+permissions:
+  pull-requests: write
+
+jobs:
+  action:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: dessant/repo-lockdown@v4
+        with:
+          pr-comment: |
+            Please do not submit a Pull Request via GitHub. Buildroot makes use of a
+            [mailing list](http://lists.buildroot.org/mailman/listinfo/buildroot) for patch submission and review.
+            See [submitting your own patches](http://buildroot.org/manual.html#submitting-patches) for more info.
+
+            Thanks for your help!
+          lock-pr: true
+          close-pr: true
\ No newline at end of file