Allow non-root processes to use the PAM service by adombeck · Pull Request #1649 · canonical/authd

adombeck · 2026-06-30T19:22:21Z

The permission check was introduced in #311 to:

"prevent spamming the service with invalid authentication request", and
"prevent some NSS (shadow) requests"

Regarding 1., our assessment is that it's not feasible to protect against DoS from a local user. For example, all D-Bus services accessible to unprivileged users on the system bus are also prone to DoS.

Regarding 2., the shadow requests (GetShadowEntries and GetShadowByName) were dropped in 62ffae0 without replacement.

There is the concern that this allows non-root processes to repeatedly call the gRPC API to brute-force the local password. The local password is hashed with argon2id (time=1, memory=64MB, threads=4), which makes each authentication attempt with the local password somewhat expensive. However, on modern hardware, it can still be done in a few dozen milliseconds. So to avoid brute forcing, this PR also adds a fail delay that delays each failed authentication attempt after the first three by 2 seconds.

Closes #856
UDENG-6981

Copilot

Pull request overview

This PR removes the previously root-only “global” permission gating for PAM gRPC calls so that non-root processes (e.g., KDE/Sway lock screens) can interact with the PAM service, and adds a failed-authentication delay to mitigate brute-force attempts.

Changes:

Remove the gRPC “global permissions” interceptor and related CheckGlobalAccess hooks for PAM/NSS.
Add per-username failed-auth tracking and impose a delay after repeated authentication failures.
Update tests and broker mocks to reflect the new authorization behavior and exercise the fail-delay logic.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/testutils/broker.go	Extend broker mock to simulate a denied authentication outcome used by new tests.
internal/services/user/user_test.go	Remove test-only interceptor that enforced the deleted global-access check.
internal/services/user/permissions.go	Remove no-op `CheckGlobalAccess` implementation (global access layer removed).
internal/services/permissions.go	Remove global unary interceptor that previously enforced PAM/NSS global access checks.
internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_not_root/IsAuthenticated	Remove golden output tied to the removed “must be root” behavior.
internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_not_root/cache.db	Remove golden DB snapshot tied to the removed “must be root” behavior.
internal/services/pam/permissions.go	Remove PAM `CheckGlobalAccess` root-only check (enables non-root PAM calls).
internal/services/pam/pam.go	Implement failed-auth tracking and delayed responses after repeated failures.
internal/services/pam/pam_test.go	Remove non-root error cases; add a new test covering the fail-delay threshold behavior.
internal/services/pam/export_test.go	Export fail-delay constants to external test package.
internal/services/manager.go	Stop wiring the removed global-permissions interceptor into the gRPC server.
internal/brokers/manager.go	Track session→username mapping to support per-user failure accounting.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.

codecov · 2026-07-01T16:04:59Z

Codecov Report

❌ Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.91%. Comparing base (0bdad0e) to head (bbc9164).

Files with missing lines	Patch %	Lines
internal/services/pam/pam.go	84.37%	5 Missing ⚠️
cmd/authd/daemon/daemon.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1649      +/-   ##
==========================================
+ Coverage   84.96%   87.91%   +2.94%     
==========================================
  Files          25       96      +71     
  Lines        1942     6965    +5023     
  Branches        0      111     +111     
==========================================
+ Hits         1650     6123    +4473     
- Misses        292      786     +494     
- Partials        0       56      +56

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

denisonbarbosa

A few things that can be addressed in a final rebase. Everything else looks great!

adombeck · 2026-07-06T12:21:03Z

Reminder: Once merged, ask affected users to try the edge version

The permission check was introduced in #311 to: 1. "prevent spamming the service with invalid authentication request", and 2. "prevent some NSS (shadow) requests" Regarding 1., our assessment is that it's not feasible to protect against DoS from a local user. For example, all D-Bus services accessible to unprivileged users on the system bus are also prone to DoS. Regarding 2., the shadow requests (GetShadowEntries and GetShadowByName) were dropped in 62ffae0 without replacement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Expose UsernameFromSessionID on the broker Manager so callers can look up the username associated with an active session without going through broker internals. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Without the root-only restriction on the PAM gRPC socket, any local user can now call IsAuthenticated in a tight loop against another user's cached (offline) OIDC credentials. The OIDC broker hashes those credentials with argon2id, but the parameters yield ~26ms per attempt on modern hardware — far faster than pam_unix's enforced 2s fail delay. Track consecutive authentication failures per username in an in-memory counter. The first authFailDelayThreshold (3) denials are returned immediately, matching the leniency of pam_faillock's deny=3 default and giving users a couple of free retries for typos. From the fourth failure onward, the response is held for authFailDelay (2s) before returning, matching the delay pam_unix imposes via pam_fail_delay(). A successful authentication resets the counter for that user. The counter is keyed by username (not session ID) so it cannot be reset by starting a new session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The three security-policy knobs that govern the PAM brute-force mitigation were hardcoded constants. Operators may need to tune them to satisfy site-specific compliance requirements, so expose them in the authd.yaml config: auth_fail_delay_threshold – failures before a delay is imposed (default 3) auth_fail_delay – length of that delay (default 2s) auth_fail_reset_window – inactivity window before reset (default 15m) authFailMaxTracked is intentionally kept hardcoded: it is a memory- safety bound rather than a security-policy value, and exposing it creates a DoS surface with no meaningful benefit. The values are grouped in pam.Config / pam.DefaultConfig following the same pattern used by users.Config for UID/GID ranges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Previously recordFailure returned 0 when the tracker was full, meaning the delay was never applied. An attacker could exploit this by flooding the tracker with bogus usernames (a fill attack) to disable brute-force protection for the real target. Return math.MaxInt instead so the delay is always applied regardless of whether the username could be recorded. This means the fill attack degrades to 'everyone gets delayed' rather than 'no one does'. Add TestIsAuthenticated_FailDelayTrackerFull to verify the fail-secure behaviour: fill the tracker to capacity with one username, then confirm the delay is still applied for a new username that cannot be tracked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Non-root PAM consumers can no longer make the follow-up SetBroker RPC after the root-only peer check, which left successful logins unable to remember their selected broker. authd already knows the authenticated user and session broker inside IsAuthenticated, so persisting the default broker there avoids the extra privileged round-trip. Drop the unused SetBroker RPC and simplify the PAM client and test flows so AcctMgmt now consistently returns PAM_IGNORE while broker persistence happens server-side. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The delay was only applied when the broker returned Denied or DeniedMaxTries. Because DeniedMaxTries is only issued after N failed attempts within the same session, an attacker who opens a fresh session for every guess always receives Retry and the delay never fires. Count Retry as a failure so that the per-username counter (shared across sessions) triggers the delay regardless of which response the broker returns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Setting the reset window to 0 was silently broken: time.Since() always returns a non-negative duration, so the stale-entry check was always true, resetting the failure counter on every attempt and defeating brute-force protection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

adombeck force-pushed the 856-allow-non-root branch from ce3a6d4 to 2f665a1 Compare June 30, 2026 20:18

adombeck requested a review from Copilot June 30, 2026 20:18

Copilot started reviewing on behalf of adombeck June 30, 2026 20:19 View session

adombeck changed the title ~~WIP: Allow non-root processes to use the PAM service~~ Allow non-root processes to use the PAM service Jun 30, 2026

Copilot AI reviewed Jun 30, 2026

View reviewed changes

Comment thread internal/services/pam/pam.go

Comment thread internal/services/pam/pam.go Outdated

Comment thread internal/services/pam/pam.go Outdated

Comment thread internal/services/pam/pam.go

Comment thread internal/services/manager.go

Comment thread internal/brokers/manager.go

Copilot started work on behalf of adombeck June 30, 2026 20:36 View session

Copilot finished work on behalf of adombeck June 30, 2026 20:45

3v1n0 reviewed Jul 1, 2026

View reviewed changes

Comment thread internal/services/pam/pam.go Outdated

adombeck force-pushed the 856-allow-non-root branch 2 times, most recently from d3d804f to cd1316f Compare July 1, 2026 13:22

adombeck requested a review from Copilot July 1, 2026 14:30

Copilot started reviewing on behalf of adombeck July 1, 2026 14:30 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

adombeck force-pushed the 856-allow-non-root branch 2 times, most recently from 96c133e to 8f242f1 Compare July 1, 2026 15:55

adombeck marked this pull request as ready for review July 1, 2026 16:04

adombeck force-pushed the 856-allow-non-root branch 4 times, most recently from 992b0c6 to 688b8cd Compare July 2, 2026 15:50

adombeck requested review from denisonbarbosa and nooreldeenmansour July 3, 2026 14:37

denisonbarbosa approved these changes Jul 3, 2026

View reviewed changes

Comment thread internal/services/pam/pam_test.go Outdated

Comment thread internal/services/pam/pam_test.go

nooreldeenmansour reviewed Jul 3, 2026

View reviewed changes

Comment thread internal/services/pam/pam.go Outdated

nooreldeenmansour reviewed Jul 3, 2026

View reviewed changes

Comment thread debian/authd-config/authd.yaml

adombeck force-pushed the 856-allow-non-root branch 2 times, most recently from 603d834 to bef5e1a Compare July 3, 2026 18:13

nooreldeenmansour approved these changes Jul 6, 2026

View reviewed changes

Comment thread internal/proto/authd/authd.pb.go

Comment thread pam/pam.go

3v1n0 reviewed Jul 6, 2026

View reviewed changes

Comment thread internal/services/pam/pam.go Outdated

adombeck force-pushed the 856-allow-non-root branch from bef5e1a to d06713e Compare July 6, 2026 11:22

3v1n0 reviewed Jul 6, 2026

View reviewed changes

Comment thread debian/authd-config/authd.yaml

adombeck and others added 8 commits July 6, 2026 19:12

brokers: track username per session in manager

5ba8613

Expose UsernameFromSessionID on the broker Manager so callers can look up the username associated with an active session without going through broker internals. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

adombeck force-pushed the 856-allow-non-root branch from 93e480b to bbc9164 Compare July 6, 2026 17:13

adombeck merged commit 4bd2fba into main Jul 7, 2026
25 of 26 checks passed

adombeck deleted the 856-allow-non-root branch July 7, 2026 21:06

adombeck mentioned this pull request Jul 8, 2026

Login fails with read-only filesystem: UpdateBrokerForUser cannot open database #1693

Open

Uh oh!

Conversation

adombeck commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

denisonbarbosa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adombeck commented Jul 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adombeck commented Jun 30, 2026 •

edited

Loading

codecov Bot commented Jul 1, 2026 •

edited

Loading