refactor(matic): replace systemd service with PAM exec for keyring unlock - #1170
Conversation
…lock Move GNOME Keyring TPM2 unlock from a systemd system service with retry loop to a pam_exec rule that runs immediately after pam_gnome_keyring in the greetd PAM session stack. Eliminates timing/retry issues entirely: - pam_gnome_keyring (order 12600): starts daemon - pam_exec (order 12610): decrypts TPM credential, speaks control socket protocol directly as the user via runuser Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello, 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 refactors the GNOME Keyring auto-unlock mechanism, particularly for fingerprint logins. By transitioning from a systemd service to a pam_exec rule, the solution is now more robust and efficient, directly integrating into the PAM session flow and resolving previous timing-related issues. 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. Footnotes
|
Mesa DescriptionTL;DRReplaces the systemd service + retry loop for GNOME Keyring auto-unlock with a What changed?
Description generated by Mesa. Update settings |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces the systemd-based GNOME Keyring auto-unlock with a PAM-driven flow that uses Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Greetd as Greetd
participant PAM as PAM
participant PamScript as pam_exec script
participant TPM as TPM2 / systemd-creds
participant UnlockPy as unlock-gnome-keyring.py
participant Keyring as GNOME Keyring Daemon
alt Fingerprint login (no password)
User->>Greetd: Authenticate (fingerprint)
Greetd->>PAM: Open session
PAM->>PamScript: Execute gnome_keyring_tpm_unlock (pam_exec)
PamScript->>TPM: Decrypt credentials
TPM-->>PamScript: Return creds
PamScript->>UnlockPy: Run as PAM user (control socket)
UnlockPy->>Keyring: Send unlock via control socket
Keyring-->>UnlockPy: Return unlock result
UnlockPy-->>PamScript: Exit code/result
PamScript-->>PAM: Return success/failure
else Password login (password available)
User->>Greetd: Authenticate (password)
Greetd->>PAM: Open session (password forwarded)
PAM->>Keyring: `pam_gnome_keyring.so` forwards password -> auto-unlock
Keyring-->>PAM: Unlock result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
📝 Coding Plan
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 |
![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
The refactoring from a systemd service to a pam_exec rule for GNOME Keyring auto-unlock is a significant improvement. It correctly addresses the timing issues and retry loops inherent in the previous approach, leading to a more robust and reliable solution, especially for fingerprint logins. The updates to default.nix accurately reflect the new PAM configuration, and the Python script has been simplified by removing the retry logic. The README.md has also been updated to explain the new mechanism clearly. One minor point: the README.md previously included a note explaining why the credential file must be located at /etc/credstore.encrypted/gnome-keyring.cred (related to TPM access for system vs. user services). While the implementation has changed, the location of the credential remains the same, and the rationale behind this specific path is still valuable context for maintainers. Consider re-adding this explanation to the README.md, perhaps in the "One-time setup" section or as a separate note, to ensure this important detail is not lost.
|
|
||
| > **Note:** `gnome-keyring-daemon --unlock` (v48+) ignores `GNOME_KEYRING_CONTROL` and always starts a fresh instance. The service works around this by writing directly to `$XDG_RUNTIME_DIR/keyring/control` using the binary protocol: credentials byte + big-endian `[oplen][op=1][pwlen][password]`, reads `[8][result]`. | ||
| > | ||
| > **Note:** The credential must be at `/etc/credstore.encrypted/gnome-keyring.cred` (not `~/.config`). User-level systemd services cannot access TPM/host keys — only the system manager can. |
There was a problem hiding this comment.
The previous version of the README included a note explaining why the credential file must be located at /etc/credstore.encrypted/gnome-keyring.cred (i.e., "User-level systemd services cannot access TPM/host keys — only the system manager can."). While the implementation has shifted from a systemd service to a PAM script, the credential location remains the same, and the rationale for this specific path is still valuable context for understanding the system's design. Consider re-adding this explanation to the README, perhaps in the "One-time setup" section or as a separate note, to prevent loss of this important detail for future maintainers.
The PAM exec script was failing (exit code 1) because the gnome-keyring-daemon control socket isn't always ready by the time pam_exec runs. Add a poll loop (up to 5s) matching the original systemd service's retry logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors GNOME Keyring auto-unlock on matic to run from the greetd PAM session stack via pam_exec, replacing the previous systemd system service + retry loop approach.
Changes:
- Removes the
gnome-keyring-unlocksystemd oneshot service and its retry loop logic. - Adds a
security.pam.services.greetd.rules.sessionpam_exec.sorule that decrypts a TPM2 credential and unlocks the running keyring daemon via the control socket protocol. - Updates host README to document the new PAM-based flow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| named-hosts/matic/default.nix | Drops systemd service and introduces PAM pam_exec session rule + root→runuser unlock script |
| named-hosts/matic/README.md | Updates operational docs to reflect PAM exec approach and new flow details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| pamScript = pkgs.writeShellScript "pam-gnome-keyring-tpm-unlock" '' | ||
| CRED="/etc/credstore.encrypted/gnome-keyring.cred" | ||
| [ -f "$CRED" ] || exit 0 | ||
| SOCK="/run/user/$(id -u "$PAM_USER")/keyring/control" | ||
| for i in 1 2 3 4 5; do | ||
| [ -S "$SOCK" ] && break | ||
| sleep 1 | ||
| done |
| pw = sys.stdin.read().rstrip("\n") | ||
| result = unlock(pw) | ||
| codes = {0: "OK", 1: "DENIED", 2: "FAILED", 3: "NO_DAEMON"} | ||
| uid = os.getuid() | ||
| sock_path = f"/run/user/{uid}/keyring/control" | ||
|
|
||
| for attempt in range(10): | ||
| # Wait for the control socket to appear (keyring daemon to start) | ||
| if not os.path.exists(sock_path): | ||
| print(f"attempt {attempt+1}: waiting for control socket...", flush=True) | ||
| time.sleep(3) | ||
| continue | ||
| result = unlock(pw) | ||
| print(f"attempt {attempt+1}: gnome-keyring unlock: {codes.get(result, result)}", flush=True) | ||
| if result == 0: | ||
| sys.exit(0) | ||
| # DENIED might mean daemon not fully ready yet, retry | ||
| time.sleep(3) | ||
|
|
||
| print("gnome-keyring unlock: gave up after 10 attempts", flush=True) | ||
| sys.exit(1) | ||
| print(f"gnome-keyring unlock: {codes.get(result, result)}", flush=True) | ||
| sys.exit(0 if result == 0 else 1) |
|
|
||
| print("gnome-keyring unlock: gave up after 10 attempts", flush=True) | ||
| sys.exit(1) | ||
| print(f"gnome-keyring unlock: {codes.get(result, result)}", flush=True) |
| 1. `pam_gnome_keyring.so` starts the keyring daemon during PAM session open (order 12600). | ||
| 2. For **password login**: PAM forwards the password and the keyring auto-unlocks. | ||
| 3. For **fingerprint login**: PAM has no password, so the keyring stays locked. Immediately after, `pam_exec.so` (order 12610) runs a script that: |
| ```bash | ||
| sudo systemctl restart gnome-keyring-unlock.service | ||
| ``` | ||
| The keyring password must be your **system login password** (the one PAM uses when you log in with password). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
named-hosts/matic/default.nix (1)
174-176:⚠️ Potential issue | 🟠 MajorPotential infinite loop if socket closes prematurely.
s.recv()returns empty bytes (b"") when the peer closes the connection. Thewhile len(resp) < 8loop will spin indefinitely sincerespnever grows.🛡️ Proposed fix: detect closed socket and add timeout
+ s.settimeout(5.0) resp = b"" while len(resp) < 8: - resp += s.recv(8 - len(resp)) + chunk = s.recv(8 - len(resp)) + if not chunk: + raise RuntimeError("socket closed before response") + resp += chunk🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/matic/default.nix` around lines 174 - 176, The loop that reads 8 bytes using resp and s.recv can hang if the peer closes the socket (recv returns b""); update the read loop in the section using resp and s.recv to detect when recv returns b"" and handle it (e.g., raise a clear exception or return an error) instead of continuing; additionally set a socket timeout (s.settimeout(...)) or use select.select()/poll before recv to avoid indefinite blocking and surface timeouts to the caller. Ensure the change preserves the intent of reading exactly 8 bytes but aborts with a descriptive error when the connection is closed or a timeout occurs.
🧹 Nitpick comments (1)
named-hosts/matic/default.nix (1)
189-202: Considerset -o pipefailfor clearer failure diagnosis.Without
pipefail, the script's exit code reflects only the last command (unlockPy). Ifsystemd-creds decryptfails, the Python script receives empty input and may report a misleading error. Sincecontrol = "optional", this won't block login, but it could complicate debugging.♻️ Proposed improvement
pamScript = pkgs.writeShellScript "pam-gnome-keyring-tpm-unlock" '' + set -o pipefail CRED="/etc/credstore.encrypted/gnome-keyring.cred"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/matic/default.nix` around lines 189 - 202, The shell script assigned to pamScript can hide upstream failures because it lacks pipefail; add a shell option like set -o pipefail (or set -euo pipefail if you want stricter checks) near the top of the pamScript body so that failures from ${pkgs.systemd}/bin/systemd-creds decrypt (the first half of the pipeline) propagate instead of being masked by the final ${unlockPy} command; keep the rest of the logic (CRED, SOCK, PAM_USER, unlockPy) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@named-hosts/matic/default.nix`:
- Around line 174-176: The loop that reads 8 bytes using resp and s.recv can
hang if the peer closes the socket (recv returns b""); update the read loop in
the section using resp and s.recv to detect when recv returns b"" and handle it
(e.g., raise a clear exception or return an error) instead of continuing;
additionally set a socket timeout (s.settimeout(...)) or use
select.select()/poll before recv to avoid indefinite blocking and surface
timeouts to the caller. Ensure the change preserves the intent of reading
exactly 8 bytes but aborts with a descriptive error when the connection is
closed or a timeout occurs.
---
Nitpick comments:
In `@named-hosts/matic/default.nix`:
- Around line 189-202: The shell script assigned to pamScript can hide upstream
failures because it lacks pipefail; add a shell option like set -o pipefail (or
set -euo pipefail if you want stricter checks) near the top of the pamScript
body so that failures from ${pkgs.systemd}/bin/systemd-creds decrypt (the first
half of the pipeline) propagate instead of being masked by the final ${unlockPy}
command; keep the rest of the logic (CRED, SOCK, PAM_USER, unlockPy) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65fe1c0c-028c-4084-b8c4-ba0da71f090f
📒 Files selected for processing (2)
named-hosts/matic/README.mdnamed-hosts/matic/default.nix
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The gnome-keyring-daemon p11-kit backend fails to initialize at PAM session-open time (GCK_IS_SESSION assertions), causing the control socket to return DENIED immediately even with the correct password. The daemon becomes fully operational a few seconds after the user session starts (evidenced by discover_other_daemon and gcr-prompter). Decrypt the TPM credential synchronously (needs root), then fork a background retry loop (every 3s, up to 8 attempts) so PAM is never blocked and the unlock hits the fully-initialized daemon. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="named-hosts/matic/default.nix">
<violation number="1" location="named-hosts/matic/default.nix:211">
P2: The `runuser` → Python unlock command has no execution timeout. If the gnome-keyring daemon accepts the socket connection but stalls (e.g., during initialization), the Python `recv` loop blocks forever, leaving an orphaned background process per login. Wrap the command in `timeout` to bound each attempt.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| for attempt in 1 2 3 4 5 6 7 8; do | ||
| sleep 3 | ||
| [ -S "$SOCK" ] || { log "attempt $attempt: socket not found"; continue; } | ||
| OUT=$(printf '%s' "$PW" | \ |
There was a problem hiding this comment.
P2: The runuser → Python unlock command has no execution timeout. If the gnome-keyring daemon accepts the socket connection but stalls (e.g., during initialization), the Python recv loop blocks forever, leaving an orphaned background process per login. Wrap the command in timeout to bound each attempt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At named-hosts/matic/default.nix, line 211:
<comment>The `runuser` → Python unlock command has no execution timeout. If the gnome-keyring daemon accepts the socket connection but stalls (e.g., during initialization), the Python `recv` loop blocks forever, leaving an orphaned background process per login. Wrap the command in `timeout` to bound each attempt.</comment>
<file context>
@@ -190,25 +190,35 @@ inputs.nixpkgs.lib.nixosSystem {
+ for attempt in 1 2 3 4 5 6 7 8; do
+ sleep 3
+ [ -S "$SOCK" ] || { log "attempt $attempt: socket not found"; continue; }
+ OUT=$(printf '%s' "$PW" | \
+ ${pkgs.util-linux}/bin/runuser -u "$PAM_USER" -- \
+ ${pkgs.coreutils}/bin/env XDG_RUNTIME_DIR="/run/user/$USER_UID" \
</file context>
| OUT=$(printf '%s' "$PW" | \ | |
| OUT=$(printf '%s' "$PW" | \ | |
| ${pkgs.coreutils}/bin/timeout 10 \ |
The background retry was spawning for the greeter user (uid=990) and producing noisy "socket not found" log entries after its session ended. Skip early for non-login users. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@named-hosts/matic/default.nix`:
- Around line 124-128: Update the explanatory comment that begins "Unlock GNOME
Keyring via TPM2 credential at login (PAM exec)" to reflect actual behavior:
remove or change the sentence claiming "so there are no timing/retry issues" and
instead document that the PAM exec includes a retry loop to handle
timing/availability of the TPM/control socket; reference the retry logic
implemented later in the same PAM exec block and briefly describe the retry
purpose and limits so operator expectations match the code.
- Around line 195-207: The decrypt step runs before validating PAM_USER/UID,
causing unnecessary decryption for system/greeter sessions; move the USER_UID
derivation and the "[ \"$USER_UID\" -lt 1000 ] && exit 0" guard above the
PW=$(${pkgs.systemd}/bin/systemd-creds decrypt ...) line so you only call
systemd-creds and populate PW when PAM_USER is present and UID >= 1000; ensure
you still handle missing/invalid PAM_USER (e.g., id -u failure) before
attempting decryption and keep the existing log/exit behavior if decryption
later fails.
🪄 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: c4989057-47ed-41e2-9a2d-ed464086ceae
📒 Files selected for processing (1)
named-hosts/matic/default.nix
| PW=$(${pkgs.systemd}/bin/systemd-creds decrypt --name=gnome-keyring "$CRED" -) | ||
| if [ -z "$PW" ]; then | ||
| log "credential decrypt failed" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # The gnome-keyring-daemon p11-kit backend is not fully initialized at | ||
| # PAM session-open time — unlock attempts at this point return DENIED. | ||
| # Fork a background retry loop so login is never blocked; the daemon | ||
| # is ready within a few seconds of the user session starting. | ||
| USER_UID=$(id -u "$PAM_USER") | ||
| # Skip system/greeter users (uid < 1000) | ||
| [ "$USER_UID" -lt 1000 ] && exit 0 |
There was a problem hiding this comment.
Gate by PAM_USER/UID before decrypting the TPM credential.
Line 195 decrypts the secret before the Line 207 system-user skip. This unnecessarily handles plaintext credentials for greeter/system sessions and does extra privileged work when PAM_USER is invalid/missing.
Suggested reorder + guard
-# Decrypt synchronously — requires root/TPM access (not available after fork).
-PW=$(${pkgs.systemd}/bin/systemd-creds decrypt --name=gnome-keyring "$CRED" -)
-if [ -z "$PW" ]; then
- log "credential decrypt failed"
- exit 1
-fi
-
# The gnome-keyring-daemon p11-kit backend is not fully initialized at
# PAM session-open time — unlock attempts at this point return DENIED.
# Fork a background retry loop so login is never blocked; the daemon
# is ready within a few seconds of the user session starting.
-USER_UID=$(id -u "$PAM_USER")
+[ -n "${PAM_USER:-}" ] || exit 0
+USER_UID=$(id -u "$PAM_USER" 2>/dev/null) || exit 0
# Skip system/greeter users (uid < 1000)
[ "$USER_UID" -lt 1000 ] && exit 0
+
+# Decrypt synchronously — requires root/TPM access (not available after fork).
+PW=$(${pkgs.systemd}/bin/systemd-creds decrypt --name=gnome-keyring "$CRED" -)
+if [ -z "$PW" ]; then
+ log "credential decrypt failed"
+ exit 1
+fi
SOCK="/run/user/$USER_UID/keyring/control"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@named-hosts/matic/default.nix` around lines 195 - 207, The decrypt step runs
before validating PAM_USER/UID, causing unnecessary decryption for
system/greeter sessions; move the USER_UID derivation and the "[ \"$USER_UID\"
-lt 1000 ] && exit 0" guard above the PW=$(${pkgs.systemd}/bin/systemd-creds
decrypt ...) line so you only call systemd-creds and populate PW when PAM_USER
is present and UID >= 1000; ensure you still handle missing/invalid PAM_USER
(e.g., id -u failure) before attempting decryption and keep the existing
log/exit behavior if decryption later fails.
- Fix infinite recv loop: check for empty bytes (daemon closed connection) - Validate PAM_USER is non-empty before use - Check systemd-creds exit code explicitly; redirect stderr to /dev/null - Check id -u exit code; log specific error if UID resolution fails - Log clear message when all retry attempts are exhausted Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Replaces the systemd system service + retry loop approach for GNOME Keyring auto-unlock with a
pam_execrule in the greetd PAM session stack.Before: System service triggered by
user@1000.servicewith a 10-attempt retry loop (3s apart) to handle the daemon startup race.After:
pam_exec.soruns at order 12610, immediately afterpam_gnome_keyring.so(12600) starts the daemon. No timing issues, no retries.The PAM script:
systemd-creds decryptto access the TPM2 credentialrunuser -u $PAM_USERto switch to the target user (required forSO_PEERCREDcheck on the control socket)--unlock)Test plan
make build && make switchsucceeds🤖 Generated with Claude Code
Summary by cubic
Replaces the GNOME Keyring auto-unlock systemd service with a
pam_exechook in thegreetdPAM session to unlock via TPM2 without blocking fingerprint logins. Decrypts as root, unlocks via the control socket as the user, retries in the background, and further hardens error handling.Refactors
pam_execrule afterpam_gnome_keyring; usessystemd-credsandrunuser, speaks the control socket protocol, retries in the background (every 3s, up to 8), and skips system/greeter users (uid < 1000).$PAM_USER, checksystemd-credsandid -uexit codes, handle closed socket reads, and log attempt results and when retries are exhausted.services.fprintdand configuresecurity.pam.services.greetdwithfprintAuthandenableGnomeKeyring. Update README to state the keyring password must match the system login password.Migration
/etc/credstore.encrypted/gnome-keyring.credexists (create withsystemd-creds encrypt --with-key=tpm2+host).Written for commit 47bd108. Summary will update on new commits.