From d643a87148c898420cd79e80a105ed357d244e1e Mon Sep 17 00:00:00 2001 From: ccross Date: Thu, 28 May 2026 11:57:03 -0400 Subject: [PATCH] =?UTF-8?q?fix(pam):=20correct=20success=3Dend=20=E2=86=92?= =?UTF-8?q?=20success=3Ddone=20across=20all=209=20sites?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 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. --- CHANGELOG.md | 14 ++++++++++++++ README.md | 2 +- docs/architecture.md | 2 +- docs/operations-guide.md | 4 ++-- docs/research/architecture-review-and-roadmap.md | 4 ++-- docs/research/domain-audit.md | 2 +- docs/research/howdy-analysis-and-visage-design.md | 2 +- packaging/debian/pam-auth-update | 2 +- packaging/nix/module.nix | 4 ++-- 9 files changed, 25 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19da65b..c3a413d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,20 @@ ### Fixed +- **PAM control keyword corrected: `success=end` → `success=done` across all 9 sites.** + `pam.conf(5)` documents exactly `ignore | bad | die | ok | done | reset | N` — + `end` is not a valid keyword. libpam logged a warning and treated it as + `ignore`, meaning a successful face match silently fell through to the next + rule (typically `pam_unix.so` → password prompt) instead of terminating the + auth stack with success. Affected: `README.md`, `docs/operations-guide.md`, + `docs/architecture.md`, `packaging/debian/pam-auth-update` (Ubuntu), + `packaging/nix/module.nix` (NixOS — `sudo` and `login` rules), and several + research docs. Caught by @SelfRef in #27. **Note for existing users:** if your + PAM stack still references the old keyword (e.g. you manually edited + `/etc/pam.d/system-auth` on Arch from the prior README, or you're on an old + Debian/Ubuntu install that hasn't re-run `pam-auth-update`), face auth has + been working as if Visage weren't installed — replace `success=end` with + `success=done` and re-test. - **`visaged` now handles SIGTERM correctly.** The shutdown signal handler in `crates/visaged/src/main.rs` previously relied on `tokio::signal::ctrl_c()`, which is SIGINT-only on Unix. `systemctl stop` / `systemctl restart` (and diff --git a/README.md b/README.md index 7c00d5e..9b408d4 100644 --- a/README.md +++ b/README.md @@ -150,7 +150,7 @@ PAM requires a manual one-line edit on Arch — add before `pam_unix.so` in `/etc/pam.d/system-auth`: ``` -auth [success=end default=ignore] pam_visage.so +auth [success=done default=ignore] pam_visage.so ``` ### What the package does diff --git a/docs/architecture.md b/docs/architecture.md index b46a3ad..afe22ab 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -600,7 +600,7 @@ enforces the same checksums at startup (fail-closed). See [ADR 009](decisions/00 The pam-auth-update profile places Visage at priority 900: ``` -[success=end default=ignore] pam_visage.so +[success=done default=ignore] pam_visage.so ``` - Face match (`PAM_SUCCESS`) → authentication succeeds, skips password diff --git a/docs/operations-guide.md b/docs/operations-guide.md index e09e380..4a91d80 100644 --- a/docs/operations-guide.md +++ b/docs/operations-guide.md @@ -114,7 +114,7 @@ PAM is **not** configured automatically on Arch. Add the following line **before for sudo only): ``` -auth [success=end default=ignore] pam_visage.so +auth [success=done default=ignore] pam_visage.so ``` Then complete setup: @@ -416,7 +416,7 @@ Output: grep pam_visage /etc/pam.d/common-auth ``` -Should show: `auth [success=end default=ignore] pam_visage.so` +Should show: `auth [success=done default=ignore] pam_visage.so` If missing, run: `sudo pam-auth-update` and enable Visage. diff --git a/docs/research/architecture-review-and-roadmap.md b/docs/research/architecture-review-and-roadmap.md index fe6708c..fb75880 100644 --- a/docs/research/architecture-review-and-roadmap.md +++ b/docs/research/architecture-review-and-roadmap.md @@ -173,7 +173,7 @@ of UVC control bytes are the adoption growth path. |------------|---------------| | Split daemon into privileged broker + unprivileged inference worker | Over-engineering for v1.0. systemd hardening provides equivalent protection. Revisit when CVE risk in ort is demonstrated. | | libcamera support | Windows Hello IR cameras are UVC devices, accessible via V4L2. libcamera targets MIPI/ISP pipelines (RPi, embedded). Add when a user submits a device that needs it. | -| Multi-factor orchestration and risk tiers | Enterprise PAM policy territory. PAM stack already handles this — `[success=end default=ignore]` is the orchestration. Not Visage's job. | +| Multi-factor orchestration and risk tiers | Enterprise PAM policy territory. PAM stack already handles this — `[success=done default=ignore]` is the orchestration. Not Visage's job. | | Polkit integration | Desktop integration milestone 5+. Core PAM → D-Bus → daemon path works without it. | | ONNX Runtime distribution strategy | The `ort` crate handles this (download-on-build or system library detection). Packaging concern, not architecture. Address during packaging milestone. | @@ -318,7 +318,7 @@ control = [1, 3, 3, 0, 0, 0, 0, 0, 0] - `.deb` package via `cargo-deb` — configured in `crates/visaged/Cargo.toml` - `packaging/systemd/visaged.service` — hardened unit (ProtectSystem=strict, DeviceAllow, CapabilityBoundingSet empty, MemoryDenyWriteExecute=false for ONNX Runtime JIT) -- `packaging/debian/pam-auth-update` — pam-configs profile, priority 900, `[success=end default=ignore]` +- `packaging/debian/pam-auth-update` — pam-configs profile, priority 900, `[success=done default=ignore]` - `packaging/debian/postinst` — creates `/var/lib/visage`, runs `pam-auth-update --package`, enables service - `packaging/debian/prerm` — stops service, runs `pam-auth-update --remove` - `packaging/debian/postrm` — purges `/var/lib/visage` on `apt purge` diff --git a/docs/research/domain-audit.md b/docs/research/domain-audit.md index b6180e8..ea104ce 100644 --- a/docs/research/domain-audit.md +++ b/docs/research/domain-audit.md @@ -280,7 +280,7 @@ No new knowledge base needed. These are straightforward implementation tasks. **Complexity:** Medium -Profile file at `/usr/share/pam-configs/visage`. Use `Auth-Type: Primary` and `[success=end default=ignore]` flags (not `sufficient`). +Profile file at `/usr/share/pam-configs/visage`. Use `Auth-Type: Primary` and `[success=done default=ignore]` flags (not `sufficient`). **Risk:** If `postinst` fails mid-execution with a partially modified PAM stack, the system may be in a broken auth state. Use `set -e` in postinst. Test on a clean Ubuntu 24.04 VM as the first thing in Step 6. diff --git a/docs/research/howdy-analysis-and-visage-design.md b/docs/research/howdy-analysis-and-visage-design.md index 55e7af0..a81035a 100644 --- a/docs/research/howdy-analysis-and-visage-design.md +++ b/docs/research/howdy-analysis-and-visage-design.md @@ -525,7 +525,7 @@ Measure time from `Verify()` D-Bus call to response, with model pre-loaded: 3. **CLAHE histogram equalization** — Essential for IR frame quality 4. **Dark frame histogram filter** — Cheap and effective IR warm-up detection 5. **Parallel model loading + camera init** — Hide latency behind I/O -6. **PAM `[success=end default=ignore]`** — Correct fallback semantics +6. **PAM `[success=done default=ignore]`** — Correct fallback semantics 7. **L2 distance matching** — Correct metric for dlib embeddings (cosine for ArcFace) 8. **Per-model metadata (id, timestamp, label)** — Useful for management 9. **Rubberstamps plugin concept** — Extensible post-auth verification diff --git a/packaging/debian/pam-auth-update b/packaging/debian/pam-auth-update index 66cd573..8c606e7 100644 --- a/packaging/debian/pam-auth-update +++ b/packaging/debian/pam-auth-update @@ -3,4 +3,4 @@ Default: yes Priority: 900 Auth-Type: Primary Auth: - [success=end default=ignore] pam_visage.so + [success=done default=ignore] pam_visage.so diff --git a/packaging/nix/module.nix b/packaging/nix/module.nix index f5fbedc..c1b94fe 100644 --- a/packaging/nix/module.nix +++ b/packaging/nix/module.nix @@ -184,12 +184,12 @@ in security.pam.services = lib.mkIf cfg.pam.enable { sudo.rules.auth.visage = { order = 900; - control = "[success=end default=ignore]"; + control = "[success=done default=ignore]"; modulePath = "${cfg.package}/lib/security/pam_visage.so"; }; login.rules.auth.visage = { order = 900; - control = "[success=end default=ignore]"; + control = "[success=done default=ignore]"; modulePath = "${cfg.package}/lib/security/pam_visage.so"; }; # Screen lockers (swaylock, hyprlock, etc.) use their own PAM service.