Re: [DISCUSS] KIP-242: Mask password fields in Kafka Connect REST response

[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
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> >
> >
>