feat: support enforcing SSL and encrypted uploads for logs #57
Conversation
| variable "allow_ssl_requests_only" { | ||
| description = "Whether or not to allow SSL requests only. If set to `true` this will create a bucket policy that `Deny` if SSL is not used in the requests using the `aws:SecureTransport` condition." | ||
| type = bool | ||
| default = false | ||
| } | ||
|
|
||
| variable "allow_encrypted_uploads_only" { | ||
| description = "Whether or not to allow encrypted uploads only. If set to `true` this will create a bucket policy that `Deny` if encryption header is missing in the requests." | ||
| type = bool | ||
| default = false | ||
| } | ||
|
|
There was a problem hiding this comment.
Does turning these off fix the log uploading issue? Or is this for client's Vanta requirements?
There was a problem hiding this comment.
@Gowiem
1.) They are false by default already. The upstream CloudPosse S3 bucket module has these false as default. So zero diff for our module consumers as well if they upgrade.
2.) This is for client's Vanta requirement. Planning to use this to set to true in the root module consuming this.
WalkthroughThe change adds two new boolean inputs to control S3 bucket policies for session logging: allow_ssl_requests_only and allow_encrypted_uploads_only. These variables are defined in variables.tf and passed through to the logs_bucket module in main.tf. README.md is updated to document the new inputs, adjust provider/module versions, replace an S3 bucket data source with VPC/subnet-related data sources, and change several networking inputs from required to optional while adding subnet_names and vpc_name. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
main.tf (3)
201-206: Heads-up: flags won’t apply when a BYO bucket name is supplied.Because
enabled = local.logs_bucket_enabledis false whenvar.session_logging_bucket_nameis set, these enforcement flags won’t be enforced on an external bucket. Consider documenting this explicitly (and/or adding an optional policy path for BYO buckets in a follow-up).
If you want, I can draft a minimalaws_s3_bucket_policyexample conditioned onsession_logging_bucket_name != "".
201-206: Consider enabling S3 Bucket Keys to reduce KMS costs.Adding
bucket_key_enabled = truetypically lowers KMS request costs for SSE-KMS buckets with continuous writes (logs). Apply like this:# Encryption / Security sse_algorithm = "aws:kms" kms_master_key_arn = local.session_logging_kms_key_arn + bucket_key_enabled = true allow_ssl_requests_only = var.allow_ssl_requests_only allow_encrypted_uploads_only = var.allow_encrypted_uploads_only force_destroy = true
201-206: Double-check semantics of “encryption disabled.”Even if
session_logging_encryption_enabled = false, this block still configures SSE-KMS (with AWS-managed key ifkms_master_key_arnis empty). If that’s intentional, great—just ensure README language reflects that the S3 bucket is always encrypted at rest. Otherwise, gatesse_algorithmon the variable.variables.tf (1)
185-196: Optional: clarify scope in descriptions.To avoid confusion with BYO buckets, append a note like “Only applies when this module creates the logs bucket (when session_logging_bucket_name is empty).”
README.md (1)
141-143: Clarify when these inputs take effect.Suggest amending both rows to note they only apply when this module creates the S3 bucket (i.e., when
session_logging_bucket_nameis not set). This prevents surprises for BYO buckets.-| allow_encrypted_uploads_only | Whether or not to allow encrypted uploads only. If set to `true` this will create a bucket policy that `Deny` if encryption header is missing in the requests. | `bool` | `false` | no | +| allow_encrypted_uploads_only | Require encrypted uploads to the session logs bucket. If `true`, a bucket policy denies requests missing SSE headers. Note: applies only when this module creates the bucket (when `session_logging_bucket_name` is empty). | `bool` | `false` | no | -| allow_ssl_requests_only | Whether or not to allow SSL requests only. If set to `true` this will create a bucket policy that `Deny` if SSL is not used in the requests using the `aws:SecureTransport` condition. | `bool` | `false` | no | +| allow_ssl_requests_only | Require SSL (HTTPS) requests to the session logs bucket. If `true`, a bucket policy denies non-SSL requests via `aws:SecureTransport`. Note: applies only when this module creates the bucket (when `session_logging_bucket_name` is empty). | `bool` | `false` | no |
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
README.md(4 hunks)main.tf(1 hunks)variables.tf(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tf
⚙️ CodeRabbit configuration file
**/*.tf: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.org/docs/.
You have a strong grasp of Terraform syntax and prioritize providing accurate and insightful code suggestions.
As a fan of the Cloud Posse / SweetOps ecosystem, you incorporate many of their best practices https://docs.cloudposse.com/best-practices/terraform/ while balancing them with general Terraform guidelines.
Files:
variables.tfmain.tf
🧠 Learnings (4)
📓 Common learnings
Learnt from: oycyc
PR: masterpointio/terraform-spacelift-events-collector-audit-trail#2
File: s3.tf:2-14
Timestamp: 2025-01-30T16:39:53.021Z
Learning: The CloudPosse S3 bucket module (cloudposse/s3-bucket/aws) comes with secure defaults including versioning, private ACL, blocked public access, and server-side encryption. These don't need to be explicitly specified unless overriding the defaults.
Learnt from: oycyc
PR: masterpointio/terraform-spacelift-events-collector-audit-trail#2
File: s3.tf:2-14
Timestamp: 2025-01-30T16:39:53.021Z
Learning: The CloudPosse S3 bucket module (cloudposse/s3-bucket/aws) comes with secure defaults: versioning is enabled by default (versioning_enabled=true), ACL is private by default (acl="private"), and public access is blocked by default (block_public_acls=true, block_public_policy=true, ignore_public_acls=true). These don't need to be explicitly specified unless overriding the defaults.
📚 Learning: 2025-01-30T16:39:53.021Z
Learnt from: oycyc
PR: masterpointio/terraform-spacelift-events-collector-audit-trail#2
File: s3.tf:2-14
Timestamp: 2025-01-30T16:39:53.021Z
Learning: The CloudPosse S3 bucket module (cloudposse/s3-bucket/aws) comes with secure defaults: versioning is enabled by default (versioning_enabled=true), ACL is private by default (acl="private"), and public access is blocked by default (block_public_acls=true, block_public_policy=true, ignore_public_acls=true). These don't need to be explicitly specified unless overriding the defaults.
Applied to files:
variables.tf
📚 Learning: 2025-01-30T16:39:53.021Z
Learnt from: oycyc
PR: masterpointio/terraform-spacelift-events-collector-audit-trail#2
File: s3.tf:2-14
Timestamp: 2025-01-30T16:39:53.021Z
Learning: The CloudPosse S3 bucket module (cloudposse/s3-bucket/aws) comes with secure defaults including versioning, private ACL, blocked public access, and server-side encryption. These don't need to be explicitly specified unless overriding the defaults.
Applied to files:
variables.tfmain.tf
📚 Learning: 2024-11-18T13:00:36.189Z
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-aws-integrations#2
File: versions.tf:5-8
Timestamp: 2024-11-18T13:00:36.189Z
Learning: If a Terraform module does not include any AWS resources, there's no need to declare the AWS provider.
Applied to files:
README.md
🔇 Additional comments (6)
main.tf (1)
201-206: Inputs correctly wired to Cloud Posse module (supports SSL-only and encrypted uploads).
allow_ssl_requests_onlyandallow_encrypted_uploads_onlyare valid inputs in cloudposse/terraform-aws-s3-bucket and behave as intended. Nice upgrade alongside v4.10.0. (docs.cloudposse.com, github.com)variables.tf (1)
185-196: New booleans look good and match upstream module inputs.Descriptions are clear, types/defaults are sane, and they align with Cloud Posse’s variables. (docs.cloudposse.com)
README.md (4)
95-95: Provider table change is fine (terraform provider shows up when using terraform_data).
102-102: Module bump to s3-bucket v4.10.0 aligns with new inputs.
124-124: Resource list updates look consistent with code (terraform_data and networking data sources).
184-186: Networking input doc tweaks LGTM.Switching to optional
subnet_idsand addingsubnet_names/vpc_namematches main.tf resolution logic.Also applies to: 190-191
🤖 I have created a release *beep* *boop* --- ## [1.8.0](v1.7.0...v1.8.0) (2025-09-05) ### Features * support enforcing SSL and encrypted uploads for logs ([#57](#57)) ([1d29672](1d29672)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: masterpointbot[bot] <177651640+masterpointbot[bot]@users.noreply.github.com>
Support enforcing SSL and encrypted uploads for the log bucket.
Even though they are not sensitive or critical, there are cases when there are requirements or rules for the sake of rules, for example, SOC2 compliance.
Summary by CodeRabbit
New Features
Documentation
Chores