Ok, thanks for the feedback, Todd. I have updated the KIP with some of the points discussed here. There is more to add based on these last comments, though.
-Flavio > On 21 Oct 2015, at 23:43, Todd Palino <tpal...@gmail.com> wrote: > > On Wed, Oct 21, 2015 at 3:38 PM, Flavio Junqueira <f...@apache.org > <mailto:f...@apache.org>> wrote: > >> >>> On 21 Oct 2015, at 21:54, Todd Palino <tpal...@gmail.com> wrote: >>> >>> Thanks for the clarification on that, Jun. Obviously, we haven't been >> doing >>> much with ZK authentication around here yet. There is still a small >> concern >>> there, mostly in that you should not share credentials any more than is >>> necessary, which would argue for being able to use a different ACL than >> the >>> default. I don't really like the idea of having to use the exact same >>> credentials for executing the admin tools as we do for running the >> brokers. >>> Given that we don't need to share the credentials with all consumers, I >>> think we can work around it. >>> >> >> Let me add that a feature to separate the sub-trees of users sharing an >> ensemble is chroot. >> >> On different credentials for admin tools, this sounds doable by setting >> the ACLs of znodes. For example, there could be an admin id and a broker >> id, both with the ability of changing znodes, but different credentials. >> Would something like that work for you? >> > > It would be a nice option to have, as the credentials can be protected > differently. I would consider this a nice to have, and not an "absolutely > must have" feature at this point. > > >> This does bring up another good question, however. What will be the >> process >>> for having to rotate the credentials? That is, if the credentials are >>> compromised and need to be changed, how can that be accomplished with the >>> cluster online. I'm guessing some combination of using skipAcl on the >>> Zookeeper ensemble and config changes to the brokers will be required, >> but >>> this is an important enough operation that we should make sure it's >>> reasonable to perform and that it is documented. >> >> Right now there is no kafka support in the plan for this. But this is >> doable directly through the zk api. Would it be sufficient to write down >> how to perform such an operation via the zk api or do we need a tool to do >> it? >> > > I think as long as there is a documented procedure for how to do it, that > will be good enough. It's mostly about making sure that we can, and that we > don't put something in place that would require downtime to a cluster in > order to change credentials. We can always develop a tool later if it is a > requested item. > > Thanks! > > -Todd > > > >> >> -Flavio >> >>> >>> >>> On Wed, Oct 21, 2015 at 1:23 PM, Jun Rao <j...@confluent.io> wrote: >>> >>>> Parth, >>>> >>>> For 2), in your approach, the broker/controller will then always have >> the >>>> overhead of resetting the ACL on startup after zookeeper.set.acl is set >> to >>>> true. The benefit of using a separate migration tool is that you paid >> the >>>> cost only once during upgrade. It is an extra step during the upgrade. >>>> However, given the other things that you need to do to upgrade to 0.9.0 >>>> (e.g. two rounds of rolling upgrades on all brokers, etc), I am not >> sure if >>>> it's worth to optimize away of this step. We probably just need to >> document >>>> this clearly. >>>> >>>> Todd, >>>> >>>> Just to be clear about the shared ZK usage. Once you set >> CREATOR_ALL_ACL + >>>> READ_ACL_UNSAFE on a path, only ZK clients with the same user as the >>>> creator can modify the path. Other ZK clients authenticated with a >>>> different user can read, but not modify the path. Are you concerned >> about >>>> the reads or the writes to ZK? >>>> >>>> Thanks, >>>> >>>> Jun >>>> >>>> >>>> >>>> On Wed, Oct 21, 2015 at 10:46 AM, Flavio Junqueira <f...@apache.org> >> wrote: >>>> >>>>> >>>>>> On 21 Oct 2015, at 18:07, Parth Brahmbhatt < >>>> pbrahmbh...@hortonworks.com> >>>>> wrote: >>>>>> >>>>>> I have 2 suggestions: >>>>>> >>>>>> 1) We need to document how does one move from secure to non secure >>>>>> environment: >>>>>> 1) change the config on all brokers to zookeeper.set.acl = false >>>>> and do a >>>>>> rolling upgrade. >>>>>> 2) Run the migration script with the jass config file so it is >>>> sasl >>>>>> authenticated with zookeeper and change the acls on all subtrees back >>>> to >>>>>> World modifiable. >>>>>> 3) remove the jaas config / or only the zookeeper section from >>>> the >>>>> jaas, >>>>>> and restart all brokers. >>>>>> >>>>> >>>>> Thanks for bringing it up, it makes sense to have a downgrade path and >>>>> document it. >>>>> >>>>> >>>>>> 2) I am not sure if we should force users trying to move from unsecure >>>> to >>>>>> secure environment to execute the migration script. In the second step >>>>>> once the zookeeper.set.acl is set to true, we can secure all the >>>> subtrees >>>>>> by calling ensureCorrectAcls as part of broker initialization (just >>>> after >>>>>> makesurePersistentPathExists). Not sure why we want to add one more >>>>>> manual/admin step when it can be automated. This also has the added >>>>>> advantage that migration script will not have to take a flag as input >>>> to >>>>>> figure out if it should set the acls to secure or unsecure given it >>>> will >>>>>> always be used to move from secure to unsecure. >>>>>> >>>>> >>>>> The advantage of the third step is to make a single traversal to change >>>>> any remaining znodes with the open ACL. As you suggest, each broker >> would >>>>> do it, so the overhead is much higher. I do agree that eliminating a >> step >>>>> is an advantage, though. >>>>> >>>>>> Given we are assuming all the information in zookeeper is world >>>> readable >>>>> , >>>>>> I don¹t see SSL support as a must have or a blocker for this KIP. >>>>> >>>>> OK, but keep in mind that SSL is only available in the 3.5 branch of >> ZK. >>>>> >>>>> -Flavio >>>>> >>>>>> >>>>>> Thanks >>>>>> Parth >>>>>> >>>>>> >>>>>> >>>>>> On 10/21/15, 9:56 AM, "Flavio Junqueira" <f...@apache.org> wrote: >>>>>> >>>>>>> >>>>>>>> On 21 Oct 2015, at 17:47, Todd Palino <tpal...@gmail.com> wrote: >>>>>>>> >>>>>>>> There seems to be a bit of detail lacking in the KIP. Specifically, >>>> I'd >>>>>>>> like to understand: >>>>>>>> >>>>>>>> 1) What znodes are the brokers going to secure? Is this >> configurable? >>>>>>>> How? >>>>>>> >>>>>>> Currently it is securing all paths here except the consumers one: >>>>>>> >>>>>>> >>>>> >>>> >> https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/utils >>>>>>> /ZkUtils.scala#L56 >>>>>>> < >>>>> >>>> >> https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/util >>>>>>> s/ZkUtils.scala#L56> >>>>>>> >>>>>>> This isn't configurable at the moment. >>>>>>> >>>>>>>> 2) What ACL is the broker going to apply? Is this configurable? >>>>>>> >>>>>>> The default is CREATOR_ALL_ACL + READ_ACL_UNSAFE, which means that an >>>>>>> authenticated client can manipulate secured znodes and everyone can >>>> read >>>>>>> znodes. The API of ZkUtils accommodates other ACLs, but the current >>>> code >>>>>>> is using the default. >>>>>>> >>>>>>>> 3) How will the admin tools (such as preferred replica election and >>>>>>>> partition reassignment) interact with this? >>>>>>>> >>>>>>> >>>>>>> Currently, you need to set a system property passing the login config >>>>>>> file to be able to authenticate the client and perform writes to ZK. >>>>>>> >>>>>>> -Flavio >>>>>>> >>>>>>>> -Todd >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Oct 21, 2015 at 9:16 AM, Ismael Juma <ism...@juma.me.uk> >>>>> wrote: >>>>>>>> >>>>>>>>> On Wed, Oct 21, 2015 at 5:04 PM, Flavio Junqueira <f...@apache.org> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Bringing the points Grant brought to this thread: >>>>>>>>>> >>>>>>>>>>> Is it worth mentioning the follow up steps that were discussed in >>>>> the >>>>>>>>> KIP >>>>>>>>>>> call in this KIP document? Some of them were: >>>>>>>>>>> >>>>>>>>>>> - Adding SSL support for Zookeeper >>>>>>>>>>> - Removing the "world readable" assumption >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Grant, how would you do it? I see three options: >>>>>>>>>> >>>>>>>>>> 1- Add to the existing KIP, but then the functionality we should >> be >>>>>>>>>> checking in soon won't include it, so the KIP will remain >>>> incomplete >>>>>>>>>> >>>>>>>>> >>>>>>>>> A "Future work" section would make sense to me, but I don't know >> how >>>>>>>>> this >>>>>>>>> is normally handled. >>>>>>>>> >>>>>>>>> Ismael
