enable CloudTrail action #881
Conversation
|
actions now require a permissions tuple describing the permissions they need. |
c7n/resources/cloudtrail.py
Outdated
| policy_json = policy_json_tmpl.format( | ||
| bucket_name=bucket_name, account_id=account_id, | ||
| ) | ||
| s3client.put_bucket_policy(Bucket=bucket_name, Policy=policy_json) |
There was a problem hiding this comment.
this needs to play nice with an existing bucket policy
There was a problem hiding this comment.
The bucket was created one line earlier, so existing policy shouldn’t be a concern. Is it possible to define a default policy for newly created buckets? That’s the only way I could see it already having one.
There was a problem hiding this comment.
thats fair, but the consideration is that we'll be doing this to trails back into place if their deleted maliciously, in which case the bucket may already exist and we're just re-establishing the connection and the bucket may already exist (aka try/except on bucket exists).
There was a problem hiding this comment.
If a trail is deleted, it won’t be passed to the process() method, so we won’t have any metadata about it (like which bucket it was attached to). The bucket here is completely made up based on the name of the trail. (I suppose a collision is still possible, so we can test for existing.)
Should we allow (require?) a bucket name for each trail in the policy so the code doesn’t have to guess?
There was a problem hiding this comment.
good point.. so the end goal is to have something like 'ensure' trail, such that no matter what an active trail is running, that we can trigger off events that modify or disable trails or for initial account setup.
c7n/resources/cloudtrail.py
Outdated
| s3client.put_bucket_policy(Bucket=bucket_name, Policy=policy_json) | ||
| new_trail = client.create_trail( | ||
| Name=trail_name, | ||
| S3BucketName=bucket_name, |
There was a problem hiding this comment.
the other options here need to be exposed for config or defaulted (global events, multi-region, kms key, etc).
c7n/resources/cloudtrail.py
Outdated
|
|
||
|
|
||
| @actions.register('enable') | ||
| class EnableTrail(BaseAction): |
There was a problem hiding this comment.
actions and filters need iam permissions as class attribute
* action limited to one trail * creates bucket if needed * preserves existing policy on bucket
|
This has been updated to support the various options. Like I mentioned on the call, the coverage didn’t match my expectations. I ran through in the debugger and at least two of the lines that supposedly aren’t getting tested are getting called when tests run, so I’m chalking it up to weirdness in the coverage package and submitting this as is. |
|
Updates based on the last discussion:
|
|
branch updated and pull request resubmitted in #1098 |
Another stab at #339. This will enable the trail(s) named in the policy, or create then enable them (and a dedicated bucket) if they don’t exist.