Skip to content

feat(cli): skip xauth setup when peer-credential auth is sufficient#1551

Open
agirault wants to merge 1 commit into
mainfrom
peer-creds-probe
Open

feat(cli): skip xauth setup when peer-credential auth is sufficient#1551
agirault wants to merge 1 commit into
mainfrom
peer-creds-probe

Conversation

@agirault

@agirault agirault commented May 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Probe the X server with XAUTHORITY=/dev/null xset -display $DISPLAY q before attempting the xauth cookie setup. If that succeeds, a non-cookie auth family (typically SI:localuser, granted to the same UID via the X server's peer-credential check on the UNIX socket) is already accepting the connection. The container runs as the same UID via -u $(id -u):$(id -g) and shares the X socket, so the same auth applies inside. Cookie setup, temp file, and the "no entries" warning are all skipped in that case.

Why

A user reported this warning after the original X11/Wayland auto-detect PR landed:

Warning: xauth nlist returned no entries for DISPLAY=:1; X11 may not authenticate inside the container.

with X11 still working fine inside the container. Root cause: on most modern Linux desktops, the cookie for the active local display is written to a display-manager-owned Xauthority file (/run/user/<uid>/gdm/Xauthority, etc.), not the user's default ~/.Xauthority. When xauth nlist reads the default file in a fresh shell, it finds nothing. The container still works because the X server is granting access via SI:localuser:<user> peer-credential auth on the UNIX socket, which does not require a cookie at all.

Probe cost

One xset invocation (~5 ms), timeout=2 so a stale DISPLAY cannot hang. Read-only, no side effects.

Behavior

  • Peer-creds works (most local UNIX-socket displays): skip xauth entirely; no warning, no temp file, no extra mount.
  • Peer-creds does not work (SSH-forwarded TCP DISPLAY, or a host without SI:localuser): fall through to existing cookie path; warning fires only if cookie path also fails.
  • xset not installed: probe returns False, behavior identical to today.

Test plan

  • python3 -m unittest utilities.cli.tests.test_container -> 27 tests pass (1 new)
  • On a local UNIX-socket DISPLAY where xauth nlist is empty: no warning, GUI still renders inside container
  • On a TCP DISPLAY (localhost:N SSH-forwarded): probe fails, cookie path runs, warning silent
  • On a host without xset: probe returns False, cookie path runs as today

@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@agirault has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 21 minutes and 21 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d21b8086-2c28-4444-a2a4-36684009ad1a

📥 Commits

Reviewing files that changed from the base of the PR and between c51d002 and 8253490.

📒 Files selected for processing (2)
  • utilities/cli/container.py
  • utilities/cli/tests/test_container.py

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@agirault agirault requested review from bhashemian and tbirdso and removed request for bhashemian May 13, 2026 00:13
@greptile-apps

greptile-apps Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a pre-probe step to _get_xauth_options that queries the X server with no cookie file before attempting xauth cookie setup. If the X server already grants access via peer-credential auth (SI:localuser), the cookie path is skipped entirely — eliminating a spurious warning on most modern Linux desktops where xauth nlist finds nothing but X11 still works inside the container.

  • _peer_creds_sufficient (new static method): runs a 2-second-timeout, read-only xset probe; handles missing xset, timeouts, and OSError gracefully, always returning False on any failure.
  • _get_xauth_options update: the probe runs unconditionally (even in --dry-run) so the printed command accurately reflects the real launch path; falls through to the existing cookie path only when peer-creds fail.
  • Tests: five new tests added, covering xset missing, exit-zero, exit-nonzero, timeout, and the end-to-end path through get_display_options; existing xauth tests updated to mock the new probe.

Confidence Score: 5/5

Safe to merge — the probe is read-only, has a hard timeout, fails closed on any error, and the two previously raised concerns have both been addressed in this revision.

The change is narrow and defensive: a short-lived subprocess probe that always returns False on any failure, an early-return guard that changes nothing for non-peer-creds paths, and five dedicated tests covering the key branches. No data is mutated, no new mounts are added, and the fallback to the existing cookie path is intact.

No files require special attention.

Important Files Changed

Filename Overview
utilities/cli/container.py Adds _peer_creds_sufficient static method and early-return guard in _get_xauth_options; logic is correct, exception handling is comprehensive, and dryrun alignment is explicitly addressed by the comment.
utilities/cli/tests/test_container.py Five new tests cover the full probe surface (missing xset, returncode 0/non-zero, timeout, and integration via get_display_options); existing tests correctly updated to mock the probe.

Reviews (2): Last reviewed commit: "feat(cli): skip xauth setup when peer-cr..." | Re-trigger Greptile

Comment thread utilities/cli/container.py
Comment thread utilities/cli/tests/test_container.py
Probe the X server with `XAUTHORITY=/dev/null xset -display $DISPLAY q`
before reaching for the xauth cookie path. If that succeeds, a non-cookie
auth family (typically SI:localuser, granted to the same UID via the X
server's peer-credential check) is already accepting the connection, and
the container - running as the same UID via `-u $(id -u):$(id -g)` and
sharing the X UNIX socket - will be accepted by the same mechanism. The
cookie dance, the warning when no entries are found, and the temp file are
all skipped in that case.

This eliminates a noisy "xauth nlist returned no entries" warning that
fired on local DISPLAYs whose cookies live in a display-manager-owned
Xauthority file outside `\$HOME/.Xauthority`, even though X11 worked fine
inside the container via peer-creds.

Signed-off-by: Alexis Girault <agirault@nvidia.com>
@agirault agirault force-pushed the peer-creds-probe branch from e56527a to 8253490 Compare May 13, 2026 00:44
@agirault agirault enabled auto-merge (squash) May 14, 2026 09:48
@bhashemian

Copy link
Copy Markdown
Member

@agirault considering the recent refactoring into holoscan-cli, is this PR still relevant? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants