Skip to content

Add zizmor to pre-commit and fix most findings #127749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Dec 10, 2024
11 changes: 10 additions & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ jobs:
- uses: actions/checkout@v4
with:
fetch-depth: 1
persist-credentials: false
- name: Runner image version
run: echo "IMAGE_VERSION=${ImageVersion}" >> "$GITHUB_ENV"
- name: Check Autoconf and aclocal versions
Expand Down Expand Up @@ -94,6 +95,8 @@ jobs:
if: needs.check_source.outputs.run_tests == 'true'
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-python@v5
with:
python-version: '3.x'
Expand Down Expand Up @@ -268,6 +271,8 @@ jobs:
LD_LIBRARY_PATH: ${{ github.workspace }}/multissl/openssl/${{ matrix.openssl_ver }}/lib
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Runner image version
run: echo "IMAGE_VERSION=${ImageVersion}" >> "$GITHUB_ENV"
- name: Restore config.cache
Expand Down Expand Up @@ -328,6 +333,8 @@ jobs:
PYTHONSTRICTEXTENSIONBUILD: 1
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Register gcc problem matcher
run: echo "::add-matcher::.github/problem-matchers/gcc.json"
- name: Install Dependencies
Expand Down Expand Up @@ -411,7 +418,7 @@ jobs:
#
# (GH-104097) test_sysconfig is skipped because it has tests that are
# failing when executed from inside a virtual environment.
${{ env.VENV_PYTHON }} -m test \
"${VENV_PYTHON}" -m test \
-W \
-o \
-j4 \
Expand Down Expand Up @@ -446,6 +453,8 @@ jobs:
ASAN_OPTIONS: detect_leaks=0:allocator_may_return_null=1:handle_segv=0
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Runner image version
run: echo "IMAGE_VERSION=${ImageVersion}" >> "$GITHUB_ENV"
- name: Restore config.cache
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/documentation-links.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,16 @@ on:
- 'Doc/**'
- '.github/workflows/doc.yml'

permissions:
pull-requests: write

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
cancel-in-progress: true

jobs:
documentation-links:
runs-on: ubuntu-latest
permissions:
pull-requests: write

steps:
- uses: readthedocs/actions/preview@v1
with:
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/jit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ jobs:
timeout-minutes: 90
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Build tier two interpreter
run: |
./configure --enable-experimental-jit=interpreter --with-pydebug
Expand Down Expand Up @@ -85,6 +87,8 @@ jobs:
runner: ${{ github.repository_owner == 'python' && 'ubuntu-24.04-aarch64' || 'ubuntu-24.04' }}
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-python@v5
with:
python-version: '3.11'
Expand Down Expand Up @@ -138,6 +142,8 @@ jobs:
- 19
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-python@v5
with:
python-version: '3.11'
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ jobs:

steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-python@v5
with:
python-version: "3.x"
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/mypy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ jobs:
timeout-minutes: 10
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-python@v5
with:
python-version: "3.13"
Expand Down
10 changes: 6 additions & 4 deletions .github/workflows/require-pr-label.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@ on:
pull_request:
types: [opened, reopened, labeled, unlabeled, synchronize]

permissions:
issues: write
pull-requests: write

jobs:
label-dnm:
name: DO-NOT-MERGE
if: github.repository_owner == 'python'
runs-on: ubuntu-latest
permissions:
issues: write
pull-requests: write
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These labels are only used on PRs, so I'm pretty sure the issues: write can be removed. In addition, I think we might not even need the pull-requests: write since AFAICT we are not writing anything.
I don't have enough energy today to also dig into this to make sure that the permissions are not needed, so feel free to merge this as is and we can open a separate issue to investigate and possibly remove these.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we can check with this separately. This PR isn't adding any extra permissions here.

timeout-minutes: 10

steps:
Expand All @@ -28,6 +27,9 @@ jobs:
name: Unresolved review
if: github.repository_owner == 'python'
runs-on: ubuntu-latest
permissions:
issues: write
pull-requests: write
timeout-minutes: 10

steps:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/reusable-change-detection.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ jobs:
- run: >-
echo '${{ github.event_name }}'
- uses: actions/checkout@v4
with:
persist-credentials: false
Copy link
Member

@ezio-melotti ezio-melotti Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a git fetch in the next step. Wouldn't that still need the credentials in order to work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it needs to be an authenticated git fetch?

It ran okay here: https://github.com/python/cpython/actions/runs/12225521755/job/34099604804

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, persist-credentials is only needed for git ops that need authentication. In the context of public repos and their workflows, in practice that means only ops that mutate the repo, rather than pulling from it 🙂

Copy link
Member

@ezio-melotti ezio-melotti Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, but if this is the case, does it also mean that as long as we are just reading from public repos, there are no credentials used so there's nothing to share with the other jobs?

IOW, persist-credentials: true would only be a concern when a token is used, either for mutating operations (e.g. pushes) or for accessing private repos, and in this case there's actually no security risk, right?

(It might still be better to explicitly add persist-credentials: false regardless though -- e.g. in case someone adds a token later.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, but if this is the case, does it also mean that as long as we are just reading from public repos, there are no credentials used so there's nothing to share with the other jobs?

Nope, unfortunately the default means that a credential is persisted, even though it isn't necessary. So persist-credentials: false actually does something usefully by explicitly removing the credential.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reply! I'm still not sure I understand if there is an actual issue with reading (i.e. with no writing/mutating operations) public repos though. I tried digging a bit deeper to understand and this is what I found.

The actions/checkout README says that it accepts both a token and an ssh-key (which we are not using):

# Personal access token (PAT) used to fetch the repository. The PAT is configured
# with the local git config, which enables your scripts to run authenticated git
# commands. The post-job step removes the PAT.
#
# We recommend using a service account with the least permissions necessary. Also
# when generating a new PAT, select the least scopes necessary.
#
# [Learn more about creating and using encrypted secrets](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets)
#
# Default: ${{ github.token }}
token: ''

# SSH key used to fetch the repository. The SSH key is configured with the local
# git config, which enables your scripts to run authenticated git commands. The
# post-job step removes the SSH key.
#
# We recommend using a service account with the least permissions necessary.
#
# [Learn more about creating and using encrypted secrets](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets)
ssh-key: ''

The comment for persist-credentials says:

# Whether to configure the token or SSH key with the local git config
# Default: true
persist-credentials: ''

So my understanding is that if I explicitly set a token/key with token: '...' or ssh-key: '...' and use persist-credentials: true (either explicitly or implicitly since it's the default), the token/key will be "persisted in the local git config" (which I assume is .git/config, i.e. it will be written on a file). This will expose the token/key to the other jobs -- which is a security risk.

If I don't explicitly specify any token/key (i.e. what we are doing in our workflows), actions/checkout will set github.token as default token and persist that. Since github.token is already available to all jobs, the fact that it is persisted shouldn't be an issue -- unless having its value written on a file poses a new threat that I'm not aware of.

Nope, unfortunately the default means that a credential is persisted, even though it isn't necessary.

If no credentials are passed explicitly by using token/ssh-key (which are not needed while reading public repos, like in our workflows) and assuming persist-credentials: true, either:

  • there is no security issue (unless either a token or ssh-key are explicitly set);
  • persisting github.token is still a security issue (maybe because it's written on a file?); 1
  • there's some other credential that I'm missing that causes a security issue;

Footnotes

  1. if this is a problem, maybe setting token: '' explicitly will prevent the github.token to be used (and persisted) at all (depending on how the action is implemented)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! Thanks for your detailed response as well.

Since github.token is already available to all jobs, the fact that it is persisted shouldn't be an issue -- unless having its value written on a file poses a new threat that I'm not aware of.

Yep: the risk with this is that it's easy to inadvertently upload/log/otherwise persist that local filesystem credential. This can't happen with the in-memory token (i.e. github.token) in the ordinary case.

For example, until recently, this would cause a token disclosure via a public artifact:

uses: actions/checkout

# other steps

uses: actions/upload-artifact
with:
  path: . # uploads the entire repo, including the persisted token

This post has some interesting/elucidative examples of that: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/

  1. if this is a problem, maybe setting token: '' explicitly will prevent the github.token to be used (and persisted) at all (depending on how the action is implemented)

Yeah, I believe this will result in an empty string being saved on the filesystem for the token. I'm not 100% sure but I think this will cause GitHub API errors in some cases.

(I think persist-credentials: false is the intended way to disable github.token.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This post has some interesting/elucidative examples of that: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/

Thanks for sharing, this is really interesting!

Now that I have all the pieces of the puzzle it finally makes sense:

  • GitHub automatically creates a github.token
  • If no token: ... is specified in actions/checkout, then github.token is used
  • If persist-credentials is true (the default), either the github.token or the specified token will be written in .git/config
  • If the file containing the token becomes publicly accessible (e.g. uploaded via actions/upload-artifact) an attacker could access it and use it
  • persist-credentials: false avoids writing the tokens on files and prevents token leaks

There are a few more caveats here and there, but this is already more than enough to justify the use of persist-credentials: false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! Your summary is great and matches my understanding perfectly 🙂

- name: Check for source changes
id: check
run: |
Expand Down
14 changes: 10 additions & 4 deletions .github/workflows/reusable-docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@ jobs:
env:
branch_base: 'origin/${{ github.event.pull_request.base.ref }}'
branch_pr: 'origin/${{ github.event.pull_request.head.ref }}'
commits: ${{ github.event.pull_request.commits }}
refspec_base: '+${{ github.event.pull_request.base.sha }}:remotes/origin/${{ github.event.pull_request.base.ref }}'
refspec_pr: '+${{ github.event.pull_request.head.sha }}:remotes/origin/${{ github.event.pull_request.head.ref }}'
steps:
- name: 'Check out latest PR branch commit'
uses: actions/checkout@v4
with:
persist-credentials: false
ref: >-
${{
github.event_name == 'pull_request'
Expand All @@ -39,15 +41,15 @@ jobs:
if: github.event_name == 'pull_request'
run: |
# Fetch enough history to find a common ancestor commit (aka merge-base):
git fetch origin ${{ env.refspec_pr }} --depth=$(( ${{ github.event.pull_request.commits }} + 1 )) \
git fetch origin "${refspec_pr}" --depth=$(( commits + 1 )) \
--no-tags --prune --no-recurse-submodules

# This should get the oldest commit in the local fetched history (which may not be the commit the PR branched from):
COMMON_ANCESTOR=$( git rev-list --first-parent --max-parents=0 --max-count=1 ${{ env.branch_pr }} )
COMMON_ANCESTOR=$( git rev-list --first-parent --max-parents=0 --max-count=1 "${branch_pr}" )
DATE=$( git log --date=iso8601 --format=%cd "${COMMON_ANCESTOR}" )

# Get all commits since that commit date from the base branch (eg: master or main):
git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \
git fetch origin "${refspec_base}" --shallow-since="${DATE}" \
--no-tags --prune --no-recurse-submodules
- name: 'Set up Python'
uses: actions/setup-python@v5
Expand All @@ -69,7 +71,7 @@ jobs:
if: github.event_name == 'pull_request'
run: |
python Doc/tools/check-warnings.py \
--annotate-diff '${{ env.branch_base }}' '${{ env.branch_pr }}' \
--annotate-diff "${branch_base}" "${branch_pr}" \
--fail-if-regression \
--fail-if-improved \
--fail-if-new-news-nit
Expand All @@ -81,6 +83,8 @@ jobs:
timeout-minutes: 60
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: 'Set up Python'
uses: actions/setup-python@v5
with:
Expand All @@ -99,6 +103,8 @@ jobs:
timeout-minutes: 60
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/cache@v4
with:
path: ~/.cache/pip
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/reusable-macos.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ jobs:
runs-on: ${{ inputs.os }}
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Runner image version
run: echo "IMAGE_VERSION=${ImageVersion}" >> "$GITHUB_ENV"
- name: Restore config.cache
Expand Down
9 changes: 7 additions & 2 deletions .github/workflows/reusable-tsan.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@ jobs:
name: 'Thread sanitizer'
runs-on: ubuntu-24.04
timeout-minutes: 60
env:
OPTIONS: ${{ inputs.options }}
SUPPRESSIONS_PATH: ${{ inputs.suppressions_path }}
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Runner image version
run: echo "IMAGE_VERSION=${ImageVersion}" >> "$GITHUB_ENV"
- name: Restore config.cache
Expand All @@ -47,7 +52,7 @@ jobs:
sudo sysctl -w vm.mmap_rnd_bits=28
- name: TSAN Option Setup
run: |
echo "TSAN_OPTIONS=log_path=${GITHUB_WORKSPACE}/tsan_log suppressions=${GITHUB_WORKSPACE}/${{ inputs.suppressions_path }} handle_segv=0" >> "$GITHUB_ENV"
echo "TSAN_OPTIONS=log_path=${GITHUB_WORKSPACE}/tsan_log suppressions=${GITHUB_WORKSPACE}/${SUPPRESSIONS_PATH} handle_segv=0" >> "$GITHUB_ENV"
echo "CC=clang" >> "$GITHUB_ENV"
echo "CXX=clang++" >> "$GITHUB_ENV"
- name: Add ccache to PATH
Expand All @@ -59,7 +64,7 @@ jobs:
save: ${{ github.event_name == 'push' }}
max-size: "200M"
- name: Configure CPython
run: ${{ inputs.options }}
run: "${OPTIONS}"
- name: Build CPython
run: make -j4
- name: Display build info
Expand Down
4 changes: 3 additions & 1 deletion .github/workflows/reusable-ubuntu.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ jobs:
TERM: linux
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Register gcc problem matcher
run: echo "::add-matcher::.github/problem-matchers/gcc.json"
- name: Install dependencies
Expand Down Expand Up @@ -94,7 +96,7 @@ jobs:
if: ${{ !inputs.free-threading }}
run: >-
python Tools/build/check_warnings.py
--compiler-output-file-path=${{ env.CPYTHON_BUILDDIR }}/compiler_output_ubuntu.txt
--compiler-output-file-path="${CPYTHON_BUILDDIR}/compiler_output_ubuntu.txt"
--warning-ignore-file-path "${GITHUB_WORKSPACE}/Tools/build/.warningignore_ubuntu"
--compiler-output-type=gcc
--fail-on-regression
Expand Down
12 changes: 7 additions & 5 deletions .github/workflows/reusable-wasi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ jobs:
CROSS_BUILD_WASI: cross-build/wasm32-wasip1
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
# No problem resolver registered as one doesn't currently exist for Clang.
- name: "Install wasmtime"
uses: bytecodealliance/actions/wasmtime/setup@v1
Expand All @@ -34,9 +36,9 @@ jobs:
- name: "Install WASI SDK" # Hard-coded to x64.
if: steps.cache-wasi-sdk.outputs.cache-hit != 'true'
run: |
mkdir ${{ env.WASI_SDK_PATH }} && \
curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sdk-${{ env.WASI_SDK_VERSION }}.0-x86_64-linux.tar.gz | \
tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip
mkdir "${WASI_SDK_PATH}" && \
curl -s -S --location "https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${WASI_SDK_VERSION}/wasi-sdk-${WASI_SDK_VERSION}.0-x86_64-linux.tar.gz" | \
tar --strip-components 1 --directory "${WASI_SDK_PATH}" --extract --gunzip
- name: "Configure ccache action"
uses: hendrikmuhs/ccache-action@v1.2
with:
Expand Down Expand Up @@ -72,6 +74,6 @@ jobs:
- name: "Make host"
run: python3 Tools/wasm/wasi.py make-host
- name: "Display build info"
run: make --directory ${{ env.CROSS_BUILD_WASI }} pythoninfo
run: make --directory "${CROSS_BUILD_WASI}" pythoninfo
- name: "Test"
run: make --directory ${{ env.CROSS_BUILD_WASI }} test
run: make --directory "${CROSS_BUILD_WASI}" test
5 changes: 4 additions & 1 deletion .github/workflows/reusable-windows-msi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ jobs:
runs-on: windows-latest
timeout-minutes: 60
env:
ARCH: ${{ inputs.arch }}
IncludeFreethreaded: true
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Build CPython installer
run: .\Tools\msi\build.bat --doc -${{ inputs.arch }}
run: .\Tools\msi\build.bat --doc -"${ARCH}"
10 changes: 8 additions & 2 deletions .github/workflows/reusable-windows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,24 +26,30 @@ jobs:
name: 'build and test (${{ inputs.arch }})'
runs-on: ${{ inputs.os }}
timeout-minutes: 60
env:
ARCH: ${{ inputs.arch }}
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- name: Register MSVC problem matcher
if: inputs.arch != 'Win32'
run: echo "::add-matcher::.github/problem-matchers/msvc.json"
- name: Build CPython
run: >-
.\\PCbuild\\build.bat
-e -d -v
-p ${{ inputs.arch }}
-p "${ARCH}"
${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }}
shell: bash
- name: Display build info # FIXME(diegorusso): remove the `if`
if: inputs.arch != 'arm64'
run: .\\python.bat -m test.pythoninfo
- name: Tests # FIXME(diegorusso): remove the `if`
if: inputs.arch != 'arm64'
run: >-
.\\PCbuild\\rt.bat
-p ${{ inputs.arch }}
-p "${ARCH}"
-d -q --fast-ci
${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }}
shell: bash
5 changes: 2 additions & 3 deletions .github/workflows/stale.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ on:
schedule:
- cron: "0 */6 * * *"

permissions:
pull-requests: write

jobs:
stale:
if: github.repository_owner == 'python'

runs-on: ubuntu-latest
permissions:
pull-requests: write
timeout-minutes: 10

steps:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/verify-ensurepip-wheels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ jobs:
timeout-minutes: 10
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-python@v5
with:
python-version: '3'
Expand Down
6 changes: 6 additions & 0 deletions .github/zizmor.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Configuration for the zizmor static analysis tool, run via pre-commit in CI
# https://woodruffw.github.io/zizmor/configuration/
rules:
dangerous-triggers:
ignore:
- documentation-links.yml
Loading
Loading