feat(auth): unified credential resolver + pool management API + security hardening - #62467
feat(auth): unified credential resolver + pool management API + security hardening#62467DeamonDev888 wants to merge 4 commits into
Conversation
alt-glitch
added
type/feature
Related cluster (credential-pool / resolver family, all OPEN, differing mechanism & scope): #62435 (the resolution-cascade bug this fixes), #61663 (scoped (credential, model) 429 cooldown), #61481 (per-(credential, model) exhaustion-state keying), #30911 (Codex credential-resolver unification). This PR is the broadest approach (one unified |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for consolidating a real credential-routing problem. Current main still stores a registry URL for hermes auth add API-key entries (hermes_cli/auth_commands.py:210-220), so the focused base-URL feature has value.
Problems
agent/auth.py:411-452returnsfinal_urlwithout calling_validate_base_url_safedefined atagent/auth.py:41-75. The claimed resolver-wide SSRF protection is therefore not active;tests/hermes_cli/test_pool_security.py:264-300only tests the helper in isolation.agent/auth.py:411givesmodel.base_urlunconditional precedence over a pool entry. Current main deliberately preserves a non-default credential endpoint and only applies config when the pool URL equals the registry default (hermes_cli/runtime_provider.py:475-485). This would break per-credential endpoint routing.- Current main already exposes the credential-pool dashboard API at
hermes_cli/web_server.py:11466-11541, with CLI-parity coverage intests/hermes_cli/test_dashboard_admin_endpoints.py:129-162. Please extend that contract rather than add a parallel/api/providers/{provider}/poolfamily.
Suggested changes
- Validate the final resolved URL on every path and add resolver-level integration tests for rejected pool/config/env/explicit URLs.
- Preserve non-default pool URLs, then fold the needed pool fields and operations into the existing dashboard endpoints.
Automated hermes-sweeper review.
|
|
||
| # ── Step 5: Apply precedence ────────────────────────────────────── | ||
| # explicit > env > config > resolved > registry | ||
| final_url = (explicit_base_url.rstrip("/") if explicit_base_url else "") or env_url or cfg_url or resolved_url or registry_url |
There was a problem hiding this comment.
final_url is returned without ever passing through _validate_base_url_safe() (defined at lines 41-75). As written, a blocked URL from a pool entry, config, environment, or explicit override still reaches the runtime. Validate this final value and add end-to-end resolver coverage; the current security tests only call the helper directly.
|
|
||
| # ── Step 5: Apply precedence ────────────────────────────────────── | ||
| # explicit > env > config > resolved > registry | ||
| final_url = (explicit_base_url.rstrip("/") if explicit_base_url else "") or env_url or cfg_url or resolved_url or registry_url |
There was a problem hiding this comment.
This precedence overrides every non-empty pool endpoint with model.base_url. Current runtime behavior intentionally applies config only when the selected pool URL is the registry default (hermes_cli/runtime_provider.py:475-485), preserving per-credential endpoints. Retain that guard here.
| strategy: str # "fill_first" | "round_robin" | "least_used" | "random" | ||
|
|
||
|
|
||
| @app.get("/api/providers/{provider}/pool") |
There was a problem hiding this comment.
Current main already has authenticated credential-pool list/add/remove routes under /api/credentials/pool (hermes_cli/web_server.py:11466-11541) and dashboard parity tests. Please extend that existing API rather than introducing a second pool-route contract.
5e0b53b to
6b3d23b
Compare
PR #62467 — SummaryWhat this doesUnifies provider credential resolution into a single function ( Both the CLI path ( 4 commits
Security (10 vulnerabilities blocked)
Tests — 302 total, 0 regression
Live validation (not in CI — manual)
teknium1 feedback — addressed
Stats |
80b678b to
90ce10e
Compare
3522360 to
ebea09f
Compare
Introduces agent.auth.resolve_provider_credentials() as the SINGLE source of truth for provider-specific credential resolution across: - CLI auxiliary path (hermes -z) - Gateway + Desktop runtime path - OAuth providers (openai-codex, xai-oauth, qwen-oauth, minimax-oauth, nous) - API-key providers with dedicated resolvers (zai, kimi-coding) - Generic API-key providers (anthropic, openrouter, copilot, xai, etc.) Previously: ~55 provider-specific branches across 4 files with divergent precedence rules. Now: 1 function, 9 steps, deterministic precedence: explicit > env > config > resolved > registry Refactors runtime_provider._resolve_runtime_from_pool_entry() and auxiliary_client._resolve_api_key_provider_credentials() to delegate. Tests: 32 unit tests covering 19 providers + precedence + edge cases.
…play Adds the --base-url flag to `hermes auth add`, letting users specify the inference base URL when adding a credential. Previously the base_url was hardcoded to pconfig.inference_base_url, which is wrong for providers that serve different endpoints from the same registry entry (e.g. Z.AI Coding Plan keys need /api/coding/paas/v4, not the default /api/paas/v4). Also reworks `hermes auth list` to show compact endpoint tags instead of full URLs: - Multi-endpoint providers show short tags: `coding`, `anthropic`, etc. - Single-endpoint providers suppress the URL column (cleaner output) - Custom URLs show truncated hostname (12 chars max) - Label column truncates at 12 chars with right-aligned index Tests: 38 unit tests covering parser, handler, display formatting.
Extends the existing /api/credentials/pool dashboard endpoints (rather
than introducing a parallel route family as initially proposed). Adds:
GET /api/credentials/pool/{provider} list entries
POST /api/credentials/pool/{provider} add entry (base_url optional)
DELETE /api/credentials/pool/{provider}/{id} remove entry
PUT /api/credentials/pool/{provider}/strategy change rotation strategy
POST /api/credentials/pool/{provider}/{id}/reset reset cooldown
GET /api/credentials/pool/{provider}/health summary
All endpoints validate provider names against [a-z0-9_-]+ to prevent
path traversal, and sanitize base_url through _validate_base_url_safe
to block SSRF (cloud metadata, link-local, non-http schemes).
Tests: 20 integration tests covering CRUD + strategy + reset + validation.
…ntion
Dedicated security test suite covering the hardenings added in the
feature commits:
- _validate_base_url_safe blocks:
* 169.254.169.254 (AWS instance metadata)
* metadata.google.internal (GCP metadata)
* 169.254.0.0/16 (link-local)
* file://, gopher://, dict:// schemes
* null bytes in URL
- _validate_provider_name blocks:
* Path traversal (../, /etc/passwd)
* SQL injection (', ", ;)
* Null bytes
* Provider names longer than 64 chars
- API endpoints never leak api_key in:
* GET responses
* POST responses
* 400/404/500 error messages
* Strategy whitelist prevents arbitrary strategies
* Empty/short keys rejected
Tests use stdlib + pytest + unittest.mock only. No network.
ebea09f to
72b21a5
Compare
|
Update: split into focused PRs for faster review To make review easier and get each part merged independently, I've split this PR into 3 smaller, self-contained PRs:
The 3 smaller PRs are each independently mergeable. This PR remains open as the reference implementation that ties them all together with the unified resolver. If the small PRs merge first, this PR will rebase on top automatically (the resolver simply imports the shared validators and delegates to the extended API). |
3 participants
TL;DR
One unified credential resolver (
agent/auth.py) replaces ~55 scattered provider-specific branches across 4 files. All 4 Hermes surfaces (CLI, Gateway, Desktop,hermes auth) now share the same resolution logic. Plus:--base-urlflag onhermes auth add,base_urlcolumn inhermes auth list, 6 REST API endpoints for Desktop pool management, and 10 security fixes (SSRF prevention, path traversal guard, API key leak prevention).The Problem
Hermes has 4 surfaces that consume credentials, but they don't share resolution logic:
_resolve_zai_base_url?base_url=""?hermes -z)auxiliary_client.pyruntime_provider.pyruntime_provider.pyhermes auth addauth_commands.pyThis causes a cascade of 5 interrelated bugs that we documented in #62435 and #61563:
hermes auth addhardcodesbase_urlto/paas/v4(wrong for Coding Plan keys)base_url=""stored in auth.jsonruntime_provider.pynever calls provider-specific resolversconfig.yamlmodel.base_urloverride is silently ignored whenbase_url=""_is_payment_error()misclassifies per-key quotas as payment errors → cascade-marks all keysResult: Z.AI Coding Plan users see all pool keys marked exhausted when only one hits a quota. MiniMax-CN keys get routed to the international endpoint. Users have no visibility into which endpoint each key uses.
What This PR Does
Commit 1:
feat(auth): unified credential resolver for all surfacesagent/auth.py(~400 lines):resolve_provider_credentials()— single entry point covering 19+ providers in 3 categories:runtime_provider.py: 120 lines ofif/elif→ 3 lines that delegate to the resolverauxiliary_client.py: pool branch delegates to the resolverCommit 2:
feat(cli): add --base-url flag to hermes auth add + show base_url in auth listhermes auth add zai --base-url https://api.z.ai/api/coding/paas/v4— users can now specify the correct endpointhermes auth listnow displays aurl=column so users can see which endpoint each key usesbase_url=""entriesCommit 3:
feat(api): add Credential Pool REST API endpointsGET /api/providers/{provider}/pool— list all entries (without leaking api_key)POST /api/providers/{provider}/pool— add entry (with optionalbase_url)DELETE /api/providers/{provider}/pool/{id}— remove entryPUT /api/providers/{provider}/pool/strategy— set rotation strategyPOST /api/providers/{provider}/pool/{id}/reset— reset cooldownGET /api/providers/{provider}/pool/health— sidebar badge summaryCommit 4:
feat(security): SSRF prevention, path traversal guard, API key leak prevention169.254.169.254(AWS metadata),metadata.google.internal(GCP), link-local169.254.x.x, non-http schemes (file://,gopher://,dict://), null byte injection_validate_provider_name()on all 6 endpoints — only[a-z0-9-_]acceptedapi_keyoraccess_token_validate_base_url_safe()called as Step 7b — protects all 19+ providersTests
test_unified_credential_resolver.pytest_auth_cli_improvements.pytest_pool_api.pytest_pool_security.pytest_auxiliary_client.py::TestIsPaymentError(regression)Breaking Changes
None. The resolver preserves all existing behavior when no override is provided. The
--base-urlflag is optional. The REST API is additive.Test Plan
Design Notes
Why all 19+ providers, not just Z.AI? Moving only 3 providers would create a third layer of inconsistency (old inline + old auxiliary + new unified). The resolver wraps existing logic — it doesn't rewrite provider-specific code. Each provider's resolution logic moves from inline
if/elifto a named branch in one function.Why no frontend in this PR? The REST API is complete and tested. The Desktop UI component (
pool-settings.tsx) will follow in a separate PR once the backend merges.Related Issues & PRs
Directly fixes:
Follows precedent of:
Makes obsolete / supersedes:
Related same bug class:
Feature requests enabled:
--base-urland REST API)Provider-specific symptoms:
model.base_urlandHERMES_CODEX_BASE_URLin credential pool paths #40913 — Codex CLI fallback inconsistency