You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Vincent Meng <vm...@zefr.com> on 2017/12/18 19:14:24 UTC
[DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response
Hi all,
I've created KIP-242, a proposal to secure credentials in kafka connect
rest endpoint.
https://cwiki.apache.org/confluence/display/KAFKA/KIP-242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
Here are something I'd like to discuss:
- The "masked" value is set to "*********" (9 stars) currently. It's an
arbitrary value I picked. Are there any better options?
- The proposal change is in the
*org.apache.kafka.connect.runtime.rest.resources.ConnectorsResource*
class, where before the response is returned we go through config and mask
the password. This has been proven to work. However I think it's cleaner if
we do the masking in
*org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo* where
config() method can return the masked config, so that we don't have to mask
the value in each endpoint (and new endpoints if added in the future). I
ran into some issue with this. So after a while, I start seeing incorrect
password being used for the connector. My conjecture is that the value
stored in kafka has been changed to the mask value. Can someone confirm
this might happen with kafka connect? Feel like *ConnectorInfo.Config()*
is used somewhere to update connect config storage topic.
If there's any comments on the KIP let me know. Thank you very much.
-Vincent
Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response
Posted by Randall Hauch <rh...@gmail.com>.
See also
https://cwiki.apache.org/confluence/display/KAFKA/KIP-297%3A+Externalizing+Secrets+for+Connect+Configurations,
which just passed.
On Mon, Mar 19, 2018 at 11:16 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:
> SSL authentication was added in KIP-208, which will be included in Kafka
> 1.1.0:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 208%3A+Add+SSL+support+to+Kafka+Connect+REST+interface
>
> Connect isn't much different from the core Kafka/client configs currently
> where in some security setups you need to pass in passwords directly, and
> since there's various dynamic broker config improvements in the works, the
> fact that Connect exposes these in a REST API doesn't make it any
> different. I think the real long term solution to this is to add pluggable
> password support where you could, e.g., get these values out of a separate
> secrets management system instead of specifying them directly. Masking
> passwords as described in this solution feels like it's more of a temporary
> workaround and in order to be able to edit and update these connector
> configs by working with the REST API, we'd have to address these issues
> anyway.
>
> -Ewen
>
> On Mon, Mar 19, 2018 at 2:33 PM, Matt Farmer <ma...@frmr.me> wrote:
>
> > What’s the status of this? This is a pretty hard blocker for us to meet
> > requirements internally to deploy connect in a distributed fashion.
> >
> > @Ewen - Regarding the concern of accessing information securely - has
> > there been any consideration of adding authentication to the connect api?
> >
> > > On Jan 17, 2018, at 3:55 PM, Randall Hauch <rh...@gmail.com> wrote:
> > >
> > > Vincent,
> > >
> > > Can the KIP more explicitly say that this is opt-in, and that by
> default
> > > nothing will change?
> > >
> > > Randall
> > >
> > > On Tue, Jan 16, 2018 at 11:18 PM, Ewen Cheslack-Postava <
> > ewen@confluent.io>
> > > wrote:
> > >
> > >> Vincent,
> > >>
> > >> I think with the addition of a configuration to control this for
> > >> compatibility, people would generally be ok with it. If you want to
> > start a
> > >> VOTE thread, the KIP deadline is coming up and the PR looks pretty
> > small. I
> > >> will take a pass at reviewing the PR so we'll be ready to merge if we
> > can
> > >> get the KIP voted through.
> > >>
> > >> Thanks,
> > >> Ewen
> > >>
> > >> On Fri, Jan 12, 2018 at 10:18 AM, Vincent Meng <vm...@zefr.com>
> wrote:
> > >>
> > >>> @Ted: The issue is kinda hard to reproduce. It's just something we
> > >> observe
> > >>> over time.
> > >>>
> > >>> @Ewen: I agree. Opt-in seems to be a good solution to me. To your
> > >> question,
> > >>> if there is no ConfDef that defines which fields are Passwords we can
> > >> just
> > >>> return the config as is.
> > >>>
> > >>> There is a PR for this KIP already. Comments/Discussions are welcome.
> > >>> https://github.com/apache/kafka/pull/4269
> > >>>
> > >>> On Tue, Jan 2, 2018 at 8:52 PM, Ewen Cheslack-Postava <
> > ewen@confluent.io
> > >>>
> > >>> wrote:
> > >>>
> > >>>> Vincent,
> > >>>>
> > >>>> Thanks for the KIP. This is definitely an issue we know is a problem
> > >> for
> > >>>> some users.
> > >>>>
> > >>>> I think the major problem with the KIP as-is is that it makes it
> > >>> impossible
> > >>>> to get the original value back out of the API. This KIP probably
> ties
> > >> in
> > >>>> significantly with ideas for securing the REST API (SSL) and adding
> > >> ACLs
> > >>> to
> > >>>> it. Both are things we know people want, but haven't happened yet.
> > >>> However,
> > >>>> it also interacts with other approaches to adding those features,
> e.g.
> > >>>> layering proxies on top of the existing API (e.g. nginx, apache,
> etc).
> > >>> Just
> > >>>> doing a blanket replacement of password values with a constant would
> > >>> likely
> > >>>> break things for people who secure things via a proxy (and may just
> > not
> > >>>> allow reads of configs unless the user is authorized for the
> > particular
> > >>>> connector). These are the types of concerns we like to think through
> > in
> > >>> the
> > >>>> compatibility section. One option to get the masking functionality
> in
> > >>>> without depending on a bunch of other security improvements might be
> > to
> > >>>> make this configurable so users that need this (and can forgo
> seeing a
> > >>>> valid config via the API) can opt-in.
> > >>>>
> > >>>> Regarding your individual points:
> > >>>>
> > >>>> * I don't think the particular value for the masked content matters
> > >> much.
> > >>>> Any constant indicating a password field is good. Your value seems
> > fine
> > >>> to
> > >>>> me.
> > >>>> * I don't think ConnectorInfo has enough info on its own to do
> proper
> > >>>> masking. In fact, I think you need to parse the config enough to get
> > >> the
> > >>>> Connector-specific ConfigDef out in order to determine which fields
> > are
> > >>>> Password fields. I would probably try to push this to be as central
> as
> > >>>> possible, maybe adding a method to AbstractHerder that can get
> configs
> > >>> with
> > >>>> a boolean indicating whether they need to have sensitive fields
> > >> removed.
> > >>>> That method could deal with parsing the config to get the right
> > >>> connector,
> > >>>> getting the connector config, and then sanitizing any configs that
> are
> > >>>> sensitive. We could have this in one location, then have the
> relevant
> > >>> REST
> > >>>> APIs just use the right flag to determine if they get sanitized or
> > >>>> unsanitized data.
> > >>>>
> > >>>> That second point raises another interesting point -- what happens
> if
> > >> the
> > >>>> connector configuration references a connector which the worker
> > serving
> > >>> the
> > >>>> REST request *does not know about*? In that case, there will be no
> > >>>> corresponding ConfigDef that defines which fields are Passwords and
> > >> need
> > >>> to
> > >>>> be sensitized. Does it return an error? Or just return the config as
> > >> is?
> > >>>>
> > >>>> -Ewen
> > >>>>
> > >>>> On Thu, Dec 28, 2017 at 3:34 AM, Ted Yu <yu...@gmail.com>
> wrote:
> > >>>>
> > >>>>> For the last point you raised, can you come up with a unit test
> that
> > >>>> shows
> > >>>>> what you observed ?
> > >>>>>
> > >>>>> Cheers
> > >>>>>
> > >>>>> On Mon, Dec 18, 2017 at 11:14 AM, Vincent Meng <vm...@zefr.com>
> > >> wrote:
> > >>>>>
> > >>>>>> Hi all,
> > >>>>>>
> > >>>>>> I've created KIP-242, a proposal to secure credentials in kafka
> > >>> connect
> > >>>>>> rest endpoint.
> > >>>>>>
> > >>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >>>>>> 242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
> > >>>>>>
> > >>>>>> Here are something I'd like to discuss:
> > >>>>>>
> > >>>>>> - The "masked" value is set to "*********" (9 stars) currently.
> > >>> It's
> > >>>>> an
> > >>>>>> arbitrary value I picked. Are there any better options?
> > >>>>>> - The proposal change is in the
> > >>>>>> *org.apache.kafka.connect.runtime.rest.resources.
> > >>>> ConnectorsResource*
> > >>>>>> class, where before the response is returned we go through
> > >> config
> > >>>> and
> > >>>>>> mask
> > >>>>>> the password. This has been proven to work. However I think it's
> > >>>>>> cleaner if
> > >>>>>> we do the masking in
> > >>>>>> *org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo*
> > >>>> where
> > >>>>>> config() method can return the masked config, so that we don't
> > >>> have
> > >>>> to
> > >>>>>> mask
> > >>>>>> the value in each endpoint (and new endpoints if added in the
> > >>>>> future). I
> > >>>>>> ran into some issue with this. So after a while, I start seeing
> > >>>>>> incorrect
> > >>>>>> password being used for the connector. My conjecture is that the
> > >>>> value
> > >>>>>> stored in kafka has been changed to the mask value. Can someone
> > >>>>> confirm
> > >>>>>> this might happen with kafka connect? Feel like
> > >>>>> *ConnectorInfo.Config()*
> > >>>>>> is used somewhere to update connect config storage topic.
> > >>>>>>
> > >>>>>> If there's any comments on the KIP let me know. Thank you very
> > >> much.
> > >>>>>>
> > >>>>>> -Vincent
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> >
> >
>
Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response
Posted by Ewen Cheslack-Postava <ew...@confluent.io>.
SSL authentication was added in KIP-208, which will be included in Kafka
1.1.0:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-208%3A+Add+SSL+support+to+Kafka+Connect+REST+interface
Connect isn't much different from the core Kafka/client configs currently
where in some security setups you need to pass in passwords directly, and
since there's various dynamic broker config improvements in the works, the
fact that Connect exposes these in a REST API doesn't make it any
different. I think the real long term solution to this is to add pluggable
password support where you could, e.g., get these values out of a separate
secrets management system instead of specifying them directly. Masking
passwords as described in this solution feels like it's more of a temporary
workaround and in order to be able to edit and update these connector
configs by working with the REST API, we'd have to address these issues
anyway.
-Ewen
On Mon, Mar 19, 2018 at 2:33 PM, Matt Farmer <ma...@frmr.me> wrote:
> What’s the status of this? This is a pretty hard blocker for us to meet
> requirements internally to deploy connect in a distributed fashion.
>
> @Ewen - Regarding the concern of accessing information securely - has
> there been any consideration of adding authentication to the connect api?
>
> > On Jan 17, 2018, at 3:55 PM, Randall Hauch <rh...@gmail.com> wrote:
> >
> > Vincent,
> >
> > Can the KIP more explicitly say that this is opt-in, and that by default
> > nothing will change?
> >
> > Randall
> >
> > On Tue, Jan 16, 2018 at 11:18 PM, Ewen Cheslack-Postava <
> ewen@confluent.io>
> > wrote:
> >
> >> Vincent,
> >>
> >> I think with the addition of a configuration to control this for
> >> compatibility, people would generally be ok with it. If you want to
> start a
> >> VOTE thread, the KIP deadline is coming up and the PR looks pretty
> small. I
> >> will take a pass at reviewing the PR so we'll be ready to merge if we
> can
> >> get the KIP voted through.
> >>
> >> Thanks,
> >> Ewen
> >>
> >> On Fri, Jan 12, 2018 at 10:18 AM, Vincent Meng <vm...@zefr.com> wrote:
> >>
> >>> @Ted: The issue is kinda hard to reproduce. It's just something we
> >> observe
> >>> over time.
> >>>
> >>> @Ewen: I agree. Opt-in seems to be a good solution to me. To your
> >> question,
> >>> if there is no ConfDef that defines which fields are Passwords we can
> >> just
> >>> return the config as is.
> >>>
> >>> There is a PR for this KIP already. Comments/Discussions are welcome.
> >>> https://github.com/apache/kafka/pull/4269
> >>>
> >>> On Tue, Jan 2, 2018 at 8:52 PM, Ewen Cheslack-Postava <
> ewen@confluent.io
> >>>
> >>> wrote:
> >>>
> >>>> Vincent,
> >>>>
> >>>> Thanks for the KIP. This is definitely an issue we know is a problem
> >> for
> >>>> some users.
> >>>>
> >>>> I think the major problem with the KIP as-is is that it makes it
> >>> impossible
> >>>> to get the original value back out of the API. This KIP probably ties
> >> in
> >>>> significantly with ideas for securing the REST API (SSL) and adding
> >> ACLs
> >>> to
> >>>> it. Both are things we know people want, but haven't happened yet.
> >>> However,
> >>>> it also interacts with other approaches to adding those features, e.g.
> >>>> layering proxies on top of the existing API (e.g. nginx, apache, etc).
> >>> Just
> >>>> doing a blanket replacement of password values with a constant would
> >>> likely
> >>>> break things for people who secure things via a proxy (and may just
> not
> >>>> allow reads of configs unless the user is authorized for the
> particular
> >>>> connector). These are the types of concerns we like to think through
> in
> >>> the
> >>>> compatibility section. One option to get the masking functionality in
> >>>> without depending on a bunch of other security improvements might be
> to
> >>>> make this configurable so users that need this (and can forgo seeing a
> >>>> valid config via the API) can opt-in.
> >>>>
> >>>> Regarding your individual points:
> >>>>
> >>>> * I don't think the particular value for the masked content matters
> >> much.
> >>>> Any constant indicating a password field is good. Your value seems
> fine
> >>> to
> >>>> me.
> >>>> * I don't think ConnectorInfo has enough info on its own to do proper
> >>>> masking. In fact, I think you need to parse the config enough to get
> >> the
> >>>> Connector-specific ConfigDef out in order to determine which fields
> are
> >>>> Password fields. I would probably try to push this to be as central as
> >>>> possible, maybe adding a method to AbstractHerder that can get configs
> >>> with
> >>>> a boolean indicating whether they need to have sensitive fields
> >> removed.
> >>>> That method could deal with parsing the config to get the right
> >>> connector,
> >>>> getting the connector config, and then sanitizing any configs that are
> >>>> sensitive. We could have this in one location, then have the relevant
> >>> REST
> >>>> APIs just use the right flag to determine if they get sanitized or
> >>>> unsanitized data.
> >>>>
> >>>> That second point raises another interesting point -- what happens if
> >> the
> >>>> connector configuration references a connector which the worker
> serving
> >>> the
> >>>> REST request *does not know about*? In that case, there will be no
> >>>> corresponding ConfigDef that defines which fields are Passwords and
> >> need
> >>> to
> >>>> be sensitized. Does it return an error? Or just return the config as
> >> is?
> >>>>
> >>>> -Ewen
> >>>>
> >>>> On Thu, Dec 28, 2017 at 3:34 AM, Ted Yu <yu...@gmail.com> wrote:
> >>>>
> >>>>> For the last point you raised, can you come up with a unit test that
> >>>> shows
> >>>>> what you observed ?
> >>>>>
> >>>>> Cheers
> >>>>>
> >>>>> On Mon, Dec 18, 2017 at 11:14 AM, Vincent Meng <vm...@zefr.com>
> >> wrote:
> >>>>>
> >>>>>> Hi all,
> >>>>>>
> >>>>>> I've created KIP-242, a proposal to secure credentials in kafka
> >>> connect
> >>>>>> rest endpoint.
> >>>>>>
> >>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>>>> 242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
> >>>>>>
> >>>>>> Here are something I'd like to discuss:
> >>>>>>
> >>>>>> - The "masked" value is set to "*********" (9 stars) currently.
> >>> It's
> >>>>> an
> >>>>>> arbitrary value I picked. Are there any better options?
> >>>>>> - The proposal change is in the
> >>>>>> *org.apache.kafka.connect.runtime.rest.resources.
> >>>> ConnectorsResource*
> >>>>>> class, where before the response is returned we go through
> >> config
> >>>> and
> >>>>>> mask
> >>>>>> the password. This has been proven to work. However I think it's
> >>>>>> cleaner if
> >>>>>> we do the masking in
> >>>>>> *org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo*
> >>>> where
> >>>>>> config() method can return the masked config, so that we don't
> >>> have
> >>>> to
> >>>>>> mask
> >>>>>> the value in each endpoint (and new endpoints if added in the
> >>>>> future). I
> >>>>>> ran into some issue with this. So after a while, I start seeing
> >>>>>> incorrect
> >>>>>> password being used for the connector. My conjecture is that the
> >>>> value
> >>>>>> stored in kafka has been changed to the mask value. Can someone
> >>>>> confirm
> >>>>>> this might happen with kafka connect? Feel like
> >>>>> *ConnectorInfo.Config()*
> >>>>>> is used somewhere to update connect config storage topic.
> >>>>>>
> >>>>>> If there's any comments on the KIP let me know. Thank you very
> >> much.
> >>>>>>
> >>>>>> -Vincent
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
>
>
Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST
response
Posted by Matt Farmer <ma...@frmr.me>.
What’s the status of this? This is a pretty hard blocker for us to meet requirements internally to deploy connect in a distributed fashion.
@Ewen - Regarding the concern of accessing information securely - has there been any consideration of adding authentication to the connect api?
> On Jan 17, 2018, at 3:55 PM, Randall Hauch <rh...@gmail.com> wrote:
>
> Vincent,
>
> Can the KIP more explicitly say that this is opt-in, and that by default
> nothing will change?
>
> Randall
>
> On Tue, Jan 16, 2018 at 11:18 PM, Ewen Cheslack-Postava <ew...@confluent.io>
> wrote:
>
>> Vincent,
>>
>> I think with the addition of a configuration to control this for
>> compatibility, people would generally be ok with it. If you want to start a
>> VOTE thread, the KIP deadline is coming up and the PR looks pretty small. I
>> will take a pass at reviewing the PR so we'll be ready to merge if we can
>> get the KIP voted through.
>>
>> Thanks,
>> Ewen
>>
>> On Fri, Jan 12, 2018 at 10:18 AM, Vincent Meng <vm...@zefr.com> wrote:
>>
>>> @Ted: The issue is kinda hard to reproduce. It's just something we
>> observe
>>> over time.
>>>
>>> @Ewen: I agree. Opt-in seems to be a good solution to me. To your
>> question,
>>> if there is no ConfDef that defines which fields are Passwords we can
>> just
>>> return the config as is.
>>>
>>> There is a PR for this KIP already. Comments/Discussions are welcome.
>>> https://github.com/apache/kafka/pull/4269
>>>
>>> On Tue, Jan 2, 2018 at 8:52 PM, Ewen Cheslack-Postava <ewen@confluent.io
>>>
>>> wrote:
>>>
>>>> Vincent,
>>>>
>>>> Thanks for the KIP. This is definitely an issue we know is a problem
>> for
>>>> some users.
>>>>
>>>> I think the major problem with the KIP as-is is that it makes it
>>> impossible
>>>> to get the original value back out of the API. This KIP probably ties
>> in
>>>> significantly with ideas for securing the REST API (SSL) and adding
>> ACLs
>>> to
>>>> it. Both are things we know people want, but haven't happened yet.
>>> However,
>>>> it also interacts with other approaches to adding those features, e.g.
>>>> layering proxies on top of the existing API (e.g. nginx, apache, etc).
>>> Just
>>>> doing a blanket replacement of password values with a constant would
>>> likely
>>>> break things for people who secure things via a proxy (and may just not
>>>> allow reads of configs unless the user is authorized for the particular
>>>> connector). These are the types of concerns we like to think through in
>>> the
>>>> compatibility section. One option to get the masking functionality in
>>>> without depending on a bunch of other security improvements might be to
>>>> make this configurable so users that need this (and can forgo seeing a
>>>> valid config via the API) can opt-in.
>>>>
>>>> Regarding your individual points:
>>>>
>>>> * I don't think the particular value for the masked content matters
>> much.
>>>> Any constant indicating a password field is good. Your value seems fine
>>> to
>>>> me.
>>>> * I don't think ConnectorInfo has enough info on its own to do proper
>>>> masking. In fact, I think you need to parse the config enough to get
>> the
>>>> Connector-specific ConfigDef out in order to determine which fields are
>>>> Password fields. I would probably try to push this to be as central as
>>>> possible, maybe adding a method to AbstractHerder that can get configs
>>> with
>>>> a boolean indicating whether they need to have sensitive fields
>> removed.
>>>> That method could deal with parsing the config to get the right
>>> connector,
>>>> getting the connector config, and then sanitizing any configs that are
>>>> sensitive. We could have this in one location, then have the relevant
>>> REST
>>>> APIs just use the right flag to determine if they get sanitized or
>>>> unsanitized data.
>>>>
>>>> That second point raises another interesting point -- what happens if
>> the
>>>> connector configuration references a connector which the worker serving
>>> the
>>>> REST request *does not know about*? In that case, there will be no
>>>> corresponding ConfigDef that defines which fields are Passwords and
>> need
>>> to
>>>> be sensitized. Does it return an error? Or just return the config as
>> is?
>>>>
>>>> -Ewen
>>>>
>>>> On Thu, Dec 28, 2017 at 3:34 AM, Ted Yu <yu...@gmail.com> wrote:
>>>>
>>>>> For the last point you raised, can you come up with a unit test that
>>>> shows
>>>>> what you observed ?
>>>>>
>>>>> Cheers
>>>>>
>>>>> On Mon, Dec 18, 2017 at 11:14 AM, Vincent Meng <vm...@zefr.com>
>> wrote:
>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> I've created KIP-242, a proposal to secure credentials in kafka
>>> connect
>>>>>> rest endpoint.
>>>>>>
>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>> 242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
>>>>>>
>>>>>> Here are something I'd like to discuss:
>>>>>>
>>>>>> - The "masked" value is set to "*********" (9 stars) currently.
>>> It's
>>>>> an
>>>>>> arbitrary value I picked. Are there any better options?
>>>>>> - The proposal change is in the
>>>>>> *org.apache.kafka.connect.runtime.rest.resources.
>>>> ConnectorsResource*
>>>>>> class, where before the response is returned we go through
>> config
>>>> and
>>>>>> mask
>>>>>> the password. This has been proven to work. However I think it's
>>>>>> cleaner if
>>>>>> we do the masking in
>>>>>> *org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo*
>>>> where
>>>>>> config() method can return the masked config, so that we don't
>>> have
>>>> to
>>>>>> mask
>>>>>> the value in each endpoint (and new endpoints if added in the
>>>>> future). I
>>>>>> ran into some issue with this. So after a while, I start seeing
>>>>>> incorrect
>>>>>> password being used for the connector. My conjecture is that the
>>>> value
>>>>>> stored in kafka has been changed to the mask value. Can someone
>>>>> confirm
>>>>>> this might happen with kafka connect? Feel like
>>>>> *ConnectorInfo.Config()*
>>>>>> is used somewhere to update connect config storage topic.
>>>>>>
>>>>>> If there's any comments on the KIP let me know. Thank you very
>> much.
>>>>>>
>>>>>> -Vincent
>>>>>>
>>>>>
>>>>
>>>
>>
Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response
Posted by Randall Hauch <rh...@gmail.com>.
Vincent,
Can the KIP more explicitly say that this is opt-in, and that by default
nothing will change?
Randall
On Tue, Jan 16, 2018 at 11:18 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:
> Vincent,
>
> I think with the addition of a configuration to control this for
> compatibility, people would generally be ok with it. If you want to start a
> VOTE thread, the KIP deadline is coming up and the PR looks pretty small. I
> will take a pass at reviewing the PR so we'll be ready to merge if we can
> get the KIP voted through.
>
> Thanks,
> Ewen
>
> On Fri, Jan 12, 2018 at 10:18 AM, Vincent Meng <vm...@zefr.com> wrote:
>
> > @Ted: The issue is kinda hard to reproduce. It's just something we
> observe
> > over time.
> >
> > @Ewen: I agree. Opt-in seems to be a good solution to me. To your
> question,
> > if there is no ConfDef that defines which fields are Passwords we can
> just
> > return the config as is.
> >
> > There is a PR for this KIP already. Comments/Discussions are welcome.
> > https://github.com/apache/kafka/pull/4269
> >
> > On Tue, Jan 2, 2018 at 8:52 PM, Ewen Cheslack-Postava <ewen@confluent.io
> >
> > wrote:
> >
> > > Vincent,
> > >
> > > Thanks for the KIP. This is definitely an issue we know is a problem
> for
> > > some users.
> > >
> > > I think the major problem with the KIP as-is is that it makes it
> > impossible
> > > to get the original value back out of the API. This KIP probably ties
> in
> > > significantly with ideas for securing the REST API (SSL) and adding
> ACLs
> > to
> > > it. Both are things we know people want, but haven't happened yet.
> > However,
> > > it also interacts with other approaches to adding those features, e.g.
> > > layering proxies on top of the existing API (e.g. nginx, apache, etc).
> > Just
> > > doing a blanket replacement of password values with a constant would
> > likely
> > > break things for people who secure things via a proxy (and may just not
> > > allow reads of configs unless the user is authorized for the particular
> > > connector). These are the types of concerns we like to think through in
> > the
> > > compatibility section. One option to get the masking functionality in
> > > without depending on a bunch of other security improvements might be to
> > > make this configurable so users that need this (and can forgo seeing a
> > > valid config via the API) can opt-in.
> > >
> > > Regarding your individual points:
> > >
> > > * I don't think the particular value for the masked content matters
> much.
> > > Any constant indicating a password field is good. Your value seems fine
> > to
> > > me.
> > > * I don't think ConnectorInfo has enough info on its own to do proper
> > > masking. In fact, I think you need to parse the config enough to get
> the
> > > Connector-specific ConfigDef out in order to determine which fields are
> > > Password fields. I would probably try to push this to be as central as
> > > possible, maybe adding a method to AbstractHerder that can get configs
> > with
> > > a boolean indicating whether they need to have sensitive fields
> removed.
> > > That method could deal with parsing the config to get the right
> > connector,
> > > getting the connector config, and then sanitizing any configs that are
> > > sensitive. We could have this in one location, then have the relevant
> > REST
> > > APIs just use the right flag to determine if they get sanitized or
> > > unsanitized data.
> > >
> > > That second point raises another interesting point -- what happens if
> the
> > > connector configuration references a connector which the worker serving
> > the
> > > REST request *does not know about*? In that case, there will be no
> > > corresponding ConfigDef that defines which fields are Passwords and
> need
> > to
> > > be sensitized. Does it return an error? Or just return the config as
> is?
> > >
> > > -Ewen
> > >
> > > On Thu, Dec 28, 2017 at 3:34 AM, Ted Yu <yu...@gmail.com> wrote:
> > >
> > > > For the last point you raised, can you come up with a unit test that
> > > shows
> > > > what you observed ?
> > > >
> > > > Cheers
> > > >
> > > > On Mon, Dec 18, 2017 at 11:14 AM, Vincent Meng <vm...@zefr.com>
> wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > I've created KIP-242, a proposal to secure credentials in kafka
> > connect
> > > > > rest endpoint.
> > > > >
> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > 242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
> > > > >
> > > > > Here are something I'd like to discuss:
> > > > >
> > > > > - The "masked" value is set to "*********" (9 stars) currently.
> > It's
> > > > an
> > > > > arbitrary value I picked. Are there any better options?
> > > > > - The proposal change is in the
> > > > > *org.apache.kafka.connect.runtime.rest.resources.
> > > ConnectorsResource*
> > > > > class, where before the response is returned we go through
> config
> > > and
> > > > > mask
> > > > > the password. This has been proven to work. However I think it's
> > > > > cleaner if
> > > > > we do the masking in
> > > > > *org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo*
> > > where
> > > > > config() method can return the masked config, so that we don't
> > have
> > > to
> > > > > mask
> > > > > the value in each endpoint (and new endpoints if added in the
> > > > future). I
> > > > > ran into some issue with this. So after a while, I start seeing
> > > > > incorrect
> > > > > password being used for the connector. My conjecture is that the
> > > value
> > > > > stored in kafka has been changed to the mask value. Can someone
> > > > confirm
> > > > > this might happen with kafka connect? Feel like
> > > > *ConnectorInfo.Config()*
> > > > > is used somewhere to update connect config storage topic.
> > > > >
> > > > > If there's any comments on the KIP let me know. Thank you very
> much.
> > > > >
> > > > > -Vincent
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response
Posted by Ewen Cheslack-Postava <ew...@confluent.io>.
Vincent,
I think with the addition of a configuration to control this for
compatibility, people would generally be ok with it. If you want to start a
VOTE thread, the KIP deadline is coming up and the PR looks pretty small. I
will take a pass at reviewing the PR so we'll be ready to merge if we can
get the KIP voted through.
Thanks,
Ewen
On Fri, Jan 12, 2018 at 10:18 AM, Vincent Meng <vm...@zefr.com> wrote:
> @Ted: The issue is kinda hard to reproduce. It's just something we observe
> over time.
>
> @Ewen: I agree. Opt-in seems to be a good solution to me. To your question,
> if there is no ConfDef that defines which fields are Passwords we can just
> return the config as is.
>
> There is a PR for this KIP already. Comments/Discussions are welcome.
> https://github.com/apache/kafka/pull/4269
>
> On Tue, Jan 2, 2018 at 8:52 PM, Ewen Cheslack-Postava <ew...@confluent.io>
> wrote:
>
> > Vincent,
> >
> > Thanks for the KIP. This is definitely an issue we know is a problem for
> > some users.
> >
> > I think the major problem with the KIP as-is is that it makes it
> impossible
> > to get the original value back out of the API. This KIP probably ties in
> > significantly with ideas for securing the REST API (SSL) and adding ACLs
> to
> > it. Both are things we know people want, but haven't happened yet.
> However,
> > it also interacts with other approaches to adding those features, e.g.
> > layering proxies on top of the existing API (e.g. nginx, apache, etc).
> Just
> > doing a blanket replacement of password values with a constant would
> likely
> > break things for people who secure things via a proxy (and may just not
> > allow reads of configs unless the user is authorized for the particular
> > connector). These are the types of concerns we like to think through in
> the
> > compatibility section. One option to get the masking functionality in
> > without depending on a bunch of other security improvements might be to
> > make this configurable so users that need this (and can forgo seeing a
> > valid config via the API) can opt-in.
> >
> > Regarding your individual points:
> >
> > * I don't think the particular value for the masked content matters much.
> > Any constant indicating a password field is good. Your value seems fine
> to
> > me.
> > * I don't think ConnectorInfo has enough info on its own to do proper
> > masking. In fact, I think you need to parse the config enough to get the
> > Connector-specific ConfigDef out in order to determine which fields are
> > Password fields. I would probably try to push this to be as central as
> > possible, maybe adding a method to AbstractHerder that can get configs
> with
> > a boolean indicating whether they need to have sensitive fields removed.
> > That method could deal with parsing the config to get the right
> connector,
> > getting the connector config, and then sanitizing any configs that are
> > sensitive. We could have this in one location, then have the relevant
> REST
> > APIs just use the right flag to determine if they get sanitized or
> > unsanitized data.
> >
> > That second point raises another interesting point -- what happens if the
> > connector configuration references a connector which the worker serving
> the
> > REST request *does not know about*? In that case, there will be no
> > corresponding ConfigDef that defines which fields are Passwords and need
> to
> > be sensitized. Does it return an error? Or just return the config as is?
> >
> > -Ewen
> >
> > On Thu, Dec 28, 2017 at 3:34 AM, Ted Yu <yu...@gmail.com> wrote:
> >
> > > For the last point you raised, can you come up with a unit test that
> > shows
> > > what you observed ?
> > >
> > > Cheers
> > >
> > > On Mon, Dec 18, 2017 at 11:14 AM, Vincent Meng <vm...@zefr.com> wrote:
> > >
> > > > Hi all,
> > > >
> > > > I've created KIP-242, a proposal to secure credentials in kafka
> connect
> > > > rest endpoint.
> > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > 242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
> > > >
> > > > Here are something I'd like to discuss:
> > > >
> > > > - The "masked" value is set to "*********" (9 stars) currently.
> It's
> > > an
> > > > arbitrary value I picked. Are there any better options?
> > > > - The proposal change is in the
> > > > *org.apache.kafka.connect.runtime.rest.resources.
> > ConnectorsResource*
> > > > class, where before the response is returned we go through config
> > and
> > > > mask
> > > > the password. This has been proven to work. However I think it's
> > > > cleaner if
> > > > we do the masking in
> > > > *org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo*
> > where
> > > > config() method can return the masked config, so that we don't
> have
> > to
> > > > mask
> > > > the value in each endpoint (and new endpoints if added in the
> > > future). I
> > > > ran into some issue with this. So after a while, I start seeing
> > > > incorrect
> > > > password being used for the connector. My conjecture is that the
> > value
> > > > stored in kafka has been changed to the mask value. Can someone
> > > confirm
> > > > this might happen with kafka connect? Feel like
> > > *ConnectorInfo.Config()*
> > > > is used somewhere to update connect config storage topic.
> > > >
> > > > If there's any comments on the KIP let me know. Thank you very much.
> > > >
> > > > -Vincent
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response
Posted by Vincent Meng <vm...@zefr.com>.
@Ted: The issue is kinda hard to reproduce. It's just something we observe
over time.
@Ewen: I agree. Opt-in seems to be a good solution to me. To your question,
if there is no ConfDef that defines which fields are Passwords we can just
return the config as is.
There is a PR for this KIP already. Comments/Discussions are welcome.
https://github.com/apache/kafka/pull/4269
On Tue, Jan 2, 2018 at 8:52 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:
> Vincent,
>
> Thanks for the KIP. This is definitely an issue we know is a problem for
> some users.
>
> I think the major problem with the KIP as-is is that it makes it impossible
> to get the original value back out of the API. This KIP probably ties in
> significantly with ideas for securing the REST API (SSL) and adding ACLs to
> it. Both are things we know people want, but haven't happened yet. However,
> it also interacts with other approaches to adding those features, e.g.
> layering proxies on top of the existing API (e.g. nginx, apache, etc). Just
> doing a blanket replacement of password values with a constant would likely
> break things for people who secure things via a proxy (and may just not
> allow reads of configs unless the user is authorized for the particular
> connector). These are the types of concerns we like to think through in the
> compatibility section. One option to get the masking functionality in
> without depending on a bunch of other security improvements might be to
> make this configurable so users that need this (and can forgo seeing a
> valid config via the API) can opt-in.
>
> Regarding your individual points:
>
> * I don't think the particular value for the masked content matters much.
> Any constant indicating a password field is good. Your value seems fine to
> me.
> * I don't think ConnectorInfo has enough info on its own to do proper
> masking. In fact, I think you need to parse the config enough to get the
> Connector-specific ConfigDef out in order to determine which fields are
> Password fields. I would probably try to push this to be as central as
> possible, maybe adding a method to AbstractHerder that can get configs with
> a boolean indicating whether they need to have sensitive fields removed.
> That method could deal with parsing the config to get the right connector,
> getting the connector config, and then sanitizing any configs that are
> sensitive. We could have this in one location, then have the relevant REST
> APIs just use the right flag to determine if they get sanitized or
> unsanitized data.
>
> That second point raises another interesting point -- what happens if the
> connector configuration references a connector which the worker serving the
> REST request *does not know about*? In that case, there will be no
> corresponding ConfigDef that defines which fields are Passwords and need to
> be sensitized. Does it return an error? Or just return the config as is?
>
> -Ewen
>
> On Thu, Dec 28, 2017 at 3:34 AM, Ted Yu <yu...@gmail.com> wrote:
>
> > For the last point you raised, can you come up with a unit test that
> shows
> > what you observed ?
> >
> > Cheers
> >
> > On Mon, Dec 18, 2017 at 11:14 AM, Vincent Meng <vm...@zefr.com> wrote:
> >
> > > Hi all,
> > >
> > > I've created KIP-242, a proposal to secure credentials in kafka connect
> > > rest endpoint.
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
> > >
> > > Here are something I'd like to discuss:
> > >
> > > - The "masked" value is set to "*********" (9 stars) currently. It's
> > an
> > > arbitrary value I picked. Are there any better options?
> > > - The proposal change is in the
> > > *org.apache.kafka.connect.runtime.rest.resources.
> ConnectorsResource*
> > > class, where before the response is returned we go through config
> and
> > > mask
> > > the password. This has been proven to work. However I think it's
> > > cleaner if
> > > we do the masking in
> > > *org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo*
> where
> > > config() method can return the masked config, so that we don't have
> to
> > > mask
> > > the value in each endpoint (and new endpoints if added in the
> > future). I
> > > ran into some issue with this. So after a while, I start seeing
> > > incorrect
> > > password being used for the connector. My conjecture is that the
> value
> > > stored in kafka has been changed to the mask value. Can someone
> > confirm
> > > this might happen with kafka connect? Feel like
> > *ConnectorInfo.Config()*
> > > is used somewhere to update connect config storage topic.
> > >
> > > If there's any comments on the KIP let me know. Thank you very much.
> > >
> > > -Vincent
> > >
> >
>
Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response
Posted by Ewen Cheslack-Postava <ew...@confluent.io>.
Vincent,
Thanks for the KIP. This is definitely an issue we know is a problem for
some users.
I think the major problem with the KIP as-is is that it makes it impossible
to get the original value back out of the API. This KIP probably ties in
significantly with ideas for securing the REST API (SSL) and adding ACLs to
it. Both are things we know people want, but haven't happened yet. However,
it also interacts with other approaches to adding those features, e.g.
layering proxies on top of the existing API (e.g. nginx, apache, etc). Just
doing a blanket replacement of password values with a constant would likely
break things for people who secure things via a proxy (and may just not
allow reads of configs unless the user is authorized for the particular
connector). These are the types of concerns we like to think through in the
compatibility section. One option to get the masking functionality in
without depending on a bunch of other security improvements might be to
make this configurable so users that need this (and can forgo seeing a
valid config via the API) can opt-in.
Regarding your individual points:
* I don't think the particular value for the masked content matters much.
Any constant indicating a password field is good. Your value seems fine to
me.
* I don't think ConnectorInfo has enough info on its own to do proper
masking. In fact, I think you need to parse the config enough to get the
Connector-specific ConfigDef out in order to determine which fields are
Password fields. I would probably try to push this to be as central as
possible, maybe adding a method to AbstractHerder that can get configs with
a boolean indicating whether they need to have sensitive fields removed.
That method could deal with parsing the config to get the right connector,
getting the connector config, and then sanitizing any configs that are
sensitive. We could have this in one location, then have the relevant REST
APIs just use the right flag to determine if they get sanitized or
unsanitized data.
That second point raises another interesting point -- what happens if the
connector configuration references a connector which the worker serving the
REST request *does not know about*? In that case, there will be no
corresponding ConfigDef that defines which fields are Passwords and need to
be sensitized. Does it return an error? Or just return the config as is?
-Ewen
On Thu, Dec 28, 2017 at 3:34 AM, Ted Yu <yu...@gmail.com> wrote:
> For the last point you raised, can you come up with a unit test that shows
> what you observed ?
>
> Cheers
>
> On Mon, Dec 18, 2017 at 11:14 AM, Vincent Meng <vm...@zefr.com> wrote:
>
> > Hi all,
> >
> > I've created KIP-242, a proposal to secure credentials in kafka connect
> > rest endpoint.
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
> >
> > Here are something I'd like to discuss:
> >
> > - The "masked" value is set to "*********" (9 stars) currently. It's
> an
> > arbitrary value I picked. Are there any better options?
> > - The proposal change is in the
> > *org.apache.kafka.connect.runtime.rest.resources.ConnectorsResource*
> > class, where before the response is returned we go through config and
> > mask
> > the password. This has been proven to work. However I think it's
> > cleaner if
> > we do the masking in
> > *org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo* where
> > config() method can return the masked config, so that we don't have to
> > mask
> > the value in each endpoint (and new endpoints if added in the
> future). I
> > ran into some issue with this. So after a while, I start seeing
> > incorrect
> > password being used for the connector. My conjecture is that the value
> > stored in kafka has been changed to the mask value. Can someone
> confirm
> > this might happen with kafka connect? Feel like
> *ConnectorInfo.Config()*
> > is used somewhere to update connect config storage topic.
> >
> > If there's any comments on the KIP let me know. Thank you very much.
> >
> > -Vincent
> >
>
Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response
Posted by Ted Yu <yu...@gmail.com>.
For the last point you raised, can you come up with a unit test that shows
what you observed ?
Cheers
On Mon, Dec 18, 2017 at 11:14 AM, Vincent Meng <vm...@zefr.com> wrote:
> Hi all,
>
> I've created KIP-242, a proposal to secure credentials in kafka connect
> rest endpoint.
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 242%3A+Mask+password+in+Kafka+Connect+Rest+API+response
>
> Here are something I'd like to discuss:
>
> - The "masked" value is set to "*********" (9 stars) currently. It's an
> arbitrary value I picked. Are there any better options?
> - The proposal change is in the
> *org.apache.kafka.connect.runtime.rest.resources.ConnectorsResource*
> class, where before the response is returned we go through config and
> mask
> the password. This has been proven to work. However I think it's
> cleaner if
> we do the masking in
> *org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo* where
> config() method can return the masked config, so that we don't have to
> mask
> the value in each endpoint (and new endpoints if added in the future). I
> ran into some issue with this. So after a while, I start seeing
> incorrect
> password being used for the connector. My conjecture is that the value
> stored in kafka has been changed to the mask value. Can someone confirm
> this might happen with kafka connect? Feel like *ConnectorInfo.Config()*
> is used somewhere to update connect config storage topic.
>
> If there's any comments on the KIP let me know. Thank you very much.
>
> -Vincent
>