Hi Jun,
On Fri, Apr 18, 2025, at 10:19 AM, Jun Rao wrote:
> Hi, Kirk,
>
> Thanks for the reply.
>
> JR1. If we have no clear usage, it's probably better to omit it. We could
> always add a new param in the future if needed, right?
OK, I'll remove the saslMechanism from the OAuthBearerConfigurable interface.
> JR2. Could we document it? Also, when is FileJwtRetriever chosen?
I've included detailed flowcharts in the KIP to document the logic for
DefaultJwtRetriever and DefaultJwtValidator. Please let me know if there is
more needed.
Thanks,
Kirk
>
> Jun
>
>
>
> On Tue, Apr 15, 2025 at 1:04 PM Kirk True <k...@kirktrue.pro> wrote:
>
> > 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
> > >
> >
>
