Hi Daniel, > If the auditor needs access to the details of the action, one could argue that even the response should be passed down to the auditor. At this point I don't think we need to include responses into the interface but if you have a use-case we can consider doing that.
> Is it feasible to convert the Java requests and responses to public API? Well I think that in this case we would need to actually transform a lot of classes and that might be a bit too invasive. Although since the protocol itself *is* a public API it might make sense to have some kind of Java representation as a public API as well. > If not, do we have another option to access this info in the auditor? I think one option would be to do what the original KIP-567 was implemented. Basically we could have an AuditEvent interface that would contain request specific data. Its obvious drawback is that it has to be implemented for most of the 40 something protocols but on the upside these classes shouldn't be complicated. I can try to do a PoC with this to see how it looks like and whether it solves the problem. To be honest I think it would be better than publishing the request classes as an API because here we're restricting access to only what is necessary. Thanks, Viktor On Fri, Sep 18, 2020 at 8:37 AM Dániel Urbán <urb.dani...@gmail.com> wrote: > Hi, > > Thanks for the KIP. > > If the auditor needs access to the details of the action, one could argue > that even the response should be passed down to the auditor. > Is it feasible to convert the Java requests and responses to public API? > If not, do we have another option to access this info in the auditor? > I know that the auditor could just send proper requests through the API to > the brokers, but that seems like an awful lot of overhead, and could > introduce timing issues as well. > > Daniel > > > Viktor Somogyi-Vass <viktorsomo...@gmail.com> ezt írta (időpont: 2020. > szept. 16., Sze, 17:17): > > > One more after-thought on your second point (AbstractRequest): the > reason I > > introduced it in the first place was that this way implementers can > access > > request data. A use case can be if they want to audit a change in > > configuration or client quotas but not just acknowledge the fact that > such > > an event happened but also capture the change itself by peeking into the > > request. Sometimes it can be useful especially when people want to trace > > back the order of events and what happened when and not just acknowledge > > that there was an event of a certain kind. I also recognize that this > might > > be a very loose interpretation of auditing as it's not strictly related > to > > authorization but rather a way of tracing the admin actions within the > > cluster. It even could be a different API therefore but because of the > > variety of the Kafka APIs it's very hard to give a method that fits all, > so > > it's easier to pass down the AbstractRequest and the implementation can > do > > the extraction of valuable info. So that's why I added this in the first > > place and I'm interested in your thoughts. > > > > On Wed, Sep 16, 2020 at 4:41 PM Viktor Somogyi-Vass < > > viktorsomo...@gmail.com> > > wrote: > > > > > Hi Mickael, > > > > > > Thanks for reviewing the KIP. > > > > > > 1.) I just wanted to follow the conventions used with the Authorizer as > > it > > > is built in a similar fashion, although it's true that in KafkaServer > we > > > call the configure() method and the start() in the next line. This > would > > be > > > the same in Auditor and even simpler as there aren't any parameters to > > > start(), so I can remove it. If it turns out there is a need for it, we > > can > > > add it later. > > > > > > 2.) Yes, this is a very good point, I will remove it, however in this > > case > > > I don't think we need to add the ApiKey as it is already available in > > > AuthorizableRequestContext.requestType(). One less parameter :). > > > > > > 3.) I'll add it. It will simply log important changes in the cluster > like > > > topic events (create, update, delete, partition or replication factor > > > change), ACL events, config changes, reassignment, altering log dirs, > > > offset delete, group delete with the authorization info like who > > initiated > > > the call, was it authorized, were there any errors. Let me know if you > > > think there are other APIs I should include. > > > > > > 4.) The builder is there mostly for easier usability but actually > > thinking > > > of it it doesn't help much so I removed it. The AuditInfo is also a > > helper > > > class so I don't see any value in transforming it into an interface but > > if > > > I simplify it (by removing the builder) it will be cleaner. Would that > > work? > > > > > > I'll update the KIP to reflect my answers. > > > > > > Viktor > > > > > > > > > On Mon, Sep 14, 2020 at 6:02 PM Mickael Maison < > mickael.mai...@gmail.com > > > > > > wrote: > > > > > >> Hi Viktor, > > >> > > >> Thanks for restarting the discussion on this KIP. Being able to easily > > >> audit usage of a Kafka cluster is a very valuable feature. > > >> > > >> Regarding the API, I have a few of questions: > > >> 1) You introduced a start() method. I don't think any other interfaces > > >> have such a method. Users can do any setup they want in configure() > > >> > > >> 2) The first argument of audit is an AbstractRequest. Unfortunately > > >> this type is not part of the public API. But actually I'm not sure > > >> having the full request is really needed here. Maybe just passing the > > >> Apikey would be enough as we already have all the resources from the > > >> auditInfos field. > > >> > > >> 3) The KIP mentions a "LoggingAuditor" default implementation. What is > > >> it doing? Can you add more details about it? > > >> > > >> 4) Can fields of AuditInfo be null? I can see there's a constructor > > >> without an Errors and that sets the error field to None. However, with > > >> the builder pattern, if error is not set it's null. > > >> > > >> 5) Should AuditInfo be an interface? > > >> > > >> On Mon, Sep 14, 2020 at 3:26 PM Viktor Somogyi-Vass > > >> <viktorsomo...@gmail.com> wrote: > > >> > > > >> > Hi everyone, > > >> > > > >> > Changed the interface a little bit to accommodate methods better > where > > >> > authorization happens for multiple operations so the implementer of > > the > > >> > audit interface will receive all authorizations together. > > >> > I'll wait a few more days to allow people to react or give feedback > > but > > >> if > > >> > there are no objections until then, I'll start a vote. > > >> > > > >> > Viktor > > >> > > > >> > On Tue, Sep 8, 2020 at 9:49 AM Viktor Somogyi-Vass < > > >> viktorsomo...@gmail.com> > > >> > wrote: > > >> > > > >> > > Hi Everyone, > > >> > > > > >> > > I'd like to restart the discussion on this. Since the KIP has been > > >> > > revamped I thought I'd start a new discussion thread. > > >> > > > > >> > > Link: > > >> > > > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-567%3A+Kafka+Cluster+Audit > > >> > > > > >> > > Short summary: > > >> > > - Would like to introduce a new interface similar to the > Authorizer > > >> called > > >> > > Auditor as follows: > > >> > > public interface Auditor { > > >> > > audit(Request r, AuthorizableRequestContext c, > AclOperation > > >> > > o, Map<ResourcePattern, Boolean> isAllowed, Map<ResourcePattern, > > >> Errors> > > >> > > errors); > > >> > > } > > >> > > - Basically it would pass down the request and the authorization > > >> > > information to the auditor implementation where various kind of > > >> reporting > > >> > > can be done based on the request. > > >> > > - A new config would be added called "auditor" which is similar to > > the > > >> > > "authorizer" config, but users can pass a list of auditor class > > names. > > >> > > - The implementation is expected to be low latency similarly to > the > > >> > > Authorizer. > > >> > > - A default implementation will be added that logs into a file. > > >> > > > > >> > > I appreciate any feedback on this. > > >> > > > > >> > > Best, > > >> > > Viktor > > >> > > > > >> > > > > > >
