fix(s3-notifications): s3 notifications not publishing to sns

[msambol]

I tested the integration test with --no-clean and it successfully published.

Oddly, when you create the S3 notification via the Console no Conditions are created but it still works.
Maybe both are needed if you include one. 😓

Closes #27994.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 3c45d4e
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[rix0rrr]

Oddly, when you create the S3 notification via the Console no Conditions are created but it still works.

They are never necessary.

One might argue they prevent a confused deputy attack, but since we already have a condition on aws:SourceArn, that confused deputy attack is already prevented.

I'm also concerned that if someone calls Bucket.fromBucketArn(), we won't correctly parse the Account ID from the ARN (given that an S3 Bucket's ARN does not contain the account ID), and adding this additional condition is going to break some people's code.

I'm seeing no upside, and only downside to merging this. Anyone feel differently?

[msambol]

I'm seeing no upside, and only downside to merging this. Anyone feel differently?

This is fixing a bug. As written, S3 notifications don't successfully trigger to SNS.

[msambol]

@rix0rrr I think the options are remove the existing condition or add this one. The docs have both, but for some reason it doesn't work if you just have the one.

[rix0rrr]

Hi Mike,

At the risk of repeating myself, Conditions never make actions work. The only thing they can do is prevent certain actions that would otherwise be allowed from working. They are like additional filters to the statement's Allow effect.

For example:

{
  "Effect": "Allow",
  "Action": "s3:GetObject",
  "Resource": "*",
  "Principal": "You"
}

Means: "you can read any S3 object".

In contrast:

{
  "Effect": "Allow",
  "Action": "s3:GetObject",
  "Resource": "*",
  "Principal": "You",
  "Condition": {               
    "IpAddress": {"aws:SourceIp": "1.2.3.4"}
   }
}

Means: "you can read any S3 object, but ONLY if you are making API calls from 1.2.3.4".

See how the Condition only takes away from the powers that the statement confers? By default, the statement applies always, but using Conditions you can make the statement only apply sometimes. Adding more Conditions only makes the statement apply successfully in fewer cases.

That's why adding a Condition to a statement that's not working, is never going to make it work.

Conditions are great, but they're somewhat complicated by the fact that you don't get to inspect the variables that are used for the evaluation. That information is not visible or logged anywhere. So if a condition doesn't do what you want, you're going to have to guess what the value of aws:SourceIp or aws:SourceArn was that the server used in its evaluation of that policy statement. This can be very frustrating! (Ask me how I know)

---

Now, I believe you if you say the statement is not working (although it's a bit odd that it would. It sure seems like it should have worked at some point, I wonder if something has changed?). Our job is to figure out why the statement is not working as it is.

I see in the documentation that the recommended ARN pattern has * in it:

            "Condition": {
                "ArnLike": {
                    "aws:SourceArn": "arn:aws:s3:*:*:bucket-name"
                },
                "StringEquals": {
                    "aws:SourceAccount": "bucket-owner-account-id"
                }
            }

This looks super odd to me, as S3 ARNs normally don't have anything in those locations blanked out by the *s. An S3 bucket ARN looks like this:

arn:aws:s3:::bucket_name

As you can see, it doesn't have region or account fields, which are commonly present in all other ARNs. I wonder if that's the bug? That the ARN that the SNS team uses for aws:SourceArn does in fact have fields there, but the ARN we're putting into the Condition by means of { "Fn::GetAtt": ["MyBucket", "Arn"] } does NOT, and that's why the Condition is not matching? (Again, extremely complicated by the fact that we can't look at the value of aws:SourceArn used in the evaluation of this policy anywhere)

Can you try that? Can you try building an ARN that has the * in there, using the bucket.bucketName to put the string together yourself (as opposed to using bucket.bucketArn which might have the wrong format)

---

Once we have the condition on aws:SourceArn, do we still need the condition on aws:SourceAccount?

The only reason these statements are in here to prevent a Confused Deputy attack. The attack goes as follows:

• By adding a statement without the Condition, you give the S3 service team permission to write to your SNS topic (Crucially! Granting permissions to a service principal give the S3 team's account permission, not any particular Bucket!)
• Someone gets a hold of your topic name, and configures their own S3 bucket to send events to your queue. They start dropping objects into their own bucket.
• Because their Bucket is configured to send to your topic, and you've given S3 permissions to write to your topic, you start receiving those notifications!
• The consumers of your Topic are at the very least confused by the weird traffic, and the worst may get DoSed by a barrage of notifications.

The way to mitigate this attack is add the Condition: "yes, S3 is allowed to write to my Topic, but ONLY for events originating from this specific Bucket".

If we have that protection, do we need to add the aws:SourceAccount protection?

🤷 It's always hard to argue against more protection, because you never know when you might need it. But my guess would be no.

[rix0rrr]

(Now of course, all of what I said could easily be invalidated by a single additional if (!hasSourceArnCondition(statement)) { return false; } in S3's code base, and I can't rule out that it's there. AWS is a land of exceptions upon exceptions 🤪 but what I said above applies broadly to 99% of IAM usage, and I would need some solid proof to be convinced otherwise)

[msambol]

@rix0rrr – I 100% agree with you: adding a condition should not make this work. That said, I tested again with the current code and I can get messages to fire to SNS. When I tried last week, it was not working until I added the code in the PR. It wasn't working for @rkraman-leo as well, who originally filed the issue. @rix0rrr, is it possible to check if there was a bug or issue last week that caused this to break?

@rkraman-leo – Can you verify that you can get messages to publish to SNS with the current code?

[rix0rrr]

I tested again with the current code and I can get messages to fire to SNS

Aha! Is what you're saying, even without the fix in this PR the code magically started working again? I'll create an internal ticket to ask around.

[msambol]

@rix0rrr yup, I tested in two different Regions and it's working. It was for sure buggy last week though, it would not trigger SNS.

[rix0rrr]

Huh. If you have an approximate date and time, and a region you observed this behavior in, that would be helpful

[msambol]

@rix0rrr It was in Oregon, ~30 minutes before the PR was opened. 😅

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[rkraman-leo]

@rix0rrr – I 100% agree with you: adding a condition should not make this work. That said, I tested again with the current code and I can get messages to fire to SNS. When I tried last week, it was not working until I added the code in the PR. It wasn't working for @rkraman-leo as well, who originally filed the issue. @rix0rrr, is it possible to check if there was a bug or issue last week that caused this to break?

@rkraman-leo – Can you verify that you can get messages to publish to SNS with the current code?

Apologies for getting back late on this.
Apparently, for the issues that I was facing, it had nothing to do with the SNS Access Policy. The problem was with S3 prefix having special character ("=") that require special handling.

Ref:

• https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines-special-handling
• https://www.geeksforgeeks.org/html-url-encoding/

After I changed the S3 prefix in the event notification from "business_config_snapshot/schemaVersion=1/" to "business_config_snapshot/schemaVersion%3D1/", it worked for me.

[msambol]

@rix0rrr I think this can be closed unless we want to close the loop with the S3 team if there was a glitch in sending notifications to SNS.

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.