kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jakub Scholz <ja...@scholz.cz>
Subject Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface
Date Wed, 10 Jan 2018 09:39:54 GMT
I'm sorry guys, I'm a bit busy with something else this week. But I will
get back to his till the end of the week.

Thanks & Regards
Jakub

On Tue, Jan 9, 2018 at 1:19 AM, Ewen Cheslack-Postava <ewen@confluent.io>
wrote:

> On Mon, Jan 8, 2018 at 11:39 AM, Randall Hauch <rhauch@gmail.com> wrote:
>
> > Nice feedback, Ewen. Thanks!
> >
> > On Thu, Jan 4, 2018 at 5:11 PM, Ewen Cheslack-Postava <ewen@confluent.io
> >
> > wrote:
> >
> > > Hey Jakub,
> > >
> > > Sorry for not getting to this sooner. Overall the proposal looks good
> to
> > > me, I just had a couple of questions.
> > >
> > > 1. For the configs/overrides, does this happen on a per-setting basis
> or
> > if
> > > one override is included do we not use any of the original settings? I
> > > suspect that if you need to override one setting, it probably means
> > you're
> > > using an entirely different config and so the latter behavior seems
> > better
> > > to me. We've talked a bit about doing something similar for the
> > > producer/consumer security settings as well so you don't have to
> specify
> > > security configs in 3 places in your worker config.
> > >
> >
> > Not sure if you were referring to
> > https://issues.apache.org/jira/browse/KAFKA-6387, but I just withdrew
> that
> > proposal (and the corresponding KIP-246) because behavior with existing
> > configurations was not backward compatible, so existing configs might
> have
> > very different behavior after the "inheritance" was implemented.
> >
> > But regardless, I do think that in this case if you have to override one
> of
> > the settings you probably need to override multiple. So I'd be in favor
> of
> > requiring all configs to be specified in the overridden `listeners.*`
> > properties.
> >
>
> Yeah, a related case i was thinking of is how key.converter and
> value.converter overrides work in Connectors. It's not exactly the same,
> but in that case, if you include the key.converter setting in the connector
> config, then nothing with key.converter prefix from the worker is passed
> along. Just might be worth clarifying the all-or-nothing behavior. Also how
> we apply it in this case (e.g. is there one key setting we can use that, if
> it appears, then we do not inherit any security configs from the worker?)
>
>
> >
> >
> > >
> > > 2. For using default values from the worker config, I am wondering how
> > > convinced we are that it will be common for them to be the same? I
> really
> > > don't have enough experience w/ these setups to know, so just a
> question
> > > here. I think the other thing to take into account here is that even
> > though
> > > we're not dealing with authorization in this KIP, we will eventually
> want
> > > it for these APIs. Would we expect to be using the same principal for
> > Kafka
> > > and the Connect REST API? In a case where a company has a Connect
> cluster
> > > that, e.g., an ops team manages and they are the only ones that are
> > > supposed to make changes, that would make sense to me. But for a setup
> > > where some dev team is allowed to use the REST API to create new
> > connectors
> > > but the cluster is managed by an ops team, I would think the Kafka
> > > credentials would be different. I'm not sure how frequent each case
> would
> > > be, so I'm a bit unsure about the default of using the worker security
> > > configs by default. Thoughts?
> > >
> > > 3. We should probably specify the default in the table for
> > > rest.advertised.security.protocol because in ConfigDef if you don't
> > > specify
> > > a default value it becomes a required config. The HTTP default will
> > > probably need to be in there anyway.
> > >
> > > 4. Do we want to list the existing settings as deprecated and just move
> > to
> > > using listeners for consistency? We don't need to remove them anytime
> > soon,
> > > but given that the broker is doing the same, maybe we should just do
> that
> > > in this KIP?
> > >
> >
> > Marking them as deprecated in this KIP sounds good to me.
> >
> > >
> > > I think these are mostly small details, overall it looks like a good
> > plan!
> > >
> >
> > +1
> >
> > Randall
> >
> >
> > >
> > > Thanks,
> > > Ewen
> > >
> > > On Tue, Oct 24, 2017 at 5:19 AM, Jakub Scholz <jakub@scholz.cz> wrote:
> > >
> > > > There has been no discussion since my last update week ago. Unless
> > > someone
> > > > has some further comments in the next 48 hours, I will start the
> voting
> > > for
> > > > this KIP.
> > > >
> > > > Thanks & Regards
> > > > Jakub
> > > >
> > > > On Tue, Oct 17, 2017 at 5:54 PM, Jakub Scholz <jakub@scholz.cz>
> wrote:
> > > >
> > > > > Ok, so I updated the KIP according to what we discussed. Please
> have
> > a
> > > > > look at the updates. Two points I'm not 100% sure about:
> > > > >
> > > > > 1) Should we mark the rest.host.name and rest.port options as
> > > > deprecated?
> > > > >
> > > > > 2) I needed to also address the advertised hostname / port. With
> > > multiple
> > > > > listeners it is not clear anymore which one should be used. I saw
> as
> > > one
> > > > > option to add advertised.listeners option and some modified version
> > of
> > > > > inter.broker.listener.name option to follow what is done in Kafka
> > > > > brokers. But for the Connect REST interface, we do not advertise
> the
> > > > > address to the clients like in Kafka broker. So we only need to
> tell
> > > > other
> > > > > workers how to connect - and for that we need only one advertised
> > > > address.
> > > > > So I decided to reuse the existing rest.advertised.host.name and
> > > > > rest.advertised.port options and add additional option
> > > > > rest.advertised.security.protocol to specify whether HTTP or HTTPS
> > > > should
> > > > > be used. Does this make sense to you? DO you think this is the
> right
> > > > > approach?
> > > > >
> > > > > Thanks & Regards
> > > > > Jakub
> > > > >
> > > > > On Mon, Oct 16, 2017 at 6:34 PM, Randall Hauch <rhauch@gmail.com>
> > > wrote:
> > > > >
> > > > >> The broker's configuration options are "listeners" (plural) and
> > > > >> "listeners.security.protocol.map". I agree that following the
> > pattern
> > > > set
> > > > >> by the broker is better, so these are really good ideas. However,
> at
> > > > this
> > > > >> point I don't see a need for the "listeners.security.procotol.
> map",
> > > > which
> > > > >> for the broker must be set if the listener name is not a security
> > > > >> protocol.
> > > > >> Can we not simply just allow "HTTP" and "HTTPS" as the names
of
> the
> > > > >> listeners (rather than the broker's "PLAINTEXT", "SSL", etc.)?
If
> > so,
> > > > then
> > > > >> for example "listeners" might be set to "http://myhost:8081,
> > > > >> https://myhost:80", which seems to work out nicely without
> needing
> > > > >> listener
> > > > >> names other than security protocols.
> > > > >>
> > > > >> I also like using the worker's SSL and SASL security configs
by
> > > default
> > > > if
> > > > >> "https" is included in the listener, but allowing the overriding
> of
> > > this
> > > > >> via other additional properties. Here, I'm not a big fan of
> > > > >> "listeners.name.https.*" prefix, which I think is pretty verbose,
> > but
> > > I
> > > > >> could see "listener.https.*" as a prefix. This allows us to add
> > other
> > > > >> security protocols at some point, if that ever becomes necessary.
> > > > >>
> > > > >> +1 for continuing down this road. Nice work.
> > > > >>
> > > > >> On Mon, Oct 16, 2017 at 9:51 AM, Ted Yu <yuzhihong@gmail.com>
> > wrote:
> > > > >>
> > > > >> > +1 to this proposal.
> > > > >> >
> > > > >> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <jakub@scholz.cz>
> > > > wrote:
> > > > >> >
> > > > >> > > I was having some more thoughts about it. We can simply
take
> > over
> > > > what
> > > > >> > > Kafka broker implements for the listeners:
> > > > >> > > - We can take over the "listener" and "
> > > > listener.security.protocol.ma
> > > > >> p"
> > > > >> > > options to define multiple REST listeners and the security
> > > protocol
> > > > >> they
> > > > >> > > should use
> > > > >> > > - The HTTPS interface will by default use the default
> > > configuration
> > > > >> > options
> > > > >> > > ("ssl.keystore.localtion" etc.). But if desired, the
values
> can
> > be
> > > > >> > > overridden for given listener (again, as in Kafka broker
"
> > > > >> listener.name
> > > > >> > > .<LISTENER_NAME>.ssl.keystore.location")
> > > > >> > >
> > > > >> > > This should address both issues raised. But before
I
> incorporate
> > > it
> > > > >> into
> > > > >> > > the KIP, I would love to get some feedback if this
sounds OK.
> > > Please
> > > > >> let
> > > > >> > me
> > > > >> > > know what do you think ...
> > > > >> > >
> > > > >> > > Jakub
> > > > >> > >
> > > > >> > > On Sun, Oct 15, 2017 at 12:23 AM, Jakub Scholz <
> jakub@scholz.cz
> > >
> > > > >> wrote:
> > > > >> > >
> > > > >> > > > I agree, adding both HTTP and HTTPS is not complicated.
I
> just
> > > > >> didn't
> > > > >> > saw
> > > > >> > > > the use case for it. But I can add it. Would you
add just
> > > support
> > > > >> for a
> > > > >> > > > single HTTP and single HTTPS interface? Or do
you see some
> > value
> > > > >> even
> > > > >> > in
> > > > >> > > > allowing more than 2 interfaces (for example one
HTTP and
> two
> > > > HTTPS
> > > > >> > with
> > > > >> > > > different configuration)? It could be done similarly
to how
> > the
> > > > >> Kafka
> > > > >> > > > broker does it through the "listener" configuration
> parameter
> > > with
> > > > >> > comma
> > > > >> > > > separated list. What do you think?
> > > > >> > > >
> > > > >> > > > As for the "rest" prefix - if we remove it, some
of the same
> > > > >> > > configuration
> > > > >> > > > options are already used today as the option for
connecting
> > from
> > > > >> Kafka
> > > > >> > > > Connect to Kafka broker. So I'm not sure we should
mix
> them. I
> > > can
> > > > >> > > > definitely imagine some cases where the client
SSL
> > configuration
> > > > >> will
> > > > >> > not
> > > > >> > > > be the same as the REST HTTPS configuration. That
is why I
> > added
> > > > the
> > > > >> > > > prefix. If we remove the prefix, how would you
deal with
> this?
> > > > >> > > >
> > > > >> > > > On Fri, Oct 13, 2017 at 6:25 PM, Randall Hauch
<
> > > rhauch@gmail.com>
> > > > >> > wrote:
> > > > >> > > >
> > > > >> > > >> Also, do we need these properties to be preceded
with
> `rest`?
> > > I'd
> > > > >> > argue
> > > > >> > > >> that we're just configuring the worker's SSL
information,
> and
> > > > that
> > > > >> the
> > > > >> > > >> REST
> > > > >> > > >> API would just use that. If we added another
non-REST API,
> > we'd
> > > > >> want
> > > > >> > to
> > > > >> > > >> use
> > > > >> > > >> the same security configuration.
> > > > >> > > >>
> > > > >> > > >> It's not that complicated in Jetty to support
both "http"
> and
> > > > >> "https"
> > > > >> > > >> simultaneously, so IMO we should add that
from the
> beginning.
> > > > >> > > >>
> > > > >> > > >> On Fri, Oct 13, 2017 at 9:34 AM, Randall Hauch
<
> > > rhauch@gmail.com
> > > > >
> > > > >> > > wrote:
> > > > >> > > >>
> > > > >> > > >> > It'd be useful to specify the default
values for the
> > > > >> configuration
> > > > >> > > >> > properties.
> > > > >> > > >> >
> > > > >> > > >> > On Tue, Oct 10, 2017 at 2:53 AM, Jakub
Scholz <
> > > jakub@scholz.cz
> > > > >
> > > > >> > > wrote:
> > > > >> > > >> >
> > > > >> > > >> >> FYI: Based on Ewen's suggestion from
the related JIRA, I
> > > > added a
> > > > >> > > >> >> clarification to the KIP that it
doesn't do anything
> > around
> > > > >> > > >> authorization
> > > > >> > > >> >> /
> > > > >> > > >> >> ACLs. While authorization / ACLs
would be for sure
> > valuable
> > > > >> > feature I
> > > > >> > > >> >> would
> > > > >> > > >> >> prefer to leave it for different
KIP.
> > > > >> > > >> >>
> > > > >> > > >> >> Jakub
> > > > >> > > >> >>
> > > > >> > > >> >> On Mon, Oct 9, 2017 at 5:25 PM, Jakub
Scholz <
> > > jakub@scholz.cz
> > > > >
> > > > >> > > wrote:
> > > > >> > > >> >>
> > > > >> > > >> >> > Hi,
> > > > >> > > >> >> >
> > > > >> > > >> >> > I would like to start a discussion
about KIP-208: Add
> > SSL
> > > > >> support
> > > > >> > > to
> > > > >> > > >> >> Kafka
> > > > >> > > >> >> > Connect REST interface (https://cwiki.apache.org/
> > > > >> > > >> >> > confluence/display/KAFKA/KIP-
> 208%3A+Add+SSL+support+to+
> > > > >> > > >> >> > Kafka+Connect+REST+interface).
> > > > >> > > >> >> >
> > > > >> > > >> >> > I think this would be useful
feature to improve the
> > > security
> > > > >> of
> > > > >> > > Kafka
> > > > >> > > >> >> > Connect.
> > > > >> > > >> >> >
> > > > >> > > >> >> > Thanks & Regards
> > > > >> > > >> >> > Jakub
> > > > >> > > >> >> >
> > > > >> > > >> >>
> > > > >> > > >> >
> > > > >> > > >> >
> > > > >> > > >>
> > > > >> > > >
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message