From 93d0414b0a44f01048e4d4fb8473f4459a9c7a36 Mon Sep 17 00:00:00 2001 From: Sam Platek Date: Tue, 4 Feb 2025 16:12:07 -0500 Subject: [PATCH 1/3] local SSL issues are preventing 8 tests from passing despite some troubleshooting; it doesn't like the self-signed certificates from json-schema.org Signed-off-by: Sam Platek --- .../data/managed-upload-infrastructure.yaml | 30 +++++++++-- src/rpdk/core/package.py | 1 + src/rpdk/core/project.py | 5 +- src/rpdk/core/submit.py | 12 +++++ src/rpdk/core/upload.py | 12 ++++- tests/test_package.py | 1 + tests/test_project.py | 30 ++++++++--- tests/test_upload.py | 50 ++++++++++++++++++- 8 files changed, 126 insertions(+), 15 deletions(-) diff --git a/src/rpdk/core/data/managed-upload-infrastructure.yaml b/src/rpdk/core/data/managed-upload-infrastructure.yaml index c1a7f71f..e4c59878 100644 --- a/src/rpdk/core/data/managed-upload-infrastructure.yaml +++ b/src/rpdk/core/data/managed-upload-infrastructure.yaml @@ -3,6 +3,21 @@ Description: > This CloudFormation template provisions all the infrastructure that is required to upload artifacts to CloudFormation's managed experience. +Parameters: + EnableKMSKeyForS3: + Type: String + Default: "true" + AllowedValues: + - "true" + - "false" + Description: > + Whether to enable a KMS key for S3 bucket encryption. + ConstraintDescription: > + Must be either "true" or "false". + +Conditions: + UseKMSKeyForS3: !Equals [!Ref EnableKMSKeyForS3, "true"] + Resources: ArtifactBucket: Type: AWS::S3::Bucket @@ -10,11 +25,15 @@ Resources: UpdateReplacePolicy: Delete Properties: AccessControl: BucketOwnerFullControl - BucketEncryption: - ServerSideEncryptionConfiguration: - - ServerSideEncryptionByDefault: - SSEAlgorithm: aws:kms - KMSMasterKeyID: !Ref EncryptionKey + BucketEncryption: !If + - UseKMSKeyForS3 + - ServerSideEncryptionConfiguration: + - ServerSideEncryptionByDefault: + SSEAlgorithm: aws:kms + KMSMasterKeyID: !Ref EncryptionKey + - ServerSideEncryptionConfiguration: + - ServerSideEncryptionByDefault: + SSEAlgorithm: AES256 LifecycleConfiguration: Rules: - Id: MultipartUploadLifecycleRule @@ -110,6 +129,7 @@ Resources: Type: AWS::KMS::Key DeletionPolicy: Retain UpdateReplacePolicy: Retain + Condition: UseKMSKeyForS3 Properties: Description: KMS key used to encrypt the resource type artifacts EnableKeyRotation: true diff --git a/src/rpdk/core/package.py b/src/rpdk/core/package.py index b82f7d69..98a06067 100644 --- a/src/rpdk/core/package.py +++ b/src/rpdk/core/package.py @@ -20,6 +20,7 @@ def package(_args): use_role=False, set_default=False, profile_name=False, + use_kms_key=True, ) diff --git a/src/rpdk/core/project.py b/src/rpdk/core/project.py index 10c48a40..bb65cbab 100644 --- a/src/rpdk/core/project.py +++ b/src/rpdk/core/project.py @@ -710,6 +710,7 @@ def submit( use_role, set_default, profile_name, + use_kms_key, ): # pylint: disable=too-many-arguments context_mgr = self._create_context_manager(dry_run) @@ -753,6 +754,7 @@ def submit( use_role, set_default, profile_name, + use_kms_key, ) def _add_overrides_file_to_zip(self, zip_file): @@ -1180,13 +1182,14 @@ def _upload( use_role, set_default, profile_name, + use_kms_key, ): # pylint: disable=too-many-arguments, too-many-locals LOG.debug("Packaging complete, uploading...") session = create_sdk_session(region_name, profile_name) LOG.debug("Uploading to region '%s'", session.region_name) cfn_client = session.client("cloudformation", endpoint_url=endpoint_url) s3_client = session.client("s3") - uploader = Uploader(cfn_client, s3_client) + uploader = Uploader(cfn_client, s3_client, use_kms_key) if use_role and not role_arn and "handlers" in self.schema: LOG.debug("Creating execution role for provider to use") diff --git a/src/rpdk/core/submit.py b/src/rpdk/core/submit.py index 8d6acefd..da0dd64f 100644 --- a/src/rpdk/core/submit.py +++ b/src/rpdk/core/submit.py @@ -16,6 +16,9 @@ def submit(args): if args.use_docker or args.no_docker: project.settings["use_docker"] = args.use_docker project.settings["no_docker"] = args.no_docker + use_kms_key = True + if args.no_kms_key: + use_kms_key = False project.submit( args.dry_run, args.endpoint_url, @@ -24,6 +27,7 @@ def submit(args): args.use_role, args.set_default, args.profile, + use_kms_key, ) @@ -43,6 +47,14 @@ def setup_subparser(subparsers, parents): help="If registration is successful, set submitted version to the default.", ) parser.add_argument("--profile", help="AWS profile to use.") + parser.add_argument( + "--no-kms-key", + action="store_true", + help=( + "Use the default Server Side Encryption algorithm for the S3 Bucket." + "Does not create a KMS key or removes the KMS key from the management of the stack if it has already been created" + ), + ) role_group = parser.add_mutually_exclusive_group() role_group.add_argument( "--role-arn", diff --git a/src/rpdk/core/upload.py b/src/rpdk/core/upload.py index 546a0cde..b9b687d0 100644 --- a/src/rpdk/core/upload.py +++ b/src/rpdk/core/upload.py @@ -15,11 +15,12 @@ class Uploader: - def __init__(self, cfn_client, s3_client): + def __init__(self, cfn_client, s3_client, use_kms_key): self.cfn_client = cfn_client self.s3_client = s3_client self.bucket_name = "" self.log_delivery_role_arn = "" + self.use_kms_key = use_kms_key @staticmethod def _get_template(): @@ -85,6 +86,15 @@ def _get_stack_output(self, stack_id, output_key): def _create_or_update_stack(self, template, stack_name): args = {"StackName": stack_name, "TemplateBody": template} + if stack_name == INFRA_STACK_NAME: + args |= { + "Parameters": [ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": self.use_kms_key, + } + ] + } # attempt to create stack. if the stack already exists, try to update it LOG.info("Creating %s", stack_name) try: diff --git a/tests/test_package.py b/tests/test_package.py index c75c151c..27b09977 100644 --- a/tests/test_package.py +++ b/tests/test_package.py @@ -19,4 +19,5 @@ def test_package_command_valid_schema(): use_role=False, set_default=False, profile_name=False, + use_kms_key=True, ) diff --git a/tests/test_project.py b/tests/test_project.py index 80d87510..f2ea057f 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -1439,7 +1439,8 @@ def test_submit_dry_run(project, is_type_configuration_available): role_arn=None, use_role=True, set_default=False, - profile_name=PROFILE + profile_name=PROFILE, + use_kms_key=True, ) # fmt: on @@ -1540,7 +1541,8 @@ def test_submit_dry_run_modules(project): role_arn=None, use_role=True, set_default=False, - profile_name=PROFILE + profile_name=PROFILE, + use_kms_key=True, ) # fmt: on @@ -1606,7 +1608,8 @@ def test_submit_dry_run_hooks(project): role_arn=None, use_role=True, set_default=False, - profile_name=PROFILE + profile_name=PROFILE, + use_kms_key=True, ) # fmt: on @@ -1732,7 +1735,8 @@ def test_submit_dry_run_hooks_with_target_info(project, session): role_arn=None, use_role=True, set_default=False, - profile_name=PROFILE + profile_name=PROFILE, + use_kms_key=True, ) # fmt: on @@ -1811,7 +1815,8 @@ def test_submit_live_run(project): role_arn=None, use_role=True, set_default=True, - profile_name=PROFILE + profile_name=PROFILE, + use_kms_key=True, ) # fmt: on @@ -1830,6 +1835,7 @@ def test_submit_live_run(project): use_role=True, set_default=True, profile_name=PROFILE, + use_kms_key=True, ) assert temp_file._was_closed @@ -1865,7 +1871,8 @@ def test_submit_live_run_for_module(project): role_arn=None, use_role=True, set_default=True, - profile_name=PROFILE + profile_name=PROFILE, + use_kms_key=True, ) # fmt: on @@ -1908,7 +1915,8 @@ def test_submit_live_run_for_hooks(project): role_arn=None, use_role=True, set_default=True, - profile_name=PROFILE + profile_name=PROFILE, + use_kms_key=True, ) # fmt: on @@ -1927,6 +1935,7 @@ def test_submit_live_run_for_hooks(project): use_role=True, set_default=True, profile_name=PROFILE, + use_kms_key=True, ) assert temp_file._was_closed @@ -1964,6 +1973,7 @@ def test__upload_good_path_create_role_and_set_default(project): use_role=True, set_default=True, profile_name=None, + use_kms_key=True, ) mock_sdk.assert_called_once_with(region_name=None, profile_name=None) @@ -2018,6 +2028,7 @@ def test__upload_good_path_create_role_and_set_default_hook(project): use_role=True, set_default=True, profile_name=None, + use_kms_key=True, ) mock_sdk.assert_called_once_with(region_name=None, profile_name=None) @@ -2075,6 +2086,7 @@ def test__upload_good_path_skip_role_creation( use_role=use_role, set_default=True, profile_name=None, + use_kms_key=True, ) mock_sdk.assert_called_once_with(region_name=None, profile_name=None) @@ -2130,6 +2142,7 @@ def test__upload_good_path_skip_role_creation_hook( use_role=use_role, set_default=True, profile_name=None, + use_kms_key=True, ) mock_sdk.assert_called_once_with(region_name=None, profile_name=None) @@ -2181,6 +2194,7 @@ def test__upload_clienterror(project): use_role=False, set_default=True, profile_name=None, + use_kms_key=True, ) mock_sdk.assert_called_once_with(region_name=None, profile_name=None) @@ -2229,6 +2243,7 @@ def test__upload_clienterror_module(project): use_role=False, set_default=True, profile_name=None, + use_kms_key=True, ) mock_sdk.assert_called_once_with(region_name=None, profile_name=None) @@ -2277,6 +2292,7 @@ def test__upload_clienterror_hook(project): use_role=False, set_default=True, profile_name=None, + use_kms_key=True, ) mock_sdk.assert_called_once_with(region_name=None, profile_name=None) diff --git a/tests/test_upload.py b/tests/test_upload.py index 19ed8c84..3324f48d 100644 --- a/tests/test_upload.py +++ b/tests/test_upload.py @@ -66,7 +66,7 @@ class AlreadyExistsException(Exception): @pytest.fixture def uploader(): - uploader = Uploader(Mock(), Mock()) + uploader = Uploader(Mock(), Mock(), True) uploader.cfn_client.exceptions.AlreadyExistsException = AlreadyExistsException return uploader @@ -206,6 +206,12 @@ def test__create_or_update_stack_stack_doesnt_exist(uploader): StackName=INFRA_STACK_NAME, TemplateBody=CONTENTS_UTF8, EnableTerminationProtection=True, + Parameters=[ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": True, + } + ], ) mock_wait.assert_called_once_with(STACK_ID, "stack_create_complete", ANY) @@ -231,11 +237,23 @@ class AlreadyExistsException(Exception): StackName=INFRA_STACK_NAME, TemplateBody=CONTENTS_UTF8, EnableTerminationProtection=True, + Parameters=[ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": True, + } + ], ) uploader.cfn_client.update_stack.assert_called_once_with( Capabilities=["CAPABILITY_IAM"], StackName=INFRA_STACK_NAME, TemplateBody=CONTENTS_UTF8, + Parameters=[ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": True, + } + ], ) mock_wait.assert_not_called() @@ -255,11 +273,23 @@ def test__create_or_update_stack_stack_exists_and_needs_changes(uploader): StackName=INFRA_STACK_NAME, TemplateBody=CONTENTS_UTF8, EnableTerminationProtection=True, + Parameters=[ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": True, + } + ], ) uploader.cfn_client.update_stack.assert_called_once_with( Capabilities=["CAPABILITY_IAM"], StackName=INFRA_STACK_NAME, TemplateBody=CONTENTS_UTF8, + Parameters=[ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": True, + } + ], ) mock_wait.assert_called_once_with(STACK_ID, "stack_update_complete", ANY) @@ -279,6 +309,12 @@ def test__create_or_update_stack_create_unknown_failure(uploader): StackName=INFRA_STACK_NAME, TemplateBody=CONTENTS_UTF8, EnableTerminationProtection=True, + Parameters=[ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": True, + } + ], ) assert uploader.get_log_delivery_role_arn() == "" @@ -301,11 +337,23 @@ def test__create_or_update_stack_update_unknown_failure(uploader): StackName=INFRA_STACK_NAME, TemplateBody=CONTENTS_UTF8, EnableTerminationProtection=True, + Parameters=[ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": True, + } + ], ) uploader.cfn_client.update_stack.assert_called_once_with( Capabilities=["CAPABILITY_IAM"], StackName=INFRA_STACK_NAME, TemplateBody=CONTENTS_UTF8, + Parameters=[ + { + "ParameterName": "EnableKMSKeyForS3", + "ParameterValue": True, + } + ], ) mock_wait.assert_not_called() From b02a7c59060ecbe36d59e2f172a10e418b95905a Mon Sep 17 00:00:00 2001 From: Sam Platek Date: Tue, 4 Feb 2025 19:16:16 -0500 Subject: [PATCH 2/3] fixing through field tests Signed-off-by: Sam Platek --- .../core/data/managed-upload-infrastructure.yaml | 10 +++++----- src/rpdk/core/submit.py | 2 +- src/rpdk/core/upload.py | 4 ++-- tests/test_upload.py | 16 ++++++++-------- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/rpdk/core/data/managed-upload-infrastructure.yaml b/src/rpdk/core/data/managed-upload-infrastructure.yaml index e4c59878..560810fc 100644 --- a/src/rpdk/core/data/managed-upload-infrastructure.yaml +++ b/src/rpdk/core/data/managed-upload-infrastructure.yaml @@ -6,17 +6,17 @@ Description: > Parameters: EnableKMSKeyForS3: Type: String - Default: "true" + Default: "True" AllowedValues: - - "true" - - "false" + - "True" + - "False" Description: > Whether to enable a KMS key for S3 bucket encryption. ConstraintDescription: > - Must be either "true" or "false". + Must be either "True" or "False". Conditions: - UseKMSKeyForS3: !Equals [!Ref EnableKMSKeyForS3, "true"] + UseKMSKeyForS3: !Equals [!Ref EnableKMSKeyForS3, "True"] Resources: ArtifactBucket: diff --git a/src/rpdk/core/submit.py b/src/rpdk/core/submit.py index da0dd64f..3aaea804 100644 --- a/src/rpdk/core/submit.py +++ b/src/rpdk/core/submit.py @@ -27,7 +27,7 @@ def submit(args): args.use_role, args.set_default, args.profile, - use_kms_key, + str(use_kms_key), ) diff --git a/src/rpdk/core/upload.py b/src/rpdk/core/upload.py index b9b687d0..35826a6a 100644 --- a/src/rpdk/core/upload.py +++ b/src/rpdk/core/upload.py @@ -15,7 +15,7 @@ class Uploader: - def __init__(self, cfn_client, s3_client, use_kms_key): + def __init__(self, cfn_client, s3_client, use_kms_key: str): self.cfn_client = cfn_client self.s3_client = s3_client self.bucket_name = "" @@ -90,7 +90,7 @@ def _create_or_update_stack(self, template, stack_name): args |= { "Parameters": [ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": self.use_kms_key, } ] diff --git a/tests/test_upload.py b/tests/test_upload.py index 3324f48d..89da6d57 100644 --- a/tests/test_upload.py +++ b/tests/test_upload.py @@ -208,7 +208,7 @@ def test__create_or_update_stack_stack_doesnt_exist(uploader): EnableTerminationProtection=True, Parameters=[ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": True, } ], @@ -239,7 +239,7 @@ class AlreadyExistsException(Exception): EnableTerminationProtection=True, Parameters=[ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": True, } ], @@ -250,7 +250,7 @@ class AlreadyExistsException(Exception): TemplateBody=CONTENTS_UTF8, Parameters=[ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": True, } ], @@ -275,7 +275,7 @@ def test__create_or_update_stack_stack_exists_and_needs_changes(uploader): EnableTerminationProtection=True, Parameters=[ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": True, } ], @@ -286,7 +286,7 @@ def test__create_or_update_stack_stack_exists_and_needs_changes(uploader): TemplateBody=CONTENTS_UTF8, Parameters=[ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": True, } ], @@ -311,7 +311,7 @@ def test__create_or_update_stack_create_unknown_failure(uploader): EnableTerminationProtection=True, Parameters=[ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": True, } ], @@ -339,7 +339,7 @@ def test__create_or_update_stack_update_unknown_failure(uploader): EnableTerminationProtection=True, Parameters=[ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": True, } ], @@ -350,7 +350,7 @@ def test__create_or_update_stack_update_unknown_failure(uploader): TemplateBody=CONTENTS_UTF8, Parameters=[ { - "ParameterName": "EnableKMSKeyForS3", + "ParameterKey": "EnableKMSKeyForS3", "ParameterValue": True, } ], From 70681cc237183f987f5eefcbe93c6ac2cf69a55d Mon Sep 17 00:00:00 2001 From: Sam Platek Date: Wed, 5 Feb 2025 11:03:51 -0500 Subject: [PATCH 3/3] adding env var for permanent set-and-forget settings Signed-off-by: Sam Platek --- src/rpdk/core/submit.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/rpdk/core/submit.py b/src/rpdk/core/submit.py index 3aaea804..2604b55c 100644 --- a/src/rpdk/core/submit.py +++ b/src/rpdk/core/submit.py @@ -3,6 +3,7 @@ Projects can be created via the 'init' sub command. """ import logging +import os from .project import Project @@ -17,7 +18,7 @@ def submit(args): project.settings["use_docker"] = args.use_docker project.settings["no_docker"] = args.no_docker use_kms_key = True - if args.no_kms_key: + if args.no_kms_key or os.getenv("CFN_CLI_NO_KMS_KEY"): use_kms_key = False project.submit( args.dry_run, @@ -52,7 +53,8 @@ def setup_subparser(subparsers, parents): action="store_true", help=( "Use the default Server Side Encryption algorithm for the S3 Bucket." - "Does not create a KMS key or removes the KMS key from the management of the stack if it has already been created" + "Does not create a KMS key or removes the KMS key from the management of the stack if it has already been created." + "Alternatively, the environment variable CFN_CLI_NO_KMS_KEY can be set to any truthy value." ), ) role_group = parser.add_mutually_exclusive_group()