Skip to content
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

[StepSecurity] ci: Harden GitHub Actions #1426

Merged

Conversation

step-security-bot
Copy link
Contributor

Summary

This is an automated pull request generated by Secure Workflows at the request of @jauderho. Please merge the Pull Request to incorporate the requested changes. Please tag @jauderho on your message if you have any questions related to the PR. You can also engage with the StepSecurity team by tagging @step-security-bot.

Security Fixes

Pinned Dependencies

A pinned dependency is a dependency that is explicitly set to a specific hashed version instead of a mutable version. Pinned dependencis ensure that development and deployment are done with the same software versions which reduces deployment risks, and enables reproducibility. It can help mitigate compromised dependencies from undermining the security of the project in certain scenarios. The dependencies were pinned using Secure WorkFlows

Feedback

For bug reports, feature requests, and general feedback; please create an issue in step-security/secure-workflows or contact us via our website.

Signed-off-by: StepSecurity Bot bot@stepsecurity.io

Signed-off-by: StepSecurity Bot <bot@stepsecurity.io>
@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Base: 64.98% // Head: 64.98% // No change to project coverage 👍

Coverage data is based on head (a2e85b5) compared to base (fc401da).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1426   +/-   ##
=======================================
  Coverage   64.98%   64.98%           
=======================================
  Files          23       23           
  Lines        2319     2319           
=======================================
  Hits         1507     1507           
  Misses        714      714           
  Partials       98       98           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@piksel
Copy link
Member

piksel commented Oct 14, 2022

The one failing check highlights the problem with this. It's currently broken and the developers are working on a fix. So, pinning it to the broken version is probably not desired at this point.

It also seems difficult to update the actions to a new version.

That being said, if we would get hit by a malicious action version it could cause a lot of damage to users using watchtower (due to it's self-updating nature). It would be have to be a very targeted attack, but it's still a bit concerning.

Perhaps we should at least make sure it's done on the release workflow?
@simskij thoughts?

@zoispag
Copy link
Member

zoispag commented Oct 14, 2022

I think it's very difficult to debug an action's failure with the hashed pinned versions. You would need to match the hash with a tag, looking at the commits history to read the README and verify for example if any input was changed. Personally, I am not in favor.

I favor pinning (when possible) to a patch version, and when dependabot finds an update, review the changes manually and carefully before merging.

@piksel
Copy link
Member

piksel commented Oct 14, 2022

@zoispag Well, this is for the actions themselves, which always run on the "latest" tagged version, so there will never be any dependabot updates (except for a new @vX tag is released). If there were a version with malicious intent, it would probably not bother to bump a new tag version.

But yeah, I agree that it's really annoying that there is no easy way of telling what "version" the hash refers to.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHubs own actions are fully trusted, let's keep the paranoia on a reasonable level. This includes codecov too.
All other third party actions are pinned, but with a comment about what version they were tagged with before pinning.

.github/workflows/codeql-analysis.yml Outdated Show resolved Hide resolved
.github/workflows/codeql-analysis.yml Outdated Show resolved Hide resolved
.github/workflows/codeql-analysis.yml Outdated Show resolved Hide resolved
.github/workflows/codeql-analysis.yml Outdated Show resolved Hide resolved
.github/workflows/greetings.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
GitHubs own actions are fully trusted, let's keep the paranoia on a reasonable level. This includes codecov too.
All other third party actions are pinned, but with a comment about what version they were tagged with before pinning.
@@ -72,18 +72,18 @@ jobs:
with:
go-version: 1.18.x
- name: Login to Docker Hub
uses: docker/login-action@v2
uses: docker/login-action@f4ef78c080cd8ba55a85445d5b36e214a81df20a #v2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piksel I would also trust docker tbh. But this is ok too. At least it's not that madness from before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, both goreleaser and docker should be trusted as far as I am concerned, but since they are in the security hotspot (getting secrets and publishing) I felt that it was better to not include them in the "madness purge" 😁

@jauderho
Copy link
Contributor

There's an open issue to have Dependabot update the version in the comment every time the hash is updated. See dependabot/dependabot-core#4691

@piksel piksel merged commit 9a2f9c4 into containrrr:main Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants