Hi
I'm also highly sceptical of wasting CPU time on such an automated scan.
Once the few universally useful low-hanging fruits are picked, only the
non-low-hanging fruits remain by definition. If they are not implemented
reasonably timely then there is likely a reason for that, be that
technical or processual. Then there is also no need for the reports to
sit within the Security tab forever and also no need for them to be
rechecked by maintainers regularly. Then there's also the issue of
what's effectively false-positives (see below) which further distract
from whatever benefit to the automated scan there might be in the first
place.
On 10/24/22 19:05, Pedro Nacht via internals wrote:
- set GitHub workflow dependencies to adopt hash-pinning instead of
major-version pinning. This is to eliminate the risk of tag-hijacking
attacks where a malicious commit is published to an Action you rely on and
the tag is then recreated to point to that malicious commit. Dependabot or
Renovatebot (which the tool also recommends adding to the project) can then
be used to keep your dependencies up-to-date.
Is there any practical benefit to this when all the workflows are
read-only with regard to the repository contents?
On the contrary I believe that hashes make it much harder to verify
which major version of an action is used, e.g. to check the changelog
for any relevant breaking changes before upgrading the action.
- set all GitHub workflows with top-level read-only permissions. It
recognizes that almost all PHP's workflows have top-level read-only
permissions, but points out that github/workflows/labeler.yml doesn't
(though it does have job-level contents read-only permissions).
I consider that a false-positive then, because the workflows *is* secure
in practice. No need to waste maintainers time with the busy-work of
moving the 'permissions' section around.
I can see the small benefit of consistency across workflows, though and
will likely send a PR to unify this if no one beats me to it.
- and yes, enforce Code-Review and maximal Branch-Protection. I
understand this would be quite impactful on the project's current workflow,
but it's the sort of thing that would mitigate the sort of
attack @ricardoboss mentioned in the linked GH issue. Whether the costs are
worth the benefit is a question you are all certainly better equipped to
determine than me.
I would hope none of the core contributers disputes the benefit of code
review, so … there is no need for a tool to tell them what they already
know.
However, others offer more continuous monitoring in case there's an
accidental slip-up: if any new workflows are added to the project in the
future without minimal permissions or without pinning dependencies, for
example, the Action will update the Security Dashboard with an alert.
The repository is already configured to only grant "read" permissions to
the workflow by default using this setting:
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#configuring-the-default-github_token-permissions
I believe this is a much more reliable solution than expecting the
maintainers to regularly check the Security Tab and noticing that a new
warning popped up.
The proposed automated scanner does not appear to detect that this
setting is enabled, thus effectively making the labeler.yml report a
double false-positive.
Best regards
Tim Düsterhus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
