Tim, Thank you for the feedback, I'll be sure to pass it on to the Scorecards team.
And if there's interest, I'll happily fix that small inconsistency in the labeler.yml permissions. And with this I believe I'll go ahead and close the GH issue and PR. I'll stay in the mailing list for a few more days if anyone has any further questions or comments. Truly thank you everyone for your time, Pedro On Thu, Oct 27, 2022 at 7:32 PM Tim Düsterhus <t...@bastelstu.be> wrote: > 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 >
