custom pam #1772
custom pam #1772
Conversation
staaldraad
force-pushed
the
sam/custom-pam
branch
from
5a65e37 to
07162ca
Compare
@samrose What's the advantage of the above compared to callPackage? Overall looks like the flake way is much more verbose. |
@steve-chavez for what it's worth, here are the advantages to using flakes, and the flake-parts framework https://gist.github.com/samrose/a9a19b7fad0aed8fd8355bdf0b87df08 |
staaldraad
force-pushed
the
sam/custom-pam
branch
from
01e56d8 to
43363e2
Compare
|
this and #1765 are directly related. I don't feel like merging the two into one version though, as the change in 1765 is pretty significant already. |
staaldraad
force-pushed
the
sam/custom-pam
branch
from
a796ea8 to
fdd95c8
Compare
@staaldraad I personally support whatever works best for you. I only created this PR to trigger some of the tests and builds and checks that let us know things are working as desired + give a simpler way to see changs. But thought you could either use this PR, or move work as you see fit. |
|
@staaldraad I believe it is the case that you are not yet ready to merge this correct? |
I'm ready, I've not been available to Shepard it through. And wanted to wait on the PG upgrade PR. 👍🏼 |
@staaldraad not trying to be an unreasonable blocker, but checking in as we're being extra careful lately: Do you think this has been thoroughly tested against latest release in our local infra? |
Great call out @samrose fully understood and you aren't being a blocker. It has been tested and it default |
|
@staaldraad is it true that this package should not be installed on a pg 15 image? |
samrose
force-pushed
the
sam/custom-pam
branch
from
f9ef9b7 to
2f18016
Compare
|
@staaldraad did a rebase on develop since we have launched pg 17.6 and 15.14 since the time you started this work. |
|
@samrose who hits the merge button on this one? 😄 |
|
@staaldraad I'll ping a few folks to do one last review, and we can merge on Monday, deploy on Tue |
ansible/tasks/setup-postgres.yml
Outdated
| set_fact: | ||
| is_psql_15: "{{ psql_version in ['psql_15'] }}" | ||
|
|
||
| - name: create placeholder pam config |
There was a problem hiding this comment.
Why do we need to create an empty file here ? Is there another process relying on the presence of this file ?
There was a problem hiding this comment.
yes, supabase-admin-api expects this file to be read-writeable. it is also the default name Postgres will use to lookup PAM rules when pg_hba lists pam as the login service
| try: | ||
| pg_major_version = int(result["stdout"].strip()) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
We want to know if we cannot parse the postgresql version. We should fail instead of using a default value.
| - name: Create symbolic link for linux-pam to find pam_jit_pg.so | ||
| become: yes | ||
| shell: | | ||
| sudo ln -s /var/lib/postgresql/.nix-profile/lib/security/pam_jit_pg.so $(find /nix/store -type d -path "/nix/store/*-linux-pam-*/lib/security" -print -quit)/pam_jit_pg.so |
There was a problem hiding this comment.
Are we trying to write in the store here ? Did you mean /usr/lib/x86_64-linux-gnu/security/pam_jit_pg.so where all the other pam modules are deployed in distribution like Ubuntu ?
Do we really need to copy that module if we can specify the full module path in the pam configuration (/etc/pam.d/postgresql) ?
auth required /var/lib/postgresql/.nix-profile/lib/security/pam_jwt_pg.so jwks=https://auth.supabase.green/auth/v1/.well-known/jwks.json mappings=/tmp/users.yaml
account required /var/lib/postgresql/.nix-profile/lib/security/pam_jwt_pg.so jwks=https://auth.supabase.green/auth/v1/.well-known/jwks.json mappings=/tmp/users.yaml
There was a problem hiding this comment.
did not try with the full path, nice!
Did you mean /usr/lib/x86_64-linux-gnu/security/pam_jit_pg.so
Yes original intention was to use that location, but during testing pam was failing to lookup the module and was looking in the store instead. Full paths would solve this
| # Check if the symlink exists in the Linux PAM security directory | ||
| result = run_ssh_command( | ||
| host["ssh"], | ||
| "find /nix/store -type f -path '*/lib/security/pam_jit_pg.so' 2>/dev/null | head -5", |
There was a problem hiding this comment.
Might be related to a previous comment but this will return the lib in the gatekeeper package like this: /nix/store/miwaglm79i2vpilmp8rxd2mymjs7nibz-gatekeeper-0.1.0/lib/security/pam_jit_pg.so. I guess we want to check that /usr/lib/x86_64-linux-gnu/security/pam_jit_pg.so exists instead ?
| print(f"\nJIT PAM module dependencies:\n{result['stdout']}") | ||
|
|
||
| # Check for required libraries | ||
| required_libs = ["libpam", "libc"] |
There was a problem hiding this comment.
Note that these dynamic dependencies/libraries are not coming from Ubuntu but from Nix which could cause problem as Nix and Ubuntu might not use the same PAM version. In the following test, we see that the module is loading PAM 1.6.0 but that version of Ubuntu is using PAM 1.5.3:
$ ldd /var/lib/postgresql/.nix-profile/lib/security/pam_jit_pg.so
linux-vdso.so.1 (0x00007fffa3338000)
libpam.so.0 => /nix/store/k491acz8rcfhrpivv0rg6q83a14zss2j-linux-pam-1.6.0/lib/libpam.so.0 (0x00007fe0c901c000)
libpthread.so.0 => /nix/store/ddwyrxif62r8n6xclvskjyy6szdhvj60-glibc-2.39-5/lib/libpthread.so.0 (0x00007fe0c9017000)
libresolv.so.2 => /nix/store/ddwyrxif62r8n6xclvskjyy6szdhvj60-glibc-2.39-5/lib/libresolv.so.2 (0x00007fe0c9006000)
libc.so.6 => /nix/store/ddwyrxif62r8n6xclvskjyy6szdhvj60-glibc-2.39-5/lib/libc.so.6 (0x00007fe0c8613000)
libaudit.so.1 => /nix/store/0i8hhak73jnlilfgfsh1wh0fd53pb0r9-audit-3.1.2/lib/libaudit.so.1 (0x00007fe0c8fd1000)
$ patchelf --print-rpath /root/.nix-profile/lib/security/pam_jit_pg.so
/nix/store/k491acz8rcfhrpivv0rg6q83a14zss2j-linux-pam-1.6.0/lib:/nix/store/ddwyrxif62r8n6xclvskjyy6szdhvj60-glibc-2.39-5/lib
$ dpkg -l libpam-runtime
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name Version Architecture Description
+++-==============-================-============-===================================
ii libpam-runtime 1.5.3-5ubuntu5.4 all Runtime support for the PAM library
We can instead remove the rpath and let the dynamic linker do its job (without looking at other nix libraries):
$ cp /var/lib/postgres/.nix-profile/lib/security/pam_jit_pg.so /usr/lib/x86_64-linux-gnu/security/
$ patchelf --remove-rpath /usr/lib/x86_64-linux-gnu/security/pam_jit_pg.so
$ ldd /usr/lib/x86_64-linux-gnu/security/pam_jit_pg.so
linux-vdso.so.1 (0x00007fff3179e000)
libpam.so.0 => /lib/x86_64-linux-gnu/libpam.so.0 (0x000078aa645ea000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x000078aa645e5000)
libresolv.so.2 => /lib/x86_64-linux-gnu/libresolv.so.2 (0x000078aa645d2000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x000078aa63a00000)
libaudit.so.1 => /lib/x86_64-linux-gnu/libaudit.so.1 (0x000078aa645a4000)
/lib64/ld-linux-x86-64.so.2 (0x000078aa64602000)
libcap-ng.so.0 => /lib/x86_64-linux-gnu/libcap-ng.so.0 (0x000078aa6459a000)
We could remove that rpath after the build of the package:
postFixup = ''
patchelf --remove-rpath $out/lib/security/pam_jit_pg.so
'';
But still there might be other surprises as the gatekeeper module has been build based on PAM 1.6.0.
Usually PAM has a good backward compatibility and the amount of dyn symbols used by gatekeeper is rather limited:
$ objdump -T /var/lib/postgres/.nix-profile/lib/security/pam_jit_pg.so | grep PAM
0000000000000000 DF *UND* 0000000000000000 (LIBPAM_1.0) pam_strerror
0000000000000000 DF *UND* 0000000000000000 (LIBPAM_EXTENSION_1.0) pam_syslog
0000000000000000 DF *UND* 0000000000000000 (LIBPAM_1.0) pam_get_item
0000000000000000 DF *UND* 0000000000000000 (LIBPAM_1.0) pam_get_user
0000000000000000 DF *UND* 0000000000000000 (LIBPAM_EXTENSION_1.1) pam_get_authtok
If (and only if) we face issue (I would test if further first), I would build it against the same version using https://lazamar.co.uk/nix-versions/?package=linux-pam&version=1.5.3&fullName=linux-pam-1.5.3&keyName=linux-pam&revision=7a339d87931bba829f68e94621536cad9132971a&channel=nixpkgs-unstable#instructions
We might want to create an automated test that exercice the module using pamtester just to verify that we don't get any dlopen errors in the logs.
There was a problem hiding this comment.
we were/are building against 1.6.0 because of postgresql having been built against it (and hence also the hackery of copying the .so to /nix/store/*-linux-pam-*/lib/security because the linker looks for the modules there instead of the default Ubuntu path)
$ patchelf --print-rpath /nix/store/2x79j9fi9j13hhf5n0yc9825qa4nynv3-postgresql-and-plugins-17.6/bin/.postgres-wrapped
/nix/store/bvjhqzpwbzmfi350nir8gf7h45igvp5z-libxml2-2.12.6/lib:/nix/store/wcwqdhl9jpwgj1fdr0pw9dbaf5m43f17-lz4-1.9.4/lib:/nix/store/mxvqrp4jj6zklxc1i32ra8iapnzynyzc-zstd-1.5.5/lib:/nix/store/ram2bh4rs56rp8sr0gcdb108wcc609si-icu4c-73.2/lib:/nix/store/ymav8nb29vsdryddi435xcig3pk7i00q-zlib-1.3.1/lib:/nix/store/0kfbzhd8qdsxsd4qf9dkpcd3f916krvr-openssl-3.0.13/lib:/nix/store/ss6bfrdx69ng9f448d5hq1683hkpxcng-systemd-255.4/lib:/nix/store/vaz0prypndg3y6zxmdcpabinwg9dmmq9-libkrb5-1.21.2/lib:/nix/store/4rdpzzfssknp4k4cn59wc7xcdiyv9j53-linux-pam-1.6.0/lib:/nix/store/10pd749c2v3m35d4hk5xdfylyfr1hzqz-glibc-2.39-5/lib
-->
/nix/store/4rdpzzfssknp4k4cn59wc7xcdiyv9j53-linux-pam-1.6.0/lib
Tried modifying the rpath on postgres-wrapped but that leads to problems looking up libaudit.so.1 (even though it is in the new rpath and when specifying the path manually to the location in the store)
staaldraad
force-pushed
the
sam/custom-pam
branch
from
804a2dc to
18c1570
Compare
staaldraad
force-pushed
the
sam/custom-pam
branch
from
60bc843 to
95ff6b3
Compare
staaldraad
force-pushed
the
sam/custom-pam
branch
3 times, most recently
from
eafa13d to
9c4d5f3
Compare
staaldraad
force-pushed
the
sam/custom-pam
branch
3 times, most recently
from
3a3a4f2 to
2ccbc5f
Compare
Adds https://github.com/supabase/jit-db-gatekeeper to allow for PAM based auth
staaldraad
force-pushed
the
sam/custom-pam
branch
from
462d6b1 to
3a79ba7
Compare
WalkthroughThis PR introduces PAM (Pluggable Authentication Module) and JIT database gatekeeper integration for PostgreSQL deployments. It adds Ansible tasks to configure PAM and conditionally install gatekeeper, defines a new Nix package for the gatekeeper Go module, updates version strings to reflect PAM support, and adds comprehensive test coverage for the PAM integration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![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
♻️ Duplicate comments (2)
nix/packages/gatekeeper.nix (1)
7-32: Consider adding unit tests from upstream.As noted in a previous review, the upstream jit-db-gatekeeper repository contains unit tests. Adding a
checkPhasewould improve confidence in the build.🔎 Suggested checkPhase addition
installPhase = '' runHook preInstall mkdir -p $out/lib/security cp pam_jit_pg.so $out/lib/security/ runHook postInstall ''; + + checkPhase = '' + runHook preCheck + go test -v ./... + runHook postCheck + ''; + + doCheck = true; };testinfra/test_ami_nix.py (1)
642-647: Consider failing instead of using default value when version parsing fails.As noted in a previous review, using a default value when unable to parse the PostgreSQL version could mask issues. Consider failing explicitly instead.
🔎 Suggested change to fail on parse error
result = run_ssh_command( host["ssh"], "sudo -u postgres psql --version | grep -oE '[0-9]+' | head -1" ) - pg_major_version = 15 # Default if result["succeeded"] and result["stdout"].strip(): try: pg_major_version = int(result["stdout"].strip()) except ValueError: - pass + raise AssertionError("Failed to parse PostgreSQL version") + else: + raise AssertionError("Failed to retrieve PostgreSQL version")
🧹 Nitpick comments (2)
nix/packages/gatekeeper.nix (1)
35-47: Consider simplifying by removing the wrapper derivation.The outer derivation simply copies the
.sofile fromupstream-gatekeeperwithout adding value. Consider exportingupstream-gatekeeperdirectly or renaming it togatekeeperif the only purpose is the package name.🔎 Simplified approach
- upstream-gatekeeper = buildGoModule124 { +in + +buildGoModule124 { pname = "jit-db-gatekeeper"; version = "1.0.1"; src = pkgs.fetchFromGitHub { owner = "supabase"; repo = "jit-db-gatekeeper"; rev = "v1.0.1"; sha256 = "sha256-4xSqQnuBYPZU6kl2LVnZbCBLCPUMKyZkezq2mPYox6k"; }; vendorHash = null; buildInputs = [ pkgs.pam ]; buildPhase = '' runHook preBuild go build -buildmode=c-shared -o pam_jit_pg.so runHook postBuild ''; installPhase = '' runHook preInstall mkdir -p $out/lib/security cp pam_jit_pg.so $out/lib/security/ runHook postInstall ''; -}; -in - -pkgs.stdenv.mkDerivation { - pname = "gatekeeper"; - version = "1.0.1"; - - buildInputs = [ upstream-gatekeeper ]; - - dontUnpack = true; - - installPhase = '' - mkdir -p $out/lib/security/ - cp ${upstream-gatekeeper}/lib/security/pam_jit_pg.so $out/lib/security/pam_jit_pg.so - ''; }ansible/tasks/stage2-setup-postgres.yml (1)
158-178: Consider using full module path in PAM configuration.The current approach uses
findto locate the Linux PAM security directory in/nix/storeand create a symlink. As discussed in previous reviews, an alternative approach would be to specify the full path to the module in the PAM configuration file (/etc/pam.d/postgresql), such as:auth required /var/lib/postgresql/.nix-profile/lib/security/pam_jit_pg.so ...This would eliminate the need for the symlink and avoid potential issues if multiple
linux-pamversions exist in the store.Alternative approach using full path
Instead of creating the symlink, configure
/etc/pam.d/postgresqlwith the full path to the module. This would make the setup more explicit and eliminate thefindcommand's brittleness.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ansible/tasks/setup-postgres.ymlansible/tasks/stage2-setup-postgres.ymlansible/vars.ymlnix/packages/default.nixnix/packages/gatekeeper.nixtestinfra/test_ami_nix.py
🧰 Additional context used
🪛 Ruff (0.14.10)
testinfra/test_ami_nix.py
663-663: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
724-724: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
821-821: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nix-eval / eval
🔇 Additional comments (9)
ansible/vars.yml (1)
13-15: LGTM! Version strings consistently updated.The version strings correctly reflect the PAM integration across all PostgreSQL variants with appropriate build number increments and "-pam" suffixes.
ansible/tasks/setup-postgres.yml (1)
178-191: LGTM! PAM configuration properly gated for non-PostgreSQL 15.The placeholder PAM configuration is correctly created only for PostgreSQL versions other than 15, with appropriate permissions and ownership as explained in previous reviews.
nix/packages/default.nix (1)
44-44: LGTM! Gatekeeper package export follows existing patterns.The package is properly exposed in the package set following the same conventions as other entries.
ansible/tasks/stage2-setup-postgres.yml (1)
162-162: LGTM! Simplified version check.The version check now uses direct equality comparison as suggested in previous reviews, making the logic clearer.
testinfra/test_ami_nix.py (4)
636-686: LGTM! Comprehensive PAM module installation test with proper PG15 guards.The test properly verifies the JIT PAM module installation in the Nix profile, checks for symlinks, and validates the shared library. The PostgreSQL 15 skip logic correctly reflects that gatekeeper is not installed for PG15.
805-823: PAM version compatibility should be monitored.As raised in a previous review, the gatekeeper module is built against PAM 1.6.0 (from Nix) but Ubuntu 24.04 uses PAM 1.5.3. While the limited PAM symbol usage suggests compatibility should be maintained, this version mismatch could cause issues. Consider adding a test to verify the PAM versions in use or document this known discrepancy.
Based on learnings from previous reviews regarding PAM version mismatches.
689-733: LGTM! PAM configuration test properly handles version differences.The test correctly verifies PAM configuration exists for non-PG15 versions and confirms its absence for PG15, with appropriate permission checks.
828-855: LGTM! PostgreSQL PAM integration verification is thorough.The test validates PAM-related settings, configuration entries, and compile flags to ensure PostgreSQL is properly configured for PAM authentication.
nix/packages/gatekeeper.nix (1)
10-14: Tag v1.0.1 is confirmed to exist in the upstream repository.The specified tag is available in supabase/jit-db-gatekeeper and represents a valid version reference.
| print(f"\nJIT PAM module found in Nix profile:\n{result['stdout']}") | ||
| else: | ||
| print("\nJIT PAM module not found in postgres user's Nix profile") | ||
| assert False, "JIT PAM module (pam_jit_pg.so) not found in expected location" |
There was a problem hiding this comment.
Replace assert False with raise AssertionError().
Using assert False is problematic because Python's -O flag removes these assertions. Use raise AssertionError() instead for reliable test failures.
🔎 Suggested fixes
else:
print("\nJIT PAM module not found in postgres user's Nix profile")
- assert False, "JIT PAM module (pam_jit_pg.so) not found in expected location"
+ raise AssertionError("JIT PAM module (pam_jit_pg.so) not found in expected location") else:
print("\nPAM config file not found")
- assert False, "PAM configuration file /etc/pam.d/postgresql not found"
+ raise AssertionError("PAM configuration file /etc/pam.d/postgresql not found") # Check for any missing dependencies
if "not found" in result["stdout"].lower():
- assert False, "JIT PAM module has missing dependencies"
+ raise AssertionError("JIT PAM module has missing dependencies")Also applies to: 724-724, 821-821
🧰 Tools
🪛 Ruff (0.14.10)
663-663: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🤖 Prompt for AI Agents
In testinfra/test_ami_nix.py around lines 663, 724 and 821, replace usages of
"assert False, ..." with explicit exceptions so tests don't get stripped with
Python -O; change each "assert False, <message>" to "raise
AssertionError(<message>)" (preserve the original message text) so failures
always raise reliably.
What kind of change does this PR introduce?
I will follow up with an explanation as to why we use this flake-parts approach soon.
The fundamental reasons are discussed at https://flake.parts/index.html
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.