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 support for SARIF output for secret scanning #920

Merged
merged 6 commits into from
Jun 27, 2024
Merged

Conversation

agateau-gg
Copy link
Collaborator

@agateau-gg agateau-gg commented Jun 24, 2024

Context

This PR adds support for generating reports in SARIF format when scanning for secrets.

See #869.

What has been done

The PR first introduces a --format (text|json) option, and make existing commands use it. --format json is a more verbose version of --json, but it makes it possible to add new formats without adding a new flag for each format.

Then it introduces a sarif value for --format. This value is only used by secret scan commands for now. Adding it to other verticals will be done separately.

Representing multi-match secrets in SARIF output

There is a bit of an impedance mismatch between ggshield secret output and SARIF output. A SARIF result is supposed to have one location, but a secret often contains multiple matches, so each match needs to be a separate location. To reconcile this, the result location starts at the start of the first match and ends at the end of the last match. Then each match is added as a "related" location.

This can be seen in this screenshot of a GitHub import of a ggshield SARIF document:

image(1)

I am open to suggestions regarding better ways to represent multi-match secrets.

Validation

The code can be tested in different ways.

The first thing is to generate some SARIF report. This can be done with a command like this:

ggshield secret scan path -ry --use-gitignore . --format sarif -o myreport.sarif

Then you can try to use the generated report in different places:

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

ggshield-test added 2 commits June 25, 2024 18:02
This prepare the room to add more formats, like SARIF.
agateau-gg and others added 2 commits June 25, 2024 18:25
This is important to let GitHub import SARIF output files
@agateau-gg agateau-gg linked an issue Jun 26, 2024 that may be closed by this pull request
Copy link
Collaborator

@fnareoh fnareoh left a comment

Choose a reason for hiding this comment

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

Great addition :D , I tested and I have a few questions on options flags.

For the multi match secret it does not seem ideal but it is probably the best we can do. Do we know how Github Advance security & others displays those with SARIF format ?
I tested with the validator and pycharm extension it looks good to me !

@agateau-gg
Copy link
Collaborator Author

For the multi match secret it does not seem ideal but it is probably the best we can do. Do we know how Github Advance security & others displays those with SARIF format ?

I have no idea :/

I tested with the validator and pycharm extension it looks good to me !

Oh, I could not find a Pycharm extension! Need to look again.

This way the help output for `--json` can say it's a shorthand for
`--format json`, making the behavior between both less ambiguous.
Copy link
Collaborator

@salome-voltz salome-voltz left a comment

Choose a reason for hiding this comment

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

It looks good ! I tested it and it was great 😄
I agree with Garance, output might be a bit weird for multimatch secrets but it's a detail

ggshield/cmd/utils/common_options.py Show resolved Hide resolved
@fnareoh fnareoh marked this pull request as ready for review June 27, 2024 08:29
@fnareoh fnareoh merged commit ef2a491 into main Jun 27, 2024
26 checks passed
@fnareoh fnareoh deleted the agateau/sarif branch June 27, 2024 08:29
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.

Sarif Output Format Support
3 participants