[FEAT]: GUI: Adding Entra Auth Option to New Targets#1762
Conversation
jbolor21
changed the title
romanlutz
left a comment
romanlutz
left a comment
There was a problem hiding this comment.
Nice work scoping Entra to OpenAI and AzureML targets explicitly. A few things — most importantly a token-leak risk on the AzureML branch, plus the frontend currently assumes every entry in TARGET_TYPE_CONFIG is Entra-capable, which won't hold as we add more target types.
| return new_params | ||
|
|
||
| if issubclass(target_class, AzureMLChatTarget): | ||
| new_params["api_key"] = get_azure_async_token_provider(_AZURE_ML_SCOPE) |
There was a problem hiding this comment.
Security: the AzureML branch has no endpoint validation, but the OpenAI branch above does. If a user submits e.g. https://attacker.example.com/score as the AML endpoint with auth_mode="entra", we'll happily mint an https://ml.azure.com/.default bearer token here and AzureMLChatTarget._get_headers_async will then send it in the Authorization header to the attacker's URL on every request.
Please add a strict hostname-suffix check for AML endpoints (e.g. *.inference.ml.azure.com) analogous to _is_azure_openai_endpoint, and raise before injecting the provider if it doesn't match. The frontend warning should mirror this so the asymmetry is gone end to end.
| # Copy params so we can modify values (eg api_key) without changing request.params. | ||
| params: dict[str, Any] = dict(request.params) | ||
|
|
||
| if request.auth_mode == "entra": |
There was a problem hiding this comment.
Related to the new explicit auth UI: auth_mode="api_key" here doesn't actually guarantee API-key auth. OpenAITarget.__init__ still silently falls back to get_azure_openai_auth(endpoint) whenever there's no key in params, no env var, and "azure" in endpoint.lower(). Combined with the frontend's if (!isEntra && apiKey) params.api_key = apiKey, a user who picks the API Key radio and leaves the field empty will end up authenticated via Entra without ever asking for it.
Now that we have an explicit auth_mode flag, please make auth_mode="api_key" strictly require a key (in params or env) and raise a clear ValueError here if neither is present. The OpenAI "no key → Entra" fallback was a UX shortcut that made sense before the user could choose; with this PR it actively contradicts the choice.
| const entraEndpointError: string | null = (() => { | ||
| if (!isEntra || endpoint === '') return null | ||
| if (isOpenAi && !isAzureOpenAiEndpoint(endpoint)) { | ||
| return 'Error: Entra auth only works with Azure OpenAI / AI Foundry endpoints (for example, *.openai.azure.com or *.ai.azure.com).' |
There was a problem hiding this comment.
These probably don't need the "Error: " prefix but you may want to check how it renders.
| content-type: JSON. | ||
| """ | ||
| if self._api_key_provider is None: | ||
| return self._get_headers() |
There was a problem hiding this comment.
can we just move that logic in here? There are no other callers of _get_headers afaict
2 participants
Description
Adding option for new targets to use entra auth instead of API key only.
Screenshots:
(new auth option):
(new test target added successfully!)
(user validation for non-azure endpoints)
Tests and Documentation
Ran existing tests, added new ones (todo)