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 > > > > > > > > > > > of > > > > > > > > > > > >> > >>>> SaslExtensionsCallback interface that > provides > > an > > > > > > > instance > > > > > > > > of > > > > > > > > > > > >> > >>> SaslExtensions > > > > > > > > > > > >> > >>>> . > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>> 2. I agree with Ron that we need > > > mechanism-specific > > > > > > > > > validation > > > > > > > > > > of > > > > > > > > > > > >> the > > > > > > > > > > > >> > >>>> values from SaslExtensions. But I think we > > could > > > do > > > > > the > > > > > > > > > > > validation > > > > > > > > > > > >> in > > > > > > > > > > > >> > >> the > > > > > > > > > > > >> > >>>> appropriate `SaslClient` implementation of > that > > > > > > > mechanism. > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>> I think we could just have a very simple > > > extensions > > > > > > class > > > > > > > > and > > > > > > > > > > > move > > > > > > > > > > > >> > >>>> everything else to appropriate internal > classes > > > of > > > > > the > > > > > > > > > > mechanisms > > > > > > > > > > > >> > using > > > > > > > > > > > >> > >>>> extensions. What do you think? > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>> public class SaslExtensions { > > > > > > > > > > > >> > >>>> private final Map<String, String> > > extensionMap; > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>> public SaslExtensions(Map<String, String> > > > > > > > extensionMap) { > > > > > > > > > > > >> > >>>> this.extensionMap = extensionMap; > > > > > > > > > > > >> > >>>> } > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>> public String extensionValue(String name) { > > > > > > > > > > > >> > >>>> return extensionMap.get(name); > > > > > > > > > > > >> > >>>> } > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>> public Set<String> extensionNames() { > > > > > > > > > > > >> > >>>> return extensionMap.keySet(); > > > > > > > > > > > >> > >>>> } > > > > > > > > > > > >> > >>>> } > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>> > > > > > > > > > > > >> > >>>>> On Sat, Jul 21, 2018 at 9:01 PM, Ron > > Dagostino < > > > > > > > > > > > rndg...@gmail.com > > > > > > > > > > > >> > > > > > > > > > > > > >> > >>> wrote: > > > > > > > > > > > >> > >>>>> > > > > > > > > > > > >> > >>>>> Hi Stanislav and Rajini. If SaslExtensions > is > > > > going > > > > > > to > > > > > > > > part > > > > > > > > > > of > > > > > > > > > > > >> the > > > > > > > > > > > >> > >>> public > > > > > > > > > > > >> > >>>>> API, then it occurred to me that one of the > > > > > > requirements > > > > > > > > of > > > > > > > > > > all > > > > > > > > > > > >> SASL > > > > > > > > > > > >> > >>>>> extensions is that the keys and values need > to > > > > match > > > > > > > > > > > >> > >> mechanism-specific > > > > > > > > > > > >> > >>>>> regular expressions. For example, RFC 5802 > ( > > > > > > > > > > > >> > >>>>> https://tools.ietf.org/html/rfc5802) > > specifies > > > > the > > > > > > > > regular > > > > > > > > > > > >> > >> expressions > > > > > > > > > > > >> > >>> for > > > > > > > > > > > >> > >>>>> the SCRAM-specific SASL mechanisms, and RFC > > > 7628 ( > > > > > > > > > > > >> > >>>>> https://tools.ietf.org/html/rfc7628) > > specifies > > > > > > > different > > > > > > > > > > > regular > > > > > > > > > > > >> > >>>>> expressions for the OAUTHBEARER SASL > > > mechanism. I > > > > > am > > > > > > > > > thinking > > > > > > > > > > > the > > > > > > > > > > > >> > >>>>> SaslExtensions class should probably > provide a > > > way > > > > > to > > > > > > > make > > > > > > > > > > sure > > > > > > > > > > > >> the > > > > > > > > > > > >> > >> keys > > > > > > > > > > > >> > >>>>> and values match the appropriate regular > > > > > expressions. > > > > > > > > What > > > > > > > > > do > > > > > > > > > > > you > > > > > > > > > > > >> > >>> think of > > > > > > > > > > > >> > >>>>> something along the lines of the below > > > definition > > > > > for > > > > > > > the > > > > > > > > > > > >> > >> SaslExtensions > > > > > > > > > > > >> > >>>>> class? It is missing Javadoc and > > > > > > > > > > toString()/hashCode()/equals() > > > > > > > > > > > >> > >>> methods, > > > > > > > > > > > >> > >>>>> of course, but aside from that, do you think > > > this > > > > is > > > > > > > > > > sufficient > > > > > > > > > > > >> and > > > > > > > > > > > >> > >>>>> appropriate? > > > > > > > > > > > >> > >>>>> > > > > > > > > > > > >> > >>>>> Ron > > > > > > > > > > > >> > >>>>> > > > > > > > > > > > >> > >>>>> public class SaslExtensions { > > > > > > > > > > > >> > >>>>> private final Map<String, String> > > > extensionsMap; > > > > > > > > > > > >> > >>>>> > > > > > > > > > > > >> > >>>>> public SaslExtensions(String mapStr, > String > > > > > > > > > > keyValueSeparator, > > > > > > > > > > > >> > >> String > > > > > > > > > > > >> > >>>>> elementSeparator, > > > > > > > > > > > >> > >>>>> Pattern saslNameRegexPattern, > > Pattern > > > > > > > > > > > >> > >> saslValueRegexPattern) > > > > > > > > > > > >> > >>> { > > > > > > > > > > > >> > >>>>> this(Utils.parseMap(mapStr, > > > > keyValueSeparator, > > > > > > > > > > > >> > >> elementSeparator), > > > > > > > > > > > >> > >>>>> saslNameRegexPattern, > saslValueRegexPattern); > > > > > > > > > > > >> > >>>>> } > > > > > > > > > > > >> > >>>>> > > > > > > > > > > > >> > >>>>> public SaslExtensions(Map<String, String> > > > > > > > extensionsMap, > > > > > > > > > > > Pattern > > > > > > > > > > > >> > >>>>> saslNameRegexPattern, > > > > > > > > > > > >> > >>>>> Pattern saslValueRegexPattern) { > > > > > > > > > > > >> > >>>>> Map<String, String> sanitizedCopy = > new > > > > > > > > > > > >> > >>>>> HashMap<>(extensionsMap.size()); > > > > > > > > > > > >> > >>>>> for (Entry<String, String> entry : > > > > > > > > > > > >> extensionsMap.entrySet()) { > > > > > > > > > > > >> > >>>>> if (!saslNameRegexPattern. > > > > > > > > matcher(entry.getKey()). > > > > > > > > > > > >> > matches() > > > > > > > > > > > >> > >>>>> || > > > > > > > > > > > >> > >>>>> > > > !saslValueRegexPattern.matcher(entry.getValue()). > > > > > > > > matches()) > > > > > > > > > > > >> > >>>>> throw new > > > > > > > IllegalArgumentException("Invalid > > > > > > > > > key > > > > > > > > > > or > > > > > > > > > > > >> > >>>>> value"); > > > > > > > > > > > >> > >>>>> sanitizedCopy.put(entry.getKey(), > > > > > > > > > entry.getValue()); > > > > > > > > > > > >> > >>>>> } > > > > > > > > > > > >> > >>>>> this.extensionsMap = > > > > > > > > > > > >> > >> Collections.unmodifiableMap(sanitizedCopy); > > > > > > > > > > > >> > >>>>> } > > > > > > > > > > > >> > >>>>> > > > > > > > > > > > >> > >>>>> public Map<String, String> map() { > > > > > > > > > > > >> > >>>>> return extensionsMap; > > > > > > > > > > > >> > >>>>> } > > > > > > > > > > > >> > >>>>> } > > > > > > > > > > > >> > >>>>> > > > > > > > > > > > >> > >>>>> On Fri, Jul 20, 2018 at 12:49 PM Stanislav > > > > > Kozlovski < > > > > > > > > > > > >> > >>>>> stanis...@confluent.io> > > > > > > > > > > > >> > >>>>> wrote: > > > > > > > > > > > >> > >>>>> > > > > > > > > > > > >> > >>>>>> Hi Ron, > > > > > > > > > > > >> > >>>>>> > > > > > > > > > > > >> > >>>>>> I saw that and decided that would be the > best > > > > > > approach. > > > > > > > > The > > > > > > > > > > > >> current > > > > > > > > > > > >> > >>>>>> ScramExtensions implementation uses a Map > in > > > the > > > > > > public > > > > > > > > > > > >> credentials > > > > > > > > > > > >> > >>> and I > > > > > > > > > > > >> > >>>>>> thought I would follow convention rather > than > > > > > > introduce > > > > > > > > my > > > > > > > > > > own > > > > > > > > > > > >> > thing, > > > > > > > > > > > >> > >>> but > > > > > > > > > > > >> > >>>>>> maybe this is best > > > > > > > > > > > >> > >>>>>> > > > > > > > > > > > >> > >>>>>>> On Fri, Jul 20, 2018 at 8:39 AM Ron > > Dagostino > > > < > > > > > > > > > > > >> rndg...@gmail.com> > > > > > > > > > > > >> > >>> wrote: > > > > > > > > > > > >> > >>>>>>> > > > > > > > > > > > >> > >>>>>>> Hi Stanislav. I'm wondering if we should > > make > > > > > > > > > > SaslExtensions > > > > > > > > > > > >> part > > > > > > > > > > > >> > >> of > > > > > > > > > > > >> > >>>>> the > > > > > > > > > > > >> > >>>>>>> public API. I mentioned this in my review > > of > > > > the > > > > > > PR, > > > > > > > > too > > > > > > > > > > (and > > > > > > > > > > > >> > >> tagged > > > > > > > > > > > >> > >>>>>>> Rajini to get her input). If we add a Map > > to > > > > the > > > > > > > > > Subject's > > > > > > > > > > > >> public > > > > > > > > > > > >> > >>>>>>> credentials we are basically making a > public > > > > > > > commitment > > > > > > > > > that > > > > > > > > > > > any > > > > > > > > > > > >> > Map > > > > > > > > > > > >> > >>>>>>> associated with the public credentials > > defines > > > > the > > > > > > > SASL > > > > > > > > > > > >> extensions > > > > > > > > > > > >> > >> and > > > > > > > > > > > >> > >>>>> we > > > > > > > > > > > >> > >>>>>>> can never add another instance > implementing > > > Map > > > > to > > > > > > the > > > > > > > > > > public > > > > > > > > > > > >> > >>>>>> credentials. > > > > > > > > > > > >> > >>>>>>> That's a very big constraint we are > > committing > > > > to, > > > > > > and > > > > > > > > I'm > > > > > > > > > > > >> > wondering > > > > > > > > > > > >> > >>> if > > > > > > > > > > > >> > >>>>>> we > > > > > > > > > > > >> > >>>>>>> should make SaslExtensions public and > attach > > > an > > > > > > > instance > > > > > > > > > of > > > > > > > > > > > >> that to > > > > > > > > > > > >> > >>> the > > > > > > > > > > > >> > >>>>>>> Subject's public credentials instead. > > > > > > > > > > > >> > >>>>>>> > > > > > > > > > > > >> > >>>>>>> Ron > > > > > > > > > > > >> > >>>>>>> > > > > > > > > > > > >> > >>>>>>> On Thu, Jul 19, 2018 at 8:15 PM Stanislav > > > > > Kozlovski > > > > > > < > > > > > > > > > > > >> > >>>>>>> stanis...@confluent.io> > > > > > > > > > > > >> > >>>>>>> wrote: > > > > > > > > > > > >> > >>>>>>> > > > > > > > > > > > >> > >>>>>>>> I have updated the PR and KIP to address > > the > > > > > > comments > > > > > > > > > made > > > > > > > > > > so > > > > > > > > > > > >> far. > > > > > > > > > > > >> > >>>>>> Please > > > > > > > > > > > >> > >>>>>>>> take another look at them and share your > > > > > thoughts. > > > > > > > > > > > >> > >>>>>>>> KIP: > > > > > > > > > > > >> > >>>>>>>> > > > > > > > > > > > >> > >>>>>>>> > > > > > > > > > > > >> > >>>>>>> > > > > > > > > > > > >> > >>>>>> https://cwiki.apache.org/ > > > > > > confluence/display/KAFKA/KIP- > > > > > > > > > > > >> > >>>>> > > > 342%3A+Add+support+for+Custom+SASL+extensions+in+ > > > > > > > > > > > >> > >>>>> OAuthBearer+authentication > > > > > > > > > > > >> > >>>>>>>> PR: Pull request < > > > > > > > > > > https://github.com/apache/kafka/pull/5379> > > > > > > > > > > > >> > >>>>>>>> > > > > > > > > > > > >> > >>>>>>>> Best, > > > > > > > > > > > >> > >>>>>>>> Stanislav > > > > > > > > > > > >> > >>>>>>>> > > > > > > > > > > > >> > >>>>>>>> On Thu, Jul 19, 2018 at 1:58 PM Stanislav > > > > > > Kozlovski < > > > > > > > > > > > >> > >>>>>>>> stanis...@confluent.io> > > > > > > > > > > > >> > >>>>>>>> wrote: > > > > > > > > > > > >> > >>>>>>>> > > > > > > > > > > > >> > >>>>>>>>> Hi Ron, > > > > > > > > > > > >> > >>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>> Agreed. `SaslExtensionsCallback` will be > > the > > > > > only > > > > > > > > public > > > > > > > > > > API > > > > > > > > > > > >> > >>>>> addition > > > > > > > > > > > >> > >>>>>>> and > > > > > > > > > > > >> > >>>>>>>>> new documentation for the extension > > strings. > > > > > > > > > > > >> > >>>>>>>>> A question that came up - should the > > > > > > > > > LoginCallbackHandler > > > > > > > > > > > >> throw > > > > > > > > > > > >> > an > > > > > > > > > > > >> > >>>>>>>>> exception or simply ignore key/value > > > extension > > > > > > pairs > > > > > > > > who > > > > > > > > > > do > > > > > > > > > > > >> not > > > > > > > > > > > >> > >>>>> match > > > > > > > > > > > >> > >>>>>>> the > > > > > > > > > > > >> > >>>>>>>>> validation regex pattern? I guess it > would > > > be > > > > > > better > > > > > > > > to > > > > > > > > > > > >> throw, as > > > > > > > > > > > >> > >>>>> to > > > > > > > > > > > >> > >>>>>>>> avoid > > > > > > > > > > > >> > >>>>>>>>> confusion. > > > > > > > > > > > >> > >>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>> And yes, I will make sure the key/value > > are > > > > > > > validated > > > > > > > > on > > > > > > > > > > the > > > > > > > > > > > >> > >> client > > > > > > > > > > > >> > >>>>>> as > > > > > > > > > > > >> > >>>>>>>>> well as in the server. Even then, I > > > structured > > > > > the > > > > > > > > > > > >> > >>>>>>>> getNegotiatedProperty() > > > > > > > > > > > >> > >>>>>>>>> method such that the OAUTHBEARER.token > can > > > > never > > > > > > be > > > > > > > > > > > >> overridden. I > > > > > > > > > > > >> > >>>>>>>>> considered adding a test for that, but I > > > > figured > > > > > > > > having > > > > > > > > > > the > > > > > > > > > > > >> regex > > > > > > > > > > > >> > >>>>>>>>> validation be enough of a guarantee. > > > > > > > > > > > >> > >>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>> On Thu, Jul 19, 2018 at 9:49 AM Ron > > > Dagostino > > > > < > > > > > > > > > > > >> rndg...@gmail.com > > > > > > > > > > > >> > > > > > > > > > > > > > >> > >>>>>>> wrote: > > > > > > > > > > > >> > >>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>> Hi Rajini and Stanislav. Rajini, yes, > I > > > > think > > > > > > you > > > > > > > > are > > > > > > > > > > > right > > > > > > > > > > > >> > >> about > > > > > > > > > > > >> > >>>>>> the > > > > > > > > > > > >> > >>>>>>>>>> login callback handler being more > > > appropriate > > > > > for > > > > > > > > > > > retrieving > > > > > > > > > > > >> the > > > > > > > > > > > >> > >>>>>> SASL > > > > > > > > > > > >> > >>>>>>>>>> extensions than the login module itself > > > (how > > > > > many > > > > > > > > times > > > > > > > > > > am > > > > > > > > > > > I > > > > > > > > > > > >> > >> going > > > > > > > > > > > >> > >>>>>> to > > > > > > > > > > > >> > >>>>>>>> have > > > > > > > > > > > >> > >>>>>>>>>> to be encouraged to leverage the > callback > > > > > > > handlers?!? > > > > > > > > > > lol). > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule should ask its > > login > > > > > > > callback > > > > > > > > > > > handler > > > > > > > > > > > >> to > > > > > > > > > > > >> > >>>>>> handle > > > > > > > > > > > >> > >>>>>>>> an > > > > > > > > > > > >> > >>>>>>>>>> instance of SaslExtensionsCallback in > > > > addition > > > > > to > > > > > > > an > > > > > > > > > > > >> instance of > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerTokenCallback, and the > default > > > > login > > > > > > > > > callback > > > > > > > > > > > >> handler > > > > > > > > > > > >> > >>>>>>>>>> implementation > > > > > > > > > (OAuthBearerUnsecuredLoginCallbackHandler) > > > > > > > > > > > >> > should > > > > > > > > > > > >> > >>>>>>> either > > > > > > > > > > > >> > >>>>>>>>>> return an empty map via callback or it > > > should > > > > > > > > recognize > > > > > > > > > > > >> > >> additional > > > > > > > > > > > >> > >>>>>>> JAAS > > > > > > > > > > > >> > >>>>>>>>>> module options of the form > > > > > > > > > > > >> > >>>>>>> > > unsecuredLoginExtension_<extensionName>=value > > > > > > > > > > > >> > >>>>>>>>>> so > > > > > > > > > > > >> > >>>>>>>>>> that arbitrary extensions can be added > in > > > > > > > development > > > > > > > > > and > > > > > > > > > > > >> test > > > > > > > > > > > >> > >>>>>>> scenarios > > > > > > > > > > > >> > >>>>>>>>>> (similar to how arbitrary claims on > > > unsecured > > > > > > > tokens > > > > > > > > > can > > > > > > > > > > be > > > > > > > > > > > >> > >>>>> created > > > > > > > > > > > >> > >>>>>> in > > > > > > > > > > > >> > >>>>>>>>>> those scenarios via the JAAS module > > options > > > > > > > > > > > >> > >>>>>>>>>> > > > unsecuredLoginStringClaim_<claimName>=value, > > > > > > etc.). > > > > > > > > > Then > > > > > > > > > > > the > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule can add a map of > > any > > > > > > > > extensions > > > > > > > > > to > > > > > > > > > > > the > > > > > > > > > > > >> > >>>>>>> Subject's > > > > > > > > > > > >> > >>>>>>>>>> public credentials where the default > SASL > > > > > client > > > > > > > > > callback > > > > > > > > > > > >> > handler > > > > > > > > > > > >> > >>>>>>> class > > > > > > > > > > > >> > >>>>>>>> ( > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerSaslClientCallbackHandler) > can > > > be > > > > > > > > amended to > > > > > > > > > > > >> support > > > > > > > > > > > >> > >>>>>>>>>> SaslExtensionsCallback and look on the > > > > Subject > > > > > > > > > > accordingly. > > > > > > > > > > > >> > >> There > > > > > > > > > > > >> > >>>>>>> would > > > > > > > > > > > >> > >>>>>>>>>> be > > > > > > > > > > > >> > >>>>>>>>>> no need to implement a custom > > > > > > > > > > sasl.client.callback.handler. > > > > > > > > > > > >> > class > > > > > > > > > > > >> > >>>>> in > > > > > > > > > > > >> > >>>>>>> this > > > > > > > > > > > >> > >>>>>>>>>> case, and no logic would need to be > moved > > > to > > > > a > > > > > > > public > > > > > > > > > > > static > > > > > > > > > > > >> > >>>>> method > > > > > > > > > > > >> > >>>>>> on > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule as I had > proposed > > > (at > > > > > > least > > > > > > > > not > > > > > > > > > > > right > > > > > > > > > > > >> > now, > > > > > > > > > > > >> > >>>>>>> anyway > > > > > > > > > > > >> > >>>>>>>>>> -- > > > > > > > > > > > >> > >>>>>>>>>> there may come a time when a need for a > > > > custom > > > > > > > > > > > >> > >>>>>>>>>> sasl.client.callback.handler.class is > > > > > > identified, > > > > > > > > and > > > > > > > > > at > > > > > > > > > > > that > > > > > > > > > > > >> > >>>>> point > > > > > > > > > > > >> > >>>>>>> the > > > > > > > > > > > >> > >>>>>>>>>> default implementation would either > have > > to > > > > > made > > > > > > > part > > > > > > > > > of > > > > > > > > > > > the > > > > > > > > > > > >> > >>>>> public > > > > > > > > > > > >> > >>>>>>> API > > > > > > > > > > > >> > >>>>>>>>>> with protected rather than private > > methods > > > so > > > > > it > > > > > > > > could > > > > > > > > > be > > > > > > > > > > > >> > >> directly > > > > > > > > > > > >> > >>>>>>>>>> extended > > > > > > > > > > > >> > >>>>>>>>>> or its logic would have to be moved to > > > public > > > > > > > static > > > > > > > > > > > methods > > > > > > > > > > > >> on > > > > > > > > > > > >> > >>>>>>>>>> OAuthBearerLoginModule). > > > > > > > > > > > >> > >>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>> So, to try to summarize, I think > > > > > > > > SaslExtensionsCallback > > > > > > > > > > > will > > > > > > > > > > > >> be > > > > > > > > > > > >> > >>>>> the > > > > > > > > > > > >> > >>>>>>> only > > > > > > > > > > > >> > >>>>>>>>>> public API addition due to this KIP in > > > terms > > > > of > > > > > > > code, > > > > > > > > > and > > > > > > > > > > > >> then > > > > > > > > > > > >> > >>>>> maybe > > > > > > > > > > > >> > >>>>>>> the > > > > > > > > > > > >> > >>>>>>>>>> recognition of the > > > unsecuredLoginExtension_< > > > > > > > > > > > >> > extensionName>=value > > > > > > > > > > > >> > >>>>>>> module > > > > > > > > > > > >> > >>>>>>>>>> options in the default unsecured case > > > (which > > > > > > would > > > > > > > > be a > > > > > > > > > > > >> > >>>>>> documentation > > > > > > > > > > > >> > >>>>>>>>>> change and an internal implementation > > issue > > > > > > rather > > > > > > > > > than a > > > > > > > > > > > >> public > > > > > > > > > > > >> > >>>>> API > > > > > > > > > > > >> > >>>>>>> in > > > > > > > > > > > >> > >>>>>>>>>> terms of code). And then also the fact > > > that > > > > > > > > extension > > > > > > > > > > > names > > > > > > > > > > > >> and > > > > > > > > > > > >> > >>>>>>> values > > > > > > > > > > > >> > >>>>>>>>>> are > > > > > > > > > > > >> > >>>>>>>>>> accessed on the server side via > > negotiated > > > > > > > > properties. > > > > > > > > > > Do > > > > > > > > > > > I > > > > > > > > > > > >> > have > > > > > > > > > > > >> > >>>>>> that > > > > > > > > > > > >> > >>>>>>>>>> summary right? > > > > > > > > > > > >> > >>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>> One thing I want to note is that the > code > > > > needs > > > > > > to > > > > > > > > make > > > > > > > > > > > sure > > > > > > > > > > > >> the > > > > > > > > > > > >> > >>>>>>>> extension > > > > > > > > > > > >> > >>>>>>>>>> names are composed of only ALPHA > [a-zA-Z] > > > > > > > characters > > > > > > > > as > > > > > > > > > > per > > > > > > > > > > > >> the > > > > > > > > > > > >> > >>>>> spec > > > > > > > > > > > >> > >>>>>>>> (not > > > > > > > > > > > >> > >>>>>>>>>> only for that reason, but to also make > > sure > > > > the > > > > > > > token > > > > > > > > > > > >> available > > > > > > > > > > > >> > >> at > > > > > > > > > > > >> > >>>>>> the > > > > > > > > > > > >> > >>>>>>>>>> OAUTHBEARER.token negotiated property > > can't > > > > be > > > > > > > > > > > overwritten). > > > > > > > > > > > >> > >>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>> Ron > > > > > > > > > > > >> > >>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>> On Thu, Jul 19, 2018 at 12:43 PM > > Stanislav > > > > > > > Kozlovski > > > > > > > > < > > > > > > > > > > > >> > >>>>>>>>>> stanis...@confluent.io> > > > > > > > > > > > >> > >>>>>>>>>> wrote: > > > > > > > > > > > >> > >>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>> Hey Ron, > > > > > > > > > > > >> > >>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>> Come to think of it, I think what > Rajini > > > > said > > > > > > > makes > > > > > > > > > more > > > > > > > > > > > >> sense > > > > > > > > > > > >> > >>>>>> than > > > > > > > > > > > >> > >>>>>>> my > > > > > > > > > > > >> > >>>>>>>>>>> initial proposal. Having the > > > > > > > > > > > >> OAuthBearerClientCallbackHandler > > > > > > > > > > > >> > >>>>>>> populate > > > > > > > > > > > >> > >>>>>>>>>>> SaslExtensionsCallback by taking a Map > > > from > > > > > the > > > > > > > > > Subject > > > > > > > > > > > >> would > > > > > > > > > > > >> > >>>>> ease > > > > > > > > > > > >> > >>>>>>>>>> users' > > > > > > > > > > > >> > >>>>>>>>>>> implementation - they'd only have to > > > > provide a > > > > > > > login > > > > > > > > > > > >> callback > > > > > > > > > > > >> > >>>>>>> handler > > > > > > > > > > > >> > >>>>>>>>>> which > > > > > > > > > > > >> > >>>>>>>>>>> attaches extensions to the Subject. > > > > > > > > > > > >> > >>>>>>>>>>> I will now update the PR and the > > examples > > > in > > > > > the > > > > > > > > KIP. > > > > > > > > > > Let > > > > > > > > > > > me > > > > > > > > > > > >> > >>>>> know > > > > > > > > > > > >> > >>>>>>> what > > > > > > > > > > > >> > >>>>>>>>>> you > > > > > > > > > > > >> > >>>>>>>>>>> think > > > > > > > > > > > >> > >>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>> Hi Rajini, > > > > > > > > > > > >> > >>>>>>>>>>> Yes, I will switch both classes to > > > > > > private/public > > > > > > > as > > > > > > > > > it > > > > > > > > > > > >> makes > > > > > > > > > > > >> > >>>>>> total > > > > > > > > > > > >> > >>>>>>>>>> sense. > > > > > > > > > > > >> > >>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>> Best, > > > > > > > > > > > >> > >>>>>>>>>>> Stanislav > > > > > > > > > > > >> > >>>>>>>>>>> On Thu, Jul 19, 2018 at 9:02 AM Rajini > > > > > Sivaram < > > > > > > > > > > > >> > >>>>>>>> rajinisiva...@gmail.com > > > > > > > > > > > >> > >>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>> wrote: > > > > > > > > > > > >> > >>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>> Hi Stanislav, > > > > > > > > > > > >> > >>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>> Thanks for the KIP. Since > > SaslExtensions > > > > will > > > > > > be > > > > > > > an > > > > > > > > > > > >> internal > > > > > > > > > > > >> > >>>>>>> class, > > > > > > > > > > > >> > >>>>>>>>>> can > > > > > > > > > > > >> > >>>>>>>>>>> we > > > > > > > > > > > >> > >>>>>>>>>>>> remove it from the KIP to avoid > > > confusion? > > > > > > Also, > > > > > > > > can > > > > > > > > > we > > > > > > > > > > > add > > > > > > > > > > > >> > >>>>> the > > > > > > > > > > > >> > >>>>>>>>>> package > > > > > > > > > > > >> > >>>>>>>>>>>> name for SaslExtensionsCallback? The > PR > > > has > > > > > it > > > > > > in > > > > > > > > > > > >> > >>>>>>>>>>>> org.apache.kafka.common.security > which > > is > > > > an > > > > > > > > internal > > > > > > > > > > > >> > >>>>> package. > > > > > > > > > > > >> > >>>>>> As > > > > > > > > > > > >> > >>>>>>> a > > > > > > > > > > > >> > >>>>>>>>>>> public > > > > > > > > > > > >> > >>>>>>>>>>>> class, it could be in > > > > > > > > > > > >> org.apache.kafka.common.security.auth. > > > > > > > > > > > >> > >>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>> Regards, > > > > > > > > > > > >> > >>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>> Rajini > > > > > > > > > > > >> > >>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>> On Thu, Jul 19, 2018 at 4:50 PM, > Rajini > > > > > > Sivaram < > > > > > > > > > > > >> > >>>>>>>>>> rajinisiva...@gmail.com > > > > > > > > > > > >> > >>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>> wrote: > > > > > > > > > > > >> > >>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>>> Hi Ron, > > > > > > > > > > > >> > >>>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>>> Is there a reason why wouldn't want > to > > > > > provide > > > > > > > > > > > extensions > > > > > > > > > > > >> > >>>>>> using > > > > > > > > > > > >> > >>>>>>> a > > > > > > > > > > > >> > >>>>>>>>>> login > > > > > > > > > > > >> > >>>>>>>>>>>>> callback handler in the same way as > we > > > > > inject > > > > > > > > > tokens? > > > > > > > > > > > The > > > > > > > > > > > >> > >>>>>>> easiest > > > > > > > > > > > >> > >>>>>>>>>> way > > > > > > > > > > > >> > >>>>>>>>>>> to > > > > > > > > > > > >> > >>>>>>>>>>>>> inject custom extensions would be > > using > > > > the > > > > > > JAAS > > > > > > > > > > config. > > > > > > > > > > > >> So > > > > > > > > > > > >> > >>>>> we > > > > > > > > > > > >> > >>>>>>>> could > > > > > > > > > > > >> > >>>>>>>>>>> have > > > > > > > > > > > >> > >>>>>>>>>>>>> both OAuthBearerTokenCallback and > > > > > > > > > > SaslExtensionsCallback > > > > > > > > > > > >> > >>>>>>>> processed > > > > > > > > > > > >> > >>>>>>>>>> by > > > > > > > > > > > >> > >>>>>>>>>>> a > > > > > > > > > > > >> > >>>>>>>>>>>>> login callback handler. And the map > > > > returned > > > > > > by > > > > > > > > > > > >> > >>>>>>>>>> SaslExtensionsCallback > > > > > > > > > > > >> > >>>>>>>>>>>>> could be added to Subject by the > > default > > > > > > > > > > > >> > >>>>>>>>>>>> OAuthBearerSaslClientCallbackHandler. > > > > > > > > > > > >> > >>>>>>>>>>>>> Since OAuth users have to provide a > > > login > > > > > > > callback > > > > > > > > > > > handler > > > > > > > > > > > >> > >>>>>>> anyway, > > > > > > > > > > > >> > >>>>>>>>>>>> wouldn't > > > > > > > > > > > >> > >>>>>>>>>>>>> it be a better fit? > > > > > > > > > > > >> > >>>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>>> Thank you, > > > > > > > > > > > >> > >>>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>>> Rajini > > > > > > > > > > > >> > >>>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>>> On Thu, Jul 19, 2018 at 4:08 PM, Ron > > > > > > Dagostino < > > > > > > > > > > > >> > >>>>>>> rndg...@gmail.com > > > > > > > > > > > >> > >>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>> wrote: > > > > > > > > > > > >> > >>>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>>>> Hi Stanislav. > > > > > > > > > > > >> > >>>>>>>>>>>>>> > > > > > > > > > > > >> > >>>>>>>>>>>>>> Implementers of a custom > > > > > > > > > > sasl.client.callback.handler. > > > > > > > > > > > >> > >>>>> class > > > > > > > > > > > >> > >>>>>>> must > > > > > > > > > > > >> > >>>>>>>> be > > > > > > > > > > > >> > >>>>>>>>>>> sure > > > > > > > > > > > >> > >>>>>>>>>>>>>> to > > > > > > > > > > > >> > >>>>>>>>>>>>>> provide the existing logic in > > > > > > > > > > > >> > >>>>>>>>>>>>>> org.apache.kafka.common. > > > > > > security.oauthbearer. > > > > > > > > > > > >> > >>>>> internals.OAuth > > > > > > > > > > > >> > >>>>>>>>>>>>>> BearerSaslClientCallbackHandler > > > > > > > > > > > >> > >>>>>>>>>>>>>> that handles instances of > > > > > > > > OAuthBearerTokenCallback > > > > > > > > > > (by > > > > > > > > > > > >> > >>>>>>> retrieving > > > > > > > > > > > >> > >>>>>>>>>> the > > > > > > > > > > > >> > >>>>>>>>>>>>>> private credential from the > > Subject); a > > > > > > custom > > > > > > > > > > > >> > >>>>> implementation > > > > > > > > > > > >> > >>>>>>>> that > > > > > > > > > > > >> > >>>>>>>>>>> fails > > > > > > > > > > > >> > >>>>>>>>>>>>>> to > > > > > > > > > > > >> > >>>>>>>>>>>> -- Best, Stanislav
