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

Feature: better remediation steps #1850

Open
laurentsimon opened this issue Apr 19, 2022 · 17 comments
Open

Feature: better remediation steps #1850

laurentsimon opened this issue Apr 19, 2022 · 17 comments
Labels
kind/enhancement New feature or request

Comments

@laurentsimon
Copy link
Contributor

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

@varunsh-coder
Copy link
Contributor

varunsh-coder commented Apr 20, 2022

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
Copy link
Contributor Author

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

@varunsh-coder
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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 github/codeql-action#1043. We could still place fixed workflow in the SARIF output to see how it looks.

@laurentsimon
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

laurentsimon commented Apr 29, 2022

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

@varunsh-coder
Copy link
Contributor

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
Copy link
Contributor Author

OK, tracked in ossf/scorecard-action#97

@varunsh-coder
Copy link
Contributor

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
Copy link
Contributor Author

laurentsimon commented Jun 15, 2022

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
Copy link
Contributor

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
Copy link

github-actions bot commented Oct 5, 2023

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

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

3 participants