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.

Sign up for GitHub

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

Update to add devskim #83

Closed
wants to merge 6 commits into from
Closed

Update to add devskim #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8cc0781
Update main.yml to add devskim
rjmurillo Jun 12, 2024
03ace62
Update DevSkim to only run on Linux
rjmurillo Jun 12, 2024
d04bf52
Update Packages.props to add DevSkim analyzers
rjmurillo Jun 12, 2024
4c4e267
Update CodeAnalysis.props to add DevSkim
rjmurillo Jun 12, 2024
36f9d49
Remove DevSkim package
rjmurillo Jun 12, 2024
72015a5
Add DevSkim as a CLI tool
rjmurillo Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .config/dotnet-tools.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@
"dotnet-verify"
],
"rollForward": false
},
"microsoft.cst.devskim.cli": {
"version": "1.0.33",
"commands": [
"devskim"
],
"rollForward": false
}
}
}
10 changes: 10 additions & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,3 +104,13 @@ jobs:
uses: github/codeql-action/analyze@v3
with:
category: "/language:csharp"

- name: Run DevSkim scanner
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like that there isn't a simple way to repro issues locally. Can we integrate it in some way to make local dev work as well?

Good

  • Add to dotnet tools
  • Add dotnet tool restore to YAML (sorry, I thought I added it already)
  • Add basic pwsh script to run devskim

Better

  • Same as above, but integrate into MSBuild via Exec or similar

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would also avoid the linux-only issue the GitHub Action has

Copy link
Owner Author

Choose a reason for hiding this comment

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

There's a package we can integrate https://www.nuget.org/packages/Microsoft.CST.DevSkim/

Copy link
Owner Author

Choose a reason for hiding this comment

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

That would also avoid the linux-only issue the GitHub Action has

I'm already over the Windows builder. It's. So. Slow.

Zootopia slow sloth stamp

Copy link
Owner Author

Choose a reason for hiding this comment

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

@MattKotsenas Updated to also include the CA rules, so it's a "yes, and"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that package is just a lib though: https://nuget.info/packages/Microsoft.CST.DevSkim/1.0.33 so I'm not sure how to drive it from that package. Haven't looked much though so go for it.

Copy link
Owner Author

@rjmurillo rjmurillo Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can't use it anyway, doesn't support netstandard 2.0 (only 2.1). I'll go the global tool route so we have something. There's also a VSIX and VSCode extension, but I don't like that for various reasons.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@MattKotsenas we're going hold off on this. I've raised two issues with DevSkim, microsoft/DevSkim#619 and microsoft/DevSkim#618 to address the issues. 619 is the one I want, so it's super consistent with everything else and "just works"

Copy link
Owner Author

Choose a reason for hiding this comment

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

Update: The "Good" or "Better" options above are the resolution to microsoft/DevSkim#618

It seems that a Dockerfile defined action (as DevSkim-Action is) is restricted to Linux pipelines only - the workaround seems to be to migrate to a Typescript action. I found a similar issue reported in another repo on GitHub with the same conclusion - peter-evans/create-pull-request#40. For us, the workaround is a little undesirable here - the docker method allows us to ensure that the .NET SDK is available and pull the latest version of the DevSkim tool, without interfering with the users environment, and ultimately the equivalent behavior of the action is only a couple lines of YML.

Link: microsoft/DevSkim#618 (comment)

I think I'm going to abandon this until microsoft/DevSkim#619 is implemented. Right now all the rules are RegEx so the perf is 1) not good, and 2) not always attached to a symbol. There will need to be some reworking on their end to get this to work as a proper analyzer.

What we're missing out on doesn't seem too critical for this project. Lots of making sure you're not doing silly things like disabling HTTPS checks, making sure you use secure URIs, not using weak crypto algos, vulnerable NuGet packages, and so on. Some of those are covered already by other analyzers (built in or otherwise).

uses: microsoft/DevSkim-Action@v1
if: runner.os == 'Linux'

- name: Upload DevSkim scan results to GitHub Security tab
uses: github/codeql-action/upload-sarif@v2
if: runner.os == 'Linux'
with:
sarif_file: devskim-results.sarif
Loading