Switch to pgx, add SSL/cert and GSSAPI auth support#4
Merged
Conversation
📝 WalkthroughWalkthroughAdds PostgreSQL SSL/TLS and GSSAPI/Kerberos support: swaps pq for pgx driver, introduces SSL CLI flags/env and validation, changes connection-string behavior to include SSL parameters, extends CI with cert- and GSSAPI-based integration jobs, updates Dockerfile, docs, tests, and integration scripts. Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 22 |
| Duplication | 10 |
TIP This summary will be updated as you push new changes. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-radar.sh (1)
2-7:⚠️ Potential issue | 🟡 MinorOutdated comment: says 4 scenarios but script now has 6.
The header comment should be updated to reflect the additional scenarios.
📝 Suggested fix
#!/bin/bash -# Test script for radar in a Debian container with PostgreSQL 18 -# Tests all 4 permission scenarios: +# Test script for radar in a Debian container with PostgreSQL 18. +# Tests all 6 scenarios: # 1. Root + superuser # 2. Root + pg_monitor # 3. Non-root + superuser # 4. Non-root + pg_monitor +# 5. Certificate authentication +# 6. GSSAPI/Kerberos authentication🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-radar.sh` around lines 2 - 7, Update the header comment at the top of test-radar.sh to reflect that the script now covers 6 scenarios (change "Tests all 4 permission scenarios" to "Tests all 6 permission scenarios") and expand the numbered list to include the two additional scenarios that were added to the script so the comment matches the actual tests present in the file.
🧹 Nitpick comments (2)
radar_test.go (1)
849-850: Cleanup pattern could leak file handles on error.The cleanup functions call
closeErrCheck(file, ...)followed byos.Remove(file.Name()). However, ifos.CreateTempsucceeds but a later operation fails before cleanup runs, the file handle might not be closed properly. Consider usingdefer certFile.Close()immediately after creation.💡 Alternative pattern
certFile, err := os.CreateTemp("", "cert*.pem") if err != nil { t.Fatal(err) } defer certFile.Close() t.Cleanup(func() { os.Remove(certFile.Name()) })Also applies to: 855-856, 861-862
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@radar_test.go` around lines 849 - 850, After creating any temp file with os.CreateTemp (e.g., certFile), call defer <file>.Close() immediately after the successful creation to ensure the handle is closed on error paths, and change the t.Cleanup closures to only remove the file (os.Remove(file.Name())) instead of calling closeErrCheck; remove the closeErrCheck usage in the cleanup for certFile and the other temp files created in the same test so cleanup only removes the file and the file descriptor is closed by the immediate defer.test-radar.sh (1)
191-193: Consider usingpg_isreadyloop instead ofsleep 3.Hardcoded sleeps after PostgreSQL restart can be flaky. Earlier in the script (lines 31-33), a simpler
sleep 3is used, but for robustness consider apg_isreadyloop, especially since the server configuration changes significantly between restarts.Also applies to: 268-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-radar.sh` around lines 191 - 193, Replace the simple "sleep 3" after the pg_ctl restart command with a loop that calls pg_isready until PostgreSQL reports ready (or timeout) to avoid flaky starts; specifically update the block that runs su - postgres -c "/usr/lib/postgresql/18/bin/pg_ctl -D $PGDATA restart -l /var/lib/postgresql/18/logfile" to poll pg_isready (checking $PGDATA/$PGPORT as appropriate) with a short sleep and a max-wait, and make the same change for the other occurrence currently using "sleep 3" (the block around lines 268-270).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 341-343: The hardcoded "sleep 3" after restarting Postgres can
cause flaky CI; replace it with the same pg_isready-style wait loop used
elsewhere in this workflow: after running sudo -u postgres
/usr/lib/postgresql/18/bin/pg_ctl -D "$PGDATA" restart -l /tmp/postgres.log,
poll postgres with pg_isready (using the same host/port/env vars as the other
jobs) in a loop with a timeout and short sleep between attempts, and fail the
job if pg_isready does not report ready within the timeout.
- Around line 474-476: The hardcoded "sleep 3" after restarting PostgreSQL
should be replaced with a readiness loop using pg_isready to avoid timing
issues; in the block that runs "sudo -u postgres
/usr/lib/postgresql/18/bin/pg_ctl -D \"$PGDATA\" restart -l /tmp/postgres.log"
remove "sleep 3" and add a loop that repeatedly calls "pg_isready -d postgres -h
/var/run/postgresql" (or simply "pg_isready -q -d postgres" as appropriate) with
a short sleep between attempts and an overall timeout (e.g., 30s) to fail early
if the server never becomes ready; ensure the loop runs as the same user (sudo
-u postgres) or prefixes the pg_isready call appropriately so the GSSAPI test
waits for an actual ready state rather than a fixed delay.
In `@docs/index.md`:
- Line 136: The docs show inconsistent flag notation: update all occurrences of
the SSL-related flags to a single consistent form (prefer using double-dash for
long flags) so examples and Usage match; specifically change `-sslmode`,
`-sslcert`, `-sslkey`, `-sslrootcert` in the Usage section to `--sslmode`,
`--sslcert`, `--sslkey`, `--sslrootcert` (and ensure the example line that
already uses `--sslmode --sslcert client.crt --sslkey client.key --sslrootcert
ca.crt` remains unchanged), and scan the rest of the document for any other
`-ssl*` instances to normalize them to the chosen double-dash form.
In `@radar.go`:
- Around line 332-379: The TLS env/validation logic (the sslmodeFlagSet check,
reading PGSSLMODE/PGSSLCERT/PGSSLKEY/PGSSLROOTCERT, validSSLModes validation,
cert/key pairing check, verify-ca/verify-full root cert requirement, and file
existence checks) should be skipped when cfg.SkipPostgres is true; wrap the
entire block starting at the sslmodeFlagSet logic through the cert file os.Stat
loop in if !cfg.SkipPostgres { ... } so none of those environment reads or
validations run for system-only runs, preserving existing behavior when
SkipPostgres is false.
- Around line 402-409: The inline quoting helper q currently only checks for
literal space and backslash/quote and fails to quote other whitespace and empty
strings; update q (the anonymous function) to detect any unicode whitespace
(e.g., use unicode.IsSpace or strings.ContainsAny with "\t\n\r\v\f") and treat
empty string as "''", escape backslashes and single quotes as now, and wrap the
value in single quotes when it contains any whitespace or is empty so fields
like dbname, user, password, and cert paths are correctly quoted in the
connection string.
In `@README.md`:
- Line 159: Update the flag notation in the Certificate example to match the
Usage section by replacing the long form `--sslmode` with the single-dash form
`-sslmode` so it is consistent with the rest of the docs; change the snippet
containing `--sslmode verify-full --sslcert client.crt --sslkey client.key
--sslrootcert ca.crt` to use `-sslmode verify-full` (keep the other flags as-is)
so both `--sslmode` and `-sslmode` are not mixed across docs.
---
Outside diff comments:
In `@test-radar.sh`:
- Around line 2-7: Update the header comment at the top of test-radar.sh to
reflect that the script now covers 6 scenarios (change "Tests all 4 permission
scenarios" to "Tests all 6 permission scenarios") and expand the numbered list
to include the two additional scenarios that were added to the script so the
comment matches the actual tests present in the file.
---
Nitpick comments:
In `@radar_test.go`:
- Around line 849-850: After creating any temp file with os.CreateTemp (e.g.,
certFile), call defer <file>.Close() immediately after the successful creation
to ensure the handle is closed on error paths, and change the t.Cleanup closures
to only remove the file (os.Remove(file.Name())) instead of calling
closeErrCheck; remove the closeErrCheck usage in the cleanup for certFile and
the other temp files created in the same test so cleanup only removes the file
and the file descriptor is closed by the immediate defer.
In `@test-radar.sh`:
- Around line 191-193: Replace the simple "sleep 3" after the pg_ctl restart
command with a loop that calls pg_isready until PostgreSQL reports ready (or
timeout) to avoid flaky starts; specifically update the block that runs su -
postgres -c "/usr/lib/postgresql/18/bin/pg_ctl -D $PGDATA restart -l
/var/lib/postgresql/18/logfile" to poll pg_isready (checking $PGDATA/$PGPORT as
appropriate) with a short sleep and a max-wait, and make the same change for the
other occurrence currently using "sleep 3" (the block around lines 268-270).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f963b50-f568-44b1-9b4b-f9420f1da684
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/ci.ymlDockerfileREADME.mddocs/index.mdgo.modpostgres.goradar.goradar_test.gotest-radar.sh
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
470-486:⚠️ Potential issue | 🟠 MajorRemove the fallback
trustrules to ensure the test actually validates GSSAPI authentication.The
hostgssencrule matches only GSSAPI-encrypted connections, while thehostrules below match any TCP connection. If GSSAPI encryption negotiation is skipped or fails, the connection falls back to thehostrule withtrustauthentication, allowing the test to pass without actually proving Kerberos auth works. Sincekinitprovides valid credentials and libpq defaults to attempting GSSAPI encryption when credentials are present, removing the fallback rules will force the connection to either use GSSAPI or fail, ensuring the test genuinely validates the feature.🔧 Proposed fix
cat > "$PGDATA/pg_hba.conf" <<HBA local all all trust hostgssenc testdb testuser 127.0.0.1/32 gss include_realm=0 - host all all 127.0.0.1/32 trust - host all all ::1/128 trust + hostgssenc testdb testuser ::1/128 gss include_realm=0 HBA🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 470 - 486, The pg_hba.conf creation block currently adds fallback "host" lines that use "trust", which lets TCP connections bypass GSSAPI; edit the here-doc that writes "$PGDATA/pg_hba.conf" to remove the two fallback host rules (the lines starting with "host all all 127.0.0.1/32 trust" and "host all all ::1/128 trust") so only the GSSAPI-specific rule ("hostgssenc testdb testuser 127.0.0.1/32 gss include_realm=0") is used for TCP, ensuring the test actually requires GSSAPI auth; keep the existing local socket rule if needed and leave the surrounding restart/kinit/radar commands unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 334-349: The IPv6 loopback (::1) is allowed by a generic trust
rule so the certificate-auth test can be bypassed; update the pg_hba.conf HBA
block so the IPv6 address family has the same certificate-only rule as IPv4 by
adding an entry mirroring "hostssl testdb testuser 127.0.0.1/32 cert" for
"::1/128" and remove the existing "host all all ::1/128 trust" line;
locate the HBA heredoc (the cat > "$PGDATA/pg_hba.conf" <<HBA ... HBA block) and
modify it accordingly so only the new IPv6 hostssl rule permits access for
testdb/testuser.
In `@radar.go`:
- Around line 371-379: The SSL file validation loop currently only checks
existence via os.Stat and should also reject directories; update the loop that
iterates over the slice containing {"--sslcert", cfg.SSLCert}, {"--sslkey",
cfg.SSLKey}, {"--sslrootcert", cfg.SSLRootCert} to call os.Stat, then check
fi.IsDir() and return a clear error (e.g., fmt.Errorf("%s: is a directory",
f.flag) or wrap the error) when a path is a directory so directory paths are
rejected early instead of passing validation.
In `@test-radar.sh`:
- Around line 186-203: The IPv6 trust rule in the generated pg_hba.conf lets
connections to ::1 bypass the intended cert requirement when radar connects to
"localhost"; update the pg_hba.conf generation so IPv6 localhost uses the same
cert-only rule as IPv4: replace or precede the line "host all all
::1/128 trust" with a hostssl entry for the testdb/testuser
(e.g. "hostssl testdb testuser ::1/128 cert") and ensure this hostssl rule
appears before any broader trust rules so libpq IPv6 attempts cannot match a
permissive trust entry first; then restart PostgreSQL and re-run the radar
invocation that uses localhost.
- Around line 266-285: The pg_hba.conf here-document in test-radar.sh currently
contains broad "host ... trust" fallback lines that will match before/instead of
GSSAPI; edit the here-doc that writes "$PGDATA/pg_hba.conf" (the block
containing hostgssenc, hostssl, host ... trust) to remove the fallback "host all
... 127.0.0.1/32 trust" and "host all ... ::1/128 trust" lines and instead add
explicit hostgssenc entries for the IPv4/IPv6 addresses (or ensure hostgssenc
for testdb/testuser precedes any generic host rules) so GSSAPI-encrypted
connections are required when running ./radar -h localhost -d testdb -U testuser
-sslmode disable; keep or adjust other entries (local and hostssl) as needed.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 470-486: The pg_hba.conf creation block currently adds fallback
"host" lines that use "trust", which lets TCP connections bypass GSSAPI; edit
the here-doc that writes "$PGDATA/pg_hba.conf" to remove the two fallback host
rules (the lines starting with "host all all
127.0.0.1/32 trust" and "host all all
::1/128 trust") so only the GSSAPI-specific rule ("hostgssenc
testdb testuser 127.0.0.1/32 gss include_realm=0") is
used for TCP, ensuring the test actually requires GSSAPI auth; keep the existing
local socket rule if needed and leave the surrounding restart/kinit/radar
commands unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b9e8fd2-bab1-41ab-ad03-715f3ba07207
📒 Files selected for processing (5)
.github/workflows/ci.ymlREADME.mddocs/index.mdradar.gotest-radar.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/index.md
- README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
prefer--sslmode,--sslcert,--sslkey,--sslrootcertflags andPGSSLMODE/PGSSLCERT/PGSSLKEY/PGSSLROOTCERTenv varsChecklist
Tracking: STAFF-9