Fix: S3 ACL analysis rule for WRITE permissions never triggers #1244
Conversation
This file contains 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
achantavy
force-pushed
the
patch-1
branch
2 times, most recently
from
a850c13 to
7c604ed
Compare
`acl.uri` and `acl.granteeid` are mutually exclusive as per https://github.com/lyft/cartography/blob/f613c32959f4bbdab53f83b2a03532c62e180e8d/cartography/intel/aws/s3.py#L554-L578. So the "WRITE" rule would never trigger. And as these rules are all about detecting _anonymous_ access permissions, there shouldn't be any clauses checking that the grantee is the owner. And as per AWS' documentation, the `WRITE` permission allows the `s3:PutObject` action, so that should be added to `anonymous_actions`. (`s3:DeleteObjectVersion` is only ever granted to bucket owners.) For the "READ_ACP" rule the granted anonymous actions are incorrect.
ramonpetgrave64
approved these changes
Sep 22, 2023
| } | ||
|
|
||
| # Assert that we properly set anonymous_actions on the S3Bucket based on the attached S3Acls | ||
| # (Can't use check_nodes() here because anonymous_actions is a list and is non-hashable.) |
There was a problem hiding this comment.
non-blocker: A few weeks ago we had a _check_nodes_as_list in the semgrep test. We could refactor it into tests.integraton.util
https://github.com/lyft/cartography/blob/ef5cbcede534d91df74d2e497cc2958ae548271e/tests/integration/cartography/intel/semgrep/test_findings.py#L19
chandanchowdhury
pushed a commit
to juju4/cartography
that referenced
this pull request
Jun 26, 2024
…graphy-cncf#1244) cartography-cncf#1128 + tests. See that original PR for full context. Reviewer notes: the main change is here: cartography-cncf@13feb15. Everything else is tests to make sure the analysis job works. --------- Co-authored-by: Jan Hecking <[email protected]>
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.
#1128 + tests. See that original PR for full context.
Reviewer notes: the main change is here: 13feb15. Everything else is tests to make sure the analysis job works.