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): Added base class for cloudsplaining iam checks to be integrated between data and resource objects #4338

Merged
merged 16 commits into from
Jan 30, 2023

Conversation

bo156
Copy link
Contributor

@bo156 bo156 commented Jan 26, 2023

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

Added a base class for handling clousplaining checks, to allow future support of both resource and data checks without code duplication.

Fixes # (issue)

New/Edited policies (Delete if not relevant)

Description

Include a description of what makes it a violation and any relevant external links.

Fix

How does someone fix the issue in code and/or in runtime?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@bo156 bo156 force-pushed the feature/bce-12485-create-base-cloudsplaining branch from 7bbf027 to a8e39e8 Compare January 26, 2023 16:44
@bo156 bo156 force-pushed the feature/bce-12485-create-base-cloudsplaining branch from 293248a to 476ac4b Compare January 29, 2023 07:57
@bo156 bo156 force-pushed the feature/bce-12485-create-base-cloudsplaining branch from 476ac4b to 4fff87d Compare January 29, 2023 08:02
@bo156 bo156 force-pushed the feature/bce-12485-create-base-cloudsplaining branch from d8b3e88 to 94431a7 Compare January 29, 2023 11:08
Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

in general, good job 💪

)


class BaseTerraformCloudsplainingDataIAMCheck(BaseDataCheck, BaseTerraformCloudsplainingIAMScanner, ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure, why you subclass ABC here? there are no abstract annotations used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we also need to implement cloudformation_analysis which is only in the check itself

from checkov.terraform.checks.utils.base_cloudsplaining_iam_scanner import BaseTerraformCloudsplainingIAMScanner


class BaseTerraformCloudsplaininResourceIAMCheck(BaseResourceCheck, BaseTerraformCloudsplainingIAMScanner, ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same


from checkov.common.models.enums import CheckResult

from cloudsplaining.scan.policy_document import PolicyDocument # noqa: TC002
Copy link
Contributor

Choose a reason for hiding this comment

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

you can actually move it under TYPE_CHECKING, but then you also have to import annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, fixing now

@bo156 bo156 merged commit 8417844 into main Jan 30, 2023
@bo156 bo156 deleted the feature/bce-12485-create-base-cloudsplaining branch January 30, 2023 14:59
andyloughran pushed a commit to andyloughran/checkov that referenced this pull request Feb 10, 2023
… integrated between data and resource objects (bridgecrewio#4338)

* Created abstraction for cloudsplaining to be used between resource and data in terraform

* Added base resource check class for cloudsplaining

* Removed usage of resource and data specific scanners

* returned setup to normal

* flake8

* fixed import

* remove abc usage

* fixed double class error in tests

* Renamed classes and cache to use terraform to avoid similarities with cache in cloudformation

* Renamed file

* Returned abc usa\ge

* ignore linting error as without it performence tests fail

* Returned the cache name to be without terraform

* Fixed parsing of policy in baseresouce check

* fixed flake with import future annotations

* Used correct class to initialize cache and fix the test
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.

None yet

4 participants