-
Notifications
You must be signed in to change notification settings - Fork 474
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
Feature: Allow unpinned in non-privileged workflows #2018
Comments
/cc @raghavkaul |
They do but it has nothing to do with security. systemd pins its dependencies to make it easier to maintain the CI. For example super-linter broke at some point https://github.com/github/super-linter/issues/2128 and when Dependabot tried to update the action it failed and the last working version was successfully used until the bug was fixed. If it hadn't been pinned the part of the CI relying on super-linter would have been just broken. Having said that I agree that unpinned read-only actions shouldn't be flagged by analyzers focusing primarily on security and it seems to me that promotional PRs like the one discussed at ossf/scorecard-action#1017 triggered semi-automatically by overzealos "security" tools fixing imaginary "security" issues can be eliminated by addressing this issue. Can it be bumped somehow? |
I think we'll try to tackle this in Q1. It's on our radar. |
This comment was marked as outdated.
This comment was marked as outdated.
@diogoteles08 is working on it afaik |
This comment was marked as outdated.
This comment was marked as outdated.
Hi there! Yes, I'll be working on this issue. Expected to end of Q1. |
I would like to emphasize that even for read-only actions, a gone-malicious action could:
So if the scorecard de-penalizes read-only actions, it closes their eyes to all of these scenarios. Is that a good idea? Am I missing something in this picture? PS: https://github.com/step-security/harden-runner tries to go the exact opposite way, in case someone is interested. |
I wonder how it can do this if it doesn't have
They can leak secrets but I don't think pinning stuff to hashes can help much here given that under the hood a lot of actions use a bunch of unpinned modules nobody has ever reviewed. PRs where Dependabot updates stuff aren't reviewed either.
They can do that too and pinning stuff doesn't help here either.
I took a look at it systemd/systemd#25205 (comment) so personally I'm not interested in that. All in all I think instead of that security theater scorecard should ideally actually audit/scan actions. It was discussed in #3022 (comment) and #2991 (comment). |
@evverx pinning makes sure that the same action code will be run every time to allow a review-once-run-multiple-times kind of workflow. That's the problem pinning solves — it makes changes visible and controlled — nothing more. That should answer 99% of your question. By build artifact I meant files uploaded via action actions/upload-artifact, which (unlike PyPI) does not require any additional secrets. |
I didn't ask any question regarding pinning stuff. I know what it is and what it's supposed to accomplish.
It's a well-known GitHub bug: google/clusterfuzzlite#84 (comment). It should be fixed by GitHub. |
@evverx if the the use of pinning is clear, what exactly is your motive and role in this discussion then?
That's arguable, but an interesting point. |
I'm not sure why stuff is arguable when GitHub bugs and shortcomings come up :-) |
@evverx either you acknowledge the security aspect of pinning for even read-only actions or you don't. If you do, to keep penalizing is in your interest, which is what I was arguing for. Excellent. |
I can acknowledge that but it doesn't mean that false positives shouldn't be fixed. And this issue is about false positives.
It isn't. I'm not interested in receiving bogus promotional "security" PRs fixing imaginary "security" issues. This issue is supposed to address false positives. |
@evverx I see. I'm not sure I mind false positives in this context, I support more hardening. Your point is clear then, thanks. |
FWIW I do too. The problem is that even though I would love to pin CIFuzz and ClusterFuzzLite they can't be pinned. Another way to address this would be #1907 but that would waste maintainer's time, wouldn't be visible outside of repositories and wouldn't prevent PRs trying to fix them. Anyway the reason I'm annoyed is that because thanks to all that automation I somehow end up receiving bogus PRs, bogus CVEs generated automatically based on OSV (with inaccurate commit ranges) and so on and all that can be avoided by addressing false positives. |
Hi! @evverx I understand your complains against False Positives and annoying notification on GitHub and we are taking them seriously! As you said, this current issue intends to reduce False Positives. About the idea of enhancing Scorecards have some kind of feature to scan/audit actions, I also think it's a good idea and it's in my radar, but it's still a bit far from Scorecard current approach and might take longer to became feasible. By now I believe it's more reasonable to focus on short-term fixes to avoid False Positives and enhance the UX of Scorecard for Maintainers. |
Thanks for the input, @hartwork. To answer your main complains, for this issue we are already considering that unpinned actions would be allowed (i.e. not cause a score decrease at the Pinned-Dependencies check) only if the action:
But I also believe that hash-pinning actions could bring security benefits even when used with read-only permissions. I tried to compile my concerns as follows:
Given those points, we could debate if we want to keep a light score reduction even for the "allowed cases" of unpinned actions discussed in this issue. Maybe something like a 0.1 score reduction for each occurrence? With this setup, a repository that prefers to keep "safe actions" unpinned won't have a 10/10, but should still get something around 9/10, which is already a pretty great score. Would like to hear @maintainers input on this. @laurentsimon @raghavkaul |
I really liked this idea of "punishing but not much" because it would also differ the case of a "safe workflow unpinned" from a "potentially dangerous workflow unpinned". Good idea @diogoteles08 ! |
I agree that it's much more important. The CLOMonitor folks and the dashboards they built is the only reason why I didn't threw out the scorecard action. (though it's still weird to me that it was somehow totally acceptable to just dump json)
I completely forgot that at least in the systemd repository CIFuzz is no longer read-only: systemd/systemd#27501 so I guess at least there it's too late :-) My opinion on the security tab and the ability to read it can be found at google/oss-fuzz#10090 (comment). That same argument is more or less applicable to the 90-day OSS-Fuzz disclosure but I guess it somehow makes people feel secure for some reason. Anyway since I'm subscribed to some OSS-Fuzz issues it seems to me that |
Am I interpreting your scenario right?
|
@spencerschrock what I had in mind was more like:
|
Oh, I'm a big proponent for publishing via CI over laptops. Couldn't agree more! I just misunderstood you before, thought you were talking about using |
This comment was marked as off-topic.
This comment was marked as off-topic.
So, we agreeing that the @upload-artifacts is not completely safe, even though it can be used with read-only permissions, right? Getting back to the original discussion of the issue, one straightforward idea is that we could add this as another requirement to have an "allowed unpinned action". So the unpinned actions would be allowed (i.e. not cause a score decrease at the Pinned-Dependencies check) only if the action:
WDYT? |
@diogoteles08 I think when starting to look for use of https://github.com/actions/upload-artifact in particular, the question would be how special that action really is. If I use a fork of that action and the action works and does something similar, it would evade detection and still be affected, I suppose? (Maybe I'm biased, I think pinning everything is the best way to go. I seem to mind less than others about the pull request and commit noise that can go along with that.) |
@diogoteles08 it seems to me that it would be better to assume that all the actions (regardless of whether they are read-only or not) can be used to produce important things (regardless of whether it's a good idea or not) so I'm inclined to say that scorecard should probably ask projects to pin stuff there too. I think false positives can be suppresed using something like #1907. |
Yes that's sad but true |
Best we could do is to penalise less the "allowed actions" I specify here, but not the 0.1 decrease I was talking about, something bigger than that, but still smaller than a reduction for an unpinned action with secrets or dangerous permissions. |
@diogoteles08 dunno. I think it would just complicate things and I don't think it's worth it. Maybe it would be better if overzealous "security" tools/scanners/analyzers using scorecard were smart enough to deal with that. I'm not sure what else I can add there so I'm going to focus on that SBOM nonsense. Luckily it hasn't been integrated yet :-) (Don't get me wrong I think SBOMs make sense but "let's push them upstream without any clear rationale and use cases and penalize projects if they don't comply" nonsense is what concerns me). |
@hartwork I missed that comment. Just to clarify I didn't mean to say that there is anything inherently wrong with that action. It does what it's supposed to do. Your use case is safe because somehow I'm pretty sure you can't be tricked into downloading bogus artifacts when you do it manually. Even those nightly builds can be distributed via artifacts properly if they are signed with something random PRs don't have access to (it isn't clear where consumers would verify those signatures :-) but that's another matter). What I was trying to say is that it's a bit harder to protect artifacts because it's easier to mess with them than with actual releases. Then again, the GitHub setting preventing CI from running on PRs submitted by first-time contributors kind of mitigates that too (and that explains why when it was introduced a lot of weird accounts started fixing typos in upstream projects for no apparent reason :-)). |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This was brought up in the sync yesterday, and was centered around the optics of false positives. Namely, that if scorecard can't detect if workflow artifacts are being used in a dangerous way, then it shouldn't. This of course is a consumer preference, and some people/use-cases have different tolerances for false positives. The general consensus was to avoid judging possible misuse of workflow artifacts (although perhaps throwing a log statement). This would keep determination of non-privileged workflow to:
|
Unfortunately scorecard can't detect that reliably because the "read-only" GitHub setting is out of reach. It can still trigger unwanted PRs like step-security/secure-repo#462 (comment) (Thanks to the link @diogoteles08 posted now I know that that stuff is prompted by bug bounties: https://crowdfunding.lfx.linuxfoundation.org/initiative/049febeb-075e-4cbc-8d98-c5ef62483388/financial. It would be nice by the way if PRs indirectly sponsored by Google/OpenSSF disclosed that).
If secrets are used to keep, say, "junk" tokens used to push stuff to third-party CI services I'm not sure it should affect repositories. For example back in the day there was a fuzzing service that used tokens to authenticate jobs sent from GitHub. It was known that those tokens could be dumped by any PRs, could be abused to overflow the queue there and so on but that service was willing to accept the risk. I'm not sure how often tokens like that are used but my guess is that the idea is to keep track of secrets used to publish stuff consumed by people and I'm not sure it can be detected automatically. |
This issue is stale because it has been open for 60 days with no activity. |
I think I've found an example of this: IIUC, every time a new release is made this workflow is responsible to build and use |
This issue is stale because it has been open for 60 days with no activity. |
fwiw actions/upload-artifact@v4 (an API break from v3) generates sealed immutable artifacts -- they can't be changed w/o something that has permission first deleting the artifact. |
This issue has been marked stale because it has been open for 60 days with no activity. |
Workflows that have no secret and all their permissions set to read/none don't benefit from being pinned, and add burden for users to keep them up to date. We may want to relax the Token-Permission check, making this a "bonus" point rather than flagging it as an problem
The text was updated successfully, but these errors were encountered: