Skip to content
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

feat: Add support for cloud-connector to read kms encrypted logs in S3 from existing cloudtrail #125

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Chili-Man
Copy link

@Chili-Man Chili-Man commented Sep 2, 2022

This change allows using an existing KMS key (that may also be in another account) for cloud connector to read from an existing encrypted cloudtrail log bucket.

Checklist

  • If test/fixtures/*/main.tf files are modified. I have updated:
    • the snippets in the README.md file under root folder.
    • the snippets in the README.md file for the corresponding example.
  • If examples folder are modified. I have updated:
    • README.md file with pertinent changes.
    • test/fixtures/*/main.tf in case the snippet needs modifications.
  • If any architectural change has been made, I have updated the diagrams.

-->

Comment on lines -57 to -58
<li>cloudtrail_s3_sns_sqs_arn: Optional 2. ARN of the queue that will ingest events forwarded from an existing cloudtrail_s3_sns</li>
<li>cloudtrail_s3_sns_sqs_url: Optional 2. URL of the queue that will ingest events forwarded from an existing cloudtrail_s3_sns<br/>sqs:ReceiveMessage and sqs:DeleteMessage permissions have to be provided to the compute role</li>
Copy link
Contributor

@wideawakening wideawakening Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is bit messy, our fault :)

The idea with the "Optional X" is to group disjointed options. Currently there are two options (cloudtrail_sns OR cloudtrail_s3).

Because the cloudtrail_kms_arn configuration applies to both, I would add it in the beginning.
Let's see if changing 1/2 for CloudltrailSns/CloudtrailS3 makes it more clear.

<ul>
      <li>cloudtrail_kms_arn: Optional. ARN of a cloudtrail KMS key used for encrypting the logs. Required in order to retrieve the encrypted logs from S3</li>
      <li>cloudtrail_sns_arn: Optional CloudtrailSns. ARN of a cloudtrail-sns topic. If specified, deployment region must match Cloudtrail S3 bucket region</li>
      <li>cloudtrail_s3_sns_sqs_arn: Optional CloudtrailS3. ARN of the queue that will ingest events forwarded from an existing cloudtrail_s3_sns</li>
      <li>cloudtrail_s3_sns_sqs_url: Optional ClouldtrailS3. URL of the queue that will ingest events forwarded from an existing cloudtrail_s3_sns<br/>sqs:ReceiveMessage and sqs:DeleteMessage permissions have to be provided to the compute role</li>
</ul>

Copy link
Contributor

@wideawakening wideawakening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR @Chili-Man !
since you've been active recently, we're seeing if we can make you a collaborator in the repo, to speedup contributions

some notes

  1. aws_kms_key.cloudtrail_kms resource should not be created if the cloudtrail_kms_arn is provided. i would conditionate the creation of it to the cloudtrail_kms_arn variable not being null/"".
  2. as for the cloudtrail_kms_arn variable description, some minor text change if previous point makes sense to you
  < description = "ARN of a pre-existing KMS key for encrypting the Cloudtrail logs. Incompatible with var.cloudtrail_kms_enable when set to true"
  > description = "when `cloudtrail_kms_enable` is set to true, the ARN of a pre-existing KMS key for encrypting the Cloudtrail logs"
  1. corrections on existing_cloudtrail_config variable block
  2. non-blocking, but could you also update the single-account-k8s example too?

@Chili-Man Chili-Man requested a review from a team as a code owner September 8, 2022 23:10
@Chili-Man
Copy link
Author

@wideawakening Ive addressed all of your comments

@wideawakening
Copy link
Contributor

@wideawakening Ive addressed all of your comments

thanks! :)
need to prioritize this task to schedule some time for it and give it a proper bump, bear with us :D

because some org GH-level secrets would be accessible to your user if we make you collaborator seems like it's not gonna be feasible for the moment.

anyhow, we gotta work on some clarifications

  • what are the required permissions for the existing cloudtrail_kms_arn? this KMS will be used by cloudtrail-s3 in write time, and by cloud-connector on read time
  • can the kms_key live in any account or it has to be in the same account as where the cloud-trail lives?
  • think we could benefit from creating a test to validate this usage, at least in the happy path (same account)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants