ossf / scorecard

OpenSSF Scorecard - Security health metrics for Open Source
https://scorecard.dev
Apache License 2.0
4.63k stars 503 forks source link

Feature: better remediation steps #1850

Open laurentsimon opened 2 years ago

laurentsimon commented 2 years ago

It's pretty hard for a user to remediate some of the results:

We should try to do better. Some thoughts:

  1. Provide a link to the StepSecurity in the results presented in the scorecard action
  2. Provide a snippet of code that a user can copy/paste into a new PR
  3. Provide a link to create a new PR with the diff pre-populated
  4. Provide diff in SARIF if possible, and let GitHub surface this information

Starting with option (1) may be the easiest to start with.

/cc @varunsh-coder

varunsh-coder commented 2 years ago

It's pretty hard for a user to remediate some of the results:

  • permissions: which to use?
  • pinned dependencies: what's the hash for each version used?
  • dependabot: which options to enable in the config file + settings?

We should try to do better. Some thoughts:

  1. Provide a link to the StepSecurity in the results presented in the scorecard action
  2. Provide a snippet of code that a user can copy/paste into a new PR
  3. Provide a link to create a new PR with the diff pre-populated
  4. Provide diff in SARIF if possible, and let GitHub surface this information

Starting with option (1) may be the easiest to start with.

/cc @varunsh-coder

Adding more details to the above options:

  1. There is already a link to app.stepsecurity.io to fix token permissions and pin Action commits, but users need to click the link, find their workflow, copy it, paste it in etc. All these steps can be eliminated by having a link to app.stepsecurity.io/secureworkflows with repo and workflow as query string inputs. The UI can then fetch the workflow for open source projects and present the fixed workflow result, which the user can copy and paste back.
  2. This option improves on the previous one by having the fixed workflow snippet be in the SARIF file, so one does not have to go to another link. Copying this diff might not be user-friendly, so this option can also be combined with 1st option, where user gets both the diff and a link to view the diff on app.stepsecurity.io/secureworkflows and copy from there.
  3. w.r.t creating PR we need to investigate if we can do this without need for an App that has write access to the repo. If anyone knows how other solutions do this, please share that info.
  4. w.r.t diff in SARIF, I will check with GitHub if there is an option to surface remediation information if it is included in SARIF

@zwass since you originally requested this, please share insights on what would work best for you/ ideas to improve these options.

laurentsimon commented 2 years ago

@varunsh-coder for (1), do you already have an API we could use now? (app.stepsecurity.io/secureworkflows?q=xxx)?

varunsh-coder commented 2 years ago

@varunsh-coder for (1), do you already have an API we could use now? (app.stepsecurity.io/secureworkflows?q=xxx)?

Not yet. I will create a work item for it. It should be doable in a few days time. I will get back once it is implemented.

laurentsimon commented 2 years ago

Fyi, that's where we create the markdown https://github.com/ossf/scorecard/blob/main/pkg/sarif.go#L397. Feel free to try things it and see what looks best to embed the link.

laurentsimon commented 2 years ago

we could also add it in https://github.com/ossf/scorecard/blob/main/pkg/sarif.go#L436 which is closer to the code highlighted by GitHub in the dashboard

varunsh-coder commented 2 years ago

Support for point 1 has been added. The format of the website is https://app.stepsecurity.io/secureworkflow/:owner/:repo/:workflowname/:branch

Here are couple of examples: https://app.stepsecurity.io/secureworkflow/caolan/async/ci.yml/master https://app.stepsecurity.io/secureworkflow/microsoft/vscode/ci.yml/main

After the page loads with the fixed workflow, the user can uncheck options and click "secure workflow" button to change options. Let me know if there is feedback, I can change the inputs/ outputs as required.

For points 2 and 3, I believe this support does not exist as of now, as per issue https://github.com/github/codeql-action/issues/1043. We could still place fixed workflow in the SARIF output to see how it looks.

laurentsimon commented 2 years ago

Looks great. Since this is for token permissions and pinning, could we disable the hardened runner for this use case? Maybe via a a ?enable=permissions,pinning or something to this effect?

varunsh-coder commented 2 years ago

Looks great. Since this is for token permissions and pinning, could we disable the hardened runner for this use case? Maybe via a a ?enable=permissions,pinning or something to this effect?

Yes, that should be easy to do. Will implement and get back.

varunsh-coder commented 2 years ago

Looks great. Since this is for token permissions and pinning, could we disable the hardened runner for this use case? Maybe via a a ?enable=permissions,pinning or something to this effect?

Yes, that should be easy to do. Will implement and get back.

This is done. The format is https://app.stepsecurity.io/secureworkflow/:owner/:repo/:workflowname/:branch?enable=permissions,pin

Here are couple of examples: https://app.stepsecurity.io/secureworkflow/caolan/async/ci.yml/master?enable=permissions,pin https://app.stepsecurity.io/secureworkflow/microsoft/vscode/ci.yml/main?enable=permissions,pin

For completeness, if someone wanted to all add harden-runner, it would be ?enable=permissions,pin,harden If enable query string is not added, all options are enabled.

If the workflow cannot be fetched, because path is wrong, or it is a private repo, an error message is shown.

Let me know if there is other feedback.

laurentsimon commented 2 years ago

LGTM. How would you you like to proceed? Do you want to take a stab at the UI for the Action (https://github.com/ossf/scorecard/issues/1850#issuecomment-1106628256 and https://github.com/ossf/scorecard/issues/1850#issuecomment-1106730364) or you'd like us to look into it?

It would be great if we could have this for the next Action release, which is almost there https://github.com/ossf/scorecard-action/issues/97

varunsh-coder commented 2 years ago

LGTM. How would you you like to proceed? Do you want to take a stab at the UI for the Action (#1850 (comment) and #1850 (comment)) or you'd like us to look into it?

It would be great if we could have this for the next Action release, which is almost there ossf/scorecard-action#97

@laurentsimon would be great if you or another maintainer can integrate it. w.r.t the options, both look good. Having it display closer to the code #1850 (comment)) will show the developer that there is an easy way to solve it, which should increase fix rate.

laurentsimon commented 2 years ago

OK, tracked in https://github.com/ossf/scorecard-action/issues/97

varunsh-coder commented 2 years ago

One of the users got redirected to app.stepsecurity.io, but it was a no-op for them. The scorecard issue says top level 'checks' permission set to 'write'. At the same time, app.stepsecurity.io currently does not change top level permissions if they are already set. This would have resulted in some confusion for the user.

Here is the link they got: https://app.stepsecurity.io/secureworkflow/Mause/duckdb_engine/pythonapp.yaml/master?enable=permissions

What is the expectation in this case? As part of the fix, should app.stepsecurity.io re-organize the permissions and move the write permissions at the job level?

laurentsimon commented 2 years ago

What is the expectation in this case? As part of the fix, should app.stepsecurity.io re-organize the permissions and move the write permissions at the job level?

I think that would be great, yes. Note that I'm thinking of making scorecard smarter in the future, and not mark this sort of things as critical if there's a single job in the workflow; or if all jobs have their permissions defined.

varunsh-coder commented 1 year ago

I wanted to share some updates on automated remediations.

https://github.com/step-security/secure-workflows now

  1. Allows adding/ updating dependabot configuration based on the project's languages
  2. Adds CodeQL workflow if not present and sets appropriate languages to scan

Both these fixes increase the Scorecard score. Each option is optional, and the UI shows which improves the Scorecard score.

Instead of fixing one file at a time, there is an option to scan all files in the repo for remediations and apply them together using a PR. PR is created using a fork, and no App installation is needed. A lot of maintainers now use this PR option. Examples are in the README file.

Shortly, the following remediations will be added:

  1. Pin images in dockerfiles
  2. Add Dependency review workflow if missing
  3. Add Scorecard workflow if missing

Please let me know if there is any feedback or questions. Thanks!

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 60 days with no activity.

github-actions[bot] commented 11 months ago

This issue is stale because it has been open for 60 days with no activity.