Hi, @Ron Agreed, tracking multiple errors would be better and would help diagnose bad extensions faster I've updated the KIP to address your two comments. Regarding the Javadoc, please read below:
@Rajini The idea of the potentially-null token and extensions is not that they can be passed to the constructor - it is that they can be nullified on a validation error occurring (like it is done in the #error() method on `OAuthBearerValidatorCallback`). Maybe it doesn't make too much sense to nullify the token, but I believe it is worth it to do the same with the extensions. I agree that the callback handler should himself populate the callback with the *validated* extensions only. Will change implementation and KIP in due time. Please share what you think about nullifying token/extensions on validation error. Best, Stanislav On Fri, Aug 10, 2018 at 7:24 PM Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Hi Stanislav, > > For the point that Ron made above for: > > public OAuthBearerExtensionsValidatorCallback(OAuthBearerToken token, > SaslExtensions > extensions) > > > I don't think we should ever invoke extensions callback without the token. > We can first validate the token and invoke extensions callback only if > token is non-null. Can we clarify that in the javadoc? > > - public SaslExtensions extensions() : Extensions should be non-null > - public OAuthBearerToken token() : Token should be non-null > > > Also agree with Ron that we should have the ability to return errors for > all invalid extensions, even if a callback handler may choose to stop on > first failure. > > I think we also need another method to return the extensions that were > validated and will be made available as negotiated properties. As per the > RFC, server should ignore unknown extensions. So callback handlers need to > be able to validate the ones they know of and return those. Other > extensions should not be added to the SaslServer's negotiated properties. > > - public SaslExtensions validatedExtensions() > > > > On Fri, Aug 10, 2018 at 3:26 PM, Ron Dagostino <rndg...@gmail.com> wrote: > > > Hi Stanislav. Here are a few KIP comments. > > > > <<<There are also additional regex validations for extension name and > > values to ensure they conform to the OAuth standard > > It is the SASL/OAUTHBEARER standard that defines the regular expressions > > (specifically, https://tools.ietf.org/html/rfc7628#section-3.1) rather > > than > > any of the OAuth specifications. It would be good to make this > > clarification. > > > > <<<public OAuthBearerExtensionsValidatorCallback(OAuthBearerToken token, > > SaslExtensions extensions) > > This constructor lacks Javadoc in the KIP. Could you add it, and also > > indicate which of the two parameters are required vs. optional? The > > Javadoc for the token() method indicates that the return value could be > > null, but that would only be true if the constructor accepted a null > value > > for the token. I'm okay with the constructor accepting a null token > > (Rajini, you may differ in opinion, in which case I defer to your > > preference). But please do clarify this issue. > > > > I also am not sure if exposing just one invalid extension name and error > > message in the OAuthBearerExtensionsValidatorCallback class is good > > enough. An alternative to invalidExtensionName() and errorMessage() > > methods would be to return an always non-null but potentially empty > > Map<String, String> so that potentially all of the provided extensions > > could be validated and the list of invalid extension names could be > > returned (along with the error message for each of them). If we adopted > > this alternative then the error(String invalidExtensionName, String > > errorMessage) method might need to be renamed addError(String > > invalidExtensionName, String errorMessage). I suspect it would be better > > to go with the map approach to support returning multiple error messages > > even if the default unsecured token validator implementation only adds > the > > first invalid extension name -- at least it would allow others to be more > > complete if they wish. It might also be worth discussing whether a has > > Error() method would be appropriate to add (returning true if the map is > > non-empty). I don't have a strong preference on the issue of supporting > 1 > > vs. multiple errors (though I lean slightly towards supporting multiple > > errors). I defer to the preference of others in this regard. > > > > Finally, now that we are actually validating extensions, the comment that > > "An attempt to use [auth] will result in an exception" might cause > > confusion and perhaps needs to be clarified to state that the exception > > occurs on the client side before the extensions are sent to the server > > rather than during extension validation on the server side (e.g. "An > > attempt to send [auth] will result in an exception on the client"). > > > > Ron > > > > > > On Fri, Aug 10, 2018 at 7:22 AM Stanislav Kozlovski < > > stanis...@confluent.io> > > wrote: > > > > > Hi Rajini, Ron > > > > > > I've updated the KIP with the latest changes following our discussion. > > > Please do give it a read. If you feel it is alright, I will follow up > > with > > > a PR later. > > > > > > Best, > > > Stanislav > > > > > > On Thu, Aug 9, 2018 at 10:09 AM Rajini Sivaram < > rajinisiva...@gmail.com> > > > wrote: > > > > > > > Hi Ron/Stansilav, > > > > > > > > OK, let's just go with 2. I think it would be better to add a > > > > OAuth-specific extensions handler OAuthBearerExtensionsValidator > > Callback > > > > that > > > > provides OAuthBearerToken. > > > > > > > > To summarise, we chose option 2 out of these four options: > > > > > > > > 1. {OAuthBearerValidatorCallback, SaslExtensionsValidatorCallback} > > : > > > We > > > > don't want to use multiple ordered callbacks since we don't want > the > > > > context of one callback to come from another.callback, > > > > 2. OAuthBearerExtensionsValidatorCallback(OAuthBearerToken token, > > > > SaslExtensions ext): This allows extensions to be validated using > > > > context from the token, we are ok with this. > > > > 3. SaslExtensionsValidatorCallback(Map<String, Object> context, > > > > SaslExtensions ext): This doesn't really offer any real advantage > > over > > > > 2. > > > > 4. OAuthBearerValidatorCallback(String token, SaslExtensions ext): > > We > > > > don't want token validator to see extensions since these are > > insecure > > > > but > > > > token validation needs to be secure. So we prefer to use a second > > > > callback > > > > handler to validate extensions after securely validating token. > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 8:52 PM, Ron Dagostino <rndg...@gmail.com> > > wrote: > > > > > > > > > Hi Rajini. I think we are considering the following two options. > > Let > > > me > > > > > try to describe them along with their relative > > > advantages/disadvantages. > > > > > > > > > > Option #1: Send two callbacks in a single array to the callback > > > handler: > > > > > ch.handle(new Callback[] {tokenCallback, extensionsCallback}); > > > > > > > > > > Option #2: Send two callbacks separately, in two separate arrays, > to > > > the > > > > > callback handler: > > > > > ch.handle(new Callback[] {tokenCallback}); > > > > > ch.handle(new Callback[] {extensionsCallback}); > > > > > > > > > > I actually don't see any objective disadvantage with #1. If we > don't > > > get > > > > > an exception then we know we have the information we need; if we do > > get > > > > an > > > > > exception then we can tell if the first callback was handled > because > > > > either > > > > > its token() method or its errorStatus() method will return > non-null; > > if > > > > > both return null then we just send the token callback by itself and > > we > > > > > don't publish any extension as negotiated properties. There is no > > > > > possibility of partial results, and I don't think there is a > > > performance > > > > > penalty due to potential re-validation here, either. > > > > > > > > > > I see a subjective disadvantage with #1. It feels awkward to me > to > > > > > provide the token as context for extension validation via the first > > > > > callback. > > > > > > > > > > Actually, it just occurred to me why it feels awkward, and I think > > this > > > > is > > > > > an objective disadvantage of this approach. It would be impossible > > to > > > do > > > > > extension validation in such a scenario without also doing token > > > > validation > > > > > first. We are using the first callback as a way to provide > context, > > > but > > > > we > > > > > are also using that first callback to request token validation. We > > are > > > > > complecting two separate things -- context and a request for > > validation > > > > -- > > > > > into one thing, so this approach has an element of complexity to > it. > > > > > > > > > > The second option has no such complexity. If we want to provide > > > context > > > > to > > > > > the extension validation then we do that by adding a token to the > > > > > extensionsCallback instance before we provide it to the callback > > > handler. > > > > > How we do that -- whether by Map<String, Object> or via a typed > > getter > > > -- > > > > > feels like a subjective decision, and assuming you agree with the > > > > > complexity argument and choose option #2, I would defer to your > > > > preference > > > > > as to how to implement it. > > > > > > > > > > Ron > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 3:10 PM Rajini Sivaram < > > rajinisiva...@gmail.com > > > > > > > > > wrote: > > > > > > > > > > > Hi Ron, > > > > > > > > > > > > Yes, I was thinking of a SaslExtensionsValidatorCallback with > > > > additional > > > > > > context as well initially, but I didn't like the idea of > name-value > > > > pairs > > > > > > and I didn't want generic objects passed around through the > > callback > > > > So > > > > > > providing context through other callbacks felt like a neater fit. > > > There > > > > > > are pros and cons for both approaches, so we could go with > either. > > > > > > > > > > > > Callbacks are provided to callback handlers in an array and there > > is > > > > > > implicit ordering in the callbacks provided to the callback > > handler. > > > > > > In the typical example of {NameCallback, PasswordCallback}, we > > expect > > > > > that > > > > > > ordering so that password callback knows what the user name is. > > Kafka > > > > > > guarantees ordering of server callbacks in each of its SASL > > > mechanisms > > > > > and > > > > > > this is explicitly stated in > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > 86%3A+Configurable+SASL+callback+handlers > > > > > > . > > > > > > Until now, we didn't need to worry about ordering for OAuth. > > > > > > > > > > > > We currently do not have any optional callbacks - configured > > callback > > > > > > handlers have to process all the callbacks for the mechanism or > > else > > > we > > > > > > fail authentication. We have to make > SaslExtensionValidationCallbac > > k > > > an > > > > > > exception, at least for backward compatibility. We will only > > include > > > > this > > > > > > callback if the client provided some extensions. I think it is > > > > reasonable > > > > > > to expect that in general, custom callback handlers will handle > > this > > > > > > callback if clients are likely to set extensions. In case it > > > doesn't, > > > > we > > > > > > don't want to make any assumptions about which callbacks may have > > > been > > > > > > handled. Instead, it would be better to invoke the callback > handler > > > > again > > > > > > without the extensions callback and not expose any extensions as > > > > > negotiated > > > > > > properties. Since we are doing this only for backward > > compatibility, > > > > the > > > > > > small performance hit would be reasonable, avoiding any > assumptions > > > > about > > > > > > the callback handler implementation and partial results on > hitting > > an > > > > > > exception. > > > > > > > > > > > > Back to the other approach of providing a Map. For OAuth, we > would > > > like > > > > > > extension validation to see the actual OAuthBearerToken object, > for > > > > > > instance to validate extensions based on scope. Having > > > > > > these mechanism-specific objects in a Map doesn't feel ideal. It > > will > > > > > > probably be better to define OAuthBearerExtensionsValidator > > Callback > > > > > with a > > > > > > token getter that returns OAuthBearerToken in that case. > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 6:09 PM, Ron Dagostino <rndg...@gmail.com > > > > > > wrote: > > > > > > > > > > > > > Hi Rajini. I also like that idea, but I think it might rely on > > one > > > > or > > > > > > > possibly two implicit assumptions that I'm not sure are > > guaranteed > > > to > > > > > be > > > > > > > true. First, I'm not sure if the CallbackHandler interface > > > > guarantees > > > > > > that > > > > > > > implementations must process callbacks in order. Second (and > > more > > > > > > > plausibly than the first), I'm not sure CallbackHandler > > guarantees > > > > that > > > > > > > callbacks are to be processed in order until either there are > no > > > more > > > > > > left > > > > > > > in the array or one of the elements is an unsupported callback. > > > The > > > > > > > Javadoc simply says it throws UnsupportedCallbackException "if > > the > > > > > > > implementation of this method does not support one or more of > the > > > > > > Callbacks > > > > > > > specified in the callbacks parameter." This statement does not > > > > preclude > > > > > > the > > > > > > > case that implementations might first check to make sure all of > > the > > > > > > > provided callbacks are supported before processing any of them. > > > > > > > > > > > > > > We could update the Javadoc for AuthenticateCallbackHandler to > > make > > > > it > > > > > > > clear how implementations must work -- i.e. they must process > the > > > > > > callbacks > > > > > > > in order, and they must process all recognized callbacks before > > > > > throwing > > > > > > > UnsupportedCallbackException due to an unrecognized one. > > > > > > > > > > > > > > Note that the above issue does not arise if we simply want the > > > > ability > > > > > to > > > > > > > validate SASL extensions in isolation -- we could just give the > > > > > callback > > > > > > > handler an array containing a single instance of the proposed > > > > > > > SaslExtensionsValidatorCallback.The issue only arises if we > > want to > > > > > > > provide > > > > > > > additional context (e.g. the token in the case of > > SASL/OATHBEARER) > > > to > > > > > the > > > > > > > validation mechanism. > > > > > > > > > > > > > > If it is not just SASL Extension validation that we are > > interested > > > in > > > > > > > adding but in fact we want to be able to provide additional > > context > > > > to > > > > > > > SaslExtensionsValidatorCallback, then adding the ordering > > > constraint > > > > > > above > > > > > > > is one way, but we could avoid the constraint by allowing > > > > > > > SaslExtensionsValidatorCallback to accept not only the > > extensions > > > to > > > > > > > validate but also an arbitrary map of name/value pairs. Each > > SASL > > > > > > > mechanism implementation could declare what additional context > it > > > > > > provides > > > > > > > (if any) and at what key(s) the information is available. This > > > > second > > > > > > > approach feels more direct than the first one and would be my > > > > > preference > > > > > > > (assuming I',m not missing anything, which is certainly > > possible). > > > > > > > Thoughts? > > > > > > > > > > > > > > Ron > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 12:39 PM Stanislav Kozlovski < > > > > > > > stanis...@confluent.io> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Rajini, > > > > > > > > > > > > > > > > That approach makes more sense to me. I like it > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 5:35 PM Rajini Sivaram < > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Ron/Stanislav, > > > > > > > > > > > > > > > > > > Do you think it makes sense to separate out > > > > > > > OAuthBearerValidatorCallback > > > > > > > > > and SaslExtensionsValidatorCallback so that it is clearer > > that > > > > > these > > > > > > > are > > > > > > > > > two separate entities that need validation? When we add > > support > > > > > > > > > for customisable extensions in other mechanisms, we could > > reuse > > > > > > > > > SaslExtensionsValidatorCallback. We will invoke > > CallbackHandler > > > > > with > > > > > > { > > > > > > > > > OAuthBearerValidatorCallback, > SaslExtensionsValidatorCallback > > } > > > > in > > > > > > > that > > > > > > > > > order like we do { NameCallback, PasswordCallback }. So > > > typically > > > > > we > > > > > > > > expect > > > > > > > > > to validate tokens with no reference to extensions, but we > > may > > > > > refer > > > > > > to > > > > > > > > > token to validate extensions. Only validated extensions > will > > be > > > > > > > available > > > > > > > > > as the server's negotiated properties. We will need to > handle > > > > > > > > > UnsupportedCallbackException for > > > SaslExtensionsValidatorCallback > > > > > for > > > > > > > > > backwards compatibility, but that should be ok. > > > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 5:06 PM, Stanislav Kozlovski < > > > > > > > > > stanis...@confluent.io> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Ron, > > > > > > > > > > > > > > > > > > > > Yes, I agree we should document it thoroughly > > > > > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 5:02 PM Ron Dagostino < > > > > rndg...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Stanislav. If the community agrees we should add it > > > then > > > > we > > > > > > > > should > > > > > > > > > > at a > > > > > > > > > > > minimum add explicit warnings in the Javadoc making it > > very > > > > > clear > > > > > > > how > > > > > > > > > > this > > > > > > > > > > > should not be used. > > > > > > > > > > > > > > > > > > > > > > Ron > > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 11:54 AM Stanislav Kozlovski < > > > > > > > > > > > stanis...@confluent.io> > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hey Ron, > > > > > > > > > > > > > > > > > > > > > > > > I fully agree that token validation is a serious > > security > > > > > > > > operation. > > > > > > > > > > > > Although, I believe allowing the user to do more > > > validation > > > > > > with > > > > > > > > the > > > > > > > > > > > > extensions does not hurt - the user is fully > > responsible > > > > for > > > > > > his > > > > > > > > > > security > > > > > > > > > > > > once he starts implementing custom code for token > > > > validation. > > > > > > You > > > > > > > > > would > > > > > > > > > > > > expect people to take the appropriate considerations > > when > > > > > > > > validating > > > > > > > > > > > > unsecured extensions against the token. > > > > > > > > > > > > I also think that using the extensions as a secondary > > > > > > validation > > > > > > > > > method > > > > > > > > > > > > might be useful. You could do your normal validation > > > using > > > > > the > > > > > > > > token > > > > > > > > > > and > > > > > > > > > > > > then have a second sanity-check validation on top > (e.g > > > > > validate > > > > > > > > > > > > hostname/port is what client expected). Keep in mind > > that > > > > the > > > > > > > > server > > > > > > > > > > > > exposes the properties via `getNegotiatedProperty` so > > it > > > > > makes > > > > > > > > sense > > > > > > > > > to > > > > > > > > > > > > allow the server to have custom validation on the > > > > extensions. > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > Stanislav > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 3:29 PM Ron Dagostino < > > > > > > rndg...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Stanislav. If you wanted to do this a good way > > > might > > > > be > > > > > > to > > > > > > > > add > > > > > > > > > a > > > > > > > > > > > > > constructor to the org.apache.kafka.common. > > > > > > > security.oauthbearer. > > > > > > > > > > > > > OAuthBearerValidatorCallback class that accepts a > > > > > > > SaslExtensions > > > > > > > > > > > instance > > > > > > > > > > > > > in addition to a token value. Then it would give > the > > > > > > callback > > > > > > > > > > handler > > > > > > > > > > > > the > > > > > > > > > > > > > option to introspect the callback to see what > > > extensions > > > > > were > > > > > > > > > > provided > > > > > > > > > > > > with > > > > > > > > > > > > > the > > > > > > > > > > > > > token. > > > > > > > > > > > > > > > > > > > > > > > > > > That being said, token validation is a very > > > > > > security-sensitive > > > > > > > > > > > operation, > > > > > > > > > > > > > and it would be a serious security issue if the > > result > > > of > > > > > > > > applying > > > > > > > > > > the > > > > > > > > > > > > > validation algorithm (which yields a valid vs. not > > > valid > > > > > > > > > > determination) > > > > > > > > > > > > > depended on anything provided by the client other > > than > > > > the > > > > > > > actual > > > > > > > > > > token > > > > > > > > > > > > > value. Nobody should ever allow the client to > > specify > > > a > > > > > JWK > > > > > > > Set > > > > > > > > > URL, > > > > > > > > > > > for > > > > > > > > > > > > > example, or a whitelist of acceptable domains for > > > > > retrieving > > > > > > > JWK > > > > > > > > > > Sets. > > > > > > > > > > > > It > > > > > > > > > > > > > feels to me that while a use case might exist (some > > > kind > > > > of > > > > > > > trace > > > > > > > > > ID, > > > > > > > > > > > for > > > > > > > > > > > > > example, to aid in debugging), someone might > > > > inadvertently > > > > > > hang > > > > > > > > > > > > themselves > > > > > > > > > > > > > if we give them the rope. The risk vs. reward > value > > > > > > > proposition > > > > > > > > > here > > > > > > > > > > > > > doesn't feel like a good one at first glance. > > > Thoughts? > > > > > > > > > > > > > > > > > > > > > > > > > > Ron > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 8, 2018 at 10:04 AM Stanislav > Kozlovski < > > > > > > > > > > > > > stanis...@confluent.io> > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Hey everybody, > > > > > > > > > > > > > > > > > > > > > > > > > > > > Sorry for reviving this, but I neglected > something > > > the > > > > > > first > > > > > > > > time > > > > > > > > > > > > around. > > > > > > > > > > > > > > I believe it would be useful to provide the > > > > > > > > > > > > > > `OAuthBearerUnsecuredValidatorCallbackHandler` > > with > > > the > > > > > > > OAuth > > > > > > > > > > > > extensions > > > > > > > > > > > > > > too. This would enable use cases where validators > > > want > > > > to > > > > > > > > > reconcile > > > > > > > > > > > > > > information from the extensions with the token > (e.g > > > if > > > > > > users > > > > > > > > have > > > > > > > > > > > > > > implemented secured OAuth tokens). > > > > > > > > > > > > > > The implementation would be to instantiate > > > > > > > > > > > > > > `OAuthBearerUnsecuredValidatorCallback` with the > > > > > extensions > > > > > > > > (also > > > > > > > > > > > leave > > > > > > > > > > > > > the > > > > > > > > > > > > > > current constructor, as its a public class). > > > > > > > > > > > > > > > > > > > > > > > > > > > > What are everybody's thoughts on this? If there > are > > > no > > > > > > > > > objections, > > > > > > > > > > > I'll > > > > > > > > > > > > > > update the KIP in due time > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 26, 2018 at 11:14 AM Rajini Sivaram < > > > > > > > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Looks good. Thanks, Stanislav. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Jul 25, 2018 at 7:46 PM, Stanislav > > > Kozlovski > > > > < > > > > > > > > > > > > > > > stanis...@confluent.io > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Rajini, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I updated the KIP. Please check if the > > > > clarification > > > > > is > > > > > > > > okay > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Jul 25, 2018 at 10:49 AM Rajini > > Sivaram < > > > > > > > > > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Stanislav, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. Can you clarify the following line in > the > > > KIP > > > > in > > > > > > the > > > > > > > > > > 'Public > > > > > > > > > > > > > > > > Interfaces' > > > > > > > > > > > > > > > > > section? When you are reading the KIP for > the > > > > first > > > > > > > time, > > > > > > > > > it > > > > > > > > > > > > sounds > > > > > > > > > > > > > > > like > > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > > > adding a new Kafka config. But we are > adding > > > JAAS > > > > > > > config > > > > > > > > > > > options > > > > > > > > > > > > > > with a > > > > > > > > > > > > > > > > > prefix that can be used with the default > > > > unsecured > > > > > > > bearer > > > > > > > > > > > tokens. > > > > > > > > > > > > > We > > > > > > > > > > > > > > > > could > > > > > > > > > > > > > > > > > include the example in this section or at > > least > > > > > link > > > > > > to > > > > > > > > the > > > > > > > > > > > > > example. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > - New config option for default, > unsecured > > > > > bearer > > > > > > > > > tokens - > > > > > > > > > > > > > > > > > `unsecuredLoginExtension_< > > extensionname>`. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. Can you add the package for > > > > > SaslExtensionsCallback > > > > > > > > > class? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 24, 2018 at 10:03 PM, Stanislav > > > > > > Kozlovski < > > > > > > > > > > > > > > > > > stanis...@confluent.io> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Ron, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the suggestions. I have > applied > > > them > > > > > to > > > > > > > the > > > > > > > > > KIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 24, 2018 at 1:39 PM Ron > > > Dagostino < > > > > > > > > > > > > rndg...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Stanislav. The statement "New > config > > > > option > > > > > > for > > > > > > > > > > > > > > > > > > OAuthBearerLoginModule" > > > > > > > > > > > > > > > > > > > is technically incorrect; it should be > > "New > > > > > > config > > > > > > > > > option > > > > > > > > > > > for > > > > > > > > > > > > > > > > default, > > > > > > > > > > > > > > > > > > > unsecured bearer tokens" since that is > > what > > > > > > > provides > > > > > > > > > the > > > > > > > > > > > > > > > > functionality > > > > > > > > > > > > > > > > > > (as > > > > > > > > > > > > > > > > > > > opposed to the login module, which does > > > not). > > > > > > > Also, > > > > > > > > > > please > > > > > > > > > > > > > state > > > > > > > > > > > > > > > > that > > > > > > > > > > > > > > > > > > > "auth" is not supported as a custom > > > extension > > > > > > name > > > > > > > > with > > > > > > > > > > any > > > > > > > > > > > > > > > > > > > SASL/OAUTHBEARER mechanism, including > the > > > > > > unsecured > > > > > > > > > one, > > > > > > > > > > > > since > > > > > > > > > > > > > it > > > > > > > > > > > > > > > is > > > > > > > > > > > > > > > > > > > reserved by the spec for what is > normally > > > > sent > > > > > in > > > > > > > the > > > > > > > > > > HTTP > > > > > > > > > > > > > > > > > Authorization > > > > > > > > > > > > > > > > > > > header an attempt to use it will result > > in > > > a > > > > > > > > > > configuration > > > > > > > > > > > > > > > exception. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Finally, please also state that while > the > > > > > > > > > > > > > OAuthBearerLoginModule > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > OAuthBearerSaslClient will be changed > to > > > > > request > > > > > > > the > > > > > > > > > > > > extensions > > > > > > > > > > > > > > > from > > > > > > > > > > > > > > > > > its > > > > > > > > > > > > > > > > > > > callback handler, for backwards > > > compatibility > > > > > it > > > > > > is > > > > > > > > not > > > > > > > > > > > > > necessary > > > > > > > > > > > > > > > for > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > callback handler to support > > > > > > SaslExtensionsCallback > > > > > > > -- > > > > > > > > > any > > > > > > > > > > > > > > > > > > > UnsupportedCallbackException that is > > thrown > > > > > will > > > > > > be > > > > > > > > > > ignored > > > > > > > > > > > > and > > > > > > > > > > > > > > no > > > > > > > > > > > > > > > > > > > extensions will be added. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Ron > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 24, 2018 at 11:20 AM > > Stanislav > > > > > > > Kozlovski > > > > > > > > < > > > > > > > > > > > > > > > > > > > stanis...@confluent.io> > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hey everybody, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have updated the KIP to reflect the > > > > latest > > > > > > > > changes > > > > > > > > > as > > > > > > > > > > > > best > > > > > > > > > > > > > > as I > > > > > > > > > > > > > > > > > > could. > > > > > > > > > > > > > > > > > > > If > > > > > > > > > > > > > > > > > > > > there aren't more suggestions, I > intent > > > to > > > > > > start > > > > > > > > the > > > > > > > > > > > [VOTE] > > > > > > > > > > > > > > > thread > > > > > > > > > > > > > > > > > > > > tomorrow. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > > > > > > > > Stanislav > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 24, 2018 at 6:34 AM Ron > > > > > Dagostino < > > > > > > > > > > > > > > rndg...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Stanislav. Could you update the > > KIP > > > > to > > > > > > > > reflect > > > > > > > > > > the > > > > > > > > > > > > > latest > > > > > > > > > > > > > > > > > > > definition > > > > > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > > > > > SaslExtensions and confirm or > correct > > > the > > > > > > > impact > > > > > > > > it > > > > > > > > > > has > > > > > > > > > > > > to > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > > SCRAM-related classes? I'm not > sure > > if > > > > the > > > > > > > > > > > > > > currently-described > > > > > > > > > > > > > > > > > > impact > > > > > > > > > > > > > > > > > > > is > > > > > > > > > > > > > > > > > > > > > still accurate. Also, could you > > > mention > > > > > the > > > > > > > > > changes > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > > > > > > OAuthBearerUnsecuredLoginCallbackHandler > > > > > in > > > > > > > the > > > > > > > > > > text in > > > > > > > > > > > > > > > > addition to > > > > > > > > > > > > > > > > > > > > giving > > > > > > > > > > > > > > > > > > > > > the examples? The examples show > the > > > new > > > > > > > > > > > > > > > > > > > > > unsecuredLoginExtension_< > > extensionName> > > > > > > > feature, > > > > > > > > > but > > > > > > > > > > > that > > > > > > > > > > > > > > > > feature > > > > > > > > > > > > > > > > > is > > > > > > > > > > > > > > > > > > > not > > > > > > > > > > > > > > > > > > > > > described anywhere prior to it > > > appearing > > > > > > there. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Ron > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Jul 23, 2018 at 1:42 PM Ron > > > > > > Dagostino < > > > > > > > > > > > > > > > rndg...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Rajini. I think a class is > fine > > > as > > > > > long > > > > > > > as > > > > > > > > we > > > > > > > > > > > make > > > > > > > > > > > > > sure > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > semantics > > > > > > > > > > > > > > > > > > > > > > of immutability are clear -- it > > would > > > > > have > > > > > > to > > > > > > > > be > > > > > > > > > a > > > > > > > > > > > > value > > > > > > > > > > > > > > > class, > > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > any > > > > > > > > > > > > > > > > > > > > > > constructor that accepts a Map as > > > input > > > > > > would > > > > > > > > > have > > > > > > > > > > to > > > > > > > > > > > > > copy > > > > > > > > > > > > > > > that > > > > > > > > > > > > > > > > > Map > > > > > > > > > > > > > > > > > > > > > rather > > > > > > > > > > > > > > > > > > > > > > than store it in a member > variable. > > > > > > > Similarly, > > > > > > > > > any > > > > > > > > > > > Map > > > > > > > > > > > > > > that > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > > might > > > > > > > > > > > > > > > > > > > > > > return would have to be > > unmodifiable. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Ron > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Jul 23, 2018 at 12:24 PM > > > Rajini > > > > > > > > Sivaram < > > > > > > > > > > > > > > > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> Hi Ron, Stanislav, > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > >> I agree with Stanislav that it > > would > > > > be > > > > > > > better > > > > > > > > > to > > > > > > > > > > > > leave > > > > > > > > > > > > > > > > > > > > `SaslExtensions` > > > > > > > > > > > > > > > > > > > > > >> as > > > > > > > > > > > > > > > > > > > > > >> a class rather than make it an > > > > > interface. > > > > > > We > > > > > > > > > > don''t > > > > > > > > > > > > > really > > > > > > > > > > > > > > > > > expect > > > > > > > > > > > > > > > > > > > > users > > > > > > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > > > >> extends this class, so it is > > > > convenient > > > > > to > > > > > > > > have > > > > > > > > > an > > > > > > > > > > > > > > > > > implementation > > > > > > > > > > > > > > > > > > > > since > > > > > > > > > > > > > > > > > > > > > >> users need to create an > instance. > > > The > > > > > > class > > > > > > > > > > provided > > > > > > > > > > > > by > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > public > > > > > > > > > > > > > > > > > > > API > > > > > > > > > > > > > > > > > > > > > >> should be sufficient in the vast > > > > > majority > > > > > > of > > > > > > > > the > > > > > > > > > > > > cases. > > > > > > > > > > > > > > Ron, > > > > > > > > > > > > > > > > do > > > > > > > > > > > > > > > > > > you > > > > > > > > > > > > > > > > > > > > > agree? > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > >> On Mon, Jul 23, 2018 at 11:35 > AM, > > > Ron > > > > > > > > Dagostino > > > > > > > > > < > > > > > > > > > > > > > > > > > > rndg...@gmail.com> > > > > > > > > > > > > > > > > > > > > > >> wrote: > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > >> > Hi Stanislav. See > > > > > > > > > > > > > > > > > > > https://tools.ietf.org/html/ > > > > > rfc7628#section-3.1, > > > > > > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > > > >> > that section refers to the > core > > > ABNF > > > > > > > > > productions > > > > > > > > > > > > > defined > > > > > > > > > > > > > > > in > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > https://tools.ietf.org/html/rfc5234#appendix-B. > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > >> > Ron > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > >> > > On Jul 23, 2018, at 1:30 AM, > > > > > Stanislav > > > > > > > > > > > Kozlovski < > > > > > > > > > > > > > > > > > > > > > >> stanis...@confluent.io> > > > > > > > > > > > > > > > > > > > > > >> > wrote: > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > >> > > Hey Ron and Rajini, > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > >> > > Here are my thoughts: > > > > > > > > > > > > > > > > > > > > > >> > > Regarding separators in > > > > > > SaslExtensions - > > > > > > > > > > Agreed, > > > > > > > > > > > > > that > > > > > > > > > > > > > > > was > > > > > > > > > > > > > > > > a > > > > > > > > > > > > > > > > > > bad > > > > > > > > > > > > > > > > > > > > > move. > > > > > > > > > > > > > > > > > > > > > >> > > Should definitely not be a > > > concern > > > > > of > > > > > > > > > > > > > CallbackHandler > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > > > LoginModule > > > > > > > > > > > > > > > > > > > > > >> > > implementors. > > > > > > > > > > > > > > > > > > > > > >> > > SaslExtensions interface - > > > > Wouldn't > > > > > > > > > > implementing > > > > > > > > > > > > it > > > > > > > > > > > > > as > > > > > > > > > > > > > > > an > > > > > > > > > > > > > > > > > > > > interface > > > > > > > > > > > > > > > > > > > > > >> mean > > > > > > > > > > > > > > > > > > > > > >> > > that users will have to make > > > sure > > > > > > > they're > > > > > > > > > > > passing > > > > > > > > > > > > in > > > > > > > > > > > > > > an > > > > > > > > > > > > > > > > > > > > unmodifiable > > > > > > > > > > > > > > > > > > > > > >> map > > > > > > > > > > > > > > > > > > > > > >> > > themselves. I believe it > would > > > be > > > > > > better > > > > > > > > if > > > > > > > > > we > > > > > > > > > > > > > > enforced > > > > > > > > > > > > > > > > that > > > > > > > > > > > > > > > > > > > > through > > > > > > > > > > > > > > > > > > > > > >> > class > > > > > > > > > > > > > > > > > > > > > >> > > constructors instead. > > > > > > > > > > > > > > > > > > > > > >> > > SaslExtensions#map() - I'd > > also > > > > > prefer > > > > > > > > this. > > > > > > > > > > The > > > > > > > > > > > > > > reason > > > > > > > > > > > > > > > I > > > > > > > > > > > > > > > > > went > > > > > > > > > > > > > > > > > > > > with > > > > > > > > > > > > > > > > > > > > > >> > > `extensionValue` and > > > > > `extensionNames` > > > > > > > was > > > > > > > > > > > because > > > > > > > > > > > > I > > > > > > > > > > > > > > > > figured > > > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > > > > made > > > > > > > > > > > > > > > > > > > > > >> sense > > > > > > > > > > > > > > > > > > > > > >> > > to have `ScramExtensions` > > extend > > > > > > > > > > > `SaslExtensions` > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > therefore > > > > > > > > > > > > > > > > > > > > have > > > > > > > > > > > > > > > > > > > > > >> > their > > > > > > > > > > > > > > > > > > > > > >> > > API be similar. In the end, > do > > > you > > > > > > think > > > > > > > > > that > > > > > > > > > > it > > > > > > > > > > > > is > > > > > > > > > > > > > > > worth > > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > > have > > > > > > > > > > > > > > > > > > > > > >> > > `ScramExtensions` extend > > > > > > > `SaslExtensions`? > > > > > > > > > > > > > > > > > > > > > >> > > @Ron, could you point me to > > the > > > > SASL > > > > > > > OAuth > > > > > > > > > > > > mechanism > > > > > > > > > > > > > > > > > specific > > > > > > > > > > > > > > > > > > > > > regular > > > > > > > > > > > > > > > > > > > > > >> > > expressions for keys/values > > you > > > > > > > mentioned > > > > > > > > > are > > > > > > > > > > in > > > > > > > > > > > > RFC > > > > > > > > > > > > > > > 7628 > > > > > > > > > > > > > > > > ( > > > > > > > > > > > > > > > > > > > > > >> > > > > > > https://tools.ietf.org/html/rfc7628 > > > > > ) > > > > > > ? > > > > > > > I > > > > > > > > > > could > > > > > > > > > > > > not > > > > > > > > > > > > > > find > > > > > > > > > > > > > > > > any > > > > > > > > > > > > > > > > > > > while > > > > > > > > > > > > > > > > > > > > > >> > > originally implementing > this. > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > >> > > Best, > > > > > > > > > > > > > > > > > > > > > >> > > Stanislav > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > >> > >> On Sun, Jul 22, 2018 at > 6:46 > > PM > > > > Ron > > > > > > > > > > Dagostino < > > > > > > > > > > > > > > > > > > > rndg...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> > wrote: > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> Hi again, Rajini and > > Stanislav. > > > > I > > > > > > > wonder > > > > > > > > > if > > > > > > > > > > > > making > > > > > > > > > > > > > > > > > > > > SaslExtensions > > > > > > > > > > > > > > > > > > > > > an > > > > > > > > > > > > > > > > > > > > > >> > >> interface rather than a > class > > > > might > > > > > > be > > > > > > > a > > > > > > > > > good > > > > > > > > > > > > > > solution. > > > > > > > > > > > > > > > > > For > > > > > > > > > > > > > > > > > > > > > example: > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> public interface > > > SaslExtensions { > > > > > > > > > > > > > > > > > > > > > >> > >> /** > > > > > > > > > > > > > > > > > > > > > >> > >> * @return an immutable > map > > > > view > > > > > of > > > > > > > the > > > > > > > > > > SASL > > > > > > > > > > > > > > > extensions > > > > > > > > > > > > > > > > > > > > > >> > >> */ > > > > > > > > > > > > > > > > > > > > > >> > >> Map<String, String> > map(); > > > > > > > > > > > > > > > > > > > > > >> > >> } > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> This solves the issue of > lack > > > of > > > > > > > clarity > > > > > > > > on > > > > > > > > > > > > > > > immutability, > > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > > > > > also > > > > > > > > > > > > > > > > > > > > > >> > >> eliminates copying, like > > this: > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> SaslExtensions myMethod() { > > > > > > > > > > > > > > > > > > > > > >> > >> Map<String, String> > > > myRetval = > > > > > > > > > > > > > > > > > > > > > getUnmodifiableSaslExtensionsMap(); > > > > > > > > > > > > > > > > > > > > > >> > >> return new > > SaslExtensions() > > > { > > > > > > > > > > > > > > > > > > > > > >> > >> public Map<String, > > > String> > > > > > > > map() { > > > > > > > > > > > > > > > > > > > > > >> > >> return myRetval; > > > > > > > > > > > > > > > > > > > > > >> > >> } > > > > > > > > > > > > > > > > > > > > > >> > >> } > > > > > > > > > > > > > > > > > > > > > >> > >> } > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> Alternatively, we could do > it > > > > like > > > > > > > this: > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> /** > > > > > > > > > > > > > > > > > > > > > >> > >> * Supplier that returns > > > immutable > > > > > map > > > > > > > > view > > > > > > > > > of > > > > > > > > > > > > SASL > > > > > > > > > > > > > > > > > Extensions > > > > > > > > > > > > > > > > > > > > > >> > >> */ > > > > > > > > > > > > > > > > > > > > > >> > >> public interface > > SaslExtensions > > > > > > extends > > > > > > > > > > > > > > > > > Supplier<Map<String, > > > > > > > > > > > > > > > > > > > > > >> String>> { > > > > > > > > > > > > > > > > > > > > > >> > >> // empty > > > > > > > > > > > > > > > > > > > > > >> > >> } > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> The we could simply return > > the > > > > > > instance > > > > > > > > > like > > > > > > > > > > > > this, > > > > > > > > > > > > > > > again > > > > > > > > > > > > > > > > > > > without > > > > > > > > > > > > > > > > > > > > > >> > copying: > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> SaslExtensions myMethod() { > > > > > > > > > > > > > > > > > > > > > >> > >> Map<String, String> > > > myRetval = > > > > > > > > > > > > > > > > > > > > > getUnmodifiableSaslExtensionsMap(); > > > > > > > > > > > > > > > > > > > > > >> > >> return () -> myRetval; > > > > > > > > > > > > > > > > > > > > > >> > >> } > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> I think the main reason for > > > > making > > > > > > > > > > > SaslExtensions > > > > > > > > > > > > > > part > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > > public > > > > > > > > > > > > > > > > > > > > > >> > >> interface is to avoid > adding > > a > > > > Map > > > > > to > > > > > > > the > > > > > > > > > > > > Subject's > > > > > > > > > > > > > > > > public > > > > > > > > > > > > > > > > > > > > > >> credentials. > > > > > > > > > > > > > > > > > > > > > >> > >> Making SaslExtensions an > > > > interface > > > > > > > meets > > > > > > > > > that > > > > > > > > > > > > > > > requirement > > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > > then > > > > > > > > > > > > > > > > > > > > > >> > allows > > > > > > > > > > > > > > > > > > > > > >> > >> us to be free to implement > > > > whatever > > > > > > we > > > > > > > > want > > > > > > > > > > > > > > internally. > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> Thoughts? > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >> Ron > > > > > > > > > > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > > > > > > > > >> > >>> On Sun, Jul 22, 2018 at > > 12:45 > > > PM > > > > > Ron > > > > > > > > > > > Dagostino < > > > > > > > > > > > > > > > > > > > > rndg...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> > wrote: > > > > > > > > > > > > > > > > > > > > > >> > >>> > > > > > > > > > > > > > > > > > > > > > >> > >>> Hi Rajini. The SaslServer > > is > > > > > going > > > > > > to > > > > > > > > > have > > > > > > > > > > to > > > > > > > > > > > > > > > validate > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > > >> extensions, > > > > > > > > > > > > > > > > > > > > > >> > >>> too, but I’m okay with > > keeping > > > > the > > > > > > > > > > validation > > > > > > > > > > > > > logic > > > > > > > > > > > > > > > > > > elsewhere > > > > > > > > > > > > > > > > > > > as > > > > > > > > > > > > > > > > > > > > > >> long > > > > > > > > > > > > > > > > > > > > > >> > as > > > > > > > > > > > > > > > > > > > > > >> > >> it > > > > > > > > > > > > > > > > > > > > > >> > >>> can be reused in both the > > > client > > > > > and > > > > > > > the > > > > > > > > > > > secret. > > > > > > > > > > > > > > > > > > > > > >> > >>> > > > > > > > > > > > > > > > > > > > > > >> > >>> I strongly prefer > exposing a > > > > map() > > > > > > > > method > > > > > > > > > as > > > > > > > > > > > > > opposed > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > > > >> > extensionNames() > > > > > > > > > > > > > > > > > > > > > >> > >>> and extensionValue(String) > > > > > methods. > > > > > > It > > > > > > > > is > > > > > > > > > a > > > > > > > > > > > > > smaller > > > > > > > > > > > > > > > API > > > > > > > > > > > > > > > > (2 > > > > > > > > > > > > > > > > > > > > methods > > > > > > > > > > > > > > > > > > > > > >> > >> instead > > > > > > > > > > > > > > > > > > > > > >> > >>> of 1), and it gives > clients > > of > > > > the > > > > > > API > > > > > > > > > full > > > > > > > > > > > > > > > map-related > > > > > > > > > > > > > > > > > > > > > >> functionality > > > > > > > > > > > > > > > > > > > > > >> > >>> (there’s a lot of support > > for > > > > > > dealing > > > > > > > > with > > > > > > > > > > > maps > > > > > > > > > > > > > in a > > > > > > > > > > > > > > > > > variety > > > > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > > > > > >> ways). > > > > > > > > > > > > > > > > > > > > > >> > >>> > > > > > > > > > > > > > > > > > > > > > >> > >>> Regardless of whether we > go > > > > with a > > > > > > > map() > > > > > > > > > > > method > > > > > > > > > > > > or > > > > > > > > > > > > > > > > > > > > > extensionNames() > > > > > > > > > > > > > > > > > > > > > >> and > > > > > > > > > > > > > > > > > > > > > >> > >>> extensionValue(String) > > > methods, > > > > > the > > > > > > > > > > semantics > > > > > > > > > > > of > > > > > > > > > > > > > > > > > mutability > > > > > > > > > > > > > > > > > > > need > > > > > > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > > > >> be > > > > > > > > > > > > > > > > > > > > > >> > >>> clear. I think either way > > we > > > > > should > > > > > > > > never > > > > > > > > > > > > share a > > > > > > > > > > > > > > map > > > > > > > > > > > > > > > > > that > > > > > > > > > > > > > > > > > > > > anyone > > > > > > > > > > > > > > > > > > > > > >> else > > > > > > > > > > > > > > > > > > > > > >> > >>> could possibly mutate — > > > either a > > > > > map > > > > > > > > that > > > > > > > > > > > > someone > > > > > > > > > > > > > > > gives > > > > > > > > > > > > > > > > us > > > > > > > > > > > > > > > > > > or > > > > > > > > > > > > > > > > > > > a > > > > > > > > > > > > > > > > > > > > > map > > > > > > > > > > > > > > > > > > > > > >> > that > > > > > > > > > > > > > > > > > > > > > >> > >> we > > > > > > > > > > > > > > > > > > > > > >> > >>> might expose. > > > > > > > > > > > > > > > > > > > > > >> > >>> > > > > > > > > > > > > > > > > > > > > > >> > >>> Thoughts? > > > > > > > > > > > > > > > > > > > > > >> > >>> > > > > > > > > > > > > > > > > > > > > > >> > >>> Ron > > > > > > > > > > > > > > > > > > > > > >> > >>> > > > > > > > > > > > > > > > > > > > > > >> > >>>> On Jul 22, 2018, at 11:23 > > AM, > > > > > > Rajini > > > > > > > > > > Sivaram > > > > > > > > > > > < > > > > > > > > > > > > > > > > > > > > > >> rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > >> > >>> wrote: > > > > > > > > > > > > > > > > > > > > > >> > >>>> > > > > > > > > > > > > > > > > > > > > > >> > >>>> Hmm.... I think we need a > > > much > > > > > > > simpler > > > > > > > > > > > > > > SaslExtensions > > > > > > > > > > > > > > > > > class > > > > > > > > > > > > > > > > > > > if > > > > > > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > > > > > > > >> are > > > > > > > > > > > > > > > > > > > > > >> > >>>> making it part of the > > public > > > > API. > > > > > > > > > > > > > > > > > > > > > >> > >>>> > > > > > > > > > > > > > > > > > > > > > >> > >>>> 1. I don't see the point > of > > > > > > including > > > > > > > > > > > separator > > > > > > > > > > > > > > > > anywhere > > > > > > > > > > > > > > > > > in > > > > > > > > > > > > > > > > > > > > > >> > >>> SaslExtensions. > > > > > > > > > > > > > > > > > > > > > >> > >>>> Extensions provide a map > > and > > > we > > > > > > > > propagate > > > > > > > > > > the > > > > > > > > > > > > map > > > > > > > > > > > > > > > from > > > > > > > > > > > > > > > > > > client > > > > > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > > > >> > server > > > > > > > > > > > > > > > > > > > > > >> > >>>> using the protocol > > associated > > > > > with > > > > > > > the > > > > > > > > > > > > mechanism > > > > > > > > > > > > > in > > > > > > > > > > > > > > > > use. > > > > > > > > > > > > > > > > > > The > > > > > > > > > > > > > > > > > > > > > >> separator > > > > > > > > > > > > > > > > > > > > > >> > >> is > > > > > > > > > > > > > > > > > > > > > >> > >>>> not configurable and > should > > > not > > > > > be > > > > > > a > > > > > > > > > > concern > > > > > > > > > > > of > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > implementor > > > > > > > > > > > > > > > > > -- Best, Stanislav
