Skip to content
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

Update azure-openai to check OIDC TokenCredential #634

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sberyozkin
Copy link
Contributor

This PR is similar to #626.

If #626 is approved then, as far as this PR is concerned, once the azure-openai and openai-common changes are reviewed, I'd trim it by avoiding adding yet another demo identical to the one added in #626 and instead, try to merge both demos into a single one, showing multiple quarkus-langchain4j models in action and complementing it with annotation based OIDC tenant selection (either Google or Microsoft Entra ID)

@geoand
Copy link
Collaborator

geoand commented May 31, 2024

Thanks!

I'll have a look next week, but I want @csotiriou to also have a look at this

@sberyozkin
Copy link
Contributor Author

sberyozkin commented May 31, 2024

Thanks @geoand, @csotiriou. Agreed, #646 is great. My hope with this PR is that it just works for Quarkus OIDC or SmallRye JWT users, they only configure Azure OIDC provider, and Azure OpenAI provider and is done, though asking users register dynamic filters to have OIDC tokens propagated can be quite reasonable too, though it will be a bit of an extra effort as they get it for free with the general OIDC token propagation which is really what this PR emulates.

But in any case, for this PR, I'm yet to confirm I can login to Azure and use my authorization code flow token to access Azure OpenAI, I did a few tries yesterday, but to no avail.

@csotiriou If you know what scope I should set here to make it work then let me know please :-)

@csotiriou
Copy link
Contributor

csotiriou commented May 31, 2024

Thanks @geoand, @csotiriou. Agreed, #646 is great. My hope with this PR is that it just works for Quarkus OIDC or SmallRye JWT users, they only configure Azure OIDC provider, and Azure OpenAI provider and is done, though asking users register dynamic filters to have OIDC tokens propagated can be quite reasonable too, though it will be a bit of an extra effort as they get it for free with the general OIDC token propagation which is really what this PR emulates.

Thank you! I'm thinking that OIDC support out of the box may be able to be provided via an additional small module which uses the mechanism of #646 as a base to set up everything automatically with OIDC, thus providing the best of both worlds. This is just a thought, and I don't know if that aligns with your vision, however.

But in any case, for this PR, I'm yet to confirm I can login to Azure and use my authorization code flow token to access Azure OpenAI, I did a few tries yesterday, but to no avail.

@csotiriou If you know what scope I should set here to make it work then let me know please :-)

Unfortunately, I'm not sure, since I'm using SSO to login and retrieve a token, and the token is vastly different.

Is there any error you are getting by using this PR, or something similar? Also, may I ask about your setup? Can I set up a similar environment using a personal account or something similar for testing purposes?

@sberyozkin
Copy link
Contributor Author

sberyozkin commented May 31, 2024

Hi @csotiriou Sure, we can finalize the setup for OIDC flows once your PR goes in. (Note the same though would have to work for any provider like vertex-gemini and other similar providers passing tokens, I have a PR opened a bit earlier which works - which is why, should we depend for OIDC on your new interface, it should really be a global one...)

I'm getting infamous "Unauthorized. Access token is missing, invalid, audience is incorrect (https://cognitiveservices.azure.com), or have expired.".

quarkus-langchain4j setup is in this PR, if you look at the samples/secure-azure-openai-poem, in application.properties.
I've tried setting a scope extra param there to https://cognitiveservices.azure.com, https://cognitiveservices.azure.com/.default. Apparently someone made it work for the client credentials grant, but I haven't seen anyone confirming it for the authorization code flow, unless there are some indirect confirmations.

I submitted a form to request adding Azure AI service to my Azure tenant, and when it was approved I created an Azure OpenAI account, created a new deployment and there I created a quarkus-langchain4j resource. There I added myself as an AI Service Developer to this resource, even though I'm already an owner of it.

The demo flow is that I login to the Quarkus endpoint with Azure and call a poem resource from the HTML page and the token now flows to Azure, but instead of a poem, the above error is returned.

If it can be of interest then indeed, it would be great if you could try this PR with your tenant if you have Azure AI service enabled there. Please take your time in any case, I'm off to my PTO from early morning tomorrow and be back on 11th June, so I won't be able to reply until then.

In any case, thanks for your thoughts, it is appreciated, thanks

@sberyozkin
Copy link
Contributor Author

Also CC Eric, @edeandrea

@geoand
Copy link
Collaborator

geoand commented Jun 3, 2024

I think this makes sense, but I also think that it should be built on top of #646

Comment on lines +308 to +310
if (tokenCredential.isResolvable() && tokenCredential.get() != null) {
context.getHeaders().add("Authorization", "Bearer " + tokenCredential.get().getToken());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this take useSecurityIdentityToken into account?

@@ -0,0 +1,115 @@
# Secure Vertex AI Gemini Poem Demo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong title :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants