Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

adds support for sarif output #453

Merged
merged 31 commits into from
Aug 18, 2022
Merged

Conversation

dani-santos-code
Copy link
Contributor

@dani-santos-code dani-santos-code commented Jul 8, 2022

Closes #436

Left some comments/questions for reviewers as part of self-review

go run cmd/main.go -f internal/sarif/fixtures/apparmor-invalid.yaml all --format="sarif" > kubeaudit.sarif will yield a new sarif report (kubeaudit.sarif):

{
  "version": "2.1.0",
  "$schema": "https://json.schemastore.org/sarif-2.1.0-rtm.5.json",
  "runs": [
    {
      "tool": {
        "driver": {
          "informationUri": "https://github.com/Shopify/kubeaudit",
          "name": "kubeaudit",
          "rules": [
            {
              "id": "AppArmorInvalidAnnotation",
              "name": "apparmor",
              "shortDescription": {
                "text": "AppArmorInvalidAnnotation"
              },
              "help": {
                "text": "**Type**: kubernetes\n**Auditor Docs**: To find out more about the issue and how to fix it, follow [this link](https://github.com/Shopify/kubeaudit/blob/main/docs/auditors/apparmor.md)\n**Description:** Finds containers that do not have AppArmor enabled\n\n *Note*: These audit results are generated with `kubeaudit`, a command line tool and a Go package that checks for potential security concerns in kubernetes manifest specs. You can read more about it at https://github.com/Shopify/kubeaudit "
              },
              "properties": {
                "tags": [
                  "security",
                  "kubernetes",
                  "infrastructure"
                ]
              }
            },
            {
              "id": "AutomountServiceAccountTokenTrueAndDefaultSA",
              "name": "asat",
              "shortDescription": {
                "text": "AutomountServiceAccountTokenTrueAndDefaultSA"
              },
              "help": {
                "text": "**Type**: kubernetes\n**Auditor Docs**: To find out more about the issue and how to fix it, follow [this link](https://github.com/Shopify/kubeaudit/blob/main/docs/auditors/asat.md)\n**Description:** Finds containers where the deprecated SA field is used or with a mounted default SA\n\n *Note*: These audit results are generated with `kubeaudit`, a command line tool and a Go package that checks for potential security concerns in kubernetes manifest specs. You can read more about it at https://github.com/Shopify/kubeaudit "
              },
              "properties": {
                "tags": [
                  "security",
                  "kubernetes",
                  "infrastructure"
                ]
              }
            }
          ]
        }
      },
      "results": [
        {
          "ruleId": "AppArmorInvalidAnnotation",
          "ruleIndex": 0,
          "level": "error",
          "message": {
            "text": "Details: AppArmor annotation key refers to a container that doesn't exist. Remove the annotation 'container.apparmor.security.beta.kubernetes.io/container: badval'.\n Auditor: apparmor\nDescription: Finds containers that do not have AppArmor enabled\nAuditor docs: https://github.com/Shopify/kubeaudit/blob/main/docs/auditors/apparmor.md "
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "internal/sarif/fixtures/apparmor-invalid.yaml",
                  "uriBaseId": "ROOTPATH"
                },
                "region": {
                  "startLine": 1
                }
              }
            }
          ]
        },
        {
          "ruleId": "AutomountServiceAccountTokenTrueAndDefaultSA",
          "ruleIndex": 1,
          "level": "error",
          "message": {
            "text": "Details: Default service account with token mounted. automountServiceAccountToken should be set to 'false' on either the ServiceAccount or on the PodSpec or a non-default service account should be used.\n Auditor: asat\nDescription: Finds containers where the deprecated SA field is used or with a mounted default SA\nAuditor docs: https://github.com/Shopify/kubeaudit/blob/main/docs/auditors/asat.md "
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "internal/sarif/fixtures/apparmor-invalid.yaml",
                  "uriBaseId": "ROOTPATH"
                },
                "region": {
                  "startLine": 1
                }
              }
            }
          ]
        }
      ]
    }
  ]
}

Examples of the sarif output being used with Github Code Scanning. SARIF uploaded with a Github action that _thepwagner set up in a private repo:

Screen Shot 2022-07-22 at 5 32 56 PM

Screen Shot 2022-07-22 at 5 33 16 PM

Type of change
  • Bug fix 🐛
  • New feature ✨
  • This change requires a documentation update 📖
  • Breaking changes ⚠️ => if we rename Name to Rule, users of the library will need to update the code when they bump the version
How Has This Been Tested?
  • Running the following go run cmd/main.go -f internal/sarif/fixtures/apparmor-invalid.yaml all -s kubeaudit.sarif will yield a new sarif report (kubeaudit.sarif) as well as print the results on the terminal
Checklist:
  • I have 🎩 my changes (A 🎩 specifically includes pulling down changes, setting them up, and manually testing the changed features and potential side effects to make sure nothing is broken)
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The test coverage did not decrease
  • I have signed the appropriate Contributor License Agreement

@dani-santos-code dani-santos-code force-pushed the ds/adds-support-for-sarif-output branch 2 times, most recently from c6dd9a0 to dd892ec Compare July 11, 2022 14:49
@dani-santos-code dani-santos-code force-pushed the ds/adds-support-for-sarif-output branch 2 times, most recently from 653c2d6 to c0f0bfa Compare July 11, 2022 16:57
@dani-santos-code dani-santos-code force-pushed the ds/adds-support-for-sarif-output branch 12 times, most recently from d62d289 to 366d31b Compare July 22, 2022 18:46
@dani-santos-code dani-santos-code marked this pull request as ready for review July 22, 2022 21:29
@dani-santos-code dani-santos-code force-pushed the ds/adds-support-for-sarif-output branch from 82baa84 to 744b2df Compare July 25, 2022 19:31
@dani-santos-code dani-santos-code force-pushed the ds/adds-support-for-sarif-output branch from 744b2df to edae749 Compare July 25, 2022 19:40
clintonShopify
clintonShopify previously approved these changes Jul 25, 2022
Copy link

@clintonShopify clintonShopify left a comment

Choose a reason for hiding this comment

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

LGTM! +1

cmd/commands/root.go Outdated Show resolved Hide resolved
internal/sarif/rules.go Outdated Show resolved Hide resolved
internal/sarif/sarif.go Outdated Show resolved Hide resolved
thepwagner
thepwagner previously approved these changes Jul 27, 2022
Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd appreciate a peek from someone with more experience in this codebase (@genevieveluyt). All concerns are in test code.

I wish there was an automated way to know that we set the new Auditor field on every *kubeaudit.AuditResult.
The Name->Rule rename is handy for bringing every *AuditResult declaration into this diff. I'm not going to pull this into an IDE and check every reference because I think reviewing the PR gives pretty good coverage.

auditors/deprecatedapis/depreceatedapis_test.go Outdated Show resolved Hide resolved
auditors/deprecatedapis/depreceatedapis_test.go Outdated Show resolved Hide resolved
internal/sarif/rules.go Outdated Show resolved Hide resolved
internal/sarif/rules.go Outdated Show resolved Hide resolved
internal/sarif/rules_test.go Outdated Show resolved Hide resolved
internal/sarif/sarif_test.go Show resolved Hide resolved
internal/sarif/sarif_test.go Outdated Show resolved Hide resolved
internal/sarif/sarif_test.go Show resolved Hide resolved
internal/sarif/sarif_test.go Show resolved Hide resolved
pkg/override/override.go Show resolved Hide resolved
Copy link
Contributor

@genevieveluyt genevieveluyt left a comment

Choose a reason for hiding this comment

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

This is really cool!

My main concern for the Name -> Rule switch was that it would be a runtime breaking change for the CLI but it's not! The CLI is unaffected and if folks upgrade the package as a dependency, it will fail during compile-time. The Rule + Auditor fields are an improvement in my eyes 👍

Other than that I mainly wondered about the choice of flags and the rest are nits.

Really nice that you introduced a sarif package to contain all that logic. Very clean and organized implementation 👏

README.md Outdated Show resolved Hide resolved
auditors/deprecatedapis/depreceatedapis_test.go Outdated Show resolved Hide resolved
auditors/deprecatedapis/depreceatedapis_test.go Outdated Show resolved Hide resolved
profile.out Outdated Show resolved Hide resolved
result.go Outdated Show resolved Hide resolved
internal/sarif/sarif.go Outdated Show resolved Hide resolved
internal/sarif/rules.go Show resolved Hide resolved
internal/sarif/sarif.go Outdated Show resolved Hide resolved
internal/sarif/sarif_test.go Show resolved Hide resolved
internal/sarif/rules_test.go Outdated Show resolved Hide resolved
genevieveluyt
genevieveluyt previously approved these changes Aug 17, 2022
Copy link
Contributor

@genevieveluyt genevieveluyt left a comment

Choose a reason for hiding this comment

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

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pkg/override/override.go Show resolved Hide resolved
Co-authored-by: Genevieve Luyt <[email protected]>
@dani-santos-code
Copy link
Contributor Author

dani-santos-code commented Aug 17, 2022

Copy link

@clintonShopify clintonShopify left a comment

Choose a reason for hiding this comment

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

+1 LGTM Nice work! :-)

@dani-santos-code dani-santos-code merged commit 32a65c8 into main Aug 18, 2022
@dani-santos-code dani-santos-code deleted the ds/adds-support-for-sarif-output branch August 18, 2022 13:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support for sarif
5 participants