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(terraform/gcp): Add GKE Shielded Nodes is Disabled query for Terraform. #6248

Merged

Conversation

bbergstrom
Copy link

@bbergstrom bbergstrom commented Mar 23, 2023

Add Terraform query for "GKE Shielded Nodes is Disabled".

Closes #6243

I submit this contribution under the Apache-2.0 license.

@bbergstrom bbergstrom changed the title Add GKE Shielded Nodes is Disabled query for Terraform. feat(terraform/gcp): Add GKE Shielded Nodes is Disabled query for Terraform. Mar 23, 2023
@cxMiguelSilva
Copy link
Collaborator

Hi @bbergstrom,
Thank you for this amazing contribution!
Regarding the proposed changes. The query logic LGTM.
I will just ask our Application Security to validate the metadata and should be good to go!

@github-advanced-security
Copy link

You have successfully added a new gosec configuration .github/workflows/go-ci.yml:security-scan. As part of the setup process, we have scanned this repository and found 19 existing alerts. Please check the repository Security tab to see all alerts.

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.

Upon further inspection, I noticed that the terraform docs suggest that by default the value is set to true. I believe the first policy that detects the default value should be removed.

Let me know what you think.

@kaplanlior kaplanlior added the community Community contribution label Mar 28, 2023
@bbergstrom
Copy link
Author

@cxMiguelSilva The first query detects for the implicit default, which should be a positive because the value could drift if it is changed outside of Terraform. The best practice for these is to explicitly set the default value in Terraform to ensure it catches and corrects drift to the secure value.

@bbergstrom bbergstrom requested a review from cxMiguelSilva April 6, 2023 21:03
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.

Hi @bbergstrom,
In KICS we do not create policies to cover default values that are not deemed as vulnerable or misconfiguration. We have come across bug reports and issues handling from the community to improve our Security Queries in order to remove those False Positive results from incorrect checks on default values.
I kindly ask you to remove the policy that checks the default value since the Terraform documentation is indicative that it is set to the correct value. If at any time the guidelines change feel free to update the Security Query and add that policy.

@kaplanlior
Copy link
Contributor

Hi @bbergstrom ,

Would you have time in the near future to do the changes Miguel asked for?

@bbergstrom
Copy link
Author

@kaplanlior Pushed suggested changes. Awaiting approval for test suite.

@bbergstrom
Copy link
Author

@kaplanlior @cxMiguelSilva Could I please get approval on the latest commit to run the test suite?

@bbergstrom bbergstrom requested a review from cxMiguelSilva May 23, 2023 16:00
@bbergstrom
Copy link
Author

@gabriel-cx Tests all passed. 🎉 Would you finish the review or should I wait for @cxMiguelSilva ?

@kaplanlior kaplanlior force-pushed the feature/6243-gke-shielded-nodes branch from cce5b84 to 9ac56ee Compare May 28, 2023 05:58
@gabriel-cx gabriel-cx merged commit 293e3cc into Checkmarx:master May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GKE Cluster Shielded Nodes is Disabled query for Terraform
4 participants