docs: update Arch Linux installation instructions in README#27
Conversation
ccross2
left a comment
There was a problem hiding this comment.
Thanks for picking this up — there's a real catch buried in here.
The success=end → success=done change is correct, and it's bigger than this PR: pam.conf(5) documents exactly ignore | bad | die | ok | done | reset | N. end isn't a valid PAM control keyword, which means libpam logs a warning and treats it as ignore — the visage auth result is silently dropped and the stack continues to pam_unix.so, so users get prompted for a password even on a successful face match. We've been shipping this since v0.1.0.
It appears in 9 places across the repo:
README.md:153
docs/operations-guide.md:117
docs/operations-guide.md:419
docs/architecture.md:603
docs/research/architecture-review-and-roadmap.md:176
docs/research/architecture-review-and-roadmap.md:321
docs/research/domain-audit.md:283
docs/research/howdy-analysis-and-visage-design.md:528
packaging/nix/module.nix:187
packaging/nix/module.nix:192
packaging/debian/pam-auth-update
A README-only fix would leave Debian (via pam-auth-update) and NixOS (via module.nix) users still hitting the bug. I'd like to land the PAM fix as a fleet sweep across all 9 sites in a separate PR rather than spread the correction over multiple commits. I'll open that this session and attribute the catch to you in the commit message.
Two more changes I'd push back on:
git clone visage-git.git (or visage/visage-bin for release)— we only publish the stablevisagePKGBUILD (packaging/aur/PKGBUILD).visage-gitandvisage-binmay exist as third-party AUR entries but aren't maintained by us; recommending them in our README without verifying they're current would be misleading. If you can point me at the AUR maintainers I'll happily document them, but until then I'd keep thevisagepackage as the recommended path.visage enroll --label default --user <your_username>—--userdefaults to$USER(crates/visage-cli/src/main.rs:35: "User to enroll for (defaults to $USER)"), so adding it as the default-shown form is noise rather than clarification. I'd drop the--userannotation unless we want to explain it's there for the cross-account enrollment case.
Two changes I'd be happy to land in this PR:
- The
sudo systemctl enable --now visaged visage-resumeline — solid, the resume unit exists and users should enable it. - The
visage verifystep — good closing verification.
Proposal: would you mind amending to drop the --user annotation and the visage-git/visage-bin alternates? I'll land the PR with the PAM-keyword catch credited to you (Reported-by:), plus the resume-enable line and the verify step. Separately I'll do the fleet PAM sweep.
If you'd rather not amend, no problem — I can land your three good changes as a clean follow-up and credit you the same way. Either path closes this out cleanly; just want to make sure your catch lands properly.
|
Follow-up — I owe you a correction on one part of my review. I checked AUR and confirmed you're the maintainer of all three packages: So the The Separately, if you'd be open to it: would you want to be added to The |
`pam.conf(5)` documents exactly `ignore | bad | die | ok | done | reset | N` as the legal value-keywords for the `[control=value …]` syntax. `end` is not in that list — libpam logs a warning and silently treats the unknown keyword as `ignore`, which means a successful face match (`PAM_SUCCESS` from `pam_visage.so`) was being dropped and the stack fell through to the next auth rule. In practice, users with the documented setup were still seeing a password prompt on every authentication; face auth appeared to succeed but had no effect on the stack flow. This has shipped since v0.1.0. The bug appeared in 9 places (some files contained two occurrences): README.md docs/architecture.md docs/operations-guide.md (×2) docs/research/architecture-review-and-roadmap.md (×2) docs/research/domain-audit.md docs/research/howdy-analysis-and-visage-design.md packaging/debian/pam-auth-update packaging/nix/module.nix (×2: sudo + login rules) Reported-by: @SelfRef <#27> Existing-user impact: anyone who manually edited `/etc/pam.d/system-auth` following the prior README (Arch path) or installed an older `.deb` / NixOS rebuild needs to swap `success=end` for `success=done` in their config and re-test. The CHANGELOG entry calls this out.
Bug-fix release covering two real shipping-user issues in v0.3.0: - **PAM control keyword corrected.** `success=end` was not a valid `pam.conf(5)` value-keyword — libpam treated it as `ignore` and dropped Visage's authentication result, so face auth was silently falling through to the password prompt since v0.1.0. Swept across all 9 affected sites (README, docs, NixOS module, Debian pam-auth-update profile, research docs). Reported-by @SelfRef in #27. Debian/Ubuntu users auto-recover on next `.deb` upgrade via `postinst`'s `pam-auth-update --package` call; Arch and NixOS users get the fix from the new install. - **`visaged` SIGTERM handler.** The shutdown signal was using `tokio::signal::ctrl_c()`, which is SIGINT-only on Unix. `systemctl stop|restart` and `visage-resume.service` post-hibernate send SIGTERM, which the daemon ignored — systemd then waited the default `TimeoutStopSec=90s` before SIGKILL. Fixes #26. Daemon now handles both SIGINT and SIGTERM via `tokio::signal::unix::signal` and `tokio::select!`, matching the pattern used elsewhere in our daemons. `visaged.service` adds `TimeoutStopSec=10s` as defense in depth. Workspace version bumped 0.3.0 → 0.3.2 across `Cargo.toml`, `Cargo.lock`, `packaging/aur/PKGBUILD`, and `packaging/nix/default.nix`. CHANGELOG section renamed from `[Unreleased]` and dated. Skipping v0.3.1 — that number was reserved for the dependency-bump cohort (image, nix, tokio, uuid, GitHub Actions) plus the community fork PRs (#25 Arch LTO fix, #29 X1 Carbon quirk). Those are still blocked on fork-PR CI approval and will land as v0.3.3 once unblocked.
|
@SelfRef no rush on this — the PAM keyword fix from your review already landed in v0.3.2 via #31 (with credit to you). The remaining items in this PR are still useful; whenever you have time to look at the |
|
@ccross2 Thank you for the review. I'm glad the PAM issue was fixed globally!
I'll be glad to help! Since I use this project personally, I'm actively maintain the packages anyway. About the PR: That's why in my case at least the Shouldn't the app use |
Type
contrib/hw/*.toml)Description
This pull request updates the installation instructions for Arch Linux in the
README.mdto clarify package names, improve setup steps, and correct PAM configuration guidance.Installation and setup improvements:
visage-git,visage, orvisage-binfor release), and clarified the usage ofmakepkgandsystemctlcommands.enrollcommand to include the--user <your_username>option and added an explicit verification step withvisage verify.Configuration correction:
[success=done default=ignore]instead of[success=end default=ignore]for thepam_visage.soline.