Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GeorgeJahad
Copy link

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:

oauth.client.credentials.grant.type=alternative_client_credentials

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"

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>
Signed-off-by: George Jahad <github@blackbirdsystems.net>
@mstruk
Copy link
Contributor

mstruk commented Apr 23, 2025

Can you better explain your use case?

The client_credentials grant type is what it is as specified in the OAuth / OIDC spec. Compliant authorization servers know how to handle it and the parameters the client is expected to send are well defined. If you have a different grant type how are we supposed to handle it without it being well specified and implemented? And if it's exactly the same as client_credentials why is the authorization server not able to handle client_credentials grant type?

@feldoh
Copy link

feldoh commented Apr 23, 2025

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 grant_type is what's used on the auth server to handle one type differently to the other. So yes it's identical to client_credentials but the auth server rejects the auth with grant_type=client_credentials but when we pass oauth.client.credentials.grant.type=alternative_client_credentials using this patch (or just by using a normal http client it works fine. We did ask if they could just treat client_credentials the same as our custom one and they said no because they're materially different on the server side. What we want is simply to pass the client assertion from the file which is provided with our token and the grant type the server understands.

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 grant_type because of the underscore. Not that adding two grant_type qsps would be sane, but it's the first thing I tried.

If you wanted to allow for this while making it clear it's non-standard and additional, perhaps an alternative like oauth.client.custom.grant.type is more amenable?

@scholzj
Copy link
Member

scholzj commented Apr 23, 2025

@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.

@feldoh
Copy link

feldoh commented Apr 23, 2025

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.

@el-maurete
Copy link

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:

  • Be non-standard (i.e., override expected behavior for a standard grant type),
  • Be confusing to clients — who might expect client_credentials to behave per spec but find it doesn't, or
  • Require hacky workarounds to simulate multiple credential types under one grant (and to disable the more standard one).

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.

@GeorgeJahad
Copy link
Author

Hey @scholzj and @mstruk Have the latest comments from my workmates changed your feelings about this PR?

@scholzj
Copy link
Member

scholzj commented May 5, 2025

@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.

@mstruk
Copy link
Contributor

mstruk commented May 6, 2025

This does look a legit case, we have not yet encountered. And indeed if simply adding the ability to set custom grant_type while all the rest of the functionality remains unchanged standard client_secret handling, and the change would enable using the library with these custom extension grants, then I think there is value in adding it. I don't think there would be much burden maintaining the library once the changes are merged, including the Strimzi Operator side of the configuration.

@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.

@feldoh
Copy link

feldoh commented May 6, 2025

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:

  • Custom auth extensions may require arbitrary key value pairs which the oauth library cannot be expected to know about, but should pass through unchanged. I would expect these to be on a separate key which behaves like the SASL extensions where there's a common prefix under which I have freedom.
  • SASL extensions to provide additional props to downstream systems which are using the JWT (I believe the regex might be incorrect here because as far as I'm aware there's no strict definition of SASL extension valid keys, Confluent include underscores at least, so I believe that should be more flexible. I don't actually need that right now as I was only testing using it to set grant_type as a workaround, which this PR would naturally support without workarounds).

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.

@GeorgeJahad
Copy link
Author

@feldoh @mstruk so do we need to address anything else here, or is this PR sufficient as it is?

@mstruk
Copy link
Contributor

mstruk commented May 13, 2025

@feldoh I have to take another close look at it. I'll let you know.

@feldoh
Copy link

feldoh commented May 13, 2025

@feldoh @mstruk so do we need to address anything else here, or is this PR sufficient as it is?

It's sufficient as is for my needs. I've got a fork running internally and working well. But it's always better to contribute upstream if we can. Help the community and avoid maintaining a fork; win-win

@GeorgeJahad
Copy link
Author

@mstruk any update?

@mstruk mstruk added this to the 0.17.0 milestone Jun 30, 2025
@mstruk
Copy link
Contributor

mstruk commented Jun 30, 2025

@mstruk any update?

Scheduled for next release.

@GeorgeJahad
Copy link
Author

Scheduled for next release.

Sounds good, thanks!

@mstruk
Copy link
Contributor

mstruk commented Jul 28, 2025

@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.

@GeorgeJahad
Copy link
Author

I opened another

Thanks @mstruk ! I'll take a look.

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

Successfully merging this pull request may close these issues.

5 participants