You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Grant Henke <gh...@cloudera.com> on 2016/07/14 16:09:00 UTC

[DISCUSS] KIP-4 ACL Admin Schema

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
grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Jun Rao <ju...@confluent.io>.

Grant,

Thanks for the writeup. Is there any benefit for sending the AlterAcls
request to the controller? The controller is currently only designed for
sending topic level metadata.

Jun

On Thu, Jul 14, 2016 at 9:09 AM, Grant Henke <gh...@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
> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Ewen Cheslack-Postava <ew...@confluent.io>.

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 <is...@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 <gh...@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
> > grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
> >
>



-- 
Thanks,
Ewen

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Jun Rao <ju...@confluent.io>.

Grant,

For your second comment, when propagating the ZK changes to acl cache, we
will also update the latest ZK version. So, if ACL requests are not issued
too quickly, the conditional updates to ZK should always be successful in
one try.

One potential benefit of serving the ACL request from any broker is for
resource managers such as Mesos. Mesos DCOS exposes a service (e.g.)
endpoint to external hosts. An external request by default can only be
issued to the service endpoint, which routes the request to an arbitrary
broker. Enabling an external request to go to a particular broker directly
(e.g., controller) requires more work.

Thanks,

Jun

On Thu, Aug 18, 2016 at 9:46 AM, Grant Henke <gh...@cloudera.com> wrote:

> Thanks for the feedback. Below are some responses:
>
>
> > 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.
>
>
> You are right, it isn't super difficult to process ACL changes in order.
> The simplicity I was thinking about comes from not needing to group and
> re-order them like in the current implementation. It also removes the
> "action" enumerator which also simplifies things a bit. I am open to both
> ideas (single alter processing in order vs separate requests) as the trade
> offs aren't drastic.
>
> I am leaning towards separate requests for a few reasons:
>
>    - The Admin API will likely submit separate requests for delete and add
>    regardless
>       - I expect most admin apis will have a removeAcl/s and addAcl/s api
>       the fires a request immediately. I don't expect "batching" explicit
> or
>       implicit to be all that common.
>    - Independent concerns and capabilities
>       - Separating delete into its own request makes it easier to define
>       "delete all" type behavior.
>       - I think 2 simple requests may be easier to document and understand
>       by client developers than 1 more complicated one.
>    - Matches the Authorizer interface
>       - Separate requests matches authorizer interface more closely and
>       still allows for actions on collections of ACLs instead of one
> call per ACL
>       via Authorizer.addAcls(acls: Set[Acl], resource: Resource) and
>       Authorizer.removeAcls(acls: Set[Acl], resource: Resource)
>
> Hmm, even if all ACL requests are directed to the controller, concurrency
> > is only guaranteed on the same connection. For requests coming from
> > different connections, the order that they get processed on the broker is
> > non-deterministic.
>
>
> I was thinking less about making sure the exact order of requests is
> handled correctly, since each client will likely get a response between
> each request before sending another. Its more about ensuring local
> state/cache is accurate and that there is no way 2 nodes can have different
> ACLs which they think are correct. Good implementations will handle this,
> but may take a performance hit.
>
> Perhaps I am overthinking it and the SimpleAclAuthorizer is the only one
> that would be affected by this (and likely rarely because volume of ACL
> write requests should not be high). The SimpleAclAuthorizer is eventually
> consistent between instances. It writes optimistically with the cached zk
> node version while writing a complete list of ACLs (not just adding
> removing single nodes for acls). Below is a concrete demonstration of the
> impact I am thinking about with the SimpleAclAuthorizer:
>
> If we force all writes to go through one instance then follow up (ignoring
> first call to warm cache) writes for a resource would:
>
>    1. Call addAcls
>    2. Call updateResourceAcls combining the current cached acls and the new
>    acls
>    3. Write the result to Zookeeper via conditional write on the cached
>    version
>    4. Success 1 remote call
>
> If requests can go through any instance then follow up writes for a
> resource may:
>
>    1. Call addAcls
>    2. Call updateResourceAcls combining the current cached acls and the new
>    acls
>       1. If no cached acls read from Zookeeper
>    3. Write the result to Zookeeper via conditional write on the cached
>    version
>       1. If the cached version is wrong due to a write on another
>       instance read from Zookeeper
>       2. Rebuild the final ACLs list
>       3. Repeat until the write is successful
>    4. Success in 1 or 4 (or more) remote calls
>
> It looks like the Sentry implementation would not have this issue and the
> Ranger implementation doesn't support modifying ACLs anyway (must use the
> Ranger UI/API).
>
> I wanted to explain my original thoughts, but I am happy to remove the
> controller constraint given the SimpleAclAuthorizer appears to be the only
> (of those I know) implementation to be affected.
>
> Thank you,
> Grant
>
>
>
>
>
>
> On Sat, Aug 13, 2016 at 5:41 PM, Ewen Cheslack-Postava <ew...@confluent.io>
> wrote:
>
> > On Mon, Aug 8, 2016 at 2:44 PM, Grant Henke <gh...@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 <gw...@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 <ji...@jagunet.com>
> > wrote:
> > > > >
> > > > >> On Jul 21, 2016, at 10:57 PM, Ismael Juma <is...@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 <ghenke@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
> > > > >>> grant@cloudera.com | twitter.com/gchenke |
> > > linkedin.com/in/granthenke
> > > > >>>
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Grant Henke
> > > Software Engineer | Cloudera
> > > grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
> > >
> >
> >
> >
> > --
> > Thanks,
> > Ewen
> >
>
>
>
> --
> Grant Henke
> Software Engineer | Cloudera
> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Grant Henke <gh...@cloudera.com>.

Thanks for the feedback. Below are some responses:


> 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.


You are right, it isn't super difficult to process ACL changes in order.
The simplicity I was thinking about comes from not needing to group and
re-order them like in the current implementation. It also removes the
"action" enumerator which also simplifies things a bit. I am open to both
ideas (single alter processing in order vs separate requests) as the trade
offs aren't drastic.

I am leaning towards separate requests for a few reasons:

   - The Admin API will likely submit separate requests for delete and add
   regardless
      - I expect most admin apis will have a removeAcl/s and addAcl/s api
      the fires a request immediately. I don't expect "batching" explicit or
      implicit to be all that common.
   - Independent concerns and capabilities
      - Separating delete into its own request makes it easier to define
      "delete all" type behavior.
      - I think 2 simple requests may be easier to document and understand
      by client developers than 1 more complicated one.
   - Matches the Authorizer interface
      - Separate requests matches authorizer interface more closely and
      still allows for actions on collections of ACLs instead of one
call per ACL
      via Authorizer.addAcls(acls: Set[Acl], resource: Resource) and
      Authorizer.removeAcls(acls: Set[Acl], resource: Resource)

Hmm, even if all ACL requests are directed to the controller, concurrency
> is only guaranteed on the same connection. For requests coming from
> different connections, the order that they get processed on the broker is
> non-deterministic.


I was thinking less about making sure the exact order of requests is
handled correctly, since each client will likely get a response between
each request before sending another. Its more about ensuring local
state/cache is accurate and that there is no way 2 nodes can have different
ACLs which they think are correct. Good implementations will handle this,
but may take a performance hit.

Perhaps I am overthinking it and the SimpleAclAuthorizer is the only one
that would be affected by this (and likely rarely because volume of ACL
write requests should not be high). The SimpleAclAuthorizer is eventually
consistent between instances. It writes optimistically with the cached zk
node version while writing a complete list of ACLs (not just adding
removing single nodes for acls). Below is a concrete demonstration of the
impact I am thinking about with the SimpleAclAuthorizer:

If we force all writes to go through one instance then follow up (ignoring
first call to warm cache) writes for a resource would:

   1. Call addAcls
   2. Call updateResourceAcls combining the current cached acls and the new
   acls
   3. Write the result to Zookeeper via conditional write on the cached
   version
   4. Success 1 remote call

If requests can go through any instance then follow up writes for a
resource may:

   1. Call addAcls
   2. Call updateResourceAcls combining the current cached acls and the new
   acls
      1. If no cached acls read from Zookeeper
   3. Write the result to Zookeeper via conditional write on the cached
   version
      1. If the cached version is wrong due to a write on another
      instance read from Zookeeper
      2. Rebuild the final ACLs list
      3. Repeat until the write is successful
   4. Success in 1 or 4 (or more) remote calls

It looks like the Sentry implementation would not have this issue and the
Ranger implementation doesn't support modifying ACLs anyway (must use the
Ranger UI/API).

I wanted to explain my original thoughts, but I am happy to remove the
controller constraint given the SimpleAclAuthorizer appears to be the only
(of those I know) implementation to be affected.

Thank you,
Grant






On Sat, Aug 13, 2016 at 5:41 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:

> On Mon, Aug 8, 2016 at 2:44 PM, Grant Henke <gh...@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 <gw...@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 <ji...@jagunet.com>
> wrote:
> > > >
> > > >> On Jul 21, 2016, at 10:57 PM, Ismael Juma <is...@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 <gh...@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
> > > >>> grant@cloudera.com | twitter.com/gchenke |
> > linkedin.com/in/granthenke
> > > >>>
> > > >
> > >
> >
> >
> >
> > --
> > Grant Henke
> > Software Engineer | Cloudera
> > grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
> >
>
>
>
> --
> Thanks,
> Ewen
>



-- 
Grant Henke
Software Engineer | Cloudera
grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Ewen Cheslack-Postava <ew...@confluent.io>.

On Mon, Aug 8, 2016 at 2:44 PM, Grant Henke <gh...@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 <gw...@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 <ji...@jagunet.com> wrote:
> > >
> > >> On Jul 21, 2016, at 10:57 PM, Ismael Juma <is...@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 <gh...@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
> > >>> grant@cloudera.com | twitter.com/gchenke |
> linkedin.com/in/granthenke
> > >>>
> > >
> >
>
>
>
> --
> Grant Henke
> Software Engineer | Cloudera
> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>



-- 
Thanks,
Ewen

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Jun Rao <ju...@confluent.io>.

Grant,

Thanks for the reply. I had one inline reply below.

On Mon, Aug 8, 2016 at 2:44 PM, Grant Henke <gh...@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.
>
> 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.
>
>
Hmm, even if all ACL requests are directed to the controller, concurrency
is only guaranteed on the same connection. For requests coming from
different connections, the order that they get processed on the broker is
non-deterministic.



> Thanks,
> Grant
>
> On Fri, Jul 29, 2016 at 12:03 AM, Gwen Shapira <gw...@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 <ji...@jagunet.com> wrote:
> > >
> > >> On Jul 21, 2016, at 10:57 PM, Ismael Juma <is...@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 <gh...@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
> > >>> grant@cloudera.com | twitter.com/gchenke |
> linkedin.com/in/granthenke
> > >>>
> > >
> >
>
>
>
> --
> Grant Henke
> Software Engineer | Cloudera
> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Grant Henke <gh...@cloudera.com>.

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.

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 <gw...@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 <ji...@jagunet.com> wrote:
> >
> >> On Jul 21, 2016, at 10:57 PM, Ismael Juma <is...@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 <gh...@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
> >>> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
> >>>
> >
>



-- 
Grant Henke
Software Engineer | Cloudera
grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Gwen Shapira <gw...@confluent.io>.

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 <ji...@jagunet.com> wrote:
>
>> On Jul 21, 2016, at 10:57 PM, Ismael Juma <is...@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 <gh...@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
>>> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>>>
>

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Jim Jagielski <ji...@jaguNET.com>.

> On Jul 21, 2016, at 10:57 PM, Ismael Juma <is...@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 <gh...@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
>> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>>

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Ismael Juma <is...@juma.me.uk>.

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 <gh...@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
> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Jason Gustafson <ja...@confluent.io>.

Hi Grant,

This looks good to me. One minor comment. 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?

Thanks,
Jason

On Thu, Jul 21, 2016 at 7:57 AM, Grant Henke <gh...@cloudera.com> wrote:

> Anyone else have any feedback on this protocol and implementation? I plan
> to start a vote soon.
>
> Thank you,
> Grant
>
> On Fri, Jul 15, 2016 at 1:04 PM, Gwen Shapira <gw...@confluent.io> wrote:
>
> > > My goal in the protocol design was to keep the request simple and be
> able
> > > to answer what I think are the 3 most common questions/requests
> > >
> > >    - What ACLs are on the cluster?
> > >    - What access do I/they have?
> > >    - Who has access to this resource?
> >
> > Thanks for clarifying. I think this is good. Perhaps just document
> > this goal next to the protocol for the record :)
> >
> > > Isn't KIP-50 itself one gigantic compatibility concern? I don't see
> > >> how your suggestions make it any worse...
> > >
> > >
> > >
> > >>  Yes, I also think we should take this chance to improve the
> Authorizer
> > interface
> > >> to make it more suitable for the ACL Admin requests.
> > >
> > >
> > > I agree we can address this in KIP-50. What I was getting at was that I
> > > wanted to handle that discussion there. We voted on KIP-50 before 0.10
> > was
> > > released with the intention that we could get it in. Now that 0.10 is
> > > released and a longer time has gone by I am not sure if the opinion of
> > > "breaking is okay" has changed. I will always prefer a backward
> > compatible
> > > approach if possible.
> >
> > Well, the entire KIP-50 discussion - both regarding compatibility and
> > possible increased scope is probably out of context here. Especially
> > since this proposal was written carefully to avoid any assumptions
> > regarding other work. I suggest taking this in a separate thread.
> >
> > Gwen
> >
> > > Thank you,
> > > Grant
> > >
> > >
> > > On Fri, Jul 15, 2016 at 7:22 AM, Ismael Juma <is...@juma.me.uk>
> wrote:
> > >
> > >> On Fri, Jul 15, 2016 at 6:45 AM, Gwen Shapira <gw...@confluent.io>
> > wrote:
> > >> >
> > >> > >>          - I suggest this be addressed in KIP-50 as well, though
> it
> > >> has
> > >> > >>          some compatibility concerns.
> > >> >
> > >> > Isn't KIP-50 itself one gigantic compatibility concern? I don't see
> > >> > how your suggestions make it any worse...
> > >> >
> > >>
> > >> Yes, I also think we should take this chance to improve the Authorizer
> > >> interface to make it more suitable for the ACL Admin requests.
> > >>
> > >> Ismael
> > >>
> > >
> > >
> > >
> > > --
> > > Grant Henke
> > > Software Engineer | Cloudera
> > > grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
> >
>
>
>
> --
> Grant Henke
> Software Engineer | Cloudera
> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Grant Henke <gh...@cloudera.com>.

Anyone else have any feedback on this protocol and implementation? I plan
to start a vote soon.

Thank you,
Grant

On Fri, Jul 15, 2016 at 1:04 PM, Gwen Shapira <gw...@confluent.io> wrote:

> > My goal in the protocol design was to keep the request simple and be able
> > to answer what I think are the 3 most common questions/requests
> >
> >    - What ACLs are on the cluster?
> >    - What access do I/they have?
> >    - Who has access to this resource?
>
> Thanks for clarifying. I think this is good. Perhaps just document
> this goal next to the protocol for the record :)
>
> > Isn't KIP-50 itself one gigantic compatibility concern? I don't see
> >> how your suggestions make it any worse...
> >
> >
> >
> >>  Yes, I also think we should take this chance to improve the Authorizer
> interface
> >> to make it more suitable for the ACL Admin requests.
> >
> >
> > I agree we can address this in KIP-50. What I was getting at was that I
> > wanted to handle that discussion there. We voted on KIP-50 before 0.10
> was
> > released with the intention that we could get it in. Now that 0.10 is
> > released and a longer time has gone by I am not sure if the opinion of
> > "breaking is okay" has changed. I will always prefer a backward
> compatible
> > approach if possible.
>
> Well, the entire KIP-50 discussion - both regarding compatibility and
> possible increased scope is probably out of context here. Especially
> since this proposal was written carefully to avoid any assumptions
> regarding other work. I suggest taking this in a separate thread.
>
> Gwen
>
> > Thank you,
> > Grant
> >
> >
> > On Fri, Jul 15, 2016 at 7:22 AM, Ismael Juma <is...@juma.me.uk> wrote:
> >
> >> On Fri, Jul 15, 2016 at 6:45 AM, Gwen Shapira <gw...@confluent.io>
> wrote:
> >> >
> >> > >>          - I suggest this be addressed in KIP-50 as well, though it
> >> has
> >> > >>          some compatibility concerns.
> >> >
> >> > Isn't KIP-50 itself one gigantic compatibility concern? I don't see
> >> > how your suggestions make it any worse...
> >> >
> >>
> >> Yes, I also think we should take this chance to improve the Authorizer
> >> interface to make it more suitable for the ACL Admin requests.
> >>
> >> Ismael
> >>
> >
> >
> >
> > --
> > Grant Henke
> > Software Engineer | Cloudera
> > grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
>



-- 
Grant Henke
Software Engineer | Cloudera
grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Gwen Shapira <gw...@confluent.io>.

> My goal in the protocol design was to keep the request simple and be able
> to answer what I think are the 3 most common questions/requests
>
>    - What ACLs are on the cluster?
>    - What access do I/they have?
>    - Who has access to this resource?

Thanks for clarifying. I think this is good. Perhaps just document
this goal next to the protocol for the record :)

> Isn't KIP-50 itself one gigantic compatibility concern? I don't see
>> how your suggestions make it any worse...
>
>
>
>>  Yes, I also think we should take this chance to improve the Authorizer interface
>> to make it more suitable for the ACL Admin requests.
>
>
> I agree we can address this in KIP-50. What I was getting at was that I
> wanted to handle that discussion there. We voted on KIP-50 before 0.10 was
> released with the intention that we could get it in. Now that 0.10 is
> released and a longer time has gone by I am not sure if the opinion of
> "breaking is okay" has changed. I will always prefer a backward compatible
> approach if possible.

Well, the entire KIP-50 discussion - both regarding compatibility and
possible increased scope is probably out of context here. Especially
since this proposal was written carefully to avoid any assumptions
regarding other work. I suggest taking this in a separate thread.

Gwen

> Thank you,
> Grant
>
>
> On Fri, Jul 15, 2016 at 7:22 AM, Ismael Juma <is...@juma.me.uk> wrote:
>
>> On Fri, Jul 15, 2016 at 6:45 AM, Gwen Shapira <gw...@confluent.io> wrote:
>> >
>> > >>          - I suggest this be addressed in KIP-50 as well, though it
>> has
>> > >>          some compatibility concerns.
>> >
>> > Isn't KIP-50 itself one gigantic compatibility concern? I don't see
>> > how your suggestions make it any worse...
>> >
>>
>> Yes, I also think we should take this chance to improve the Authorizer
>> interface to make it more suitable for the ACL Admin requests.
>>
>> Ismael
>>
>
>
>
> --
> Grant Henke
> Software Engineer | Cloudera
> grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Grant Henke <gh...@cloudera.com>.

Thank you for the review Gwen, Manikumar, & Ismael. See my responses below:

I am a bit confused about specifying resources.
> resource_type is something like "TOPIC" and resource_name is a name of
> a specific topic?

This resource is a protocol representation of the existing Resource
<https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/security/auth/Resource.scala>
class. Currently there are 3 resource types: Cluster, Topic, and Group. The
resource name is statically "kafka-cluster" when using the type Cluster,
otherwise its the name of the resource. The topic name for a topic resource
and the consumer group for a group resource.

I have not changed or made anything new, only exposed it via the protocol.
My goal in KIP-4 is to focus on exposing what exists and then later
iterating on improvements in order to keep the scope down.

> Can you clarify a bit more about the use here? Can I have regexp?

I don't plan to support regex. I haven't seen a use/need for that feature
yet but we could add it later if its found useful.

Can I leave resource_name empty and have the ACLs for everything in a
> resource type?

I was not planning on supporting passing an empty resource name and getting
all resources for that type. Similar to regex, I haven't seen a use/need
for that feature yet but we could add it later if its found useful.

> Also, can you describe the interaction between principal and resource?
> I assume that if I specify both, I get all ACLs for a principal for
> the resources specified, but just making sure :)

I tried to cover this in the KIP wiki with the following

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.

It may not be the most clear and didn't really cover the case of both.
Essentially these fields act as filters.

   - If none are specified, all ACLs will be listed.
   - If a principle is specified, all ACLs for the principle will be listed.
   - If a resource is specified, all ACLs for the resource will be listed.
   - If both are specified, all ACLs for that resource & principle will be
   listed.

I will update wiki to make this more clear.

Can we allow ListAcls to take list of resources?  This may help when we
> have many associated
> resources under same principal.

My goal in the protocol design was to keep the request simple and be able
to answer what I think are the 3 most common questions/requests

   - What ACLs are on the cluster?
   - What access do I/they have?
   - Who has access to this resource?

We could have a more complicated request schema that allows you to get a
very precise result set back. However, I imagined in that case it would be
okay for some client side filtering or multiple requests.

If we offer the ability to list multiple resources, we may consider listing
multiple principles too. Essentially they would act as filters on the
returned ACLs.

Do others think taking a list of principles and resources is valuable and
worth the extra request complexity? Do you have any good examples of how it
would be used?

Isn't KIP-50 itself one gigantic compatibility concern? I don't see
> how your suggestions make it any worse...

>  Yes, I also think we should take this chance to improve the Authorizer interface
> to make it more suitable for the ACL Admin requests.

I agree we can address this in KIP-50. What I was getting at was that I
wanted to handle that discussion there. We voted on KIP-50 before 0.10 was
released with the intention that we could get it in. Now that 0.10 is
released and a longer time has gone by I am not sure if the opinion of
"breaking is okay" has changed. I will always prefer a backward compatible
approach if possible.

Thank you,
Grant

On Fri, Jul 15, 2016 at 7:22 AM, Ismael Juma <is...@juma.me.uk> wrote:

> On Fri, Jul 15, 2016 at 6:45 AM, Gwen Shapira <gw...@confluent.io> wrote:
> >
> > >>          - I suggest this be addressed in KIP-50 as well, though it
> has
> > >>          some compatibility concerns.
> >
> > Isn't KIP-50 itself one gigantic compatibility concern? I don't see
> > how your suggestions make it any worse...
> >
>
> Yes, I also think we should take this chance to improve the Authorizer
> interface to make it more suitable for the ACL Admin requests.
>
> Ismael
>

-- 
Grant Henke
Software Engineer | Cloudera
grant@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Ismael Juma <is...@juma.me.uk>.

On Fri, Jul 15, 2016 at 6:45 AM, Gwen Shapira <gw...@confluent.io> wrote:
>
> >>          - I suggest this be addressed in KIP-50 as well, though it has
> >>          some compatibility concerns.
>
> Isn't KIP-50 itself one gigantic compatibility concern? I don't see
> how your suggestions make it any worse...
>

Yes, I also think we should take this chance to improve the Authorizer
interface to make it more suitable for the ACL Admin requests.

Ismael

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Manikumar Reddy <ma...@gmail.com>.

Hi,

Can we allow ListAcls to take list of resources?  This may help when we
have many associated
resources under same principal.


Thanks
Manikumar

On Fri, Jul 15, 2016 at 11:15 AM, Gwen Shapira <gw...@confluent.io> wrote:

> Thank you, Grant. This is lovely :)
>
> Few comments / requests for clarifications below:
>
>
> >> ListAcls Request (Version: 0) => principal resource
> >>   principal => NULLABLE_STRING
> >>   resource => resource_type resource_name
> >>     resource_type => INT8
> >>     resource_name => STRING
>
> I am a bit confused about specifying resources.
> resource_type is something like "TOPIC" and resource_name is a name of
> a specific topic?
> Can you clarify a bit more about the use here? Can I have regexp? Can
> I leave resource_name empty and have the ACLs for everything in a
> resource type?
> Also, can you describe the interaction between principal and resource?
> I assume that if I specify both, I get all ACLs for a principal for
> the resources specified, but just making sure :)
>
>
> >> Alter ACLs Request
> >>
> >>    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.
>
> I like this decision
>
> >>          - I suggest this be addressed in KIP-50 as well, though it has
> >>          some compatibility concerns.
>
> Isn't KIP-50 itself one gigantic compatibility concern? I don't see
> how your suggestions make it any worse...
>

Re: [DISCUSS] KIP-4 ACL Admin Schema

Posted by Gwen Shapira <gw...@confluent.io>.

Thank you, Grant. This is lovely :)

Few comments / requests for clarifications below:


>> ListAcls Request (Version: 0) => principal resource
>>   principal => NULLABLE_STRING
>>   resource => resource_type resource_name
>>     resource_type => INT8
>>     resource_name => STRING

I am a bit confused about specifying resources.
resource_type is something like "TOPIC" and resource_name is a name of
a specific topic?
Can you clarify a bit more about the use here? Can I have regexp? Can
I leave resource_name empty and have the ACLs for everything in a
resource type?
Also, can you describe the interaction between principal and resource?
I assume that if I specify both, I get all ACLs for a principal for
the resources specified, but just making sure :)


>> Alter ACLs Request
>>
>>    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.

I like this decision

>>          - I suggest this be addressed in KIP-50 as well, though it has
>>          some compatibility concerns.

Isn't KIP-50 itself one gigantic compatibility concern? I don't see
how your suggestions make it any worse...