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 <e...@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 <e...@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 <yuzhih...@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 > > > > > > > > > > > > > > >
