I am not sure if the additional complexity in the Controller is worth it
for this use case. It seems like it would be better to swap the tools to
use AdminClient and then restrict access to ZK (via ACLs and/or network
segmentation). Either way, that proposal should be done via a separate KIP
as KIP-108 was specifically about create topic requests that are done via
the Kafka protocol.
Ismael
On Wed, May 31, 2017 at 12:37 AM, Dong Lin <lindon...@gmail.com> wrote:
> On Tue, May 30, 2017 at 4:26 PM, Colin McCabe <cmcc...@apache.org> wrote:
>
> > On Tue, May 30, 2017, at 15:55, Dong Lin wrote:
> > > Hey Colin,
> > >
> > > I think one big advantage of the broker side config is that it can not
> be
> > > ignored by the malicious client, right?
> >
> > Hi Dong,
> >
> > The scenario I was thinking of is where a malicious client communicates
> > directly with ZooKeeper, bypassing the broker. As far as I can see,
> > nothing that we do on the broker can prevent this from happening. It
> > has to be blocked by ZooKeeper itself.
> >
>
> I see. I agree that malicious client can still create the topic via
> zookeeper if we don't have ACL. The approach using the new config can
> prevent non-malicious client from using old script to create topic via
> zookeeper.
>
>
> >
> > >
> > > Thanks,
> > > Dong
> > >
> > > On Tue, May 30, 2017 at 3:53 PM, Dong Lin <lindon...@gmail.com> wrote:
> > >
> > > > Do we have an old version of bin/kafka-topics.sh which creates topic
> > via
> > > > ZK and still allows user to access ZK with ACL? Another concern is
> that
> > > > some user may not have ACL service deployed in their cluster. If
> > neither of
> > > > these is issue, then I would prefer the zookeeper approach instead
> of
> > > > adding a new broker config if the zookeeper approach is doable.
> >
> > Unfortunately, the latest version of kafka-topics.sh still creates
> > topics by talking directly to ZK. It has not been converted to use the
> > new AdminClient, although that is planned.
> >
>
> Yep. Thus the new config-based approach still has its advantage over
> ZK-based approach because it ensures that non-malicious user will not
> create topic via zookeeper.
>
>
> >
> > best,
> > Colin
> >
> >
> > > >
> > > > However, regardless of whether we secure the zookeeper from
> > unauthorized
> > > > user, I think KIP-108 should provide a solution to guarantee that all
> > topic
> > > > creation logic goes through the topic creation policy.
> > > >
> > > > Thanks,
> > > > Dong
> > > >
> > > > On Tue, May 30, 2017 at 3:39 PM, Colin McCabe <cmcc...@apache.org>
> > wrote:
> > > >
> > > >> It seems like, to make it really secure, we need the enforcement to
> be
> > > >> done at the ZooKepeer level. Any broker or client-side
> configuration
> > > >> can just be ignored by a malicious client. Do we have documentation
> > or
> > > >> code that configures ZK to prevent unprivileged users from modifying
> > the
> > > >> topic configurations?
> > > >>
> > > >> best,
> > > >> Colin
> > > >>
> > > >>
> > > >> On Tue, May 30, 2017, at 15:02, Dong Lin wrote:
> > > >> > Hey Ismael,
> > > >> >
> > > >> > I agree that it makes sense not to cover ZK-based topic creation
> > with
> > > >> the
> > > >> > topic creation policy and limit ZK access to brokers only going
> > forward.
> > > >> > My
> > > >> > point is that we need a way to disable ZK-based topic creation so
> > that
> > > >> > all
> > > >> > topic creation goes through the topic creation policy as specified
> > in
> > > >> > KIP-108. Does this make sense?
> > > >> >
> > > >> > One example solution is to add a broker-side config
> > > >> > "enable.zookeeper.topic.creation"
> > > >> > which defaults to "true". If user has overridden this config to be
> > > >> > "false",
> > > >> > then controller will delete the znode /brokers/topics/{topic} that
> > is
> > > >> not
> > > >> > created by the controller. We probably need some trick to
> > differentiate
> > > >> > between znode created by controller and znode created by outdated
> > tools.
> > > >> > For example, the new controller code can add a new field
> > "isController"
> > > >> > in
> > > >> > the znode /brokers/topics/{topic} when it creates this new znode.
> > Then
> > > >> if
> > > >> > the znode doesn't have this field AND there is no child under this
> > > >> znode,
> > > >> > controller can be sure it is created by outdated tools and remove
> > this
> > > >> > znode from zookeeper. Users who are using outdated tools to create
> > topic
> > > >> > will find that the topic is not created.
> > > >> >
> > > >> > Dong
> > > >> >
> > > >> > On Tue, May 30, 2017 at 2:24 PM, Ismael Juma <ism...@juma.me.uk>
> > wrote:
> > > >> >
> > > >> > > Hi Dong,
> > > >> > >
> > > >> > > No, ZK-based topic creation doesn't go through the policy since
> it
> > > >> doesn't
> > > >> > > go through the broker. Given that, I am not sure how the broker
> > config
> > > >> > > would work. Can you please elaborate? It seems like the way
> > forward
> > > >> is to
> > > >> > > limit ZK access to brokers only.
> > > >> > >
> > > >> > > Ismael
> > > >> > >
> > > >> > > On Tue, May 30, 2017 at 10:19 PM, Dong Lin <lindon...@gmail.com
> >
> > > >> wrote:
> > > >> > >
> > > >> > > > Hey Ismael,
> > > >> > > >
> > > >> > > > Thanks for the KIP. This is definitely useful.
> > > >> > > >
> > > >> > > > Does the KIP apply the topic creation policy to ZK-based topic
> > > >> creation?
> > > >> > > If
> > > >> > > > not, which seems to be the case from my understanding, should
> we
> > > >> have a
> > > >> > > new
> > > >> > > > broker config to disable ZK-based topic creation? This seems
> > > >> necessary to
> > > >> > > > prevent user from using stray builds to evade the topic
> creation
> > > >> policy.
> > > >> > > >
> > > >> > > > Thanks,
> > > >> > > > Dong
> > > >> > > >
> > > >> > > >
> > > >> > > >
> > > >> > > >
> > > >> > > >
> > > >> > > >
> > > >> > > > On Mon, Jan 9, 2017 at 1:42 PM, Roger Hoover <
> > > >> roger.hoo...@gmail.com>
> > > >> > > > wrote:
> > > >> > > >
> > > >> > > > > Got it. Thanks, Ismael.
> > > >> > > > >
> > > >> > > > > On Mon, Jan 9, 2017 at 10:42 AM, Ismael Juma <
> > ism...@juma.me.uk>
> > > >> > > wrote:
> > > >> > > > >
> > > >> > > > > > Hi Roger,
> > > >> > > > > >
> > > >> > > > > > That's a good question. The server defaults are passed via
> > the
> > > >> > > > > `configure`
> > > >> > > > > > method of the `Configurable` interface that is implemented
> > by
> > > >> > > > > > `CreateTopicPolicy`. I'll mention this explicitly in the
> > KIP.
> > > >> > > > > >
> > > >> > > > > > Ismael
> > > >> > > > > >
> > > >> > > > > > On Mon, Jan 9, 2017 at 6:04 PM, Roger Hoover <
> > > >> roger.hoo...@gmail.com
> > > >> > > >
> > > >> > > > > > wrote:
> > > >> > > > > >
> > > >> > > > > > > This is great. Thanks, Ismael.
> > > >> > > > > > >
> > > >> > > > > > > One question. When TopicDetails are passed to the
> policy
> > > >> > > > > implementation,
> > > >> > > > > > > would the server defaults already have been merged? If
> > not, I
> > > >> > > think
> > > >> > > > > the
> > > >> > > > > > > policy also needs access to the server defaults.
> > > >> > > > > > >
> > > >> > > > > > > Cheers,
> > > >> > > > > > >
> > > >> > > > > > > Roger
> > > >> > > > > > >
> > > >> > > > > > > On Fri, Jan 6, 2017 at 9:26 AM, Ismael Juma <
> > > >> ism...@juma.me.uk>
> > > >> > > > wrote:
> > > >> > > > > > >
> > > >> > > > > > > > Thanks for the review Jun. Yes, that's a good point, I
> > have
> > > >> > > updated
> > > >> > > > > the
> > > >> > > > > > > > KIP.
> > > >> > > > > > > >
> > > >> > > > > > > > Ismael
> > > >> > > > > > > >
> > > >> > > > > > > > On Fri, Jan 6, 2017 at 5:15 PM, Jun Rao <
> > j...@confluent.io>
> > > >> > > wrote:
> > > >> > > > > > > >
> > > >> > > > > > > > > Hi, Ismael,
> > > >> > > > > > > > >
> > > >> > > > > > > > > Thanks for the KIP. Looks reasonable to me. To be
> > > >> consistent
> > > >> > > with
> > > >> > > > > the
> > > >> > > > > > > > > pattern used in other pluggable interfaces, we
> > probably
> > > >> should
> > > >> > > > make
> > > >> > > > > > the
> > > >> > > > > > > > new
> > > >> > > > > > > > > interface configurable and closable?
> > > >> > > > > > > > >
> > > >> > > > > > > > > Jun
> > > >> > > > > > > > >
> > > >> > > > > > > > > On Fri, Jan 6, 2017 at 4:16 AM, Ismael Juma <
> > > >> ism...@juma.me.uk
> > > >> > > >
> > > >> > > > > > wrote:
> > > >> > > > > > > > >
> > > >> > > > > > > > > > Thanks Dan and Colin for the feedback. I updated
> the
> > > >> KIP to
> > > >> > > > > include
> > > >> > > > > > > the
> > > >> > > > > > > > > > addition of a validation mode. Since we need to
> > bump the
> > > >> > > > protocol
> > > >> > > > > > > > version
> > > >> > > > > > > > > > for that, I also added an error message per topic
> > to the
> > > >> > > > > response.
> > > >> > > > > > I
> > > >> > > > > > > > had
> > > >> > > > > > > > > > the latter as "Future Work", but I actually felt
> > that it
> > > >> > > should
> > > >> > > > > be
> > > >> > > > > > in
> > > >> > > > > > > > the
> > > >> > > > > > > > > > first version (good to have feedback confirming
> > that).
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > Let me know if the changes look good to you.
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > Ismael
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > On Thu, Jan 5, 2017 at 9:54 PM, Colin McCabe <
> > > >> > > > cmcc...@apache.org
> > > >> > > > > >
> > > >> > > > > > > > wrote:
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > > Yeah, I agree... having a validation mode would
> be
> > > >> nice.
> > > >> > > We
> > > >> > > > > > should
> > > >> > > > > > > > be
> > > >> > > > > > > > > > > explicit that passing validation doesn't 100%
> > > >> guarantee
> > > >> > > that
> > > >> > > > a
> > > >> > > > > > > > > > > subsequent call to create the topic will
> succeed,
> > > >> though.
> > > >> > > > > There
> > > >> > > > > > is
> > > >> > > > > > > > an
> > > >> > > > > > > > > > > obvious race condition there-- for example,
> with a
> > > >> plugin
> > > >> > > > which
> > > >> > > > > > > > > consults
> > > >> > > > > > > > > > > some external authentication system, there could
> > be a
> > > >> > > change
> > > >> > > > to
> > > >> > > > > > the
> > > >> > > > > > > > > > > privileges in between validation and attempted
> > > >> creation.
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > It also seems like we should try to provide a
> > helpful
> > > >> > > > exception
> > > >> > > > > > > > message
> > > >> > > > > > > > > > > for the cases where topic creation fails. This
> > might
> > > >> > > involve
> > > >> > > > > > > adding
> > > >> > > > > > > > > > > more detail about error conditions to
> > > >> > > CreateTopicsRequest...
> > > >> > > > > > right
> > > >> > > > > > > > now
> > > >> > > > > > > > > > > it just returns an error code, but a text
> message
> > > >> would be
> > > >> > > a
> > > >> > > > > nice
> > > >> > > > > > > > > > > addition.
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > cheers,
> > > >> > > > > > > > > > > Colin
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > On Thu, Jan 5, 2017, at 13:41, dan wrote:
> > > >> > > > > > > > > > > > it would be nice to have a dry-run or validate
> > > >> ability
> > > >> > > > added
> > > >> > > > > to
> > > >> > > > > > > > this
> > > >> > > > > > > > > > kip.
> > > >> > > > > > > > > > > > since we are offloading validation to a 3rd
> > party
> > > >> > > > > implementor a
> > > >> > > > > > > > > random
> > > >> > > > > > > > > > > > user
> > > >> > > > > > > > > > > > can't know a priori (based solely on kafka
> > configs)
> > > >> > > > whether a
> > > >> > > > > > > call
> > > >> > > > > > > > > > should
> > > >> > > > > > > > > > > > succeed without actually creating the topic.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > a similar case is in connect where there is a
> > > >> separate
> > > >> > > > > endpoint
> > > >> > > > > > > > > > > > <https://github.com/apache/kaf
> > > >> ka/blob/trunk/connect/
> > > >> > > > > > > > > > > runtime/src/main/java/org/apac
> > > >> > > he/kafka/connect/runtime/rest/
> > > >> > > > > > > > resources/
> > > >> > > > > > > > > > > ConnectorPluginsResource.java#L49-L58>
> > > >> > > > > > > > > > > > to attempt to validate a connect configuration
> > > >> without
> > > >> > > > > actually
> > > >> > > > > > > > > > creating
> > > >> > > > > > > > > > > > the connector.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > thanks
> > > >> > > > > > > > > > > > dan
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > On Thu, Jan 5, 2017 at 7:34 AM, Ismael Juma <
> > > >> > > > > ism...@juma.me.uk
> > > >> > > > > > >
> > > >> > > > > > > > > wrote:
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Hi all,
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > We've posted "KIP-108: Create Topic Policy"
> > for
> > > >> > > > discussion:
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > https://cwiki.apache.org/confl
> > > >> uence/display/KAFKA/KIP-
> > > >> > > > > > > > > > > > > 108%3A+Create+Topic+Policy
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Please take a look. Your feedback is
> > appreciated.
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Thanks,
> > > >> > > > > > > > > > > > > Ismael
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >>
> > > >
> > > >
> >
>
