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

fix(query): fix false positive for rds backup_retention_period not set #5902

Merged
merged 1 commit into from
Oct 6, 2022
Merged

fix(query): fix false positive for rds backup_retention_period not set #5902

merged 1 commit into from
Oct 6, 2022

Conversation

patrickpichler
Copy link
Contributor

Since the there is a default value for backup_retention_period, the query reported a false positive if the field was not set. This has been fixed.

Closes #5882

Proposed Changes

  • Ignore missing backup_retention_period value for rds_instances

I submit this contribution under the Apache-2.0 license.

@rafaela-soares rafaela-soares added query New query feature community Community contribution hacktoberfest labels Oct 3, 2022
cxMiguelSilva
cxMiguelSilva requested changes Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@cxMiguelSilva cxMiguelSilva left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @patrickpichler
Thank you so much for your amazing contribution!
I would like only to request one change:

  • in positive_expected_result.json change the fileName, on line 12, from positive3.tf to positive2.tf

The query is failing in unit tests due to being expected a result in positive3.tf that does not exist

rafaela-soares
rafaela-soares suggested changes Oct 4, 2022
View reviewed changes
assets/queries/terraform/aws/rds_with_backup_disabled/query.rego
@@ -3,45 +3,6 @@ package Cx
import data.generic.common as common_lib
import data.generic.terraform as tf_lib

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello again, @patrickpichler! Thank you for another contribution!

In addition to Miguel's suggestion, do you mind also updating the Ansible query, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've adjusted the Ansible query. I will squash commits later into one, when everything is working 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you so much 🚀

@patrickpichler
Copy link
Contributor Author

@cxMiguelSilva the tests are still failing with

=== CONT  Test_E2E_CLI/should_load_descriptions_from_a_custom_server_[E2E-CLI-052]_0
    json.go:72:
                Error Trace:    /home/patrick/development/projects/kics/e2e/json.go:72
                                                        /home/patrick/development/projects/kics/e2e/json.go:46
                                                        /home/patrick/development/projects/kics/e2e/cli_test.go:147
                                                        /home/patrick/development/projects/kics/e2e/cli_test.go:79
                Error:          Should be true
                Test:           Test_E2E_CLI/should_load_descriptions_from_a_custom_server_[E2E-CLI-052]_0
                Messages:       Schema Validation Failed
                                Schema: 'file:///home/patrick/development/projects/kics/e2e/fixtures/schemas/resultCIS.json'
                                Actual File: 'file:///home/patrick/development/projects/kics/e2e/output/E2E_CLI_052_RESULTS_ALL_HAVE_CIS.json'
                                Failed validations:
                                - queries.0: cis_description_id is required
                                - queries.0: cis_description_title is required
                                - queries.0: cis_description_text is required
                                - queries.1: cis_description_id is required
                                - queries.1: cis_description_title is required
                                - queries.1: cis_description_text is required
                                - (root): Must validate all the schemas (allOf)

Do you have any idea?

@cxMiguelSilva
Copy link
Collaborator

@cxMiguelSilva the tests are still failing with

=== CONT  Test_E2E_CLI/should_load_descriptions_from_a_custom_server_[E2E-CLI-052]_0
    json.go:72:
                Error Trace:    /home/patrick/development/projects/kics/e2e/json.go:72
                                                        /home/patrick/development/projects/kics/e2e/json.go:46
                                                        /home/patrick/development/projects/kics/e2e/cli_test.go:147
                                                        /home/patrick/development/projects/kics/e2e/cli_test.go:79
                Error:          Should be true
                Test:           Test_E2E_CLI/should_load_descriptions_from_a_custom_server_[E2E-CLI-052]_0
                Messages:       Schema Validation Failed
                                Schema: 'file:///home/patrick/development/projects/kics/e2e/fixtures/schemas/resultCIS.json'
                                Actual File: 'file:///home/patrick/development/projects/kics/e2e/output/E2E_CLI_052_RESULTS_ALL_HAVE_CIS.json'
                                Failed validations:
                                - queries.0: cis_description_id is required
                                - queries.0: cis_description_title is required
                                - queries.0: cis_description_text is required
                                - queries.1: cis_description_id is required
                                - queries.1: cis_description_title is required
                                - queries.1: cis_description_text is required
                                - (root): Must validate all the schemas (allOf)

Do you have any idea?

Hi @patrickpichler, to run E2E tests and pass all tests you should run the npm mock server to correctly test the CIS Descriptions. You can find the docs here

rafaela-soares
rafaela-soares previously approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@rafaela-soares rafaela-soares left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@patrickpichler
Copy link
Contributor Author

Thansk @cxMiguelSilva 🤦
I totally missed that in the docs 😅

fix(query): fix false positive for rds backup_retention_period not set
c444d90 
Since the there is a default value for `backup_retention_period`, the
query reported a false positive if the field was not set. This has been
fixed.

Closes Checkmarx#5882
@patrickpichler patrickpichler force-pushed the 5882-fix_rds_backup_false_positive branch from 1083aa9 to c444d90 Compare October 4, 2022 10:05
cxMiguelSilva
cxMiguelSilva approved these changes Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@cxMiguelSilva cxMiguelSilva left a comment

Choose a reason for hiding this comment

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

LGTM

@rafaela-soares rafaela-soares merged commit c8832d4 into Checkmarx:master Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cxMiguelSilva cxMiguelSilva cxMiguelSilva approved these changes

@rafaela-soares rafaela-soares rafaela-soares left review comments

Assignees
No one assigned
Labels
community Community contribution hacktoberfest query New query feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RDS With Backup Disabled Terraform AWS Security Query False Positive Result
3 participants
@patrickpichler @cxMiguelSilva @rafaela-soares