feat: enable advanced database-backed authentication by default#5144
feat: enable advanced database-backed authentication by default#5144OliverBryant wants to merge 10 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables advanced, database-backed authentication by default in Xinference, updating the documentation, constants, and API routers accordingly. To prevent test failures, various test fixtures are updated to explicitly disable advanced authentication. The review feedback highlights two key issues: first, a potential race condition in _get_or_create_persisted_secret when multiple processes concurrently attempt to generate and write the secret key; second, an environment inconsistency in test fixtures where XINFERENCE_AUTH_ADVANCED is disabled only after the cluster subprocess has already been started.
Advanced auth (SQLite-backed user accounts, API keys, permissions) was previously opt-in via XINFERENCE_AUTH_ADVANCED. It is now on by default: an admin account is auto-created on first run with a one-time password printed to the log, and the JWT/encryption secrets are auto-generated and persisted under XINFERENCE_HOME/auth/ when not explicitly set via environment variables. Set XINFERENCE_AUTH_ADVANCED=false to restore the previous no-auth or --auth-config behavior. Updated test fixtures that start the REST API without credentials to explicitly opt out of advanced auth, since they are not exercising authentication.
OliverBryant
force-pushed
the
feat/auth-advanced-default-on
branch
from
dc68b49 to
f99c5a2
Compare
OliverBryant
changed the title
OliverBryant
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enables the advanced, database-backed authentication system by default in Xinference. It updates the documentation, modifies the parsing of the XINFERENCE_AUTH_ADVANCED environment variable to default to true, disables advanced authentication in several test suites to maintain unauthenticated test coverage, and introduces a mechanism to persist generated JWT and encryption keys. The review feedback highlights a potential race condition in the key persistence mechanism when multiple processes start concurrently, and suggests using atomic file creation with a retry loop to resolve it.
Add a full section covering the advanced auth system introduced in v3.0: how to enable/disable it, the auto-created admin account, secret and database storage locations, permissions, and user/API-key management endpoints. The existing simple OAuth2 documentation is left unchanged, with a note pointing to the new section.
Address code review feedback on the auth-advanced-by-default change: - _get_or_create_persisted_secret used a non-atomic check-then-write (os.path.exists + O_TRUNC), so concurrent first-time launches (e.g. supervisor and worker starting together) could each generate a different secret and disagree on it. Switch to O_CREAT | O_EXCL with a bounded retry/read-back loop so only one process wins the write and the rest read back the same value. - Several test fixtures set XINFERENCE_AUTH_ADVANCED=false only after already launching a cluster subprocess that inherits the process environment. Move the env var assignment before any subprocess is started so the intended value is actually in effect for all of them.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enables the advanced, database-backed authentication system by default in Xinference starting from v3.0. It updates the documentation to explain the new default behavior, configuration options, and REST endpoints, and updates various test suites to explicitly disable advanced authentication where unauthenticated requests are expected. Additionally, it introduces a mechanism to automatically generate and persist JWT signing secrets and encryption keys. The review feedback highlights a critical issue in this secret persistence logic: if a previous run crashes after creating the secret file but before writing to it, the server will fail to start on subsequent runs. A robust recovery mechanism and better handling of concurrent file access are recommended.
Address further review feedback: if a process was killed after creating the secret file (O_CREAT | O_EXCL) but before writing the secret to it, the file is left behind at 0 bytes. Previously, every subsequent startup would see the file, read an empty string, retry, and eventually raise RuntimeError -- permanently blocking startup until the file was removed by hand. Now an empty file is only waited on for a short grace period; past that, it's treated as abandoned and removed so a new secret can be generated. Reads are also wrapped to tolerate the file disappearing or being inaccessible mid-check.
Add translations for the new "Advanced Authentication System" section (and the updated note at the top of the page) to all locale catalogs: de, es, fr, it, ja, ko, pt_BR, zh_CN, zh_TW. Several existing and newly-added CJK translations used inline markup (``code``, :ref:`...`, **bold**) directly adjacent to CJK punctuation or text with no separating space, which breaks docutils' inline-markup boundary detection and silently degrades rendering (backticks show up literally, or in the :ref: case produces an "undefined label" warning). Fixed by inserting RST escaped spaces (\ ) at the affected boundaries; verified by building the HTML output for all 9 languages and confirming zero Sphinx warnings/errors for this page. Also fixed two instances of this same pre-existing bug in the Korean translation unrelated to this change (single backticks and missing escapes around API key examples).
Xinference now requires authentication out of the box, but a fresh deployment previously had no way to sign in short of reading a randomly generated password out of the server log. Add a proper first-run setup flow instead: - POST /v1/admin/setup lets an operator create the very first admin account with a password of their choosing. It is only usable while the user table is empty (checked atomically via BEGIN IMMEDIATE in Database.create_first_user, so concurrent callers across processes can't both win), and permanently refuses further calls once one account exists. - GET /v1/admin/setup/status reports whether setup is still needed, so the web UI can route to the right page automatically. - Enforce a configurable minimum password length (XINFERENCE_PASSWORD_MIN_LENGTH, default 8) on setup, user creation, and password changes. - New web UI setup page (scenes/setup) with a shared two-column layout (scenes/login/authPageLayout.js) reused by the login page, highlighting real Xinference capabilities. LoginAuth now checks setup status before deciding whether to render the setup or login screen. This removes the old behavior of auto-creating an "admin" user with a random password logged to stderr on first startup.
…default-on # Conflicts: # xinference/ui/web/ui/src/router/index.js # xinference/ui/web/ui/src/scenes/login/login.js
The old React UI's setup page was dropped when the legacy frontend was removed (xorbitsai#5145). Re-implement it on the Next.js frontend, matching the existing login page's two-column layout: - New /setup route (components/pages/setup) with the same brand/form split as the login page, highlighting real Xinference capabilities (one-command model serving, OpenAI-compatible API, distributed deployment) instead of generic copy. - AppInit now calls GET /v1/admin/setup/status when no token is present; a fresh deployment (needs_setup: true) is routed to /setup instead of /login, and visiting /setup after setup is already complete bounces back to /login. - Login page shows a one-time "account created" toast after a successful setup, handed off via a sessionStorage flag since the setup page does a full navigation rather than client-side routing state. - Added `setup` i18n strings in en/zh/ja/ko. The setup form itself posts to /v1/admin/setup with a plain fetch rather than the shared request client, so a losing race (403, someone else already completed setup) and validation errors (400) don't trigger the client's generic global-toast error handling -- this page needs to control that messaging itself.
Verifies API keys are enforced when advanced auth is on (accepted, rejected for unauthorized models, rejected when invalid) and become inert when advanced auth is off, since the check short-circuits to a no-op instead of validating the xf- key format.
| ) | ||
|
|
||
|
|
||
| async def setup_admin(request: Request) -> JSONResponse: |
There was a problem hiding this comment.
This public endpoint allows whichever network client arrives first to create the full-privilege administrator. BEGIN IMMEDIATE guarantees only one winner, but it does not establish that the winner is the deployment owner; a fresh or upgraded instance exposed on the network can therefore be taken over before the operator opens the UI. Please require a trusted bootstrap credential, restrict setup to a trusted local channel, or provide an equivalent trust mechanism. Also reject immediately when needs_setup() is false before computing the bcrypt hash; otherwise this permanently remains an unauthenticated CPU-expensive endpoint even after setup is complete.
There was a problem hiding this comment.
Documenting the exposure does not resolve the takeover risk: default-on authentication should not require the operator to win a race before the service port becomes reachable. A practical fix is a one-time setup token. Support XINFERENCE_AUTH_SETUP_TOKEN for containers and Kubernetes; when it is unset, generate a random token, persist it under the auth directory with mode 0600, and print it once at startup. Require that token in a header or request field for POST /v1/admin/setup, verify it with secrets.compare_digest, and remove the persisted token after the first admin is created. The setup UI can prompt for it while create_first_user remains the atomic final guard. Please add missing-token, wrong-token, valid-token, and concurrent-setup tests.
- The stale-secret wait loop previously used a fixed iteration budget (50 * 0.1s = 5s) that could never reach the 10s stale-file grace period, permanently raising RuntimeError instead of recovering from a crashed writer. Replace it with a wall-clock deadline comfortably above the grace period, and extract both as named constants. - setup_admin now rejects immediately via needs_setup() before validating the password or hashing it, so the endpoint doesn't keep paying bcrypt cost once the first admin account already exists. - Update auth_system.rst: it still described the old auto-generated admin password printed to the log, which no longer happens now that first-run setup goes through /v1/admin/setup.
3 participants
Summary
XINFERENCE_AUTH_ADVANCED=trueto opt in.XINFERENCE_AUTH_ADVANCEDto be enabled by default, so Xinference now requires authentication to access the API out of the box:adminaccount is auto-created with a one-time password printed to the server log (reusing the existing_init_adminlogic), and the user is forced to change the password on first login.XINFERENCE_HOME/auth/when not explicitly set via environment variables, so restarts and multi-process deployments keep using the same keys instead of invalidating previously issued tokens.XINFERENCE_AUTH_ADVANCED=falsestill restores the previous no-auth or--auth-configfile-based mode (the existing mutual-exclusivity check between the two modes is preserved).setup/setup_cluster, which would otherwise fail once advanced auth defaults to on. Updatedconftest.py,test_metrics.py,test_client.py,test_async_client.py,test_llm_model.py, andtest_restful_api.pyto explicitly disable advanced auth in those fixtures, keeping them focused on their original (non-auth) test purpose.doc/source/user_guide/auth_system.rstto document the new default behavior and how to disable it.Test plan
pytest xinference/core/tests/test_restful_api.py: 26 passed, 1 pre-existing failure unrelated to this change (test_lang_chain, missinglangchaindependency, fails identically onmain)pytest xinference/api/tests/test_oauth2_permission_escalation.py xinference/api/tests/test_oauth2_token_expiration.py xinference/core/tests/test_monitor_config_store.py xinference/api/tests/test_launch_history.py: 61 passedXINFERENCE_AUTH_ADVANCEDdefaults toTrue, JWT/encryption keys are auto-generated and persisted, and reused across restarts; explicitly setting it to0/false/no(case-insensitive) correctly disables advanced authpre-commit run --files <changed files>passes (black/flake8/isort/mypy/codespell)