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

Add Scorecard #3670

Closed
wants to merge 3 commits into from
Closed

Add Scorecard #3670

wants to merge 3 commits into from

Conversation

gabibguti
Copy link

Resolves #3090

This PR adds Scorecard security tool. Thanks for trying the tool out! And let me know if you have any feedback.


steps:
- name: "Checkout code"
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why we aren't just using the published versions?

Copy link
Author

@gabibguti gabibguti Oct 26, 2023

Choose a reason for hiding this comment

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

One of the security best practices Scorecard recommends is using commit SHAs to reference dependency versions instead of, in this case, branches like main or tags like v4. The reason is that branches and tags get automatically the latest updates and with that it can get a malicious commit without notice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can see OSFF doesn't even support Cargo projects, is that correct?
In any case, seeing that this is a library pinning dependencies is imo a terrible idea.

I feel similarly about GitHub Actions dependencies, they should definitely not be pinned in a public project, where zero-day exploits can be used against us by anybody who can trigger the CI workflow, which is more or less everybody.

Copy link
Author

Choose a reason for hiding this comment

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

As far as I can see OSFF doesn't even support Cargo projects, is that correct?

Yes, true. The Scorecard Pinned-Dependencies checks does not identify Cargo dependencies. I will report that for Scorcard and suggest the documentation is improved to inform which languages/tools are supported.

In any case, seeing that this is a library pinning dependencies is imo a terrible idea.

I partially agree. I think there are pros and cons. My current view is that pinning build and release dependencies is still "safer" for these processes. But pinning the library dependencies and test dependencies is not something I agree with too. So, pinning GitHub actions dependencies in this workflow would still make sense for me but I understand it comes with a maintainance cost.

@daxpedda
Copy link
Collaborator

Can we also add the badge here or can we do this only as a follow-up?

gabibguti and others added 2 commits October 26, 2023 10:22
Limit was set to 5 days, now it is set to the default of 90 days limit to view the GitHub action artifacts after the action was ran.

Co-authored-by: daxpedda <daxpedda@gmail.com>
Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
@daxpedda
Copy link
Collaborator

I'm looking at the results here: https://securityscorecards.dev/viewer/?uri=github.com/rustwasm/wasm-bindgen.

Is there some way to add exceptions to binary artifacts? Currently it includes stuff from examples and benchmarks.
SAST is also an issue, we do use Clippy, but as far as I can see OSFF doesn't support detecting that.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM to me, would only like to see the GitHub Actions dependencies unpinned, this is a security practice I don't really agree is appropriate for wasm-bindgen, I elaborated on this above.

OSFF doesn't really support Cargo project, so I'm starting to be a bit skeptical of it's usefulness here. Overall, I'm still fine with this.

@alexcrichton I feel like you should give your stamp of approval here as well.

@alexcrichton
Copy link
Contributor

I'd personally be a bit wary of this given the amount of configuration required in this repository in addition to some of the results. Some of the aspects on the security card I don't understand (e.g. I haven't heard of SAST before) and some aren't really that applicable (fuzzing for this repo, pinned dependencies, wasms-interpreted-as-binary, signed releases, etc).

All that being said I'm not too too involved in wasm-bindgen any more so I don't think I should count as a "final say" of sorts, I think it's fine to add this if you'd like to @daxpedda

@daxpedda
Copy link
Collaborator

@hamza1311 @Liamolucko, you guys have any opinion on that?
I'm currently not against it but not for it as well, pretty much for the same reasons Alex Crichton pointed out: the result doesn't seem very representative to me for wasm-bindgen.

So unless there is any further approval I'm gonna close this.

@gabibguti
Copy link
Author

I'm looking at the results here: https://securityscorecards.dev/viewer/?uri=github.com/rustwasm/wasm-bindgen.

Is there some way to add exceptions to binary artifacts? Currently it includes stuff from examples and benchmarks. SAST is also an issue, we do use Clippy, but as far as I can see OSFF doesn't support detecting that.

No, there's currently no way of adding exceptions, but Scorecard plans on working on such a feature. And, yes, Scorecard does not support Clippy as far as I know, I will report that too.

@gabibguti
Copy link
Author

In any case, thanks for the valuable feedback!

@ranile
Copy link
Collaborator

ranile commented Oct 27, 2023

The Scorecard Pinned-Dependencies checks does not identify Cargo dependencies

This reduces the usefulness by a lot.

Also, from the docs

Risk: Critical (vulnerable to repository compromise)

This check determines whether the project's GitHub Action workflows has dangerous code patterns.

image

Dangerous-Workflow (critical risk)

no dangerous workflow patterns detected

This makes me question the effectiveness of the product. It reports dangerous workflow as critical risk but then says it didn't find any dangerous workflow patterns?

At this point:

  • it can't detect supply chain vulnerabilities
  • it can't filter results to actual library code
  • it can't properly report the issues (see above)
  • static code analysis does not use clippy -- what does it use then? if it's better than clippy, then effort should be put to making clippy better. We already run clippy so it's not too important for us

I'm with @daxpedda on closing the PR

@daxpedda
Copy link
Collaborator

Alright, as there are no further approvals, I'm going to close this.

Thanks @gabibguti nonetheless.

@daxpedda daxpedda closed this Oct 30, 2023
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.

Add the OpenSSF Scorecard GitHub Action
4 participants