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

Use Scorecard on OSS-Fuzz #7425

Open
jonathanmetzman opened this issue Mar 23, 2022 · 12 comments
Open

Use Scorecard on OSS-Fuzz #7425

jonathanmetzman opened this issue Mar 23, 2022 · 12 comments

Comments

@jonathanmetzman
Copy link
Contributor

jonathanmetzman commented Mar 23, 2022

We want to use the action.
We can start this process by running scorecard manually on OSS-Fuzz and fixing the issues/fixing scorecard.

@jonathanmetzman
Copy link
Contributor Author

CC @oliverchang @laurentsimon

@jonathanmetzman
Copy link
Contributor Author

jonathanmetzman commented Mar 23, 2022

@laurentsimon
The first issue I see scorecard flag for us is Binary-Artifacts such as:

infra/cifuzz/test_data/build-out/example_crash_fuzzer
infra/cifuzz/test_data/build-out/example_nocrash_fuzzer

It's not really practical to compile these binaries during tests, we don't want compilers as dependencies for OSS-Fuzz.
I see that scorecard ignores binaries in testdata? Should this be extend to other common names for the same concept.
We use test_data. I think this is pretty common, Chrome uses test_data and testdata: https://source.chromium.org/search?q=path:test_data&ss=chromium

I can file an issue about this.

@jonathanmetzman
Copy link
Contributor Author

jonathanmetzman commented Mar 23, 2022

Branch-Protection: 2/10
I don't think we want to remediate this warning: Warn: number of required reviewers is only 1 on branch 'master'. OSS-Fuzz gets many PRs given the number of maintainers and these PRs typically only affect one project, the damage is pretty limited.
I don't think we should change our policy to make this warning go away.

Other complaints:

  • settings do not apply to administrators
  • status checks do not require up to date branches for master
  • stale approvals not dismissed.

Stale approvals not dismissed seems somewhat important to me, but enforcing this would increase maintainers' workloads and make things harder for users.

@jonathanmetzman
Copy link
Contributor Author

jonathanmetzman commented Mar 23, 2022

SAST: 0/10

  • 0 commits out of 30 are checked with a SAST tool
  • CodeQL tool not detected

Do we need to use codeql to get the full score?

@jonathanmetzman
Copy link
Contributor Author

jonathanmetzman commented Mar 23, 2022

Pinned-Dependencies: 0/10

This one has too many false positives/mismatches to list.
OSS-Fuzz can't pin docker images like scorecards expects, it's too impractical given oss-fuzz's design

@jonathanmetzman
Copy link
Contributor Author

Dependency-Update-Tool: 0/10

  • Warn: dependabot config file not detected in source location

@jonathanmetzman
Copy link
Contributor Author

Fuzzing: 0/10

  • Warn: project is not fuzzed

Ironic :-)

@jonathanmetzman
Copy link
Contributor Author

Packaging: -1

  • no GitHub publishing workflow detected

I'm not sure we want packages for this repo, the repo is basically an interface for a service, not really a package people want to use.

@jonathanmetzman
Copy link
Contributor Author

Signed-Releases: ?

  • Warn: no GitHub releases found

Don't think we need releases either

@jonathanmetzman
Copy link
Contributor Author

jonathanmetzman commented Mar 23, 2022

Token-Permissions: 0/10

  • Warn: no top level permission defined: .github/workflows/infra_tests.yml:1
  • Warn: no top level permission defined: .github/workflows/presubmit.yml:1
  • Warn: no top level permission defined: .github/workflows/project_tests.yml:1

@laurentsimon
Copy link

Thanks @jonathanmetzman I agree with most of your conclusions.

For binary data: agreed this is false positive, and we're aware of this. You have the choice to "Won't Fix" in the scanning dashboard to get rid of the alert. Is that sufficient in practice or do you think we should provide a config file (this issue is also asking for something similar ossf/scorecard-action#143) for files to ignore in the action

For Pinned-Dependencies: you say it has too many false positives: can you explain?

CodeQl: you can enable that, yes. Is there another tool you're using? I have a pending ossf/scorecard#1487 to support more tools so I can add other tools if necessary

Branch-Protection: settings do not apply to administrators is one setting you should probably set. stale approvals not dismissed is hard to enforce in practice due to usability concerns, as you pointed out

Signed-Release: we've removed this from the GitHub action https://github.com/ossf/scorecard-action/blob/main/policies/template.yml#L52 so you should not receive this alert anymore.

@jonathanmetzman
Copy link
Contributor Author

For Pinned-Dependencies: you say it has too many false positives: can you explain?

Eh...I was sort of incorrect. They aren't false positives, scorecard was right that we weren't pinning. But I meant that most of the instances flagged (there were so many that I didn't look through all of them) can't really be fixed by us. I'll elaborate more in a bit

asraa pushed a commit that referenced this issue Mar 23, 2022
jonathanmetzman added a commit that referenced this issue Mar 24, 2022
MartinPetkov pushed a commit to MartinPetkov/oss-fuzz that referenced this issue Aug 15, 2022
MartinPetkov pushed a commit to MartinPetkov/oss-fuzz that referenced this issue Aug 15, 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

No branches or pull requests

2 participants