Skip to content

chore(ci): hardening workflows using zizmor - #2292

Merged
soonum merged 10 commits into
mainfrom
dt/ci/check_workflows_security
May 6, 2025
Merged

soonum merged 10 commits into
mainfrom
dt/ci/check_workflows_security

Conversation

@soonum

@soonum soonum commented Apr 25, 2025 •

Copy link
Copy Markdown
Collaborator

This is part of security issues remediation campaign after having analyzed workflows using zizmor cargo tool.


This change is Reviewable

@soonum soonum added the ci label Apr 25, 2025
@soonum soonum self-assigned this Apr 25, 2025
@cla-bot cla-bot Bot added the cla-signed label Apr 25, 2025
@soonum
soonum marked this pull request as draft April 25, 2025 12:37
@soonum
soonum force-pushed the dt/ci/check_workflows_security branch 4 times, most recently from 05012f2 to df9cfd9 Compare April 25, 2025 15:50
@soonum
soonum marked this pull request as ready for review April 25, 2025 15:51

@IceTDrinker IceTDrinker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks that's already a huge bit you did here

Reviewed 52 of 64 files at r1, 18 of 18 files at r2, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @nsarlin-zama)


.github/workflows/aws_tfhe_backward_compat_tests.yml line 131 at r2 (raw file):

          echo "PULL_REQUEST_MD_LINK=[pull-request](${PR_BASE_URL}${{ github.event.pull_request.number }}), "  >> "${GITHUB_ENV}"
        env:
          PR_BASE_URL: ${{ vars.PR_BASE_URL }}

what's vars ?


.github/workflows/benchmark_gpu.yml line 78 at r2 (raw file):

        run: |
          echo "${INPUTS_PROFILE}" | sed 's|\(.*\)[[:space:]](.*)|\1|'
          echo "profile=$?" >> "${GITHUB_OUTPUT}"

$? is the exit code ?


.github/workflows/benchmark_gpu.yml line 84 at r2 (raw file):

        run: |
          echo "${INPUTS_PROFILE}" | sed 's|.*[[:space:]](\(.*\))|\1|'
          echo "name=$?" >> "${GITHUB_OUTPUT}"

same here


.github/workflows/benchmark_gpu.yml line 99 at r2 (raw file):

      all_precisions: ${{ inputs.all_precisions }}
    secrets:
      BOT_USERNAME: ${{ secrets.BOT_USERNAME }}

this is a way to restrict secret access ?


.github/actions/gpu_setup/action.yml line 47 at r1 (raw file):

      shell: bash
      run: |
        CUDA_PATH=/usr/local/cuda-${CUDA_VERSION}

unquotted var expand


.github/workflows/approve_label.yml line 10 at r1 (raw file):

permissions: {}

what are empty permissions ?


.github/actionlint.yaml line 22 at r1 (raw file):

      # Sed is used to extract a value from a string in our case, this task cannot be
      # performed by the ${variable//search/replace} pattern.
      - 'shellcheck reported issue in this script: SC2001:.+'

let's avoid disabling lints, if there are other ways to perform the same thing let's use that, otherwise you may use python as the shell for a step


.github/workflows/benchmark_signed_integer.yml line 66 at r2 (raw file):

        run: |
          echo "OP_FLAVOR=[\"default\"]" >> "${GITHUB_ENV}"
          if [[ "${INPUTS_BENCH_TYPE}" == "both" ]]; then

missing env var setup ?


.github/workflows/benchmark_integer.yml line 66 at r2 (raw file):

        run: |
          echo "OP_FLAVOR=[\"default\"]" >> "${GITHUB_ENV}"
          if [[ "${INPUTS_BENCH_TYPE}" == "both" ]]; then

missing env var setup ?


.github/workflows/aws_tfhe_backward_compat_tests.yml line 90 at r1 (raw file):

        id: backward_compat_sha
        run: |
          SHA=$(git ls-remote ${{ env.REPO_URL }} refs/heads/"${BACKWARD_COMPAT_BRANCH}" | awk '{print $1}')

replace the ${{ env.REPO_URL }} by ${REPO_URL}, also make sure it is quoted ?

or maybe replace by the url directly ?


.github/workflows/aws_tfhe_backward_compat_tests.yml line 108 at r1 (raw file):

        with:
          persist-credentials: 'false'
          token: ${{ env.CHECKOUT_TOKEN }}

${{ secrets.CHECKOUT_TOKEN }}

?


.github/workflows/benchmark_shortint.yml line 123 at r2 (raw file):

          --database tfhe_rs \
          --hardware "hpc7a.96xlarge" \
          --project-version "${{ env.COMMIT_HASH }}" \

don't use env. when it's an env var, use the ${} expansion directly


.github/workflows/benchmark_shortint.yml line 125 at r2 (raw file):

          --project-version "${{ env.COMMIT_HASH }}" \
          --branch "${REF_NAME}" \
          --commit-date ${{ env.COMMIT_DATE }} \

here as well


.github/workflows/benchmark_shortint.yml line 126 at r2 (raw file):

          --branch "${REF_NAME}" \
          --commit-date ${{ env.COMMIT_DATE }} \
          --bench-date "${{ env.BENCH_DATE }}" \

and again


.github/workflows/benchmark_gpu_erc20.yml line 36 at r2 (raw file):

        run: |
          echo "${INPUTS_PROFILE}" | sed 's|\(.*\)[[:space:]](.*)|\1|'
          echo "profile=$?" >> "${GITHUB_OUTPUT}"

this is the exit code


.github/workflows/benchmark_gpu_erc20.yml line 42 at r2 (raw file):

        run: |
          echo "${INPUTS_PROFILE}" | sed 's|.*[[:space:]](\(.*\))|\1|'
          echo "name=$?" >> "${GITHUB_OUTPUT}"

this is the exit code


.github/workflows/benchmark_zk_pke.yml line 80 at r2 (raw file):

        if: github.event_name == 'workflow_dispatch'
        run: |
          if [[ "${INPUTS_BENCH_TYPE}" == "both" ]]; then

missing env var setup ?

@soonum

soonum commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator Author

.github/workflows/approve_label.yml line 10 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

what are empty permissions ?

This blocks all permissions for GITHUB_TOKEN, which are very (very) broad by default.

@soonum

soonum commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator Author

.github/workflows/aws_tfhe_backward_compat_tests.yml line 90 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

replace the ${{ env.REPO_URL }} by ${REPO_URL}, also make sure it is quoted ?

or maybe replace by the url directly ?

Sure

@soonum

soonum commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator Author

.github/workflows/aws_tfhe_backward_compat_tests.yml line 108 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

${{ secrets.CHECKOUT_TOKEN }}

?

Nope, look in the top-level env directive. I did it that way to deal with secret accessibility in case of PR from external contributors.

@soonum

soonum commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator Author

.github/workflows/aws_tfhe_backward_compat_tests.yml line 131 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

what's vars ?

It's an environment variable set up at the repository level (like any secrets) but it is handled as plaintext.
We choose so since the PR URL is public anyway.

@soonum

soonum commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator Author

.github/workflows/benchmark_gpu.yml line 78 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

$? is the exit code ?

Hmm you're right, I've to find another way to do what I want. Maybe a python script like you mention above.
Basically, I attempted this implementation to deal with double-quoting issues.

@soonum

soonum commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator Author

.github/workflows/benchmark_gpu.yml line 99 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

this is a way to restrict secret access ?

What do you mean ?

@soonum

soonum commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator Author

.github/workflows/benchmark_shortint.yml line 123 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

don't use env. when it's an env var, use the ${} expansion directly

Ok.

@soonum
soonum force-pushed the dt/ci/check_workflows_security branch 4 times, most recently from 7e200bb to dc3eaab Compare May 2, 2025 12:25

@soonum soonum left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reviewable status: 52 of 64 files reviewed, 5 unresolved discussions (waiting on @IceTDrinker and @nsarlin-zama)


.github/actionlint.yaml line 22 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

let's avoid disabling lints, if there are other ways to perform the same thing let's use that, otherwise you may use python as the shell for a step

The only simple solution I can think of, is to reduce the globing of files to specific files.


.github/workflows/benchmark_gpu.yml line 99 at r2 (raw file):

Previously, soonum (David Testé) wrote…

What do you mean ?

Oh I got it. Yes you're right. If we use secrets: inherits then the sub workflow can see all of our secrets. This situation is not problematic as long as the sub-workflows are tfhe-rs ones.

@soonum
soonum requested a review from IceTDrinker May 2, 2025 12:26
@soonum
soonum force-pushed the dt/ci/check_workflows_security branch 3 times, most recently from b0ccfa1 to 7ed637d Compare May 2, 2025 13:28

@IceTDrinker IceTDrinker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed 7 of 12 files at r4, 31 of 35 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nsarlin-zama)


.github/workflows/ci_lint.yml line 35 at r6 (raw file):

      - name: Lint workflows
        run: |
          make lint_workflow

this does not run zizmor ? maybe group lints and security under lint_workflow ?


.github/actions/gpu_setup/action.yml line 29 at r6 (raw file):

        sha256sum -c checksum
        bash cmake-"${CMAKE_VERSION}"-linux-x86_64.sh --skip-license
        sudo ln -s cmake-"${CMAKE_VERSION}"-linux-x86_64/bin/cmake /usr/local/bin

unclear this ln is correct, also you could put in in $HOME/.local/bin to avoid the sudo stuff


.github/actions/gpu_setup/action.yml line 34 at r6 (raw file):

      env:
        CMAKE_VERSION: ${{ inputs.cmake-version }}
        CMAKE_SCRIPT_SHA: "6e4fada5cba3472ae503a11232b6580786802f0879cead2741672bf65d97488a"

having a variable version but a fixed sha seems weird, should be a fixed version as well ?

@soonum
soonum force-pushed the dt/ci/check_workflows_security branch from 841015c to 32f58d9 Compare May 5, 2025 08:22
@soonum
soonum force-pushed the dt/ci/check_workflows_security branch 4 times, most recently from 54a595c to bcc6fcf Compare May 5, 2025 12:09
@soonum
soonum requested a review from IceTDrinker May 5, 2025 12:10
soonum added 10 commits May 5, 2025 18:03
This is part of security issues remediation campaign after having
analyzed workflow using zizmor cargo tool.
Usage of bash script to download and extract the final binary has
been dropped.
Instead, the tarball is directly fetched according to the
ACTIONLINT_VERSION value and the integrity of the tarball is
checked with an hardcoded SHA256 sum.
This is redundant with the use of gpu_setup.yml action.
@soonum
soonum force-pushed the dt/ci/check_workflows_security branch from bcc6fcf to d099228 Compare May 5, 2025 16:03

@IceTDrinker IceTDrinker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

Reviewed 2 of 2 files at r8, 5 of 5 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nsarlin-zama)

@soonum
soonum merged commit ce6454c into main May 6, 2025
@soonum
soonum deleted the dt/ci/check_workflows_security branch May 6, 2025 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants