From d19bf9cbdbb2a88595626f6971171b703c484274 Mon Sep 17 00:00:00 2001 From: Cameron Sparr Date: Thu, 5 Dec 2024 09:45:23 -0800 Subject: [PATCH] Limit /etc/pki directory appnet agent bind mount to iso regions followup to #4437 --- agent/engine/ordering_integ_test.go | 5 + agent/engine/serviceconnect/manager_linux.go | 38 +++++- .../serviceconnect/manager_linux_test.go | 127 ++++++++++++++++++ .../manager_linux_test_common.go | 88 +++++++++++- 4 files changed, 250 insertions(+), 8 deletions(-) diff --git a/agent/engine/ordering_integ_test.go b/agent/engine/ordering_integ_test.go index 6547ac687ee..808af698e4e 100644 --- a/agent/engine/ordering_integ_test.go +++ b/agent/engine/ordering_integ_test.go @@ -161,6 +161,11 @@ func TestDependencyComplete(t *testing.T) { // Container 'parent' depends on container 'dependency' to START. We ensure that the 'parent' container starts only // after the 'dependency' container has started. func TestDependencyStart(t *testing.T) { + // Skip these tests on WS 2016 until the failures are root-caused. + isWindows2016, err := config.IsWindows2016() + if err == nil && isWindows2016 == true { + t.Skip() + } taskEngine, done, _, _ := setupWithDefaultConfig(t) defer done() diff --git a/agent/engine/serviceconnect/manager_linux.go b/agent/engine/serviceconnect/manager_linux.go index 54b14c4a211..68abd308be7 100644 --- a/agent/engine/serviceconnect/manager_linux.go +++ b/agent/engine/serviceconnect/manager_linux.go @@ -26,6 +26,8 @@ import ( "github.com/pborman/uuid" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/arn" + "github.com/aws/aws-sdk-go/aws/endpoints" apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container" apiserviceconnect "github.com/aws/amazon-ecs-agent/agent/api/serviceconnect" @@ -200,6 +202,37 @@ func defaultMkdirAllAndChown(path string, perm fs.FileMode, uid, gid int) error return nil } +func getRegionFromContainerInstanceARN(containerInstanceARN string) string { + // Parse the ARN + parsedARN, err := arn.Parse(containerInstanceARN) + if err != nil { + return "" + } + + // Extract the region from the parsed ARN + return parsedARN.Region +} + +func isIsoRegion(region string) bool { + partition, ok := endpoints.PartitionForRegion(endpoints.DefaultPartitions(), region) + if !ok { + // if partition is not found, assume it's iso + return true + } + switch partition.ID() { + case endpoints.AwsPartitionID: + return false + case endpoints.AwsUsGovPartitionID: + return false + case endpoints.AwsCnPartitionID: + return false + default: + // region partition is not 'aws', 'aws-us-gov', nor 'aws-cn', so assume it's + // an iso region. + return true + } +} + func (m *manager) initAgentDirectoryMounts(taskId string, container *apicontainer.Container, hostConfig *dockercontainer.HostConfig) (string, error) { statusPathHost := filepath.Join(m.statusPathHostRoot, taskId) @@ -213,7 +246,10 @@ func (m *manager) initAgentDirectoryMounts(taskId string, container *apicontaine hostConfig.Binds = append(hostConfig.Binds, getBindMountMapping(statusPathHost, m.statusPathContainer)) hostConfig.Binds = append(hostConfig.Binds, getBindMountMapping(m.relayPathHost, m.relayPathContainer)) - hostConfig.Binds = append(hostConfig.Binds, getBindMountMapping(hostPKIDirPath, hostPKIDirPath)) + region := getRegionFromContainerInstanceARN(m.containerInstanceARN) + if isIsoRegion(region) { + hostConfig.Binds = append(hostConfig.Binds, getBindMountMapping(hostPKIDirPath, hostPKIDirPath)) + } // create logging directory and bind mount, if customer has not configured a logging driver if container.GetLogDriver() == "" { diff --git a/agent/engine/serviceconnect/manager_linux_test.go b/agent/engine/serviceconnect/manager_linux_test.go index dea6b71477a..73fef2a80fa 100644 --- a/agent/engine/serviceconnect/manager_linux_test.go +++ b/agent/engine/serviceconnect/manager_linux_test.go @@ -182,3 +182,130 @@ func TestGetSupportedAppnetInterfaceVerToCapabilities(t *testing.T) { }) } } + +func TestGetRegionFromContainerInstanceARN(t *testing.T) { + tests := []struct { + name string + containerInstanceARN string + expectedRegion string + }{ + { + name: "Valid ARN - US West 2", + containerInstanceARN: "arn:aws:ecs:us-west-2:123456789012:container-instance/12345678-1234-1234-1234-123456789012", + expectedRegion: "us-west-2", + }, + { + name: "Valid ARN - EU Central 1", + containerInstanceARN: "arn:aws:ecs:eu-central-1:123456789012:container-instance/87654321-4321-4321-4321-210987654321", + expectedRegion: "eu-central-1", + }, + { + name: "Valid ARN - AP Southeast 1", + containerInstanceARN: "arn:aws:ecs:ap-southeast-1:123456789012:container-instance/11223344-5566-7788-9900-112233445566", + expectedRegion: "ap-southeast-1", + }, + { + name: "Valid ARN - US Gov West 1", + containerInstanceARN: "arn:aws-us-gov:ecs:us-gov-west-1:123456789012:container-instance/98765432-1234-5678-9012-123456789012", + expectedRegion: "us-gov-west-1", + }, + { + name: "Valid ARN - CN North 1", + containerInstanceARN: "arn:aws-cn:ecs:cn-north-1:123456789012:container-instance/11223344-5566-7788-9900-112233445566", + expectedRegion: "cn-north-1", + }, + { + name: "Invalid ARN - Missing Region", + containerInstanceARN: "arn:aws:ecs::123456789012:container-instance/12345678-1234-1234-1234-123456789012", + expectedRegion: "", + }, + { + name: "Invalid ARN - Wrong Service", + containerInstanceARN: "arn:aws:ec2:us-west-2:123456789012:instance/i-1234567890abcdef0", + expectedRegion: "us-west-2", + }, + { + name: "Invalid ARN Format", + containerInstanceARN: "invalid:arn:format", + expectedRegion: "", + }, + { + name: "Empty ARN", + containerInstanceARN: "", + expectedRegion: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getRegionFromContainerInstanceARN(tt.containerInstanceARN) + assert.Equal(t, tt.expectedRegion, result, "Unexpected region for ARN: %s", tt.containerInstanceARN) + }) + } +} + +func TestIsIsoRegion(t *testing.T) { + tests := []struct { + name string + region string + expectedResult bool + }{ + { + name: "AWS Standard Region - US West 2", + region: "us-west-2", + expectedResult: false, + }, + { + name: "AWS Standard Region - EU Central 1", + region: "eu-central-1", + expectedResult: false, + }, + { + name: "AWS GovCloud Region - US Gov West 1", + region: "us-gov-west-1", + expectedResult: false, + }, + { + name: "AWS GovCloud Region - US Gov East 1", + region: "us-gov-east-1", + expectedResult: false, + }, + { + name: "AWS China Region - CN North 1", + region: "cn-north-1", + expectedResult: false, + }, + { + name: "AWS China Region - CN Northwest 1", + region: "cn-northwest-1", + expectedResult: false, + }, + { + name: "ISO Region - US ISO East 1", + region: "us-iso-east-1", + expectedResult: true, + }, + { + name: "ISO Region - US ISOB East 1", + region: "us-isob-east-1", + expectedResult: true, + }, + { + name: "Unknown Region", + region: "unknown-region", + expectedResult: true, + }, + { + name: "Empty Region", + region: "", + expectedResult: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isIsoRegion(tt.region) + assert.Equal(t, tt.expectedResult, result, "Unexpected result for region: %s", tt.region) + }) + } +} diff --git a/agent/engine/serviceconnect/manager_linux_test_common.go b/agent/engine/serviceconnect/manager_linux_test_common.go index 35b38444f37..dd07e6b1e9c 100644 --- a/agent/engine/serviceconnect/manager_linux_test_common.go +++ b/agent/engine/serviceconnect/manager_linux_test_common.go @@ -111,6 +111,17 @@ func getAWSVPCTask(t *testing.T) (*apitask.Task, *apicontainer.Container, *apico return sleepTask, pauseContainer, serviceConnectContainer } +func copyMap(input map[string]string, addk string, addv string) map[string]string { + output := make(map[string]string) + if len(addk) > 0 && len(addv) > 0 { + output[addk] = addv + } + for k, v := range input { + output[k] = v + } + return output +} + func testAgentContainerModificationsForServiceConnect(t *testing.T, privilegedMode bool) { backupMkdirAllAndChown := mkdirAllAndChown tempDir := t.TempDir() @@ -129,14 +140,12 @@ func testAgentContainerModificationsForServiceConnect(t *testing.T, privilegedMo fmt.Sprintf("%s/status/%s:%s", tempDir, scTask.GetID(), "/some/other/run"), fmt.Sprintf("%s/relay:%s", tempDir, "/not/var/run"), fmt.Sprintf("%s/log/%s:%s", tempDir, scTask.GetID(), "/some/other/log"), - "/etc/pki:/etc/pki", } expectedENVs := map[string]string{ "ReLaYgOeShErE": "unix:///not/var/run/relay_file_of_holiness", "StAtUsGoEsHeRe": "/some/other/run/status_file_of_holiness", "APPNET_AGENT_ADMIN_MODE": "uds", "ENVOY_ENABLE_IAM_AUTH_FOR_XDS": "0", - "ECS_CONTAINER_INSTANCE_ARN": "fake_container_instance", "APPNET_ENVOY_LOG_DESTINATION": "/some/other/log", } @@ -147,15 +156,80 @@ func testAgentContainerModificationsForServiceConnect(t *testing.T, privilegedMo expectedBinds []string expectedBindDirPerm string expectedBindDirOwner uint32 + containerInstanceARN string } testcases := []testCase{ { - name: "Service connect container has extra binds/ENV", + name: "Service connect container has extra binds/ENV. Commercial region has no /etc/pki mount.", container: serviceConnectContainer, - expectedENV: expectedENVs, + expectedENV: copyMap(expectedENVs, "ECS_CONTAINER_INSTANCE_ARN", "arn:aws:ecs:us-west-2:123456789012:container-instance/12345678-test-test-test-123456789012"), expectedBinds: expectedBinds, expectedBindDirPerm: fs.FileMode(0700).String(), expectedBindDirOwner: serviceconnect.AppNetUID, + containerInstanceARN: "arn:aws:ecs:us-west-2:123456789012:container-instance/12345678-test-test-test-123456789012", + }, + { + name: "Service connect container has extra binds/ENV. US gov region has no /etc/pki mount.", + container: serviceConnectContainer, + expectedENV: copyMap(expectedENVs, "ECS_CONTAINER_INSTANCE_ARN", "arn:aws:ecs:us-gov-west-1:123456789012:container-instance/12345678-test-test-test-123456789012"), + expectedBinds: expectedBinds, + expectedBindDirPerm: fs.FileMode(0700).String(), + expectedBindDirOwner: serviceconnect.AppNetUID, + containerInstanceARN: "arn:aws:ecs:us-gov-west-1:123456789012:container-instance/12345678-test-test-test-123456789012", + }, + { + name: "Service connect container has extra binds/ENV. China region has no /etc/pki mount.", + container: serviceConnectContainer, + expectedENV: copyMap(expectedENVs, "ECS_CONTAINER_INSTANCE_ARN", "arn:aws:ecs:cn-north-1:123456789012:container-instance/12345678-test-test-test-123456789012"), + expectedBinds: expectedBinds, + expectedBindDirPerm: fs.FileMode(0700).String(), + expectedBindDirOwner: serviceconnect.AppNetUID, + containerInstanceARN: "arn:aws:ecs:cn-north-1:123456789012:container-instance/12345678-test-test-test-123456789012", + }, + { + name: "Service connect container has extra binds/ENV. Iso region gets extra /etc/pki bind mount.", + container: serviceConnectContainer, + expectedENV: copyMap(expectedENVs, "ECS_CONTAINER_INSTANCE_ARN", "arn:aws:ecs:us-iso-east-1:123456789012:container-instance/12345678-test-test-test-123456789012"), + expectedBinds: append(expectedBinds, "/etc/pki:/etc/pki"), + expectedBindDirPerm: fs.FileMode(0700).String(), + expectedBindDirOwner: serviceconnect.AppNetUID, + containerInstanceARN: "arn:aws:ecs:us-iso-east-1:123456789012:container-instance/12345678-test-test-test-123456789012", + }, + { + name: "Service connect container has extra binds/ENV. Iso region gets extra /etc/pki bind mount.", + container: serviceConnectContainer, + expectedENV: copyMap(expectedENVs, "ECS_CONTAINER_INSTANCE_ARN", "arn:aws:ecs:eu-isoe-west-1:123456789012:container-instance/12345678-test-test-test-123456789012"), + expectedBinds: append(expectedBinds, "/etc/pki:/etc/pki"), + expectedBindDirPerm: fs.FileMode(0700).String(), + expectedBindDirOwner: serviceconnect.AppNetUID, + containerInstanceARN: "arn:aws:ecs:eu-isoe-west-1:123456789012:container-instance/12345678-test-test-test-123456789012", + }, + { + name: "Service connect container has extra binds/ENV. Iso region gets extra /etc/pki bind mount.", + container: serviceConnectContainer, + expectedENV: copyMap(expectedENVs, "ECS_CONTAINER_INSTANCE_ARN", "arn:aws:ecs:us-isof-south-1:123456789012:container-instance/12345678-test-test-test-123456789012"), + expectedBinds: append(expectedBinds, "/etc/pki:/etc/pki"), + expectedBindDirPerm: fs.FileMode(0700).String(), + expectedBindDirOwner: serviceconnect.AppNetUID, + containerInstanceARN: "arn:aws:ecs:us-isof-south-1:123456789012:container-instance/12345678-test-test-test-123456789012", + }, + { + name: "Service connect container has extra binds/ENV. Unknown region gets /etc/pki bind mount.", + container: serviceConnectContainer, + expectedENV: copyMap(expectedENVs, "ECS_CONTAINER_INSTANCE_ARN", "arn:aws:ecs:ap-iso-southeast-1:123456789012:container-instance/12345678-test-test-test-123456789012"), + expectedBinds: append(expectedBinds, "/etc/pki:/etc/pki"), + expectedBindDirPerm: fs.FileMode(0700).String(), + expectedBindDirOwner: serviceconnect.AppNetUID, + containerInstanceARN: "arn:aws:ecs:ap-iso-southeast-1:123456789012:container-instance/12345678-test-test-test-123456789012", + }, + { + name: "Service connect container has extra binds/ENV. Invalid region gets /etc/pki bind mount.", + container: serviceConnectContainer, + expectedENV: copyMap(expectedENVs, "ECS_CONTAINER_INSTANCE_ARN", "foo-bar-invalid-arn"), + expectedBinds: append(expectedBinds, "/etc/pki:/etc/pki"), + expectedBindDirPerm: fs.FileMode(0700).String(), + expectedBindDirOwner: serviceconnect.AppNetUID, + containerInstanceARN: "foo-bar-invalid-arn", }, } // Add test cases for other containers expecting no modifications @@ -179,14 +253,14 @@ func testAgentContainerModificationsForServiceConnect(t *testing.T, privilegedMo agentContainerImageName: "container", appnetInterfaceVersion: "v1", - containerInstanceARN: "fake_container_instance", - logPathContainer: "/some/other/log", - logPathHostRoot: filepath.Join(tempDir, "log"), + logPathContainer: "/some/other/log", + logPathHostRoot: filepath.Join(tempDir, "log"), } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { hostConfig := &dockercontainer.HostConfig{} + scManager.containerInstanceARN = tc.containerInstanceARN err := scManager.AugmentTaskContainer(scTask, tc.container, hostConfig) if err != nil { t.Fatal(err)