Hi Ron, I think we should be able to process substitutions for both static JAAS configuration file as well as `sasl.jaas.config` property. We load the configuration using org.apache.kafka.common.security. JaasContext.loadXXXContext() and that would be a good place to do any substitution. The method has access to the producer/consumer/broker configs as well in case we want keys to refer to these.
On Fri, Apr 13, 2018 at 2:15 PM, Ron Dagostino <rndg...@gmail.com> wrote: > Hi Rajini. Regarding processing the sasl.jaas.config value up-front, there > are a couple of things that occur to me about it. First, the older way of > storing the JAAS config in a separate file is still supported (and is at > this time the prevalent mechanism on the broker side since sasl.jaas.config > support for brokers was only recently added via KIP 226). We could state > that substitution is only supported via sasl.jaas.config and force people > to convert over to get substitution functionality, but that wouldn't be > necessary if we let the login module do the substitution later on. > > The second thing that occurs to me is related to namespacing and creates > tension with the first point above. If we refer to the "fubar" key in the > config, is that key a JAAS module option or is it a value in the > cluster/producer/consumer config? It would be very positive if we could > eliminate namespacing entirely such that when you reference another key it > is always very clear what is being referred to -- i.e. always a key in the > cluster/producer/consumer config. Otherwise the docs have to spell out > when it is one versus the other. > > That is a good point about being able to provide substitution support to > SASL/GSSAPI (which relies upon login module code that we do not control) if > we choose the simple, consistent way of doing things up front. > > You asked if there are OAuth use cases that require substitutions to be > performed in a login module that cannot be done if the substitution is > performed when the configuration is parsed. I don't think so, no; the > timing should not matter. > > I hadn't thought about the listener prefix issue. I don't know that area > of the code very well, but I have looked enough to guess that the > underlying "originals" map in AbstractConfig is what we would want to use > when making a reference to something. It would eliminate the listener > prefix namespacing confusion if we always refer to the key as originally > provided. > > I'm willing to go with doing substitution once, up-front, at the > cluster/producer/consumer config level, and supporting substitution for > JAAS configs only when provided via sasl.jaas.config. I'm willing to try > the coding to introduce it at that level -- tentative given my > unfamiliarity with the code and its subtleties, but willing to try. Let me > chew on it for a day or two and see what I can make happen. In case you > want to try as well, you can pull the current implementation (which I think > is in good shape and might only need cosmetic/stylistic code review changes > as opposed to wholesale API adjustments) from the KAFKA-6664 branch of > https://github.com/rondagostino/kafka.git. > > Ron > > On Fri, Apr 13, 2018 at 7:49 AM, Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > > > Hi Ron, > > > > Thanks for the notes and KIP update. > > > > Handling `sasl.jaas.config` as a special case is fine, but it would be > > better if we can do any substitutions before we create a `Configuration` > > object rather than expect the login module to do the substitution. That > > way, we will have a consistent substitution format for all login modules > > including built-in ones. And since we have users who already have their > own > > login modules (before KIP-86), they will benefit from substitution too > > without adding additional code to the login module, But you have thought > > about this more in the context of OAuth, so this is more of a question. > Are > > there use cases that require substitutions to be performed in a login > > module that cannot be done if the substitution is performed when the > > configuration is parsed? > > > > The ability to refer to other keys is generally quite useful. But as you > > said, "*there is a namespacing of sorts going on*". Even with regular > > configs, we have listener prefix, which is also a "*namespacing of > sorts"*. > > Our current config framework doesn't represent these well. As you already > > noticed before, there is magic that removes prefixes, flattening the > > namespace. Perhaps that is not an issue if we want to allow references to > > keys that are in the global namespace (non-listener-prefixed) as well. > But > > we probably want to make sure namespaces are handled consistently for ` > > sasl.jaas.config` and other configs. > > > > > > > > On Tue, Apr 10, 2018 at 3:41 AM, Ron Dagostino <rndg...@gmail.com> > wrote: > > > > > Hi folks. I updated KIP 269 to help clarify some of the issues > mentioned > > > previously. In particular, I added a new single-method > UnderlyingValues > > > interface to make it clear how data is to be provided to > > > SubstitutableValues, and I added information about if/how the > underlying > > > values might be re-read in case they can potentially change (basically > an > > > instance of SubstitutableValues never re-reads anything, so if the > > > underlying values are expected to change a new instance of > > > SubstitutableValues must be allocated in order to have any chance of > > seeing > > > those changes). I kept the KIP focused on the same JAAS use case for > > now, > > > but these additions/clarifications should help if we want to expand the > > > scope to cluster/producer/consumer configs. > > > > > > Ron > > > > > > On Mon, Apr 9, 2018 at 11:22 AM, Ron Dagostino <rndg...@gmail.com> > > wrote: > > > > > > > Hi folks. Here is a summary of where I think we stand on this KIP > and > > > > what I believe it means for how we move forward. > > > > > > > > > > > > - There is some desire to use substitution more broadly beyond > just > > > > JAAS module options. Specifically, cluster/producer/consumer > config > > > values > > > > such as ssl.keystore.password are places where substitution adds > > value > > > > (dormant KIP 76 > > > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > 76+Enable+getting+password+from+executable+rather+than+ > > > passing+as+plaintext+in+config+files> > > > > was an attempt to add value here in the past). > > > > - *More broad review of this KIP is needed given the potential for > > its > > > > expanded scope* > > > > - If substitution is applied more broadly, then the > sasl.jaas.config > > > > value should not have substitution performed on it at the same > times > > > as > > > > other cluster/producer/consumer configs; that value should be > passed > > > > unchanged to the login module where substitution can be applied > > later. > > > > - There are some adjustments to this KIP that should be made to > > > > reflect the possibility of more broad use: > > > > > > > > > > > > 1. The use of delimiters that trigger a substitution attempt but > > that > > > > fail to parse should result in the text being passed through > > > unchanged > > > > instead of raising an exception > > > > 2. The application of substitution should generally be on an > > opt-in > > > > basis > > > > 3. The implicit fact that substitution was associated with a > > > > namespace of sorts (i.e. saying that a default came from a > > > particular key > > > > meant a JAAS module option) needs to be made explicit. The > > > namespace is > > > > defined by the Map that is passed into the > SubstitutableValues() > > > constructor > > > > 4. It is not clear to me if the Map that is passed into the > > > > SubstitutableValues() constructor can be relied upon to contain > > > only String > > > > values in the context of cluster/producer/consumer configs. > The > > > > AbstractConfig's so-called "originals" map seems to support > > values > > > of type > > > > String, Boolean, Password, Integer, Short, Long, Number, List, > > and > > > Class. > > > > It is not difficult to support non-String values in the Map > that > > > is passed > > > > to the SubstitutableValues() constructor, so I can explicitly > add > > > support > > > > for that. > > > > > > > > I don't think these changes impact usage in a JAAS context, so they > do > > no > > > > harm to the original use case while increasing the potential for more > > > broad > > > > use of the concept. > > > > > > > > > > > > Ron > > > > > > > > > > > > On Sun, Apr 8, 2018 at 8:46 PM, Ron Dagostino <rndg...@gmail.com> > > wrote: > > > > > > > >> Hi Rajini. I've also been thinking about how sasl.jaas.config will > be > > > >> parsed. Something that is implicit in the current proposal needs to > > be > > > >> made explicit if this is to be applied more broadly, and that is the > > > fact > > > >> that there is a namespacing of sorts going on. For example, in the > > > current > > > >> proposal, when you indicate that you want to somehow refer to > another > > > value > > > >> (via defaultKey=<key> or fromValueOfKey) then the key being referred > > to > > > is > > > >> taken as a JAAS module option name. If we allow substitution at the > > > >> cluster/producer/consumer config level then within the context of > > > >> something like ssl.keystore.password any such key being referred to > > > >> would be a cluster/producer/consumer config key. But I think within > > the > > > >> context of sasl.jaas.config any key reference should still be > > referring > > > >> to a JAAS module option. I think sasl.jaas.config would need to be > > > >> special-cased in the sense that its value would not have > substitution > > > >> applied to it up front but instead would have substitution applied > to > > it > > > >> later on. In other words, the login module would be where the > > > substitution > > > >> logic is applied. Note that we re-use an existing kerberos login > > > module, > > > >> so we do not control that code and cannot apply substitution logic > > > there, > > > >> so I think the statement regarding if/how sasl.jaas.config (or any > > JAAS > > > >> configuration file) is processed with respect to substitution is to > > say > > > >> that it is determined by the login module. > > > >> > > > >> I think the chances of an unintended substitution accidentally > parsing > > > >> correctly is pretty close to zero, but making substitution an opt-in > > > means > > > >> the possibility -- regardless of how small it might be -- will be > > > >> explicitly acknowledged. I think that makes it fine. > > > >> > > > >> I suspect you are right that adding substitution into > > > cluster/producer/consumer > > > >> configs will require careful code changes given how configs are > > > >> currently implemented. I will take a closer look to see how it > might > > be > > > >> done; if it isn't obvious to me then perhaps it would be best to > split > > > that > > > >> change out into a separate JIRA issue so that someone with more > > > experience > > > >> with that part of the code can do it. But I'll still take a look > just > > > in > > > >> case I can see how it should be done. > > > >> > > > >> I agree that the OAuth callback handler that needs to be written > > anyway > > > >> could simply go out and talk directly to a password vault. With > > > >> substitution as an option, though, the callback handler can just ask > > for > > > >> the value from a JAAS module option, and then whether that goes out > > to a > > > >> password vault or not would be up to how the app is configured. I > > think > > > >> the ability to encapsulate the actual mechanism behind a module > option > > > is > > > >> valuable. > > > >> > > > >> Ron > > > >> > > > >> On Sun, Apr 8, 2018 at 4:47 AM, Rajini Sivaram < > > rajinisiva...@gmail.com > > > > > > > >> wrote: > > > >> > > > >>> Hi Ron, > > > >>> > > > >>> Thanks for the responses. > > > >>> > > > >>> For broader use as configs, opt-in makes sense to avoid any > surprises > > > >>> and a > > > >>> global opt-in ought to be fine. > > > >>> > > > >>> If we do want to use this for all configs, a few things to > consider: > > > >>> > > > >>> - How will sasl.jaas.config property will get parsed? This is > > > >>> essentially a compound config which may contain some options > that > > > are > > > >>> substitutable. Will it be possible to handle this and static > JAAS > > > >>> configuration files in the same way? > > > >>> - At the moment I think the whole option is redacted if one > > > >>> substitution > > > >>> is marked redact. Would it make sense to define values that > > consist > > > of > > > >>> some redactable components. I am thinking of sasl.jaas.config > as a > > > >>> single property, but perhaps treating this alone separately is > > good > > > >>> enough. > > > >>> - If we did treat failed substitutions as pass-thru, would it > > cover > > > >>> all > > > >>> cases, or do we also need to be concerned about valid > > substitutions > > > >>> that > > > >>> weren't intended to be substitutions? I am thinking that we > don't > > > >>> need to > > > >>> worry about the latter if substitutions are only by opt-in. > > > >>> - It will be good to get more feedback on this KIP before > updating > > > the > > > >>> code to use it for all configs since the code may need to change > > > >>> quite a > > > >>> bit to fit in with the config classes. > > > >>> > > > >>> > > > >>> For the callbacks, I agree that we want a LoginModule for OAuth > that > > > can > > > >>> be > > > >>> reused. But to use OAuth, you will probably have your own callback > > > >>> handler > > > >>> implementation to process OAuthBearerLoginCallback . From the > > example, > > > it > > > >>> is not clear to me why the callback handler that processes > > > >>> OAuthBearerLoginCallback cannot do whatever a custom substitution > > class > > > >>> would do, e,g. read some options like passwordVaultUrl from the > JAAS > > > >>> config > > > >>> (which it has access to) and retrieve passwords from a password > > vault? > > > If > > > >>> we are going to have extensible substitution anyway, then > obviously, > > we > > > >>> could use that as an option here too. > > > >>> > > > >>> > > > >>> > > > >>> On Fri, Apr 6, 2018 at 2:47 PM, Ron Dagostino <rndg...@gmail.com> > > > wrote: > > > >>> > > > >>> > Hi folks. I think there are a couple of issues that were just > > raised > > > >>> in > > > >>> > this thread. One is whether the ability to use PasswordCallback > > > >>> exists, > > > >>> > and if so whether that impacts the applicability of this KIP to > the > > > >>> > SASL/OAUTHBEARER KIP-255. The second issue is related to how we > > > might > > > >>> > leverage this KIP more broadly (including as an alternative to > > > KIP-76) > > > >>> > while maintaining forward compatibility and not causing > unexpected > > > >>> > substitutions/parsing exceptions. > > > >>> > > > > >>> > Let me address the second issue (more broad use) first, since I > > think > > > >>> > Rajini hit on a good possibility. Currently this KIP addresses > the > > > >>> > possibility of an unexpected substitution by saying "This would > > > cause a > > > >>> > substitution to be attempted, which of course would fail and > raise > > an > > > >>> > exception." I think Rajini's idea is to explicitly state that > any > > > >>> > substitution that cannot be parsed is to be treated as a > pass-thru > > > or a > > > >>> > no-op. So, for example, if a configured password happened to > look > > > like > > > >>> > "Asd$[,mhsd_4]Q" and a substitution was attempted on that value > > then > > > >>> the > > > >>> > result should not be an exception simply because "$[,mhsd_4]" > > > couldn't > > > >>> be > > > >>> > parsed according to the required delimited syntax but should > > instead > > > >>> just > > > >>> > end up doing nothing and the password would > remain"Asd$[,mhsd_4]Q". > > > >>> > Rajini, do I have that right? If so, then I think it is worth > > > >>> considering > > > >>> > the possibility that substitution could be turned on more broadly > > > with > > > >>> an > > > >>> > acceptably low risk. In the interest of caution substitution > could > > > >>> still > > > >>> > be turned on only as an opt-in, but it could be a global opt-in > if > > we > > > >>> > explicitly take a "do no harm" approach to things that have > > > delimiters > > > >>> but > > > >>> > do not parse as valid substitutions. > > > >>> > > > > >>> > Regarding whether the ability to use PasswordCallback exists in > > > >>> > SASL/OAUTHBEARER KIP-255, I don't think it does. The reason is > > > because > > > >>> > customers do not generally write the login module implementation; > > > they > > > >>> use > > > >>> > the implementation provided, which is short and delegates the > token > > > >>> > retrieval to the callback handler (which users are expected to > > > >>> provide). > > > >>> > Here is the login module code: > > > >>> > > > > >>> > @Override > > > >>> > public boolean login() throws LoginException { > > > >>> > OAuthBearerLoginCallback callback = new > > > >>> OAuthBearerLoginCallback(); > > > >>> > try { > > > >>> > this.callbackHandler.handle(new Callback[] > > {callback}); > > > >>> > } catch (IOException | UnsupportedCallbackException e) { > > > >>> > log.error(e.getMessage(), e); > > > >>> > throw new LoginException("An internal error > occurred"); > > > >>> > } > > > >>> > token = callback.token(); > > > >>> > if (token == null) { > > > >>> > log.info(String.format("Login failed: %s : %s > > (URI=%s)", > > > >>> > callback.errorCode(), callback.errorDescription(), > > > >>> > callback.errorUri())); > > > >>> > throw new LoginException(callback. > errorDescription()); > > > >>> > } > > > >>> > log.info("Login succeeded"); > > > >>> > return true; > > > >>> > } > > > >>> > > > > >>> > I don't see the callbackHandler using a PasswordCallback instance > > to > > > >>> ask > > > >>> > (itself?) to retrieve a password. If the callbackHandler needs a > > > >>> password, > > > >>> > then I imagine it will get that password from a login module > > option, > > > >>> and > > > >>> > that could in turn come from a file, environment variable, > password > > > >>> vault, > > > >>> > etc. if substitution is applicable. > > > >>> > > > > >>> > Ron > > > >>> > > > > >>> > > > > >>> > > > > >>> > On Fri, Apr 6, 2018 at 4:41 AM, Rajini Sivaram < > > > >>> rajinisiva...@gmail.com> > > > >>> > wrote: > > > >>> > > > > >>> > > Yes, I was going to suggest that we should do this for all > > configs > > > >>> > earlier, > > > >>> > > but was reluctant to do that since in its current form, there > is > > a > > > >>> > > property enableSubstitution > > > >>> > > (in JAAS config at the moment) that indicates if substitution > is > > to > > > >>> be > > > >>> > > performed. If enabled, all values in that config are considered > > for > > > >>> > > substitution. That works for JAAS configs with a small number > of > > > >>> > > properties, but I wasn't sure it was reasonable to do this for > > all > > > >>> Kafka > > > >>> > > configs where there are several configs that may contain > > arbitrary > > > >>> > > characters including substitution delimiters. It will be good > if > > > some > > > >>> > > configs that contain arbitrary characters can be specified > > directly > > > >>> in > > > >>> > the > > > >>> > > config while others are substituted from elsewhere. Perhaps a > > > >>> > substitution > > > >>> > > type that simply uses the value within delimiters would work? > > Ron, > > > >>> what > > > >>> > do > > > >>> > > you think? > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> > > On Fri, Apr 6, 2018 at 7:49 AM, Manikumar < > > > manikumar.re...@gmail.com > > > >>> > > > > >>> > > wrote: > > > >>> > > > > > >>> > > > Hi, > > > >>> > > > > > > >>> > > > Substitution mechanism can be useful to configure regular > > > password > > > >>> > > configs > > > >>> > > > liken ssl.keystore.password, ssl.truststore.password, etc. > > > >>> > > > This is can be good alternative to previously proposed KIP-76 > > and > > > >>> will > > > >>> > > give > > > >>> > > > more options to the user. > > > >>> > > > > > > >>> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > >>> > > > 76+Enable+getting+password+from+executable+rather+than+ > > > >>> > > > passing+as+plaintext+in+config+files > > > >>> > > > > > > >>> > > > > > > >>> > > > Thanks, > > > >>> > > > > > > >>> > > > On Fri, Apr 6, 2018 at 4:29 AM, Rajini Sivaram < > > > >>> > rajinisiva...@gmail.com> > > > >>> > > > wrote: > > > >>> > > > > > > >>> > > > > Hi Ron, > > > >>> > > > > > > > >>> > > > > For the password example, you could define a login > > > >>> CallbackHandler > > > >>> > that > > > >>> > > > > processes PasswordCallback to provide passwords. We don't > > > >>> currently > > > >>> > do > > > >>> > > > this > > > >>> > > > > with PLAIN/SCRAM because login callback handlers were not > > > >>> > configurable > > > >>> > > > > earlier and we haven't updated the login modules to do > this. > > > But > > > >>> that > > > >>> > > > could > > > >>> > > > > be one way of providing passwords and integrating with > other > > > >>> password > > > >>> > > > > sources, now that we have configurable login callback > > handlers. > > > >>> I was > > > >>> > > > > wondering whether similar approach could be used for the > > > >>> parameters > > > >>> > > that > > > >>> > > > > OAuth needed to obtain at runtime. We could still have this > > KIP > > > >>> with > > > >>> > > > > built-in substitutable types to handle common cases like > > > getting > > > >>> > > options > > > >>> > > > > from a file without writing any code. But I wasn't sure if > > > there > > > >>> were > > > >>> > > > OAuth > > > >>> > > > > options that couldn't be handled as callbacks using the > login > > > >>> > callback > > > >>> > > > > handler. > > > >>> > > > > > > > >>> > > > > On Thu, Apr 5, 2018 at 10:25 PM, Ron Dagostino < > > > >>> rndg...@gmail.com> > > > >>> > > > wrote: > > > >>> > > > > > > > >>> > > > > > Hi Rajini. Thanks for the questions. I could see > someone > > > >>> wanting > > > >>> > to > > > >>> > > > > > retrieve a password from a vended password vault solution > > > (for > > > >>> > > > example); > > > >>> > > > > > that is the kind of scenario that the ability to add new > > > >>> > > substitutable > > > >>> > > > > > types would be meant for. I do still consider this KIP > 269 > > > to > > > >>> be a > > > >>> > > > > > prerequisite for the SASL/OAUTHBEARER KIP 255. I am open > > to > > > a > > > >>> > > > different > > > >>> > > > > > perspective in case I missed or misunderstood your point. > > > >>> > > > > > > > > >>> > > > > > Ron > > > >>> > > > > > > > > >>> > > > > > On Thu, Apr 5, 2018 at 8:13 AM, Rajini Sivaram < > > > >>> > > > rajinisiva...@gmail.com> > > > >>> > > > > > wrote: > > > >>> > > > > > > > > >>> > > > > > > Hi Ron, > > > >>> > > > > > > > > > >>> > > > > > > Now that login callback handlers are configurable, is > > this > > > >>> KIP > > > >>> > > still > > > >>> > > > a > > > >>> > > > > > > pre-req for OAuth? I was wondering whether we still > need > > > the > > > >>> > > ability > > > >>> > > > to > > > >>> > > > > > add > > > >>> > > > > > > new substitutable types or whether it would be > sufficient > > > to > > > >>> add > > > >>> > > the > > > >>> > > > > > > built-in ones to read from file etc. > > > >>> > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > On Thu, Mar 29, 2018 at 6:48 AM, Ron Dagostino < > > > >>> > rndg...@gmail.com> > > > >>> > > > > > wrote: > > > >>> > > > > > > > > > >>> > > > > > > > Hi everyone. There have been no comments on this > KIP, > > > so I > > > >>> > > intend > > > >>> > > > to > > > >>> > > > > > put > > > >>> > > > > > > > it to a vote next week if there are no comments that > > > might > > > >>> > entail > > > >>> > > > > > changes > > > >>> > > > > > > > between now and then. Please take a look in the > > meantime > > > >>> if > > > >>> > you > > > >>> > > > > wish. > > > >>> > > > > > > > > > > >>> > > > > > > > Ron > > > >>> > > > > > > > > > > >>> > > > > > > > On Thu, Mar 15, 2018 at 2:36 PM, Ron Dagostino < > > > >>> > > rndg...@gmail.com> > > > >>> > > > > > > wrote: > > > >>> > > > > > > > > > > >>> > > > > > > > > Hi everyone. > > > >>> > > > > > > > > > > > >>> > > > > > > > > I created KIP-269: Substitution Within > Configuration > > > >>> Values > > > >>> > > > > > > > > <https://cwiki.apache.org/ > > > confluence/display/KAFKA/KIP+ > > > >>> > > > > > > > 269+Substitution+Within+Configuration+Values> > > > >>> > > > > > > > > (https://cwiki.apache.org/conf > > > >>> luence/display/KAFKA/KIP+269+ > > > >>> > > > > > > > > Substitution+Within+Configuration+Values > > > >>> > > > > > > > > <https://cwiki.apache.org/ > confluence/pages/viewpage. > > > >>> > > > > > > > action?pageId=75968876> > > > >>> > > > > > > > > ). > > > >>> > > > > > > > > > > > >>> > > > > > > > > This KIP proposes adding support for substitution > > > within > > > >>> > client > > > >>> > > > > JAAS > > > >>> > > > > > > > > configuration values for PLAIN and SCRAM-related > SASL > > > >>> > > mechanisms > > > >>> > > > > in a > > > >>> > > > > > > > > backwards-compatible manner and making the > > > functionality > > > >>> > > > available > > > >>> > > > > to > > > >>> > > > > > > > other > > > >>> > > > > > > > > existing (or future) configuration contexts where > it > > is > > > >>> > deemed > > > >>> > > > > > > > appropriate. > > > >>> > > > > > > > > > > > >>> > > > > > > > > This KIP was extracted from (and is now a > > prerequisite > > > >>> for) > > > >>> > > > > KIP-255: > > > >>> > > > > > > > > OAuth Authentication via SASL/OAUTHBEARER > > > >>> > > > > > > > > <https://cwiki.apache.org/ > confluence/pages/viewpage. > > > >>> > > > > > > > action?pageId=75968876> > > > >>> > > > > > > > > based on discussion of that KIP. > > > >>> > > > > > > > > > > > >>> > > > > > > > > Ron > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >> > > > >> > > > > > > > > > >
