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

feat: add the occurrences field #1383

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

nikpivkin
Copy link
Collaborator

@nikpivkin nikpivkin commented Jul 11, 2023

Added the occurrences field. Based on this field, we can display a list of occurrences in reports.
See aquasecurity/trivy#4581

Example of json output:

{
  "results": [
    {
      "rule_id": "AVD-AWS-0107",
      "long_id": "aws-ec2-no-public-ingress-sgr",
      "rule_description": "An ingress security group rule allows traffic from /0.",
      "rule_provider": "aws",
      "rule_service": "ec2",
      "impact": "Your port exposed to the internet",
      "resolution": "Set a more restrictive cidr range",
      "links": [
        "https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/security-group-rules-reference.html"
      ],
      "description": "Security group rule allows ingress from public internet.",
      "severity": "CRITICAL",
      "warning": false,
      "status": 0,
      "resource": "module.aws-security-groups[\"db1\"]",
      "occurrences": [
        {
          "resource": "aws_security_group_rule.ingress_with_cidr_blocks[0]",
          "filename": "terraform-aws-modules/security-group/aws/main.tf",
          "start_line": 191,
          "end_line": 227
        },
        {
          "resource": "module.aws-security-groups[\"db1\"]",
          "filename": "sg.tf",
          "start_line": 1,
          "end_line": 13
        }
      ],
      "location": {
        "filename": "terraform-aws-modules/security-group/aws/main.tf",
        "start_line": 197,
        "end_line": 204
      }
    }
  ]
}

@nikpivkin nikpivkin force-pushed the nik-report-resource branch from 022b942 to aa18860 Compare July 11, 2023 07:02
@nikpivkin
Copy link
Collaborator Author

@simar7 I followed the logic in tfsec and took only the first occurrence in the file, otherwise there will be a conclusion that seems verbose, but on the other hand, more accurate:

"occurrences": [
                                {
                                        "resource": "aws_security_group_rule.ingress_with_cidr_blocks[0]",
                                        "filename": "terraform-aws-modules/security-group/aws/main.tf",
                                        "start_line": 191,
                                        "end_line": 227
                                },
                                {
                                        "resource": "module.aws-security-groups[\"db1\"]",
                                        "filename": "sg.tf",
                                        "start_line": 1,
                                        "end_line": 13
                                }
                        ],

@simar7
Copy link
Member

simar7 commented Jul 12, 2023

@simar7 I followed the logic in tfsec and took only the first occurrence in the file, otherwise there will be a conclusion that seems verbose, but on the other hand, more accurate:

"occurrences": [
                                {
                                        "resource": "aws_security_group_rule.ingress_with_cidr_blocks[0]",
                                        "filename": "terraform-aws-modules/security-group/aws/main.tf",
                                        "start_line": 191,
                                        "end_line": 227
                                },
                                {
                                        "resource": "module.aws-security-groups[\"db1\"]",
                                        "filename": "sg.tf",
                                        "start_line": 1,
                                        "end_line": 13
                                }
                        ],

could you explain what you mean by first occurrence?

@nikpivkin
Copy link
Collaborator Author

@simar7 For example, if the metadata of the resource will refer to the following sources:

terraform-aws-modules/security-group/aws/main.tf:197-204
terraform-aws-modules/security-group/aws/main.tf:191-227
sg.tf:1-13

tfsec takes only the first occurrence in the file (terraform-aws-modules/security-group/aws/main.tf:197-204), the second is not used for the report

@simar7
Copy link
Member

simar7 commented Jul 12, 2023

@simar7 For example, if the metadata of the resource will refer to the following sources:

terraform-aws-modules/security-group/aws/main.tf:197-204
terraform-aws-modules/security-group/aws/main.tf:191-227
sg.tf:1-13

tfsec takes only the first occurrence in the file (terraform-aws-modules/security-group/aws/main.tf:197-204), the second is not used for the report

Are they unique misconfigurations? If so, we should report them all at once.

@nikpivkin
Copy link
Collaborator Author

@simar7 No, it refers to one misconfiguration.

@simar7
Copy link
Member

simar7 commented Jul 13, 2023

@simar7 No, it refers to one misconfiguration.

Got it. So in the table output we will still see one misconfiguration right?

If so, we should include all occurrences that are related to one misconfiguration.

@nikpivkin nikpivkin marked this pull request as ready for review July 14, 2023 07:16
@nikpivkin nikpivkin requested review from giorod3 and simar7 as code owners July 14, 2023 07:16
@simar7 simar7 force-pushed the nik-report-resource branch from 625d845 to afc8815 Compare July 16, 2023 08:07
Copy link
Member

@simar7 simar7 left a comment

Choose a reason for hiding this comment

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

just left one comment around testing this change

Comment on lines +347 to +366
func (r *Result) Occurrences() []Occurrence {
var occurrences []Occurrence

mod := &r.metadata

for {
mod = mod.Parent()
if mod == nil {
break
}
parentRange := mod.Range()
occurrences = append(occurrences, Occurrence{
Resource: mod.Reference(),
Filename: parentRange.GetFilename(),
StartLine: parentRange.GetStartLine(),
EndLine: parentRange.GetEndLine(),
})
}
return occurrences
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test for this? We can probably add it in pkg/scanners/cloud/aws/scanner_test.go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@simar7 How good is this place for the test? this file refers to tests of aws services. Since the method is public and converts the result to another structure, maybe it's worth adding it to pkg/scan/result_test.go?

@nikpivkin nikpivkin requested a review from simar7 July 17, 2023 12:33
@nikpivkin
Copy link
Collaborator Author

@simar7 Do you think this will close this issue?

@simar7
Copy link
Member

simar7 commented Jul 18, 2023

@simar7 Do you think this will close this issue?

Yeah good catch. I think so, it satisfies the ask.

@simar7 simar7 force-pushed the nik-report-resource branch from f51eb51 to 49aee1f Compare July 20, 2023 08:19
@simar7 simar7 force-pushed the nik-report-resource branch from 49aee1f to bd06b9a Compare July 20, 2023 11:44
@simar7 simar7 enabled auto-merge (squash) July 20, 2023 12:01
@simar7 simar7 merged commit a6686fe into aquasecurity:master Jul 20, 2023
@nikpivkin nikpivkin deleted the nik-report-resource branch July 20, 2023 16:10
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.

2 participants