-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Improve CI security and maintanability #8034
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
base: main
Are you sure you want to change the base?
Conversation
Bumps the actions-dependencies group with 5 updates: | Package | From | To | | --- | --- | --- | | [actions/checkout](https://github.com/actions/checkout) | `4` | `6` | | [actions/setup-python](https://github.com/actions/setup-python) | `5` | `6` | | [actions/upload-artifact](https://github.com/actions/upload-artifact) | `4` | `5` | | [github/codeql-action](https://github.com/github/codeql-action) | `3` | `4` | | [codecov/codecov-action](https://github.com/codecov/codecov-action) | `4` | `5` | Updates `actions/checkout` from 4 to 6 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v6) Updates `actions/setup-python` from 5 to 6 - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v5...v6) Updates `actions/upload-artifact` from 4 to 5 - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v4...v5) Updates `github/codeql-action` from 3 to 4 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@v3...v4) Updates `codecov/codecov-action` from 4 to 5 - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v4...v5) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-dependencies - dependency-name: actions/setup-python dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-dependencies - dependency-name: actions/upload-artifact dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-dependencies - dependency-name: github/codeql-action dependency-version: '4' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-dependencies - dependency-name: codecov/codecov-action dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-dependencies ... Signed-off-by: dependabot[bot] <[email protected]>
…ons-dependencies-7aa040f97f actions: bump the actions-dependencies group with 5 updates
Bumps the actions-dependencies group with 1 update: [actions/cache](https://github.com/actions/cache). Updates `actions/cache` from 4 to 5 - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@v4...v5) --- updated-dependencies: - dependency-name: actions/cache dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-dependencies ... Signed-off-by: dependabot[bot] <[email protected]>
…ons-dependencies-97f38a5d32 actions: bump actions/cache from 4 to 5 in the actions-dependencies group
Signed-off-by: StepSecurity Bot <[email protected]>
…urity-remediation [StepSecurity] ci: Harden GitHub Actions
|
|
Thanks for your contribution. I am not in favor of pinning (at least for this project) because it will unnecessary noise (lots of PRs), work (see next point) and might actually cause problems in case we rely on the upstream to be updated to get a fix. We are usually going with rolling updates on a certain version/branch. The heightened security is not necessary as we are not automatically uploading/writing to any repo which could be poisoned or secrets which could be exposed (I think the jobs are all read-only). And these changes actually decrease the maintainability. IMO pinning is also is not making much sense. Sure it might protect from pulling a possibly poisoned version without your knowledge. But if you pin a version and it was already poisoned you need to do manual work to make sure that you will be using the bad version. And if something breaks in the pinned version because of some external dependency you also need to check all the time all the time and update branches so they keep working instead of "fixing itself" when the fix was supplied. And if the bot generates a PR there is not much point for us in checking the changes and they will probably just be merged so they won't pile up - so there is no point on not just using the branch. And the obfuscation by the using the hash instead of a proper version is a really bad thing as it makes things harder to understand - a tag would be better (but those could also be poisoned). |
|
Also bumping a lot of different versions of multiple modules in a single merge is also not great since it makes it harder to bisect potential issues later on and produces less "documentation" via the commit log. |
|
Something that would be very helpful thought, if you could configure dependabot to generate a PR when a new high-level branch (e.g. |
|
Hi firewave, thanks for the feedback, appreciate it
Regarding this point, Dependabot is configured to group updates in a single PR opened once a week, and this configuration can be changed to whatever rhythm would work for you. I specifically chose this configuration to avoid too much noise in the first place ;)
Only for Cygwin and ClusterFuzz, if I'm not mistaken ? While I understand the logic for ClusterFuzz (it's the recommended way in the documentation, and they do not version using tags AFAICT), I fail to see why it is the case for Cygwin (it's the way mentioned in the README, but they also version using tags, and there is not much active development happening on the master branch since July). But fair point, I can revert the change for these Actions.
These are also fair points. The only comment I have on this, is that by using only the major tag (like v6), you do not have a reproducible pipeline, and you actually do not know which version you are using when the workflow is triggered. Could be v6.0.1, v6.1.0 or whatever. It might be fine if the the Action maintainer is following the semantic versioning rules and is not introducing breaking changes for minor versions, but you might also run into regressions introduced from v6.0.x to v6.1.x, for example.
The version is also provided in comments next to the hash, and is updated by Dependabot in its PRs, so you do not lose any information about the version being used compared to what you already have today (you actually have more infos, as it provides the full tag)
Fair point, would you prefer multiple commits updating one Action per commit, or multiple PRs instead ?
I believe this should be just a matter of cherry-picking the Dependabot config proposed in this PR with your current workflows, I'll try in my fork and let you know. Since we are on the topic of the CI, I have some comments that could lead to several minor improvements (I can open an issue or a discussion for them, but I thought it could be "pre-discussed" here): 1. About the Python Action and pip packages The latest v6.1.0 release for the Could be removed and replaced with the following Action definition: I can also suggest to use the Additionally, the scriptcheck workflow is currently defining the following Python versions as a matrix strategy:
Versions 3.7, 3.8 and 3.9 are now end of life and unsupported, is there a potential cleanup to do here ? 2. About the Runners OSes Most of the workflows running on Ubuntu are using v22.04 for the Is there any updating or harmonization that would be useful here (migrating v22.04 to v24.04 or Regarding MacOS, all jobs are using macos-15, except for Regarding Windows, all jobs or matrixes are using windows-2025 (with addition of windows-2022 for matrixes) so this seems fine. 3. About the TODOs There are quite a lot of TODOs in the workflow files, some of them seems outdated and/or fixable, for example in the CI-Windows file:
Would it be useful to revisit them ? Sorry for the long post, and thanks for the feedback ! Footnotes
|



Hi !
This PR introduce the following changes to the CI workflow files:
Let me know if you have any questions or remarks :)
Good weekend everyone !
Footnotes
Cf. https://docs.github.com/en/actions/reference/security/secure-use#using-third-party-actions ↩
Cf. https://app.stepsecurity.io/securerepo ↩