From be29eb587b41078ad3922b87bb0547959df92024 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 23 Apr 2024 23:29:54 +0200 Subject: [PATCH 1/6] Switch CI to pull_request_target Also make CI run for all pull requests, not just ones to the main branch. This allows testing pull_request_target changes in this repo. --- .github/workflows/ci.yml | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97f9648..0defa1e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,8 +1,11 @@ name: CI on: - pull_request: - branches: - - main + # We use pull_request_target such that the code owner validation works for PRs from forks, + # because we need repository secrets for that, which pull_request wouldn't allow from forks. + # However, it's very important that we don't run code from forks without sandboxing it, + # because that way anybody could potentially extract repository secrets! + # Furthermore, using pull_request_target doesn't require manually approving first-time contributors + pull_request_target: jobs: xrefcheck: @@ -10,6 +13,12 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + ref: refs/pull/${{ github.event.pull_request.number }}/merge + path: untrusted-pr + - uses: serokell/xrefcheck-action@v1 + with: + xrefcheck-args: "--root untrusted-pr" # TODO: Use https://github.com/marketplace/actions/github-codeowners-validator From 8dda28d66c5610aa535fa614aa184dfc3b64653a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 23 Apr 2024 19:38:23 +0200 Subject: [PATCH 2/6] Validate CODEOWNERS --- .github/workflows/ci.yml | 35 ++++++++++++++++++++++++++++++++++- doc/org-repo.md | 4 +--- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0defa1e..8601548 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,4 +21,37 @@ jobs: with: xrefcheck-args: "--root untrusted-pr" -# TODO: Use https://github.com/marketplace/actions/github-codeowners-validator + codeowners: + name: Validate codeowners + runs-on: ubuntu-latest + steps: + + - uses: actions/checkout@v4 + with: + path: trusted-base + + - uses: actions/checkout@v4 + with: + ref: refs/pull/${{ github.event.pull_request.number }}/merge + path: untrusted-pr + + - uses: mszostok/codeowners-validator@v0.7.4 + with: + # GitHub access token is required only if the `owners` check is enabled + # See https://github.com/mszostok/codeowners-validator/blob/main/docs/gh-auth.md#public-repositories + github_access_token: "${{ secrets.OWNERS_VALIDATOR_GITHUB_SECRET }}" + + # The repository path in which CODEOWNERS file should be validated." + repository_path: untrusted-pr + + # The owner and repository name. For example, gh-codeowners/codeowners-samples. Used to check if GitHub team is in the given organization and has permission to the given repository." + owner_checker_repository: "${{ github.repository }}" + + # "The comma-separated list of experimental checks that should be executed. By default, all experimental checks are turned off. Possible values: notowned,avoid-shadowing" + experimental_checks: "notowned,avoid-shadowing" + + # Specifies whether CODEOWNERS may have unowned files. For example, `/infra/oncall-rotator/oncall-config.yml` doesn't have owner and this is not reported. + owner_checker_allow_unowned_patterns: "false" + + # Specifies whether only teams are allowed as owners of files. + owner_checker_owners_must_be_teams: "false" diff --git a/doc/org-repo.md b/doc/org-repo.md index 7314250..d213cb8 100644 --- a/doc/org-repo.md +++ b/doc/org-repo.md @@ -4,9 +4,7 @@ This repository itself is the entry point for documentation on official resource Everybody in the [CODEOWNERS](../.github/CODEOWNERS) file has write permission to this repository. This allows people to get automatic review requests and merge PRs for the files that concern them. - -TODO: Enable branch protection to require reviews by code owners. -TODO: Ensure that all files have a code owner +PRs can only be merged if a codeowner for the respective files approves it, and all files need to have a codeowner entry. Furthermore, the code owners for the CODEOWNERS file should have permission to give more people write access to this repository. These people get requested for reviews when new people add themselves to CODEOWNERS, allowing them to give write access when merged. From 9669ab7ae2a2b034eb8df25bb002c600ea767bbc Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 23 Apr 2024 19:40:41 +0200 Subject: [PATCH 3/6] CODEOWNERS: Add infinisil and zimbatm to all files with no current owner To satisfy the new check requiring all files to have a code owner --- .github/CODEOWNERS | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b2ecce3..0f95be8 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,5 +1,19 @@ -# See ./org-repo.md +# See doc/org-repo.md + +/README.md @infinisil @zimbatm +/CONTRIBUTING.md @infinisil @zimbatm +/LICENSE @infinisil @zimbatm /.github/CODEOWNERS @infinisil @zimbatm +/.github/workflows @infinisil @zimbatm +/review-body.sh @infinisil @zimbatm /doc/org-repo.md @infinisil @zimbatm +/doc/discourse.md @infinisil @zimbatm +/doc/github.md @infinisil @zimbatm +/doc/matrix.md @infinisil @zimbatm +/doc/moderation.md @infinisil @zimbatm +/doc/nixos-releases.md @infinisil @zimbatm +/doc/resources.md @infinisil @zimbatm +/doc/rfcs.md @infinisil @zimbatm +/doc/teams.md @infinisil @zimbatm From b78c63f10171f7bb31b911aa684375467c6da89f Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 23 Apr 2024 23:33:06 +0200 Subject: [PATCH 4/6] Check write permissions --- .github/CODEOWNERS | 1 + .github/workflows/ci.yml | 10 +++++++ scripts/unprivileged-owners.sh | 50 ++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+) create mode 100755 scripts/unprivileged-owners.sh diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 0f95be8..b5efa3c 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -6,6 +6,7 @@ /.github/CODEOWNERS @infinisil @zimbatm /.github/workflows @infinisil @zimbatm +/scripts @infinisil @zimbatm /review-body.sh @infinisil @zimbatm /doc/org-repo.md @infinisil @zimbatm diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8601548..e22906a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,6 +25,7 @@ jobs: name: Validate codeowners runs-on: ubuntu-latest steps: + - uses: cachix/install-nix-action@v26 - uses: actions/checkout@v4 with: @@ -55,3 +56,12 @@ jobs: # Specifies whether only teams are allowed as owners of files. owner_checker_owners_must_be_teams: "false" + + # The above validator doesn't currently ensure that people have write access: https://github.com/mszostok/codeowners-validator/issues/157 + # So we're doing it manually instead + - name: Check that codeowners have write access + # Important that we run the script from the base branch, + # because otherwise a PR from a fork could change it to extract the secret + run: trusted-base/scripts/unprivileged-owners.sh untrusted-pr ${{ github.repository }} + env: + GH_TOKEN: "${{ secrets.OWNERS_VALIDATOR_GITHUB_SECRET }}" diff --git a/scripts/unprivileged-owners.sh b/scripts/unprivileged-owners.sh new file mode 100755 index 0000000..36e16d3 --- /dev/null +++ b/scripts/unprivileged-owners.sh @@ -0,0 +1,50 @@ +#!/usr/bin/env nix-shell +#!nix-shell -i bash --pure --keep GH_TOKEN -I nixpkgs=channel:nixpkgs-unstable -p codeowners github-cli + +set -euo pipefail + +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' exit + +if (( $# != 2 )); then + echo "Usage: $0 PATH OWNER/REPO" + exit 1 +fi + +root=$1 +repo=$2 + +# Writes all code owners into $tmp/codeowners, one user per line (without @) +while read -r -a fields; do + # The first field is the filename + unset 'fields[0]' + if [[ "${fields[1]}" != "(unowned)" ]]; then + (IFS=$'\n'; echo "${fields[*]##@}") + fi +done < <(cd "$root"; codeowners) | + sort -u > "$tmp/codeowners" + +# Get all users with push access +gh api \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + --method GET \ + -f permission=push \ + /repos/"$repo"/collaborators \ + -F per_page=100 \ + --paginate \ + --jq '.[].login' | + sort > "$tmp/collaborators" + +# Figure out all the owners that aren't collaborators +readarray -t unprivilegedOwners < <(comm -23 "$tmp/codeowners" "$tmp/collaborators") + +if (( "${#unprivilegedOwners[@]}" == 0 )); then + echo "All code owners have write permission" +else + echo "There are code owners that don't have write permission. Either remove them as code owners or give them write permission:" + for handle in "${unprivilegedOwners[@]}"; do + echo "- [ ] @$handle" + done + exit 1 +fi From 1d10641b3372c650256ef5504b12266be603090d Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 24 Apr 2024 00:15:18 +0200 Subject: [PATCH 5/6] Move review-body.sh into scripts directory --- .github/CODEOWNERS | 1 - .github/workflows/review.yml | 2 +- review-body.sh => scripts/review-body.sh | 0 3 files changed, 1 insertion(+), 2 deletions(-) rename review-body.sh => scripts/review-body.sh (100%) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b5efa3c..bb4f960 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -7,7 +7,6 @@ /.github/CODEOWNERS @infinisil @zimbatm /.github/workflows @infinisil @zimbatm /scripts @infinisil @zimbatm -/review-body.sh @infinisil @zimbatm /doc/org-repo.md @infinisil @zimbatm /doc/discourse.md @infinisil @zimbatm diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index b63c140..130d4d9 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -19,6 +19,6 @@ jobs: -H "X-GitHub-Api-Version: 2022-11-28" \ /repos/"$GITHUB_REPOSITORY"/issues \ -f title="[$(date +'%Y %B')] Regular manual review " \ - -f body="$(./review-body.sh)" + -f body="$(./scripts/review-body.sh)" env: GH_TOKEN: ${{ github.token }} diff --git a/review-body.sh b/scripts/review-body.sh similarity index 100% rename from review-body.sh rename to scripts/review-body.sh From 9028d175b20f3179377a10b1590d53005c0b689e Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 24 Apr 2024 00:45:08 +0200 Subject: [PATCH 6/6] Check that code owners have write access for the regular review --- .github/workflows/review.yml | 16 ++++++++++++++-- scripts/review-body.sh | 25 ++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 130d4d9..bc70633 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -11,14 +11,26 @@ jobs: update: runs-on: ubuntu-latest steps: + - uses: cachix/install-nix-action@v26 + - uses: actions/checkout@v4 + with: + path: repo + + - name: Generate issue body + run: repo/scripts/review-body.sh repo ${{ github.repository }} > body + env: + # This token has read-only admin access to see who has write access to this repo + GH_TOKEN: "${{ secrets.OWNERS_VALIDATOR_GITHUB_SECRET }}" + - run: | gh api \ --method POST \ -H "Accept: application/vnd.github+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ - /repos/"$GITHUB_REPOSITORY"/issues \ + /repos/${{ github.repository }}/issues \ -f title="[$(date +'%Y %B')] Regular manual review " \ - -f body="$(./scripts/review-body.sh)" + -F body=@body env: + # This token has write access to only issues to create one GH_TOKEN: ${{ github.token }} diff --git a/scripts/review-body.sh b/scripts/review-body.sh index c9e8758..2b68d59 100755 --- a/scripts/review-body.sh +++ b/scripts/review-body.sh @@ -1,9 +1,21 @@ -#!/usr/bin/env bash +#!/usr/bin/env nix-shell +#!nix-shell -i bash --pure --keep GH_TOKEN -I nixpkgs=channel:nixpkgs-unstable -p codeowners github-cli gitMinimal + set -euo pipefail # This script outputs the contents of the regular review issue, see ./github/workflows/review.yml -rev=$(git rev-parse HEAD) +SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) + +if (( $# != 2 )); then + echo "Usage: $0 PATH OWNER/REPO" + exit 1 +fi + +root=$1 +repo=$2 + +rev=$(git -C "$root" rev-parse HEAD) echo "Because the documentation in this repository may slowly deviate from reality, this monthly issue is created to regularly review the files. @@ -30,4 +42,11 @@ while read -r file users; do continue fi echo "- [ ] \`$file\`: $users" -done < .github/CODEOWNERS +done < "$root"/.github/CODEOWNERS + +echo "" + +# Check that all code owners have write permissions +# `|| true` because this script fails when there are code owners without permissions, +# which is useful to fail PRs, but not here +bash "$SCRIPT_DIR"/unprivileged-owners.sh "$root" "$repo" || true