Hi Kirk, thanks for the KIP! Some questions and comments from a first pass:
LM1. There are several new classes proposed that are just based on existing classes. I assume the intention/logic of those remain unchanged with this KIP, and they are just being renamed and moved to make them part of the public, is that correct? LM2. Just to double check, the DefaultJwtValidator behaviour seems to be a proxy to the Client or Broker validator, is my understanding correct? LM3. Regarding the config for sasl.oauthbearer.jwt.validator.class, I expect it allows validator implementations other than the ones provided in the KIP, correct? If so, in the case where a user implements a custom validator and provides it in the config, would that custom validation apply on top of the predefined Broker/ClientJwtValidator as an extension? Or would it be the only validator applied? (The Client and Broker validators seem to perform some basic JWT checks, so trying to understand if this config would allow to bypass those basic validations). LM4. Regarding the config sasl.oauthbearer.assertion.not.before.minutes , why is it that we need this configurable? I get the value of configurable nbf for JWTs when we want to have tokens that will only be valid in a time frame in the future. But in our case we will be authenticating kafka clients, won’t we always need tokens “valid from now on”? (in which case we wouldn’t need to have the user worrying about a new config) LM5. On the proposed changes to the OAuthCompatibilityTool, is there a reason to not allow the new options (props passed by file) to co-exist with the existing options (props passed explicitly)? I expect the main motivation/usage is to pass client secrets via file, but we’re making the users change how they use the tool completely (vs. bringing in a new option to pass client props via file). We have a similar situation in other tools, ex. console-consumer, where we do allow to provide consumer props explicitly in the command line, and via a file, and they co-exist, just one taking precedence (I may be biased on this because of the console consumer). Thanks! Lianet On Thu, Apr 10, 2025 at 5:05 AM Manikumar <manikumar.re...@gmail.com> wrote: > Hi Kirk, > > Thanks for the KIP. This will be a valuable addition for implementing the > JWT Bearer Grant Type in OAuth 2.0 authorization flow. > > I had a few comments and suggestions: > > 1. The “Rejected Alternatives” section notes that Java's WatchService won't > be used. Could you clarify when a dynamic mechanism for detecting file > changes would be required? > Is this aimed at supporting automatic key rotation on the client side? > > 2. We've previously encountered CVEs related to unsafe file access. Should > we consider introducing an allowlist mechanism for file-based configs such > as: > - sasl.oauthbearer.assertion.private.key.file > - sasl.oauthbearer.assertion.file > Similar to the existing ALLOWED_SASL_OAUTHBEARER_URLS_CONFIG? > > 3. I assume these changes work seamlessly with: > - The existing RefreshingLogin mechanism on the client > - Broker reauthentication via connections.max.reauth.ms > Could you please confirm? > > 4. I recommend including Keycloak-based integration tests to ensure > compatibility with standard OAuth providers. > > 5. We currently lack user-facing documentation for OAuth. As part of the > implementation, it would be helpful to include: > - Example client configurations > - A full end-to-end usage guide for the JWT bearer grant flow in Kafka > > > Thanks, > Manikumar > > On Sat, Mar 15, 2025 at 12:23 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 >
