Re: 4 and Create/Delete vs Alter, I'm a fan of being able to bundle a bunch of changes in one request. Seems like an ACL change could easily include additions + deletions and is nice to bundle in one request that can be processed as quickly as possible. I don't think its a requirement, but the usage pattern for changing ACLs does seem like its different from how you manage topics where you probably usually are either only creating or only deleting topics.
I'm a bit concerned about the ordering -- the KIP mentions processing deletes before adds. Isn't that likely to lead to gaps in security? For example, lets say I have a very simple 1 rule ACL. To change it, I have to delete it and add a new one. If we do the delete first, don't we end up with an (albeit small) window where the resource ends up unprotected? I would think processing them in the order they are requested gives the operator the control they need to correctly protect resources. Reordering the processing of requests is also just unintuitive to me and I think probably unintuitive to users as well. -Ewen On Thu, Jul 21, 2016 at 7:57 PM, Ismael Juma <ism...@juma.me.uk> wrote: > Hi Grant, > > Thanks for the KIP. A few questions and comments: > > 1. My main concern is that we are skipping the discussion on the desired > model for controlling ACL access and updates. I understand the desire to > reduce the scope, but this seems to be a fundamental aspect of the design > that we need to get right. Without a plan for that, it is difficult to > evaluate if that part of the current proposal is fine. > 2. Are the Java objects in "org.apache.kafka.common.security.auth" going to > be public API? If so, we should explain why they should be public and > describe them in the KIP. If not, we should mention that. > 3. It would be nice to have a name for a (Resource, ACL) pair. The current > protocol uses `requests`/`responses` for the list of such pairs, but it > would be nice to have something more descriptive, if possible. Any ideas? > 4. There is no CreateAcls or DeleteAcls (unlike CreateTopics and > DeleteTopics, for example). It would be good to explain the reasoning for > this choice (Jason also asked this question). > 5. What is the plan for when we add standard exceptions to the Authorizer > interface? Will we bump the protocol version? > > Thanks, > Ismael > > On Thu, Jul 14, 2016 at 5:09 PM, Grant Henke <ghe...@cloudera.com> wrote: > > > The KIP-4 Delete Topic Schema vote has passed and the patch > > <https://github.com/apache/kafka/pull/1616> is available for review. > Now I > > would like to start the discussion for the Acls request/response and > server > > side implementations. This includes the ListAclsRequest/Response and the > > AlterAclsRequest/Response. > > > > Details for this implementation can be read here: > > * > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-4+-+Command+line+and+centralized+administrative+operations#KIP-4-Commandlineandcentralizedadministrativeoperations-ACLAdminSchema(KAFKA-3266) > > < > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-4+-+Command+line+and+centralized+administrative+operations#KIP-4-Commandlineandcentralizedadministrativeoperations-ACLAdminSchema(KAFKA-3266) > > >* > > > > I have included the exact content below for clarity: > > > > > ACL Admin Schema (KAFKA-3266 > > > <https://issues.apache.org/jira/browse/KAFKA-3266>) > > > > > > *Note*: Some of this work/code overlaps with "KIP-50 - Move Authorizer > to > > > o.a.k.common package > > > < > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-50+-+Move+Authorizer+to+o.a.k.common+package > > >". > > > KIP-4 does not change the Authorizer interface at all, but does provide > > > java objects in "org.apache.kafka.common.security.auth" to be used in > the > > > protocol request/response classes. It also provides translations > between > > > the Java and Scala versions for server side compatibility with > > > the Authorizer interface. > > > > > > List ACLs Request > > > > > > > > > > > > ListAcls Request (Version: 0) => principal resource > > > principal => NULLABLE_STRING > > > resource => resource_type resource_name > > > resource_type => INT8 > > > resource_name => STRING > > > > > > Request semantics: > > > > > > 1. Can be sent to any broker > > > 2. If a non-null principal is provided the returned ACLs will be > > > filtered by that principle, otherwise ACLs for all principals will > be > > > listed. > > > 3. If a resource with a resource_type != -1 is provided ACLs will be > > > filtered by that resource, otherwise ACLs for all resources will be > > listed. > > > 4. Any principle can list their own ACLs where the permission type > is > > > "Allow", Otherwise the principle must be authorized to the "All" > > Operation > > > on the "Cluster" resource to list ACLs. > > > - Unauthorized requests will receive a ClusterAuthorizationException > > > - This avoids adding a new operation that an existing authorizer > > > implementation may not be aware of. > > > - This can be reviewed and further refined/restricted as a follow > > > up ACLs review after this KIP. See Follow Up Changes > > > < > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-4+-+Command+line+and+centralized+administrative+operations#KIP-4-Commandlineandcentralizedadministrativeoperations-follow-up-changes > > > > > > . > > > 5. Requesting a resource or principle that does not have any ACLs > will > > > not result in an error, instead empty response list is returned > > > > > > List ACLs Response > > > > > > > > > > > > ListAcls Response (Version: 0) => [responses] error_code > > > responses => resource [acls] > > > resource => resource_type resource_name > > > resource_type => INT8 > > > resource_name => STRING > > > acls => acl_principle acl_permission_type acl_host acl_operation > > > acl_principle => STRING > > > acl_permission_type => INT8 > > > acl_host => STRING > > > acl_operation => INT8 > > > error_code => INT16 > > > > > > Alter ACLs Request > > > > > > > > > > > > AlterAcls Request (Version: 0) => [requests] > > > requests => resource [actions] > > > resource => resource_type resource_name > > > resource_type => INT8 > > > resource_name => STRING > > > actions => action acl > > > action => INT8 > > > acl => acl_principle acl_permission_type acl_host acl_operation > > > acl_principle => STRING > > > acl_permission_type => INT8 > > > acl_host => STRING > > > acl_operation => INT8 > > > > > > Request semantics: > > > > > > 1. Must be sent to the controller broker > > > 2. If there are multiple instructions for the same resource in one > > > request an InvalidRequestException will be logged on the broker and > a > > > single error code for that resource will be returned to the client > > > - This is because the list of requests is modeled server side as > a > > > map with resource as the key > > > 3. ACLs with a delete action will be processed first and the add > > > action second. > > > 1. This is to prevent confusion about sort order and final state > when > > > a batch message is sent. > > > 2. If an add request was processed first, it could be deleted > right > > > after. > > > 3. Grouping ACLs by their action allows batching requests to the > > > authorizer via the Authorizer.addAcls and Authorizer.removeAcls > > calls. > > > 4. The request is not transactional. One failure wont stop others > from > > > running. > > > 1. If an error occurs on one action, the others could still be > run. > > > 2. Errors are reported independently. > > > 5. The principle must be authorized to the "All" Operation on the > > > "Cluster" resource to alter ACLs. > > > - Unauthorized requests will receive a > > ClusterAuthorizationException > > > - This avoids adding a new operation that an existing authorizer > > > implementation may not be aware of. > > > - This can be reviewed and further refined/restricted as a follow > > > up ACLs review after this KIP. See Follow Up Changes > > > < > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-4+-+Command+line+and+centralized+administrative+operations#KIP-4-Commandlineandcentralizedadministrativeoperations-follow-up-changes > > > > > > . > > > > > > QA: > > > > > > - Why doesn't this request have a timeout and implement any blocking > > > like the CreateTopicsRequest? > > > - The Authorizer implementation is synchronous and exposes no > > > details about propagating the ACLs to other nodes. > > > - The best we can do in the existing implementation is > > > call Authorizer.addAcls and Authorizer.removeAcls and hope the > > underlying > > > implementation handles the rest. > > > - What happens if there is an error in the Authorizer? > > > - Currently the best we can do is log the error broker side and > > > return a generic exception because there are no "standard" > > exceptions > > > defined in the Authorizer interface to provide a more clear code > > > - KIP-50 > > > < > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-50+-+Move+Authorizer+to+o.a.k.common+package#KIP-50-MoveAuthorizertoo.a.k.commonpackage-AddexceptionsrelatedtoAuthorizer > .> > > is > > > tracking adding the standard exceptions > > > - The Authorizer interface also provides no feedback about > > > individual ACLs when added or deleted in a group > > > - Authorizer.addAcls is a void function, the best we can do is > > > return an error for all ACLs and let the user check the > current > > state by > > > listing the ACLs > > > - Autohrizer.removeAcls is a boolean function, the best we > can > > > do is return an error for all ACLs and let the user check the > > current state > > > by listing the ACLs > > > - Behavior here could vary drastically between implementations > > > - I suggest this be addressed in KIP-50 as well, though it has > > > some compatibility concerns. > > > - Why require the request to go to the controller? > > > - The controller is responsible for the cluster metadata and its > > > propagation > > > - This ensures one instance of the Authorizer sees all the > changes > > > and reduces concurrency issues, especially because the Authorizer > > interface > > > exposes no details about propagating the ACLs to other nodes. > > > - See Request Forwarding > > > < > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-4+-+Command+line+and+centralized+administrative+operations#KIP-4-Commandlineandcentralizedadministrativeoperations-request > > > > > > below > > > > > > Alter ACLs Response > > > > > > > > > > > > AlterAcls Response (Version: 0) => [responses] > > > responses => resource [results] > > > resource => resource_type resource_name > > > resource_type => INT8 > > > resource_name => STRING > > > results => action acl error_code > > > action => INT8 > > > acl => acl_principle acl_permission_type acl_host acl_operation > > > acl_principle => STRING > > > acl_permission_type => INT8 > > > acl_host => STRING > > > acl_operation => INT8 > > > error_code => INT16 > > > > > > > > > > > > > Thank you, > > Grant > > -- > > Grant Henke > > Software Engineer | Cloudera > > gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke > > > -- Thanks, Ewen
