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

Add COMPOSER_AUTH env variable to workflows #200

Merged
merged 1 commit into from
Sep 14, 2024
Merged

Add COMPOSER_AUTH env variable to workflows #200

merged 1 commit into from
Sep 14, 2024

Conversation

Potherca
Copy link
Member

@Potherca Potherca commented Jan 5, 2023

Proposed Changes

Because of the amount of GitHub API calls composer does, the CI runs into a rate limit. This causes jobs to fail or be aborted. Adding the GITHUB_TOKEN as auth method should avoid these rate limits.

Related Issues

@Potherca Potherca requested a review from jrfnl January 5, 2023 10:41
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Reviewed:

  1. While I'm not sure it will solve all the issues (the most recent ones point to an issue with powershell ?), I totally agree that this should help.
  2. Do we really need it in all workflows though ? Or only in the integrationtest.yml file ? (which is the only one which has been giving us trouble)
  3. Is setting it via env: COMPOSER_AUTH the right way to set this ? Or should we leverage the setup-php action for this ? (which can set the Composer authentication up persistently)
    Ref: https://github.com/shivammathur/setup-php#github-composer-authentication

@Potherca
Copy link
Member Author

Potherca commented Jan 5, 2023

Accidentaly pressed the "Request Review" button again. Sorry!

@Potherca Potherca added this to the Not Version Related milestone Jan 5, 2023
@Potherca
Copy link
Member Author

Potherca commented Jan 5, 2023

From what I could see, the error in the powershell were triggered by composer, caused by the rate limit:

Run shivammathur/setup-php@v2
"C:\Program Files\PowerShell\7\pwsh.exe" D:\a_actions\shivammathur\setup-php\v2\src\scripts\run.ps1

...
==> Setup Tools
✗ composer 403: rate limit exceeded
Error: The process 'C:\Program Files\PowerShell\7\pwsh.exe' failed with exit code 1

https://github.com/PHPCSStandards/composer-installer/actions/runs/3845986458/jobs/6550770322#step:3:27

The fix was proposed in ramsey/composer-install#182 (comment) but sadly this change does not seem to fix the rate limit issue so another approach seems needed. I'll take a look at the setup-php docs.

I just added it to all workflows that do a composer install/update just to be safe (as I have no idea how the rate limit between GitHub / Composer / GHA.

Leaving this for now.

@Potherca Potherca marked this pull request as draft January 5, 2023 11:13
@Potherca Potherca marked this pull request as ready for review August 26, 2024 09:50
@Potherca
Copy link
Member Author

Potherca commented Aug 26, 2024

@jrfnl I'm inclined to merge this as-is (as it shouldn't do any harm) or close it, and resolve any other items discussed here later (if at all).

@jrfnl
Copy link
Member

jrfnl commented Sep 10, 2024

I'm inclined to merge this as-is (as it shouldn't do any harm) or close it, and resolve any other items discussed here later (if at all).

@Potherca In principle, I'm fine with that, though I do have two niggly questions:

  • Could always providing the github-oauth have security implications ?
  • We're not limiting the workflows to only run in this repo (excluding forks), so could this "leak" the GITHUB_TOKEN to forks ?

Other than that, this probably should be rebased before merging. It also looks like one workflow was missed - linting.yaml -, though that may be deliberate as that workflow uses only docker containers ?

Because of the amount of GitHub API calls composer does, the CI runs into a rate limit.
This causes jobs to fail or be aborted. By adding the GITHUB_TOKEN as auth method, this can be avoided.
@Potherca
Copy link
Member Author

Good questions!

Some basic points, before answering the individual questions:

  1. When you enable GitHub Actions, GitHub installs a GitHub App on your repository. The GITHUB_TOKEN secret is a GitHub App installation access token.
  2. At the start of each workflow job, GitHub automatically creates a unique GITHUB_TOKEN secret to use in your workflow.
  3. The token's permissions are limited to the repository that contains your workflow.
  4. The GITHUB_TOKEN expires when a job finishes or after a maximum of 24 hours.

Dource: https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication

So, to answer the questions, in order:

  • Could always providing the github-oauth have security implications ?

    • It shouldn't... That is to say, using HTTPS means the token isn't broadcast in the clear and the receiver (packagist) is a trusted party. However, if either ramsey/composer-install, Composer, or Packagist are compromised, then Yes. We would be compromised together with all other users of those services. The impact would be limited to the repository, for the duration of its validity (see 3. and 4.).

      This scenario could be mitigated by setting "Workflow permissions" (in the "Actions permissions" settings to read-only. Assuming we don't do any writing in the actions this should not break our CI/CD.

  • We're not limiting the workflows to only run in this repo (excluding forks), so could this "leak" the GITHUB_TOKEN to forks ?

    • No. The secrets.GITHUB_TOKEN in the fork would only work for that repo. (see 1. and 2.) As it is a secret it will not be shown in the action's output. This can be validated by adding a - run: echo ${{ secrets.GITHUB_TOKEN }} to any action.
  • Other than that, this probably should be rebased before merging.

    • Done.
  • It also looks like one workflow was missed - linting.yaml -, though that may be deliberate as that workflow uses only docker containers ?

    • That is correct. As it does not do a composer install, it does not require a token

@jrfnl
Copy link
Member

jrfnl commented Sep 13, 2024

@Potherca Thanks for that additional context. I've seen some interesting scenario's floating around about vulnerabilities and GHA, so yes, let's err on the side of caution and limit those workflow permissions.

@Potherca
Copy link
Member Author

@jrfnl FYI: I've just set the "Workflow permissions" for the GITHUB_TOKEN in this repo to read-only. So if the build starts randomly breaking (YOLO) this might be good to know.

@jrfnl
Copy link
Member

jrfnl commented Sep 14, 2024

@jrfnl FYI: I've just set the "Workflow permissions" for the GITHUB_TOKEN in this repo to read-only. So if the build starts randomly breaking (YOLO) this might be good to know.

Thanks and should be straight-forward to check by triggering a build for each workflow (which I've done just now). I'll check in a couple of hours if the builds have passed and if so, I'll merge this PR.

@jrfnl jrfnl merged commit c4a2452 into main Sep 14, 2024
101 checks passed
@jrfnl jrfnl deleted the ci/composer-auth branch September 14, 2024 23:48
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.

2 participants