New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to play nice with an existing bucket policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, i'd like to see the delta diff on trail pulled out into a separate method, so the diff for update args computation can be tested more fully sans api usage.
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.