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

Add support for Kerberos credentials #297

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add support for Kerberos credentials #297

wants to merge 6 commits into from

Conversation

rohitsanj
Copy link
Contributor

@rohitsanj rohitsanj commented Jan 15, 2025

Summary of Changes

Completely untested.

Closes #127

The options for Kerberos auth are:

  • keytab_path (required)
  • principal (required), and
  • service_name (optional, defaults to kafka if not provided)

Any additional details or context that should be provided?

SR does not support Kerberos as a client auth mechanism, hence KerberosCredentials is not included in the spec for schema_registry.credentials.

Integration tests for Kerberos auth can be added at a later point (TODO: create issue). For reference, see how librdkafka sets up Kafka environments, using the trivup library.

Pull request checklist

Please check if your PR fulfills the following (if applicable):

  • Tests:
    • Added new
    • Updated existing
    • Deleted existing
  • Have you validated this change locally against a running instance of the Quarkus dev server?
    make quarkus-dev
  • Have you validated this change against a locally running native executable?
    make mvn-package-native && ./target/ide-sidecar-*-runner

@rohitsanj rohitsanj requested a review from a team as a code owner January 15, 2025 08:41
@rohitsanj rohitsanj self-assigned this Jan 15, 2025
@sonarqube-confluent

This comment has been minimized.

@sonarqube-confluent

This comment has been minimized.

String path,
String what
) {
if (principal == null || principal.isBlank()) {
Copy link
Contributor

@jlrobins jlrobins Jan 15, 2025

Choose a reason for hiding this comment

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

Neither should have embedded double-quotes, else the formatting of sasl.jaas.config in kafkaClientProperties() will be corrupted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. I think double-quotes would have to be escaped.

Copy link
Contributor

@Cerchie Cerchie left a comment

Choose a reason for hiding this comment

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

Will there be tests in tests/credentials as well?

@rohitsanj
Copy link
Contributor Author

rohitsanj commented Jan 16, 2025

Will there be tests in tests/credentials as well?

@Cerchie, thanks for noticing! I hadn't looked into it.

I don't think we need to extend the RedactedTestBase for KerberosCredentials (similar to ApiKeyAndSecretTest and so on) since both principal and keytab_path are not secret values, IIUC.

@Cerchie Cerchie self-requested a review January 16, 2025 21:20
@Cerchie
Copy link
Contributor

Cerchie commented Jan 16, 2025

Will there be tests in tests/credentials as well?

@Cerchie, thanks for noticing! I hadn't looked into it.

I don't think we need to extend the RedactedTestBase for KerberosCredentials (similar to ApiKeyAndSecretTest and so on) since both principal and keytab_path are not secret values, IIUC.

thanks!

@sonarqube-confluent

This comment has been minimized.

This reverts commit 944d0c2.
@rohitsanj
Copy link
Contributor Author

I've reverted the commit that added the almost-working integration tests. @flippingbits helped click-test a Quarkus app jar built using this branch against a local Kerberos-enabled CP Kafka environment. However, click-testing with the native executable threw some "method not found" errors. The offending classes will need to be added to the GraalVM reflection configuration.

@sonarqube-confluent
Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 81.60% Coverage (80.20% Estimated after merge)
  • Duplications No duplication information (0.60% Estimated after merge)

Project ID: ide-sidecar

View in SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Kerberos (SSL/GSSAPI) authN to direct connections
4 participants