The "Token-Permissions" check seems to complain about codeql and labeler #1257
Comments
|
Apparently actually Regarding Author: Evgeny Vereshchagin <evvers@ya.ru>
Date: Sun Nov 14 09:41:42 2021 +0000
ci: tighten codeql and labeler even more
by moving the read permissions to the top level and
granting additional permissions to the specific jobs.
It should help to prevent new jobs that could be added
there eventually from having write access to resources they
most likely would never need.
diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml
index c003cc5179..460002eaeb 100644
--- a/.github/workflows/codeql-analysis.yml
+++ b/.github/workflows/codeql-analysis.yml
@@ -11,6 +11,9 @@ on:
schedule:
- cron: '0 1 * * *'
+permissions:
+ contents: read
+
jobs:
analyze:
name: Analyze
@@ -20,7 +23,6 @@ jobs:
cancel-in-progress: true
permissions:
actions: read
- contents: read
security-events: write
strategy:
diff --git a/.github/workflows/labeler.yml b/.github/workflows/labeler.yml
index 800f8877a3..34d9d63d42 100644
--- a/.github/workflows/labeler.yml
+++ b/.github/workflows/labeler.yml
@@ -9,11 +9,12 @@ on:
permissions:
contents: read
- pull-requests: write
jobs:
triage:
runs-on: ubuntu-latest
+ permissions:
+ pull-requests: write
steps:
- uses: actions/labeler@69da01b8e0929f147b8943611bee75ee4175a49e
with:but I wouldn't expect everybody to change their actions (that most likely were borrowed from the official documentation) to please |
|
Anyway, on an unrelated note, somehow |
|
Thanks for the report and the time you're investing in this. Don't give up, I'll help you out. Just tell me what you expect and I'll tell you why it's happening and we'll try to improve things, whether it's documentation, logging or result computation |
is this related to #1100?
I agree with the reasoning. It's better to have a fail-safe that fail-open solution.
I see your point. @josepalafox @jhutchings1 would you be open to updating the workflow examples and GitHub documentation in general to nudge users towards defining the top-level |
@laurentsimon I'm not sure it is. When I opened the issue the systemd project had at the top level of the labeler action. and didn't even warn that there was an action with write permissions. |
|
The "codeql" part of this issue seems to be the same as #1100 though. Closing. |
so I think it was: the workflow are not job-aware yet. Thanks for confirming.
Got it. It's listed as |
@laurentsimon I agree I apologize for the confusion. I should have opened a separate issue about labeler. |
Good point. We'd like to do that better. Today, we don't Warn if We're hoping to leave this decision to users of scorecard, by providing our results in github.com/ossf/allstar and by providing raw results in the future: we hope raw results will be easier to act upon, e.g. thru a policy language. On a related note: we have a larger plan to cover dangerous workflows in #426 (comment). We added the Please don't hesitate to add ideas to the tracking issue #426 (comment). Suggestions and feedback are greatly appreciated.
no worries. |
I think by analogy with the list of well-know CI services in the CI-Test check new well-known actions could be added on a case-by-case basis. I haven't used any other actions with write permissions apart from
The problem with this approach is that as far as I understand I could theoretically configure
I tried it out with the systemd repository and it failed with "internal error: invalid GitHub workflow" as far as I can see |
Thanks, we can do that. Can you create an issue for it? That should be pretty easy to implement.
what do you mean?
It's more geared towards orgs that have many repos.
in a couple weeks, we'll have a beta version of scorecard -as-GitHub-action. If you're willing to give it a try, it'd be awesome.
We're currently updating all our checks to using github.com/rhysd/actionlint, so I think we'll be able to fix this problem very soon. @asraa @chrismcgehee FYI |
I'll update the dangerous-workflow check with the workflow parser and make sure it passes this test. Thank you so much for pointing out these behaviors. |
I think I need to reread the allstar documentation to be more specific. It could be I misunderstood how it's configured on top of scorecard, which it uses under the hood as far as I understand.
Looking at #1074, it seems the action is supposed to send data to the "code-scanning" tab of repositories (by analogy with codeql I assume). I'm not sure I like this approach because it kind of makes it hard for external contributors who don't have access to that tab to be able to see issues to try to fix them. Basically, they have to fork repositories, wait for the cron job (or whatever else updates that tab) and keep track of those issues by constantly updating their forks. And they can't see issues marked as "false positives" in upstream projects for example so they might send PRs only to discover that they would be rejected. |
|
the action is aimed at repo owners, to help them run scorecard continuously without the pain of installing the tool. We'd like to extend it to be more usable for consumers... but for now we only support the BigQuery table. We have floated around the idea of REST API as well. |
|
@laurentsimon I think issues actions like that report should be fixed eventually and it usually helps when more people have access to various dashboards. I'm not saying it's an issue related to the scorecard action specifically. It's just that that "code scanning" tab hides alerts from everybody apart from repo owners (which probably makes sense considering the nature of the tab) while at the same time all those alerts pop up on forks if they get updated more or less regularly so it isn't clear (to me at least) why that tab is hidden in the first place. |
|
I will double check but I believe when you fork a repo it does keep the action configuration, but it has to be manually triggered, it doesn't run by default when the project is forked. This is in effect the fork-er re-running the scanner rather than the same scan results being replicated. It wouldn't carry over suppressed alerts or the alert history with it either, I don't think. The small difference here is anyone could fork anything and run scorecard or any other SAST tool against it. I don't know if that helps the discussion but figured I'd just toss in a note for clarification. |
And that's probably what motivated contributors would do. But I doubt "drive-by" contributors would want to go that far to just look at alerts they could potentially fix so they just go elsewhere. FWIW judging by #1074 (comment) the scorecard project has already run into something like that |
|
Forgot to mention that also it doesn't seem to be possible to refer to those alerts in the commit messages because unlike, for example, links to OSS-Fuzz issues that are opened to the public eventually, all links to the security tab seem to always point to 404 for everybody except for repo owners. |
I agree public dashboard would be a plus, this is an idea we've been tossing around. GitHub could have an option for devs to opt-in in order to share their dashboard, for example. Like you say, someone can run dependabot on a fork repo and dependenbot for security alerts. Doing this at scale for everyone is a little more challenging :-) |
Thanks for the info @josepalafox ! |
@josepalafox I might be wrong but looking at systemd/systemd#21343 I think that's how Dependabot is supposed to work (in theory at least). GitHub Actions sending data to security scanning tabs are run automatically according to their configs. |
|
I think the workflow you're looking for will be resolved in future updates to the platform and looks to be in beta now I believe: github/roadmap#332 Re:dependabot I think runs differently because it is a vendored action. |
@laurentsimon FWIW I don't think scorecard should recommend using Dependabot until the issue mentioned at systemd/systemd#21343 is fixed. Apparenlty it annoys people maintaining stable forks very much and they can't just remove and recreate those forks. |
|
I've just opened #1336 where I mentioned what Dependabot brings with it. |
|
To be fair though, other than that I think it's better than alternatives. RenovateBot creates PRs like evverx/systemd#30 that are apparently targeted at robots who can figure out what SHAs correspond to by just looking at them |
|
As far as I understand all "corner" cases should be addressed by config files by analogy with the binary-artifacts check: #1256 (comment). Closing this one. |
|
@evverx just be sure: is there still a problem with |
|
Sorry. I wrote that comment semi-automatically. To judge from it should be fixed once #1100 is closed. And when that happens as far as I understand it should be possible to use config files to prevent scorecard from complaining about it.
I have to admit I'm not sure I like the idea of moving everything to config files because I think scorecard should be clever enough to handle most corner cases by default. But as far as I can tell "consumers" are willing to do that and as long as they don't try to push them upstream or ask maintainers to write those configs for them I think it should be fine. Just to clarify, I'm not saying there is anything wrong with config files. It's just that I don't think I'm a "consumer" in that particular sense. |
I actually think this issue has been addressed. The code now parses the GitHub action instead of using a simple regex. @chrismcgehee can we close this issue?
just FYI, we'll be exposing a default policy file, which I think is where wdut? |
I think what I was trying to say is that
It's better than just "raw" results but I'm not sure how those default policies would be shipped. If they aren't included in scorecard out of the box they should be kept somewhere and passed additionally to it and it makes it kind of hard to actually use them. |
This should be doable with the raw results. Users will be able to tweak the default policy to add additional rules without becoming experts in the policy language.
the default policy will be shipped within scorecard and used by default. For example |
I think as long as the default policy is supposed to cover most corner cases it should be fine (and probably it doesn't even matter how it's implemented). It's just that I was initially under the impression that it was targeted at people somewhat specializing in writing those policies and personally I wouldn't want to do that because it would be one more thing to maintain and keep up to date. |
|
FWIW @laurentsimon I think this issue can be closed. I'd go ahead and close it but since it was already reopened once I'm not sure if there is anything else that should be addressed here |
it's just to allow use cases where users want a different policy from the default one. Today, the score is too coarse to allow for fine-grained policy, which is why we want to expose "raw" results. But we wont force users to write these policies, we'll have a default one. |
The problem is that power users like that are usually best equipped to improve default policies. If it's much easier for them to keep their changes "downstream" they usually do just that. I mean, for example, it's hard to deal with selinux policies so people just have to report their issues upstream and the default policy gets better :-)
Good to know. Thanks! |
I agree we want power users to help us make the default policies better. But there are cases when different companies/teams view things differently about what's appropriate or not. Policies will allow them to do what they want.
|
Can't argue with that but given that I've never seen anyone from those companies asking for anything here on GitHub publicly (or even discussing it) I can only speculate (while I'm pretty sure there are specific use cases that those policies should cater to). Anyway, not that it matters, but all things considered I agree that (unlike some decisions that I completely disagree with) those policies are useful. Thanks! |
Both
codeqlandlabelerare widely used GH Actions that require non read-only tokens. The links to the "official" documentation where the permissions they have to have are mentioned:https://github.com/github/codeql-action#usage
https://docs.github.com/en/actions/security-guides/automatic-token-authentication#example-1-passing-the-github_token-as-an-input
The text was updated successfully, but these errors were encountered: