Azure Service Principal with Secret authentication workflow. #5131

kzych-inpost · 2024-08-09T14:35:03Z

Context

To my best knowledge (version v1.42.12), the only way to authentiacate using Service Principal workflow is to provide azure_ad_token - either hardcoded (token expires though) or providing a path that starts with oidc/:

model_name: "azure-gpt-35"
    litellm_params:
      model: "azure/gpt-35-turbo"
      api_base: "some-url"
      azure_ad_token: "oidc/azure/api://AzureADTokenExchange"

Why?

However, this workflow does not suit my needs, as it requires AZURE_FEDERATED_TOKEN_FILE.
My goal is to use Service Principal with Secret workflow.

How?

There is a possibility in openai SDK to use azure_ad_token_provider parameter, which is not used yet in the LiteLLM Proxy.

Relevant issues

Related issue: #4417.

Type

🆕 New Feature

Changes

introduced get_azure_ad_token_provider() function;
modified set_client() so it uses get_azure_ad_token_provider() when necessary.

[REQUIRED] Testing - Attach a screenshot of any new tests passing locall

If UI changes, send a screenshot/GIF of working UI fixes

❯ poetry run pytest -q --disable-warnings .
sssssssssssssssssssssssssssssssssssssssssssssssssssssssssss....ssssssssssssssssssss                                           [100%]
4 passed, 79 skipped, 143 warnings in 4.61s

vercel · 2024-08-09T14:35:08Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
litellm	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 20, 2024 11:18am

kzych-inpost · 2024-08-09T14:39:18Z

New usage would be simple, without any azure_ad_token key:

model_name: "azure-gpt-35"
    litellm_params:
      model: "azure/gpt-35-turbo"
      api_base: "some-url"

but these environment variables need to be passed: AZURE_CLIENT_ID, AZURE_TENANT_ID, AZURE_CLIENT_SECRET.

ishaan-jaff

hi @kzych-inpost can you send a screenshot of using azure_ad_token_provider working for your on your proxy server ?

kzych-inpost · 2024-08-12T09:05:21Z

Oh right, hope these screenshots would suffice:

docker-compose.yml - two important lines:

line 81 - using image built locally with my change;
lines 94-96 - environment variables: AZURE_TENANT_ID, AZURE_CLIENT_ID and AZURE_CLIENT_SECRET are used by LiteLLM Proxy

litellm-config.yaml - defined models from my Azure deployments:

and here is the docker output with successful requests:

krrishdholakia · 2024-08-13T02:47:48Z

@ishaan-jaff is this good for merge?

shulkx · 2024-08-14T08:09:44Z

Hopefully, it will be merged soon and then there will no need to refresh the token schedulely anymore when using the model from azure.

Manouchehri · 2024-08-16T20:42:15Z

This has a really high risk of moving people away from the more secure OIDC flow, and encouraging them to use static secrets instead of more-secure temporary credentials. I would really want to see #4417 fixed before this is merged in.

Also, I really don't like that this is loading way more than the title implies. See this huge list for all the things it's doing:

https://learn.microsoft.com/en-us/dotnet/api/azure.identity.defaultazurecredential?view=azure-dotnet

I would really want to see this narrowed down to just ClientSecretCredential.

https://learn.microsoft.com/en-us/python/api/azure-identity/azure.identity.clientsecretcredential?view=azure-python

And finally, get_azure_ad_token_provider is an incredibly misleading name, and will cause people to think that this is the only recommended way of doing auth to Azure, instead of using the more secure (and tested) OIDC flow. At the least, please do not merge this until get_azure_ad_token_provider is removed or renamed to something less misleading.

Manouchehri

See comments in #5131 (comment).

kzych-inpost · 2024-08-19T14:37:24Z

@Manouchehri:

This has a really high risk of moving people away from the more secure OIDC flow, and encouraging them to use static secrets instead of more-secure temporary credentials.

I see your point, but there are use-cases (like mine - Service Principal authentication), where classic OIDC flow for user authentication is not what suits one's need. What is more, the authentication method I've proposed also incorporates fetching temporary token (temporary credential mentioned by you), so I believe this method is also quite secure.

I would really want to see this narrowed down to just ClientSecretCredential.

Sure, I can do that.
On the other hand, authenticating using EnvironmentCredential would make Service Principal authentication more flexible - it would be possible not only for Service principal with secret flow, but also Service principal with certificate and User with username and password. What do you think?

get_azure_ad_token_provider is an incredibly misleading name

Well, azure_ad_token_provider is an exact name of a parameter used in openai.AsyncAzureOpenAI (link). Don't know how can I make the function get_azure_ad_token_provider less misleading, it basically gets the azure_ad_token_provider (the function that fetches the temporary token I mentioned above) required by openai.AsyncAzureOpenAI.

kzych-inpost · 2024-08-19T14:48:00Z

To add more context: advantage of this solution is that it passes the responsibility for Azure authentication to Azure SDK.

krrishdholakia · 2024-08-19T17:02:45Z

litellm/proxy/secret_managers/get_azure_ad_token_provider.py

+import os
+from typing import Callable
+
+from azure.identity import DefaultAzureCredential


can we move the azure. imports to inside the function. This way it's only required, if this approach is used

krrishdholakia · 2024-08-19T17:03:34Z

litellm/router_utils/client_initalization_utils.py

@@ -186,9 +187,16 @@ def set_client(litellm_router_instance: LitellmRouter, model: dict):
                    f"api_base is required for Azure OpenAI. Set it on your config. Model - {_filtered_model}"
                )
            azure_ad_token = litellm_params.get("azure_ad_token")
+            azure_ad_token_provider = None


can we type this correctly, so it's clear what the potential values could be here

krrishdholakia · 2024-08-19T17:04:34Z

litellm/router_utils/client_initalization_utils.py

@@ -303,6 +315,7 @@ def set_client(litellm_router_instance: LitellmRouter, model: dict):
                    "azure_endpoint": api_base,
                    "api_version": api_version,
                    "azure_ad_token": azure_ad_token,
+                    "azure_ad_token_provider": azure_ad_token_provider,


can we add a basic unit test here -

litellm/litellm/tests/test_router_client_init.py

Line 17 in 04d6946

async def test_router_init():

with a mock test to make sure there's no future regressions

OK, should I make another test-function next to this one with mocking environment variables (AZURE_CLIENT_ID, AZURE_CLIENT_SECRET and AZURE_TENANT_ID)?

The test you mentioned is skipped on my machine (problably due to the missing pytest-asyncio plugin and pytest.mark.asyncio decorator). Is this expected behaviour and this would be executed on CI/CD?

Can't test it locally anyway because I have no redis installed.

Prepared the commit 85a8b9e with environment variables and other prerequisites mocked.

krrishdholakia · 2024-08-19T17:05:04Z

Hi @kzych-inpost looks good - left some comments re: refactoring code + Adding unit tests, before we can merge. thank you for this PR!

…zure Service Principal with Secret authentication workflow.

kzych-inpost · 2024-08-20T11:05:11Z

How about the change in the documentation indicatiing that this feature is possible?

…g Azure Service Principal with Secret authentication workflow.

kzych-inpost · 2024-08-29T08:49:58Z

@krrishdholakia all requested changes are already commited :)
Are we ready to merge then?

…5437) * Azure Service Principal with Secret authentication workflow. (#5131) * Implement Azure Service Principal with Secret authentication workflow. * Use `ClientSecretCredential` instead of `DefaultAzureCredential`. * Move imports into the function. * Add type hint for `azure_ad_token_provider`. * Add unit test for router initialization and sample completion using Azure Service Principal with Secret authentication workflow. * Add unit test for router initialization with neither API key nor using Azure Service Principal with Secret authentication workflow. * fix(client_initializtion_utils.py): fix typing + overrides * test: fix linting errors * fix(client_initialization_utils.py): fix client init azure ad token logic * fix(router_client_initialization.py): add flag check for reading azure ad token from environment * test(test_streaming.py): skip end of life bedrock model * test(test_router_client_init.py): add correct flag to test --------- Co-authored-by: kzych-inpost <142029278+kzych-inpost@users.noreply.github.com>

Implement Azure Service Principal with Secret authentication workflow.

9ad75bf

vercel bot deployed to Preview August 9, 2024 14:35 View deployment

ishaan-jaff requested changes Aug 9, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

This was referenced Aug 16, 2024

[Bug]: Azure OpenAI attempts to reuse Azure AD token after it expires, add support for enable_azure_ad_token_refresh with OIDC #4417

Open

(oidc): Add support for loading tokens via a file, env var, and path in env var #5251

Merged

Manouchehri suggested changes Aug 16, 2024

View reviewed changes

krrishdholakia reviewed Aug 19, 2024

View reviewed changes

kzych-inpost added 3 commits August 20, 2024 10:35

Use ClientSecretCredential instead of DefaultAzureCredential.

6237183

Move imports into the function.

3abe163

Add type hint for azure_ad_token_provider.

7f66dee

vercel bot deployed to Preview August 20, 2024 08:54 View deployment

Add unit test for router initialization and sample completion using A…

85a8b9e

…zure Service Principal with Secret authentication workflow.

vercel bot deployed to Preview August 20, 2024 10:59 View deployment

Add unit test for router initialization with neither API key nor usin…

4a360d6

…g Azure Service Principal with Secret authentication workflow.

vercel bot deployed to Preview August 20, 2024 11:18 View deployment

krrishdholakia changed the base branch from main to litellm_stable_merge_prs August 30, 2024 01:34

krrishdholakia merged commit 5787754 into BerriAI:litellm_stable_merge_prs Aug 30, 2024
2 checks passed

kzych-inpost deleted the feature/service_principal_with_secret_workflow branch September 4, 2024 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure Service Principal with Secret authentication workflow. #5131

Azure Service Principal with Secret authentication workflow. #5131

kzych-inpost commented Aug 9, 2024

vercel bot commented Aug 9, 2024 •

edited

Loading

kzych-inpost commented Aug 9, 2024 •

edited

Loading

ishaan-jaff left a comment

kzych-inpost commented Aug 12, 2024

krrishdholakia commented Aug 13, 2024

shulkx commented Aug 14, 2024

This comment was marked as outdated.

This comment was marked as outdated.

Manouchehri commented Aug 16, 2024

Manouchehri left a comment

kzych-inpost commented Aug 19, 2024

kzych-inpost commented Aug 19, 2024

krrishdholakia Aug 19, 2024

kzych-inpost Aug 20, 2024

krrishdholakia Aug 19, 2024

kzych-inpost Aug 20, 2024

krrishdholakia Aug 19, 2024

kzych-inpost Aug 20, 2024 •

edited

Loading

kzych-inpost Aug 20, 2024

krrishdholakia commented Aug 19, 2024

kzych-inpost commented Aug 20, 2024

kzych-inpost commented Aug 29, 2024

Azure Service Principal with Secret authentication workflow. #5131

Azure Service Principal with Secret authentication workflow. #5131

Conversation

kzych-inpost commented Aug 9, 2024

Context

Why?

How?

Relevant issues

Type

Changes

[REQUIRED] Testing - Attach a screenshot of any new tests passing locall

vercel bot commented Aug 9, 2024 • edited Loading

kzych-inpost commented Aug 9, 2024 • edited Loading

ishaan-jaff left a comment

Choose a reason for hiding this comment

kzych-inpost commented Aug 12, 2024

krrishdholakia commented Aug 13, 2024

shulkx commented Aug 14, 2024

This comment was marked as outdated.

This comment was marked as outdated.

Manouchehri commented Aug 16, 2024

Manouchehri left a comment

Choose a reason for hiding this comment

kzych-inpost commented Aug 19, 2024

kzych-inpost commented Aug 19, 2024

krrishdholakia Aug 19, 2024

Choose a reason for hiding this comment

kzych-inpost Aug 20, 2024

Choose a reason for hiding this comment

krrishdholakia Aug 19, 2024

Choose a reason for hiding this comment

kzych-inpost Aug 20, 2024

Choose a reason for hiding this comment

krrishdholakia Aug 19, 2024

Choose a reason for hiding this comment

kzych-inpost Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

kzych-inpost Aug 20, 2024

Choose a reason for hiding this comment

krrishdholakia commented Aug 19, 2024

kzych-inpost commented Aug 20, 2024

kzych-inpost commented Aug 29, 2024

vercel bot commented Aug 9, 2024 •

edited

Loading

kzych-inpost commented Aug 9, 2024 •

edited

Loading

kzych-inpost Aug 20, 2024 •

edited

Loading