Skip to content

Adding support for new resource types in check-no-public-access api.#49

Merged
mponaws merged 1 commit intoawslabs:mainfrom
hbendapu:main
May 21, 2025
Merged

Adding support for new resource types in check-no-public-access api.#49
mponaws merged 1 commit intoawslabs:mainfrom
hbendapu:main

Conversation

@hbendapu
Copy link
Copy Markdown
Contributor

Description of changes:
Adding support for the following resource types in check-no-public-access api.

  1. AWS::S3Tables::TableBucket
  2. AWS::ApiGateway::RestApi
  3. AWS::CodeArtifact::Domain
  4. AWS::Backup::BackupVault
  5. AWS::CloudTrail::Dashboard
  6. AWS::CloudTrail::EventDataStore
  7. AWS::S3Express::AccessPoint

Changes made include the following -

  1. For each resource type -
    a. Add a parser to extract the policy from the cloud formation template.
    b. Add logic to evaluate supported intrinsic functions (Ref, GetAttr)
    c. Add tests under tests/parser_tests/resource_tests
  2. Update the README file.
  3. Bump up the version.
  4. Minor changes to test_s3_multi_region_access_point_validator.py to fix a failing test.
  5. Update test_files/public_access_test.yml to add sample policies for the new resource types and update test_cli.py accordingly.

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

@hbendapu hbendapu marked this pull request as ready for review May 19, 2025 23:30
Comment thread README.md
| AWS::SNS::TopicPolicy | x | | x |
| AWS::SecretsManager::ResourcePolicy | x | | x |
| AWS::IAM::Role (trust policy) | x | x | x |
| AWS::S3Tables::TableBucket | | | x |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

x means supported?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes

from cfn_policy_validator import client
from cfn_policy_validator.application_error import ApplicationError

def get_dashboard_created_time(region, resource_name, resource=None):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Q: why resource as parameter if it's anyway never going to be used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the method call in cfn_policy_validator/parsers/utils/intrinsic_functions/fn_get_att_evaluator.py is from a map of resource types to appropriate method. For one of them resource is needed. Hence, ended up adding to all.



def get_dashboard_attribute(region, resource_name, attribute):
supported_attributes = ['Type', 'CreatedTimestamp', 'Status', 'UpdatedTimestamp']
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's define it as an enum, instead of copy-pasting the literal string values.


def parse(self, _, resource):
evaluated_resource = resource.eval(rest_api_policy_schema)
properties = evaluated_resource['Properties']
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

probably in a separate PR, but i think we will be benefited from integrating with https://github.com/keleshev/schema, rather than defining our own json based schema here

name = properties['Name']

policy = Policy('Policy', policy_document)
resource = Resource(name, 'AWS::ApiGateway::RestApi', policy)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shall we have a enum for all the different type of resource types like AWS::ApiGateway::RestApi?
it can be in a separate CR

'AWS::CloudFront::CloudFrontOriginAccessIdentity': {
'S3CanonicalUserId': get_canonical_user
},
'AWS::ApiGateway::RestApi': {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would love to have these as enums, or at least global variables


# Resolution of the rest api id.
def get_rest_api_id(region, rest_api_name, resource=None):
if rest_api_name in api_cache:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: this cache-validation logic can be moved to get_attributes method

@hbendapu
Copy link
Copy Markdown
Contributor Author

Will take care of the following changes in a separate PR

  1. Use enums where possible.
  2. Add tests for scenarios where the GetAttr can fail if the resource doesn't exist.

@mponaws mponaws merged commit b4958f2 into awslabs:main May 21, 2025
10 checks passed
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.

3 participants