Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auth: Support Azure Entra (Event Hub with Kafka Protocol) #530

Merged
merged 12 commits into from
Sep 18, 2024

Conversation

tnewman-at-gm
Copy link
Contributor

@tnewman-at-gm tnewman-at-gm commented Aug 26, 2024

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)
Azure Event Hub provides Kafka protocol compatibility. This PR adds Azure Entra support to allow Kafka UI to authenticate with Event Hub with Kafka protocol using Azure Entra credentials.

Is there anything you'd like reviewers to focus on?
New Azure Entra classes and changes to the SASL configuration options on the front end.

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary) - Created an Event Hub in the Premium Tier with Kafka Enabled. Verified sending and receiving messages.
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES) - Yes, check out Azure Entra Support ui-docs#18
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)
image

@tnewman-at-gm tnewman-at-gm requested review from a team as code owners August 26, 2024 12:54
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels Aug 26, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi tnewman-at-gm! 👋

Welcome, and thank you for opening your first PR in the repo!

Please wait for triaging by our maintainers.

Please take a look at our contributing guide.

@Haarolean Haarolean requested review from germanosin and a team August 26, 2024 14:42
@Haarolean Haarolean added type/enhancement En enhancement/improvement to an already existing feature scope/backend Related to backend changes area/auth App authentication related issues and removed status/triage/manual Manual triage in progress labels Aug 26, 2024
Copy link
Member

@germanosin germanosin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks for contribution

@tnewman-at-gm
Copy link
Contributor Author

This looks great! Thanks for contribution

Thank you. @Haarolean made some very good feedback on this PR, so I will make those updates within the next day or two.

@tnewman-at-gm
Copy link
Contributor Author

This looks great! Thanks for contribution

Thank you. @Haarolean made some very good feedback on this PR, so I will make those updates within the next day or two.

Updated based on feedback

@Haarolean Haarolean self-requested a review August 28, 2024 09:02
@Haarolean Haarolean added the scope/frontend Related to frontend changes label Aug 28, 2024
@Haarolean Haarolean self-assigned this Aug 29, 2024
@tnewman-at-gm
Copy link
Contributor Author

@Haarolean, I addressed all of your comments. Can you take a look again?

@Haarolean Haarolean added this to the 1.1 milestone Sep 18, 2024
@Haarolean Haarolean changed the title FE/BE - Azure Entra Support (Event Hub with Kafka Protocol) Auth: Support Azure Entra (Event Hub with Kafka Protocol) Sep 18, 2024
@Haarolean Haarolean merged commit 04d15b3 into kafbat:main Sep 18, 2024
17 of 18 checks passed
@Haarolean
Copy link
Member

@tnewman-at-gm thanks for your contribution!

@tnewman-at-gm tnewman-at-gm deleted the Feature/Azure-entra branch September 18, 2024 23:29
@credmond
Copy link

credmond commented Dec 1, 2024

@Haarolean / @tnewman-at-gm: why is a a login callback handler (AzureEntraLoginCallbackHandler) being specified as sasl.client.callback.handler.class and not sasl.login.callback.handler.class?

I think either the class is named incorrectly, or the property is incorrect...?

@Haarolean
Copy link
Member

@credmond why should it be? It's the same for IAM:

  'sasl.client.callback.handler.class':
            'software.amazon.msk.auth.iam.IAMClientCallbackHandler',

@credmond
Copy link

credmond commented Dec 2, 2024

It's not the same as IAMClientCallbackHandler (correctly) has the word "Client" in it, not "Login". (I tried to use "bold" in my previous comment to make my point obvious.) There is also a class called: software.amazon.msk.auth.iam.IAMOAuthBearerLoginCallbackHandler. See the word "Login"? That's intended for sasl.login.callback.handler.class (if used). They're following the naming convention.

There are two important properties in Kafka Java clients:

sasl.login.callback.handler.class
Some examples:

  • org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginCallbackHandler
  • org.apache.kafka.common.security.oauthbearer.secured.OAuthBearerLoginCallbackHandler
  • org.apache.kafka.common.security.oauthbearer.internals.OAuthBearerSaslClientCallbackHandler
  • org.apache.kafka.common.security.oauthbearer.internals.unsecured.OAuthBearerUnsecuredLoginCallbackHandlerientCallbackHandler
  • com.certak.kafkio.logic.kafka.pt.auth.azure.AzureEntraLoginCallbackHandler
  • software.amazon.msk.auth.iam.IAMOAuthBearerLoginCallbackHandler

None of those refer to "Client" in their class name.

sasl.client.callback.handler.class
Examples:

  • org.apache.kafka.common.security.authenticator.SaslClientCallbackHandler
  • org.apache.kafka.common.security.kerberos.KerberosClientCallbackHandler
  • software.amazon.msk.auth.iam.IAMClientCallbackHandler
  • io.kafbat.ui.sasl.azure.entra.AzureEntraLoginCallbackHandler

None of these refer to "Login" in their classname, except yours.

So if you look around at all internal or 3rd-party libs, you'll see many classes with the word "Login", intended for use with sasl.login.callback.handler.class and many with the word "Client", intended for sasl.client.callback.handler.class. It's the convention. You do not see a class with the word "Login" in its name, intended to be used with sasl.client.callback.handler.class, or vica-versa.

Also, to make things more confusing, the Azure docs refer to using the "login" (not client) property (https://learn.microsoft.com/en-us/azure/event-hubs/azure-event-hubs-kafka-overview) in their examples:

sasl.login.callback.handler.class=CustomAuthenticateCallbackHandler

My point is, if you're building a class intended to be used for sasl.login.callback.handler.class, you usually do not use the word "Client" in it, and if you're building a class intended for use with sasl.client.callback.handler.class, you usually do not use the word "Login" in it. It's confusing and goes against the convention that everyone else had followed.

So my question is why you've done this and if it's definitely correct?

@Haarolean
Copy link
Member

@credmond feel free to raise a PR, I don't mind accepting the renaming until this is released.

@tnewman-at-gm what do you think?

@tnewman-at-gm
Copy link
Contributor Author

tnewman-at-gm commented Dec 2, 2024

@Haarolean

I agree that sasl.login.callback.handler.class is the correct name for this property.

The Kafka docs directly call out that sasl.login.callback.handler.class is what we should use: https://docs.confluent.io/platform/7.2/kafka/authentication_sasl/authentication_sasl_oauth.html#production-use-of-sasl-oauthbearer

When is the next release planned? I can try to get this fixed, tested and PRed.

@credmond
Copy link

credmond commented Dec 2, 2024

Thanks folks -- I was mainly just curious how/why it was working for you... I don't have an Azure a/c to test.

@tnewman-at-gm
Copy link
Contributor Author

@credmond, I don't use the UI to add configuration. I edit the configuration YAML, so I think you pointed out something that was never going to work if someone tried it.

@credmond
Copy link

credmond commented Dec 2, 2024

Ah, good to know :) Thanks

@tnewman-at-gm
Copy link
Contributor Author

tnewman-at-gm commented Dec 2, 2024

I tested the UI locally. It works with sasl.client.callback.handler.class but not sasl.login.callback.handler.class. I'm going to leave it as is.

image

image

@Haarolean
Copy link
Member

Shouldn't we rename the class in this case?

@tnewman-at-gm
Copy link
Contributor Author

Shouldn't we rename the class in this case?

It could be renamed if this will reduce confusion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/auth App authentication related issues scope/backend Related to backend changes scope/frontend Related to frontend changes status/triage/completed Automatic triage completed type/enhancement En enhancement/improvement to an already existing feature
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants