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.

Sign up for GitHub

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

Jump to bottom

feat(misconf): Add compliance check support #3130

Merged
merged 13 commits into from
Dec 7, 2022
Merged

feat(misconf): Add compliance check support #3130

merged 13 commits into from
Dec 7, 2022

Conversation

simar7
Copy link
Member

@simar7 simar7 commented Nov 4, 2022 •
edited by knqyf263
Loading

Signed-off-by: Simar simar@linux.com

Description

This adds the --compliance check flag to the misconfiguration scanning. It works as follows:

$ trivy aws  --compliance=awscis1.2

You can also pass in custom reports such as:

$ trivy aws --compliance=@/path/to/custom/report.yaml

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)

@simar7 simar7 self-assigned this Nov 4, 2022
@simar7 simar7 changed the title feat(aws): Add compliance check support feat(misconf): Add compliance check support Nov 4, 2022
@simar7 simar7 force-pushed the aws-compliance-yaml branch 4 times, most recently from 00fd5d1 to daea72b Compare November 21, 2022 23:33
@simar7 simar7 marked this pull request as ready for review November 21, 2022 23:55
@simar7 simar7 requested a review from knqyf263 as a code owner November 21, 2022 23:55
@knqyf263 knqyf263 requested a review from chen-keinan November 22, 2022 07:08
@chen-keinan chen-keinan mentioned this pull request Nov 22, 2022
Closed
@chen-keinan
Copy link
Contributor

@simar7 you are not using the compliance.BuildComplianceReport(scanResults, complianceSpec) functionality built for producing compliance reports ?

chen-keinan
chen-keinan reviewed Nov 22, 2022
View reviewed changes
pkg/flag/report_flags.go Outdated
@@ -89,7 +89,7 @@ var (
Name: "compliance",
ConfigName: "scan.compliance",
Value: "",
Usage: "compliance report to generate (nsa)",
Usage: "compliance report to generate (nsa, cis)",
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if there is a way to override it so it will not show cis for trivy k8s and nsa for trivy aws sub command as it is not supported and throw an error if used by users

Copy link
Member Author

Choose a reason for hiding this comment

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

We could leave the specs out of the help text to make it more generic.

Suggested change
Usage: "compliance report to generate (nsa, cis)",
Usage: "compliance report to generate",

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can override usage like 10ad755, but should the value cis? I thought it should be awscis1.2 and awscis1.4. Please correct me if I'm missing something.
BTW, even though we can override the accepted values, do we still need the aws prefix? I mean cis1.2 vs awscis1.2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@simar7 Please review my change. Also, do we support CIS 1.4 too? Don't we mention that in the doc? Looks like the current doc mentions CIS 1.2 only.

Copy link
Member Author

@simar7 simar7 Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Your change looks good, thanks I learnt something new!

I updated the docs to include 1.4 as well.

As for the prefixes, I think we should include them. CIS publishes many benchmarks and we should be clear that its the AWS version of the CIS benchmarks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As for the prefixes, I think we should include them. CIS publishes many benchmarks and we should be clear that its the AWS version of the CIS benchmarks.

Got it 👍

@simar7
Copy link
Member Author

simar7 commented Nov 22, 2022

@simar7 you are not using the compliance.BuildComplianceReport(scanResults, complianceSpec) functionality built for producing compliance reports ?

That’s right. As we discussed last time, it uses options.ScannerwithSpec in defsec to load only the checks that need to be loaded for a particular spec.

@simar7 simar7 force-pushed the aws-compliance-yaml branch from 6619acf to 5050795 Compare November 23, 2022 01:33
@chen-keinan
Copy link
Contributor

chen-keinan commented Nov 23, 2022 •
edited
Loading

@simar7 you are not using the compliance.BuildComplianceReport(scanResults, complianceSpec) functionality built for producing compliance reports ?

That’s right. As we discussed last time, it uses options.ScannerwithSpec in defsec to load only the checks that need to be loaded for a particular spec.

compliance.BuildComplianceReport is not just loading/filtering checks, it is also producing a specific compliance report view, it will be great to have all compliance report view to look a like.
example:
https://github.com/aquasecurity/trivy/blob/main/docs/docs/kubernetes/cli/compliance.md

@simar7
Copy link
Member Author

simar7 commented Nov 23, 2022

@simar7 you are not using the compliance.BuildComplianceReport(scanResults, complianceSpec) functionality built for producing compliance reports ?

That’s right. As we discussed last time, it uses options.ScannerwithSpec in defsec to load only the checks that need to be loaded for a particular spec.

compliance.BuildComplianceReport is not just loading/filtering checks, it is also producing a specific compliance report view, it will be great to have all compliance report view to look a like. example: https://github.com/aquasecurity/trivy/blob/main/docs/docs/kubernetes/cli/compliance.md

Good point - I didn't think of that. I added it here https://github.com/aquasecurity/trivy/pull/3130/files#diff-fe969e5428e7e462874a674a3116c74787735b8a6d25f113cd3c06653433eb38 along with some tests. PTAL.

@simar7 simar7 requested a review from chen-keinan November 23, 2022 23:57
@chen-keinan
Copy link
Contributor

lgtm

knqyf263
knqyf263 reviewed Dec 1, 2022
View reviewed changes
docs/docs/cloud/aws/compliance.md Outdated Show resolved Hide resolved
docs/docs/cloud/aws/compliance.md Show resolved Hide resolved
simar7 and others added 5 commits December 2, 2022 12:12
@simar7
feat(aws): Add compliance check support
7703260 
Addresses: #2919
Requires: aquasecurity/defsec#1045

Signed-off-by: Simar <simar@linux.com>
@simar7
add more docs
1e6eaaf 
Signed-off-by: Simar <simar@linux.com>
@simar7
add formatted compliance report
8a9ed8e 
Signed-off-by: Simar <simar@linux.com>
@simar7
make help text more generic
3dc1cbc
@simar7
please the linter-san
67e71dc 
Signed-off-by: Simar <simar@linux.com>
simar7 and others added 3 commits December 2, 2022 12:13
@simar7
simplify the logic to generate report
084ef0a 
Signed-off-by: Simar <simar@linux.com>
@knqyf263 @simar7
docs: override usage
dfc40b6
@simar7
export a defsec helper to simplify logic
35b2842 
Signed-off-by: Simar <simar@linux.com>
@simar7 simar7 force-pushed the aws-compliance-yaml branch from 5adfb90 to d9d268e Compare December 2, 2022 20:15
@simar7
update docs
c282023 
Signed-off-by: Simar <simar@linux.com>
@simar7 simar7 force-pushed the aws-compliance-yaml branch from d9d268e to 1b350a6 Compare December 3, 2022 02:25
@simar7
update tests
ef36a6a 
Signed-off-by: Simar <simar@linux.com>
@simar7 simar7 force-pushed the aws-compliance-yaml branch from 1b350a6 to ef36a6a Compare December 3, 2022 03:02
@knqyf263
Copy link
Collaborator

knqyf263 commented Dec 7, 2022

I believe you just forgot to remove the png, so I've removed it in 3c15cd0. Please let me know if I'm missing something.

@knqyf263 knqyf263 merged commit a3eece4 into main Dec 7, 2022
@knqyf263 knqyf263 deleted the aws-compliance-yaml branch December 7, 2022 20:43
@chen-keinan chen-keinan mentioned this pull request Dec 29, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chen-keinan chen-keinan chen-keinan approved these changes

@knqyf263 knqyf263 knqyf263 approved these changes

@AnaisUrlichs AnaisUrlichs Awaiting requested review from AnaisUrlichs

@itaysk itaysk Awaiting requested review from itaysk

Assignees

@simar7 simar7

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Support AWS 1.4 Automated CIS benchmarks
3 participants
@simar7 @chen-keinan @knqyf263