IAM: addAccountCondition adds condition which is not AWS:SourceAccount

[mdavis-xyz]

Describe the issue

The description for addAccountCondition says:

Add a condition that limits to a given account.

Based on this, I assumed that it would add

      "Condition": {
        "StringEquals": {
          "AWS:SourceAccount": "1234"
        }
      }

But it does not. It adds:

      "Condition": {
        "StringEquals": {
          "sts:ExternalId": "1234"
        }
      }

These are for very different use cases.

In general, any account can set any value for "sts:ExternalId". An attacker who knows my account number could just set my account number as the ExternalId when assuming their own role, which would satisfy this constraint. So this condition doesn't actually relate to an account, just a passphrase.

My use case is that I'm granting S3 permission to publish to my SNS topic, with a topic policy. But I don't want other accounts to publish to my topic. I want to restrict it with an account condition.

When AWS assumes a role (e.g. S3 publishing notifications to my SNS topic), it does not set the assume role passphrase to the account number. So this condition actually ends up blocking legitimate traffic from my account.

I feel like this function is misnamed.

I'm guessing that changing the behavior of this function from "sts:ExternalId" to "AWS:SourceAccount" is a backwards-incompatible change that will mess with other users.

So what I propose is that the documentation should just say that this doesn't actually add a source account condition. And it should say how you should add a source account condition.

Something like:

Add a "sts:ExternalId" condition that the caller used an external ID when assuming their role which is equal to the specified account number, to prevent a confused deputy attack.
Note that this is different to a condition limiting the source account. To do that, either use the account's principal with addAwsAccountPrincipal, or use statement.addCondition("StringEquals", { "AWS:SourceAccount": Aws.ACCOUNT_ID }) .

(I also think it would be nice to make the argument to addAccountCondition and addAwsAccountPrincipal optional, defaulting to the current account. And to add another function addSourceAccountCondition, which uses "AWS:SourceAccount")

Links

https://docs.aws.amazon.com/cdk/api/v1/docs/@aws-cdk_aws-iam.PolicyStatement.html#addwbraccountwbrconditionaccountid

[pahud]

According to this doc, In your use case:

client -> S3 bucket(owner:111111111111) -> SNS Topic(owner:222222222222)

I believe you will need two conditions in your resource policy document:

1. aws:SourceArn with ArnEquals
2. aws:SourceAccount with StringEquals

And your CDK code probably should look like this:

export class Demo2Stack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const stack = Stack.of(this);
    const statement = new iam.PolicyStatement({
      actions: ['sns:*'],
      resources: ['*'],
      principals: [ new iam.ServicePrincipal('s3.amazonaws.com') ],
    });

    statement.addConditions(
      {
        'ArnEquals': { 'aws:SourceArn': bucketArn },
        'StringEquals': { 'aws:SourceAccount': '111111111111' },
      }
    );

    const topic = new sns.Topic(this, 'Topic');
    topic.addToResourcePolicy(statement)
  };
};

• please note the sample snippet is FYR only. I didn't test or validate it in my account.

And I agree we probably should add a addSourceAccountCondition function for aws:SourceAccount, at least to improve the document for addAccountCondition.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.