-
Notifications
You must be signed in to change notification settings - Fork 63
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 vertex-ai-gemini to check OIDC TokenCredential #626
Update vertex-ai-gemini to check OIDC TokenCredential #626
Conversation
...e/src/main/java/io/quarkiverse/langchain4j/vertexai/runtime/gemini/VertxAiGeminiRestApi.java
Outdated
Show resolved
Hide resolved
OK, it works now, got:
Let me update README with the security considerations section. |
c9986ef
to
c2bfa61
Compare
@geoand @jmartisk It all works now nicely, please review. The user authenticates to Google by accessing I've also added a configuration property, keeping the new option enabled by default as it gives a user specific access to the Gemini API out of the box, since setting up default credentials can not be user-unique if used by more than one user. I plan to follow up with a similar demo for Azure OpenAI Thanks |
c2bfa61
to
52bdad5
Compare
I update the demo's user message to request only a short poem about Java. Requesting both |
By the way, this injected |
What can make a very cool demo is to have both vertex ai gemini and azure openai providers used in the same demo but make them accessible to either Azure or Google authenticated users, thus also demonstrating OIDC multitenancy |
52bdad5
to
a41c061
Compare
a41c061
to
35a91a8
Compare
I decided to disable picking up a security identity token by default, it is easy to enable with a property and users should make a decision if it is what they'd like to do |
I like it! I have the same comment as with #634 |
Thanks @geoand, this one can not be because AuthProvider interface is OpenAiRestApi bound, this is what I implied in #634. |
Have fun! We'll take this one up again when you are back |
Thanks @geoand, I'm back now. So, let me refer you to the way OIDC token propagation works in Quarkus in general. For example, with For example, let's say we have an OIDC secured An important point is that adding The situation with this PR (and with #634) is identical as far as the token propagation is concerned - I login with Google to the frontend resources which must propagate a token to the downstream Vertex Gemini (or I login with Azure and then have a token propagated to Azure OpenAI - though I haven't made it confirmed it working yet). But using I see that what I did in these 2 PRs might appear as inflexible, direct dependency on quarkus-security, and some tweaking with If you prefer both of these PRs be re-implemented in terms of Perhaps a 3rd option is to add a REST client filter to the demo which propagates the token, but as implied in the docs, this is less flexible as one can't restrict to specific models. I don't have a strong opinion, let me know please what you prefer, thanks |
Thanks for the input @sberyozkin. Just to make sure we are on the same page, when you say
I assume you mean |
@geoand Thanks, yes, apologies for the typo, meant |
Cool thanks. So here is how I understand this, please correct me if I'm wrong: We move
WDYT? |
Hi @geoand That looks like it can work but I was thinking that we won't use
With So I'm imagining, that if we had What do you think ? |
That sounds fine with me. What I really want is this to be an opt in for users and to be easy to use. |
Sure, for example, in the demo introduced with this PR, users just add I guess finer details like For the final demo I have in mind though, once it also works for Azure OpenAI, I plan to have both providers propagating Azure and Google specific tokens respectively with a single
Definitely, won't be a problem. So how do we proceed ? Can I suggest for @csotiriou to look at such a move ( |
That makes sense to me |
@geoand @sberyozkin hello, just saw this conversation. So, in short I understand that I would need to
|
Yeah, that sounds correct :) |
@csotiriou Indeed, I'll be happy to add a module shipping a Quarkus Security API specific auth provider, and rework this PR to use that new module instead, after you get a chance to move AuthProvider up to the core and update the Azure OpenAI code. |
Closing in favor of #708 |
Related to #610