Hi Lianet! Thank you for your feedback!
My responses are inline below... On Fri, Apr 11, 2025, at 1:16 PM, Lianet M. wrote: > 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? Yes. The high level intent is to "promote" those internal classes to be publicly reusable. But it does delve into a Ship of Theseus area of discussion because so much of the internals change as a result of the refactoring. > LM2. Just to double check, the DefaultJwtValidator behaviour seems to be a > proxy to the Client or Broker validator, is my understanding correct? 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). The validator logic as specified by the user's configuration is used _instead of_ the default. Perhaps it makes sense to refactor the validation logic into more reusable components that can then be chained/composed. Do you think that's important enough to implement for the first pass? > 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) I agree with your point that the token is only "valid from now on." I guess I was worried about clock skew between the client and the broker. I'm not sure exactly the purview of the JWT nbf claim vs. clock skew. I'll need to give it more though and circle back on that. > 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). You raise a good argument and the tools we package should use the same patterns, where possible. Plus, the tool is technically part of the public interface. It's a little bit than, say, the console consumer, since the OAuth validation tool specifies client and broker configuration, but I'll return to that part of the KIP with your perspective. I'll update the KIP to keep the use of individual command line arguments *and* the config argument, but add some sort of disclaimer in the docs/usage. Thanks, Kirk > > 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 > > >
