You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "Serwios (via GitHub)" <gi...@apache.org> on 2024/12/10 21:22:31 UTC
[PR] MINOR: Refactored default resource type based auth [kafka]
Serwios opened a new pull request, #18130:
URL: https://github.com/apache/kafka/pull/18130
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
Refactors the authorization logic to improve readability and maintainability. The main changes include:
1. Created class to separate already big default method implementation from other interface methods
2. Separated big method into smaller onse for better readability and maintainability
2. I ensured that ACL bindings and permission types are handled clearly, and the logic for handling resource patterns (literal and prefix) is optimized
### Committer Checklist (excluded from commit message)
- [X] Verify design and implementation
- [X] Verify test coverage and CI build status
- [X] 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] MINOR: Refactored default resource type based auth [kafka]
Posted by "overpathz (via GitHub)" <gi...@apache.org>.
overpathz commented on code in PR #18130:
URL: https://github.com/apache/kafka/pull/18130#discussion_r1878885824
##########
clients/src/main/java/org/apache/kafka/server/authorizer/DefaultResourceTypeBasedAuthorization.java:
##########
@@ -0,0 +1,144 @@
+package org.apache.kafka.server.authorizer;
+
+import org.apache.kafka.common.acl.*;
+import org.apache.kafka.common.errors.AuthorizationException;
+import org.apache.kafka.common.resource.PatternType;
+import org.apache.kafka.common.resource.ResourcePattern;
+import org.apache.kafka.common.resource.ResourcePatternFilter;
+import org.apache.kafka.common.resource.ResourceType;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.utils.SecurityUtils;
+
+import java.util.*;
+
+public class DefaultResourceTypeBasedAuthorization {
+ private DefaultResourceTypeBasedAuthorization() {}
+
+ public static AuthorizationResult authorize(
+ AuthorizableRequestContext requestContext,
+ AclOperation op,
+ ResourceType resourceType,
+ Authorizer authorizer
+ ) {
+ SecurityUtils.authorizeByResourceTypeCheckArgs(op, resourceType);
+
+ if (isSuperUserAuthorized(requestContext, op, resourceType, authorizer)) {
+ return AuthorizationResult.ALLOWED;
+ }
+
+ Map<PatternType, Set<String>> denyPatterns = initializePatternMap();
+ Map<PatternType, Set<String>> allowPatterns = initializePatternMap();
+
+ boolean hasWildcardAllow = false;
+ KafkaPrincipal principal = createKafkaPrincipal(requestContext);
+ String hostAddr = requestContext.clientAddress().getHostAddress();
+
+ for (AclBinding binding : authorizer.acls(createAclBindingFilter(resourceType))) {
+ if (!isValidBinding(binding, principal, hostAddr, op)) {
+ continue;
+ }
+
+ if (binding.entry().permissionType() == AclPermissionType.DENY) {
+ processDenyAcl(binding, denyPatterns);
+ continue;
+ }
+
+ if (binding.entry().permissionType() == AclPermissionType.ALLOW) {
+ processAllowAcl(binding, allowPatterns);
+ if (binding.pattern().name().equals(ResourcePattern.WILDCARD_RESOURCE)) {
+ hasWildcardAllow = true;
+ }
+ }
+ }
+
+ if (hasWildcardAllow) {
+ return AuthorizationResult.ALLOWED;
+ }
+
+ return evaluateAuthorizationResult(allowPatterns, denyPatterns);
+ }
+
+ private static boolean isSuperUserAuthorized(AuthorizableRequestContext requestContext, AclOperation op, ResourceType resourceType, Authorizer authorizer) {
+ return authorizer.authorize(requestContext, Collections.singletonList(new Action(
+ op, new ResourcePattern(resourceType, "hardcode", PatternType.LITERAL),
+ 0, true, false)))
+ .getFirst() == AuthorizationResult.ALLOWED;
+ }
+
+ private static Map<PatternType, Set<String>> initializePatternMap() {
+ return new EnumMap<>(PatternType.class) {{
+ put(PatternType.LITERAL, new HashSet<>());
+ put(PatternType.PREFIXED, new HashSet<>());
+ }};
+ }
+
+ private static KafkaPrincipal createKafkaPrincipal(AuthorizableRequestContext requestContext) {
+ return new KafkaPrincipal(
+ requestContext.principal().getPrincipalType(),
+ requestContext.principal().getName());
+ }
+
+ private static AclBindingFilter createAclBindingFilter(ResourceType resourceType) {
+ ResourcePatternFilter resourcePatternFilter = new ResourcePatternFilter(resourceType, null, PatternType.ANY);
+ return new AclBindingFilter(resourcePatternFilter, AccessControlEntryFilter.ANY);
+ }
+
+ private static boolean isValidBinding(AclBinding binding, KafkaPrincipal principal, String hostAddr, AclOperation op) {
+ return (binding.entry().host().equals(hostAddr) || binding.entry().host().equals("*"))
+ && (SecurityUtils.parseKafkaPrincipal(binding.entry().principal()).equals(principal) || binding.entry().principal().equals("User:*"))
+ && (binding.entry().operation() == op || binding.entry().operation() == AclOperation.ALL);
Review Comment:
Since you did a good change, here I would also consider decrease cognitive load during read this part of code
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18130:
URL: https://github.com/apache/kafka/pull/18130#issuecomment-2550248664
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "Serwios (via GitHub)" <gi...@apache.org>.
Serwios commented on code in PR #18130:
URL: https://github.com/apache/kafka/pull/18130#discussion_r1878925772
##########
clients/src/main/java/org/apache/kafka/server/authorizer/DefaultResourceTypeBasedAuthorization.java:
##########
@@ -0,0 +1,144 @@
+package org.apache.kafka.server.authorizer;
+
+import org.apache.kafka.common.acl.*;
+import org.apache.kafka.common.errors.AuthorizationException;
+import org.apache.kafka.common.resource.PatternType;
+import org.apache.kafka.common.resource.ResourcePattern;
+import org.apache.kafka.common.resource.ResourcePatternFilter;
+import org.apache.kafka.common.resource.ResourceType;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.utils.SecurityUtils;
+
+import java.util.*;
+
+public class DefaultResourceTypeBasedAuthorization {
+ private DefaultResourceTypeBasedAuthorization() {}
+
+ public static AuthorizationResult authorize(
+ AuthorizableRequestContext requestContext,
+ AclOperation op,
+ ResourceType resourceType,
+ Authorizer authorizer
+ ) {
+ SecurityUtils.authorizeByResourceTypeCheckArgs(op, resourceType);
+
+ if (isSuperUserAuthorized(requestContext, op, resourceType, authorizer)) {
+ return AuthorizationResult.ALLOWED;
+ }
+
+ Map<PatternType, Set<String>> denyPatterns = initializePatternMap();
+ Map<PatternType, Set<String>> allowPatterns = initializePatternMap();
+
+ boolean hasWildcardAllow = false;
+ KafkaPrincipal principal = createKafkaPrincipal(requestContext);
+ String hostAddr = requestContext.clientAddress().getHostAddress();
+
+ for (AclBinding binding : authorizer.acls(createAclBindingFilter(resourceType))) {
+ if (!isValidBinding(binding, principal, hostAddr, op)) {
+ continue;
+ }
+
+ if (binding.entry().permissionType() == AclPermissionType.DENY) {
+ processDenyAcl(binding, denyPatterns);
+ continue;
+ }
+
+ if (binding.entry().permissionType() == AclPermissionType.ALLOW) {
+ processAllowAcl(binding, allowPatterns);
+ if (binding.pattern().name().equals(ResourcePattern.WILDCARD_RESOURCE)) {
+ hasWildcardAllow = true;
+ }
+ }
+ }
+
+ if (hasWildcardAllow) {
+ return AuthorizationResult.ALLOWED;
+ }
+
+ return evaluateAuthorizationResult(allowPatterns, denyPatterns);
+ }
+
+ private static boolean isSuperUserAuthorized(AuthorizableRequestContext requestContext, AclOperation op, ResourceType resourceType, Authorizer authorizer) {
+ return authorizer.authorize(requestContext, Collections.singletonList(new Action(
+ op, new ResourcePattern(resourceType, "hardcode", PatternType.LITERAL),
+ 0, true, false)))
+ .getFirst() == AuthorizationResult.ALLOWED;
+ }
+
+ private static Map<PatternType, Set<String>> initializePatternMap() {
+ return new EnumMap<>(PatternType.class) {{
+ put(PatternType.LITERAL, new HashSet<>());
+ put(PatternType.PREFIXED, new HashSet<>());
+ }};
+ }
+
+ private static KafkaPrincipal createKafkaPrincipal(AuthorizableRequestContext requestContext) {
+ return new KafkaPrincipal(
+ requestContext.principal().getPrincipalType(),
+ requestContext.principal().getName());
+ }
+
+ private static AclBindingFilter createAclBindingFilter(ResourceType resourceType) {
+ ResourcePatternFilter resourcePatternFilter = new ResourcePatternFilter(resourceType, null, PatternType.ANY);
+ return new AclBindingFilter(resourcePatternFilter, AccessControlEntryFilter.ANY);
+ }
+
+ private static boolean isValidBinding(AclBinding binding, KafkaPrincipal principal, String hostAddr, AclOperation op) {
+ return (binding.entry().host().equals(hostAddr) || binding.entry().host().equals("*"))
+ && (SecurityUtils.parseKafkaPrincipal(binding.entry().principal()).equals(principal) || binding.entry().principal().equals("User:*"))
+ && (binding.entry().operation() == op || binding.entry().operation() == AclOperation.ALL);
Review Comment:
Thx, simplified in latest commit
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "overpathz (via GitHub)" <gi...@apache.org>.
overpathz commented on code in PR #18130:
URL: https://github.com/apache/kafka/pull/18130#discussion_r1878884623
##########
clients/src/main/java/org/apache/kafka/server/authorizer/DefaultResourceTypeBasedAuthorization.java:
##########
@@ -0,0 +1,144 @@
+package org.apache.kafka.server.authorizer;
+
+import org.apache.kafka.common.acl.*;
+import org.apache.kafka.common.errors.AuthorizationException;
+import org.apache.kafka.common.resource.PatternType;
+import org.apache.kafka.common.resource.ResourcePattern;
+import org.apache.kafka.common.resource.ResourcePatternFilter;
+import org.apache.kafka.common.resource.ResourceType;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.utils.SecurityUtils;
+
+import java.util.*;
Review Comment:
I consider not to use wildcard import
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "overpathz (via GitHub)" <gi...@apache.org>.
overpathz commented on PR #18130:
URL: https://github.com/apache/kafka/pull/18130#issuecomment-2532939808
In general, LGTML. Very good for your first MINOR change.
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18130:
URL: https://github.com/apache/kafka/pull/18130#issuecomment-2735241446
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 leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.
<p>
If you are having difficulty finding a reviewer, please reach out on the
[mailing list](https://kafka.apache.org/contact).
<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] MINOR: Refactored default resource type based auth [kafka]
Posted by "Serwios (via GitHub)" <gi...@apache.org>.
Serwios commented on PR #18130:
URL: https://github.com/apache/kafka/pull/18130#issuecomment-2552211075
Update. I made it simpler: Everything is in interface, only one extracted method.
Regarding benchmark, on my laptop ETA is 8 hours XD, let me know if it's necessary @mumrah
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "overpathz (via GitHub)" <gi...@apache.org>.
overpathz commented on code in PR #18130:
URL: https://github.com/apache/kafka/pull/18130#discussion_r1878884623
##########
clients/src/main/java/org/apache/kafka/server/authorizer/DefaultResourceTypeBasedAuthorization.java:
##########
@@ -0,0 +1,144 @@
+package org.apache.kafka.server.authorizer;
+
+import org.apache.kafka.common.acl.*;
+import org.apache.kafka.common.errors.AuthorizationException;
+import org.apache.kafka.common.resource.PatternType;
+import org.apache.kafka.common.resource.ResourcePattern;
+import org.apache.kafka.common.resource.ResourcePatternFilter;
+import org.apache.kafka.common.resource.ResourceType;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.utils.SecurityUtils;
+
+import java.util.*;
Review Comment:
I would consider not to use wildcard import
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #18130: MINOR: Refactored default resource type based auth
URL: https://github.com/apache/kafka/pull/18130
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "Serwios (via GitHub)" <gi...@apache.org>.
Serwios commented on PR #18130:
URL: https://github.com/apache/kafka/pull/18130#issuecomment-2545577459
Hi, request for revbiew @AndrewJSchofield
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18130:
URL: https://github.com/apache/kafka/pull/18130#issuecomment-2814462967
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "Serwios (via GitHub)" <gi...@apache.org>.
Serwios commented on code in PR #18130:
URL: https://github.com/apache/kafka/pull/18130#discussion_r1878924442
##########
clients/src/main/java/org/apache/kafka/server/authorizer/DefaultResourceTypeBasedAuthorization.java:
##########
@@ -0,0 +1,144 @@
+package org.apache.kafka.server.authorizer;
+
+import org.apache.kafka.common.acl.*;
+import org.apache.kafka.common.errors.AuthorizationException;
+import org.apache.kafka.common.resource.PatternType;
+import org.apache.kafka.common.resource.ResourcePattern;
+import org.apache.kafka.common.resource.ResourcePatternFilter;
+import org.apache.kafka.common.resource.ResourceType;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.utils.SecurityUtils;
+
+import java.util.*;
Review Comment:
Changed to specific imports
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "Serwios (via GitHub)" <gi...@apache.org>.
Serwios commented on code in PR #18130:
URL: https://github.com/apache/kafka/pull/18130#discussion_r1878875515
##########
clients/src/main/java/org/apache/kafka/server/authorizer/Authorizer.java:
##########
@@ -182,114 +171,6 @@ default int aclCount() {
* given type. Return {@link AuthorizationResult#DENIED} otherwise.
*/
default AuthorizationResult authorizeByResourceType(AuthorizableRequestContext requestContext, AclOperation op, ResourceType resourceType) {
- SecurityUtils.authorizeByResourceTypeCheckArgs(op, resourceType);
-
- // Check a hard-coded name to ensure that super users are granted
Review Comment:
I removed comment's as thet seems to be redunant when code is self secriptive by separated methods
--
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] MINOR: Refactored default resource type based auth [kafka]
Posted by "Serwios (via GitHub)" <gi...@apache.org>.
Serwios commented on code in PR #18130:
URL: https://github.com/apache/kafka/pull/18130#discussion_r1878875515
##########
clients/src/main/java/org/apache/kafka/server/authorizer/Authorizer.java:
##########
@@ -182,114 +171,6 @@ default int aclCount() {
* given type. Return {@link AuthorizationResult#DENIED} otherwise.
*/
default AuthorizationResult authorizeByResourceType(AuthorizableRequestContext requestContext, AclOperation op, ResourceType resourceType) {
- SecurityUtils.authorizeByResourceTypeCheckArgs(op, resourceType);
-
- // Check a hard-coded name to ensure that super users are granted
Review Comment:
I removed comments as they seems to be redunant when code is self secriptive by separated methods
--
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