From f855830153dbcee4a8719437a8b1903a095cff64 Mon Sep 17 00:00:00 2001 From: Rick Rossi Date: Wed, 25 Sep 2024 10:55:45 -0400 Subject: [PATCH 1/3] Add support for confused deputy headers --- cfg/aws/credentials.go | 37 ++++++++++++++- cfg/aws/credentials_test.go | 89 +++++++++++++++++++++++++++++++++++++ cfg/envconfig/envconfig.go | 4 ++ 3 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 cfg/aws/credentials_test.go diff --git a/cfg/aws/credentials.go b/cfg/aws/credentials.go index c0867ba258..e8bd986dc1 100644 --- a/cfg/aws/credentials.go +++ b/cfg/aws/credentials.go @@ -16,9 +16,11 @@ import ( "github.com/aws/aws-sdk-go/aws/credentials/ec2rolecreds" "github.com/aws/aws-sdk-go/aws/credentials/stscreds" "github.com/aws/aws-sdk-go/aws/endpoints" + "github.com/aws/aws-sdk-go/aws/request" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/sts" + "github.com/aws/amazon-cloudwatch-agent/cfg/envconfig" "github.com/aws/amazon-cloudwatch-agent/extension/agenthealth/handler/stats/agent" ) @@ -174,7 +176,7 @@ func (s *stsCredentialProvider) Retrieve() (credentials.Value, error) { func newStsCredentials(c client.ConfigProvider, roleARN string, region string) *credentials.Credentials { regional := &stscreds.AssumeRoleProvider{ - Client: sts.New(c, &aws.Config{ + Client: newStsClient(c, &aws.Config{ Region: aws.String(region), STSRegionalEndpoint: endpoints.RegionalSTSEndpoint, HTTPClient: &http.Client{Timeout: 1 * time.Minute}, @@ -188,7 +190,7 @@ func newStsCredentials(c client.ConfigProvider, roleARN string, region string) * fallbackRegion := getFallbackRegion(region) partitional := &stscreds.AssumeRoleProvider{ - Client: sts.New(c, &aws.Config{ + Client: newStsClient(c, &aws.Config{ Region: aws.String(fallbackRegion), Endpoint: aws.String(getFallbackEndpoint(fallbackRegion)), STSRegionalEndpoint: endpoints.RegionalSTSEndpoint, @@ -203,6 +205,37 @@ func newStsCredentials(c client.ConfigProvider, roleARN string, region string) * return credentials.NewCredentials(&stsCredentialProvider{regional: regional, partitional: partitional}) } +const ( + SourceArnHeaderKey = "x-amz-source-arn" + SourceAccountHeaderKey = "x-amz-source-account" +) + +var ( + sourceAccount = os.Getenv(envconfig.AmzSourceAccount) // populates the "x-amz-source-account" header + sourceArn = os.Getenv(envconfig.AmzSourceArn) // populates the "x-amz-source-arn" header +) + +// newStsClient creates a new STS client with the provided config and options. +// Additionally, if specific environment variables are set, it also appends the confused deputy headers to requests +// made by the client. These headers allow resource-based policies to limit the permissions that a service has to +// a specific resource. Note that BOTH environment variables need to contain non-empty values in order for the headers +// to be set. +// +// See https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html#cross-service-confused-deputy-prevention +func newStsClient(p client.ConfigProvider, cfgs ...*aws.Config) *sts.STS { + client := sts.New(p, cfgs...) + if sourceAccount != "" && sourceArn != "" { + client.Handlers.Sign.PushFront(func(r *request.Request) { + r.HTTPRequest.Header.Set(SourceArnHeaderKey, sourceArn) + r.HTTPRequest.Header.Set(SourceAccountHeaderKey, sourceAccount) + }) + + log.Printf("I! Found confused deputy header environment variables: source account: %q, source arn: %q", sourceAccount, sourceArn) + } + + return client +} + // The partitional STS endpoint used to fallback when regional STS endpoint is not activated. func getFallbackEndpoint(region string) string { partition := getPartition(region) diff --git a/cfg/aws/credentials_test.go b/cfg/aws/credentials_test.go new file mode 100644 index 0000000000..39aa0188de --- /dev/null +++ b/cfg/aws/credentials_test.go @@ -0,0 +1,89 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: MIT + +package aws + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/credentials" + "github.com/aws/aws-sdk-go/awstesting/mock" + "github.com/aws/aws-sdk-go/service/sts" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConfusedDeputyHeaders(t *testing.T) { + tests := []struct { + name string + envSourceArn string + envSourceAccount string + expectedHeaderArn string + expectedHeaderAccount string + }{ + { + name: "unpopulated", + envSourceArn: "", + envSourceAccount: "", + expectedHeaderArn: "", + expectedHeaderAccount: "", + }, + { + name: "both populated", + envSourceArn: "arn:aws:ec2:us-east-1:474668408639:instance/i-08293cd9825754f7c", + envSourceAccount: "539247453986", + expectedHeaderArn: "arn:aws:ec2:us-east-1:474668408639:instance/i-08293cd9825754f7c", + expectedHeaderAccount: "539247453986", + }, + { + name: "only source arn populated", + envSourceArn: "arn:aws:ec2:us-east-1:474668408639:instance/i-08293cd9825754f7c", + envSourceAccount: "", + expectedHeaderArn: "", + expectedHeaderAccount: "", + }, + { + name: "only source account populated", + envSourceArn: "", + envSourceAccount: "539247453986", + expectedHeaderArn: "", + expectedHeaderAccount: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set global variables which will get picked up by newStsClient + sourceArn = tt.envSourceArn + sourceAccount = tt.envSourceAccount + + client := newStsClient(mock.Session, &aws.Config{ + // These are examples credentials pulled from: + // https://docs.aws.amazon.com/STS/latest/APIReference/API_GetAccessKeyInfo.html + Credentials: credentials.NewStaticCredentials("AKIAIOSFODNN7EXAMPLE", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", ""), + Region: aws.String("us-east-1"), + }) + + request, _ := client.AssumeRoleRequest(&sts.AssumeRoleInput{ + // We aren't going to actually make the assume role call, we are just going + // to verify the headers are present once signed so the RoleArn and RoleSessionName + // arguments are irrelevant. Fill them out with something so the request is valid. + RoleArn: aws.String("arn:aws:iam::012345678912:role/XXXXXXXX"), + RoleSessionName: aws.String("MockSession"), + }) + + // Headers are generated after the request is signed (but before it's sent) + err := request.Sign() + require.NoError(t, err) + + headerSourceArn := request.HTTPRequest.Header.Get("x-amz-source-arn") + assert.Equal(t, tt.expectedHeaderArn, headerSourceArn) + + headerSourceAccount := request.HTTPRequest.Header.Get("x-amz-source-account") + assert.Equal(t, tt.expectedHeaderAccount, headerSourceAccount) + }) + } + + sourceArn = "" + sourceAccount = "" +} diff --git a/cfg/envconfig/envconfig.go b/cfg/envconfig/envconfig.go index 3fd4b637d2..afbf4918de 100644 --- a/cfg/envconfig/envconfig.go +++ b/cfg/envconfig/envconfig.go @@ -32,6 +32,10 @@ const ( CWConfigContent = "CW_CONFIG_CONTENT" CWOtelConfigContent = "CW_OTEL_CONFIG_CONTENT" CWAgentMergedOtelConfig = "CWAGENT_MERGED_OTEL_CONFIG" + + // confused deputy prevention related headers + AmzSourceAccount = "AMZ_SOURCE_ACCOUNT" // populates the "x-amz-source-account" header + AmzSourceArn = "AMZ_SOURCE_ARN" // populates the "x-amz-source-arn" header ) const ( From 7e4753185a65e73d085b09beb9d89c90b9db99b3 Mon Sep 17 00:00:00 2001 From: Rick Rossi Date: Tue, 28 Jan 2025 11:29:12 -0500 Subject: [PATCH 2/3] terraform init on terraform destroy workflow --- .github/workflows/ec2-integration-test.yml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ec2-integration-test.yml b/.github/workflows/ec2-integration-test.yml index 2a31c4d468..a74d674b98 100644 --- a/.github/workflows/ec2-integration-test.yml +++ b/.github/workflows/ec2-integration-test.yml @@ -92,9 +92,11 @@ jobs: terraform init if terraform apply --auto-approve \ - -var="ssh_key_value=${{env.PRIVATE_KEY}}" -var="github_test_repo=${{ inputs.test_repo_url }}" \ + -var="ssh_key_value=${{env.PRIVATE_KEY}}" \ + -var="github_test_repo=${{ inputs.test_repo_url }}" \ -var="test_name=${{ matrix.arrays.os }}" \ - -var="cwa_github_sha=${{inputs.github_sha}}" -var="install_agent=${{ matrix.arrays.installAgentCommand }}" \ + -var="cwa_github_sha=${{inputs.github_sha}}" \ + -var="install_agent=${{ matrix.arrays.installAgentCommand }}" \ -var="github_test_repo_branch=${{inputs.test_repo_branch}}" \ -var="ec2_instance_type=${{ matrix.arrays.instanceType }}" \ -var="user=${{ matrix.arrays.username }}" \ @@ -121,4 +123,7 @@ jobs: max_attempts: 2 timeout_minutes: 8 retry_wait_seconds: 5 - command: cd ${{ inputs.test_dir }} && terraform destroy -var="region=${{ inputs.region }}" -var="ami=${{ matrix.arrays.ami }}" --auto-approve \ No newline at end of file + command: | + cd ${{ inputs.test_dir }} + terraform init + terraform destroy -var="region=${{ inputs.region }}" -var="ami=${{ matrix.arrays.ami }}" --auto-approve \ No newline at end of file From 20257aff110bed25128ad5815736040c670e3f32 Mon Sep 17 00:00:00 2001 From: Rick Rossi Date: Wed, 29 Jan 2025 09:36:12 -0500 Subject: [PATCH 3/3] Don't use globals --- cfg/aws/credentials.go | 9 ++++----- cfg/aws/credentials_test.go | 14 +++++++------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/cfg/aws/credentials.go b/cfg/aws/credentials.go index e8bd986dc1..7701d6b7f3 100644 --- a/cfg/aws/credentials.go +++ b/cfg/aws/credentials.go @@ -210,11 +210,6 @@ const ( SourceAccountHeaderKey = "x-amz-source-account" ) -var ( - sourceAccount = os.Getenv(envconfig.AmzSourceAccount) // populates the "x-amz-source-account" header - sourceArn = os.Getenv(envconfig.AmzSourceArn) // populates the "x-amz-source-arn" header -) - // newStsClient creates a new STS client with the provided config and options. // Additionally, if specific environment variables are set, it also appends the confused deputy headers to requests // made by the client. These headers allow resource-based policies to limit the permissions that a service has to @@ -223,6 +218,10 @@ var ( // // See https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html#cross-service-confused-deputy-prevention func newStsClient(p client.ConfigProvider, cfgs ...*aws.Config) *sts.STS { + + sourceAccount := os.Getenv(envconfig.AmzSourceAccount) + sourceArn := os.Getenv(envconfig.AmzSourceArn) + client := sts.New(p, cfgs...) if sourceAccount != "" && sourceArn != "" { client.Handlers.Sign.PushFront(func(r *request.Request) { diff --git a/cfg/aws/credentials_test.go b/cfg/aws/credentials_test.go index 39aa0188de..722590db60 100644 --- a/cfg/aws/credentials_test.go +++ b/cfg/aws/credentials_test.go @@ -12,6 +12,8 @@ import ( "github.com/aws/aws-sdk-go/service/sts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/aws/amazon-cloudwatch-agent/cfg/envconfig" ) func TestConfusedDeputyHeaders(t *testing.T) { @@ -53,9 +55,9 @@ func TestConfusedDeputyHeaders(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Set global variables which will get picked up by newStsClient - sourceArn = tt.envSourceArn - sourceAccount = tt.envSourceAccount + + t.Setenv(envconfig.AmzSourceAccount, tt.envSourceAccount) + t.Setenv(envconfig.AmzSourceArn, tt.envSourceArn) client := newStsClient(mock.Session, &aws.Config{ // These are examples credentials pulled from: @@ -76,14 +78,12 @@ func TestConfusedDeputyHeaders(t *testing.T) { err := request.Sign() require.NoError(t, err) - headerSourceArn := request.HTTPRequest.Header.Get("x-amz-source-arn") + headerSourceArn := request.HTTPRequest.Header.Get(SourceArnHeaderKey) assert.Equal(t, tt.expectedHeaderArn, headerSourceArn) - headerSourceAccount := request.HTTPRequest.Header.Get("x-amz-source-account") + headerSourceAccount := request.HTTPRequest.Header.Get(SourceAccountHeaderKey) assert.Equal(t, tt.expectedHeaderAccount, headerSourceAccount) }) } - sourceArn = "" - sourceAccount = "" }