-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
fix(azure_sentinel): respect AZURE_AUTHORITY_HOST and derive the Azure Monitor audience per cloud #36137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(azure_sentinel): respect AZURE_AUTHORITY_HOST and derive the Azure Monitor audience per cloud #36137
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,10 @@ | |
| import os | ||
| import time | ||
| import traceback | ||
| from collections.abc import Mapping | ||
| from types import MappingProxyType | ||
| from typing import Final | ||
| from urllib.parse import urlparse | ||
|
|
||
| from litellm._logging import verbose_logger | ||
| from litellm.integrations.custom_batch_logger import CustomBatchLogger | ||
|
|
@@ -27,6 +30,16 @@ | |
| ) | ||
| from litellm.types.utils import StandardAuditLogPayload, StandardLoggingPayload | ||
|
|
||
| DEFAULT_AZURE_AUTHORITY_HOST: Final = "https://login.microsoftonline.com" | ||
| DEFAULT_AZURE_MONITOR_SCOPE: Final = "https://monitor.azure.com/.default" | ||
|
|
||
| MONITOR_SCOPE_BY_AUTHORITY_HOST: Final[Mapping[str, str]] = MappingProxyType( | ||
| { | ||
| "login.microsoftonline.com": DEFAULT_AZURE_MONITOR_SCOPE, | ||
| "login.microsoftonline.us": "https://monitor.azure.us/.default", | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| class AzureSentinelLogger(CustomBatchLogger): | ||
| """ | ||
|
|
@@ -42,6 +55,7 @@ def __init__( | |
| client_id: str | None = None, | ||
| client_secret: str | None = None, | ||
| audit_stream_name: str | None = None, | ||
| authority_host: str | None = None, | ||
| **kwargs, | ||
| ): | ||
| """ | ||
|
|
@@ -62,6 +76,10 @@ def __init__( | |
| If not provided, will use AZURE_SENTINEL_CLIENT_SECRET or AZURE_CLIENT_SECRET env var. | ||
| audit_stream_name (str, optional): Stream name from DCR for audit logs. | ||
| If not provided, will use AZURE_SENTINEL_AUDIT_STREAM_NAME env var or the standard stream name. | ||
| authority_host (str, optional): Microsoft Entra authority host that issues the OAuth2 token, | ||
| e.g. "https://login.microsoftonline.us" for Azure Government. If not provided, will use | ||
| AZURE_AUTHORITY_HOST env var or default to the Azure Public Cloud authority. The Azure | ||
| Monitor audience is derived from it. | ||
| """ | ||
| self.async_httpx_client = get_async_httpx_client(llm_provider=httpxSpecialProvider.LoggingCallback) | ||
|
|
||
|
|
@@ -76,6 +94,9 @@ def __init__( | |
| resolved_client_secret: Final = ( | ||
| client_secret or os.getenv("AZURE_SENTINEL_CLIENT_SECRET") or os.getenv("AZURE_CLIENT_SECRET") | ||
| ) | ||
| resolved_authority_host: Final = self._normalize_authority_host( | ||
| authority_host or os.getenv("AZURE_AUTHORITY_HOST") or DEFAULT_AZURE_AUTHORITY_HOST | ||
| ) | ||
|
|
||
| if not resolved_dcr_immutable_id: | ||
| raise ValueError( | ||
|
|
@@ -119,7 +140,8 @@ def __init__( | |
| ) | ||
|
|
||
| # OAuth2 scope for Azure Monitor | ||
| self.oauth_scope = "https://monitor.azure.com/.default" | ||
| self.authority_host = resolved_authority_host | ||
| self.oauth_scope = self._resolve_oauth_scope(authority_host=resolved_authority_host) | ||
| self.oauth_token: str | None = None | ||
| self.oauth_token_expires_at: float | None = None | ||
|
|
||
|
|
@@ -129,6 +151,26 @@ def __init__( | |
| self.log_queue: list[StandardLoggingPayload] = [] | ||
| self.audit_log_queue: list[StandardAuditLogPayload] = [] | ||
|
|
||
| @staticmethod | ||
| def _normalize_authority_host(authority_host: str) -> str: | ||
| """ | ||
| Normalize an authority host into an absolute URL with no trailing slash. | ||
|
|
||
| Accepts the scheme-qualified form litellm documents ("https://login.microsoftonline.us") | ||
| and the bare-host form the azure-identity AzureAuthorityHosts constants use. | ||
| """ | ||
| stripped: Final = authority_host.strip().rstrip("/") | ||
| return stripped if "://" in stripped else f"https://{stripped}" | ||
|
Comment on lines
+154
to
+163
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟨 Authority host is used unvalidated to build the token URL, allowing client secrets over cleartext or to an arbitrary host
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Declined, same as the Cursor finding. Authority host is proxy-admin-only via env, and that admin already holds the client secret. |
||
|
|
||
| @staticmethod | ||
| def _resolve_oauth_scope(authority_host: str) -> str: | ||
| """ | ||
| Map an authority host to the Azure Monitor Logs Ingestion audience for the same cloud, | ||
| falling back to the Azure Public Cloud audience for an unrecognized host. | ||
| """ | ||
| host: Final = urlparse(authority_host).hostname or "" | ||
| return MONITOR_SCOPE_BY_AUTHORITY_HOST.get(host, DEFAULT_AZURE_MONITOR_SCOPE) | ||
|
Comment on lines
+165
to
+172
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Logs still fail for Azure China and other sovereign clouds because the wrong audience is silently used An authority host that is not one of the two hard-coded entries falls back to the commercial Azure Monitor audience ( Unrecognized authority hosts silently map to the commercial audience
Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deliberate scope cut; description now says China is unmapped. Unlisted clouds get the commercial audience exactly as before this PR, so no regression. |
||
|
|
||
| @staticmethod | ||
| def _build_api_endpoint(endpoint: str, dcr_immutable_id: str, stream_name: str) -> str: | ||
| return f"{endpoint.rstrip('/')}/dataCollectionRules/{dcr_immutable_id}/streams/{stream_name}?api-version=2023-01-01" | ||
|
|
@@ -150,7 +192,7 @@ async def _get_oauth_token(self) -> str: | |
| assert self.client_id is not None, "client_id is required" | ||
| assert self.client_secret is not None, "client_secret is required" | ||
|
|
||
| token_url: Final = f"https://login.microsoftonline.com/{self.tenant_id}/oauth2/v2.0/token" | ||
| token_url: Final = f"{self.authority_host}/{self.tenant_id}/oauth2/v2.0/token" | ||
|
|
||
| token_data: Final = { | ||
| "client_id": self.client_id, | ||
|
|
||
This comment was marked as off-topic.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deliberately cut as speculative after review. Mixed sovereign-identity plus commercial-Sentinel deployments aren't a shape we're solving here. Documented under Behavior changes.