From 35001eba0999e6a989066b165b09b219c42f7ed0 Mon Sep 17 00:00:00 2001 From: amzn-hdoke <61443753+hdoke@users.noreply.github.com> Date: Tue, 22 Jun 2021 16:31:46 -0700 Subject: [PATCH] Fix resource mappings file names and client auth warnings and asserts. Fix cdk permissions (#1487) * Fix resource mappings file names and client auth warnings and asserts conditions * Add comments to explain reasoning for client auth role permissions * Update comments based on feedback * Fix AWSClientAuth unit test --- ...e_mappings.json => default_aws_resource_mappings.json} | 0 .../Windows/aws_metrics/aws_metrics_automation_test.py | 2 +- .../AWS/Windows/client_auth/test_anonymous_credentials.py | 2 +- .../AWS/Windows/client_auth/test_password_signin.py | 2 +- AutomatedTesting/Registry/awscoreconfiguration.setreg | 2 +- .../Authentication/AWSCognitoAuthenticationProvider.cpp | 4 ++-- .../Authentication/AuthenticationProviderManager.cpp | 4 ++-- .../Authentication/GoogleAuthenticationProvider.cpp | 8 ++++---- .../Source/Authentication/LWAAuthenticationProvider.cpp | 8 ++++---- .../Authorization/AWSCognitoAuthorizationController.cpp | 4 ++-- .../AuthenticationProviderManagerScriptCanvasBusTest.cpp | 2 +- .../Authentication/AuthenticationProviderManagerTest.cpp | 2 +- Gems/AWSClientAuth/cdk/README.md | 7 +++++++ Gems/AWSClientAuth/cdk/auth/cognito_identity_pool_role.py | 7 +++++-- Gems/AWSClientAuth/cdk/auth/cognito_user_pool_sms_role.py | 8 +++++++- 15 files changed, 39 insertions(+), 23 deletions(-) rename AutomatedTesting/Config/{aws_resource_mappings.json => default_aws_resource_mappings.json} (100%) diff --git a/AutomatedTesting/Config/aws_resource_mappings.json b/AutomatedTesting/Config/default_aws_resource_mappings.json similarity index 100% rename from AutomatedTesting/Config/aws_resource_mappings.json rename to AutomatedTesting/Config/default_aws_resource_mappings.json diff --git a/AutomatedTesting/Gem/PythonTests/AWS/Windows/aws_metrics/aws_metrics_automation_test.py b/AutomatedTesting/Gem/PythonTests/AWS/Windows/aws_metrics/aws_metrics_automation_test.py index 511d9b3ecd..d8fd8b145a 100644 --- a/AutomatedTesting/Gem/PythonTests/AWS/Windows/aws_metrics/aws_metrics_automation_test.py +++ b/AutomatedTesting/Gem/PythonTests/AWS/Windows/aws_metrics/aws_metrics_automation_test.py @@ -112,7 +112,7 @@ def remove_file(file_path: str) -> None: @pytest.mark.parametrize('project', ['AutomatedTesting']) @pytest.mark.parametrize('level', ['AWS/Metrics']) @pytest.mark.parametrize('feature_name', [AWS_METRICS_FEATURE_NAME]) -@pytest.mark.parametrize('resource_mappings_filename', ['aws_resource_mappings.json']) +@pytest.mark.parametrize('resource_mappings_filename', ['default_aws_resource_mappings.json']) @pytest.mark.parametrize('profile_name', ['AWSAutomationTest']) @pytest.mark.parametrize('region_name', ['us-west-2']) @pytest.mark.parametrize('assume_role_arn', ['arn:aws:iam::645075835648:role/o3de-automation-tests']) diff --git a/AutomatedTesting/Gem/PythonTests/AWS/Windows/client_auth/test_anonymous_credentials.py b/AutomatedTesting/Gem/PythonTests/AWS/Windows/client_auth/test_anonymous_credentials.py index f8aa5b85eb..a477e89821 100644 --- a/AutomatedTesting/Gem/PythonTests/AWS/Windows/client_auth/test_anonymous_credentials.py +++ b/AutomatedTesting/Gem/PythonTests/AWS/Windows/client_auth/test_anonymous_credentials.py @@ -36,7 +36,7 @@ logger = logging.getLogger(__name__) @pytest.mark.usefixtures('cdk') @pytest.mark.parametrize('feature_name', [AWS_CLIENT_AUTH_FEATURE_NAME]) @pytest.mark.usefixtures('resource_mappings') -@pytest.mark.parametrize('resource_mappings_filename', ['aws_resource_mappings.json']) +@pytest.mark.parametrize('resource_mappings_filename', ['default_aws_resource_mappings.json']) @pytest.mark.usefixtures('aws_utils') @pytest.mark.parametrize('region_name', ['us-west-2']) @pytest.mark.parametrize('assume_role_arn', ['arn:aws:iam::645075835648:role/o3de-automation-tests']) diff --git a/AutomatedTesting/Gem/PythonTests/AWS/Windows/client_auth/test_password_signin.py b/AutomatedTesting/Gem/PythonTests/AWS/Windows/client_auth/test_password_signin.py index 28b17fdeee..a0bcf56b0d 100644 --- a/AutomatedTesting/Gem/PythonTests/AWS/Windows/client_auth/test_password_signin.py +++ b/AutomatedTesting/Gem/PythonTests/AWS/Windows/client_auth/test_password_signin.py @@ -35,7 +35,7 @@ logger = logging.getLogger(__name__) @pytest.mark.usefixtures('cdk') @pytest.mark.parametrize('feature_name', [AWS_CLIENT_AUTH_FEATURE_NAME]) @pytest.mark.usefixtures('resource_mappings') -@pytest.mark.parametrize('resource_mappings_filename', ['aws_resource_mappings.json']) +@pytest.mark.parametrize('resource_mappings_filename', ['default_aws_resource_mappings.json']) @pytest.mark.usefixtures('aws_utils') @pytest.mark.parametrize('region_name', ['us-west-2']) @pytest.mark.parametrize('assume_role_arn', ['arn:aws:iam::645075835648:role/o3de-automation-tests']) diff --git a/AutomatedTesting/Registry/awscoreconfiguration.setreg b/AutomatedTesting/Registry/awscoreconfiguration.setreg index b7c60b0fb9..1927909328 100644 --- a/AutomatedTesting/Registry/awscoreconfiguration.setreg +++ b/AutomatedTesting/Registry/awscoreconfiguration.setreg @@ -4,7 +4,7 @@ "AWSCore": { "ProfileName": "AWSAutomationTest", - "ResourceMappingConfigFileName": "aws_resource_mappings.json" + "ResourceMappingConfigFileName": "default_aws_resource_mappings.json" } } } \ No newline at end of file diff --git a/Gems/AWSClientAuth/Code/Source/Authentication/AWSCognitoAuthenticationProvider.cpp b/Gems/AWSClientAuth/Code/Source/Authentication/AWSCognitoAuthenticationProvider.cpp index 74865c0044..20f5bf68cc 100644 --- a/Gems/AWSClientAuth/Code/Source/Authentication/AWSCognitoAuthenticationProvider.cpp +++ b/Gems/AWSClientAuth/Code/Source/Authentication/AWSCognitoAuthenticationProvider.cpp @@ -146,12 +146,12 @@ namespace AWSClientAuth void AWSCognitoAuthenticationProvider::DeviceCodeGrantSignInAsync() { - AZ_Assert(true, "Not supported"); + AZ_Assert(false, "Not supported"); } void AWSCognitoAuthenticationProvider::DeviceCodeGrantConfirmSignInAsync() { - AZ_Assert(true, "Not supported"); + AZ_Assert(false, "Not supported"); } void AWSCognitoAuthenticationProvider::RefreshTokensAsync() diff --git a/Gems/AWSClientAuth/Code/Source/Authentication/AuthenticationProviderManager.cpp b/Gems/AWSClientAuth/Code/Source/Authentication/AuthenticationProviderManager.cpp index d4d2d0d67b..fe8c526a11 100644 --- a/Gems/AWSClientAuth/Code/Source/Authentication/AuthenticationProviderManager.cpp +++ b/Gems/AWSClientAuth/Code/Source/Authentication/AuthenticationProviderManager.cpp @@ -53,7 +53,7 @@ namespace AWSClientAuth if (!m_settingsRegistry->MergeSettingsFile(resolvedPath.data(), AZ::SettingsRegistryInterface::Format::JsonMergePatch)) { - AZ_Error("AuthenticationProviderManager", true, "Error merging settings registry for path: %s", resolvedPath.data()); + AZ_Error("AuthenticationProviderManager", false, "Error merging settings registry for path: %s", resolvedPath.data()); return false; } @@ -199,7 +199,7 @@ namespace AWSClientAuth { return enumValue.value(); } - AZ_Warning("AuthenticationProviderManager", true, "Incorrect string value for enum: %s", name.c_str()); + AZ_Warning("AuthenticationProviderManager", false, "Incorrect string value for enum: %s", name.c_str()); return ProviderNameEnum::None; } diff --git a/Gems/AWSClientAuth/Code/Source/Authentication/GoogleAuthenticationProvider.cpp b/Gems/AWSClientAuth/Code/Source/Authentication/GoogleAuthenticationProvider.cpp index 7762b3919b..efabaf5fc1 100644 --- a/Gems/AWSClientAuth/Code/Source/Authentication/GoogleAuthenticationProvider.cpp +++ b/Gems/AWSClientAuth/Code/Source/Authentication/GoogleAuthenticationProvider.cpp @@ -39,7 +39,7 @@ namespace AWSClientAuth { if (!settingsRegistry.lock()->GetObject(m_settings.get(), azrtti_typeid(m_settings.get()), GoogleSettingsPath)) { - AZ_Warning("AWSCognitoAuthenticationProvider", true, "Failed to get Google settings object for path %s", GoogleSettingsPath); + AZ_Warning("AWSCognitoAuthenticationProvider", false, "Failed to get Google settings object for path %s", GoogleSettingsPath); return false; } return true; @@ -49,21 +49,21 @@ namespace AWSClientAuth { AZ_UNUSED(username); AZ_UNUSED(password); - AZ_Assert(true, "Not supported"); + AZ_Assert(false, "Not supported"); } void GoogleAuthenticationProvider::PasswordGrantMultiFactorSignInAsync(const AZStd::string& username, const AZStd::string& password) { AZ_UNUSED(username); AZ_UNUSED(password); - AZ_Assert(true, "Not supported"); + AZ_Assert(false, "Not supported"); } void GoogleAuthenticationProvider::PasswordGrantMultiFactorConfirmSignInAsync(const AZStd::string& username, const AZStd::string& confirmationCode) { AZ_UNUSED(username); AZ_UNUSED(confirmationCode); - AZ_Assert(true, "Not supported"); + AZ_Assert(false, "Not supported"); } // Call Google authentication provider device code end point. diff --git a/Gems/AWSClientAuth/Code/Source/Authentication/LWAAuthenticationProvider.cpp b/Gems/AWSClientAuth/Code/Source/Authentication/LWAAuthenticationProvider.cpp index a86e01a58e..f43611f2c0 100644 --- a/Gems/AWSClientAuth/Code/Source/Authentication/LWAAuthenticationProvider.cpp +++ b/Gems/AWSClientAuth/Code/Source/Authentication/LWAAuthenticationProvider.cpp @@ -38,7 +38,7 @@ namespace AWSClientAuth { if (!settingsRegistry.lock()->GetObject(m_settings.get(), azrtti_typeid(m_settings.get()), LwaSettingsPath)) { - AZ_Warning("AWSCognitoAuthenticationProvider", true, "Failed to get login with Amazon settings object for path %s", LwaSettingsPath); + AZ_Warning("AWSCognitoAuthenticationProvider", false, "Failed to get login with Amazon settings object for path %s", LwaSettingsPath); return false; } return true; @@ -48,21 +48,21 @@ namespace AWSClientAuth { AZ_UNUSED(username); AZ_UNUSED(password); - AZ_Assert(true, "Not supported"); + AZ_Assert(false, "Not supported"); } void LWAAuthenticationProvider::PasswordGrantMultiFactorSignInAsync(const AZStd::string& username, const AZStd::string& password) { AZ_UNUSED(username); AZ_UNUSED(password); - AZ_Assert(true, "Not supported"); + AZ_Assert(false, "Not supported"); } void LWAAuthenticationProvider::PasswordGrantMultiFactorConfirmSignInAsync(const AZStd::string& username, const AZStd::string& confirmationCode) { AZ_UNUSED(username); AZ_UNUSED(confirmationCode); - AZ_Assert(true, "Not supported"); + AZ_Assert(false, "Not supported"); } // Call LWA authentication provider device code end point. diff --git a/Gems/AWSClientAuth/Code/Source/Authorization/AWSCognitoAuthorizationController.cpp b/Gems/AWSClientAuth/Code/Source/Authorization/AWSCognitoAuthorizationController.cpp index 5e2c07bdbb..1cae2bd0b1 100644 --- a/Gems/AWSClientAuth/Code/Source/Authorization/AWSCognitoAuthorizationController.cpp +++ b/Gems/AWSClientAuth/Code/Source/Authorization/AWSCognitoAuthorizationController.cpp @@ -149,7 +149,7 @@ namespace AWSClientAuth } else { - AZ_Warning("AWSCognitoAuthorizationController", true, "No logins found. Fetching anonymous/unauthenticated credentials"); + AZ_Warning("AWSCognitoAuthorizationController", false, "No logins found. Fetching anonymous/unauthenticated credentials"); } AZ::JobContext* jobContext = nullptr; @@ -277,7 +277,7 @@ namespace AWSClientAuth // Check anonymous credentials as they are optional settings in Cognito Identity pool. if (!m_cognitoCachingAnonymousCredentialsProvider->GetAWSCredentials().IsEmpty()) { - AZ_Warning("AWSCognitoAuthorizationCredentialHandler", true, "No logins found. Using Anonymous credential provider"); + AZ_Warning("AWSCognitoAuthorizationCredentialHandler", false, "No logins found. Using Anonymous credential provider"); return m_cognitoCachingAnonymousCredentialsProvider; } diff --git a/Gems/AWSClientAuth/Code/Tests/Authentication/AuthenticationProviderManagerScriptCanvasBusTest.cpp b/Gems/AWSClientAuth/Code/Tests/Authentication/AuthenticationProviderManagerScriptCanvasBusTest.cpp index 7673840299..6aef72af7e 100644 --- a/Gems/AWSClientAuth/Code/Tests/Authentication/AuthenticationProviderManagerScriptCanvasBusTest.cpp +++ b/Gems/AWSClientAuth/Code/Tests/Authentication/AuthenticationProviderManagerScriptCanvasBusTest.cpp @@ -257,5 +257,5 @@ TEST_F(AuthenticationProviderManagerScriptCanvasTest, Initialize_Fail_InvalidPat { AZ_TEST_START_TRACE_SUPPRESSION; ASSERT_FALSE(m_mockController->Initialize(m_enabledProviderNames, "")); - AZ_TEST_STOP_TRACE_SUPPRESSION(1); + AZ_TEST_STOP_TRACE_SUPPRESSION(2); } diff --git a/Gems/AWSClientAuth/Code/Tests/Authentication/AuthenticationProviderManagerTest.cpp b/Gems/AWSClientAuth/Code/Tests/Authentication/AuthenticationProviderManagerTest.cpp index 4b5bdfb841..ce91e29a1a 100644 --- a/Gems/AWSClientAuth/Code/Tests/Authentication/AuthenticationProviderManagerTest.cpp +++ b/Gems/AWSClientAuth/Code/Tests/Authentication/AuthenticationProviderManagerTest.cpp @@ -256,5 +256,5 @@ TEST_F(AuthenticationProviderManagerTest, Initialize_Fail_InvalidPath) { AZ_TEST_START_TRACE_SUPPRESSION; ASSERT_FALSE(m_mockController->Initialize(m_enabledProviderNames, "")); - AZ_TEST_STOP_TRACE_SUPPRESSION(1); + AZ_TEST_STOP_TRACE_SUPPRESSION(2); } diff --git a/Gems/AWSClientAuth/cdk/README.md b/Gems/AWSClientAuth/cdk/README.md index 4fe668cb9d..3bc2b59fc1 100644 --- a/Gems/AWSClientAuth/cdk/README.md +++ b/Gems/AWSClientAuth/cdk/README.md @@ -51,6 +51,13 @@ To add additional dependencies, for example other CDK libraries, just add them to your requirements.txt file and rerun the `..\..\..\Lumberyard\python\pip.cmd install -r .\Gems\AWSClientAuth\cdk\requirements.txt` command. + +## Update Authorization Permissions +To give permissions to call AWS resources, please update CognitoIdentityPoolRole class with correct policy statements. + +An example IAM permission policy is provided to grant both authenticated and unauthenticated the permission to list S3 buckets in the project. +However, it is expected that developers replace these permissions with those required by your users to use your resources. + ## Useful commands * `cdk ls` list all stacks in the app diff --git a/Gems/AWSClientAuth/cdk/auth/cognito_identity_pool_role.py b/Gems/AWSClientAuth/cdk/auth/cognito_identity_pool_role.py index 3a2e413617..52df897db9 100755 --- a/Gems/AWSClientAuth/cdk/auth/cognito_identity_pool_role.py +++ b/Gems/AWSClientAuth/cdk/auth/cognito_identity_pool_role.py @@ -53,14 +53,17 @@ class CognitoIdentityPoolRole: } }, assume_role_action='sts:AssumeRoleWithWebIdentity')) - # basic permissions + # The above role is created for developers to add custom permissions that they need to provide authorized + # clients. Developers should update the policy statements below to add their required permissions. + # As an example s3:ListBuckets permissions are provided. + # Note: There must be at least one policy statement here. stack_statement = iam.PolicyStatement( actions=[ 's3:ListBuckets' ], effect=iam.Effect.ALLOW, resources=[ - '*' + f'arn:aws:s3:::{project_name}/*' ], sid=name_utils.format_aws_resource_sid(feature_name, project_name, iam.PolicyStatement.__name__) ) diff --git a/Gems/AWSClientAuth/cdk/auth/cognito_user_pool_sms_role.py b/Gems/AWSClientAuth/cdk/auth/cognito_user_pool_sms_role.py index 286b439a76..c4a442f481 100755 --- a/Gems/AWSClientAuth/cdk/auth/cognito_user_pool_sms_role.py +++ b/Gems/AWSClientAuth/cdk/auth/cognito_user_pool_sms_role.py @@ -32,12 +32,18 @@ class CognitoUserPoolSMSRole: name_utils.format_aws_resource_id(feature_name, project_name, env, iam.Role.__name__), description='Role permissions used by Cognito user pool to send sms', assumed_by=iam.ServicePrincipal("cognito-idp.amazonaws.com"), + # Deny all others and then allow only for the current sms role. inline_policies={ 'SNSRoleInlinePolicy': iam.PolicyDocument( statements=[ + # SMS role will be used by CognitoIDP tp allow to publish to SNS topic owned by CognitoIDP + # team to push a sms. + # Need to use * as the resource name used by CognitoIDP principal service is unknown. iam.PolicyStatement( - actions=["sns:Publish"], resources=["*"] + effect=iam.Effect.ALLOW, + actions=['sns:Publish'], + resources=['*'] ) ] )