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