Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Confused Deputy Prevention #1503

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .github/workflows/ec2-integration-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}" \
Expand All @@ -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
command: |
cd ${{ inputs.test_dir }}
terraform init
terraform destroy -var="region=${{ inputs.region }}" -var="ami=${{ matrix.arrays.ami }}" --auto-approve
36 changes: 34 additions & 2 deletions cfg/aws/credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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},
Expand All @@ -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,
Expand All @@ -203,6 +205,36 @@ 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"
)

// 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 {

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) {
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)
Expand Down
89 changes: 89 additions & 0 deletions cfg/aws/credentials_test.go
Original file line number Diff line number Diff line change
@@ -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"

"github.com/aws/amazon-cloudwatch-agent/cfg/envconfig"
)

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) {

t.Setenv(envconfig.AmzSourceAccount, tt.envSourceAccount)
t.Setenv(envconfig.AmzSourceArn, tt.envSourceArn)

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(SourceArnHeaderKey)
assert.Equal(t, tt.expectedHeaderArn, headerSourceArn)

headerSourceAccount := request.HTTPRequest.Header.Get(SourceAccountHeaderKey)
assert.Equal(t, tt.expectedHeaderAccount, headerSourceAccount)
})
}

}
4 changes: 4 additions & 0 deletions cfg/envconfig/envconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
Loading