strimzi / strimzi-kafka-oauth

OAuth2 support for Apache Kafka® to work with many OAuth2 authorization servers
Apache License 2.0
140 stars 89 forks source link

Fix nullpointer that occurs when OAuthKafkaPrincipalBuilder is used with Kerberos listener #207

Closed akaczano closed 7 months ago

akaczano commented 10 months ago

As described in #208,

The principal builder extends the DefaultKafkaPrincipalBuilder but it just passes nulls to the two objects, SslPrincipalMapper and KerberosShortNamer in the super class constructor. For the first object, some reflection is used to initialize it anyway, but this is not done for the KerberosShortNamer. The result is that, if this principal builder is used on a broker that has a listener configured for Kerberos authentication, a null pointer exception is thrown here whenever a client tries to authenticate. Since my goal is to add an Oauth listener in-place to an existing Kafka cluster and then begin migrating clients from Kerberos to Oauth, this is a huge problem. As far as I know, there is no way to configure a principal builder for a single listener.

This PR just uses the existing reflection mechanisms to instantiate the KerberosShortNamer in addition to the SslPrincipalMapper, by recreating what Kafka does here and here.

mstruk commented 10 months ago

The scenario when using OAuth and Kerberos authentication in the same cluster was indeed not accounted for. The use-case of needing this co-existence in order to be able to slowly migrate away from LDAP is legitimate, yet this is the first time I've heard of it :)

I don't see a problem adding this support as long as the existing testsuite tests keep passing. We don't intend to ever use Kerberos authentication in Strimzi Operator project.

akaczano commented 10 months ago

@mstruk Thanks for your quick response and willingness to support our use case. As far as I can see, all the test suites are passing. Let me know if there is anything else you need me to do. Any idea when we can expect to see a strimzi-kafka-oauth release with these changes included?

mstruk commented 10 months ago

@akaczano I suggest you add a test to the testsuite, otherwise it's very likely some future work may inadvertently break your fix. You could add a test to testsuite/mock-oauth-tests. There is a docker-compose.yml file where you'll need to add an additional listener in Kafka configuration that is configured with Kerberos authentication, and add some LDAP server container to start up in addition to 'mockauth', 'kafka', and 'zookeeper' containers. Your test should then try to produce some messages to that listener.

akaczano commented 10 months ago

Thanks for the review @mstruk. I just cleaned up the test file like you mentioned. I do not plan to add anything further.

scholzj commented 10 months ago

@mstruk Why does it not run the Travis tests? We should not merge it without that.

mstruk commented 10 months ago

@mstruk Why does it not run the Travis tests? We should not merge it without that.

That's a good question.