Hi Jun!

I really appreciate your feedback.

Responses below...

On Fri, Apr 11, 2025, at 1:47 PM, Jun Rao wrote:
> Hi, Kirk,
> 
> Thanks for the KIP. A few comments below.
> 
> JR1. OAuthBearerConfigurable.configure(): Could you describe the possible
> values for saslMechanism? Is it always OAUTHBEARER?

In this case, yes, it's always OAUTHBEARER. I wanted to keep the same signature 
in case there was a reason in the future that something else would be used. 
Maybe I'm trying to make it _too_ future-proof?

> JR2. If sasl.oauthbearer.jwt.retriever.class is not configured, what other
> configs lead to the selection of JwtBearerJwtRetriever?

If sasl.oauthbearer.jwt.retriever.class is not configured, the default 
implementation (DefaultJwtRetriever) will use the sasl.oauthbearer.grant.type 
configuration to determine the implementation.

If the grant type is "client_credentials" (the default), 
ClientCredentialsJwtRetriever will be used. If the grant type value is 
"urn:ietf:params:oauth:grant-type:jwt-bearer", then JwtBearerJwtRetriever is 
used.

> JR3. If sasl.oauthbearer.jwt.validator.class is not configured, how do we
> choose which built-in class to use?

If sasl.oauthbearer.jwt.validator.class is not configured, the default 
implementation (DefaultJwtValidator) will use the presence of the existing 
sasl.oauthbearer.jwks.endpoint.url configuration to make the determination. If 
there is a value for sasl.oauthbearer.jwks.endpoint.url, BrokerJwtValidator 
will be used, otherwise ClientJwtValidator is used.

I will make this more clear in the KIP.

> JR4. Could we document what type can be returned for Object in the
> following?
> public interface AssertionJwtTemplate extends OAuthBearerConfigurable {
> 
>     /**
>      * Returns a map containing zero or more header values.
>      *
>      * @returns Values to include in the JWT header
>      */
>      Map<String, Object> header();
> 
>     /**
>      * Returns a map containing zero or more JWT payload claim values.
>      *
>      * @returns Values to include in the JWT payload
>      */
>      Map<String, Object> payload();

JSON/JWT supports arbitrary data structures, so it's difficult to give a 
succinct list, but I will expound on this in the KIP.

> JR5. "alg": RS256 : Should  RS256 be quoted?

Yes. Thanks :)

> JR6. "Step 2 is to create the JWT payload from the claims provided to
> {@link #create(Map)}": The parameter for create() is not a Map.

Thanks for pointing that out. They used to be maps, but now they're a little 
more involved.

> JR7. For the new configuration table, could we document the type of each
> new config?

Good idea. I'll update the KIP.

> JR8. Could we describe the format of sasl.oauthbearer.assertion.file?

It's a serialized JWT, but I'll elaborate on that in the KIP for clarity.

> JR9. sasl.oauthbearer.assertion.private.key.passphrase: In the "Mutually
> exclusive configuration" part, it should say "If the
> sasl.oauthbearer.assertion.file is used, this will be ignored.", right?

Yes, we could ignore (or log) incompatible configuration instead of throwing an 
error. I'll double check the configuration to make sure the mutually exclusive 
ones are explained correctly.

> JR10. sasl.oauthbearer.assertion.expiration.minutes and
> sasl.oauthbearer.assertion.not.before.minutes: Why are they in minutes
> instead of seconds?

Simply because this design was based on a previous implementation that used 
minutes vs. seconds. I did find minute units odd, but there seemed to be 
precedent for it in offsets.retention.minutes and log.retention.minutes.

JWT expiration being expressed in minutes is fairly typical from what I've seen 
in OAuth applications. However, using seconds (or even milliseconds) fits with 
Kafka convention better and may allow users sub-minute expiration, if desired.

> JR11. "Two of the JAAS options (clientId and clientSecret) for the existing
> client_credentials grant type are deprecated." Should we
> include sasl.oauthbearer.scope too?

Good catch. "scope" in the sasl.jaas.config configuration is deprecated in 
favor of the sasl.oauthbearer.scope configuration. I'll update the KIP.

Thanks,
Kirk

> Thanks,
> 
> Jun
> 
> On Fri, Mar 14, 2025 at 11:52 AM Kirk True <k...@kirktrue.pro> wrote:
> 
> > Hi all,
> >
> > I would like to start a discussion for KIP-1139: Add support for OAuth
> > jwt-bearer grant type:
> >
> > https://cwiki.apache.org/confluence/x/uIxEF
> >
> > The proposal is twofold:
> >
> > * Add support for the OAuth 2.0 JWT Bearer grant type to avoid use of
> > plaintext client secrets
> > * Promote internal APIs for public use by this and future OAuth work
> >
> > Thanks!
> > Kirk
> 

Reply via email to