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