chore: allow access to RDS from any source #122
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes update Terraform configurations and GitHub workflows related to the RDS instance and infrastructure management. The networking module adds an ingress rule allowing inbound TCP traffic on port 5432 from any IP address. The storage module sets the RDS instance as publicly accessible. A new output variable 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deployment/modules/networking/main.tf(1 hunks)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: test / Unit and Integration Tests
| # Allow access from anywhere (0.0.0.0/0) | ||
| ingress { | ||
| from_port = 5432 | ||
| to_port = 5432 | ||
| protocol = "tcp" | ||
| cidr_blocks = ["0.0.0.0/0"] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Critical security risk: Public RDS exposure
Allowing 0.0.0.0/0 ingress on your RDS security group opens your database to the entire internet, greatly increasing the risk of unauthorized access and data exfiltration. At minimum, you should restrict access to known IP ranges, a bastion host, or VPN. Better yet, parameterize public access so it can be safely disabled in production.
Example refactor to make public ingress optional via a variable:
@@ -74,7 +74,12 @@ resource "aws_security_group" "rds" {
- ingress {
- from_port = 5432
- to_port = 5432
- protocol = "tcp"
- cidr_blocks = ["0.0.0.0/0"]
+ # Public access controlled via variable
+ ingress {
+ from_port = 5432
+ to_port = 5432
+ protocol = "tcp"
+ cidr_blocks = var.rds_public_cidr_blocks
+ description = "Optional public RDS access"
}Add to your variables file:
variable "rds_public_cidr_blocks" {
type = list(string)
description = "CIDR blocks allowed to access RDS publicly; leave empty to disable public access"
default = []
}🤖 Prompt for AI Agents
In deployment/modules/networking/main.tf lines 74 to 80, the security group rule
allows ingress from 0.0.0.0/0, exposing the RDS instance to the entire internet,
which is a major security risk. To fix this, replace the hardcoded cidr_blocks
value with a variable that controls public access, and set it to an empty list
by default to disable public ingress. Update your variable definitions
accordingly to make the access configurable and safer for production
environments.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deployment/modules/storage/main.tf(1 hunks)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: e2e-tests / E2E Tests
| @@ -23,7 +23,7 @@ module "rds" { | |||
|
|
|||
| maintenance_window = "Mon:00:00-Mon:03:00" | |||
|
|
|||
| publicly_accessible = false | |||
| publicly_accessible = true | |||
There was a problem hiding this comment.
Critical Security Concern: Exposing RDS to the public Internet
Setting publicly_accessible = true opens your database endpoint to the Internet. Combined with an ingress rule of 0.0.0.0/0 on port 5432, this exposes sensitive data to anyone and violates security best practices. Please consider reverting this change or scoping it behind a controlled variable and locking down CIDRs.
Suggested refactor:
- publicly_accessible = true
+ publicly_accessible = var.rds_publicly_accessibleAnd in variables.tf:
+variable "rds_publicly_accessible" {
+ description = "Whether the RDS instance is publicly accessible"
+ type = bool
+ default = false
+}Additionally, enforce tighter network controls (whitelisted CIDRs or VPN/bastion), and enable features like deletion_protection for extra safety.
Would you like help wiring up the variable or locking down the allowed CIDRs?
🤖 Prompt for AI Agents
In deployment/modules/storage/main.tf at line 26, the setting
publicly_accessible is set to true, which exposes the RDS instance to the public
internet. Change this value to false to restrict access, or make it configurable
via a variable that defaults to false. Additionally, review and update the
security group ingress rules to restrict access to trusted CIDRs, avoiding
0.0.0.0/0. Ensure that sensitive data is protected by limiting public exposure.
|
still need to manage to add the db to a public subnet, as currently is in a private one |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/upgrade-current-deployment-infra.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: e2e-tests / E2E Tests
|
|
||
| # Display RDS connection information | ||
| echo "=== RDS Connection Information ===" | ||
| echo "RDS Endpoint: $(terraform output -raw rds_endpoint)" | ||
| echo "Connection String: postgresql://${{ secrets.DATALAYER_PG_USER }}:${{ secrets.DATALAYER_PG_PASSWORD }}@$(terraform output -raw rds_endpoint)/GitcoinDatalayer{{Green|Blue}}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid exposing credentials in logs & correct the placeholder
Echoing the full connection string (including ${{ secrets.DATALAYER_PG_PASSWORD }}) risks leaking sensitive data—even if GitHub masks secrets—and the literal {{Green|Blue}} will not resolve to the active environment. Instead, mask or omit the password in the log and interpolate the ${active_deployment} shell variable for the database name.
Suggested diff:
- # Display RDS connection information
- echo "=== RDS Connection Information ==="
- echo "RDS Endpoint: $(terraform output -raw rds_endpoint)"
- echo "Connection String: postgresql://${{ secrets.DATALAYER_PG_USER }}:${{ secrets.DATALAYER_PG_PASSWORD }}@$(terraform output -raw rds_endpoint)/GitcoinDatalayer{{Green|Blue}}"
+ # Display RDS connection information (password masked)
+ echo "=== RDS Connection Information ==="
+ echo "RDS Endpoint: $(terraform output -raw rds_endpoint)"
+ echo "Connection String: postgresql://${TF_VAR_DATALAYER_PG_USER}:****@$(terraform output -raw rds_endpoint)/${active_deployment}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Display RDS connection information | |
| echo "=== RDS Connection Information ===" | |
| echo "RDS Endpoint: $(terraform output -raw rds_endpoint)" | |
| echo "Connection String: postgresql://${{ secrets.DATALAYER_PG_USER }}:${{ secrets.DATALAYER_PG_PASSWORD }}@$(terraform output -raw rds_endpoint)/GitcoinDatalayer{{Green|Blue}}" | |
| # Display RDS connection information (password masked) | |
| echo "=== RDS Connection Information ===" | |
| echo "RDS Endpoint: $(terraform output -raw rds_endpoint)" | |
| echo "Connection String: postgresql://${TF_VAR_DATALAYER_PG_USER}:****@$(terraform output -raw rds_endpoint)/${active_deployment}" |
🤖 Prompt for AI Agents
In .github/workflows/upgrade-current-deployment-infra.yml around lines 79 to 83,
the script echoes the full database connection string including the secret
password, which risks exposing sensitive credentials in logs, and the
placeholder {{Green|Blue}} is not dynamically replaced. Remove or mask the
password from the echoed connection string to prevent credential leaks, and
replace the placeholder with the actual environment variable that specifies the
deployment name, such as ${active_deployment}. Ensure the connection string logs
only non-sensitive information and correctly references environment variables.
hussedev
force-pushed
the
chore/allow-access-to-db
branch
from
532fb3b to
5ae5d02
Compare
🤖 Linear
Closes PAR-XXX
Description
Checklist before requesting a review
Summary by CodeRabbit