From 71d0f8c47bfec07bc94dfe96a9b31ecbd2403e43 Mon Sep 17 00:00:00 2001 From: Alex Chantavy Date: Fri, 22 Sep 2023 14:47:52 -0700 Subject: [PATCH] Fix: S3 ACL analysis rule for WRITE permissions never triggers (#1244) https://github.com/lyft/cartography/pull/1128 + tests. See that original PR for full context. Reviewer notes: the main change is here: https://github.com/lyft/cartography/pull/1244/commits/13feb15dc6ea53f4290ca95c6f0e9ddc26b390f3. Everything else is tests to make sure the analysis job works. --------- Co-authored-by: Jan Hecking --- .../jobs/analysis/aws_s3acl_analysis.json | 9 ++- cartography/intel/aws/s3.py | 7 ++- tests/data/aws/s3.py | 63 +++++++++++++++++++ .../cartography/intel/aws/test_s3_acls.py | 51 +++++++++++++++ 4 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 tests/integration/cartography/intel/aws/test_s3_acls.py diff --git a/cartography/data/jobs/analysis/aws_s3acl_analysis.json b/cartography/data/jobs/analysis/aws_s3acl_analysis.json index 73a80581c8..27f4e759f6 100644 --- a/cartography/data/jobs/analysis/aws_s3acl_analysis.json +++ b/cartography/data/jobs/analysis/aws_s3acl_analysis.json @@ -1,22 +1,27 @@ { "statements": [ { + "__comment__": "READ -> ListBucket, ListBucketVersions, ListBucketMultipartUploads", "query": "MATCH (acl:S3Acl)-[:APPLIES_TO]->(bucket:S3Bucket)<-[:RESOURCE]-(aws:AWSAccount{id: $AWS_ID})\nWHERE acl.uri IN ['http://acs.amazonaws.com/groups/global/AllUsers', 'http://acs.amazonaws.com/groups/global/AuthenticatedUsers'] AND acl.permission = 'READ'\nSET bucket.anonymous_access = true, bucket.anonymous_actions = coalesce(bucket.anonymous_actions, []) + ['s3:ListBucket', 's3:ListBucketVersions', 's3:ListBucketMultipartUploads']", "iterative": false }, { - "query": "MATCH (acl:S3Acl)-[:APPLIES_TO]->(bucket:S3Bucket)<-[:RESOURCE]-(aws:AWSAccount{id: $AWS_ID})\nWHERE acl.uri IN ['http://acs.amazonaws.com/groups/global/AllUsers', 'http://acs.amazonaws.com/groups/global/AuthenticatedUsers'] AND acl.permission = 'WRITE'\nAND (acl.ownerid = acl.granteeid)\nSET bucket.anonymous_access = true, bucket.anonymous_actions = coalesce(bucket.anonymous_actions, []) + ['s3:DeleteObjectVersion']", + "__comment__": "WRITE -> PutObject", + "query": "MATCH (acl:S3Acl)-[:APPLIES_TO]->(bucket:S3Bucket)<-[:RESOURCE]-(aws:AWSAccount{id: $AWS_ID})\nWHERE acl.uri IN ['http://acs.amazonaws.com/groups/global/AllUsers', 'http://acs.amazonaws.com/groups/global/AuthenticatedUsers'] AND acl.permission = 'WRITE'\nSET bucket.anonymous_access = true, bucket.anonymous_actions = coalesce(bucket.anonymous_actions, []) + ['s3:PutObject']", "iterative": false }, { - "query": "MATCH (acl:S3Acl)-[:APPLIES_TO]->(bucket:S3Bucket)<-[:RESOURCE]-(aws:AWSAccount{id: $AWS_ID})\nWHERE acl.uri IN ['http://acs.amazonaws.com/groups/global/AllUsers', 'http://acs.amazonaws.com/groups/global/AuthenticatedUsers'] AND acl.permission = 'READ_ACP'\nSET bucket.anonymous_access = true, bucket.anonymous_actions = coalesce(bucket.anonymous_actions, []) + ['s3:DeleteObjectVersion']", + "__comment__": "READ_ACP -> GetBucketAcl", + "query": "MATCH (acl:S3Acl)-[:APPLIES_TO]->(bucket:S3Bucket)<-[:RESOURCE]-(aws:AWSAccount{id: $AWS_ID})\nWHERE acl.uri IN ['http://acs.amazonaws.com/groups/global/AllUsers', 'http://acs.amazonaws.com/groups/global/AuthenticatedUsers'] AND acl.permission = 'READ_ACP'\nSET bucket.anonymous_access = true, bucket.anonymous_actions = coalesce(bucket.anonymous_actions, []) + ['s3:GetBucketAcl']", "iterative": false }, { + "__comment__": "WRITE_ACP -> PutBucketAcl", "query": "MATCH (acl:S3Acl)-[:APPLIES_TO]->(bucket:S3Bucket)<-[:RESOURCE]-(aws:AWSAccount{id: $AWS_ID})\nWHERE acl.uri IN ['http://acs.amazonaws.com/groups/global/AllUsers', 'http://acs.amazonaws.com/groups/global/AuthenticatedUsers'] AND acl.permission = 'WRITE_ACP'\nSET bucket.anonymous_access = true, bucket.anonymous_actions = coalesce(bucket.anonymous_actions, []) + ['s3:PutBucketAcl']", "iterative": false }, { + "__comment__": "FULL_CONTROL -> Pretty much everything", "query": "MATCH (acl:S3Acl)-[:APPLIES_TO]->(bucket:S3Bucket)<-[:RESOURCE]-(aws:AWSAccount{id: $AWS_ID})\nWHERE acl.uri IN ['http://acs.amazonaws.com/groups/global/AllUsers', 'http://acs.amazonaws.com/groups/global/AuthenticatedUsers'] AND acl.permission = 'FULL_CONTROL'\nSET bucket.anonymous_access = true, bucket.anonymous_actions = coalesce(bucket.anonymous_actions, []) + ['s3:ListBucket', 's3:ListBucketVersions', 's3:ListBucketMultipartUploads', 's3:PutObject', 's3:DeleteObject', 's3:DeleteObjectVersion', 's3:PutBucketAcl']", "iterative": false }], diff --git a/cartography/intel/aws/s3.py b/cartography/intel/aws/s3.py index 6db11cc9fd..396c275926 100644 --- a/cartography/intel/aws/s3.py +++ b/cartography/intel/aws/s3.py @@ -222,7 +222,12 @@ def _is_common_exception(e: Exception, bucket: Dict) -> bool: @timeit -def _load_s3_acls(neo4j_session: neo4j.Session, acls: Dict, aws_account_id: str, update_tag: int) -> None: +def _load_s3_acls( + neo4j_session: neo4j.Session, + acls: List[Dict[str, Any]], + aws_account_id: str, + update_tag: int, +) -> None: """ Ingest S3 ACL into neo4j. """ diff --git a/tests/data/aws/s3.py b/tests/data/aws/s3.py index 4ad06c4e27..1793782d31 100644 --- a/tests/data/aws/s3.py +++ b/tests/data/aws/s3.py @@ -23,6 +23,69 @@ }, } +OPEN_BUCKET_ACLS = { + "bucket-1": { + "Owner": { + "DisplayName": "my-display-name-1", + "ID": "3cb8", + }, + "Grants": [ + { + "Grantee": { + "DisplayName": "my-display-name-1", + "ID": "3cb8", + "Type": "CanonicalUser", + }, + "Permission": "FULL_CONTROL", + }, + ], + }, + "bucket-2": { + "Owner": { + "DisplayName": "my-display-name-2", + "ID": "828a", + }, + "Grants": [ + { + "Grantee": { + "Type": "Group", + "URI": "http://acs.amazonaws.com/groups/global/AllUsers", + }, + "Permission": "READ", + }, + { + "Grantee": { + "Type": "Group", + "URI": "http://acs.amazonaws.com/groups/global/AuthenticatedUsers", + }, + "Permission": "READ_ACP", + }, + ], + }, + "bucket-3": { + "Owner": { + "DisplayName": "my-display-name-2", + "ID": "828a", + }, + "Grants": [ + { + "Grantee": { + "Type": "Group", + "URI": "http://acs.amazonaws.com/groups/global/AllUsers", + }, + "Permission": "WRITE_ACP", + }, + { + "Grantee": { + "Type": "Group", + "URI": "http://acs.amazonaws.com/groups/global/AuthenticatedUsers", + }, + "Permission": "WRITE", + }, + ], + }, +} + GET_ENCRYPTION = { 'bucket': 'bucket-1', 'default_encryption': True, diff --git a/tests/integration/cartography/intel/aws/test_s3_acls.py b/tests/integration/cartography/intel/aws/test_s3_acls.py new file mode 100644 index 0000000000..389d9d1bd9 --- /dev/null +++ b/tests/integration/cartography/intel/aws/test_s3_acls.py @@ -0,0 +1,51 @@ +from cartography.intel.aws.s3 import _load_s3_acls +from cartography.intel.aws.s3 import load_s3_buckets +from cartography.intel.aws.s3 import parse_acl +from tests.data.aws.s3 import LIST_BUCKETS +from tests.data.aws.s3 import OPEN_BUCKET_ACLS +from tests.integration.cartography.intel.aws.iam.test_iam import _create_base_account +from tests.integration.util import check_nodes + +TEST_ACCOUNT_ID = '000000000000' +TEST_REGION = 'us-east-1' +TEST_UPDATE_TAG = 123456789 + + +def test_load_s3_acls(neo4j_session): + """ + Ensure that the S3 ACL that sets anonymous_access and anonymous_actions works. + """ + # Arrange: hackily add s3 data to the test graph + _create_base_account(neo4j_session) + load_s3_buckets(neo4j_session, LIST_BUCKETS, TEST_ACCOUNT_ID, TEST_UPDATE_TAG) + parsed_acls = [] + for bucket_name, acl in OPEN_BUCKET_ACLS.items(): + acl = parse_acl(acl, bucket_name, TEST_ACCOUNT_ID) + if acl: + parsed_acls.extend(acl) + + # Act: run the analysis job + _load_s3_acls(neo4j_session, parsed_acls, TEST_ACCOUNT_ID, TEST_UPDATE_TAG) + + # Assert that the anonymous_access field is set as expected + assert check_nodes(neo4j_session, 'S3Bucket', ['name', 'anonymous_access']) == { + ('bucket-1', None), + ('bucket-3', True), + ('bucket-2', True), + } + + # 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.) + actual = neo4j_session.run( + """ + MATCH (r:S3Bucket) RETURN r.name, r.anonymous_actions; + """, + ) + actual_nodes = [ + (n['r.name'], sorted(n['r.anonymous_actions']) if n['r.anonymous_actions'] else []) for n in actual + ] + assert sorted(actual_nodes) == [ + ('bucket-1', []), + ('bucket-2', ['s3:GetBucketAcl', 's3:ListBucket', 's3:ListBucketMultipartUploads', 's3:ListBucketVersions']), + ('bucket-3', ['s3:PutBucketAcl', 's3:PutObject']), + ]