Skip to content

Conversation

@Barneyjm
Copy link
Contributor

@Barneyjm Barneyjm commented Nov 8, 2023

Description of changes:
Certain suggestions, warnings, etc might be considered "blocking" internally whereas Access Analyzer only considers them suggestions.

I don't want to block on all SUGGESTION findings but there are some SUGGESTION codes that I do want to block on, specifically: SUGGESTION.ALLOW_WITH_UNSUPPORTED_TAG_CONDITION_KEY_FOR_SERVICE

Many teams try to build policies with condition tags that don't work with the service and do something like this pseudo policy:

Action: kinesis:*
Resource: *
Condition: Tags Must Include = 123456

This gets flagged as a suggestion but actually grants full Kinesis:* to the policy, despite the user wanting to scope permissions down.

By blocking on this specific suggestion code, the user is protected from over permissive policy actions that they didn't intend on granting themselves.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Barneyjm
Copy link
Contributor Author

@hanboooli Would you be able to review this?

@ninaw3
Copy link
Contributor

ninaw3 commented Mar 28, 2024

Hi James, I am Nina from the team maintaining this repo. Thanks for your contribution. It might not make much sense to reuse --treat-finding-type-as-blocking. Can we instead create a separate flag --treat-finding-code-as-blocking? Finding codes are unique so we wouldn’t need to do something like "SUGGESTION.FINDING_CODE"

Copy link
Contributor Author

@Barneyjm Barneyjm left a comment

Choose a reason for hiding this comment

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

@ninaw3 added the requested flag and additional test. please review when you get a moment. Thank you!

"""

def __init__(self, findings_to_ignore, finding_types_that_are_blocking, allowed_external_principals):
def __init__(self, findings_to_ignore, finding_types_that_are_blocking, allowed_external_principals, finding_codes_that_are_blocking=default_finding_codes_that_are_blocking):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

open to not doing a kwarg here but don't want to break existing implementations before a larger release

Comment on lines +68 to +70
# for finding_code in finding_codes:
# if finding_code not in ['ERROR', 'SECURITY_WARNING', 'SUGGESTION', 'WARNING', 'NONE']:
# raise ArgumentTypeError(f"Invalid finding code: {finding_code}.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there any way to pull all possible finding codes? otherwise validation is just "does it split"

}
}

provider "aws" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tf 1.5.7 was complaining about not having a provider configured when i ran this with the new conditions below

Resource = "*",
Condition = {
"StringEquals" = {
"aws:ResourceTag/environment" = "production"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't have an impact on existing tests but does test the new code capability

check = Validator("123456789012", "us-west-2", "aws")
check.run(plan)
test_findings = _load_json_file(f"test/multiple_policies/identity_codes.json")
report = Reporter(None, [], None, ['ALLOW_WITH_UNSUPPORTED_TAG_CONDITION_KEY_FOR_SERVICE']).build_report_from(check.findings).to_json()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was pretty difficult to nail down the exact message. not sure how precise we need it but this allows us to validate that the FINDING CODE does indeed get added to the BreakingFindings

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