feat: switch to include directives in pg_hba #1765
Conversation
|
Requires pg16+ |
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
e258813 to
6d11c7d
Compare
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
7b62c4f to
641951d
Compare
hunleyd
left a comment
hunleyd
left a comment
There was a problem hiding this comment.
lgtm, minus the conflict
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
641951d to
9a840bf
Compare
staaldraad
dismissed
hunleyd’s stale review
conflict resolved and rebased to use latest admin_api and admin_mgr as introduced by #1780
Using include directives makes changing the pg_hba.conf on the fly more flexible. Enabling / disabling ssl enforcement for example only requires creating or removing a file, leaving the pg_hba.conf untouched. Allowing for more repeatable and stable processes and no need for regex based replace or custom parsers. This will also support the just-in-time access work by allowing jit to be dynamically enabled/disabled
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
9a840bf to
4e6292b
Compare
WalkthroughAdds version-aware PostgreSQL HBA handling: PostgreSQL 15 uses Changes
Sequence Diagram(s)sequenceDiagram
participant Deployer
participant VersionDetector as Version Detector
participant BuildSystem as Nix/Docker
participant Ansible
participant ConfigStore as HBA Templates
participant Postgres
Deployer->>VersionDetector: Determine Postgres major version
VersionDetector->>BuildSystem: Provide version (e.g., 15 / other)
BuildSystem->>Ansible: Trigger provisioning with psql_version
alt psql_version == 15
Ansible->>ConfigStore: Select `pg_hba.conf_15.j2`
Ansible->>Postgres: Deploy `/etc/postgresql/pg_hba.conf` (from _15 template)
else psql_version != 15
Ansible->>ConfigStore: Select `pg_hba.conf.j2`, `pg_hba_public.conf.j2`, `pg_hba_users_public.conf.j2`
Ansible->>Postgres: Deploy `/etc/postgresql/pg_hba.conf` and modular public/user HBA files
end
Postgres-->>Deployer: HBA configuration applied
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix/packages/lib.nix (1)
11-18: Fix missingversionargument in nix/checks.nix (line 102).The call to
makePostgresDevSetupis missing the requiredversionargument. Add it to the object passed at line 102:start-postgres-server-bin = pkgs-lib.makePostgresDevSetup { inherit pkgs pgroonga; name = "start-postgres-server-test"; version = ""; # or appropriate version value extraSubstitutions = {The call in nix/packages/default.nix correctly passes
version = activeVersion.
🤖 Fix all issues with AI agents
In `@ansible/tasks/setup-pgbouncer.yml`:
- Around line 46-52: The pgbouncer user creation task uses ansible.builtin.user
with name "pgbouncer" and has a typo in the shell parameter
("/usr/sbin/nolign"); update the shell value to the correct "/usr/sbin/nologin"
in that task so the pgbouncer user gets the proper non-login shell.
In `@nix/tools/run-server.sh.in`:
- Around line 216-225: The extra HBA glob currently uses pg_hba*.conf* and can
match the base pg_hba.conf and fail when there are no matches; change the logic
in run-server.sh.in: enable nullglob (shopt -s nullglob) before building
extra_hba_files, use the narrower pattern
${PG_HBA_FILE%pg_hba.conf}pg_hba_*.conf to only match include files, iterate and
copy them as before, then restore nullglob (shopt -u nullglob) if needed; ensure
the earlier cp of "${PG_HBA_FILE}" remains and that the new glob will never
duplicate that base file.
🧹 Nitpick comments (3)
ansible/files/postgresql_config/pg_hba_public.conf.j2 (1)
1-2: Non-standard IPv6 notation.The IPv6 CIDR
::0/0is functional but the standard notation is::/0. Consider using::/0for consistency with PostgreSQL documentation and broader convention.Suggested fix
host all all 0.0.0.0/0 scram-sha-256 -host all all ::0/0 scram-sha-256 +host all all ::/0 scram-sha-256ansible/files/postgresql_config/pg_hba.conf.j2 (1)
34-35: Same IPv6 notation note for replication rules.Consider
::/0instead of::0/0for consistency.Suggested fix
host replication supabase_replication_admin 0.0.0.0/0 scram-sha-256 -host replication supabase_replication_admin ::0/0 scram-sha-256 +host replication supabase_replication_admin ::/0 scram-sha-256ansible/tasks/setup-pgbouncer.yml (1)
112-119: Duplicate entry in loop.
/etc/pgbouncer-custom/ssl-config.iniappears twice in the loop (lines 113 and 117). This is harmless but redundant.Proposed fix
loop: - /etc/pgbouncer-custom/ssl-config.ini - /etc/postgresql/pg_hba.conf - /etc/postgresql/pg_hba_users_public.conf - /etc/postgresql/pg_hba_public.conf - - /etc/pgbouncer-custom/ssl-config.ini
| if [ "$VERSION" = "15" ]; then | ||
| cp $(dirname "$PG_HBA_FILE")/pg_hba.conf_15* "$DATDIR/pg_hba.conf" | ||
| else | ||
| cp "${PG_HBA_FILE}" "$DATDIR/pg_hba.conf" | ||
| # copy extra hba_*.conf files over | ||
| extra_hba_files=( ${PG_HBA_FILE%pg_hba.conf}pg_hba*.conf* ) | ||
| for f in "${extra_hba_files[@]}"; do | ||
| base=$(basename "$f") | ||
| cp "$f" "$DATDIR/${base%%.conf*}.conf" | ||
| done |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for pg_hba files and PG_HBA references
fd -t f 'pg_hba' --exclude node_modules --exclude '.git'Repository: supabase/postgres
Length of output: 271
🏁 Script executed:
# Search for PG_HBA and related configuration patterns
rg -i 'pg_hba|PG_HBA' -t sh -t nix -t dockerfile --max-count=20Repository: supabase/postgres
Length of output: 95
🏁 Script executed:
# Examine the run-server.sh.in file in full to understand context
cat nix/tools/run-server.sh.in | head -250Repository: supabase/postgres
Length of output: 7806
🏁 Script executed:
# Check for version-specific pg_hba files and templates
find . -name '*pg_hba*' 2>/dev/nullRepository: supabase/postgres
Length of output: 279
Narrow the extra HBA glob to match only include files, not the base config.
The pattern pg_hba*.conf* matches all pg_hba files in the directory, including the base pg_hba.conf, which is already copied on the preceding line. This creates redundant copies and masks the intent to copy only include files like pg_hba_public.conf and pg_hba_users_public.conf. Additionally, unmatched globs will cause cp to attempt copying a literal pattern string, resulting in an error.
Use pg_hba_*.conf (underscore-prefixed) and enable nullglob to safely skip when no matches exist.
🛠️ Suggested fix
- # copy extra hba_*.conf files over
- extra_hba_files=( ${PG_HBA_FILE%pg_hba.conf}pg_hba*.conf* )
- for f in "${extra_hba_files[@]}"; do
- base=$(basename "$f")
- cp "$f" "$DATDIR/${base%%.conf*}.conf"
- done
+ # copy extra pg_hba_*.conf include files over
+ hba_dir=$(dirname "$PG_HBA_FILE")
+ shopt -s nullglob
+ extra_hba_files=( "$hba_dir"/pg_hba_*.conf )
+ for f in "${extra_hba_files[@]}"; do
+ cp "$f" "$DATDIR/$(basename "$f")"
+ done
+ shopt -u nullglob📝 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.
| if [ "$VERSION" = "15" ]; then | |
| cp $(dirname "$PG_HBA_FILE")/pg_hba.conf_15* "$DATDIR/pg_hba.conf" | |
| else | |
| cp "${PG_HBA_FILE}" "$DATDIR/pg_hba.conf" | |
| # copy extra hba_*.conf files over | |
| extra_hba_files=( ${PG_HBA_FILE%pg_hba.conf}pg_hba*.conf* ) | |
| for f in "${extra_hba_files[@]}"; do | |
| base=$(basename "$f") | |
| cp "$f" "$DATDIR/${base%%.conf*}.conf" | |
| done | |
| if [ "$VERSION" = "15" ]; then | |
| cp $(dirname "$PG_HBA_FILE")/pg_hba.conf_15* "$DATDIR/pg_hba.conf" | |
| else | |
| cp "${PG_HBA_FILE}" "$DATDIR/pg_hba.conf" | |
| # copy extra pg_hba_*.conf include files over | |
| hba_dir=$(dirname "$PG_HBA_FILE") | |
| shopt -s nullglob | |
| extra_hba_files=( "$hba_dir"/pg_hba_*.conf ) | |
| for f in "${extra_hba_files[@]}"; do | |
| cp "$f" "$DATDIR/$(basename "$f")" | |
| done | |
| shopt -u nullglob | |
| fi |
🤖 Prompt for AI Agents
In `@nix/tools/run-server.sh.in` around lines 216 - 225, The extra HBA glob
currently uses pg_hba*.conf* and can match the base pg_hba.conf and fail when
there are no matches; change the logic in run-server.sh.in: enable nullglob
(shopt -s nullglob) before building extra_hba_files, use the narrower pattern
${PG_HBA_FILE%pg_hba.conf}pg_hba_*.conf to only match include files, iterate and
copy them as before, then restore nullglob (shopt -u nullglob) if needed; ensure
the earlier cp of "${PG_HBA_FILE}" remains and that the new glob will never
duplicate that base file.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix/packages/lib.nix (1)
11-18: Addversionparameter to themakePostgresDevSetupcall in nix/checks.nix.The call at nix/checks.nix line 102 is missing the required
versionparameter. This will fail Nix evaluation. The call in nix/packages/default.nix already includesversion = activeVersion;but nix/checks.nix needs the same fix.
🤖 Fix all issues with AI agents
In `@ansible/tasks/setup-pgbouncer.yml`:
- Around line 108-120: The loop in the Ansible task named "Grant pg_hba and
pgbouncer grp perm for adminapi updates" contains a duplicate entry for
"/etc/pgbouncer-custom/ssl-config.ini"; remove the repeated item from the list
in the loop so each path is unique (adjust the array under the loop that uses
loop_var "pgbouncer_group_item"), leaving only one occurrence of
"/etc/pgbouncer-custom/ssl-config.ini".
🧹 Nitpick comments (1)
nix/packages/lib.nix (1)
70-94: Normalizeversionbefore comparing to"15".If
versionincludes a patch/build suffix (e.g.,15.14.x-hba),version == "15"will miss and route PG15 to the include-based config, which the PR notes is unsupported. Consider deriving the major version first.♻️ Suggested change
- extraPaths = - if version == "15" then + extraPaths = + let + majorVersion = lib.versions.major version; + in + if majorVersion == "15" then
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
0b26f01 to
c420bb1
Compare
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
c420bb1 to
088a3d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ansible/tasks/setup-pgbouncer.yml`:
- Around line 108-118: The task "Grant pg_hba and pgbouncer grp perm for
adminapi updates" is trying to set modes on files that don't exist on PG15
hosts; add a conditional to skip the task on PG15 by adding when: not is_psql_15
(same pattern used in the postgres setup block) so the loop over
pgbouncer_group_item (/etc/pgbouncer-custom/ssl-config.ini,
/etc/postgresql/pg_hba.conf, /etc/postgresql/pg_hba_users_public.conf,
/etc/postgresql/pg_hba_public.conf) only runs on non-PG15 hosts.
| - name: Grant pg_hba and pgbouncer grp perm for adminapi updates | ||
| ansible.builtin.file: | ||
| mode: '0664' | ||
| mode: "0664" | ||
| path: "{{ pgbouncer_group_item }}" | ||
| loop: | ||
| - /etc/pgbouncer-custom/ssl-config.ini | ||
| - /etc/postgresql/pg_hba.conf | ||
| - /etc/postgresql/pg_hba_users_public.conf | ||
| - /etc/postgresql/pg_hba_public.conf | ||
| loop_control: | ||
| loop_var: 'pgbouncer_group_item' | ||
| loop_var: "pgbouncer_group_item" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "setup-pgbouncer.yml" -type fRepository: supabase/postgres
Length of output: 96
🏁 Script executed:
find . -name "*.yml" -path "*/tasks/*" -o -name "*.yaml" -path "*/tasks/*" | head -20Repository: supabase/postgres
Length of output: 873
🏁 Script executed:
git ls-files | grep -E "(setup-pgbouncer|postgres)" | head -20Repository: supabase/postgres
Length of output: 1116
🏁 Script executed:
cat -n ./ansible/tasks/setup-pgbouncer.ymlRepository: supabase/postgres
Length of output: 5102
🏁 Script executed:
grep -r "pg_hba_users_public.conf\|pg_hba_public.conf" ./ansible --include="*.yml" --include="*.yaml" -B 2 -A 2Repository: supabase/postgres
Length of output: 1674
🏁 Script executed:
grep -r "psql_version.*15\|when:.*pg16\|when:.*PG16" ./ansible/tasks --include="*.yml" --include="*.yaml" -A 2 -B 2Repository: supabase/postgres
Length of output: 1948
🏁 Script executed:
grep -A 20 -B 5 "pg_hba_public.conf" ./ansible/tasks/setup-postgres.yml | head -50Repository: supabase/postgres
Length of output: 1136
🏁 Script executed:
grep -B 30 "import pg_hba_public.conf" ./ansible/tasks/setup-postgres.yml | tail -40Repository: supabase/postgres
Length of output: 1128
🏁 Script executed:
grep -B 50 "Add pg_hba_public.conf" ./ansible/tasks/setup-postgres.yml | grep -E "when:|block:|name:" | tail -20Repository: supabase/postgres
Length of output: 509
🏁 Script executed:
grep -r "pg_hba_users_public.conf\|pg_hba_public.conf" ./ansible/tasks --include="*.yml" | grep -v "Grant pg_hba"Repository: supabase/postgres
Length of output: 938
🏁 Script executed:
grep -r "ignore_errors\|failed_when" ./ansible/tasks/setup-pgbouncer.ymlRepository: supabase/postgres
Length of output: 43
🏁 Script executed:
# Check if there's a task that creates pg_hba_public.conf on all versions
grep -B 10 -A 3 "pg_hba_public.conf" ./ansible/tasks/setup-postgres.yml | grep -E "when:|state:" | head -20Repository: supabase/postgres
Length of output: 91
🏁 Script executed:
# Let me check the exact ansible.builtin.file behavior and whether there's error handling
cat -n ./ansible/tasks/setup-pgbouncer.yml | sed -n '102,125p'Repository: supabase/postgres
Length of output: 994
🏁 Script executed:
# Check if there are any ignore_errors or failed_when directives around this task
grep -B 15 "Grant pg_hba and pgbouncer grp perm" ./ansible/tasks/setup-pgbouncer.yml | tail -20Repository: supabase/postgres
Length of output: 569
🏁 Script executed:
# Verify the condition guard in setup-postgres.yml more precisely
cat -n ./ansible/tasks/setup-postgres.yml | grep -B 15 "create pg_hba.conf with includes" | tail -20Repository: supabase/postgres
Length of output: 729
🏁 Script executed:
# Check Ansible documentation behavior - verify if ansible.builtin.file creates files
# Let's also look for any handlers or error handling around this task
python3 << 'EOF'
# ansible.builtin.file with state="file" (default) does NOT create files
# It only manages permissions/ownership of existing files
# If file doesn't exist, it will error unless state is "touch" or "absent"
print("ansible.builtin.file behavior (default state='file'):")
print("- Does NOT create files")
print("- Will FAIL if file doesn't exist (unless ignore_errors is set)")
print("- Only manages permissions/ownership of existing files")
EOFRepository: supabase/postgres
Length of output: 257
🏁 Script executed:
# Verify whether the pgbouncer task has any error handling
grep -A 2 "Grant pg_hba and pgbouncer grp perm" ./ansible/tasks/setup-pgbouncer.yml | grep -E "ignore_errors|register"Repository: supabase/postgres
Length of output: 43
🏁 Script executed:
# Double-check: is_psql_15 definition and what happens on PG15
grep -B 5 -A 15 "create pg_hba.conf with includes" ./ansible/tasks/setup-postgres.yml | head -30Repository: supabase/postgres
Length of output: 820
🏁 Script executed:
# Let me verify: will the pgbouncer task fail on PG15 when these files don't exist?
# Check if there's any ignore_errors or special handling in the pgbouncer task
grep -A 10 "Grant pg_hba and pgbouncer grp perm" ./ansible/tasks/setup-pgbouncer.ymlRepository: supabase/postgres
Length of output: 431
🏁 Script executed:
# Confirm: are pg_hba_public.conf and pg_hba_users_public.conf ONLY deployed on non-PG15?
grep -B 5 "create pg_hba.conf with includes" ./ansible/tasks/setup-postgres.yml | grep "when:"Repository: supabase/postgres
Length of output: 87
Add when: not is_psql_15 condition to skip include-file permissions on PG15 hosts.
The task will fail on PG15 nodes because /etc/postgresql/pg_hba_users_public.conf and /etc/postgresql/pg_hba_public.conf don't exist there (they are only deployed by setup-postgres.yml when not is_psql_15). Without this condition, the file mode task will error trying to modify non-existent files. Use the same PG16+ condition pattern from the postgres setup block.
🤖 Prompt for AI Agents
In `@ansible/tasks/setup-pgbouncer.yml` around lines 108 - 118, The task "Grant
pg_hba and pgbouncer grp perm for adminapi updates" is trying to set modes on
files that don't exist on PG15 hosts; add a conditional to skip the task on PG15
by adding when: not is_psql_15 (same pattern used in the postgres setup block)
so the loop over pgbouncer_group_item (/etc/pgbouncer-custom/ssl-config.ini,
/etc/postgresql/pg_hba.conf, /etc/postgresql/pg_hba_users_public.conf,
/etc/postgresql/pg_hba_public.conf) only runs on non-PG15 hosts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/tools/run-server.sh.in`:
- Around line 192-195: The script currently prints secret key contents by
echoing and catting the KEY_FILE (KEY_FILE, DATDIR variables); remove the cat
and the "KEY_FILE contents:" echo or gate them behind a debug-only check so
secrets are never printed in normal runs (e.g. replace the two lines with a
conditional that only prints/redacts when DEBUG or VERBOSE is explicitly
enabled, otherwise print "KEY_FILE: [REDACTED]" and keep export KEY_FILE as-is).
Ensure you target the export KEY_FILE, the echo "KEY_FILE:" and cat "$KEY_FILE"
statements for the change.
| export KEY_FILE="$DATDIR/pgsodium.key" | ||
| echo "KEY_FILE: $KEY_FILE" | ||
| echo "KEY_FILE contents:" | ||
| cat "$KEY_FILE" | ||
| cat "$KEY_FILE" |
There was a problem hiding this comment.
Stop printing KEY_FILE contents to logs.
Line 195 exposes secret material in stdout/CI logs. Please remove this or gate it behind a debug-only path with redaction.
🛠️ Suggested fix
echo "KEY_FILE: $KEY_FILE"
-echo "KEY_FILE contents:"
-cat "$KEY_FILE"📝 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.
| export KEY_FILE="$DATDIR/pgsodium.key" | |
| echo "KEY_FILE: $KEY_FILE" | |
| echo "KEY_FILE contents:" | |
| cat "$KEY_FILE" | |
| cat "$KEY_FILE" | |
| export KEY_FILE="$DATDIR/pgsodium.key" | |
| echo "KEY_FILE: $KEY_FILE" |
🤖 Prompt for AI Agents
In `@nix/tools/run-server.sh.in` around lines 192 - 195, The script currently
prints secret key contents by echoing and catting the KEY_FILE (KEY_FILE, DATDIR
variables); remove the cat and the "KEY_FILE contents:" echo or gate them behind
a debug-only check so secrets are never printed in normal runs (e.g. replace the
two lines with a conditional that only prints/redacts when DEBUG or VERBOSE is
explicitly enabled, otherwise print "KEY_FILE: [REDACTED]" and keep export
KEY_FILE as-is). Ensure you target the export KEY_FILE, the echo "KEY_FILE:" and
cat "$KEY_FILE" statements for the change.
Using include directives makes changing the pg_hba.conf on the fly more flexible. Enabling / disabling ssl enforcement for example only requires creating or removing a file, leaving the pg_hba.conf untouched. Allowing for more repeatable and stable processes and no need for regex based replace or custom parsers.
This will also support the just-in-time access work by allowing jit to be dynamically enabled/disabled
The required admin-api update is included as v0.88.0 added to ansible/vars.yml
Summary by CodeRabbit
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.