Skip to content
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

#62221 GitHub Actions workflow hardening #8007

Closed
wants to merge 52 commits into from

Conversation

johnbillion
Copy link
Member

@johnbillion johnbillion commented Dec 16, 2024

What this does:

  • Removes all usage of inline GitHub Actions expressions in scripts, either replacing them with environment variables or refactoring the affected code. Actions expressions are not safe to use inline inside scripts.
  • Addresses all instances of missing quotes around variables in scripts to prevent word splitting.
  • Adds some missing permissions: {} declarations to workflows and jobs.
  • Introduces Actionlint for static analysis of workflow files. It detects syntax errors, semantic errors with expressions, inputs, outputs, and secrets, and includes several security checks.
  • Fixes a few issues detected by Actionlint that don't relate to security, for example references to non-existent matrix values and inputs.
  • Standardises on using maps for the environment properties in docker-compose.yml.
  • Refactors and reformats a few areas of code for legibility and clarity.

Notes

This PR previously included additional static analysis of workflow files using Octoscan, Zizmor, and Poutine, but I've removed these and I'll be proposing them separately at a later date.


Trac ticket: https://core.trac.wordpress.org/ticket/62221

This comment was marked as off-topic.

@github-advanced-security

This comment was marked as outdated.

@johnbillion johnbillion marked this pull request as ready for review January 21, 2025 12:32
Copy link

github-actions bot commented Jan 21, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props johnbillion, swissspidy, flixos90, desrosj.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@johnbillion johnbillion requested review from swissspidy, felixarntz, aaronjorbin and desrosj and removed request for felixarntz January 21, 2025 13:17
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@johnbillion Based on my limited GH Actions knowledge, this looks reasonable to me.

Since it covers all the different workflows, I think we need to do some extra post-commit verification though. Obviously all workflows should pass, but there are other aspects that we also need to test that wouldn't cause a workflow to fail - such as ensuring the performance data for each commit is still sent to the Code Vitals dashboard as expected.

Comment on lines -335 to -339
- name: Set commit details
# Only needed when publishing results.
if: ${{ github.event_name == 'push' && github.ref == 'refs/heads/trunk' && ! inputs.memcached && ! inputs.multisite }}
# Write to an environment variable to have the output available in later steps of the job.
run: echo "COMMITTED_AT=$(git show -s $GITHUB_SHA --format='%cI')" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

This was just recently modified to fix a problem in https://core.trac.wordpress.org/changeset/59582. Your change looks right, but let's double-check that the commit data is still sent correctly after this has been committed to Core.

@@ -64,6 +64,10 @@ env:
LOCAL_PHP: ${{ inputs.php-version }}${{ 'latest' != inputs.php-version && '-fpm' || '' }}
LOCAL_MULTISITE: ${{ inputs.multisite }}

# Disable permissions for all available scopes by default.
# Any needed permissions should be configured at the job level.
permissions: {}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite familiar with how this permissions element works in GH Actions. Makes sense that it's an empty default for security. But I'm curious, for what kind of steps or jobs would one need to override this?

Copy link
Member

Choose a reason for hiding this comment

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

Typically for any write action, like pushing changes to the repository or writing PR comments or changing labels.

By disabling that by default if not needed, this reduces risk of privilege escalation, where for example one can suddenly push changes to a repository through a workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some really fine-grained permissions available, so there's a good number of possibilities.

We also use contents: read permissions for the Slack notifications workflow so that the steps can read previous workflow runs and determine which message to send to Slack.

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

Thanks for this, @johnbillion! Looks really great. I left mostly questions, some inline documentation additions, and a few suggestions. Feel free to commit after going through them!

.github/workflows/workflow-lint.yml Outdated Show resolved Hide resolved
.github/workflows/workflow-lint.yml Outdated Show resolved Hide resolved

# https://github.com/rhysd/actionlint
- name: Run actionlint
uses: docker://rhysd/actionlint:1.7.7
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if this get picked up by Dependabot as currently configured? It's not clear in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unclear. I'll add it to a list of a few things I've got to follow up on.

.github/workflows/reusable-workflow-lint.yml Outdated Show resolved Hide resolved
.github/workflows/reusable-workflow-lint.yml Outdated Show resolved Hide resolved
.github/workflows/reusable-workflow-lint.yml Outdated Show resolved Hide resolved
.github/workflows/reusable-workflow-lint.yml Show resolved Hide resolved
.github/workflows/reusable-workflow-lint.yml Show resolved Hide resolved
.github/workflows/workflow-lint.yml Outdated Show resolved Hide resolved
@johnbillion johnbillion mentioned this pull request Jan 22, 2025
@johnbillion
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants