From 59aee1ca5d7b0cf324372795990e2d84d7cc61c1 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 26 Oct 2024 15:01:12 +0200 Subject: [PATCH 1/3] workflows/codeowners: Fix security issue Co-Authored-By: 13x1 Co-Authored-By: basti564 --- .github/workflows/codeowners.yml | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index 9d781c6ef080..1e2e5c9304ac 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -1,12 +1,24 @@ name: Codeowners -# This workflow depends on a GitHub App with the following permissions: -# - Repository > Administration: read-only -# - Organization > Members: read-only -# - Repository > Pull Requests: read-write -# The App needs to be installed on this repository -# the OWNER_APP_ID repository variable needs to be set -# the OWNER_APP_PRIVATE_KEY repository secret needs to be set +# This workflow depends on two GitHub Apps with the following permissions: +# - For checking code owners: +# - Permissions: +# - Repository > Administration: read-only +# - Organization > Members: read-only +# - Install App on this repository, setting these variables: +# - OWNER_RO_APP_ID (variable) +# - OWNER_RO_APP_PRIVATE_KEY (secret) +# - For requesting code owners: +# - Permissions: +# - Repository > Administration: read-only +# - Organization > Members: read-only +# - Repository > Pull Requests: read-write +# - Install App on this repository, setting these variables: +# - OWNER_APP_ID (variable) +# - OWNER_APP_PRIVATE_KEY (secret) +# +# This split is done because checking code owners requires handling untrusted PR input, +# while requesting code owners requires PR write access, and those shouldn't be mixed. on: pull_request_target: @@ -45,8 +57,8 @@ jobs: - uses: actions/create-github-app-token@5d869da34e18e7287c1daad50e0b8ea0f506ce69 # v1.11.0 id: app-token with: - app-id: ${{ vars.OWNER_APP_ID }} - private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }} + app-id: ${{ vars.OWNER_RO_APP_ID }} + private-key: ${{ secrets.OWNER_RO_APP_PRIVATE_KEY }} - uses: actions/checkout@eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871 # v4.2.1 with: From 6b8ce4aedf3e953624a442c02e8e7bf4d974d2b0 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 26 Oct 2024 15:03:37 +0200 Subject: [PATCH 2/3] workflows: Fix security issues read-all permissions gives access to e.g. security-events, which these don't need, and can easily lead to leaks Co-Authored-By: 13x1 Co-Authored-By: basti564 --- .github/workflows/codeowners.yml | 3 +++ .github/workflows/editorconfig.yml | 4 +++- .github/workflows/manual-nixos.yml | 3 ++- .github/workflows/manual-nixpkgs.yml | 3 ++- .github/workflows/nix-parse.yml | 4 +++- 5 files changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index 1e2e5c9304ac..e210cacda14f 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -24,6 +24,9 @@ on: pull_request_target: types: [opened, ready_for_review, synchronize, reopened, edited] +# We don't need any default GitHub token +permissions: {} + env: OWNERS_FILE: ci/OWNERS # Don't do anything on draft PRs diff --git a/.github/workflows/editorconfig.yml b/.github/workflows/editorconfig.yml index b4ef16a734b7..63264595bcc7 100644 --- a/.github/workflows/editorconfig.yml +++ b/.github/workflows/editorconfig.yml @@ -1,6 +1,8 @@ name: "Checking EditorConfig" -permissions: read-all +permissions: + pull-requests: read + contents: read on: # avoids approving first time contributors diff --git a/.github/workflows/manual-nixos.yml b/.github/workflows/manual-nixos.yml index 2ae4d929c12d..eed27f5565ed 100644 --- a/.github/workflows/manual-nixos.yml +++ b/.github/workflows/manual-nixos.yml @@ -1,6 +1,7 @@ name: "Build NixOS manual" -permissions: read-all +permissions: + contents: read on: pull_request_target: diff --git a/.github/workflows/manual-nixpkgs.yml b/.github/workflows/manual-nixpkgs.yml index 676a554107d5..14994e6c7542 100644 --- a/.github/workflows/manual-nixpkgs.yml +++ b/.github/workflows/manual-nixpkgs.yml @@ -1,6 +1,7 @@ name: "Build Nixpkgs manual" -permissions: read-all +permissions: + contents: read on: pull_request_target: diff --git a/.github/workflows/nix-parse.yml b/.github/workflows/nix-parse.yml index 352cb81d87ed..d3991424617c 100644 --- a/.github/workflows/nix-parse.yml +++ b/.github/workflows/nix-parse.yml @@ -1,6 +1,8 @@ name: "Check whether nix files are parseable" -permissions: read-all +permissions: + pull-requests: read + contents: read on: # avoids approving first time contributors From 5bbbc3a30b1a843bc2267f3bb4a42e8af3411498 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 26 Oct 2024 15:23:06 +0200 Subject: [PATCH 3/3] workflows: Rename after security fixes In the previous two commits, security issues with these workflows were fixed. In order for these to not be exploitable for PRs to branches that don't have the fixes yet (including read-only branches like nixos-unstable), these workflows are renamed, so that the old ones can be turned off manually via GitHub interface. Co-Authored-By: 13x1 Co-Authored-By: basti564 --- .github/workflows/{codeowners.yml => codeowners-v2.yml} | 2 +- .github/workflows/{editorconfig.yml => editorconfig-v2.yml} | 2 +- .github/workflows/{manual-nixos.yml => manual-nixos-v2.yml} | 2 +- .github/workflows/{manual-nixpkgs.yml => manual-nixpkgs-v2.yml} | 2 +- .github/workflows/{nix-parse.yml => nix-parse-v2.yml} | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) rename .github/workflows/{codeowners.yml => codeowners-v2.yml} (99%) rename .github/workflows/{editorconfig.yml => editorconfig-v2.yml} (98%) rename .github/workflows/{manual-nixos.yml => manual-nixos-v2.yml} (97%) rename .github/workflows/{manual-nixpkgs.yml => manual-nixpkgs-v2.yml} (97%) rename .github/workflows/{nix-parse.yml => nix-parse-v2.yml} (97%) diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners-v2.yml similarity index 99% rename from .github/workflows/codeowners.yml rename to .github/workflows/codeowners-v2.yml index e210cacda14f..23720e25e526 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners-v2.yml @@ -1,4 +1,4 @@ -name: Codeowners +name: Codeowners v2 # This workflow depends on two GitHub Apps with the following permissions: # - For checking code owners: diff --git a/.github/workflows/editorconfig.yml b/.github/workflows/editorconfig-v2.yml similarity index 98% rename from .github/workflows/editorconfig.yml rename to .github/workflows/editorconfig-v2.yml index 63264595bcc7..9bae2c32774d 100644 --- a/.github/workflows/editorconfig.yml +++ b/.github/workflows/editorconfig-v2.yml @@ -1,4 +1,4 @@ -name: "Checking EditorConfig" +name: "Checking EditorConfig v2" permissions: pull-requests: read diff --git a/.github/workflows/manual-nixos.yml b/.github/workflows/manual-nixos-v2.yml similarity index 97% rename from .github/workflows/manual-nixos.yml rename to .github/workflows/manual-nixos-v2.yml index eed27f5565ed..26a6279dcf22 100644 --- a/.github/workflows/manual-nixos.yml +++ b/.github/workflows/manual-nixos-v2.yml @@ -1,4 +1,4 @@ -name: "Build NixOS manual" +name: "Build NixOS manual v2" permissions: contents: read diff --git a/.github/workflows/manual-nixpkgs.yml b/.github/workflows/manual-nixpkgs-v2.yml similarity index 97% rename from .github/workflows/manual-nixpkgs.yml rename to .github/workflows/manual-nixpkgs-v2.yml index 14994e6c7542..f51c1c5ef9de 100644 --- a/.github/workflows/manual-nixpkgs.yml +++ b/.github/workflows/manual-nixpkgs-v2.yml @@ -1,4 +1,4 @@ -name: "Build Nixpkgs manual" +name: "Build Nixpkgs manual v2" permissions: contents: read diff --git a/.github/workflows/nix-parse.yml b/.github/workflows/nix-parse-v2.yml similarity index 97% rename from .github/workflows/nix-parse.yml rename to .github/workflows/nix-parse-v2.yml index d3991424617c..03f6af1b876b 100644 --- a/.github/workflows/nix-parse.yml +++ b/.github/workflows/nix-parse-v2.yml @@ -1,4 +1,4 @@ -name: "Check whether nix files are parseable" +name: "Check whether nix files are parseable v2" permissions: pull-requests: read