Add Confused Deputy Prevention #449

dricross · 2025-01-15T20:14:15Z

Description of the issue

To support confused deputy prevention, the CloudWatch Agent needs to be able to pass confused deputy context keys in the request headers of STS AssumeRole calls so that dependent service teams can allow their customers to use confused deputy context keys in their role policies.

For background on the confused deputy problem, see: https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html

Description of changes

Move old assume_role test to credentials_file
- The test uses aws sts assume-role call to grab credentials for a specific role and creates a credentials file which the agent will use. It does not actually test the agent's ability to assume a role via the agent configuration. The new tests do so a rename of the existing test is warranted to avoid confusion.
Create a new assume_role test suite with several subtests, including positive and negative confused deputy prevention tests. This test runs on EC2 linux host only
- Creates four IAM roles:
  - cwa-integ-assume-role-<test id>: Standard CloudWatch Agent permissions role with no confused deputy context keys
  - cwa-integ-assume-role-<test id>-source_arn_key: Standard CloudWatch Agent permissions role and uses the aws:SourceArn context key
  - cwa-integ-assume-role-<test id>-source_account_key: Standard CloudWatch Agent permissions role and uses the aws:SourceAccount context key
  - cwa-integ-assume-role-<test id>-all_context_keys: Standard CloudWatch Agent permissions role and uses the both the aws:SourceAccount and aws:SourceArn context keys
- Runs the following tests:
  - AssumeRoleTest: Test that the agent can assume a standard IAM role and push metrics to CloudWatch
  - SourceArnKeyOnlyTest: Populate the AMZ_SOURCE_ACCOUNT and AMZ_SOURCE_ARN environment variables which configures the agent to populate confused deputy request headers and verify that the agent can assume an IAM role which uses a matching aws:SourceArn context key.
  - SourceAccountKeyOnlyTest: Populate the AMZ_SOURCE_ACCOUNT and AMZ_SOURCE_ARN environment variables which configures the agent to populate confused deputy request headers and verify that the agent can assume an IAM role which uses a matching aws:SourceAccount context key.
  - AllKeysTest: Populate the AMZ_SOURCE_ACCOUNT and AMZ_SOURCE_ARN environment variables which configuresthe agent to populate confused deputy request headers and verify that the agent can assume an IAM role which uses matching aws:SourceArn and the aws:SourceAccount context keys.
  - MissingSourceArnEnvTest: Populate only the AMZ_SOURCE_ACCOUNT environment variable and verify that the agent cannot assume an IAM role which uses the aws:SourceArn context keys.
  - MissingSourceAccountEnvTest: Populate only the AMZ_SOURCE_ARN environment variable and verify that the agent cannot assume an IAM role which uses the aws:SourceAccount context keys.
  - ContextKeyMismatchAccountTest: Populate the AMZ_SOURCE_ACCOUNT and AMZ_SOURCE_ARN environment variables which configures the agent to populate confused deputy request headers which do not match the aws:SourceAccount context key of the IAM role it is configured to assume, and verify the agent is not able to assume the role
  - ContextKeyMismatchArnTest: Populate the AMZ_SOURCE_ACCOUNT and AMZ_SOURCE_ARN environment variables which configures the agent to populate confused deputy request headers which do not match the aws:SourceArn context key of the IAM role it is configured to assume, and verify the agent is not able to assume the role

+-------------------------------+----------------------------------------------------+----------------+------------+--------------------+------------+-----------------+
|             Test              |                   Role to Assume                   | AMZ_SOURCE_ARN | match role | AMZ_SOURCE_ACCOUNT | match role | expected result |
+-------------------------------+----------------------------------------------------+----------------+------------+--------------------+------------+-----------------+
| AssumeRoleTest                | cwa-integ-assume-role-<test id>                    | not populated  | n/a        | not populated      | n/a        | can assume      |
| SourceArnKeyOnlyTest          | cwa-integ-assume-role-<test id>-source_arn_key     | populated      | n/a        | populated          | yes        | can assume      |
| SourceAccountKeyOnlyTest      | cwa-integ-assume-role-<test id>-source_account_key | populated      | yes        | populated          | n/a        | can assume      |
| AllKeysTest                   | cwa-integ-assume-role-<test id>-all_context_keys   | populated      | yes        | populated          | yes        | can assume      |
| MissingSourceArnEnvTest       | cwa-integ-assume-role-<test id>-source_arn_key     | not populated  | n/a        | populated          | n/a        | cannot assume   |
| MissingSourceAccountEnvTest   | cwa-integ-assume-role-<test id>-source_account_key | populated      | yes        | not populated      | n/a        | cannot assume   |
| ContextKeyMismatchAccountTest | cwa-integ-assume-role-<test id>-all_context_keys   | populated      | yes        | fake account       | no         | cannot assume   |
| ContextKeyMismatchArnTest     | cwa-integ-assume-role-<test id>-all_context_keys   | fake ARN       | no         | populated          | yes        | cannot assume   |
+-------------------------------+----------------------------------------------------+----------------+------------+--------------------+------------+-----------------+

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Example run: https://github.com/aws/amazon-cloudwatch-agent/actions/runs/13020191778. See assume_role test, e.g. https://github.com/aws/amazon-cloudwatch-agent/actions/runs/13065824342/job/36483181515.

The existing assume role test doesn't test the agent assuming a role. The test does assume a role, but only to populate a credentials file which the agent will use to obtain credentials. The real test is the agent using a credentials file so using a more appropriate name.

We renamed the old assume_role test to credentials_file so we can rename agent_assume_role to assume_role to make it more concise

test/assume_role/assume_role_unix.go

terraform/ec2/assume_role/main.tf

terraform/ec2/assume_role/variables.tf

test/assume_role/assume_role_unix.go

terraform/ec2/assume_role/main.tf

test/assume_role/agent_configs/config.json

musa-asad · 2025-02-03T15:19:23Z

terraform/ec2/assume_role/variables.tf

+  default     = "go run ./install/install_agent.go rpm"
+}
+
+variable "ca_cert_path" {


nit: Do we need this variable?

musa-asad · 2025-02-03T15:24:18Z

test/credentials_file/credentials_file_unix.go

+		Status: status.FAILED,
+	}
+
+	dims := getDimensions(environment.GetEnvironmentMetaData().InstanceId)


nit: Could we make environment.GetEnvironmentMetaData() a global variable and reference it throughout?

dricross requested a review from a team as a code owner January 15, 2025 20:14

This was referenced Jan 15, 2025

Add Confused Deputy Prevention aws/amazon-cloudwatch-agent#1503

Merged

Add Confused Deputy Prevention amazon-contributing/opentelemetry-collector-contrib#272

Merged

dricross requested review from sky333999 and jefchien January 15, 2025 20:43

dricross changed the title ~~Dricross/confused deputy~~ Add Confused Deputy Prevention Jan 15, 2025

dricross force-pushed the dricross/confused-deputy branch 5 times, most recently from 988d472 to 827e41b Compare January 28, 2025 20:37

dricross added 19 commits January 28, 2025 17:54

Create first agent assume role test

1c093e3

Rename agent_assume_role to assume_role

272aa0d

We renamed the old assume_role test to credentials_file so we can rename agent_assume_role to assume_role to make it more concise

Use bash sudo commands instead of go built-ins

20bea9d

Create 4 iam roles

5b8f9c7

Fix role reference

84e4d17

Create test suite

c7f6aeb

Run the test suite

9e08394

Fix path to service config

9c40b83

Temporarily remove other tests

f6f7748

Add more helpful errors

69d7c45

Fix instanceArn argument

ffa86d1

Fix output

eb5551e

More debugging info

7438bde

More debugging

1ff94a0

Use different delimiter for sed commands

394ca54

sed commands need to be sudo

dcd96e4

Fix role_arn setting in agent config

f167c03

Use role arn instead of instance arn in config

f9e0d70

dricross added 4 commits January 28, 2025 17:54

Fixes and stuff

aef3939

terraform fmt

d49b1ad

Fix incorrect source arn/account tests

cbf2c86

Re-add all the other tests

0e07445

dricross force-pushed the dricross/confused-deputy branch from 8ba06eb to 0e07445 Compare January 28, 2025 22:54

sky333999 previously approved these changes Jan 29, 2025

View reviewed changes

dricross dismissed sky333999’s stale review via 1865e1d January 29, 2025 15:15

Pass account id instead of hardcoding it

6297669

dricross force-pushed the dricross/confused-deputy branch from 1865e1d to 6297669 Compare January 29, 2025 15:29

dricross added 2 commits January 29, 2025 13:51

Improve validations

c3111be

Move daemon reload after all service file manipulation

134d2b7

sky333999 previously approved these changes Jan 29, 2025

View reviewed changes

jefchien reviewed Jan 30, 2025

View reviewed changes

Paramadon reviewed Jan 30, 2025

View reviewed changes

terraform/ec2/assume_role/main.tf Outdated Show resolved Hide resolved

Paramadon reviewed Jan 30, 2025

View reviewed changes

test/assume_role/agent_configs/config.json Show resolved Hide resolved

Paramadon previously approved these changes Jan 30, 2025

View reviewed changes

Minor PR feedback

f2fa641

dricross dismissed stale reviews from Paramadon and sky333999 via f2fa641 January 31, 2025 02:58

dricross added 2 commits January 30, 2025 22:05

Merge branch 'main' into dricross/confused-deputy

efe0716

Re-add required terraform variables

dbcc6f3

musa-asad reviewed Feb 3, 2025

View reviewed changes

jefchien approved these changes Feb 3, 2025

View reviewed changes

Paramadon approved these changes Feb 3, 2025

View reviewed changes

sky333999 merged commit 5300b49 into main Feb 3, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Confused Deputy Prevention #449

Add Confused Deputy Prevention #449

dricross commented Jan 15, 2025 •

edited

Loading

musa-asad Feb 3, 2025 •

edited

Loading

musa-asad Feb 3, 2025 •

edited

Loading

Add Confused Deputy Prevention #449

Add Confused Deputy Prevention #449

Conversation

dricross commented Jan 15, 2025 • edited Loading

Description of the issue

Description of changes

License

Tests

musa-asad Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

musa-asad Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

dricross commented Jan 15, 2025 •

edited

Loading

musa-asad Feb 3, 2025 •

edited

Loading

musa-asad Feb 3, 2025 •

edited

Loading