Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118892
Message-ID: <19932b6e-efe8-d24a-0e1e-e8960fb1566b@bastelstu.be>
Date: Fri, 28 Oct 2022 00:32:18 +0200
MIME-Version: 1.0
Content-Language: en-US
To: Pedro Nacht <pnacht@google.com>, Jordan LeDoux <jordan.ledoux@gmail.com>
Cc: internals@lists.php.net
References: <CAFWHjfpriiBX1Xg34F95zXsgwKQeh1-AwActDws5wuRL83pHhQ@mail.gmail.com>
 <CAMrTa2E5VHJGANm0u3Nrs9a9DeoJspE7JAg0902TzDi1Pq4iiA@mail.gmail.com>
 <CAFWHjfr0gFxzqRX12N7u5pWfNkafDeW8Y81d8E==5ibg2_=MiQ@mail.gmail.com>
In-Reply-To: <CAFWHjfr0gFxzqRX12N7u5pWfNkafDeW8Y81d8E==5ibg2_=MiQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Adding the OpenSSF Scorecards GitHub Action
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)

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