On Mon, Aug 8, 2016 at 2:44 PM, Grant Henke <ghe...@cloudera.com> wrote:
> Thank you for the feedback everyone. Below I respond to the last batch of > emails: > > You mention that "delete" actions > > will get processed before "add" actions, which makes sense to me. An > > alternative to avoid the confusion in the first place would be to replace > > the AlterAcls APIs with separate AddAcls and DeleteAcls APIs. Was this > > option already rejected? > > > 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). > > > 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 had chosen to handle everything (Add/Delete) in Alter mainly for the > reason Ewen mentioned. I though it would be fairly common for a single > change to Add and Remove permissions at the same time. That said the idea > of having separate requests has not been discussed and is not out of the > question. More on this below. > > 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. > > > I agree that this is a bit of a nuance that could lead to rare but > confusing behavior. This is another chip in the "separate requests" > bucket. > > The more I think about the protocol and its most common usage in clients, > the more I lean towards separate requests. Clients are most likely to have > separate add and remove apis. In order to ensure correct/controlled > ordering and expected access they will likely fire separate requests > instead of batching. > > Though breaking the request into 2 requests causes more protocol messages, > its vastly simplifies the broker side implementation and makes the > expectations very clear. It also follows the Authorizer API much more > closely. > > I don't have any problem with breaking things into 2 requests if it's necessary or optimal. But can you explain why separate requests "vastly simplifies the broker side implementation"? It doesn't seem like it should be particularly complex to process each ACL change in order. -Ewen > 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. > > > ++1. > > > Do you have any specific thoughts on the approach you would like to see? I > may be missing the impact and how it affects the rest of the proposal. > > The current proposal suggests using a wide authorization requirement where > the principal must be authorized to the "All" Operation on the "Cluster" > resource to alter ACLs. I suggested this approach so we could maintain > backwards compatibility with existing Authorizer implementations while > still leaving the door open for a more fine grained approach later. If we > add new Operations or ResourceTypes the existing Authorizer implementations > will not expect them and could break. > > I think choosing to solve it now or later should have no bearing on the > approach or impact since the "All" Operation on the "Cluster" resource will > grant access to any model we choose. I could be missing something though. > > I had marked it in the follow up work section on the KIP-4 Wiki: > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > 4+-+Command+line+and+centralized+administrative+operations#KIP-4- > Commandlineandcentralizedadministrativeoperations-ACLAdminSchema > > > > 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. > > > They are public API. They are leveraged and exposed in the Request/Response > objects and likely the Admin clients. I can describe them a bit more in the > KIP. They are basically 1-1 mapping from the Authorizer interface. The only > reason they are being moved/duplicated is because they are needed by the > clients package and in Java. > > > > 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? > > > The problem w/ being more descriptive is that its possible that > > it restricts potential use cases if people think that somehow > > their use case wouldn't fit. > > > > > In the database world (Resource, ACL) pair is typically called a > > "grant". (sorry) > > > > You "grant" permission on a resource to a user. > > > "grant" has a nice sound to it...I can't figure it out but I like it a lot. > Happy to use that as the term for the resource & ACLs pair. I don't think > it will restrict any potential use cases. > > I didn't use the term since its not used in the Authorizer API methods > (addAcls, removeAcls) or existing docs. > > 5. What is the plan for when we add standard exceptions to the Authorizer > > interface? Will we bump the protocol version? > > > Currently I sort of "punt" on this issue. I catch any throwable and log it > server side before letting the generic error handling take over. If the > error is not mapped in Errors.java it results in a Unknown error code back > to the client. This means that when we decide to set standard/descriptive > exceptions (KIP-50?) they can be communicated without any server side > changes. > > An alternative is to wrap all exceptions in some generic > AuthorizerException but that doesn't feel much more descriptive or helpful. > > I could also add the current exceptions laid out in KIP-50 here > <https://cwiki.apache.org/confluence/display/KAFKA/KIP- > 50+-+Move+Authorizer+to+o.a.k.common+package#KIP-50-MoveAuthorizertoo.a.k. > commonpackage-AddexceptionsrelatedtoAuthorizer.>, > but they will not be thrown until new Authorizer implementations use them. > > Is there any benefit for sending the AlterAcls > > request to the controller? The controller is currently only designed for > > sending topic level metadata. > > > > Essentially I am calling the controller node the leader for ACL writes. The > Authorizer API exposes no details of delayed writes or consensus. If we > allow writes to go to any node, we will be crossing our fingers that any > Authorizer implementation handles this concurrency problem well today. Even > if the implementation is sound, directing the low volume writes to a single > node prevents delays due to retries and stale caches. > > As an example, "KAFKA-3328 > <https://issues.apache.org/jira/browse/KAFKA-3328>: SimpleAclAuthorizer > can > lose ACLs with frequent add/remove calls" would have been a much bigger > issue if requests could go to all nodes. Even after the fix, it could cause > delayed writes because of retries. > > Its not absolutely required, but I think its a good simplification. > > Thanks, > Grant > > On Fri, Jul 29, 2016 at 12:03 AM, Gwen Shapira <g...@confluent.io> wrote: > > > In the database world (Resource, ACL) pair is typically called a > > "grant". (sorry) > > > > You "grant" permission on a resource to a user. > > > > http://dev.mysql.com/doc/refman/5.7/en/show-grants.html > > > > Gwen > > > > > > > > > > On Fri, Jul 22, 2016 at 4:13 AM, Jim Jagielski <j...@jagunet.com> wrote: > > > > > >> On Jul 21, 2016, at 10: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. > > > > > > ++1. > > > > > >> 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? > > > > > > The problem w/ being more descriptive is that its possible that > > > it restricts potential use cases if people think that somehow > > > their use case wouldn't fit. > > > > > >> 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 > > >>> > > > > > > > > > -- > Grant Henke > Software Engineer | Cloudera > gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke > -- Thanks, Ewen
