Harden facelock auth paths and verification#16
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens Facelock’s privileged execution paths (PAM, daemon D-Bus interface, model loading, rate limiting, and filesystem permissions) and aligns packaging/test assets and documentation with the hardened behavior.
Changes:
- Make config resolution privilege-aware (process-local
--configoverride; ignoreFACELOCK_CONFIGfor privileged/root/PAM flows). - Enforce model integrity verification for both bundled and custom models via manifest/configured SHA256.
- Replace in-memory rate limiting with SQLite-backed persistence shared across daemon + oneshot paths, and tighten runtime file/dir permissions (systemd
UMask=0027, explicit modes for audit/snapshots/DB sidecars).
Reviewed changes
Copilot reviewed 33 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/run-oneshot-tests.sh | Makes oneshot E2E tests more deterministic (timeouts, output assertions). |
| test/run-integration-tests.sh | Runs a real system bus in the container and adds stronger assertions/timeouts. |
| test/container-config.toml | Adjusts container defaults (dark threshold, recognition timeout). |
| test/Containerfile | Adds runtime deps for hardened model/runtime paths in tests. |
| systemd/facelock-daemon.service | Adds UMask=0027 defense-in-depth. |
| justfile | Aligns installer behavior with hardened permissions and legacy file refresh. |
| docs/security.md | Updates security guidance to match shipped behavior (permissions, D-Bus auth, persistent rate limiting). |
| docs/contracts.md | Updates runtime contracts for privileged config handling, files, and IPC authorization. |
| docs/configuration.md | Documents privilege-aware config resolution and new config keys. |
| docs/cli.md | Documents root requirement/behavior for facelock restart and config env semantics. |
| dist/nix/module.nix | Aligns Nix module with D-Bus activation + systemd hardening settings. |
| dist/facelock.install | Tightens post-install/upgrade directory permissions. |
| dist/debian/postinst | Tightens postinst directory permissions. |
| crates/pam-facelock/src/lib.rs | Removes env-based config for PAM; validates auth_bin path safety. |
| crates/facelock-store/src/db.rs | Secures DB + sidecar permissions on open; adds rate-limit persistence test. |
| crates/facelock-face/src/provider.rs | Adds an explicit ORT “ensure loaded” entry point to avoid deadlocks. |
| crates/facelock-face/src/models.rs | Adds manifest lookup + resolves required SHA256 for bundled/custom models. |
| crates/facelock-face/src/lib.rs | Verifies only the configured detector/embedder models using resolved SHA256. |
| crates/facelock-face/src/embedder.rs | Ensures ORT is loaded before session builder creation. |
| crates/facelock-face/src/detector.rs | Ensures ORT is loaded before session builder creation. |
| crates/facelock-daemon/tests/daemon_integration.rs | Adds an integration test for rate-limit persistence across handler restarts. |
| crates/facelock-daemon/src/rate_limit.rs | Switches rate limiting to SQLite-backed persistence. |
| crates/facelock-daemon/src/handler.rs | Records auth failures into persistent rate limiter. |
| crates/facelock-daemon/src/auth.rs | Uses persistent rate limiter in pre-check; writes snapshots with explicit modes. |
| crates/facelock-daemon/src/audit.rs | Writes audit logs with explicit restrictive modes and secured parent dirs. |
| crates/facelock-core/src/paths.rs | Implements process override + privilege-aware env handling for config path resolution. |
| crates/facelock-core/src/lib.rs | Exposes new fs_security module. |
| crates/facelock-core/src/fs_security.rs | Adds helpers to create/secure dirs and files with explicit modes. |
| crates/facelock-core/src/config.rs | Adds SHA256 config fields + validation for custom model verification. |
| crates/facelock-cli/src/main.rs | Adds global --config and wires it into process-local override. |
| crates/facelock-cli/src/direct.rs | Refactors camera selection to share auto-detection/quirk logic across direct paths. |
| crates/facelock-cli/src/commands/setup.rs | Secures setup-created paths, writes files with explicit modes, and writes model SHA256 into config. |
| crates/facelock-cli/src/commands/daemon.rs | Adds method-level D-Bus authorization and preview camera ownership enforcement. |
| crates/facelock-cli/src/commands/config.rs | Requires root for facelock restart. |
| crates/facelock-cli/src/commands/auth.rs | Uses persistent rate limiting in oneshot auth and records only failed attempts. |
| config/facelock.toml | Updates reference config to match hardened privilege-aware config and model hash requirements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn secure_database_files(db_path: &Path) -> Result<()> { | ||
| for path in [ | ||
| db_path.to_path_buf(), | ||
| db_path.with_extension("db-wal"), | ||
| db_path.with_extension("db-shm"), | ||
| ] { |
There was a problem hiding this comment.
secure_database_files derives SQLite WAL/SHM sidecar paths via with_extension("db-wal") / with_extension("db-shm"), which is not how SQLite names these files (it appends -wal / -shm to the full database filename). This means overridden db_path values (e.g. facelock.sqlite or paths without a .db extension) will leave the real *-wal/*-shm files unsecured. Please compute sidecar paths by appending -wal / -shm to the full db_path string (or equivalent) instead of replacing the extension.
| fn is_privileged_process() -> bool { | ||
| let Ok(status) = std::fs::read_to_string("/proc/self/status") else { | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
is_privileged_process() returns false when /proc/self/status can't be read. That fails open (treats the process as unprivileged) and would allow FACELOCK_CONFIG to override the config path even in privileged contexts if procfs is unavailable/restricted. For hardened auth paths, this should fail safe (assume privileged when the check can’t be performed), or use a more reliable EUID query on Unix.
| if let Some(ref sha256) = self.recognition.detector_sha256 | ||
| && !is_sha256_hex(sha256) | ||
| { | ||
| return Err(ConfigError::Validation(format!( | ||
| "recognition.detector_sha256 must be a 64-character lowercase hex SHA256, got {}", | ||
| sha256 | ||
| ))); | ||
| } | ||
| if let Some(ref sha256) = self.recognition.embedder_sha256 | ||
| && !is_sha256_hex(sha256) | ||
| { | ||
| return Err(ConfigError::Validation(format!( | ||
| "recognition.embedder_sha256 must be a 64-character lowercase hex SHA256, got {}", | ||
| sha256 | ||
| ))); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| fn is_sha256_hex(value: &str) -> bool { | ||
| value.len() == 64 && value.chars().all(|c| c.is_ascii_hexdigit()) | ||
| } |
There was a problem hiding this comment.
The validation error messages say the SHA256 must be "lowercase hex", but is_sha256_hex() currently accepts uppercase A–F via is_ascii_hexdigit(). Either enforce lowercase explicitly or adjust the message so it matches the actual accepted format.
| run_test_contains() { | ||
| local name="$1" | ||
| local cmd="$2" | ||
| local pattern="$3" | ||
|
|
||
| echo -n "TEST: $name ... " | ||
| if eval "$cmd" > /tmp/test-output 2>&1; then | ||
| result=0 | ||
| else | ||
| result=$? | ||
| fi |
There was a problem hiding this comment.
run_test_contains runs under set -o pipefail but (unlike run_test) doesn’t temporarily disable pipefail while capturing the exit status. If a future test command includes a pipeline, this helper can start failing unexpectedly. Consider wrapping the eval with the same set +o pipefail / set -o pipefail pattern used in run_test for consistency.
Summary
Verification