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

[sarif] Sarif Conversion from Finding Node #5256

Merged
merged 15 commits into from
Jan 28, 2025
Merged

Conversation

DavidBakerEffendi
Copy link
Collaborator

@DavidBakerEffendi DavidBakerEffendi commented Jan 27, 2025

  • Implemented an extensible SARIF 2.1.0 conversion from Finding Nodes.
  • The actual design consists of SARIF-themed traits with the minimal properties we "want", followed by a version-specific case class implementation which may extend upon the base properties.
  • The query step has an implicit configuration which allows one to rename the tool, version, description, etc. related component of what is exported.
  • Test posts a sample SARIF file generated by joern-scan + toSarifJson to an online Microsoft SARIF validator.

Question to reviewers: Do we want the structure of a trait which is extended upon by versioned case classes? Or do we only want to support one version at a given time?

@DavidBakerEffendi
Copy link
Collaborator Author

toolFullName: String = "Joern - The Bug Hunter's Workbench",
toolInformationUri: URI = URI("https://joern.io"),
organization: String = "Joern.io",
semanticVersion: String = "4.x.x",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see the Joern REPL has a version call, which I can use to dynamically load this - one sec...

Copy link
Contributor

Choose a reason for hiding this comment

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

So semanticVersion refers to the joern version?
If you get that from joern via e.g. io.joern.joerncli.console.JoernConsole.version, then ocular would also refer to the joern version, which is probably fine.
That can be changed, but at the same time we should keep things simple...
just to make sure I understand correctly, can you elaborate on what that version means?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This version refers to the tool. I have a branch on Ocular which implements this work and runs this configuration before the REPL begins: https://github.com/ShiftLeftSecurity/ocular/compare/master...dave/ocular-sarif#diff-36b4f70cbc3441ca2033a3f58698e662fabe8f4e3fcaae662f7ba3c414782d00R55

I should likely name organization and semanticVersion more consistently with the tool prefix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mpollmeier I see that joerncli is not accessible here - where do you suggest I initialize this kind of config value for the REPL? I see there are some runBefore-style hooks in the BridgeBase interface but I'm wondering where specifically may be best

Copy link
Collaborator Author

@DavidBakerEffendi DavidBakerEffendi Jan 28, 2025

Choose a reason for hiding this comment

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

For now I've added something under BridgeBase which is able to obtain the version

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 sounds good

Copy link
Contributor

@mpollmeier mpollmeier left a comment

Choose a reason for hiding this comment

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

this looks nice and clean 👍🏻
left a few remarks / suggestions

@ml86 ml86 self-requested a review January 28, 2025 08:58
Copy link
Contributor

@ml86 ml86 left a comment

Choose a reason for hiding this comment

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

Please also add further unit tests which check not just the SARIF case classes but instead the resulting SARIF json.

@DavidBakerEffendi DavidBakerEffendi merged commit e2d410d into master Jan 28, 2025
5 checks passed
@DavidBakerEffendi DavidBakerEffendi deleted the dave/joern-sarif branch January 28, 2025 14:08
DavidBakerEffendi added a commit that referenced this pull request Jan 29, 2025
Just discovered the latest addition to #5256 of the sarif config was `val` and would not be able to be redefined for tools extending the REPL.
DavidBakerEffendi added a commit that referenced this pull request Jan 29, 2025
Just discovered the latest addition to #5256 of the sarif config was `val` and would not be able to be redefined for tools extending the REPL.
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.

3 participants