SARIF Output Support

[anthturner]

Signed-off-by: Anthony Turner 225599+anthturner@users.noreply.github.com

What I did

Added SARIF-compatible output structures as an output option in the same vein as SonarQube

How to verify it

Use -o sarif as an option with Horusec to output a SARIF report

- Description for the changelog
Adds SARIF output support

---

Want to note that this is not necessarily complete; there are several things which just don't exist in Horusec right now. For example, I notice not all of the engine modules have RuleIDs populated, and there is other metadata (such as URL) which need to have a lookup table or some other place to pull them from. This might mean authoring a .csv file to track the metadata or maybe embedding it into code somehow is better.

Hopefully this at least helps get the conversation started.

• See also SARIF Output Support #937

[anthturner]

I might take a crack at adding RuleId to the records for non-HorusecEngine scanners. I'll do that as a separate PR since it'll be a different set of potential issues to fix.

[matheusalcantarazup]

and there is other metadata (such as URL) which need to have a lookup table or some other place to pull them from.

I think that this would be hard to do, since we not control the results of the tools that we orchestrate.

For example, I notice not all of the engine modules have RuleIDs populated

This is because we not populate this field from tools that we orchestrate. This field should be unique, I don't know how we can control this "uniqueness" for this other tools.

BTW, the schema says that ruleId should be a object, not a string, I'm seeing the correct schema?

I would like to suggest to using git commit --amend to amed your changes on the current git commit instead create a new one, with this you don't need to add DCO on all commits (and when is PR will be merged all commits will be squashed anyway).

[matheusalcantarazup]

Oh, I think that I was using the wrong schema, this implementation is based on this documentation right?

[anthturner]

Oh, I think that I was using the wrong schema, this implementation is based on this documentation right?

Correct. RuleID is a string rather than object. It's mostly just used for correlation between multiple files/lines which have the same violation type.

[anthturner]

For example, I notice not all of the engine modules have RuleIDs populated

This is because we not populate this field from tools that we orchestrate. This field should be unique, I don't know how we can control this "uniqueness" for this other tools.

See #949 for my thoughts on how we can introduce this.

[matheusalcantarazup]

Correct. RuleID is a string rather than object. It's mostly just used for correlation between multiple files/lines which have the same violation type.

According to the documentation this field is optional, can we follow with this implementation separately from adding the rule id for all vulnerabilities?

Maybe for this initial implementation we can leave this rule id empty (for vulnerability that was not found by horusec engine) and when we found a solution for #949 we will automatically have this value on output for all tools.

I'm not finding the helpUri and informationUri fields on docs, these fields is optional too? If it yes we can use the same approach described above.

cc @wiliansilvazup @nathanmartinszup @iancardosozup

[anthturner]

Correct. RuleID is a string rather than object. It's mostly just used for correlation between multiple files/lines which have the same violation type.

According to the documentation this field is optional, can we follow with this implementation separately from adding the rule id for all vulnerabilities?

Maybe for this initial implementation we can leave this rule id empty (for vulnerability that was not found by horusec engine) and when we found a solution for #949 we will automatically have this value on output for all tools.

I'm not finding the helpUri and informationUri fields on docs, these fields is optional too? If it yes we can use the same approach described above.

cc @wiliansilvazup @nathanmartinszup @iancardosozup

I'm fine dropping the URIs -- again, those were originally for my own purposes, since they can link out to corresponding doc pages. That's incredibly useful for developers to help them fix an error they may not understand.

A lot of how I'm thinking of this visually can be seen by using:
https://microsoft.github.io/sarif-web-component/

I've attached two sample SARIF files (as TXT, because of GitHub limitations)

sample-a.sarif.txt -- This one uses RuleId for grouping
sample-b.sarif.txt -- This one does not group

These files can be used with the above viewer to visualize what this might look like in an external tool.

You can probably see how scaling this out to what could be hundreds, if not thousands, of vulnerabilities across large targets (as in my use case) would be challenging without an adequate grouping mechanism. Just a thought. 😄

[iancardosozup]

Maybe for this initial implementation we can leave this rule id empty (for vulnerability that was not found by horusec engine) and when we found a solution for #949 we will automatically have this value on output for all tools.

I guess this is a good approach to get sarif output in a quicker way, but i think #949 should be prioritized since we're gonna get an inconsistent output between our engine and tools that we orchestrate.

[matheusalcantarazup]

You can probably see how scaling this out to what could be hundreds, if not thousands, of vulnerabilities across large targets (as in my use case) would be challenging without an adequate grouping mechanism. Just a thought.

This is definitely a problem, but I think that we should concentrate the discussion about the rule id on #949, and on this PR we ensure that we export the Sarif output correctly.

[matheusalcantarazup]

I think that would be good add some unit tests, to ensure that we convert the analysis vulnerabilities to sarif.Report struct correctly.

To test this, we can basically create an analysis.Analysis with a bunch of vulnerabilities and them call ConvertVulnerabilityToSarif and assert that the fields from Report is filled correctly.

Please, ask any question if you have any problem to create these tests.

[matheusalcantarazup]

There is some others linter warnings, you can check them here.

[iancardosozup]

@anthturner i think if you run make format and make lint on root repository would be easier to fix those lint errors that @matheusalcantarazup mentioned, and please add your signature to commit to pass the DCO check (find more about on CONTRIBUTING.md)

[anthturner]

@anthturner i think if you run make format and make lint on root repository would be easier to fix those lint errors that @matheusalcantarazup mentioned, and please add your signature to commit to pass the DCO check (find more about on CONTRIBUTING.md)

Aside from the challenges of line lengths and function lengths with this one, I'm stuck on a number of similar errors to this: importShadow: shadow of imported from 'github.com/ZupIT/horusec-devkit/pkg/entities/analysis' package 'analysis'

That occurs when I don't use the aliasing of the package on import; if I do, this doesn't appear.

Googling it is not much help; was hoping someone could help me along so I can pass the linter.

[nathanmartinszup]

@anthturner i think if you run make format and make lint on root repository would be easier to fix those lint errors that @matheusalcantarazup mentioned, and please add your signature to commit to pass the DCO check (find more about on CONTRIBUTING.md)

Aside from the challenges of line lengths and function lengths with this one, I'm stuck on a number of similar errors to this: importShadow: shadow of imported from 'github.com/ZupIT/horusec-devkit/pkg/entities/analysis' package 'analysis'

That occurs when I don't use the aliasing of the package on import; if I do, this doesn't appear.

Googling it is not much help; was hoping someone could help me along so I can pass the linter.

This error occurs when a package and a variable have the same name, as @matheusalcantarazup suggested you can rename the variables as analysiss to avoid this.

[matheusalcantarazup]

Can you please add a documentation about this private functions that do the parsing using this guide? They seems a bit tricky do understand in first place.

I'm not convinced about this methods should have to many arguments, but for now I don't have any better idea, maybe embed on Sarif struct, so these methods just use them?

cc @wiliansilvazup @nathanmartinszup @iancardosozup

[iancardosozup]

Can you please add a documentation about this private functions that do the parsing using this guide? They seems a bit tricky do understand in first place.

I'm not convinced about this methods should have to many arguments, but for now I don't have any better idea, maybe embed on Sarif struct, so these methods just use them?

cc @wiliansilvazup @nathanmartinszup @iancardosozup

i agree, if these fields are into Sarif struct the method initToolStructure could be refactored to fit in NewSarif method

[anthturner]

Can you please add a documentation about this private functions that do the parsing using this guide? They seems a bit tricky do understand in first place.
I'm not convinced about this methods should have to many arguments, but for now I don't have any better idea, maybe embed on Sarif struct, so these methods just use them?
cc @wiliansilvazup @nathanmartinszup @iancardosozup

i agree, if these fields are into Sarif struct the method initToolStructure could be refactored to fit in NewSarif method

This was a great suggestion and helped considerably.

I added documentation as best as I could, including why each association map was necessary. It's admittedly a challenge, as the SARIF format requires very specific grouping of different elements which Horusec does not differentiate between in its report by default.

[matheusalcantarazup]

LGTM

Thanks very much for your contribution @anthturner

[matheusalcantarazup]

Some commits still failing because of DCO. I suggest you squash all 15 commits in a single one signed commit.

To fix linter warnings you can run make format and them commit those changes.

[iancardosozup]

LGTM. Thanks for yoru contribution, you rock!

[nathanmartinszup]

Awesome job @anthturner.

Thanks for your contribution!

[anthturner]

Some commits still failing because of DCO. I suggest you squash all 15 commits in a single one signed commit.

To fix linter warnings you can run make format and them commit those changes.

Will do. One thing I noticed, if you want to add it to docs, is that the goimports tool requires $HOME/go/bin be in PATH for the make format command to run properly. (On MacOS, this is not the case by default)

I'm currently struggling with the next part of this, which is that make format seems to be expecting a very specific path format:

❯ make format
go install golang.org/x/tools/cmd/goimports@latest
go install mvdan.cc/gofumpt@latest
go install github.com/daixiang0/gci@latest
gofmt -s -l -w $(find . -name '*.go' | grep -v vendor | grep -v /examples/)
goimports -w -local github.com/ZupIT/horusec/ $(find . -name '*.go' | grep -v vendor | grep -v /examples/)
gofumpt -l -w $(find . -name '*.go' | grep -v vendor | grep -v /examples/)
gci -w -local github.com/ZupIT/horusec/ $(find . -name '*.go' | grep -v vendor | grep -v /examples/)
Using the old parameters is deprecated, please use the named subcommands!
Error: stat github.com/ZupIT/horusec/: no such file or directory

Note that for me, I'm cloned into ~/github_forks/horusec.

Thoughts?

[anthturner]

I'd appreciate your insight on an issue we've discussed in the past; "ruleId". For engines which don't have this, the SARIF validator fails with:

SARIF1010: runs[1].results[0]: This result contains neither of the properties 'ruleId' or 'rule.id'. The SARIF specification ([§3.27.5](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317643)) requires at least one of these properties to be present.

In this case, would it make sense to make ruleId the same as the result text, just in the interest of meeting the specification?

[matheusalcantarazup]

I'm currently struggling with the next part of this, which is that make format seems to be expecting a very specific path format:

The gci release a new version with some breaking changes. I'll open a PR to fix this, but I found a bug on this new version too, so gci format will still broken after this PR. The tool that fix the linter issue still working, so you can run make format and commit the changes anyway. You already fix, linter is happy now.

In this case, would it make sense to make ruleId the same as the result text, just in the interest of meeting the specification?

I don't know about this, maybe tool name? I'm afraid of use the result text and make the rule id field to big.

cc @nathanmartinszup @iancardosozup

[iancardosozup]

Reading the sarif converter error @anthturner mentioned i found some rules we should base on:

Not all existing analysis tools emit the equivalent of a ruleId in their output. A SARIF converter which converts the output of such an analysis tool to the SARIF format SHOULD synthesize ruleId from other information available in the analysis tool's output.

Each SARIF converter might synthesize ruleId in a different way. Therefore, a SARIF consumer SHOULD NOT attempt to compare or combine the output from different converters for the same analysis tool. See Appendix D for more information about production of SARIF by converters.

If the input data does not include an equivalent for any SARIF element, a converter MAY attempt to synthesize that element. (For example, a converter might heuristically extract a rule id from the text of an unstructured error message.)

Since each converter might synthesize SARIF elements differently (notably the rule id; see §3.27.5), a SARIF consumer SHOULD NOT attempt to combine results produced by different converters for the same tool.

based on that i suggest to review every tool that does not generate an rule id and generate id based on some retrieved information( i don't think we can be generic here ), but we have to ensure this information remains stable between our versions ( so we're back to #949 )

[anthturner]

Reading the sarif converter error @anthturner mentioned i found some rules we should base on:

Not all existing analysis tools emit the equivalent of a ruleId in their output. A SARIF converter which converts the output of such an analysis tool to the SARIF format SHOULD synthesize ruleId from other information available in the analysis tool's output.

Each SARIF converter might synthesize ruleId in a different way. Therefore, a SARIF consumer SHOULD NOT attempt to compare or combine the output from different converters for the same analysis tool. See Appendix D for more information about production of SARIF by converters.

If the input data does not include an equivalent for any SARIF element, a converter MAY attempt to synthesize that element. (For example, a converter might heuristically extract a rule id from the text of an unstructured error message.)

Since each converter might synthesize SARIF elements differently (notably the rule id; see §3.27.5), a SARIF consumer SHOULD NOT attempt to combine results produced by different converters for the same tool.

based on that i suggest to review every tool that does not generate an rule id and generate id based on some retrieved information( i don't think we can be generic here ), but we have to ensure this information remains stable between our versions ( so we're back to #949 )

Reference #967 for addition of many RuleIDs

[wiliansilvazup]

thank you very much for your contribution @anthturner, it was really an excellent job done here. I'm very happy that more people are interested in our project. You rock 🚀

[nathanmartinszup]

Reading the sarif converter error @anthturner mentioned i found some rules we should base on:
Not all existing analysis tools emit the equivalent of a ruleId in their output. A SARIF converter which converts the output of such an analysis tool to the SARIF format SHOULD synthesize ruleId from other information available in the analysis tool's output.
Each SARIF converter might synthesize ruleId in a different way. Therefore, a SARIF consumer SHOULD NOT attempt to compare or combine the output from different converters for the same analysis tool. See Appendix D for more information about production of SARIF by converters.
If the input data does not include an equivalent for any SARIF element, a converter MAY attempt to synthesize that element. (For example, a converter might heuristically extract a rule id from the text of an unstructured error message.)
Since each converter might synthesize SARIF elements differently (notably the rule id; see §3.27.5), a SARIF consumer SHOULD NOT attempt to combine results produced by different converters for the same tool.
based on that i suggest to review every tool that does not generate an rule id and generate id based on some retrieved information( i don't think we can be generic here ), but we have to ensure this information remains stable between our versions ( so we're back to #949 )

Reference #967 for addition of many RuleIDs

I'm not sure how we should proceed with this. How useful will this information be for the user? Do we need to print this information? Maybe it should be something unique to the sarif parser.

[matheusalcantarazup]

@anthturner can we merge this PR and focus the discussion about rule id on #967? There is something more that we need to discuss here about Sarif output specifically?

[anthturner]

@anthturner can we merge this PR and focus the discussion about rule id on #967? There is something more that we need to discuss here about Sarif output specifically?

Absolutely feel free to merge this; I assumed we'd move to the #967 discussion anyways :)

[matheusalcantarazup]

Thanks very much @anthturner