mirror of
https://github.com/usestrix/strix.git
synced 2026-10-16 04:28:23 +00:00
fix(llm): address review feedback on Anthropic WIF
- Federate only `anthropic/` models. A bare `claude-` name takes the OpenAI route, which never carries the federated token, so startup reported workload identity auth while every model call failed. - After a failed refresh, re-read the clock before serving the cached token: the token command and exchange can outlast its validity. - Serialize Entra login and logout with token refreshes, so a refresh in flight can't recreate a deleted sign-in or overwrite a new one.
This commit is contained in:
parent
c08256c3ee
commit
e1a4d9ba1a
6 changed files with 87 additions and 15 deletions
|
|
@ -13,7 +13,7 @@ This guide covers two kinds of host:
|
|||
| Any other Linux host: a laptop or desktop, a local VM, or a VM on-premises or in another public or private cloud | Your Entra user | `strix auth login entra` |
|
||||
|
||||
<Note>
|
||||
Federation applies only to models that call Anthropic directly (`anthropic/…` or a bare `claude-…`). **An API key always wins:** if `LLM_API_KEY` is set, in the environment or in the Strix config file, Strix uses it and prints a warning. Federated runs record `"auth_mode": "workload_identity"` in `strix_runs/<run>/run.json`.
|
||||
Federation applies only to `anthropic/…` models, which call Anthropic directly. **An API key always wins:** if `LLM_API_KEY` is set, in the environment or in the Strix config file, Strix uses it and prints a warning. Federated runs record `"auth_mode": "workload_identity"` in `strix_runs/<run>/run.json`.
|
||||
</Note>
|
||||
|
||||
## How setup fits together
|
||||
|
|
|
|||
|
|
@ -43,7 +43,7 @@ Then check the setup with `strix auth test`: it runs one exchange with the same
|
|||
|
||||
Strix re-reads `ANTHROPIC_IDENTITY_TOKEN_FILE` on every exchange, so the file must hold an unexpired JWT each time. `ANTHROPIC_IDENTITY_TOKEN` (a literal JWT) also works for short runs.
|
||||
|
||||
Federation applies only to models that call Anthropic's API directly (`anthropic/...` or a bare `claude-...` name). Claude through Bedrock, Vertex AI, or OpenRouter keeps that provider's authentication.
|
||||
Federation applies only to models that call Anthropic's API directly (`anthropic/...`). A bare `claude-...` name isn't routed to Anthropic, so it doesn't federate either. Claude through Bedrock, Vertex AI, or OpenRouter keeps that provider's authentication.
|
||||
|
||||
### Other identity providers
|
||||
|
||||
|
|
|
|||
|
|
@ -62,15 +62,16 @@ def is_configured() -> bool:
|
|||
|
||||
|
||||
def is_anthropic_model(model_name: str) -> bool:
|
||||
"""Whether LiteLLM sends ``model_name`` to Anthropic's own API.
|
||||
"""Whether ``model_name`` is routed to Anthropic's own API (``anthropic/…``).
|
||||
|
||||
Claude on Bedrock or Vertex carries that provider's prefix and keeps that
|
||||
provider's auth.
|
||||
Only that prefix reaches the LiteLLM route that carries the federated token:
|
||||
a bare ``claude-…`` name goes to the OpenAI route. Claude on Bedrock or
|
||||
Vertex carries that provider's prefix and keeps that provider's auth.
|
||||
"""
|
||||
name = model_name.strip().lower()
|
||||
for prefix in ("litellm/", "any-llm/"):
|
||||
name = name.removeprefix(prefix)
|
||||
return name.startswith("anthropic/") or ("/" not in name and name.startswith("claude"))
|
||||
return name.startswith("anthropic/")
|
||||
|
||||
|
||||
def applies(model_name: str | None, api_key: str | None = None) -> bool:
|
||||
|
|
@ -301,7 +302,8 @@ def get_valid_token() -> str:
|
|||
try:
|
||||
_cached = exchange()
|
||||
except WifAuthError:
|
||||
if _cached is not None and now < _cached[1] - _MANDATORY_REFRESH_S:
|
||||
# The failed exchange (token command plus HTTP) can take a minute.
|
||||
if _cached is not None and time.time() < _cached[1] - _MANDATORY_REFRESH_S:
|
||||
logger.warning(
|
||||
"Anthropic workload identity refresh failed; using the cached token",
|
||||
exc_info=True,
|
||||
|
|
|
|||
|
|
@ -87,6 +87,12 @@ def read_record() -> dict[str, Any] | None:
|
|||
|
||||
|
||||
def save_record(record: dict[str, Any]) -> None:
|
||||
"""Store a new sign-in, after any refresh in flight (this or another process)."""
|
||||
with _refresh_guard():
|
||||
_write_record(record)
|
||||
|
||||
|
||||
def _write_record(record: dict[str, Any]) -> None:
|
||||
write_secret_text(AUTH_PATH, json.dumps(record, indent=2))
|
||||
|
||||
|
||||
|
|
@ -95,11 +101,15 @@ def is_authenticated() -> bool:
|
|||
|
||||
|
||||
def logout() -> bool:
|
||||
"""Delete the stored sign-in; return whether there was one."""
|
||||
existed = AUTH_PATH.exists()
|
||||
with contextlib.suppress(OSError):
|
||||
AUTH_PATH.unlink()
|
||||
return existed
|
||||
"""Delete the stored sign-in; return whether there was one.
|
||||
|
||||
Waits for any refresh in flight, so the refresh can't recreate the file.
|
||||
"""
|
||||
with _refresh_guard():
|
||||
existed = AUTH_PATH.exists()
|
||||
with contextlib.suppress(OSError):
|
||||
AUTH_PATH.unlink()
|
||||
return existed
|
||||
|
||||
|
||||
def is_configured() -> bool:
|
||||
|
|
@ -178,7 +188,7 @@ def get_valid_token() -> str:
|
|||
if latest and latest["refresh"] != record["refresh"] and not _near_expiry(latest):
|
||||
return str(latest["access"])
|
||||
raise
|
||||
save_record(refreshed)
|
||||
_write_record(refreshed)
|
||||
return str(refreshed["access"])
|
||||
|
||||
|
||||
|
|
@ -271,7 +281,9 @@ def _record_from_token_response(
|
|||
|
||||
@contextlib.contextmanager
|
||||
def _refresh_guard() -> Generator[None]:
|
||||
"""Serialize refreshes within (lock) and across (flock) Strix processes."""
|
||||
"""Serialize refreshes, sign-ins and sign-outs within (lock) and across (flock)
|
||||
Strix processes. A refresh holds it from reading the record to saving the new
|
||||
one, so a login or logout can't land in between and be overwritten."""
|
||||
with _refresh_lock:
|
||||
try:
|
||||
import fcntl
|
||||
|
|
|
|||
|
|
@ -392,12 +392,33 @@ def test_mandatory_refresh_failure_raises() -> None:
|
|||
anthropic_wif.get_valid_token()
|
||||
|
||||
|
||||
def test_slow_failed_refresh_does_not_serve_an_expired_token() -> None:
|
||||
with (
|
||||
mock.patch.object(anthropic_wif.time, "time", return_value=1000.0),
|
||||
mock.patch.object(anthropic_wif.requests, "post", return_value=_ok()),
|
||||
):
|
||||
anthropic_wif.get_valid_token()
|
||||
# Expires at 1600. The refresh starts at 1555, inside the advisory window but
|
||||
# outside the mandatory one, and fails only after the token has expired.
|
||||
clock = iter([1555.0, 1555.0, 1610.0, 1610.0])
|
||||
|
||||
def _slow_failure(*_args: Any, **_kwargs: Any) -> Any:
|
||||
raise requests.ConnectionError("down")
|
||||
|
||||
with (
|
||||
mock.patch.object(anthropic_wif.time, "time", side_effect=lambda: next(clock)),
|
||||
mock.patch.object(anthropic_wif.requests, "post", side_effect=_slow_failure),
|
||||
pytest.raises(anthropic_wif.WifAuthError),
|
||||
):
|
||||
anthropic_wif.get_valid_token()
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("model", "expected"),
|
||||
[
|
||||
("anthropic/claude-sonnet-5-5", True),
|
||||
("litellm/anthropic/claude-sonnet-5-5", True),
|
||||
("claude-sonnet-5-5", True),
|
||||
("claude-sonnet-5-5", False),
|
||||
("bedrock/anthropic.claude-sonnet-5-5", False),
|
||||
("vertex_ai/claude-sonnet-5-5", False),
|
||||
("openrouter/anthropic/claude-sonnet-5-5", False),
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ from __future__ import annotations
|
|||
|
||||
import base64
|
||||
import json
|
||||
import threading
|
||||
import time
|
||||
import urllib.parse
|
||||
from typing import TYPE_CHECKING, Any
|
||||
|
|
@ -141,6 +142,42 @@ def test_token_near_expiry_is_refreshed_and_saved() -> None:
|
|||
assert saved["refresh"] == "refresh-2"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("action", ["logout", "login"])
|
||||
def test_logout_and_login_wait_for_a_refresh_in_flight(action: str) -> None:
|
||||
entra_user.save_record(_record(expires_at=time.time() + 60))
|
||||
started, release = threading.Event(), threading.Event()
|
||||
body = {"access_token": "access-2", "refresh_token": "refresh-2", "expires_in": 3600}
|
||||
|
||||
def _slow_post(*_args: Any, **_kwargs: Any) -> Any:
|
||||
started.set()
|
||||
assert release.wait(5)
|
||||
return _response(200, body)
|
||||
|
||||
with mock.patch.object(entra_user.requests, "post", side_effect=_slow_post):
|
||||
refresher = threading.Thread(target=entra_user.get_valid_token)
|
||||
refresher.start()
|
||||
assert started.wait(5)
|
||||
if action == "logout":
|
||||
other = threading.Thread(target=entra_user.logout)
|
||||
else:
|
||||
other = threading.Thread(
|
||||
target=entra_user.save_record, args=(_record(access="new-login"),)
|
||||
)
|
||||
other.start()
|
||||
other.join(0.2)
|
||||
assert other.is_alive() # blocked until the refresh has saved
|
||||
release.set()
|
||||
refresher.join(5)
|
||||
other.join(5)
|
||||
|
||||
saved = entra_user.read_record()
|
||||
if action == "logout":
|
||||
assert saved is None
|
||||
else:
|
||||
assert saved is not None
|
||||
assert saved["access"] == "new-login"
|
||||
|
||||
|
||||
def test_not_signed_in_raises() -> None:
|
||||
with pytest.raises(entra_user.EntraAuthError, match="strix auth login entra"):
|
||||
entra_user.get_valid_token()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue