Add Related Locations to Misconfiguration SARIF output

[cholesterol]

Description

👋🏻 Hey there,

I've already written the code necessary to address this and didn't realize I should have created a discussion first so an issue could get spun up. Apologies for doing this out of order. If folks agree this would be useful to have I can immediately open up a PR to close the corresponding feature issue.

Currently when using Trivy with it's corresponding GitHub Action I noticed that although I get related locations for Terraform modules in the standard table output that this is missing from the SARIF output.

As a result if you're working in a monorepo where lots of folks are using the same module as a dependency it can be incredibly difficult (if not impossible) to triage which local file made reference to that module, and where.

It would be helpful to have the same output that is available in the Table format available in GitHub via SARIF. Example of the output I'm referencing below.

The PR I'm working on instead takes any artifact and if it's not local (prepended by github.com) will change the markdown so the user gets a link to the upstream module code (including the line range/etc). They also get a list of artifacts local to that repository in their related locations that can be expanded in-line like any other GitHub Code-scanning alert. The output looks like the following.

• In the above example the artifact is now a link to the upstream module. If it were local it would instead be expandable.
• for related locations anything upstream is referenced as "remote" and is how a link in the markdown, anything local to the GitHub repository instead expands within the GitHub interface.
• This should be compatible with SARIF standards while also making GitHub Advanced Security output more legible and actionable.

---

I think this would be a huge QoL upgrade for folks that want to see the dependency path for terraform misconfigurations directly in GitHub.

Thank you and apologies again for doing this a bit out of order,

Target

Filesystem

Scanner

Misconfiguration

[simar7]

hi @cholesterol while I like the idea, I do think it will be hard to maintain this as not all modules are hosted off of GitHub. Do you have an example of a related location URL?

[cholesterol]

Hey @simar7, I'm only prepending https:// to URIs that start with github.com in the message text, so for other module sources nothing changes (although admittedly I'm also changing the ref= part of the link to blob instead so the link can work properly and also link you directly to the lines in question, this could be taken out though or refactored to only happen specifically with github.com URIs as well). I guess I'm not understanding what additional maintenance this really adds if it's just modifying the messages so they are actionable? Without this any alerts with absolute URIs aren't really actionable if you're using SARIF and GitHub Advanced Security since GitHub forces all absolute URIs into being relative when the SARIF gets uploaded, if there is a true cost in terms of maintenance I'd advocate that it might well be worth it for Trivy alerts to be infinitely more actionable with code-scanning when modules outside of the repo being scanned are involved.

I have an example of the output I'm generating. Here is an example of the SARIF prior to the code changes I've implemented in my fork (just for one result). I've included some mock HCL as well to simulate what would cause this type of alert.

fake hcl calling the terraform-aws-modules ssh module which has a default var for egress which is here

module "security_group_fake" {
  source  = "terraform-aws-modules/security-group/aws//modules/ssh"
  version = "3.17.0"

  name        = "fake-sg"
  description = "Allow SSH from specific set of IPs."

  vpc_id = fake.vpc_id
  ingress_cidr_blocks = [
    fake.cidrs.vpn,
  ]
}

Current form - there is no way to tell where in my terraform this upstream module was called via the SARIF, I only get information about the module that was called, not where in my actual terraform the module was called. In a small repo it's not a big deal to run the Trivy CLI tool locally and figure out where the local artifact is, but when you end up with multiple hits on the same rule you'll get multiple duplicate issues that all look the same. The artifact link doesn't work because it's using ref= vs /blob/ in the URL. It also has no line numbers in the URL to help get me instantly where I need to be. Admittedly none of that is all that important as knowing where in the module there is an issue doesn't help you pinpoint where it's being called locally.

       {
          "ruleId": "AVD-AWS-0104",
          "ruleIndex": 0,
          "level": "error",
          "message": {
            "text": "Artifact: github.com/terraform-aws-modules/terraform-aws-security-group?ref=27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf\nType: terraform\nVulnerability AVD-AWS-0104\nSeverity: CRITICAL\nMessage: Security group rule allows egress to multiple public internet addresses.\nLink: [AVD-AWS-0104](https://avd.aquasec.com/misconfig/avd-aws-0104)"
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "github.com/terraform-aws-modules/terraform-aws-security-group?ref=27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf",
                  "uriBaseId": "ROOTPATH"
                },
                "region": {
                  "startLine": 440,
                  "startColumn": 1,
                  "endLine": 440,
                  "endColumn": 1
                }
              },
              "message": {
                "text": "github.com/terraform-aws-modules/terraform-aws-security-group?ref=27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf"
              }
            }
          ]
        }
      ]

w/ related locations URIs (grabbed from additional occurrences on the same cause metadata that Trivy generates). I've removed the actual local path and just given an example. This also gives a good idea of what the format of the eventual SARIF is with that new markdown generation. Artifact link in the message text is actually a clickable link now and has blob vs ref=?, also has the line numbers that are relevant to the alert in the upstream module

       {
          "ruleId": "AVD-AWS-0104",
          "ruleIndex": 0,
          "level": "error",
          "message": {
            "text": "Artifact: [link](https://github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf#L440-L440)\nType: terraform\nVulnerability AVD-AWS-0104\nSeverity: CRITICAL\nMessage: Security group rule allows egress to multiple public internet addresses.\nLink: [AVD-AWS-0104](https://avd.aquasec.com/misconfig/avd-aws-0104)\nRelated Locations: [remote](https://github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf#L434-L448) [remote](https://github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/modules/ssh/main.tf#L1-L115) [local](3) "
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf",
                  "uriBaseId": "ROOTPATH"
                },
                "region": {
                  "startLine": 440,
                  "startColumn": 1,
                  "endLine": 440,
                  "endColumn": 1
                }
              },
              "message": {
                "text": "github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf"
              }
            }
          ],
          "relatedLocations": [
            {
              "id": 1,
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf",
                  "uriBaseId": "ROOTPATH"
                },
                "region": {
                  "startLine": 434,
                  "startColumn": 1,
                  "endLine": 448,
                  "endColumn": 1
                }
              },
              "message": {
                "text": "github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/main.tf"
              }
            },
            {
              "id": 2,
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/modules/ssh/main.tf",
                  "uriBaseId": "ROOTPATH"
                },
                "region": {
                  "startLine": 1,
                  "startColumn": 1,
                  "endLine": 115,
                  "endColumn": 1
                }
              },
              "message": {
                "text": "github.com/terraform-aws-modules/terraform-aws-security-group/blob/27e2fabaa1debe32c253886e91e6415e2c194f50/modules/ssh/main.tf"
              }
            },
            {
              "id": 3,
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "local/artifact/path/here/main.tf",
                  "uriBaseId": "ROOTPATH"
                },
                "region": {
                  "startLine": 1,
                  "startColumn": 1,
                  "endLine": 12,
                  "endColumn": 1
                }
              },
              "message": {
                "text": "local/artifact/path/here"
              }
            }
          ]
        }
      ],

What's nice about this is that now because I have related location URIs from those additional occurrences I can track down where someone in a large monorepo is calling in that community module and using the default module variables (in this case defaulting to allowing all egress traffic). This could be useful in a situation where an engineer wants dismiss an alert as a "won't fix" for one part of their code if they have a good reason, or perhaps file a temporary exception with their security team, but only for that specific instance of a module being called in that specific location. Without that we would basically have to use --tf-exclude-downloaded-modules and either not get any of those alerts in GitHub, or run Trivy locally and manually create issues so we have the flexibility to file exceptions for a few alerts for the same rule.

I suppose I could wrap Trivy in my own GitHub action and manually generate SARIF with the output of the table (since it has those incredibly useful additional paths), but I figured other folks would maybe find this useful since the docs on SARIF recommend using it for GitHub Advanced Security - in it's current state the SARIF reporting isn't really useful if you're calling an upstream module with defaults. At that point I'd probably be better off just using my fork separately with my own action, I do think other folks would get benefits out of this though if they are using GitHub so I figured I'd give it a shot if other folks are interested.

[cholesterol]

@simar7 @nikpivkin 👋🏻 ,

I'm also noticing now as I'm trying to use the --tf-exclude-downloaded-modules flag as a workaround that lots of my historical issues related to upstream providers don't seem to close or open like I would expect. I read through the upload sarif source code and it looks like GitHub uses the primary artifact location as a fallback when computing hashes to generate a fingerprint if the sarif is missing this information, and that it has to be local to the repository.

Is this intended behavior? Should the docs be updated to dissuade people not excluding downloaded tf modules when using Trivy in an actions pipeline since it doesn't generate fingerprints? Or perhaps just be updated to let people know of the potential gotchas when implementing Trivy w/ Advanced Security? Should the primary location for any given result generated from an upstream provider be the location in the local source code instead of the upstream artifact so these can reliably be used for fingerprint generation? As it stands if you're calling the same module in multiple locations they will all have the same primary artifact location and share the same fingerprint which will increase the odds of dupes/inconsistent closing/opening of alerts in GitHub Advanced Security.

Here is an example of where this would be an issue (using the vscode sarif extension to view the raw sarif)

Since these both share the exact same location as the root cause of the misconfiguration (line 440/441 in the community security group provider) I believe they will share a fingerprint.

[simar7]

I think it's best to ask GitHub about what their expectation is in this case. If you think the current implementation doesn't suit your use case, you can always write a Trivy plugin to cater the output to your needs: https://aquasecurity.github.io/trivy/v0.52/docs/plugin/developer-guide/

[cholesterol]

Hey @simar7 - they do document that they expect partialFingerprints as per the official sarif specifications here

I guess what I'm getting at is that to say that Trivy supports sarif (and the docs even recommend using it for GitHub) is a bit misleading if it's not following the specifications outlined by both GitHub and OASIS for it to function as intended. At least documenting these "gotchas" would be beneficial to other consumers of the tool so they don't run into these headaches going forward.

[cholesterol]

also a related discussion on fingerprints here

[simar7]

Fair enough, I think I understand it now. As you mentioned, it would be very helpful if you could document your findings so far to explain the caveats we have today. The best place to do so would be the Trivy Action repo's README here: https://github.com/aquasecurity/trivy-action

[cholesterol]

Since Trivy itself is what's generating the Sarif and the documentation around Sarif is for Trivy itself and not the action that doesn't make a whole lot of sense to me, people find their way to the action by way of the Trivy docs. In any event I've already spent too much time on this between my fork which is a massive QoL upgrade and the back and forth on how sarif works (with the maintainers of a tool that advertises sarif support). I'll find another tool to use.