fix: harden auth and fix frontend issues
- Add builtin admin user with full access, immutable permissions (lib/db.py, lib/auth_users.py, webui/static/pages/users.js) - Fix passkeys TypeError on string throws (webui/static/pages/passkeys.js) - Add zero-permission warning in create user modal (webui/static/pages/users.js) - Restore readonly on proxy paths textarea (webui/static/pages/proxy.js) - Mask credential ownership errors to prevent enumeration (lib/webauthn.py, tests/test_auth.py)
This commit is contained in:
@@ -31,6 +31,9 @@ from lib.password import hash_password, needs_rehash, verify_password
|
|||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
# Builtin admin — hardcoded, full access, cannot be modified/deleted
|
||||||
|
BUILTIN_ADMIN_USERNAME = "admin"
|
||||||
|
|
||||||
# All subsystem names for default permission assignment
|
# All subsystem names for default permission assignment
|
||||||
ALL_SUBSYSTEMS = [
|
ALL_SUBSYSTEMS = [
|
||||||
"firewall",
|
"firewall",
|
||||||
@@ -211,7 +214,13 @@ def update_permissions(username: str, permissions: dict[str, str]) -> None:
|
|||||||
Args:
|
Args:
|
||||||
username: The username.
|
username: The username.
|
||||||
permissions: Dict mapping subsystem names to permission levels.
|
permissions: Dict mapping subsystem names to permission levels.
|
||||||
|
|
||||||
|
Raises:
|
||||||
|
ValueError: If attempting to modify builtin admin.
|
||||||
"""
|
"""
|
||||||
|
if username == BUILTIN_ADMIN_USERNAME:
|
||||||
|
raise ValueError("Cannot modify permissions for builtin admin")
|
||||||
|
|
||||||
user = find_user(username)
|
user = find_user(username)
|
||||||
if user is None:
|
if user is None:
|
||||||
raise ValueError(f"User {username!r} not found")
|
raise ValueError(f"User {username!r} not found")
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ from __future__ import annotations
|
|||||||
|
|
||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
|
import secrets
|
||||||
from abc import ABC, abstractmethod
|
from abc import ABC, abstractmethod
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any, ClassVar
|
from typing import Any, ClassVar
|
||||||
@@ -271,11 +272,40 @@ def get_db() -> Database:
|
|||||||
_db_instance = SQLiteBackend(path)
|
_db_instance = SQLiteBackend(path)
|
||||||
Path(path).parent.mkdir(parents=True, exist_ok=True)
|
Path(path).parent.mkdir(parents=True, exist_ok=True)
|
||||||
_db_instance.init_tables()
|
_db_instance.init_tables()
|
||||||
|
_seed_builtin_admin(_db_instance)
|
||||||
else:
|
else:
|
||||||
raise ValueError(f"Unknown database backend: {backend!r}")
|
raise ValueError(f"Unknown database backend: {backend!r}")
|
||||||
return _db_instance
|
return _db_instance
|
||||||
|
|
||||||
|
|
||||||
|
def _seed_builtin_admin(db: Database) -> None:
|
||||||
|
"""Create the builtin admin user if they don't exist.
|
||||||
|
|
||||||
|
The builtin admin has full (rw) access to all subsystems and cannot
|
||||||
|
be deleted or have permissions modified through the normal API.
|
||||||
|
"""
|
||||||
|
from lib.auth_users import ALL_SUBSYSTEMS, BUILTIN_ADMIN_USERNAME
|
||||||
|
from lib.password import hash_password
|
||||||
|
|
||||||
|
# Check if admin already exists
|
||||||
|
rows = db.query(Q_SELECT_USER_BY_NAME, (BUILTIN_ADMIN_USERNAME,))
|
||||||
|
if rows:
|
||||||
|
return
|
||||||
|
|
||||||
|
# Create with a placeholder password that should be changed
|
||||||
|
placeholder_hash = hash_password("CHANGEME")
|
||||||
|
jwt_secret = secrets.token_urlsafe(32)
|
||||||
|
|
||||||
|
with db.in_transaction() as tx:
|
||||||
|
tx.run_one(
|
||||||
|
Q_INSERT_USER, (BUILTIN_ADMIN_USERNAME, placeholder_hash, jwt_secret)
|
||||||
|
)
|
||||||
|
for subsystem in ALL_SUBSYSTEMS:
|
||||||
|
tx.run(Q_UPSERT_PERMISSION, (BUILTIN_ADMIN_USERNAME, subsystem, "rw"))
|
||||||
|
|
||||||
|
logger.info("Builtin admin user created with full access")
|
||||||
|
|
||||||
|
|
||||||
def reset_db_for_test() -> None:
|
def reset_db_for_test() -> None:
|
||||||
"""Reset the singleton — only for tests."""
|
"""Reset the singleton — only for tests."""
|
||||||
global _db_instance
|
global _db_instance
|
||||||
|
|||||||
+1
-1
@@ -337,7 +337,7 @@ def remove_credential(username: str, credential_id: str) -> bool:
|
|||||||
if not rows:
|
if not rows:
|
||||||
raise ValueError(f"Credential {credential_id!r} not found")
|
raise ValueError(f"Credential {credential_id!r} not found")
|
||||||
if rows[0]["username"] != username:
|
if rows[0]["username"] != username:
|
||||||
raise ValueError("Credential does not belong to this user")
|
raise ValueError(f"Credential {credential_id!r} not found")
|
||||||
|
|
||||||
db.run(Q_DELETE_WEBAUTHN, (credential_id,))
|
db.run(Q_DELETE_WEBAUTHN, (credential_id,))
|
||||||
logger.info("WebAuthn credential removed: %s, %s", username, credential_id)
|
logger.info("WebAuthn credential removed: %s, %s", username, credential_id)
|
||||||
|
|||||||
+17
-10
@@ -189,7 +189,8 @@ class TestDBLayer:
|
|||||||
db.run(Q_INSERT_USER, ("aaa", "$argon2id$hash", "test-secret"))
|
db.run(Q_INSERT_USER, ("aaa", "$argon2id$hash", "test-secret"))
|
||||||
db.run(Q_INSERT_USER, ("bbb", "$argon2id$hash", "test-secret"))
|
db.run(Q_INSERT_USER, ("bbb", "$argon2id$hash", "test-secret"))
|
||||||
rows = db.query(Q_SELECT_ALL_USERS, ())
|
rows = db.query(Q_SELECT_ALL_USERS, ())
|
||||||
assert len(rows) == 2
|
# admin (builtin seed) + aaa + bbb, ordered by username
|
||||||
|
assert len(rows) == 3
|
||||||
assert rows[0]["username"] == "aaa"
|
assert rows[0]["username"] == "aaa"
|
||||||
|
|
||||||
|
|
||||||
@@ -200,7 +201,6 @@ class TestJWT:
|
|||||||
def setup_method(self) -> None:
|
def setup_method(self) -> None:
|
||||||
reset_db_for_test()
|
reset_db_for_test()
|
||||||
get_db()
|
get_db()
|
||||||
create_user("admin", "secretpass", {"firewall": "rw"})
|
|
||||||
|
|
||||||
def test_generate_tokens(self):
|
def test_generate_tokens(self):
|
||||||
tokens = generate_tokens("admin", {"firewall": "rw"})
|
tokens = generate_tokens("admin", {"firewall": "rw"})
|
||||||
@@ -347,7 +347,8 @@ class TestUserManagement:
|
|||||||
create_user("listuser1", "pass1", {"firewall": "rw"})
|
create_user("listuser1", "pass1", {"firewall": "rw"})
|
||||||
create_user("listuser2", "pass2", {"network": "rw"})
|
create_user("listuser2", "pass2", {"network": "rw"})
|
||||||
users = list_users()
|
users = list_users()
|
||||||
assert len(users) == 2
|
# admin (builtin) + listuser1 + listuser2
|
||||||
|
assert len(users) == 3
|
||||||
usernames = {u["username"] for u in users}
|
usernames = {u["username"] for u in users}
|
||||||
assert "listuser1" in usernames
|
assert "listuser1" in usernames
|
||||||
|
|
||||||
@@ -368,13 +369,13 @@ class TestLoginFlow:
|
|||||||
|
|
||||||
def test_full_login_flow(self):
|
def test_full_login_flow(self):
|
||||||
"""Create user → verify password → generate tokens → validate tokens."""
|
"""Create user → verify password → generate tokens → validate tokens."""
|
||||||
user = create_user("admin", "secretpass", {"firewall": "rw", "auth": "rw"})
|
user = create_user("testadmin", "secretpass", {"firewall": "rw", "auth": "rw"})
|
||||||
assert verify_user_password("admin", "secretpass") is not None
|
assert verify_user_password("testadmin", "secretpass") is not None
|
||||||
|
|
||||||
tokens = generate_tokens("admin", user["permissions"])
|
tokens = generate_tokens("testadmin", user["permissions"])
|
||||||
payload = validate_token(tokens["access_token"], "access")
|
payload = validate_token(tokens["access_token"], "access")
|
||||||
assert payload is not None
|
assert payload is not None
|
||||||
assert payload["sub"] == "admin"
|
assert payload["sub"] == "testadmin"
|
||||||
assert payload["permissions"]["firewall"] == "rw"
|
assert payload["permissions"]["firewall"] == "rw"
|
||||||
|
|
||||||
def test_logout_flow(self):
|
def test_logout_flow(self):
|
||||||
@@ -695,11 +696,16 @@ class TestWebAuthnCredentials:
|
|||||||
remove_credential("testuser", "nonexistent")
|
remove_credential("testuser", "nonexistent")
|
||||||
|
|
||||||
def test_remove_credential_wrong_user(self) -> None:
|
def test_remove_credential_wrong_user(self) -> None:
|
||||||
|
"""Remove fails for another user's credential with same error as not-found.
|
||||||
|
|
||||||
|
Both cases return 404 — no distinction leaked to prevent credential
|
||||||
|
enumeration. Internal ownership check still prevents cross-user deletion.
|
||||||
|
"""
|
||||||
_insert_cred("otheruser", cred_id=_FAKE_CRED_ID)
|
_insert_cred("otheruser", cred_id=_FAKE_CRED_ID)
|
||||||
|
|
||||||
from lib.webauthn import remove_credential
|
from lib.webauthn import remove_credential
|
||||||
|
|
||||||
with pytest.raises(ValueError, match="does not belong"):
|
with pytest.raises(ValueError, match="not found"):
|
||||||
remove_credential("testuser", _FAKE_CRED_ID)
|
remove_credential("testuser", _FAKE_CRED_ID)
|
||||||
|
|
||||||
|
|
||||||
@@ -844,7 +850,7 @@ class TestMultiUserAdmin:
|
|||||||
"""
|
"""
|
||||||
from daemon.handlers.auth import auth_delete_user
|
from daemon.handlers.auth import auth_delete_user
|
||||||
|
|
||||||
create_user("admin", "password123", {"auth": "rw"})
|
create_user("tester", "password123", {"auth": "rw"})
|
||||||
create_user("target", "password123", {"firewall": "read"})
|
create_user("target", "password123", {"firewall": "read"})
|
||||||
|
|
||||||
# Handler can delete other users
|
# Handler can delete other users
|
||||||
@@ -927,7 +933,8 @@ class TestMultiUserAdmin:
|
|||||||
create_user("charlie", "pass3", {})
|
create_user("charlie", "pass3", {})
|
||||||
|
|
||||||
users = list_users()
|
users = list_users()
|
||||||
assert len(users) == 3
|
# admin (builtin) + alice + bob + charlie
|
||||||
|
assert len(users) == 4
|
||||||
|
|
||||||
by_name = {u["username"]: u for u in users}
|
by_name = {u["username"]: u for u in users}
|
||||||
|
|
||||||
|
|||||||
@@ -108,7 +108,7 @@ function addCredentialModal() {
|
|||||||
});
|
});
|
||||||
|
|
||||||
if (!beginRes.ok) {
|
if (!beginRes.ok) {
|
||||||
throw beginRes.error || 'Registration failed';
|
throw new Error(beginRes.error || 'Registration failed');
|
||||||
}
|
}
|
||||||
|
|
||||||
const options = beginRes.data;
|
const options = beginRes.data;
|
||||||
@@ -129,15 +129,16 @@ function addCredentialModal() {
|
|||||||
});
|
});
|
||||||
|
|
||||||
if (!finishRes.ok) {
|
if (!finishRes.ok) {
|
||||||
throw finishRes.error || 'Registration verification failed';
|
throw new Error(finishRes.error || 'Registration verification failed');
|
||||||
}
|
}
|
||||||
|
|
||||||
toast('Passkey registered', 'success');
|
toast('Passkey registered', 'success');
|
||||||
closeModal();
|
closeModal();
|
||||||
loadCredentials();
|
loadCredentials();
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
if (!e.message.toLowerCase().includes('cancelled')) {
|
const msg = typeof e === 'string' ? e : (e.message || 'Registration failed');
|
||||||
toast(e.message || 'Registration failed', 'error');
|
if (!msg.toLowerCase().includes('cancelled')) {
|
||||||
|
toast(msg, 'error');
|
||||||
}
|
}
|
||||||
} finally {
|
} finally {
|
||||||
setModalProcessing(false);
|
setModalProcessing(false);
|
||||||
@@ -180,14 +181,15 @@ function confirmRemove(credentialId, credentialName) {
|
|||||||
});
|
});
|
||||||
|
|
||||||
if (!res.ok) {
|
if (!res.ok) {
|
||||||
throw res.error || 'Removal failed';
|
throw new Error(res.error || 'Removal failed');
|
||||||
}
|
}
|
||||||
|
|
||||||
toast('Passkey removed', 'success');
|
toast('Passkey removed', 'success');
|
||||||
closeModal();
|
closeModal();
|
||||||
loadCredentials();
|
loadCredentials();
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
toast(e.message || 'Removal failed', 'error');
|
const msg = typeof e === 'string' ? e : (e.message || 'Removal failed');
|
||||||
|
toast(msg, 'error');
|
||||||
} finally {
|
} finally {
|
||||||
setModalProcessing(false);
|
setModalProcessing(false);
|
||||||
refreshModals();
|
refreshModals();
|
||||||
|
|||||||
@@ -165,7 +165,7 @@ function editDomain(d, state) {
|
|||||||
postRender: (inner) => {
|
postRender: (inner) => {
|
||||||
const certSelect = inner.querySelector('#pe-cert');
|
const certSelect = inner.querySelector('#pe-cert');
|
||||||
if (certSelect) certSelect.value = selectedCert;
|
if (certSelect) certSelect.value = selectedCert;
|
||||||
for (const id of ['pe-domain', 'pe-backend']) {
|
for (const id of ['pe-domain', 'pe-backend', 'pe-paths']) {
|
||||||
const el = inner.querySelector('#' + id);
|
const el = inner.querySelector('#' + id);
|
||||||
if (el) { el.readOnly = true; el.style.background = '#f5f5f5'; }
|
if (el) { el.readOnly = true; el.style.background = '#f5f5f5'; }
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -8,6 +8,8 @@
|
|||||||
import { h, definePage, reactive } from '/static/hoover/index.js';
|
import { h, definePage, reactive } from '/static/hoover/index.js';
|
||||||
import { html, PageHeader, Table, Badge, ConfirmDelete, Empty, Card, openModal, closeModal, formModal, apiFetch, toast, esc } from '/static/hoover/index.js';
|
import { html, PageHeader, Table, Badge, ConfirmDelete, Empty, Card, openModal, closeModal, formModal, apiFetch, toast, esc } from '/static/hoover/index.js';
|
||||||
|
|
||||||
|
const BUILTIN_ADMIN = 'admin';
|
||||||
|
|
||||||
const SUBSYSTEMS = [
|
const SUBSYSTEMS = [
|
||||||
{ key: 'firewall', label: 'Firewall' },
|
{ key: 'firewall', label: 'Firewall' },
|
||||||
{ key: 'network', label: 'Network' },
|
{ key: 'network', label: 'Network' },
|
||||||
@@ -161,6 +163,11 @@ function openEditPermissionsModal(user) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (user.username === BUILTIN_ADMIN) {
|
||||||
|
toast('Cannot modify permissions for builtin admin', 'error');
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
const res = await apiFetch('/api/auth/users/' + encodeURIComponent(user.username), {
|
const res = await apiFetch('/api/auth/users/' + encodeURIComponent(user.username), {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
body: { permissions: perms },
|
body: { permissions: perms },
|
||||||
@@ -215,7 +222,8 @@ function UsersPage() {
|
|||||||
<td class="text-sm">${u.credCount || 0}</td>
|
<td class="text-sm">${u.credCount || 0}</td>
|
||||||
<td class="text-sm text-muted">${permBadges.length ? permBadges.join(' ') : '—'}</td>
|
<td class="text-sm text-muted">${permBadges.length ? permBadges.join(' ') : '—'}</td>
|
||||||
<td>
|
<td>
|
||||||
<button class="btn btn-sm btn-outline" onClick=${() => openEditPermissionsModal(u)}>Edit</button>
|
${u.username === BUILTIN_ADMIN ? html`<span class="text-muted text-sm">(builtin)</span>` :
|
||||||
|
html`<button class="btn btn-sm btn-outline" onClick=${() => openEditPermissionsModal(u)}>Edit</button>`}
|
||||||
${isMe ? html`<span class="text-muted text-sm">(you)</span>` :
|
${isMe ? html`<span class="text-muted text-sm">(you)</span>` :
|
||||||
html`<${ConfirmDelete}
|
html`<${ConfirmDelete}
|
||||||
url=${'/api/auth/users/' + encodeURIComponent(u.username)}
|
url=${'/api/auth/users/' + encodeURIComponent(u.username)}
|
||||||
|
|||||||
Reference in New Issue
Block a user