feat: enable SSL/TLS for PgBouncer client connections #49
Conversation
PgBouncer was rejecting SSL connections on port 5432 because client-side TLS was disabled. This change enables SSL for all client connections to PgBouncer with sslmode=require as the default. Changes: - Add client TLS configuration to PgBouncer entrypoint script - Auto-generate self-signed certificates if not provided - Add openssl package to PgBouncer Dockerfile for cert generation - Add PGBOUNCER_CLIENT_TLS_* environment variables to docker-compose - Update .env.example with new TLS configuration options - Remove obsolete pgbouncer.ini.tpl (config generated dynamically) The default sslmode is 'require', enforcing SSL for all connections. Users can set PGBOUNCER_CLIENT_TLS_SSLMODE=disable to opt out.
Summary of ChangesHello @paudley, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the security posture of PgBouncer by enabling and enforcing SSL/TLS for all client connections. It resolves a previous issue where client-side TLS was not properly configured, ensuring that data in transit between clients and PgBouncer is encrypted. The changes also include the convenience of automatic self-signed certificate generation, making it easier to deploy secure PgBouncer instances. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables SSL/TLS for client connections to PgBouncer, which is a great security enhancement. The implementation includes auto-generation of self-signed certificates and moves from a static configuration template to a more flexible dynamic generation within the entrypoint script. The changes are well-structured and clear. I have one suggestion to improve the robustness of the entrypoint script by adding better error handling during the certificate generation process, which will make potential startup failures easier to debug.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Patrick Audley <paudley@blackcat.ca>
There was a problem hiding this comment.
Pull request overview
This PR enables SSL/TLS encryption for client connections to PgBouncer by implementing client-side TLS configuration with automatic self-signed certificate generation. The change addresses a gap where PgBouncer only had server-side (backend) TLS configured but rejected client SSL connections. The implementation follows the existing pattern used for PostgreSQL SSL configuration in the codebase.
Key Changes
- Auto-generates RSA 4096-bit self-signed certificates at container startup if not provided
- Sets
client_tls_sslmode=requireas the default, enforcing SSL for all client connections - Removes static configuration template in favor of dynamic configuration generation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
pgbouncer/entrypoint.sh |
Adds client TLS environment variable defaults, certificate generation logic, and dynamic configuration injection |
docker/pgbouncer/Dockerfile |
Adds openssl package dependency for certificate generation |
docker-compose.yml |
Passes new PGBOUNCER_CLIENT_TLS_* environment variables to the container |
.env.example |
Documents the five new client TLS configuration options with defaults and descriptions |
pgbouncer/pgbouncer.ini.tpl |
Removed - replaced by dynamic configuration generation in entrypoint |
Comments suppressed due to low confidence (1)
pgbouncer/entrypoint.sh:172
- [nitpick] The
client_tls_configvariable may contain an empty string whenPGBOUNCER_CLIENT_TLS_SSLMODEis set to "disable", which will result in a blank line being inserted into the pgbouncer.ini configuration file at line 172. While this may not cause a functional issue, it results in inconsistent configuration file formatting.
Consider only inserting the variable if it's non-empty:
server_tls_sslmode = ${PGBOUNCER_SERVER_TLS_MODE:-require}
$(if [[ -n "${client_tls_config}" ]]; then echo "${client_tls_config}"; fi)
EOFOr use a conditional here-doc approach.
stats_users = ${PGBOUNCER_STATS_USERS}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export PGBOUNCER_CLIENT_TLS_KEY_FILE=${PGBOUNCER_CLIENT_TLS_KEY_FILE:-/tmp/pgbouncer/tls/server.key} | ||
| export PGBOUNCER_CLIENT_TLS_SELF_SIGNED_SUBJECT=${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_SUBJECT:-/CN=core_data_pgbouncer} | ||
| export PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS=${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS:-730} | ||
|
|
There was a problem hiding this comment.
The PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS value is passed directly to the openssl command without validation. If a user provides a non-numeric value, the openssl command will fail. While the error is suppressed by the output redirection, this could lead to confusing failures.
Consider adding validation:
if ! [[ "${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS}" =~ ^[0-9]+$ ]]; then
echo "[pgbouncer] ERROR: PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS must be a positive integer, got: ${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS}" >&2
exit 1
fi| # Validate that PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS is a positive integer | |
| if ! [[ "${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS}" =~ ^[0-9]+$ ]]; then | |
| echo "[pgbouncer] ERROR: PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS must be a positive integer, got: ${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS}" >&2 | |
| exit 1 | |
| fi |
| export PGBOUNCER_CLIENT_TLS_CERT_FILE=${PGBOUNCER_CLIENT_TLS_CERT_FILE:-/tmp/pgbouncer/tls/server.crt} | ||
| export PGBOUNCER_CLIENT_TLS_KEY_FILE=${PGBOUNCER_CLIENT_TLS_KEY_FILE:-/tmp/pgbouncer/tls/server.key} |
There was a problem hiding this comment.
[nitpick] The default certificate and key paths (/tmp/pgbouncer/tls/) are in a world-writable directory (/tmp). While the individual file permissions are restrictive (600 for key, 644 for cert), the parent directory structure could potentially be manipulated by other processes or users if the container is shared.
Consider using a more secure default location that's not under /tmp, such as /var/lib/pgbouncer/tls/ or /opt/core_data/pgbouncer/tls/. This would require updating the defaults in both entrypoint.sh and the environment variable documentation.
| export PGBOUNCER_CLIENT_TLS_CERT_FILE=${PGBOUNCER_CLIENT_TLS_CERT_FILE:-/tmp/pgbouncer/tls/server.crt} | |
| export PGBOUNCER_CLIENT_TLS_KEY_FILE=${PGBOUNCER_CLIENT_TLS_KEY_FILE:-/tmp/pgbouncer/tls/server.key} | |
| export PGBOUNCER_CLIENT_TLS_CERT_FILE=${PGBOUNCER_CLIENT_TLS_CERT_FILE:-/opt/core_data/pgbouncer/tls/server.crt} | |
| export PGBOUNCER_CLIENT_TLS_KEY_FILE=${PGBOUNCER_CLIENT_TLS_KEY_FILE:-/opt/core_data/pgbouncer/tls/server.key} |
| echo "[pgbouncer] Generating self-signed TLS certificate for client connections." >&2 | ||
| if ! command -v openssl >/dev/null 2>&1; then | ||
| echo "[pgbouncer] ERROR: openssl not available; cannot create TLS assets." >&2 | ||
| exit 1 | ||
| fi | ||
| if ! openssl_output=$(openssl req -x509 -nodes -newkey rsa:4096 \ |
There was a problem hiding this comment.
The logic for generating certificates only checks if either file is missing, but should verify both files exist together or both are missing. If only one file exists (e.g., cert exists but key is missing), the code will attempt to regenerate both, potentially overwriting the existing certificate. This could cause issues if a user provided only one file.
Consider using separate checks:
if [[ ! -f "${PGBOUNCER_CLIENT_TLS_CERT_FILE}" ]] || [[ ! -f "${PGBOUNCER_CLIENT_TLS_KEY_FILE}" ]]; then
if [[ -f "${PGBOUNCER_CLIENT_TLS_CERT_FILE}" ]] || [[ -f "${PGBOUNCER_CLIENT_TLS_KEY_FILE}" ]]; then
echo "[pgbouncer] ERROR: Both cert and key files must be provided together or both will be auto-generated." >&2
exit 1
fi
# ... proceed with generation
fi| echo "[pgbouncer] Generating self-signed TLS certificate for client connections." >&2 | |
| if ! command -v openssl >/dev/null 2>&1; then | |
| echo "[pgbouncer] ERROR: openssl not available; cannot create TLS assets." >&2 | |
| exit 1 | |
| fi | |
| if ! openssl_output=$(openssl req -x509 -nodes -newkey rsa:4096 \ | |
| if [[ -f "${PGBOUNCER_CLIENT_TLS_CERT_FILE}" || -f "${PGBOUNCER_CLIENT_TLS_KEY_FILE}" ]]; then | |
| echo "[pgbouncer] ERROR: Both cert and key files must be provided together or both will be auto-generated." >&2 | |
| exit 1 | |
| fi | |
| echo "[pgbouncer] Generating self-signed TLS certificate for client connections." >&2 | |
| if ! command -v openssl >/dev/null 2>&1; then | |
| echo "[pgbouncer] ERROR: openssl not available; cannot create TLS assets." >&2 | |
| exit 1 | |
| fi | |
| if ! openssl_output=$(openssl req -x509 -nodes -newkey rsa:4096 \ |
| fi | ||
| chmod 600 "${PGBOUNCER_CLIENT_TLS_KEY_FILE}" |
There was a problem hiding this comment.
[nitpick] The umask 077 is set at line 95, which ensures restrictive permissions for subsequently created files. However, the TLS certificate generation at lines 97-117 explicitly sets permissions with chmod 600 for the key and chmod 644 for the certificate. While this is correct behavior, the explicit chmod calls make the earlier umask setting less effective for these specific files.
This is fine as-is since the permissions are explicitly controlled, but consider adding a comment explaining why these specific permissions are needed (e.g., "# Key must be readable only by owner; cert can be world-readable") for clarity.
| fi | |
| chmod 600 "${PGBOUNCER_CLIENT_TLS_KEY_FILE}" | |
| fi | |
| # Key must be readable only by owner for security | |
| chmod 600 "${PGBOUNCER_CLIENT_TLS_KEY_FILE}" | |
| # Certificate can be world-readable as it is public |
| tls_cert_dir=$(dirname "${PGBOUNCER_CLIENT_TLS_CERT_FILE}") | ||
| tls_key_dir=$(dirname "${PGBOUNCER_CLIENT_TLS_KEY_FILE}") | ||
| mkdir -p "${tls_cert_dir}" "${tls_key_dir}" |
There was a problem hiding this comment.
[nitpick] The tls_cert_dir and tls_key_dir variables are extracted but may point to the same directory (which is the case with the default values: both /tmp/pgbouncer/tls). The code creates both directories separately, which is redundant when they're the same. While this doesn't cause a functional problem, it's inefficient.
Consider optimizing:
tls_dir=$(dirname "${PGBOUNCER_CLIENT_TLS_CERT_FILE}")
if [[ "$(dirname "${PGBOUNCER_CLIENT_TLS_KEY_FILE}")" != "${tls_dir}" ]]; then
mkdir -p "${tls_dir}" "$(dirname "${PGBOUNCER_CLIENT_TLS_KEY_FILE}")"
else
mkdir -p "${tls_dir}"
fiOr simply use mkdir -p on the unique directory paths, which handles duplicates naturally:
mkdir -p "$(dirname "${PGBOUNCER_CLIENT_TLS_CERT_FILE}")" "$(dirname "${PGBOUNCER_CLIENT_TLS_KEY_FILE}")"| tls_cert_dir=$(dirname "${PGBOUNCER_CLIENT_TLS_CERT_FILE}") | |
| tls_key_dir=$(dirname "${PGBOUNCER_CLIENT_TLS_KEY_FILE}") | |
| mkdir -p "${tls_cert_dir}" "${tls_key_dir}" | |
| mkdir -p "$(dirname "${PGBOUNCER_CLIENT_TLS_CERT_FILE}")" "$(dirname "${PGBOUNCER_CLIENT_TLS_KEY_FILE}")" |
| if ! openssl_output=$(openssl req -x509 -nodes -newkey rsa:4096 \ | ||
| -keyout "${PGBOUNCER_CLIENT_TLS_KEY_FILE}" \ | ||
| -out "${PGBOUNCER_CLIENT_TLS_CERT_FILE}" \ | ||
| -days "${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS}" \ | ||
| -subj "${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_SUBJECT}" 2>&1); then |
There was a problem hiding this comment.
Error output from the openssl command is suppressed with >/dev/null 2>&1, which makes debugging failures difficult. If certificate generation fails silently, the script will continue and may fail later when PgBouncer tries to use non-existent or invalid certificate files.
Consider removing the output redirection or at least checking the exit status:
if ! openssl req -x509 -nodes -newkey rsa:4096 \
-keyout "${PGBOUNCER_CLIENT_TLS_KEY_FILE}" \
-out "${PGBOUNCER_CLIENT_TLS_CERT_FILE}" \
-days "${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS}" \
-subj "${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_SUBJECT}" 2>&1; then
echo "[pgbouncer] ERROR: Failed to generate TLS certificate" >&2
exit 1
fi|
|
||
| # PgBouncer TLS configuration (client-side SSL for connections to PgBouncer) | ||
| # Self-signed certificates are generated automatically if files are absent | ||
| # sslmode options: disable, allow, prefer, require, verify-ca, verify-full |
There was a problem hiding this comment.
The documentation mentions verify-ca and verify-full as valid sslmode options, but PgBouncer doesn't support these modes for client_tls_sslmode. According to PgBouncer documentation, only disable, allow, prefer, and require are valid for client_tls_sslmode. The verify-ca and verify-full modes are only supported for server_tls_sslmode (backend connections).
Update the comment to only list the supported modes:
# sslmode options: disable, allow, prefer, require| # sslmode options: disable, allow, prefer, require, verify-ca, verify-full | |
| # sslmode options: disable, allow, prefer, require |
| export POSTGRES_DB=${POSTGRES_DB:-postgres} | ||
|
|
||
| # Client TLS configuration (connections from clients to PgBouncer) | ||
| export PGBOUNCER_CLIENT_TLS_SSLMODE=${PGBOUNCER_CLIENT_TLS_SSLMODE:-require} |
There was a problem hiding this comment.
There's no validation of the PGBOUNCER_CLIENT_TLS_SSLMODE value. If a user provides an invalid value (e.g., verify-full, which is documented in .env.example but not supported), PgBouncer will fail to start with a potentially cryptic error message.
Consider adding validation:
valid_modes="disable allow prefer require"
if [[ ! " ${valid_modes} " =~ " ${PGBOUNCER_CLIENT_TLS_SSLMODE} " ]]; then
echo "[pgbouncer] ERROR: Invalid PGBOUNCER_CLIENT_TLS_SSLMODE='${PGBOUNCER_CLIENT_TLS_SSLMODE}'. Must be one of: ${valid_modes}" >&2
exit 1
fi| export PGBOUNCER_CLIENT_TLS_SSLMODE=${PGBOUNCER_CLIENT_TLS_SSLMODE:-require} | |
| export PGBOUNCER_CLIENT_TLS_SSLMODE=${PGBOUNCER_CLIENT_TLS_SSLMODE:-require} | |
| # Validate PGBOUNCER_CLIENT_TLS_SSLMODE | |
| valid_modes="disable allow prefer require" | |
| if [[ ! " ${valid_modes} " =~ " ${PGBOUNCER_CLIENT_TLS_SSLMODE} " ]]; then | |
| echo "[pgbouncer] ERROR: Invalid PGBOUNCER_CLIENT_TLS_SSLMODE='${PGBOUNCER_CLIENT_TLS_SSLMODE}'. Must be one of: ${valid_modes}" >&2 | |
| exit 1 | |
| fi |
| # Client TLS configuration (connections from clients to PgBouncer) | ||
| export PGBOUNCER_CLIENT_TLS_SSLMODE=${PGBOUNCER_CLIENT_TLS_SSLMODE:-require} | ||
| export PGBOUNCER_CLIENT_TLS_CERT_FILE=${PGBOUNCER_CLIENT_TLS_CERT_FILE:-/tmp/pgbouncer/tls/server.crt} | ||
| export PGBOUNCER_CLIENT_TLS_KEY_FILE=${PGBOUNCER_CLIENT_TLS_KEY_FILE:-/tmp/pgbouncer/tls/server.key} | ||
| export PGBOUNCER_CLIENT_TLS_SELF_SIGNED_SUBJECT=${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_SUBJECT:-/CN=core_data_pgbouncer} | ||
| export PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS=${PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS:-730} |
There was a problem hiding this comment.
The new default of PGBOUNCER_CLIENT_TLS_SSLMODE=require will require SSL for all client connections. However, existing tests in tests/test_manage.py (lines 1066-1074, 1096-1104) connect to PgBouncer without specifying an SSL mode, which means they'll use the psycopg default behavior. This could cause test failures if the self-signed certificate isn't trusted or if SSL isn't properly negotiated.
Consider either:
- Updating tests to explicitly handle SSL (e.g.,
sslmode='require'parameter) - Documenting that tests need to trust the self-signed certificate
- Using
sslmode='disable'in test environments if SSL testing isn't the goal
This lack of test coverage for the new TLS functionality is a gap that should be addressed.
1 participant
Summary
sslmode=requireenforces SSL for all client connectionsProblem
SSL connections to port 5432 (PgBouncer) were being rejected because client-side TLS was disabled in the PgBouncer configuration. Only
server_tls_sslmode(backend) was configured, notclient_tls_sslmode(frontend).Changes
pgbouncer/entrypoint.shdocker/pgbouncer/Dockerfileopensslpackage for certificate generationdocker-compose.yml.env.examplePGBOUNCER_CLIENT_TLS_*optionspgbouncer/pgbouncer.ini.tplNew Configuration Options
PGBOUNCER_CLIENT_TLS_SSLMODE=require # disable|allow|prefer|require PGBOUNCER_CLIENT_TLS_CERT_FILE=/tmp/pgbouncer/tls/server.crt PGBOUNCER_CLIENT_TLS_KEY_FILE=/tmp/pgbouncer/tls/server.key PGBOUNCER_CLIENT_TLS_SELF_SIGNED_SUBJECT=/CN=core_data_pgbouncer PGBOUNCER_CLIENT_TLS_SELF_SIGNED_DAYS=730Test plan
psql "sslmode=require" -p 5432psql "sslmode=require" -p 6432psql "sslmode=disable" -p 5432→ "FATAL: SSL required"🤖 Generated with Claude Code