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