Feat: Implement PAM authentication support #202
Conversation
- Updated user database schema to include PAM user status. - Enhanced user creation and retrieval methods to handle PAM users. - Added PAM authentication endpoints in the auth route. - Integrated PAM authentication logic in the frontend login form. - Introduced PAMAuthService for handling PAM-related operations. - Updated AuthContext to manage PAM login and availability status.
WalkthroughAdds Linux PAM authentication end-to-end: DB schema and user model include Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant L as LoginForm (UI)
participant C as AuthContext
participant API as api.auth.pamLogin
participant R as /api/auth/pam-login
participant PAM as PAMAuthService
participant DB as userDb
participant T as TokenGen
U->>L: Enter username/password, select PAM
L->>C: submit()
C->>API: pamLogin(username, password)
API->>R: POST /pam-login {username, password}
R->>PAM: isAvailable()
alt PAM unavailable
R-->>API: 501 { pamAvailable: false }
API-->>C: error (PAM unavailable)
else PAM available
R->>PAM: authenticate(username, password)
alt success
R->>DB: getUserByUsername -> createUser/updateUserPamStatus, update last_login
R->>T: generate token
R-->>API: 200 { token, user{..., is_pam_user} }
API-->>C: token, user
C-->>L: login success
else failure
R-->>API: 401/403
API-->>C: auth failure
C-->>L: show error
end
end
sequenceDiagram
participant App as App
participant C as AuthContext
participant API as api.auth.pamStatus / status
participant R as /api/auth/{status,pam-status}
participant PAM as PAMAuthService
App->>C: on load -> check status
C->>API: GET /api/auth/status
API->>R: /status
R->>PAM: isAvailable()
R-->>API: { pamAvailable: bool, ... }
API-->>C: store pamAvailable
C-->>App: enable PAM toggle if true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/auth.js (1)
95-104: Handle PAM users in the local/loginpath.When a PAM-backed account exists, its
password_hashisNULL. Hitting the legacy/loginendpoint with that username now causesbcrypt.compare(password, user.password_hash)to throw because the hash isnull, which drops the request into the catch block and returns a 500. That is exactly what happens if a PAM user forgets to flip the new UI toggle (the default is still “Local”). Please guard this case—either short-circuit with a 403 instructing the client to use PAM, or populate a password hash for PAM users—so we don’t regress into server errors on a valid but misrouted login attempt.
🧹 Nitpick comments (1)
server/database/db.js (1)
42-47: Align returned property casing with existing conventions.
createUsernow returns an object containingis_pam_user, but elsewhere in the codebase (e.g., API responses and AuthContext) we expose camelCase (isPamUser). Mixing casing makes it easy to forget to normalize the value and can trigger subtle bugs when the rawcreateUserpayload is reused. Please standardize on camelCase at the point of object construction (e.g., return{ id: ..., username, isPamUser: isPamUser }) and adjust downstream consumers accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/database/db.js(2 hunks)server/database/init.sql(1 hunks)server/routes/auth.js(2 hunks)server/services/pamAuth.js(1 hunks)src/components/LoginForm.jsx(3 hunks)src/contexts/AuthContext.jsx(5 hunks)src/utils/api.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
server/services/pamAuth.js (1)
server/database/db.js (1)
result(45-45)
src/contexts/AuthContext.jsx (2)
server/routes/auth.js (6)
pamAvailable(13-13)pamAvailable(142-142)pamAvailable(194-194)token(57-57)token(107-107)token(169-169)src/utils/api.js (3)
api(23-147)api(23-147)token(3-3)
src/utils/api.js (2)
src/components/LoginForm.jsx (2)
username(6-6)password(7-7)src/components/SetupForm.jsx (2)
username(6-6)password(7-7)
src/components/LoginForm.jsx (2)
src/contexts/AuthContext.jsx (6)
error(31-31)useAuth(17-23)useAuth(17-23)pamAvailable(30-30)pamLogin(134-157)login(85-107)server/routes/auth.js (3)
pamAvailable(13-13)pamAvailable(142-142)pamAvailable(194-194)
server/routes/auth.js (3)
server/database/db.js (1)
userDb(30-100)src/contexts/AuthContext.jsx (3)
pamAvailable(30-30)user(26-26)token(27-27)server/middleware/auth.js (3)
user(34-34)token(24-24)generateToken(48-57)
| async authenticateWithSu(username, password) { | ||
| return new Promise((resolve) => { | ||
| const child = spawn('su', [username, '-c', 'exit'], { | ||
| stdio: ['pipe', 'pipe', 'pipe'] | ||
| }); | ||
|
|
||
| let output = ''; | ||
| let error = ''; | ||
|
|
||
| child.stdout.on('data', (data) => { | ||
| output += data.toString(); | ||
| }); | ||
|
|
||
| child.stderr.on('data', (data) => { | ||
| error += data.toString(); | ||
| }); | ||
|
|
||
| child.on('close', (code) => { | ||
| resolve(code === 0); | ||
| }); | ||
|
|
||
| child.on('error', () => { | ||
| resolve(false); | ||
| }); | ||
|
|
||
| // Send password to stdin | ||
| child.stdin.write(password + '\n'); | ||
| child.stdin.end(); | ||
|
|
||
| // Timeout after 5 seconds | ||
| setTimeout(() => { | ||
| if (!child.killed) { | ||
| child.kill(); | ||
| resolve(false); | ||
| } | ||
| }, 5000); | ||
| }); |
There was a problem hiding this comment.
su path can authenticate everyone when the service runs as root
If this Node service ends up running as UID 0 (very common in containers or when started with sudo), su ... -c exit will always succeed regardless of the supplied password because PAM’s pam_rootok module lets root skip password checks entirely. That means a wrong password is accepted as soon as this branch is hit. We need to avoid using su (or wrap it in a helper that enforces PAM verification) when the caller is privileged, and instead call a real PAM binding that validates the target user’s credentials.(man7.org)
server/services/pamAuth.js
Outdated
| async authenticateWithLogin(username, password) { | ||
| return new Promise((resolve) => { | ||
| // This is a fallback method that checks if the user exists | ||
| // Note: This doesn't actually verify the password, just checks user existence | ||
| const child = spawn('id', [username], { | ||
| stdio: ['pipe', 'pipe', 'pipe'] | ||
| }); | ||
|
|
||
| child.on('close', (code) => { | ||
| // If user exists, we'll assume authentication for now | ||
| // In a real implementation, you'd want to use a proper PAM module | ||
| resolve(code === 0); | ||
| }); | ||
|
|
||
| child.on('error', () => { | ||
| resolve(false); | ||
| }); | ||
|
|
||
| // Timeout after 3 seconds | ||
| setTimeout(() => { | ||
| if (!child.killed) { | ||
| child.kill(); | ||
| resolve(false); | ||
| } | ||
| }, 3000); | ||
| }); |
There was a problem hiding this comment.
Fallback treats mere account existence as a successful login
The authenticateWithLogin branch just runs id username and returns true if the account exists. Any password—even empty—will therefore authenticate as long as the user is present on the system. This bypasses the entire password check and turns PAM auth into unconditional access. Please remove this fallback and replace it with an actual PAM check (e.g., via a proper PAM library or a helper binary that invokes pam_authenticate).
- Streamlined the authenticate method to use only the 'su' command for PAM authentication. - Removed unused authentication methods (sudo and login) to reduce complexity. - Updated isAvailable method to check only for the 'su' command availability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
server/services/pamAuth.js (5)
58-64: Timeout/kill robustnessKilling only once after 5s may leave
sulingering. Send SIGTERM, then escalate to SIGKILL after a short grace period, and ensure we don’t resolve twice.- // Timeout after 5 seconds - setTimeout(() => { - if (!child.killed) { - child.kill(); - resolve(false); - } - }, 5000); + // Timeout after 5 seconds with two-phase kill + const timer = setTimeout(() => { + if (!child.killed) { + child.kill('SIGTERM'); + setTimeout(() => { + if (!child.killed) child.kill('SIGKILL'); + }, 250); + resolve(false); + } + }, 5000); + child.on('close', () => clearTimeout(timer));
73-85:isAvailable()checks the wrong capabilityPresence of
sudoesn’t prove PAM auth viability for the intended service. If you keepserviceName, probe for/etc/pam.d/${serviceName}(or attempt a no-op PAM call) instead.Possible approach:
- Check
fs.access('/etc/pam.d/' + this.serviceName)and return true only if readable.- Or prefer a real PAM library and use a lightweight probe call during startup.
90-121: HardengetUserInfo: validate fields and normalize typesGuard for malformed
getentoutput and return numeric uid/gid. Current code may return strings and can mis-index on unexpected formats.- child.on('close', (code) => { - if (code === 0 && output) { - const parts = output.trim().split(':'); - resolve({ - username: parts[0], - uid: parts[2], - gid: parts[3], - name: parts[4], - home: parts[5], - shell: parts[6] - }); - } else { - resolve(null); - } - }); + child.on('close', (code) => { + if (code !== 0 || !output) return resolve(null); + const parts = output.trim().split(':'); + if (parts.length < 7) return resolve(null); + const uid = Number.parseInt(parts[2], 10); + const gid = Number.parseInt(parts[3], 10); + resolve({ + username: parts[0], + uid: Number.isNaN(uid) ? undefined : uid, + gid: Number.isNaN(gid) ? undefined : gid, + name: parts[4] || '', + home: parts[5] || '', + shell: parts[6] || '' + }); + });
5-7:serviceNameis unusedYou set and expose
serviceNamebut never use it in auth. Either wire it into the real PAM call or remove the property/mutator.Also applies to: 127-129
1-3: Remove unused import
promisifyisn’t used.-import { promisify } from 'util';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/services/pamAuth.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/services/pamAuth.js (1)
server/database/db.js (1)
result(45-45)
🔇 Additional comments (1)
server/services/pamAuth.js (1)
31-66: Blocker:su-based “PAM” auth is insecure (root bypass) and nonfunctional without a TTY
- If this process runs as UID 0,
su username -c exitwill succeed regardless of password (PAM’s pam_rootok), so wrong passwords authenticate.- Even when not root,
suexpects a controlling TTY; writing the password to stdin (Lines 55-56) won’t satisfy the PAM conversation, causing flaky failures/timeouts.Replace this approach with a real PAM call (library or minimal setuid helper that invokes
pam_authenticate). As an immediate mitigation, hard‑fail when running as root and stop attemptingsuauth.Minimal mitigation (guard root + remove stdin write):
async authenticateWithSu(username, password) { return new Promise((resolve) => { - const child = spawn('su', [username, '-c', 'exit'], { + // Hard-fail when privileged to avoid pam_rootok bypass + if (typeof process.getuid === 'function' && process.getuid() === 0) { + console.warn('[PAM] Refusing su-based auth while running as root'); + return resolve(false); + } + + const child = spawn('su', [username, '-c', 'exit'], { stdio: ['pipe', 'pipe', 'pipe'] }); @@ - // Send password to stdin - child.stdin.write(password + '\n'); - child.stdin.end(); + // su requires a TTY; stdin piping is ineffective and removed intentionally.Preferred solution (switch to a proper PAM binding and use serviceName):
-import { spawn } from 'child_process'; -import { promisify } from 'util'; +import { spawn } from 'child_process'; +// Example: `authenticate-pam` (or similar) native binding +import pam from 'authenticate-pam'; class PAMAuthService { @@ - async authenticate(username, password) { + async authenticate(username, password) { try { - // Only use su command for PAM authentication - const result = await this.authenticateWithSu(username, password); - return result; + return await new Promise((resolve) => { + pam.authenticate(username, password, (err) => { + resolve(!err); + }, { serviceName: this.serviceName }); + }); } catch (error) { console.error('PAM authentication error:', error); return false; } }Verification (ensure we’re not running as root in containers/PM2 configs):
#!/bin/bash # Find container/runtime configs that set USER root (or omit non-root user) fd -t f -a -E node_modules 'Dockerfile*' | xargs -I{} sh -c 'echo "== {} =="; rg -n -H "^\s*USER\s+" "{}" || true' # Check for any explicit dropping of privileges in Node code rg -nH "process\.(setuid|setgid|getuid)" -g '!**/node_modules/**' # Map all PAM usage to confirm no other fallbacks exist rg -nH "pamAuth|/pam-login|authenticateWithSu" -g '!**/node_modules/**'
Summary by CodeRabbit