You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "rbaddam (via GitHub)" <gi...@apache.org> on 2023/10/05 07:47:44 UTC
[PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
rbaddam opened a new pull request, #14491:
URL: https://github.com/apache/kafka/pull/14491
Made the changes to allow custom KafkaPrincipalBuilder implementations to access SslPrincipalMapper and kerberosShortNamer so that custom KafkaPrincipalBuilders will be able to parse Regex Rules from BrokerSecurityConfigs SSL_PRINCIPAL_MAPPING_RULES_CONFIG
[KIP-982](https://cwiki.apache.org/confluence/display/KAFKA/KIP-982%3A+Access+SslPrincipalMapper+and+kerberosShortNamer+in+Custom+KafkaPrincipalBuilder) have complete details about the change
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "rbaddam (via GitHub)" <gi...@apache.org>.
rbaddam commented on PR #14491:
URL: https://github.com/apache/kafka/pull/14491#issuecomment-1761825493
Also we have implemented the SPIFFE Integration in our echo system and [these changes](https://github.com/apache/kafka/pull/14491) will help to make the Spiffe Integration smooth.
There an Open KIP on the Spiffe Integration [KIP-880](https://cwiki.apache.org/confluence/display/KAFKA/KIP-880%3A+X509+SAN+based+SPIFFE+URI+ACL+within+mTLS+Client+Certificates)
If needed I am willing to Contribute to the above KIP-880 as well.
cc: @omkreddy / @satishd / @mimaison
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "rbaddam (via GitHub)" <gi...@apache.org>.
rbaddam commented on PR #14491:
URL: https://github.com/apache/kafka/pull/14491#issuecomment-1749347191
@mimaison @divijvaidya Could you please review this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "rbaddam (via GitHub)" <gi...@apache.org>.
rbaddam commented on PR #14491:
URL: https://github.com/apache/kafka/pull/14491#issuecomment-1758769891
Hi @mjsax, @jlprat, @satishd, @jolshan, @raboof, @dajac, Would any of you be available to review this new PR I raised?
Your insights and suggestions would be very helpful.
Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "plazma-prizma (via GitHub)" <gi...@apache.org>.
plazma-prizma commented on code in PR #14491:
URL: https://github.com/apache/kafka/pull/14491#discussion_r1350253419
##########
clients/src/main/java/org/apache/kafka/common/network/ChannelBuilders.java:
##########
@@ -240,12 +242,19 @@ public static KafkaPrincipalBuilder createPrincipalBuilder(Map<String, ?> config
KerberosShortNamer kerberosShortNamer,
SslPrincipalMapper sslPrincipalMapper) {
Class<?> principalBuilderClass = (Class<?>) configs.get(BrokerSecurityConfigs.PRINCIPAL_BUILDER_CLASS_CONFIG);
- final KafkaPrincipalBuilder builder;
+ KafkaPrincipalBuilder builder;
if (principalBuilderClass == null || principalBuilderClass == DefaultKafkaPrincipalBuilder.class) {
builder = new DefaultKafkaPrincipalBuilder(kerberosShortNamer, sslPrincipalMapper);
} else if (KafkaPrincipalBuilder.class.isAssignableFrom(principalBuilderClass)) {
- builder = (KafkaPrincipalBuilder) Utils.newInstance(principalBuilderClass);
+ try {
+ Constructor<?> constructor = principalBuilderClass.getConstructor(KerberosShortNamer.class, SslPrincipalMapper.class);
+ builder = (KafkaPrincipalBuilder) constructor.newInstance(kerberosShortNamer, sslPrincipalMapper);
Review Comment:
This looks as if it is solving a problem for a certain Principal type. Not all of the principals have to use KerberosShortNamer, it is related if the credentials are provided by the Kerberos authentication.
Why don't you define a subclass of KerberosPrincipalBuilder instead and provide these details only to that one?
Moreover, you could even define SSLPrincipalBuilder as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "rbaddam (via GitHub)" <gi...@apache.org>.
rbaddam commented on code in PR #14491:
URL: https://github.com/apache/kafka/pull/14491#discussion_r1350996498
##########
clients/src/main/java/org/apache/kafka/common/network/ChannelBuilders.java:
##########
@@ -240,12 +242,19 @@ public static KafkaPrincipalBuilder createPrincipalBuilder(Map<String, ?> config
KerberosShortNamer kerberosShortNamer,
SslPrincipalMapper sslPrincipalMapper) {
Class<?> principalBuilderClass = (Class<?>) configs.get(BrokerSecurityConfigs.PRINCIPAL_BUILDER_CLASS_CONFIG);
- final KafkaPrincipalBuilder builder;
+ KafkaPrincipalBuilder builder;
if (principalBuilderClass == null || principalBuilderClass == DefaultKafkaPrincipalBuilder.class) {
builder = new DefaultKafkaPrincipalBuilder(kerberosShortNamer, sslPrincipalMapper);
} else if (KafkaPrincipalBuilder.class.isAssignableFrom(principalBuilderClass)) {
- builder = (KafkaPrincipalBuilder) Utils.newInstance(principalBuilderClass);
+ try {
+ Constructor<?> constructor = principalBuilderClass.getConstructor(KerberosShortNamer.class, SslPrincipalMapper.class);
+ builder = (KafkaPrincipalBuilder) constructor.newInstance(kerberosShortNamer, sslPrincipalMapper);
Review Comment:
@plazma-prizma made the changes as per your inputs, please review when you get a chance, Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "rbaddam (via GitHub)" <gi...@apache.org>.
rbaddam commented on PR #14491:
URL: https://github.com/apache/kafka/pull/14491#issuecomment-1762132178
Thank you @omkreddy for the review and feedback.
I have updated the [KIP-982](https://cwiki.apache.org/confluence/display/KAFKA/KIP-982%3A+Access+SslPrincipalMapper+and+kerberosShortNamer+in+Custom+KafkaPrincipalBuilder) with the latest changes from the pull request.
Could you please review the changes when you have a moment? Your feedback is highly appreciated.
Thanks,
Raghu Baddam
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "plazma-prizma (via GitHub)" <gi...@apache.org>.
plazma-prizma commented on code in PR #14491:
URL: https://github.com/apache/kafka/pull/14491#discussion_r1353388791
##########
clients/src/main/java/org/apache/kafka/common/network/ChannelBuilders.java:
##########
@@ -240,12 +242,19 @@ public static KafkaPrincipalBuilder createPrincipalBuilder(Map<String, ?> config
KerberosShortNamer kerberosShortNamer,
SslPrincipalMapper sslPrincipalMapper) {
Class<?> principalBuilderClass = (Class<?>) configs.get(BrokerSecurityConfigs.PRINCIPAL_BUILDER_CLASS_CONFIG);
- final KafkaPrincipalBuilder builder;
+ KafkaPrincipalBuilder builder;
if (principalBuilderClass == null || principalBuilderClass == DefaultKafkaPrincipalBuilder.class) {
builder = new DefaultKafkaPrincipalBuilder(kerberosShortNamer, sslPrincipalMapper);
} else if (KafkaPrincipalBuilder.class.isAssignableFrom(principalBuilderClass)) {
- builder = (KafkaPrincipalBuilder) Utils.newInstance(principalBuilderClass);
+ try {
+ Constructor<?> constructor = principalBuilderClass.getConstructor(KerberosShortNamer.class, SslPrincipalMapper.class);
+ builder = (KafkaPrincipalBuilder) constructor.newInstance(kerberosShortNamer, sslPrincipalMapper);
Review Comment:
Thanks for the changes. LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14491:
URL: https://github.com/apache/kafka/pull/14491#issuecomment-1888380144
This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) <p> If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "rbaddam (via GitHub)" <gi...@apache.org>.
rbaddam commented on code in PR #14491:
URL: https://github.com/apache/kafka/pull/14491#discussion_r1350920227
##########
clients/src/main/java/org/apache/kafka/common/network/ChannelBuilders.java:
##########
@@ -240,12 +242,19 @@ public static KafkaPrincipalBuilder createPrincipalBuilder(Map<String, ?> config
KerberosShortNamer kerberosShortNamer,
SslPrincipalMapper sslPrincipalMapper) {
Class<?> principalBuilderClass = (Class<?>) configs.get(BrokerSecurityConfigs.PRINCIPAL_BUILDER_CLASS_CONFIG);
- final KafkaPrincipalBuilder builder;
+ KafkaPrincipalBuilder builder;
if (principalBuilderClass == null || principalBuilderClass == DefaultKafkaPrincipalBuilder.class) {
builder = new DefaultKafkaPrincipalBuilder(kerberosShortNamer, sslPrincipalMapper);
} else if (KafkaPrincipalBuilder.class.isAssignableFrom(principalBuilderClass)) {
- builder = (KafkaPrincipalBuilder) Utils.newInstance(principalBuilderClass);
+ try {
+ Constructor<?> constructor = principalBuilderClass.getConstructor(KerberosShortNamer.class, SslPrincipalMapper.class);
+ builder = (KafkaPrincipalBuilder) constructor.newInstance(kerberosShortNamer, sslPrincipalMapper);
Review Comment:
@plazma-prizma, I appreciate your suggestion. To address your concern about the specificity of the solution, I will create Interfaces/subclasses of KerberosPrincipalBuilder and SSLPrincipalBuilder to handle different types of principals and their configurations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14491:
URL: https://github.com/apache/kafka/pull/14491#issuecomment-2568654597
This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #14491: KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982)
URL: https://github.com/apache/kafka/pull/14491
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "rbaddam (via GitHub)" <gi...@apache.org>.
rbaddam commented on code in PR #14491:
URL: https://github.com/apache/kafka/pull/14491#discussion_r1350588084
##########
clients/src/main/java/org/apache/kafka/common/network/ChannelBuilders.java:
##########
@@ -240,12 +242,19 @@ public static KafkaPrincipalBuilder createPrincipalBuilder(Map<String, ?> config
KerberosShortNamer kerberosShortNamer,
SslPrincipalMapper sslPrincipalMapper) {
Class<?> principalBuilderClass = (Class<?>) configs.get(BrokerSecurityConfigs.PRINCIPAL_BUILDER_CLASS_CONFIG);
- final KafkaPrincipalBuilder builder;
+ KafkaPrincipalBuilder builder;
if (principalBuilderClass == null || principalBuilderClass == DefaultKafkaPrincipalBuilder.class) {
builder = new DefaultKafkaPrincipalBuilder(kerberosShortNamer, sslPrincipalMapper);
} else if (KafkaPrincipalBuilder.class.isAssignableFrom(principalBuilderClass)) {
- builder = (KafkaPrincipalBuilder) Utils.newInstance(principalBuilderClass);
+ try {
+ Constructor<?> constructor = principalBuilderClass.getConstructor(KerberosShortNamer.class, SslPrincipalMapper.class);
+ builder = (KafkaPrincipalBuilder) constructor.newInstance(kerberosShortNamer, sslPrincipalMapper);
Review Comment:
Thanks @plazma-prizma for the feedback,
The above logic will be used whenever user want to config a custom KafkaPrincipalBuilder by extending the existing KafkaPrincipalBuilder.
The goal is to pass already created kerberosShortNamer and sslPrincipalMapper so that User don't miss any functionalities like SSL_PRINCIPAL_MAPPING_RULES_CONFIG when they use custom(own) KafkaPrincipalBuilder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15452: Access SslPrincipalMapper and kerberosShortNamer in Custom KafkaPrincipalBuilder(KIP-982) [kafka]
Posted by "omkreddy (via GitHub)" <gi...@apache.org>.
omkreddy commented on PR #14491:
URL: https://github.com/apache/kafka/pull/14491#issuecomment-1762024387
@rbaddam Thanks for the PR. Can you please update the KIP with newly added interfaces also. I will review the PR once KIP passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org