Read Block Public Access at the account level too - #12
Merged
Merged
Conversation
S3 Block Public Access has two levels, the bucket and the account, and the effective setting is the union. The check read only the bucket level, so an account that enables Block Public Access account-wide and configures nothing per bucket, which is the preferred posture, came back HIGH on every bucket it owns. A prospect running this against a well-run account got a wall of false positives and the correct conclusion that we did not know the API has two levels. The account lookup is one s3control call for the whole run, and the account id already exists in the live path from sts:GetCallerIdentity. Clients stay injected, so the demo drives it from a synthetic account-level fixture with two of the four settings on, which makes the union visible in the first thing anyone runs. A denied or unreachable lookup narrows the check to the bucket level rather than crashing, and the evidence then says "bucket level" so the report never claims coverage it did not have. The evidence string is now keyed on whether a bucket configuration exists. Reporting "disabled settings: <all four>" for a bucket that carries no configuration at all asserts something the client can disprove by looking, which loses the argument on a bucket where our conclusion was right.
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
1 participant
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.
Account-level Block Public Access
The check read Block Public Access only at the bucket level. AWS applies it
at two levels and the effective setting is the union, so an account that
turns it on account-wide and configures nothing per bucket, which is the
preferred posture, came back HIGH on every bucket. That is the worst
failure available to a posture tool: a wall of false HIGHs against a
well-run account.
One
s3control:GetPublicAccessBlockcall per run, unioned with the bucketsetting.
s3:GetAccountPublicAccessBlockadded to the IAM policy. Clientsstay injected, so demo mode drives it from a new synthetic fixture with two
of the four settings enabled at the account level, which puts the union in
front of anyone who runs
posture --demo. A denied or unreachable lookupdegrades to the bucket level and the evidence then says "bucket level"
rather than claiming coverage the run did not have.
Evidence string for absent configuration
A bucket with no Block Public Access configuration was reported as
"disabled settings: BlockPublicAcls, BlockPublicPolicy, IgnorePublicAcls,
RestrictPublicBuckets". "No configuration exists" and "these four were set
to false" are different facts about the client's account and only the first
one is ours to state. Two evidence strings now, keyed on absent versus
present but partial.
Tests
49 pass, up from 40. Nine new tests cover the union, account-level BPA
clearing an unconfigured bucket, one lookup per run, denied and unreachable
degradation, an absent account configuration counting as knowledge rather
than failure, and both evidence strings.
Two existing assertions changed because they asserted the old behaviour:
test_evidence_lists_disabled_settings_in_sorted_orderpinned the stringthis PR replaces, and
test_missing_configuration_is_treated_as_disabledwas named for a conclusion the check no longer draws. Both still assert the
same finding is raised.