Repository: cloud-custodian/cloud-custodian

Issue:
The s3 action "remove-statements" errors out when it encounters a bucket policy statement without a sid
**Describe the bug**
s3.remove-statements fails when a sid-less bucket policy statement is encountered

You can see the key error in the traceback. Bucket policy statements do not require Sids and S3 omits the key from describeBucketPolicy response when it does not exist.

**To Reproduce**
Attempt to use remove-statements to remove a statement from a bucket with a sid-less statement (one example of which is the "aws-sam-cli-managed-default-samclisourcebucket-..." buckets created  by AWS SAM CLI.)

**Expected behavior**
I expected the statement which does not contain a SID to be iterated over as non-matching.

**Background (please complete the following information):**
 - OS: AWS Lambda
 - Python Version:  Python 3.8
 - Custodian Version:  0.9.8
 - Tool Version: n/a
 - Cloud Provider: AWS
 - Policy: [please exclude any account/sensitive information]
```json
        {
          "statement_ids": [
            "denyAccessToBucket"
          ],
          "type": "remove-statements"
        }
```
 - Traceback: [if applicable, please exclude sensitive/account information]
 [ERROR] KeyError: 'Sid'
Traceback (most recent call last):
  File "/var/task/custodian_policy.py", line 4, in run
    return handler.dispatch_event(event, context)
  File "/var/task/c7n/handler.py", line 165, in dispatch_event
    p.push(event, context)
  File "/var/task/c7n/policy.py", line 1140, in push
    return mode.run(event, lambda_ctx)
  File "/var/task/c7n/policy.py", line 853, in run
    resources = super(ConfigRuleMode, self).run(event, lambda_context)
  File "/var/task/c7n/policy.py", line 453, in run
    return self.run_resource_set(event, resources)
  File "/var/task/c7n/policy.py", line 483, in run_resource_set
    results = action.process(resources)
  File "/var/task/c7n/resources/s3.py", line 1272, in process
    results += filter(None, [f.result()])
  File "/var/lang/lib/python3.8/concurrent/futures/_base.py", line 432, in result
    return self.__get_result()
  File "/var/lang/lib/python3.8/concurrent/futures/_base.py", line 388, in __get_result
    raise self._exception
  File "/var/lang/lib/python3.8/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/var/task/c7n/resources/s3.py", line 1282, in process_bucket
    statements, found = self.process_policy(
  File "/var/task/c7n/actions/policy.py", line 21, in process_policy
    return remove_statements(
  File "/var/task/c7n/actions/policy.py", line 37, in remove_statements
    elif s['Sid'] in match_ids:
 - `custodian version --debug` output: n/a

**Additional context**
Add any other context about the problem here.


Discussion:
anthonybgale: #6374 

Patch:
diff --git a/c7n/actions/policy.py b/c7n/actions/policy.py
index c7e2a9e02..833e1309e 100644
--- a/c7n/actions/policy.py
+++ b/c7n/actions/policy.py
@@ -31,7 +31,7 @@ def remove_statements(match_ids, statements, matched=()):
         elif match_ids == 'matched':
             if s in matched:
                 s_found = True
-        elif s['Sid'] in match_ids:
+        elif 'Sid' in s and s['Sid'] in match_ids:
             s_found = True
         if s_found:
             found.append(s)
diff --git a/c7n/resources/iam.py b/c7n/resources/iam.py
index 9ad247466..9758e0876 100644
--- a/c7n/resources/iam.py
+++ b/c7n/resources/iam.py
@@ -2258,6 +2258,43 @@ class IamGroupInlinePolicy(Filter):
         return res
 
 
+@Group.action_registry.register('delete-inline-policies')
+class GroupInlinePolicyDelete(BaseAction):
+    """Delete inline policies embedded in an IAM group.
+
+    :example:
+
+      .. code-block:: yaml
+
+        - name: iam-delete-group-policies
+          resource: aws.iam-group
+          filters:
+            - type: value
+              key: GroupName
+              value: test
+          actions:
+            - type: delete-inline-policies
+    """
+    schema = type_schema('delete-inline-policies')
+    permissions = ('iam:ListGroupPolicies', 'iam:DeleteGroupPolicy',)
+
+    def process(self, resources):
+        client = local_session(self.manager.session_factory).client('iam')
+        for r in resources:
+            self.process_group(client, r)
+
+    def process_group(self, client, r):
+        if 'c7n:InlinePolicies' not in r:
+            r['c7n:InlinePolicies'] = client.list_group_policies(
+                GroupName=r['GroupName'])['PolicyNames']
+        for policy in r.get('c7n:InlinePolicies', []):
+            try:
+                self.manager.retry(client.delete_group_policy,
+                    GroupName=r['GroupName'], PolicyName=policy)
+            except client.exceptions.NoSuchEntityException:
+                continue
+
+
 @Group.action_registry.register('delete')
 class UserGroupDelete(BaseAction):
     """Delete an IAM User Group.
diff --git a/tools/c7n_gcp/c7n_gcp/mu.py b/tools/c7n_gcp/c7n_gcp/mu.py
index 4decc5563..352956574 100644
--- a/tools/c7n_gcp/c7n_gcp/mu.py
+++ b/tools/c7n_gcp/c7n_gcp/mu.py
@@ -1,6 +1,7 @@
 # Copyright The Cloud Custodian Authors.
 # SPDX-License-Identifier: Apache-2.0
 
+import base64
 from collections import namedtuple
 import json
 import logging
@@ -470,7 +471,7 @@ class PubSubSource(EventSource):
 
         client.execute_command('setIamPolicy', {'resource': topic, 'body': {'policy': policy}})
 
-    def add(self):
+    def add(self, func):
         self.ensure_topic()
 
     def remove(self):
@@ -590,6 +591,7 @@ class PeriodicEvent(EventSource):
         elif self.target_type == 'pubsub':
             job['pubsubTarget'] = {
                 'topicName': target.get_topic_param(),
+                'data': base64.b64encode("{\"schedule\": true}".encode('utf-8')).decode('utf-8'),
             }
         return job
 
diff --git a/tools/c7n_gcp/c7n_gcp/policy.py b/tools/c7n_gcp/c7n_gcp/policy.py
index c1ec21dee..fd15c5ff6 100644
--- a/tools/c7n_gcp/c7n_gcp/policy.py
+++ b/tools/c7n_gcp/c7n_gcp/policy.py
@@ -64,7 +64,7 @@ class PeriodicMode(FunctionMode, PullMode):
         'gcp-periodic',
         rinherit=FunctionMode.schema,
         required=['schedule'],
-        **{'trigger-type': {'enum': ['http', 'pubsub']},
+        **{'target-type': {'enum': ['http', 'pubsub']},
            'tz': {'type': 'string'},
            'schedule': {'type': 'string'}})
 
diff --git a/tools/c7n_gcp/readme.md b/tools/c7n_gcp/readme.md
index 65c9f6a34..fcf9322f7 100644
--- a/tools/c7n_gcp/readme.md
+++ b/tools/c7n_gcp/readme.md
@@ -29,4 +29,12 @@ credentials, which will be picked up via by the custodian cli via setting the
 
 # Serverless
 
-Custodian supports both periodic and api call events for serverless policy execution.
+Custodian supports both periodic and api call events for serverless
+policy execution.
+
+GCP Cloud Functions require cloudbuild api be enabled on the project
+the functions are deployed to.
+
+Periodic execution mode also requires cloudscheduler api be enabled on
+a project. Cloudscheduler usage also requires an app engine instance
+in the same region as the function deployment.
diff --git a/tools/c7n_policystream/policystream.py b/tools/c7n_policystream/policystream.py
index 30c46f811..add736b07 100644
--- a/tools/c7n_policystream/policystream.py
+++ b/tools/c7n_policystream/policystream.py
@@ -399,11 +399,7 @@ class PolicyRepo:
                     current_policies += self.policy_files[f]
             elif delta.status == GIT_DELTA_INVERT['GIT_DELTA_DELETED']:
                 if f in self.policy_files:
-                    # if the policies were moved, only add in policies
-                    # that are not already accounted for.
-                    current_policies += self.policy_files[f].select(
-                        set(current_policies.keys()).difference(
-                            self.policy_files[f].keys()))
+                    current_policies += self.policy_files[f]
                     removed.add(f)
             elif delta.status == GIT_DELTA_INVERT['GIT_DELTA_RENAMED']:
                 change_policies += self._policy_file_rev(f, change)


Test patch:
diff --git a/tests/data/placebo/test_iam_group_delete_inline_policies/iam.DeleteGroupPolicy_1.json b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.DeleteGroupPolicy_1.json
new file mode 100644
index 000000000..5b2170a07
--- /dev/null
+++ b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.DeleteGroupPolicy_1.json
@@ -0,0 +1,6 @@
+{
+    "status_code": 200,
+    "data": {
+        "ResponseMetadata": {}
+    }
+}
\ No newline at end of file
diff --git a/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_1.json b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_1.json
new file mode 100644
index 000000000..d2164b637
--- /dev/null
+++ b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_1.json
@@ -0,0 +1,10 @@
+{
+    "status_code": 200,
+    "data": {
+        "PolicyNames": [
+            "test"
+        ],
+        "IsTruncated": false,
+        "ResponseMetadata": {}
+    }
+}
\ No newline at end of file
diff --git a/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_2.json b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_2.json
new file mode 100644
index 000000000..02a3e7bf2
--- /dev/null
+++ b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_2.json
@@ -0,0 +1,8 @@
+{
+    "status_code": 200,
+    "data": {
+        "PolicyNames": [],
+        "IsTruncated": false,
+        "ResponseMetadata": {}
+    }
+}
\ No newline at end of file
diff --git a/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_3.json b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_3.json
new file mode 100644
index 000000000..02a3e7bf2
--- /dev/null
+++ b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroupPolicies_3.json
@@ -0,0 +1,8 @@
+{
+    "status_code": 200,
+    "data": {
+        "PolicyNames": [],
+        "IsTruncated": false,
+        "ResponseMetadata": {}
+    }
+}
\ No newline at end of file
diff --git a/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroups_1.json b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroups_1.json
new file mode 100644
index 000000000..1ab4b4791
--- /dev/null
+++ b/tests/data/placebo/test_iam_group_delete_inline_policies/iam.ListGroups_1.json
@@ -0,0 +1,41 @@
+{
+    "status_code": 200,
+    "data": {
+        "Groups": [
+            {
+                "Path": "/",
+                "GroupName": "test1",
+                "GroupId": "AGPAY5Y4D45RFZP34ODAF",
+                "Arn": "arn:aws:iam::644160558196:group/test1",
+                "CreateDate": {
+                    "__class__": "datetime",
+                    "year": 2020,
+                    "month": 12,
+                    "day": 15,
+                    "hour": 13,
+                    "minute": 34,
+                    "second": 34,
+                    "microsecond": 0
+                }
+            },
+            {
+                "Path": "/",
+                "GroupName": "test2",
+                "GroupId": "AGPAY5Y4D45RJTOJYVPSR",
+                "Arn": "arn:aws:iam::644160558196:group/test2",
+                "CreateDate": {
+                    "__class__": "datetime",
+                    "year": 2020,
+                    "month": 12,
+                    "day": 15,
+                    "hour": 13,
+                    "minute": 34,
+                    "second": 41,
+                    "microsecond": 0
+                }
+            }
+        ],
+        "IsTruncated": false,
+        "ResponseMetadata": {}
+    }
+}
\ No newline at end of file
diff --git a/tests/test_iam.py b/tests/test_iam.py
index d9b8f99b5..2c916b613 100644
--- a/tests/test_iam.py
+++ b/tests/test_iam.py
@@ -1338,6 +1338,27 @@ class IamInlinePolicyUsage(BaseTest):
         self.assertEqual(len(resources), 2)
         self.assertFalse(resources[0]["c7n:InlinePolicies"])
 
+    def test_iam_group_delete_inline_policies(self):
+        session_factory = self.replay_flight_data("test_iam_group_delete_inline_policies")
+        p = self.load_policy(
+            {
+                "name": "iam-delete-group-policies",
+                "resource": "aws.iam-group",
+                "actions": [{
+                    "type": "delete-inline-policies"
+                }],
+            },
+            session_factory=session_factory,
+        )
+        resources = {r["GroupName"]: r for r in p.run()}
+        noncompliant_group = "test1"
+        self.assertIn(noncompliant_group, resources)
+        self.assertEqual(len(resources[noncompliant_group]["c7n:InlinePolicies"]), 1)
+
+        client = session_factory().client("iam")
+        inline_policies_after = client.list_group_policies(GroupName=noncompliant_group)
+        self.assertEqual(len(inline_policies_after["PolicyNames"]), 0)
+
 
 class KMSCrossAccount(BaseTest):
 
diff --git a/tools/c7n_policystream/tests/test_policystream.py b/tools/c7n_policystream/tests/test_policystream.py
index ee8795e0e..e4b107b5c 100644
--- a/tools/c7n_policystream/tests/test_policystream.py
+++ b/tools/c7n_policystream/tests/test_policystream.py
@@ -5,6 +5,7 @@ import json
 import subprocess
 import os
 import yaml
+import logging
 
 import pytest
 
@@ -85,6 +86,10 @@ class GitRepo:
 @pytest.mark.skipif(pygit2 is None, reason="pygit2 not installed")
 class StreamTest(TestUtils):
 
+    def setUp(self):
+        self.maxDiff = None
+        logging.getLogger("").setLevel(logging.DEBUG)
+
     def setup_basic_repo(self):
         git = GitRepo(self.get_temp_dir())
         git.init()
@@ -187,3 +192,91 @@ class StreamTest(TestUtils):
             rows[-1]['policy'],
             {'data': {'name': 'lambda-check', 'resource': 'aws.lambda'},
              'file': 'example.yml'})
+
+    def test_stream_remove_file(self):
+        git = self.setup_basic_repo()
+        git.rm('example.yml')
+        git.commit('remove file')
+
+        policy_repo = policystream.PolicyRepo(git.repo_path, git.repo())
+        changes = [c.data() for c in policy_repo.delta_stream(
+            sort=pygit2.GIT_SORT_TOPOLOGICAL | pygit2.GIT_SORT_REVERSE)]
+        self.assertEqual(
+            [(c['change'],
+              c['policy']['data']['name'],
+              c['commit']['message'].strip()) for c in changes],
+            [('add', 'codebuild-check', 'add something'),
+             ('remove', 'codebuild-check', 'switch'),
+             ('add', 'lambda-check', 'switch'),
+             ('remove', 'lambda-check', 'remove file')])
+
+    def test_stream_move_subdir(self):
+        git = GitRepo(self.get_temp_dir())
+        git.init()
+        git.change('aws/ec2.yml', {'policies': [
+            {'name': 'ec2-check',
+             'resource': 'aws.ec2'}]})
+        git.change('lambda.yml', {'policies': [
+            {'name': 'lambda-check',
+             'resource': 'aws.lambda'}]})
+        git.commit('init')
+        git.move('lambda.yml', 'aws/lambda.yml')
+        git.change('aws/ec2.yml', {'policies': [
+            {'name': 'ec2-check',
+             'resource': 'aws.ec2'},
+            {'name': 'ec2-ami-check',
+             'resource': 'aws.ec2'}]})
+        git.commit('move')
+        git.rm('aws/ec2.yml')
+        git.rm('aws/lambda.yml')
+        git.change('aws/all.yml', {'policies': [
+            {'name': 'lambda-check',
+             'resource': 'aws.lambda'},
+            {'name': 'ec2-check',
+             'resource': 'aws.ec2'}]})
+        git.commit('consolidate')
+        policy_repo = policystream.PolicyRepo(git.repo_path, git.repo())
+        changes = [c.data() for c in policy_repo.delta_stream(
+            sort=pygit2.GIT_SORT_TOPOLOGICAL | pygit2.GIT_SORT_REVERSE)]
+        self.assertEqual(
+            [(c['change'],
+              c['policy']['data']['name'],
+              c['commit']['message'].strip()) for c in changes],
+            [('add', 'ec2-check', 'init'),
+             ('add', 'lambda-check', 'init'),
+             ('add', 'ec2-ami-check', 'move'),
+             ('moved', 'lambda-check', 'move'),
+             ('remove', 'ec2-ami-check', 'consolidate'),
+             ('moved', 'ec2-check', 'consolidate'),
+             ('moved', 'lambda-check', 'consolidate')]
+        )
+
+    def test_stream_move_policy(self):
+        git = self.setup_basic_repo()
+        git.change('newfile.yml', {
+            'policies': [{
+                'name': 'ec2-check',
+                'resource': 'aws.ec2'}]})
+        git.commit('new file')
+        git.rm('example.yml')
+        git.change('newfile.yml', {
+            'policies': [{
+                'name': 'ec2-check',
+                'resource': 'aws.ec2',
+            }, {
+                'name': 'lambda-check',
+                'resource': 'aws.lambda'}]})
+        git.commit('move policy')
+
+        policy_repo = policystream.PolicyRepo(git.repo_path, git.repo())
+        changes = [c.data() for c in policy_repo.delta_stream(
+            sort=pygit2.GIT_SORT_TOPOLOGICAL | pygit2.GIT_SORT_REVERSE)]
+        self.assertEqual(
+            [(c['change'],
+              c['policy']['data']['name'],
+              c['commit']['message'].strip()) for c in changes],
+            [('add', 'codebuild-check', 'add something'),
+             ('remove', 'codebuild-check', 'switch'),
+             ('add', 'lambda-check', 'switch'),
+             ('add', 'ec2-check', 'new file'),
+             ('moved', 'lambda-check', 'move policy')])