fix: use Proxmox authentication for Web UI - #346
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8410012f71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self._has_tfa({"data": data}): | ||
| return {"ok": False, "code": "TFA_REQUIRED", | ||
| "message": "Two-factor authentication is required but not supported by this login flow."} | ||
| if not isinstance(data, dict) or not isinstance(data.get("ticket"), str) or not data["ticket"]: | ||
| return {"ok": False, "code": "LOGIN_FAILED", "message": "Invalid credentials."} |
There was a problem hiding this comment.
When an administrator account has Proxmox TFA enabled, /access/ticket can return a nonempty partial TFA challenge ticket rather than one of the three fields checked by _has_tfa. This response passes the generic ticket check, is authorized locally as the user, and creates an Ultimate Updater session after only the password, bypassing the account's second factor. Detect and reject Proxmox's TFA ticket format before treating the response as a completed login.
Useful? React with 👍 / 👎.
| login = self.server.auth.login(username, password, realm, self.client_address[0]) | ||
| if not login.get("ok"): | ||
| self.send_json(error_payload(login.get("code", "LOGIN_FAILED"), login.get("message", "Login failed.")), |
There was a problem hiding this comment.
After a client accumulates five failed attempts within the 60-second window, AuthStore.login() returns None, but this new handler unconditionally calls login.get(...). Every subsequent attempt during the throttle window therefore raises AttributeError and closes the request without a JSON response instead of returning the intended authentication failure; return a structured failure from the throttle branch or handle None here.
Useful? React with 👍 / 👎.
| def authenticate(self, username, password, realm): | ||
| if not isinstance(username, str) or not USER_RE.fullmatch(username): | ||
| return {"ok": False, "code": "LOGIN_FAILED", "message": "Invalid credentials."} |
There was a problem hiding this comment.
For a valid Proxmox account whose username does not match the older local-PAM pattern—for example, a numeric-leading username such as 1admin@pve—this check rejects the credentials before consulting Proxmox. Proxmox realm usernames are not constrained to start with a letter or underscore, so legitimate administrators can be locked out; validate against the Proxmox userid rules or let the ticket endpoint perform this validation.
Useful? React with 👍 / 👎.
|
@codex Please address one final performance/safety issue before merge. Current Please update the existing
Do not merge the PR yourself. Report the new HEAD and test results here when done. |
|
@codex address that feedback Please address all three findings from the automated review on commit Required fixes:
Tests must cover all four points, including repeated authorization/session checks inside TTL not spawning repeated Keep Proxmox completely unmodified, keep #343 files untouched, run focused + full regression tests and |
Summary
Testing
|
Summary
Testing
|
1 participant
Summary
Fixes #344 by making Proxmox the authoritative Web UI authentication and authorization source.
/access/domains./to include the complete built-inAdministratorrole.UU_AUTH_BACKEND=internaland the explicit legacy PAM backend available.Proxmox users, realms, roles, ACLs, PAM, and system configuration are not modified.
Validation
git diff --check: PASS.A live positive login was not attempted because no test password was available; the authentication boundary is covered with mocked API tests and read-only schema validation.