fix(aws/s3): correlate SSL bucket policy to its bucket by identity [FIX-355] - #76
Open
drewblas wants to merge 1 commit into
Open
fix(aws/s3): correlate SSL bucket policy to its bucket by identity [FIX-355]#76drewblas wants to merge 1 commit into
drewblas wants to merge 1 commit into
Conversation
…IX-355]
The S3.5 (require-SSL) check confirmed a bucket policy's non-SSL deny
statement covered the attached bucket by string-matching the deny
statement's Resource ARNs against the policy's `bucket` argument. That
comparison breaks when the two sides resolve differently: e.g. the
`bucket` argument resolves to a literal name (name set from a local/var)
while the deny statement references `aws_s3_bucket.x.arn`, which stays an
unresolved synthetic token. A correct, textbook SSL-deny policy was then
reported as non-compliant.
Move the resource-coverage decision into PostProcess, where the policy is
already linked to its bucket, and match the deny Resource ARNs against
both of the bucket's identities:
- its resolved name (arn:aws:s3:::<name> or a bare <name>), and
- its synthetic instance token, which equals bucket.ID and is exactly
what .arn/.id render to when unresolved.
Matching on both is independent of which side resolved, with no
benefit-of-the-doubt fallback, so it still rejects policies that protect
a different bucket or omit the bucket/objects ARNs.
The SSL-deny facts (whether a compliant deny statement is present and the
resources it lists) are extracted at parse time into a new transient
BucketPolicy.SSLDeny field. PostProcess runs in the parser plugin before
serialization, so this needs no proto/codegen change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a false positive in the S3.5 (require SSL) FinOps policy: a correct, textbook SSL-deny bucket policy is reported as non-compliant.
Root cause. The check confirmed that a bucket policy's non-SSL deny statement covers the attached bucket by string-matching the deny statement's
ResourceARNs against the policy'sbucketargument. That comparison breaks under mixed resolution — when one side resolves and the other doesn't:bucket = aws_s3_bucket.x.bucketresolves to the literal name, butaws_s3_bucket.x.arn, whose ARN isn't known until apply, so it renders as an unresolved synthetic token.A real name never matches a synthetic token, so
resourcesCoverBucketreturned false and the compliant policy was flagged.Fix
Move the resource-coverage decision out of the parser (which only had the policy's
bucketstring) and intos3.S3.PostProcess, where the policy is already linked to its bucket. Match each denyResourceARN against both of the bucket's identities:arn:aws:s3:::<name>or a bare<name>), andbucket.ID, exactly what.arn/.idrender to when unresolved.This is independent of which side resolved, with no benefit-of-the-doubt fallback, so it still rejects policies that protect a different bucket or omit the bucket/objects ARNs.
The SSL-deny facts (whether a compliant deny statement exists and the resources it lists) are extracted at parse time into a new transient
BucketPolicy.SSLDenyfield.PostProcessruns in the parser plugin before serialization, so no proto/codegen change is required.Changes
pkg/tree/aws/s3/bucket_policy.go— add transientSSLDeny *SSLDenyInfo.pkg/tree/aws/s3/s3.go— finalizeDeniesInsecureTransportinPostProcess; adddenyResourcesCoverBucket/arnRefersToBucket(identity-based matching).Verification
SSLDenyInfo), an 8-case matrix covers every resolution combination; guards for wrong-bucket / objects-only coverage still fail correctly.denies=true, so S3.5 passes.Rollout
Cross-repo, release-ordered: merge and release go-proto first, then bump
infracost/parser'sgo.modto the new version and merge the paired parser PR.Linear: FIX-355