KAFKA-6591: Move super user check before ACL matching #4618

soenkeliebau · 2018-02-25T22:12:49Z

Currently the check whether a user as a super user in SimpleAclAuthorizer is performed only after all other ACLs have been evaluated. Since all requests from a super user are granted we don't really need to apply the ACLs.

This commit returns true if the user is a super user before checking ACLs, thus bypassing the needless evaluation effort.

soenkeliebau · 2018-02-27T03:41:16Z

retest this please

omkreddy · 2018-03-23T17:30:08Z

core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala

@@ -106,6 +106,12 @@ class SimpleAclAuthorizer extends Authorizer with Logging {
    val host = session.clientAddress.getHostAddress
    val acls = getAcls(resource) ++ getAcls(new Resource(resource.resourceType, Resource.WildCardResource))

+    // Check if the user is a super user to avoid needlessly checking all ACLs
+    // for super users
+    if (isSuperUser(operation, resource, principal, host)) {


nit: Can we do this check before initializing acls?

omkreddy · 2018-03-23T17:33:47Z

core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala

    //when no acls are found or if no deny acls are found and at least one allow acls matches.
-    val authorized = isSuperUser(operation, resource, principal, host) ||
-      isEmptyAclAndAuthorized(operation, resource, principal, host, acls) ||
+    val authorized = isEmptyAclAndAuthorized(operation, resource, principal, host, acls) ||
      (!denyMatch && allowMatch)

    logAuditMessage(principal, authorized, operation, resource, host)


looks like we miss audit messages for super users. we need to call logAuditMessage in above check.

soenkeliebau · 2018-03-26T21:12:46Z

Thanks for reviewing @omkreddy . You are right, I did miss the log statement. have refactored so that the same return statement gets used for all cases to avoid duplicating the log statement.

hachikuji

Thanks, seems reasonable. Left a couple minor comments.

hachikuji · 2018-03-27T20:31:48Z

core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala

- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
+  * Licensed to the Apache Software Foundation (ASF) under one or more


Is this change intentional?

Absolutely not. I just moved to a new laptop and apparently Intellij went back to ignoring whitespace changes in diffs again. I'll revert that.

hachikuji · 2018-03-27T20:49:09Z

core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala

+      val allowMatch = allowOps.exists(operation => aclMatch(operation, resource, principal, host, Allow, acls))
+
+      //we allow an operation if no acls are found and user has configured to allow all users
+      //when no acls are found or if no deny acls are found and at least one allow acls matches.
      isEmptyAclAndAuthorized(operation, resource, principal, host, acls) ||


Would it make sense to move the empty check above the ACL matching since we don't really care about the results anyway in that case?

Makes sense, I'll amend the PR.

Actually after doing some more thinking and looking at the code it may make sense to leave the check as is.

The log message that is currently being displayed is:

authorizerLogger.debug(s"No acl found for resource $resource, authorized = $shouldAllowEveryoneIfNoAclIsFound")

Which would not be factually correct any more if we made the switch, as we never looked at ACLs, so there may very well be ACLs as well that would permit the operation when this message is shown.

We could reword the log message, but I'd argue that the information that no ACLs matched can be useful when securing an already busy cluster to find clients that were missed in the migration (and probably other scenarios as well).

The overhead is also reasonable I think, we need to load the ACLs anyway, otherwise we can't perform the check and then its just two .exists checks against an empty collection that need to be performed (once for Deny matches once for Allow matches).

Update:
Something kept nagging me about this and I reread the code. I missed the .isempty check in isEmptyAclAndAuthorized which would suppress the log message in this case. Hence it does make sense to move the check.

Yeah I didn't see the harm since we would still check isEmptyAclAndAuthorized in any case. Not that big of a deal though.

I've created a commit that addresses this comment as well.. I'm not sure that I like it much better than the code that is currently there though..we can't simply add the check as an "if else" block, since we need to retrieve the ACLs after the superuser check, so it becomes a nested if..
Also this created "dead" code in isEmptyAclAndAuthorized so I inlined that function as it was extremely short and I suspect it was only initially created to keep the conditional statement readable.

I've pushed to an alternative branch so I don't have to revert here if we decide against it. Feel free to have a look and let me know what you think.

@hachikuji Did you have a chance to have a look at the proposed alternative and have any thoughts on whether we want to use that or keep the PR as is?

@soenkeliebau The alternative looks good to me.

soenkeliebau · 2018-04-25T10:56:42Z

@hachikuji I've merged the alternative with this PR. The test failure is unrelated I believe:

09:21:00 ERROR: Step ?Publish coverage to GitHub? aborted due to exception: 09:21:00 java.io.IOException: Exceeded rate limit for repository

so won't request a retest if we exceed a rate limit anyway..

soenkeliebau · 2018-06-10T18:57:46Z

retest this please

…necessary effort when evaluating requests from super users.

soenkeliebau · 2018-06-20T09:19:34Z

I've rebased on trunk to accomodate changes from KAFKA-6841
@big-andy-coates could I trouble you to have a quick look I correctly adjusted to match the new way of loading ACLs?

(I spotted a typo in one comment, will abstain from fixing that for now and do that later if changes should become necessary)

big-andy-coates

@soenkeliebau LGTM, few suggestions to improve things even more. This is a improvement I noticed when introducing ACLs on prefixed resource patterns, but didn't want to muddy the PR with such changes - great that you picked this up!

big-andy-coates · 2018-06-21T08:06:44Z

core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala

+      } else {
+        // ACLs found for this resource
+        // Check if there is any Deny acl match that would disallow this operation.
+        val denyMatch = aclMatch(operation, resource, principal, host, Deny, acls)


May as well not check for allow acls if there are deny acls, right?

Addressed in the refactored version of the code.

big-andy-coates · 2018-06-21T08:07:21Z

core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala

+      // for superusers
+      true
+    } else {
+      // User is no superuser, load ACLs


nit: User is not superuser.

I'm not a native speaker, so probably on very thin ice here, but to my mind's ear either "no superuser" or "not a superuser" sound right.

Then again, the discussion is mostly academic, as the comment has been removed in the refactored version anyway :)

big-andy-coates · 2018-06-21T08:09:20Z

core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala

+      // Check if the user is a superuser to avoid needlessly checking all ACLs
+      // for superusers
+      true
+    } else {


consider moving the 'else' into a separate method, then this becomes, (I don't think any comments are required on such obvious logic):

val authorized = isSuperUser(...) || aclsAllowAccess(...)

Following the same pattern, aclsAllowAccess could be:

val acls = getMatchingAcls(resource.resourceType, resource.name) val authorized = isEmptyAclAndAuthorized(...) || (!denyAclsExist(acls, ...) && alllowAclsExist(acls, ...))

This breaking out into different methods will likely lead to move readable code and less reliance on comments to explain what's happening. These methods can be local to authorize, which I think ends up with something like:

override def authorize(session: Session, operation: Operation, resource: Resource): Boolean = { if (resource.patternType != PatternType.LITERAL) { throw new IllegalArgumentException("Only literal resources are supported. Got: " + resource.patternType) } val principal = session.principal val host = session.clientAddress.getHostAddress def isEmptyAclAndAuthorized(acls: Set[Acl]): Boolean = { if (acls.isEmpty) { authorizerLogger.debug(s"No acl found for resource $resource, authorized = $shouldAllowEveryoneIfNoAclIsFound") shouldAllowEveryoneIfNoAclIsFound } else false } def denyAclsExist(acls: Set[Acl]) = { aclMatch(operation, resource, principal, host, Deny, acls) } def allowAclsExist(acls: Set[Acl]) = { // Check if there are any Allow ACLs which would allow this operation. // Allowing read, write, delete, or alter implies allowing describe. // See #{org.apache.kafka.common.acl.AclOperation} for more details about ACL inheritance. val allowOps = operation match { case Describe => Set[Operation](Describe, Read, Write, Delete, Alter) case DescribeConfigs => Set[Operation](DescribeConfigs, AlterConfigs) case _ => Set[Operation](operation) } allowOps.exists(operation => aclMatch(operation, resource, principal, host, Allow, acls)) } def aclsAllowAccess = { val acls = getMatchingAcls(resource.resourceType, resource.name) isEmptyAclAndAuthorized(acls) || (!denyAclsExist(acls) && allowAclsExist(acls)) } val authorized = isSuperUser(operation, resource, principal, host) || aclsAllowAccess logAuditMessage(principal, authorized, operation, resource, host) authorized }

Though you'll want to check my quick hacking about!

Also, you might want to check a suitable unit test exists that calls authorize for a super user, where deny acls exist. (which should allow access).

Your hacking was pretty much spot on, I only took the liberty to very slightly rename the *AclsExist methods.

testSuperUserHasAccess addresses the scenario where a super user still has access even though there is a Deny Acl in place.

big-andy-coates · 2018-06-21T08:15:45Z

core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala

+      true
+    } else {
+      // User is no superuser, load ACLs
+      // To avoid unnecessarily loading ACLs for super users we need to nest this as a second if


nit: there is no 'loading' of ACLs done in getMatchingAcls: ACLs are evaluated only. They are loaded in 'configure()'

"loading" was probably a poor choice of words here, yes. What I was referring to here was the retrieval of matching ACLs from the Acl-Map.
Comment has become obsolete though.

soenkeliebau · 2018-06-22T13:44:02Z

@big-andy-coates thanks for the review! I've incorporated your comments.

Test failure is unrelated I believe.

@hachikuji can you take a look as well and merge if happy with the changes?

soenkeliebau · 2018-06-25T13:01:40Z

retest this please

big-andy-coates

LGTM @soenkeliebau. Thanks for making the changes!

cc: @cmccabe

hachikuji

Thanks, LGTM

Currently the check whether a user as a super user in `SimpleAclAuthorizer` is performed only after all other ACLs have been evaluated. Since all requests from a super user are granted we don't really need to apply the ACLs. This patch returns true if the user is a super user before checking ACLs, thus bypassing the needless evaluation effort. Reviewers: Andy Coates <big-andy-coates@users.noreply.github.com>, Manikumar Reddy <manikumar.reddy@gmail.com>, Jason Gustafson <jason@confluent.io>

ewencp added the core Kafka Broker label Feb 28, 2018

omkreddy reviewed Mar 23, 2018

View reviewed changes

hachikuji reviewed Mar 27, 2018

View reviewed changes

KAFKA-6591: Moved super user check in before ACL matching to avoid un…

52a946e

…necessary effort when evaluating requests from super users.

soenkeliebau force-pushed the KAFKA-6591 branch from 6a21561 to 52a946e Compare June 19, 2018 15:56

Forgot to commit change to load prefixed ACLs.

d54103f

big-andy-coates reviewed Jun 21, 2018

View reviewed changes

Addressed comments from Andy.

c819606

big-andy-coates approved these changes Jun 25, 2018

View reviewed changes

hachikuji approved these changes Jun 25, 2018

View reviewed changes

hachikuji merged commit 3cdc78e into apache:trunk Jun 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-6591: Move super user check before ACL matching #4618

KAFKA-6591: Move super user check before ACL matching #4618

soenkeliebau commented Feb 25, 2018

soenkeliebau commented Feb 27, 2018

omkreddy Mar 23, 2018

omkreddy Mar 23, 2018

soenkeliebau commented Mar 26, 2018

hachikuji left a comment

hachikuji Mar 27, 2018

soenkeliebau Mar 28, 2018 •

edited

Loading

hachikuji Mar 27, 2018

soenkeliebau Mar 28, 2018

soenkeliebau Apr 5, 2018 •

edited

Loading

hachikuji Apr 5, 2018

soenkeliebau Apr 5, 2018 •

edited

Loading

soenkeliebau Apr 20, 2018

hachikuji Apr 20, 2018

soenkeliebau commented Apr 25, 2018

soenkeliebau commented Jun 10, 2018

soenkeliebau commented Jun 20, 2018

big-andy-coates left a comment

big-andy-coates Jun 21, 2018

soenkeliebau Jun 22, 2018

big-andy-coates Jun 21, 2018

soenkeliebau Jun 22, 2018

big-andy-coates Jun 21, 2018

soenkeliebau Jun 22, 2018 •

edited

Loading

big-andy-coates Jun 21, 2018

soenkeliebau Jun 22, 2018

soenkeliebau commented Jun 22, 2018

soenkeliebau commented Jun 25, 2018

big-andy-coates left a comment •

edited

Loading

hachikuji left a comment

KAFKA-6591: Move super user check before ACL matching #4618

KAFKA-6591: Move super user check before ACL matching #4618

Conversation

soenkeliebau commented Feb 25, 2018

soenkeliebau commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soenkeliebau commented Mar 26, 2018

hachikuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soenkeliebau Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soenkeliebau Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soenkeliebau Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soenkeliebau commented Apr 25, 2018

soenkeliebau commented Jun 10, 2018

soenkeliebau commented Jun 20, 2018

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soenkeliebau Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soenkeliebau commented Jun 22, 2018

soenkeliebau commented Jun 25, 2018

big-andy-coates left a comment • edited Loading

Choose a reason for hiding this comment

hachikuji left a comment

Choose a reason for hiding this comment

soenkeliebau Mar 28, 2018 •

edited

Loading

soenkeliebau Apr 5, 2018 •

edited

Loading

soenkeliebau Apr 5, 2018 •

edited

Loading

soenkeliebau Jun 22, 2018 •

edited

Loading

big-andy-coates left a comment •

edited

Loading