Allow GuardDuty to write to AWS logging bucket

[chrisgilmerproj]

Replaces #54 and #63

@pjdufour-dds I'm not entirely clear on why #63 was closed by @dynamike by looking at the PR comments. However, we still use this code and I'd love to have it implemented. I've gotten everything up to date and it appears to work fine with our infra. I'm happy to take over taking this forward.

[chtakahashi]

Hi @chrisgilmerproj, I'm having a look at this right now. The functionality you're proposing looks reasonable, so we'll do some of our own testing. Will tag you in the PR when its ready!

[chtakahashi]

Hi @chrisgilmerproj, I've spent some time working through the changes you proposed and noted the following:

• The terraform configuration in examples/guardduty has the aws_logs module referred to as simply 'logs'.
• when I fix the above, I get an error indicating that I should use the -target flag when applying.
• To avoid this error, I grabbed all the contents of "dod-iac/guardduty/aws" and eliminated the usage of count by directly referencing the S3 bucket

The resulting errors I get are related to there being a pre-existing guardduty detector (which is fine, we are aware this is the case for our CI account), and a 403 Access Denied error coming from an attempt to put an object into the specified S3 bucket. We have looked at the AWS documentation surrounding granting GuardDuty permissions to a bucket, and noted some permissions differences from what you are proposing, namely the lack of condition statements to specify source account and the specific detector. Have you encountered any of these issues when working with the changes you've suggested?

[chrisgilmerproj]

@chtakahashi I have not run into the issues you are seeing. Unfortunately we don't have a CI system set up for this so for those errors I'll have to lean on you. When it comes to the granting permissions, we haven't had errors but if there's a recommended way from AWS then I would suggest we go with that and you have my permission to update the policy. For reference, we run this on all of our commercial and govcloud accounts and haven't had any issues that I'm aware of.

[chrisgilmerproj]

@chtakahashi - what do you mean by:

when I fix the above, I get an error indicating that I should use the -target flag when applying.

Can you paste an image of the error you are seeing? Also, can you include the command you used for testing?

[chrisgilmerproj]

@chtakahashi

a 403 Access Denied error coming from an attempt to put an object into the specified S3 bucket.

If you want the bucket to allow you to put in objects manually I think you need to configure it that way and modify the prefix. I think the way that the policy is set up ONLY the guardduty service is allowed to place an object, not users (unless you're logged in as the root user, which I imagine you are not).

Can you hook up your own detector and see if it will save resource logs? I'm fine if you want to copy the contents of our guardduty module to place in this example directly in order to make things simpler.

[chrisgilmerproj]

@chtakahashi - Refering to this comment:

namely the lack of condition statements to specify source account and the specific detector.

This is really up to how you want to create the module. In the current construction it allows you to create the bucket first, then enable guardduty, and later add the detector. I think you're proposing that you'd prefer to lock it down by forcing folks to first set up the detector and then later set up the bucket with the policy.

Additionally, if you wanted to allow multiple detectors to log you would need a new policy statement for each detector ID you included.

As a possible solution you could let the default set of detector IDs be [], in which case no conditions are created. OR you could choose to have it be a required list and folks can put in [*] if they so choose. Lastly, folks could choose to be more specific with the ARNs if they so choose.

Please let me know how you want to proceed on that front and I'll do it.

[chrisgilmerproj]

@chtakahashi - I found the problem you had with the count on the guardduty resource. It seems to have issues when you run terraform plan because it can't determine until runtime if it should make those resources. But if you apply those resources its fine. Unfortunately its not something I can fix right away but at least it explains the behavior.

[dynamike]

After a bit of back and forth about the value of adding GuardDuty findings to this module, it still feels like adding this functionality is a bit different than the other use cases we have today. It basically boiled down to the fact that this module is primarily used for AWS service logs and AWS GuardDuty findings are specifically security related and distinctly different from general AWS service logs (e.g., ALBs). If something around this were to exist, it may make sense to keep it apart of a more general purpose AWS GuardDuty module like what you have the dod-iac/guardduty module.