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

(Feedback) Unclear value proposition for Action #1017

Open
joycebrum opened this issue Nov 21, 2022 · 11 comments
Open

(Feedback) Unclear value proposition for Action #1017

joycebrum opened this issue Nov 21, 2022 · 11 comments

Comments

@joycebrum
Copy link
Contributor

In some discussions, maintainers questioned what are the real benefit of having scorecard running as an action with arguments such as:

PHP

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.

Curl

We already know we use code
analyzers, tests, fuzzing and have a security policy. I don't think we need a
tool to tell us this. I don't think it helps our security.

How can we improve or show the value of scorecard as an action?

@naveensrinivasan
Copy link
Member

Scorecards add new checks which based on findings and trends. If the team is on top of the security updates and would find scorecard as redundant then they have a choice to make.

Convincing everyone is hard.

@evverx
Copy link

evverx commented Nov 22, 2022

On a somewhat related note I wonder why scorecard keeps promoting questionable services: systemd/systemd#25205 (comment)? Generally I don't care about what scorecard promotes but projects showing badges happen to promote that sort of stuff transitively as well. I wonder if links to that can be removed from https://api.securityscorecards.dev/projects/github.com/systemd/systemd and the security tab? Or should I just revert scorecard to fix that?

@evverx
Copy link

evverx commented Nov 22, 2022

Looking at the action a bit closer it appears it still can't handle PRs so issues like systemd/systemd#23693 (comment) can be caught only after PRs are merged (which isn't as useful as it can be). It's probably worth mentioning that in that particular case scorecard could probably have helped to catch the unpinned action and the missing permissions but it would have missed the typo (because typos and typosquatting in general isn't covered at all) and wouldn't have helped to audit the external actions in any way. Other than that #176 has never been addressed.

I have to admit I have no idea how the action in its current form ended up in the systemd repository. (I was on vacation and probably missed PRs where it was introduced). I'll take a closer look.

Anyway I really hope that links to bogus bolt-on "security" stuff can be removed from https://api.securityscorecards.dev/projects/github.com/systemd/systemd and the security tab.

@evverx
Copy link

evverx commented Nov 23, 2022

It appears those links are all over the place partly because due to I assume some scorecard bug the "Token-Permissions" check is somehow special and scorecard insists on clicking those links even when it doesn't make sense:

    {
      "name": "Token-Permissions",
      "score": 9,
      "reason": "non read-only tokens detected in GitHub workflows",
      "details": [
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/build_test.yml:16: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/build_test.yml/main?enable=permissions",
        "Info: topLevel permissions set to 'read-all': .github/workflows/cflite_pr.yml:12: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/cflite_pr.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/cifuzz.yml:9: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/cifuzz.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/codeql.yml:24: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/codeql.yml/main?enable=permissions",
        "Info: jobLevel 'actions' permission set to 'read': .github/workflows/codeql.yml:34: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/codeql.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/coverity.yml:13: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/coverity.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/development_freeze.yml:11: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/development_freeze.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/differential-shellcheck.yml:11: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/differential-shellcheck.yml/main?enable=permissions",
        "Warn: jobLevel 'security-events' permission set to 'write': .github/workflows/differential-shellcheck.yml:19: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/differential-shellcheck.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/issue_labeler.yml:9: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/issue_labeler.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/labeler.yml:11: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/labeler.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/linter.yml:14: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/linter.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/mkosi.yml:45: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/mkosi.yml/main?enable=permissions",
        "Info: topLevel permissions set to 'read-all': .github/workflows/scorecards.yml:20: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/scorecards.yml/main?enable=permissions",
        "Info: topLevel 'contents' permission set to 'read': .github/workflows/unit_tests.yml:13: update your workflow using https://app.stepsecurity.io/secureworkflow/systemd/systemd/unit_tests.yml/main?enable=permissions"
      ],
      "documentation": {
        "short": "Determines if the project's workflows follow the principle of least privilege.",
        "url": "https://github.com/ossf/scorecard/blob/c40859202d739b31fd060ac5b30d17326cd74275/docs/checks.md#token-permissions"
      }
    },

I'm not sure why what should actually be debug messages pop up as info messages and why exactly top-level read permissions should be remediated by updating all the workflows. The "warn" shouldn't be there either (and as far as I understand that's what ossf/scorecard#2338 was supposed to address).

The remaining links come from the "Pinned-Dependencies" check and flag CIFuzz and ClusterFuzzLite. They shouldn't be flagged either.

As far as I can tell if the underlying scorecard bugs were fixed those links wouldn't even pop up anywhere.

@joycebrum
Copy link
Contributor Author

Hi @evverx thanks for reaching out, I really appreciate you sharing your concerns on the tool once you are now using it, it is a valuable feedback!

About the link you mentioned, you mean the ones in the suggestions bellow?

Just to clarify we are not suggesting (in this suggestions) or recommending the Harden Runner, the link mentioned is only about the online tool provided by the step security which you can use to automatically pin all dependencies (Pin-Dependencies Check) and also set the content permission to read only by default (Token Permission Check).

That's why the flags marked should be the first (enable=permissions) and third ones (enable=pin), probably all of them have been flagged and since the Harden Runner is a Step Security tool, it was also added to the systemd/systemd#25205 (comment) PR, but it is not related to any scorecard check.

image

About the Scorecard running on PRs, it currently runs on PRs but we have recently created a issue to really make the run on PR useful to maintainers #1019.

About the token permissions, it seems that you are right, it doesn't make sense this json output "links" in the Token-Permission check, since there is nothing there to fix. I'll open an issue to look it closely and see if I am able to try to fix it myself to help scorecard team.

About the Pinned-Dependencies, good to see that scorecard team is already aware that it is not really a security issue not to pin the action that has read only permissions (ossf/scorecard#2018) but this would not interfere with this links mentioned in the Token-Permissions.

At least the Infos in the Pinned Dependencies details seems right

Info: GitHub-owned GitHubActions are pinned Info: no insecure (not pinned by hash) dependency downloads   
found in Dockerfiles 

Info: no insecure (not pinned by hash) dependency downloads found in shell scripts

@evverx
Copy link

evverx commented Nov 23, 2022

Just to clarify we are not suggesting (in this suggestions) or recommending the Harden Runner

I agree that scorecard doesn't recommend it directly but the links provided by scorecard lead to a page where it can be easily enabled and included into those "security" PRs (and well-meaning contributors are likely to bring it because why wouldn't they? On paper it looks like it actually makes sense plus it's somewhat indirectly endorsed by OpenSSF through scorecard). Of course that kind of "sandbox" is totally bogus but nobody is going to look at how it works under the hood because they would probably just trust scorecard.

FWIW those semi-automated PRs can actually be considered harmful in the case of CFLite: ossf/scorecard#1907 (comment)

About the Scorecard running on PRs, it currently runs on PRs but we have recently created a issue to really make the run on PR useful to maintainers #1019.

Good to know. Thanks!

About the token permissions, it seems that you are right, it doesn't make sense this json output "links" in the Token-Permission check, since there is nothing there to fix. I'll open an issue to look it closely and see if I am able to try to fix it myself to help scorecard team.

Thanks!

good to see that scorecard team is already aware that it is not really a security issue not to pin the action that has read only permissions (ossf/scorecard#2018)

I think the scorecard team has been aware of that issue (and a bunch of other issues) for a long time. The problem is that they have never been addressed for various reasons.

At least the Infos in the Pinned Dependencies details seems right

Looks like it.

@varunsh-coder
Copy link

Just to clarify we are not suggesting (in this suggestions) or recommending the Harden Runner

I agree that scorecard doesn't recommend it directly but the links provided by scorecard lead to a page where it can be easily enabled and included into those "security" PRs (and well-meaning contributors are likely to bring it because why wouldn't they? On paper it looks like it actually makes sense plus it's somewhat indirectly endorsed by OpenSSF through scorecard). Of course that kind of "sandbox" is totally bogus but nobody is going to look at how it works under the hood because they would probably just trust scorecard.

@evverx, please feel free to create an issue or discussion at https://github.com/step-security/harden-runner if you think there are areas of improvement. I also replied to your comment on systemd/systemd#25205 (comment). The whole point of having it be open source is that developers look at how it works under the hood and help improve it.

@evverx
Copy link

evverx commented Nov 24, 2022

The whole point of having it be open source is that developers look at how it works under the hood and help improve it.

From systemd/systemd#25205 (comment)

Can you please clarify what you mean by “shady” in technical terms? W.r.t switch licenses on the fly, for secure-workflows it was set to AGPL in Feb 2022 and we don’t intend to change it.

It was set to AGPL after I pointed out that it was relisenced on the fly as soon as it was integrated into the scorecard documentation masquerading as an open-source project with the Apache license. I doubt it would be AGPL now if that move hadn't been noticed.

Regarding the harden runner I appreciate the details but I'm not here to audit or help to improve it. I think if anyone is interested they can take a look at the code and decide for themselves how hard it is to escape from that "sandbox" (or whether they need to escape from it at all).

@varunsh-coder
Copy link

The whole point of having it be open source is that developers look at how it works under the hood and help improve it.

From systemd/systemd#25205 (comment)

Can you please clarify what you mean by “shady” in technical terms? W.r.t switch licenses on the fly, for secure-workflows it was set to AGPL in Feb 2022 and we don’t intend to change it.

It was set to AGPL after I pointed out that it was relisenced on the fly as soon as it was integrated into the scorecard documentation masquerading as an open-source project with the Apache license. I doubt it would be AGPL now if that move hadn't been noticed.

I already explained earlier that I was new to open-source licenses. I looked at a few projects and decided to use the license that dependabot uses. After getting feedback, I apologized, and promptly updated the license. As I mentioned before, we haven’t changed the project license since then and don’t intend to do so in the future.

Regarding the harden runner I appreciate the details but I'm not here to audit or help to improve it. I think if anyone is interested they can take a look at the code and decide for themselves how hard it is to escape from that "sandbox" (or whether they need to escape from it at all).

That is fair. Request you to please not refer to it as bogus without a thorough audit or review. We spend a significant amount of effort building & maintaining these open-source solutions and there are many open-source contributors who provide constructive feedback. Reading a comment that the project is bogus without proper reasoning or discussion is disheartening.

@evverx
Copy link

evverx commented Nov 24, 2022

Request you to please not refer to it as bogus without a thorough audit or review

Is this some sort of cease and desist? Should I also remove the comments where I said that those semi-automated PRs fix imaginary "security" issues and in the case of CFLite they are actually malicious?

@gabibguti
Copy link

Updating a case of unclear value proposition of the tool. Scorecard is not able to identify Cargo dependencies in Pinned-Dependencies check, it is disagreeable if some dependencies should be pinned or not in a library case, and Clippy tool is not detected in SAST check. This is seen as an "incorrect" analysis by maintainers and leads to a lack of trust of wheather the security recommendations are worth or not. Additionally, the securityscorecards.dev/viewer does not provide further information on why an unpinned dependency is dangerous, or why a SAST tool is important. The page does provide links to the documentation, but without entering the documentation and studying about each check to validate if the recommendation is valid or not, it is hard to glance at the page and understand the value of the recommendations.

rustwasm/wasm-bindgen#3670

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

No branches or pull requests

5 participants