tools: use actions pinned by commit hash on coverage-linux.yml by gabibguti · Pull Request #46294 · nodejs/node · GitHub

As discussed in the Security WG's latest meeting, here is how to use actions by commit hash in GitHub workflows.

Motivation: Using actions by commit hash reference is a remediation for, when actions are compromised or go under a dependency confusion attack, you are not using the malicious version. This remediation along with using least privilege principle for each action in the workflow, makes it harder for a possible action hijacker to have high access to your repository.

Signed-off-by: Gabriela Gutierrez gabigutierrez@google.com

Related to: nodejs/security-wg#851

Review requested:

@nodejs/actions

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

Does it really make sense for Actions provided by GitHub? Since they're the ones who also provide the hardware, we have to trust them at some point, and keeping this updated seems more likely to happen if we're using a tag rather than a commit hash. +1 one for doing it for the codecov action though. wdyt?

I agree with you it's debatable. After all, if GitHub itself gets hijacked we would have bigger problems. About keeping the hash updated it's a good point. Dependabot can take care of updating the hash and the version tag comment, which is more readable. But, yes, keeping the tag version or tag major can be a good option for GitHub actions if you folks prefer.

Dependabot can take care of updating the hash and the version tag comment, which is more readable.

To provide context: dependabot/dependabot-core#4691 (comment)

Shall we resolve the linting warning?
I do this pinning also in my personal projects but have the comment above the actual uses line.

Edit: seems according to the dependabot link like dependabot/dependabot-core#4691 (comment) only this comment format is supported? Not sure if the linting issue will go away without doing any further changes.

Might be a problem with the linting rules of GH.

In my opinion it is good to pin all actions, also those of GitHub. We can not be sure if and who pushes what code. What if someone has bad intentions and joins GitHub just to get malicious code into such solutions?

Also at least I do not know which persons maintain the project, have push and write permissions and if I can trust every single one.

Some PRs also seem to be spam or malicious: https://github.com/actions/checkout/pulls

I doubt that the same persons who have access to the hardware at GH (totally different security restrictions) also work on the actions or have access to them.

Tags can also be changed / pointed to different commits at any time - at least in Git.

I believe I fixed the lint warning but kept the version comment inline, cause, in my understanding, dependabot can only update the version comment inline.

Perfect, thank you very much. This is ok for me as long as dependabot can automatically handle this.

cc: @nodejs/security-wg

This is a positive change, as discussed in the Security WG. I like the comment with the real version so it is easy to read 👍

The changes look fine. Also good, that dependabot finally supports this (wasn't the case when I did GHA pinning in my own projects).

Do we have dependabot enabled for this?

Shall we resolve the linting warning?
I do this pinning also in my personal projects but have the comment above the actual uses line.

Edit: seems according to the dependabot link like dependabot/dependabot-core#4691 (comment) only this comment format is supported? Not sure if the linting issue will go away without doing any further changes.

Might be a problem with the linting rules of GH.

Do we have dependabot enabled for this?

Dependabot version updates is not enabled, we could enable it if we configure it to only check for update for actions (other dependencies are updated by other automations, or manually, but we wouldn't want to use Dependabot for those I don't think).

I would be more comfortable approving this PR if:

the linter wasn't complaining regarding the comment format.
we were making the changes on all GHA YAML files rather than just this one.
we were adding a dependabot.yml file to ensure automation has our back.
we were adding the version comment on other actions pinned by hash in the repo.

I realize this is asking for a lot of work, so I want to clarify that only the linter issue is blocking, the rest is just a proposal, and could happen in follow-up PRs.

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

@aduh95 I fixed the lint warning. I think we can add dependabot in this PR and configure it to update only GitHub Actions. I will leave both GitHub actions and third-party actions hash-pinned. Then, we can see if dependabot and hash-pinning is working for node maintanance and do it to all GHA and other actions hash-pinned. Does that make sense?

LGTM

@gabibguti thanks for the PR it's good to see a concrete example

@aduh95 we had asked for an example to start. I agree that we should enable for all and turn on dependabot for the actions as well.

@gabibguti sounds like you are happy to do the rest and add the dependabot yaml. It might make sense to see what comments we get on this issue for a few more days before doing that larger piece of work.

@gabibguti I'm assuming we're going to have PRs for all actions, so it's better to provide the context in the title.

@RafaelGSS Perfect! Totally agree and thanks for updating this PR title!

Hey everyone, I'm taking some time because I'm looking into dependabot, but I ended up with the following two questions:

Do you have a preference between dependabot or renovatebot?
Do you want to bump your actions whenever there's a new version or only upgrade in case there's a security update?

Do you want to bump your actions whenever there's a new version or only upgrade in case there's a security update?

I think we could stick to security updates only. AFAIK nowadays the update is performed manually, so it won't change anything in our workflow. But, adding a workflow to update the actions once per month (regardless of security updates) would be great too.

Wdyt @nodejs/tsc ?

AFAIK nowadays the update is performed manually

That's not quite correct for actions from GitHub (e.g. actions/checkout) which are using the latest semver-minor. This PR is pinning them to a hash, so it would mean they are no longer automatically using the latest anymore. Not sure if it should be a concern, but wanted to point it out.
I agree we should definitely have automation for security releases, I don't have an opinion regarding whether it should be more frequent than that.

I think we could stick to security updates only. AFAIK nowadays the update is performed manually, so it won't change anything in our workflow

My thought is that sticking to security updates only is a good way to start.

Do you have a preference between dependabot or renovatebot?

We already use dependabot in some of the project repos so sticking to that makes sense to me.

As an existing example: https://github.com/nodejs/TSC/blob/main/.github/dependabot.yml

Okay! For only security updates, we don't need dependabot.yml, we just need to enable Dependabot in the repository settings. We can do this by enabling "Dependabot security updates", which will also enable "Dependabot alerts".

https://docs.github.com/en/code-security/dependabot/dependabot-security-updates/configuring-dependabot-security-updates

If we do that, we don't need anything else in this PR.

However, I could not fully understand if Dependabot works properly identifying actions vulnerabilities by just enabling the security updates check. That said, I think it's better to enable the version updates by adding dependabot.yml, like we did here https://github.com/nodejs/TSC/blob/main/.github/dependabot.yml and with PR limits. What do you think?

That said, I think it's better to enable the version updates by adding dependabot.yml, like we did here https://github.com/nodejs/TSC/blob/main/.github/dependabot.yml and with PR limits.

My take is that we can try enabling the version updates by adding dependabot.yml

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

fine with this, but do agree it's probably unnecessary for GitHub-provided Actions.

@gabibguti is this ready to go from your perspective now? We are currently in lockdown due to the security release but after that if this is ready I'll take a look at landing it.

@gabibguti is this ready to go from your perspective now? We are currently in lockdown due to the security release but after that if this is ready I'll take a look at landing it.

Yeah! Just need to figure out the lint error to fix.

@gabibguti made some suggestions which I think will resolve the linter issues if you want to review/accept them.

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

That said, I think it's better to enable the version updates by adding dependabot.yml, like we did here https://github.com/nodejs/TSC/blob/main/.github/dependabot.yml and with PR limits.

My take is that we can try enabling the version updates by adding dependabot.yml

@mhdawson We did it?

@RafaelGSS We did add dependabot.yml for version updates of GHAs in this PR. The interval is monthly and limit of 10 PRs.

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com> PR-URL: #46294 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Tierney Cyren <hello@bnb.im>

Landed in ff89399

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com> PR-URL: #46294 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Tierney Cyren <hello@bnb.im>

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jan 20, 2023

tools: use actions pinned by commit hash in coverage-linux

cbd8c8a

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

gabibguti force-pushed the hash-pinned-actions branch from 6996e1c to cbd8c8a Compare January 20, 2023 16:42

gabibguti changed the title Use actions pinned by commit hash tools: use actions pinned by commit hash Jan 20, 2023

aduh95 reviewed Jan 20, 2023

View reviewed changes

RafaelGSS self-requested a review January 21, 2023 04:02

UlisesGascon approved these changes Jan 22, 2023

View reviewed changes

DanielRuf approved these changes Jan 22, 2023

View reviewed changes

tools: Double space inline comments

f023b10

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

mhdawson approved these changes Jan 23, 2023

View reviewed changes

RafaelGSS approved these changes Jan 24, 2023

View reviewed changes

richardlau approved these changes Jan 24, 2023

View reviewed changes

RafaelGSS changed the title tools: use actions pinned by commit hash tools: use actions pinned by commit hash on coverage-linux.yml Jan 24, 2023

RafaelGSS mentioned this pull request Jan 24, 2023

Add OSSF Scorecard nodejs/security-wg#851

Closed

fraxken approved these changes Jan 24, 2023

View reviewed changes

tools: Add dependabot for GHA version updates

b8bfce1

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

tools: Fix lint double quote error

0194f28

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

bnb approved these changes Feb 8, 2023

View reviewed changes

mhdawson reviewed Feb 16, 2023

View reviewed changes

Comment thread .github/dependabot.yml Outdated

mhdawson reviewed Feb 16, 2023

View reviewed changes

Comment thread .github/dependabot.yml Outdated

mhdawson reviewed Feb 16, 2023

View reviewed changes

Comment thread .github/dependabot.yml Outdated

tools: fix linter issues

2bda6e1

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>

gabibguti force-pushed the hash-pinned-actions branch from 9992139 to 2bda6e1 Compare February 16, 2023 18:12

RafaelGSS added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Feb 17, 2023

RafaelGSS approved these changes Feb 17, 2023

View reviewed changes

mhdawson closed this Feb 21, 2023

RafaelGSS mentioned this pull request Feb 24, 2023

feat: ossf scorecard reporting improvements nodejs/security-wg#886

Merged

gabibguti deleted the hash-pinned-actions branch February 24, 2023 15:20

gabibguti mentioned this pull request Feb 24, 2023

tools: use actions pinned by commit hash on all workflows #46820

Merged

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

                   runs-on: ubuntu-latest
                   steps:
-                    - uses: actions/checkout@v3
+                    - uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

                   runs-on: ubuntu-latest
                   steps:
-                    - uses: actions/checkout@v3
+                    - uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

tools: use actions pinned by commit hash on coverage-linux.yml by gabibguti · Pull Request #46294 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: use actions pinned by commit hash on coverage-linux.yml - #46294

tools: use actions pinned by commit hash on coverage-linux.yml#46294gabibguti wants to merge 5 commits intonodejs:mainnodejs/node:mainfrom gabibguti:hash-pinned-actionsgabibguti/node:hash-pinned-actionsCopy head branch name to clipboard

Conversation

gabibguti commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 20, 2023

Uh oh!

aduh95 Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

gabibguti Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

panva Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

DanielRuf Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRuf Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabibguti Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

DanielRuf Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

RafaelGSS commented Jan 21, 2023

Uh oh!

UlisesGascon left a comment

Choose a reason for hiding this comment

Uh oh!

DanielRuf left a comment

Choose a reason for hiding this comment

Uh oh!

DanielRuf Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Jan 22, 2023

Uh oh!

gabibguti commented Jan 23, 2023

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 23, 2023

Uh oh!

RafaelGSS commented Jan 24, 2023

Uh oh!

gabibguti commented Jan 24, 2023

Uh oh!

gabibguti commented Jan 24, 2023

Uh oh!

RafaelGSS commented Jan 24, 2023

Uh oh!

aduh95 commented Jan 24, 2023

Uh oh!

mhdawson commented Jan 25, 2023

Uh oh!

mhdawson commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabibguti commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Jan 31, 2023

Uh oh!

bnb left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Feb 9, 2023

Uh oh!

tools: use actions pinned by commit hash on coverage-linux.yml#46294
gabibguti wants to merge 5 commits into
nodejs:mainfrom
gabibguti:hash-pinned-actions

gabibguti commented Jan 20, 2023 •
edited
Loading

DanielRuf Jan 22, 2023 •
edited
Loading

DanielRuf Jan 22, 2023 •
edited
Loading

DanielRuf Jan 22, 2023 •
edited
Loading

mhdawson commented Jan 25, 2023 •
edited
Loading

gabibguti commented Jan 30, 2023 •
edited
Loading

gabibguti commented Feb 17, 2023 •
edited
Loading