[DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

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

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

Sorry, that is a typo. It should be rest.ssl.client.auth and it should
correspond to the ssl.client.auth option in the broker.

The rest.ssl.client.auth option would support values of required, requested
and none (none being the default). It will control whether:
* the connecting client is required to do SSL/TLS client authentication
(required)
* the connecting client can decide to skip the SSL/TLS client
authentication (requested)
* the SSL/TLS authentication is be completely disabled (none).

I updated the KIP page. Thanks for noticing it.

Regards
Jakub

On Mon, Oct 9, 2017 at 5:52 PM, Ted Yu <yu...@gmail.com> wrote:

> For rest.ssl.clientAuth , I don't find counterpart in existing code. Can
> you add explanation on the KIP ?
>
> Thanks
>
> On Mon, Oct 9, 2017 at 8:25 AM, Jakub Scholz <ja...@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
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Ted Yu <yu...@gmail.com>]

For rest.ssl.clientAuth , I don't find counterpart in existing code. Can
you add explanation on the KIP ?

Thanks

On Mon, Oct 9, 2017 at 8:25 AM, Jakub Scholz <ja...@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
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

I opened the PR: https://issues.apache.org/jira/browse/KAFKA-4029

It is still work in progress, missing mainly tests, docu etc. I will
continue to work on it tomorrow. But it shows the implementation. One of
the things which came to my mind - the PR is currently using the
HttpURLConnection
class for forwarding the HTTP(S) calls to the leader (as was used by Kafka
before). Configuring SSL - especially things like cipher suites is quite
cumbersome there. What about using some more advanced HTTP client? Such as
the Jetty client. It would mean additional dependencies, but we have
already Jetty server, so the different might not be so big. What is your
opinion? I don't know whether there is some general policy for this.

Thanks & Regards
Jakub

On Sun, Jan 14, 2018 at 10:20 PM, Jakub Scholz <ja...@scholz.cz> wrote:

> Hi Ewen,
>
> Thanks for your comments / questions.
>
> re 1) I was using the valuesWithPrefixOverride(...) method from
> AbstractConfig class. That takes the overrides setting by setting. It
> should not be hard to change the code to use only the overrides if one
> value has changed. But I think that the broker is using the same for the
> SSL configuration. So I guess the question is whether we want to keep thins
> behave the same within Connect (e.g. with the key.converter /
> value.converter) or with the broker configuration. I would be ultimately
> fine with both, so if you want I can change it to the approach you
> suggested.
>
> re 2) I think the dangerous thing is that we are mixing different purposes
> for the same fields. The truststore is used for server authentication when
> connecting to Kafka broker and for user authentication when used for the
> REST API. The keystore is used for client authentication when connecting to
> Kafka brokers but as server certificate for the REST API. It can work fine,
> but it might be confusing and it is not obviously clear what are all the
> functions of given truststore / keystore. And that might not be a good
> thing for security related configuration.
>
> However because they have different roles, using the same truststore
> configuration for both doesn't mean that every Kafka Connect user is
> automatically able to connect also directly to Kafka broker. You can have
> in the truststore several public keys - each for different purpose.
>
> I think the original KIP actually proposed to use always separate fields.
> It was modified later after feedback from the discussion. I think that the
> current way is good because it is most flexible. But I'm fine with
> implementing it both ways.
>
> re 3) What I'm actually using right now is the default value of "null"
> (although in the KIP it was actually empty - I added the null only now).
> That makes it not required in ConfigDef. In the same time it makes it
> possible for Kafka Connect that if the protocol is not specified, I can
> define it based on the listeners field. Thanks to that, when the user has
> only one listener specified in the "listeners" field he does not need to
> specify the protocol. When I set the default value to HTTP, users will need
> to change it if they want to use HTTPS for the communication which would
> make it more complicated for the users. So I think my current version is
> better. What do you think?
>
> re 4) Yeah, I was thinking about it as well. I changed the KIP.
>
> I will rebase the work I have done so far, finish it and open a WIP PR, so
> that you can see the code as well.
>
> Thanks & regards
> Jakub
>
> On Wed, Jan 10, 2018 at 10:39 AM, Jakub Scholz <ja...@scholz.cz> wrote:
>
>> 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 <ew...@confluent.io>
>> wrote:
>>
>>> On Mon, Jan 8, 2018 at 11:39 AM, Randall Hauch <rh...@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 <ja...@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 <ja...@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.m
>>> ap",
>>> > > > 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 <yu...@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-2
>>> 08%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
>>> > > > >> > > >> >> >
>>> > > > >> > > >> >>
>>> > > > >> > > >> >
>>> > > > >> > > >> >
>>> > > > >> > > >>
>>> > > > >> > > >
>>> > > > >> > > >
>>> > > > >> > >
>>> > > > >> >
>>> > > > >>
>>> > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

Hi all,

I did one more update to the KIP-208. I added the
"ssl.endpoint.identification.algorithm" to the list of supported options.
It can be used to enable / disable the hostname validation when the Kafka
Connect nodes are forwarding the requests to the leader. It is minor but
useful change, so I hope nobody minds :-).

Thanks & Regards
Jakub

On Fri, Jan 19, 2018 at 12:53 AM, Jakub Scholz <ja...@scholz.cz> wrote:

> Hi Randall,
>
> Yes the KIP should be up to date. The VOTE thread is actually running
> already for more than 2 months. So the only thing we need is the votes. I
> pinged the vote thread so that it gets more attention.
>
> Thanks & Regards
> Jakub
>
> On Thu, Jan 18, 2018 at 7:33 PM, Randall Hauch <rh...@gmail.com> wrote:
>
>> Jakub, have you had a chance to update the KIP with the latest changes?
>> Would be great to start the vote today so that it's open long enough to
>> adopt before the deadline on Tuesday. Let me know if I can help.
>>
>> On Wed, Jan 17, 2018 at 1:25 AM, Jakub Scholz <ja...@scholz.cz> wrote:
>>
>> > I have been thinking about this a bit more yesterday while updating the
>> > code. I think you are right, we should use only the prefixed values if
>> at
>> > least one of them exists. Even I got quite easily confused what setup is
>> > actually used when the fields are mixed :-). Randall was also in favour
>> of
>> > this approach. So I think we should go this way. I will update the KIP
>> > accordingly.
>> >
>> >
>> > > I'm fine with consistency, but maybe the thing to do here then is to
>> > ensure
>> > > that we definitely log the "effective" or "derived" config before
>> using
>> > it
>> > > so there is at least some useful trace of what we actually used that
>> can
>> > be
>> > > helpful in debugging.
>> >
>>
>
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

Hi Randall,

Yes the KIP should be up to date. The VOTE thread is actually running
already for more than 2 months. So the only thing we need is the votes. I
pinged the vote thread so that it gets more attention.

Thanks & Regards
Jakub

On Thu, Jan 18, 2018 at 7:33 PM, Randall Hauch <rh...@gmail.com> wrote:

> Jakub, have you had a chance to update the KIP with the latest changes?
> Would be great to start the vote today so that it's open long enough to
> adopt before the deadline on Tuesday. Let me know if I can help.
>
> On Wed, Jan 17, 2018 at 1:25 AM, Jakub Scholz <ja...@scholz.cz> wrote:
>
> > I have been thinking about this a bit more yesterday while updating the
> > code. I think you are right, we should use only the prefixed values if at
> > least one of them exists. Even I got quite easily confused what setup is
> > actually used when the fields are mixed :-). Randall was also in favour
> of
> > this approach. So I think we should go this way. I will update the KIP
> > accordingly.
> >
> >
> > > I'm fine with consistency, but maybe the thing to do here then is to
> > ensure
> > > that we definitely log the "effective" or "derived" config before using
> > it
> > > so there is at least some useful trace of what we actually used that
> can
> > be
> > > helpful in debugging.
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Randall Hauch <rh...@gmail.com>]

Jakub, have you had a chance to update the KIP with the latest changes?
Would be great to start the vote today so that it's open long enough to
adopt before the deadline on Tuesday. Let me know if I can help.

On Wed, Jan 17, 2018 at 1:25 AM, Jakub Scholz <ja...@scholz.cz> wrote:

> I have been thinking about this a bit more yesterday while updating the
> code. I think you are right, we should use only the prefixed values if at
> least one of them exists. Even I got quite easily confused what setup is
> actually used when the fields are mixed :-). Randall was also in favour of
> this approach. So I think we should go this way. I will update the KIP
> accordingly.
>
>
> > I'm fine with consistency, but maybe the thing to do here then is to
> ensure
> > that we definitely log the "effective" or "derived" config before using
> it
> > so there is at least some useful trace of what we actually used that can
> be
> > helpful in debugging.
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

I have been thinking about this a bit more yesterday while updating the
code. I think you are right, we should use only the prefixed values if at
least one of them exists. Even I got quite easily confused what setup is
actually used when the fields are mixed :-). Randall was also in favour of
this approach. So I think we should go this way. I will update the KIP
accordingly.


> I'm fine with consistency, but maybe the thing to do here then is to ensure
> that we definitely log the "effective" or "derived" config before using it
> so there is at least some useful trace of what we actually used that can be
> helpful in debugging.

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Ewen Cheslack-Postava <ew...@confluent.io>]

On Sun, Jan 14, 2018 at 1:20 PM, Jakub Scholz <ja...@scholz.cz> wrote:

> Hi Ewen,
>
> Thanks for your comments / questions.
>
> re 1) I was using the valuesWithPrefixOverride(...) method from
> AbstractConfig class. That takes the overrides setting by setting. It
> should not be hard to change the code to use only the overrides if one
> value has changed. But I think that the broker is using the same for the
> SSL configuration. So I guess the question is whether we want to keep thins
> behave the same within Connect (e.g. with the key.converter /
> value.converter) or with the broker configuration. I would be ultimately
> fine with both, so if you want I can change it to the approach you
> suggested.
>
>
This is a fair point. Mainly my concern is that the "effective"
configuration would become confusing -- if you don't remember that it is
per-field, your config files could look fine (separate blocks of configs if
you need them to be different), but still end up with an effective config
that would be a mishmash of the two.

I'm fine with consistency, but maybe the thing to do here then is to ensure
that we definitely log the "effective" or "derived" config before using it
so there is at least some useful trace of what we actually used that can be
helpful in debugging.


> re 2) I think the dangerous thing is that we are mixing different purposes
> for the same fields. The truststore is used for server authentication when
> connecting to Kafka broker and for user authentication when used for the
> REST API. The keystore is used for client authentication when connecting to
> Kafka brokers but as server certificate for the REST API. It can work fine,
> but it might be confusing and it is not obviously clear what are all the
> functions of given truststore / keystore. And that might not be a good
> thing for security related configuration.
>
> However because they have different roles, using the same truststore
> configuration for both doesn't mean that every Kafka Connect user is
> automatically able to connect also directly to Kafka broker. You can have
> in the truststore several public keys - each for different purpose.
>
> I think the original KIP actually proposed to use always separate fields.
> It was modified later after feedback from the discussion. I think that the
> current way is good because it is most flexible. But I'm fine with
> implementing it both ways.
>

Sure, I wasn't sure how common it would be to want to inherit the
keystore/truststore settings but override others, i.e. whether you would
have included both principals together or, if they were going to be
different anyway, possibly ship them separately.

I don't feel strongly about this, it's mostly a question from ignorance on
how people would be managing these principals/permissions/configurations.
Mainly I'm trying to make sure we get a good balance of ease of use,
encouraging good security practices, and being able to lock down strongly &
carefully.


>
> re 3) What I'm actually using right now is the default value of "null"
> (although in the KIP it was actually empty - I added the null only now).
> That makes it not required in ConfigDef. In the same time it makes it
> possible for Kafka Connect that if the protocol is not specified, I can
> define it based on the listeners field. Thanks to that, when the user has
> only one listener specified in the "listeners" field he does not need to
> specify the protocol. When I set the default value to HTTP, users will need
> to change it if they want to use HTTPS for the communication which would
> make it more complicated for the users. So I think my current version is
> better. What do you think?
>

I see, this sounds fine then. I had thought it was just no default value at
all.


>
> re 4) Yeah, I was thinking about it as well. I changed the KIP.
>
> I will rebase the work I have done so far, finish it and open a WIP PR, so
> that you can see the code as well.
>
>
Sounds good, thanks!

If we think we've converged, we should probably revive the vote thread
again and try to get this passed through. KIP deadline and then feature
freeze are coming up soon. Not a big deal if we can't make it for this
release, but I know quite a few folks are looking forward to this feature.

-Ewen



> Thanks & regards
> Jakub
>
> On Wed, Jan 10, 2018 at 10:39 AM, Jakub Scholz <ja...@scholz.cz> wrote:
>
> > 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 <rh...@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 <ja...@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 <ja...@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.m
> >> ap",
> >> > > > 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 <yu...@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-2
> >> 08%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
> >> > > > >> > > >> >> >
> >> > > > >> > > >> >>
> >> > > > >> > > >> >
> >> > > > >> > > >> >
> >> > > > >> > > >>
> >> > > > >> > > >
> >> > > > >> > > >
> >> > > > >> > >
> >> > > > >> >
> >> > > > >>
> >> > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

Hi Ewen,

Thanks for your comments / questions.

re 1) I was using the valuesWithPrefixOverride(...) method from
AbstractConfig class. That takes the overrides setting by setting. It
should not be hard to change the code to use only the overrides if one
value has changed. But I think that the broker is using the same for the
SSL configuration. So I guess the question is whether we want to keep thins
behave the same within Connect (e.g. with the key.converter /
value.converter) or with the broker configuration. I would be ultimately
fine with both, so if you want I can change it to the approach you
suggested.

re 2) I think the dangerous thing is that we are mixing different purposes
for the same fields. The truststore is used for server authentication when
connecting to Kafka broker and for user authentication when used for the
REST API. The keystore is used for client authentication when connecting to
Kafka brokers but as server certificate for the REST API. It can work fine,
but it might be confusing and it is not obviously clear what are all the
functions of given truststore / keystore. And that might not be a good
thing for security related configuration.

However because they have different roles, using the same truststore
configuration for both doesn't mean that every Kafka Connect user is
automatically able to connect also directly to Kafka broker. You can have
in the truststore several public keys - each for different purpose.

I think the original KIP actually proposed to use always separate fields.
It was modified later after feedback from the discussion. I think that the
current way is good because it is most flexible. But I'm fine with
implementing it both ways.

re 3) What I'm actually using right now is the default value of "null"
(although in the KIP it was actually empty - I added the null only now).
That makes it not required in ConfigDef. In the same time it makes it
possible for Kafka Connect that if the protocol is not specified, I can
define it based on the listeners field. Thanks to that, when the user has
only one listener specified in the "listeners" field he does not need to
specify the protocol. When I set the default value to HTTP, users will need
to change it if they want to use HTTPS for the communication which would
make it more complicated for the users. So I think my current version is
better. What do you think?

re 4) Yeah, I was thinking about it as well. I changed the KIP.

I will rebase the work I have done so far, finish it and open a WIP PR, so
that you can see the code as well.

Thanks & regards
Jakub

On Wed, Jan 10, 2018 at 10:39 AM, Jakub Scholz <ja...@scholz.cz> wrote:

> 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 <ew...@confluent.io>
> wrote:
>
>> On Mon, Jan 8, 2018 at 11:39 AM, Randall Hauch <rh...@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 <ja...@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 <ja...@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 <rh...@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.m
>> ap",
>> > > > 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 <yu...@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-2
>> 08%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
>> > > > >> > > >> >> >
>> > > > >> > > >> >>
>> > > > >> > > >> >
>> > > > >> > > >> >
>> > > > >> > > >>
>> > > > >> > > >
>> > > > >> > > >
>> > > > >> > >
>> > > > >> >
>> > > > >>
>> > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

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 <ew...@confluent.io>
wrote:

> On Mon, Jan 8, 2018 at 11:39 AM, Randall Hauch <rh...@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 <ja...@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 <ja...@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 <rh...@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 <yu...@gmail.com>
> > wrote:
> > > > >>
> > > > >> > +1 to this proposal.
> > > > >> >
> > > > >> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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
> > > > >> > > >> >> >
> > > > >> > > >> >>
> > > > >> > > >> >
> > > > >> > > >> >
> > > > >> > > >>
> > > > >> > > >
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Ewen Cheslack-Postava <ew...@confluent.io>]

On Mon, Jan 8, 2018 at 11:39 AM, Randall Hauch <rh...@gmail.com> wrote:

> Nice feedback, Ewen. Thanks!
>
> On Thu, Jan 4, 2018 at 5:11 PM, Ewen Cheslack-Postava <ew...@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 <ja...@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 <ja...@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 <rh...@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 <yu...@gmail.com>
> wrote:
> > > >>
> > > >> > +1 to this proposal.
> > > >> >
> > > >> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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
> > > >> > > >> >> >
> > > >> > > >> >>
> > > >> > > >> >
> > > >> > > >> >
> > > >> > > >>
> > > >> > > >
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Randall Hauch <rh...@gmail.com>]

Nice feedback, Ewen. Thanks!

On Thu, Jan 4, 2018 at 5:11 PM, Ewen Cheslack-Postava <ew...@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.


>
> 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 <ja...@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 <ja...@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 <rh...@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 <yu...@gmail.com> wrote:
> > >>
> > >> > +1 to this proposal.
> > >> >
> > >> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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 <ja...@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
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >
> > >> > > >> >
> > >> > > >>
> > >> > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Ewen Cheslack-Postava <ew...@confluent.io>]

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.

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?

I think these are mostly small details, overall it looks like a good plan!

Thanks,
Ewen

On Tue, Oct 24, 2017 at 5:19 AM, Jakub Scholz <ja...@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 <ja...@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 <rh...@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 <yu...@gmail.com> wrote:
> >>
> >> > +1 to this proposal.
> >> >
> >> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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 <ja...@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 <rh...@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
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >
> >> > > >> >
> >> > > >>
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

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 <ja...@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 <rh...@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 <yu...@gmail.com> wrote:
>>
>> > +1 to this proposal.
>> >
>> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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 <ja...@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 <rh...@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 <rh...@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 <ja...@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 <ja...@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
>> > > >> >> >
>> > > >> >>
>> > > >> >
>> > > >> >
>> > > >>
>> > > >
>> > > >
>> > >
>> >
>>
>
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

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 <rh...@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 <yu...@gmail.com> wrote:
>
> > +1 to this proposal.
> >
> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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.map"
> > > 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 <ja...@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 <rh...@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 <rh...@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 <ja...@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 <ja...@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
> > > >> >> >
> > > >> >>
> > > >> >
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Randall Hauch <rh...@gmail.com>]

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 <yu...@gmail.com> wrote:

> +1 to this proposal.
>
> On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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.map"
> > 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 <ja...@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 <rh...@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 <rh...@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 <ja...@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 <ja...@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
> > >> >> >
> > >> >>
> > >> >
> > >> >
> > >>
> > >
> > >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Ted Yu <yu...@gmail.com>]

+1 to this proposal.

On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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.map"
> 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 <ja...@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 <rh...@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 <rh...@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 <ja...@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 <ja...@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
> >> >> >
> >> >>
> >> >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

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.map"
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 <ja...@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 <rh...@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 <rh...@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 <ja...@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 <ja...@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
>> >> >
>> >>
>> >
>> >
>>
>
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

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 <rh...@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 <rh...@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 <ja...@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 <ja...@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
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Ted Yu <yu...@gmail.com>]

I agree with Randall.

Actually I had the same thought during first round of review.

On Fri, Oct 13, 2017 at 9:25 AM, Randall Hauch <rh...@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 <rh...@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 <ja...@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 <ja...@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
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Randall Hauch <rh...@gmail.com>]

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 <rh...@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 <ja...@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 <ja...@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
>> >
>>
>
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Randall Hauch <rh...@gmail.com>]

It'd be useful to specify the default values for the configuration
properties.

On Tue, Oct 10, 2017 at 2:53 AM, Jakub Scholz <ja...@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 <ja...@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
> >
>

Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface

[Jakub Scholz <ja...@scholz.cz>]

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