-
Notifications
You must be signed in to change notification settings - Fork 100
Make the client_credentials grant-type-string configurable. #267
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: George Jahad <github@blackbirdsystems.net>
Signed-off-by: George Jahad <github@blackbirdsystems.net>
Signed-off-by: George Jahad <github@blackbirdsystems.net>
Signed-off-by: George Jahad <github@blackbirdsystems.net>
Can you better explain your use case? The |
I'll follow up with our Auth team for more details on exactly why but my understanding as an end user is that there's n different types of client credential distributed in different situations. A single user may have n different credentials and they can't all be the same type. In these situations Your lib already sort of allows for this with the sasl extensions which let us add almost anything to the url, but sadly it doesn't let us override If you wanted to allow for this while making it clear it's non-standard and additional, perhaps an alternative like |
@feldoh Please keep in mind that we cannot support every single user who decides to do things "differently". So you need to explain why this is an option that makes general sense and is commonly used. |
A very fair point. In this case I honestly believed that this was a common pattern. From my perspective as a client it seems perfectly reasonable that there's a different class of credential for e.g. a dev env, a remote env etc. Why even have grant type be a param if it's just a hard coded string that's always the same (clearly that's why you have protected it), it seems natural to a consumer that it would take multiple values and all of the other systems I've had to interact with so far simply allowed setting it, so I never had a reason to consider it was an unusual pattern. As I say, I'm an end user with no control over the auth server so I will have to ask around to determine why it's different for us, just adding my 👍to say I tried the patch with our auth server to auth with our Kafka clusters and it worked perfectly. It let me swap between our local setup and Strimzi with just a few lines of SASL config. |
Hi all — I'd like to provide some additional context as requested. When we said "our OAuth server has extended the grant_type syntax," that may have been a bit misleading. What we've actually done is define a few custom grant types that use the same parameter structure as client_credentials, but they're intentionally not called client_credentials. Instead, we use identifiers like kubernetes, kerberos, agent, or aws — this allows our identity provider to distinguish between different trust models and validate credentials appropriately. Our internal security policy explicitly disallows the use of the standard client_credentials grant type, as it's not considered secure enough in its default form. While we could technically implement a stricter version of client_credentials within our IdentityServer setup, doing so would:
Instead, we opted for the standard mechanism for custom grants, as described in RFC 6749, Section 4.5. We're using IdentityServer, which fully supports this approach and encourages clearly separated custom flows for better clarity and security. We're definitely not the only ones following this model — the fact that both the OAuth2 spec and IdentityServer explicitly support custom grants shows there’s a broader need for it. Now, I fully appreciate your decision to hardcode grant_type=client_credentials — it helps avoid misconfigurations like typos (client_credential), and provides a safe default. That said, our proposed change is backward-compatible and still preserves your defaults: if the user doesn’t configure a custom grant type, it falls back to client_credentials. It’s a simple extension that enables a standards-based customization path, while keeping your intent and behavior intact for the majority of users. Hope this clarifies our reasoning and helps you feel more confident about supporting this PR. |
@mstruk You are the expert. You will need to decide if this makes sense given the value it adds and the maintenance effort related to it. If you are not sure, we should take it for Issue triage for the next community call. |
This does look a legit case, we have not yet encountered. And indeed if simply adding the ability to set custom @feldoh Can you elaborate on how sasl extensions come into play as part of your overall solution here? It sounds like they are an integral part to implementing the extension grants. |
Happy to. My use of the SASL extensions was only really as a workaround. As they allow setting arbitrary key value pairs, I had wondered if I could use these to set fields which already existed like grant_type. I was foiled here because the underscore isn't permitted in the SASL extensions validation regex in this library. To be clear, I don't think that this "should" work if my understanding of SASL extensions is correct. I believe they should only be passed when using the token, not getting it, but I've seen a few other implementations which combined these systems and passed any SASL extensions as additional auth params. This is really conflating two separate aspects:
As my colleague said, in our case we've used only pre-existing oauth standard keys, so setting grant type is enough for us. I don't (currently) need any SASL extensions. In the more general case I think there's room for a follow up PR to more explicitly support arbitrary extra params on the auth side. There's also plausibly something to look at around that SASL extension regex, but that is definitely unrelated to this PR. |
@feldoh I have to take another close look at it. I'll let you know. |
@mstruk any update? |
Scheduled for next release. |
Sounds good, thanks! |
@GeorgeJahad I opened another PR that reverts some unnecessary changes, makes some cosmetic modifications and rebases the branch so it can be merged. The config options and the logic should be the same, but better have another look there. |
Thanks @mstruk ! I'll take a look. |
My company is using an OAuth server that has extended the grant_type syntax for client_credentials.
To get strimzi-kafka-oauth working with that server, we needed to make the client_credentials grant-type-string configurable.
This PR supports that approach by adding the following sasl.jaas.config option:
If set as above, the JaasClientOauthLoginCallbackHandler sends the string "grant_type=alternative_client_credentials" to the OAuth server. If the option is not set, it falls back to sending the standard string: "grant_type=client_credentials"