Skip to content

Conversation

@emaincourt
Copy link
Contributor

@emaincourt emaincourt commented Nov 4, 2025

what

  • Allow to set both allow_ssl_requests_only and allow_encrypted_uploads_only in order to configure it at the underlying S3 bucket level.

why

  • We must enable secured traffic for compliance purposes.

references

  • N/A

Summary by CodeRabbit

  • New Features
    • Added allow_ssl_requests_only configuration option to enforce SSL-only requests
    • Added allow_encrypted_uploads_only configuration option to require encrypted uploads

@emaincourt emaincourt requested a review from a team as a code owner November 4, 2025 14:55
@emaincourt emaincourt requested a review from dudymas November 4, 2025 14:55
@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Walkthrough

This pull request adds two new boolean configuration variables to the Terraform module: allow_ssl_requests_only and allow_encrypted_uploads_only. These variables are defined in variables.tf with default values of false, and their corresponding module arguments are forwarded in the tailscale_subnet_router module block in main.tf. The changes introduce security-related configuration options without modifying existing logic or control flow.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify that the tailscale_subnet_router module block properly accepts and handles these two new parameters
  • Confirm variable descriptions and naming conventions align with existing patterns in the codebase
  • Validate that default values (false for both) represent the intended secure-by-default behavior

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding configuration options to tune S3 bucket traffic security policies (allow_ssl_requests_only and allow_encrypted_uploads_only).
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7de2d2c and 00e081d.

📒 Files selected for processing (2)
  • 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.tf
  • main.tf
🧠 Learnings (5)
📓 Common learnings
Learnt from: gberenice
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: modules/spacelift-automation/variables.tf:0-0
Timestamp: 2024-10-30T17:11:38.812Z
Learning: In the Terraform module's `variables.tf` file, the variable `autodeploy` defaulting to `true` is acceptable since it matches the default provider value.
📚 Learning: 2024-10-30T16:46:36.813Z
Learnt from: gberenice
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: modules/spacelift-automation/variables.tf:0-0
Timestamp: 2024-10-30T16:46:36.813Z
Learning: For the variable `terraform_smart_sanitization` in `variables.tf`, prefer to keep the default value as `false`.

Applied to files:

  • variables.tf
📚 Learning: 2024-10-30T17:11:38.812Z
Learnt from: gberenice
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: modules/spacelift-automation/variables.tf:0-0
Timestamp: 2024-10-30T17:11:38.812Z
Learning: In the Terraform module's `variables.tf` file, the variable `autodeploy` defaulting to `true` is acceptable since it matches the default provider value.

Applied to files:

  • variables.tf
📚 Learning: 2025-01-30T16:39:53.021Z
Learnt from: oycyc
Repo: masterpointio/terraform-spacelift-events-collector-audit-trail PR: 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
Repo: masterpointio/terraform-spacelift-events-collector-audit-trail PR: 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.tf
🔇 Additional comments (2)
variables.tf (1)

73-83: Variable definitions look good, but consider security-by-default for compliance features.

The two new boolean variables follow the established conventions in this file with clear descriptions referencing AWS policy conditions. However, since these are compliance-related security controls, defaulting to false (non-enforcing) means operators must explicitly enable them. For compliance workloads, you may want to reconsider whether these should default to true or provide explicit guidance in documentation/comments about when these should be enabled.

You should verify that the downstream module (masterpointio/ssm-agent/aws v1.8.0) actually accepts these parameters. If these parameters aren't yet supported by that module version, the Terraform plan will fail with an unknown variable error.

main.tf (1)

56-57: Module arguments verified and properly forwarded.

The allow_ssl_requests_only parameter enforces HTTPS-only requests to the S3 bucket, while allow_encrypted_uploads_only blocks unencrypted object uploads—both are legitimate security controls for the session logging infrastructure. Your implementation correctly sources these variables with consistent formatting.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@oycyc oycyc left a comment

Choose a reason for hiding this comment

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

Looks good.

This was supported by the PR in the upstream module: masterpointio/terraform-aws-ssm-agent#57

@oycyc oycyc merged commit 5c0451b into masterpointio:main Nov 4, 2025
5 checks passed
@oycyc
Copy link
Contributor

oycyc commented Nov 4, 2025

Thanks @emaincourt -- available as v1.11.0

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