Gwen, In cases where you want completely different callbacks for different mechanisms, I was thinking that the choice would be between a map of classes (mechanism -> callbackHandler class) or a delegator class that chooses the appropriate callback handler class based on mechanism. I chose the latter since it makes it easier to configure in Kafka. Since we create a callback handler for each channel and configure it with the client-selected mechanism, it is straightforward to have one wrapper class that delegates to the right mechanism-specific class to handle callbacks. In many cases, a single class may be sufficient (the PR uses a single callback class for Kerberos and PLAIN). I do see your point about the flexibility that multiple classes would provide, but since you need to be able to associate the callback with a mechanism for this to be useful, I am not sure if just a list would add value.
Login class is slightly different since the proposal is to use a single login context with multiple login modules to handle multiple mechanisms. In this case, you want to perform login for all the mechanisms that are enabled. And you want to call loginContext.login() only once. Again, you can delegate to multiple classes if you wish to add some complex mechanism-specific logic, but a single login class makes the mapping to a single login context and the login cache more obvious (the PR has a test that includes Kerberos and PLAIN). Thoughts? On Mon, Mar 7, 2016 at 9:57 PM, Gwen Shapira <g...@confluent.io> wrote: > Hi, > > Before I can vote on this KIP, I have two additional questions / > comments on the new configuration: > > 1. sasl.callback.handler.class - it looks like we want a single class > that implements all mechanisms. I think this will make it difficult to > extend since the only way I can add a mechanism will be by > implementing every single existing mechanism (otherwise customers will > need to choose between new and existing when selecting which class to > use). If Microsoft releases a proprietary "AD Mechanism" and Oracle > releases "OID mechanism", there will be no class that implements both. > Can we make it a list of classes instead? I realize this complicates > the code a bit (some kind of factory will be required to choose the > right class to use), but important IMO. > 2. similar for sasl.login.class - if I have a class for Kerberos (with > refresh thread) and a class for "plain", we need to be able to load > both. > > Gwen > > On Wed, Mar 2, 2016 at 12:30 AM, Rajini Sivaram > <rajinisiva...@googlemail.com> wrote: > > Jun, > > > > Thanks, I have added a note to the KIP. I will add a comment in the > > implementation and also add a unit test to ensure that conflicts are > > avoided when version number is modified. > > > > On Tue, Mar 1, 2016 at 5:43 PM, Jun Rao <j...@confluent.io> wrote: > > > >> Rajini, > >> > >> Thanks for the explanation. For 1, this implies that we have to be > careful > >> with changing the 2-byte version in the future to avoid conflict. Could > you > >> document this in the KIP and also in the implementation? > >> > >> Jun > >> > >> On Tue, Mar 1, 2016 at 2:47 AM, Rajini Sivaram < > >> rajinisiva...@googlemail.com > >> > wrote: > >> > >> > Jun, > >> > > >> > Thank you for the review. > >> > > >> > > >> > 1. With GSSAPI, the first context establishment packet starts with > the > >> > byte 0x60 (tag for APPLICATION-0) followed by a variable-length > >> encoded > >> > size, followed by various tags and contents. And the packet also > >> > contains a > >> > checksum. This is completely different from the mechanism packet > from > >> > Kafka > >> > clients which start with a two-byte version set to zero currently, > >> > followed > >> > by just a String mechanism. > >> > 2. Agreed, I have removed the version from the server response in > the > >> > KIP. Thanks. > >> > > >> > > >> > On Tue, Mar 1, 2016 at 2:33 AM, Jun Rao <j...@confluent.io> wrote: > >> > > >> > > Rajini, > >> > > > >> > > Thanks for the updates. Just a couple of minor comments. > >> > > > >> > > 1. With the default GSSAPI, what's the first packet that the client > >> sends > >> > > to the server? Is that completely different from the packet format > that > >> > we > >> > > will use for non-GSSAPI mechanisms? > >> > > > >> > > 2. In the server response, it doesn't seem that we need to include > the > >> > > version since the client knows the version of the request that it > >> sends. > >> > > > >> > > Jun > >> > > > >> > > On Mon, Feb 29, 2016 at 10:14 AM, Rajini Sivaram < > >> > > rajinisiva...@googlemail.com> wrote: > >> > > > >> > > > Harsha, > >> > > > > >> > > > Thank you for the review. I will wait another day to see if there > is > >> > more > >> > > > feedback and then start a voting thread. > >> > > > > >> > > > Rajini > >> > > > > >> > > > On Mon, Feb 29, 2016 at 2:51 PM, Harsha <ka...@harsha.io> wrote: > >> > > > > >> > > > > Rajini, > >> > > > > Thanks for the changes to the KIP. It looks good > to > >> > me. I > >> > > > > think we can move to voting. > >> > > > > Thanks, > >> > > > > Harsha > >> > > > > > >> > > > > On Mon, Feb 29, 2016, at 12:43 AM, Rajini Sivaram wrote: > >> > > > > > I have added some more detail to the KIP based on the > discussion > >> in > >> > > the > >> > > > > > last KIP meeting to simplify support for multiple mechanisms. > >> Have > >> > > also > >> > > > > > changed the property names to reflect this. > >> > > > > > > >> > > > > > Also updated the PR in > >> > > > https://issues.apache.org/jira/browse/KAFKA-3149 > >> > > > > > to > >> > > > > > reflect the KIP. > >> > > > > > > >> > > > > > Any feedback is appreciated. > >> > > > > > > >> > > > > > > >> > > > > > On Tue, Feb 23, 2016 at 9:36 PM, Rajini Sivaram < > >> > > > > > rajinisiva...@googlemail.com> wrote: > >> > > > > > > >> > > > > > > I have updated the KIP based on the discussion in the KIP > >> meeting > >> > > > > today. > >> > > > > > > > >> > > > > > > Comments and feedback are welcome. > >> > > > > > > > >> > > > > > > On Wed, Feb 3, 2016 at 7:20 PM, Rajini Sivaram < > >> > > > > > > rajinisiva...@googlemail.com> wrote: > >> > > > > > > > >> > > > > > >> Hi Harsha, > >> > > > > > >> > >> > > > > > >> Thank you for the review. Can you clarify - I think you are > >> > saying > >> > > > > that > >> > > > > > >> the client should send its mechanism over the wire to the > >> > server. > >> > > Is > >> > > > > that > >> > > > > > >> correct? The exchange is slightly different in the KIP > (the PR > >> > > > > matches the > >> > > > > > >> KIP) from the one you described to enable interoperability > >> with > >> > > > > 0.9.0.0. > >> > > > > > >> > >> > > > > > >> > >> > > > > > >> On Wed, Feb 3, 2016 at 1:56 PM, Harsha <m...@harsha.io> > >> wrote: > >> > > > > > >> > >> > > > > > >>> Rajini, > >> > > > > > >>> I looked at the PR you have. I think its better > >> with > >> > > > your > >> > > > > > >>> earlier approach rather than extending the > >> protocol. > >> > > > > > >>> What I was thinking initially is, Broker has a config > option > >> of > >> > > say > >> > > > > > >>> sasl.mechanism = GSSAPI, PLAIN > >> > > > > > >>> and the client can have similar config of > >> sasl.mechanism=PLAIN. > >> > > > > Client > >> > > > > > >>> can send its sasl mechanism before the handshake starts > and > >> if > >> > > the > >> > > > > > >>> broker accepts that particular mechanism than it can go > ahead > >> > > with > >> > > > > > >>> handshake otherwise return a error saying that the > mechanism > >> > not > >> > > > > > >>> allowed. > >> > > > > > >>> > >> > > > > > >>> Thanks, > >> > > > > > >>> Harsha > >> > > > > > >>> > >> > > > > > >>> On Wed, Feb 3, 2016, at 04:58 AM, Rajini Sivaram wrote: > >> > > > > > >>> > A slightly different approach for supporting different > SASL > >> > > > > mechanisms > >> > > > > > >>> > within a broker is to allow the same "*security > protocol*" > >> to > >> > > be > >> > > > > used > >> > > > > > >>> on > >> > > > > > >>> > different ports with different configuration options. An > >> > > > advantage > >> > > > > of > >> > > > > > >>> > this > >> > > > > > >>> > approach is that it extends the configurability of not > just > >> > > SASL, > >> > > > > but > >> > > > > > >>> any > >> > > > > > >>> > protocol. For instance, it would enable the use of SSL > with > >> > > > mutual > >> > > > > > >>> client > >> > > > > > >>> > authentication on one port or different certificate > chains > >> on > >> > > > > another. > >> > > > > > >>> > And > >> > > > > > >>> > it avoids the need for SASL mechanism negotiation. > >> > > > > > >>> > > >> > > > > > >>> > Kafka would have the same "*security protocols" > *defined as > >> > > > today, > >> > > > > but > >> > > > > > >>> > with > >> > > > > > >>> > (a single) configurable SASL mechanism. To have > different > >> > > > > > >>> configurations > >> > > > > > >>> > of > >> > > > > > >>> > a protocol within a broker, users can define new > protocol > >> > names > >> > > > > which > >> > > > > > >>> are > >> > > > > > >>> > configured versions of existing protocols, perhaps using > >> just > >> > > > > > >>> > configuration > >> > > > > > >>> > entries and no additional code. > >> > > > > > >>> > > >> > > > > > >>> > For example: > >> > > > > > >>> > > >> > > > > > >>> > A single mechanism broker would be configured as: > >> > > > > > >>> > > >> > > > > > >>> > listeners=SASL_SSL://:9092 > >> > > > > > >>> > sasl.mechanism=GSSAPI > >> > > > > > >>> > sasl.kerberos.class.name=kafka > >> > > > > > >>> > ... > >> > > > > > >>> > > >> > > > > > >>> > > >> > > > > > >>> > And a multi-mechanism broker would be configured as: > >> > > > > > >>> > > >> > > > > > >>> > listeners=gssapi://:9092,plain://:9093,custom://:9094 > >> > > > > > >>> > gssapi.security.protocol=SASL_SSL > >> > > > > > >>> > gssapi.sasl.mechanism=GSSAPI > >> > > > > > >>> > gssapi.sasl.kerberos.class.name=kafka > >> > > > > > >>> > ... > >> > > > > > >>> > plain.security.protocol=SASL_SSL > >> > > > > > >>> > plain.sasl.mechanism=PLAIN > >> > > > > > >>> > .. > >> > > > > > >>> > custom.security.protocol=SASL_PLAINTEXT > >> > > > > > >>> > custom.sasl.mechanism=CUSTOM > >> > > > > > >>> > > >> > > custom.sasl.callback.handler.class=example.CustomCallbackHandler > >> > > > > > >>> > > >> > > > > > >>> > > >> > > > > > >>> > > >> > > > > > >>> > This is still a big change because it affects the > currently > >> > > fixed > >> > > > > > >>> > enumeration of security protocol definitions, but one > that > >> is > >> > > > > perhaps > >> > > > > > >>> > more > >> > > > > > >>> > flexible than defining every new SASL mechanism as a new > >> > > security > >> > > > > > >>> > protocol. > >> > > > > > >>> > > >> > > > > > >>> > Thoughts? > >> > > > > > >>> > > >> > > > > > >>> > > >> > > > > > >>> > On Tue, Feb 2, 2016 at 12:20 PM, Rajini Sivaram < > >> > > > > > >>> > rajinisiva...@googlemail.com> wrote: > >> > > > > > >>> > > >> > > > > > >>> > > As Ismael has said, we do not have a requirement to > >> support > >> > > > > multiple > >> > > > > > >>> > > protocols in a broker. But I agree with Jun's > observation > >> > > that > >> > > > > some > >> > > > > > >>> > > companies might want to support a different > >> authentication > >> > > > > mechanism > >> > > > > > >>> for > >> > > > > > >>> > > internal users or partners. For instance, we do use > two > >> > > > different > >> > > > > > >>> > > authentication mechanisms, it just so happens that we > are > >> > > able > >> > > > > to use > >> > > > > > >>> > > certificate-based authentication for internal users, > and > >> > > hence > >> > > > > don't > >> > > > > > >>> > > require multiple SASL mechanisms in a broker. > >> > > > > > >>> > > > >> > > > > > >>> > > As Tao has pointed out, mechanism negotiation is a > common > >> > > usage > >> > > > > > >>> pattern. > >> > > > > > >>> > > Many existing protocols that support SASL do already > use > >> > this > >> > > > > > >>> pattern. AMQP > >> > > > > > >>> > > ( > >> > > > > > >>> > > > >> > > > > > >>> > >> > > > > > >> > > > > >> > > > >> > > >> > http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-security-v1.0-os.html#type-sasl-mechanisms > >> > > > > > >>> ), > >> > > > > > >>> > > which, as a messaging protocol maybe closer to Kafka > in > >> use > >> > > > cases > >> > > > > > >>> than > >> > > > > > >>> > > Zookeeper, is an example. Other examples where the > client > >> > > > > negotiates > >> > > > > > >>> or > >> > > > > > >>> > > sends SASL mechanism to server include ACAP that is > used > >> as > >> > > an > >> > > > > > >>> example in > >> > > > > > >>> > > the SASL RFCs, POP3, LDAP, SMTP etc. This is not to > say > >> > that > >> > > > > Kafka > >> > > > > > >>> > > shouldn't use a different type of mechanism selection > >> that > >> > > fits > >> > > > > > >>> better with > >> > > > > > >>> > > the existing Kafka design. Just that negotiation is a > >> > common > >> > > > > pattern > >> > > > > > >>> and > >> > > > > > >>> > > since we typically turn on javax.net.debug to debug > TLS > >> > > > > negotiation > >> > > > > > >>> issues, > >> > > > > > >>> > > having to use Kafka logging to debug SASL negotiation > >> > issues > >> > > is > >> > > > > not > >> > > > > > >>> that > >> > > > > > >>> > > dissimilar. > >> > > > > > >>> > > > >> > > > > > >>> > > > >> > > > > > >>> > > On Tue, Feb 2, 2016 at 6:12 AM, tao xiao < > >> > > xiaotao...@gmail.com > >> > > > > > >> > > > > > >>> wrote: > >> > > > > > >>> > > > >> > > > > > >>> > >> I am the author of KIP-44. I hope my use case will > add > >> > some > >> > > > > values > >> > > > > > >>> to this > >> > > > > > >>> > >> discussion. The reason I raised KIP44 is that I want > to > >> be > >> > > > able > >> > > > > to > >> > > > > > >>> > >> implement a custom security protocol that can fulfill > >> the > >> > > need > >> > > > > of my > >> > > > > > >>> > >> company. As pointed out by Ismael KIP-43 now > supports a > >> > > > > pluggable > >> > > > > > >>> way to > >> > > > > > >>> > >> inject custom security provider to SASL I think it is > >> > enough > >> > > > to > >> > > > > > >>> cover the > >> > > > > > >>> > >> use case I have and address the concerns raised in > >> KIP-44. > >> > > > > > >>> > >> > >> > > > > > >>> > >> For multiple security protocols support > simultaneously > >> it > >> > is > >> > > > not > >> > > > > > >>> needed in > >> > > > > > >>> > >> my use case and I don't foresee it is needed in the > >> future > >> > > but > >> > > > > as i > >> > > > > > >>> said > >> > > > > > >>> > >> this is my use case only there may be other use cases > >> that > >> > > > need > >> > > > > it. > >> > > > > > >>> But if > >> > > > > > >>> > >> we want to support it in the future I prefer to get > it > >> > right > >> > > > at > >> > > > > the > >> > > > > > >>> first > >> > > > > > >>> > >> place given the fact that security protocol is an > ENUM > >> and > >> > > if > >> > > > we > >> > > > > > >>> stick to > >> > > > > > >>> > >> that implementation it is very hard to extend in the > >> > future > >> > > > > when we > >> > > > > > >>> decide > >> > > > > > >>> > >> multiple security protocols is needed. > >> > > > > > >>> > >> > >> > > > > > >>> > >> Protocol negotiation is a very common usage pattern > in > >> > > > security > >> > > > > > >>> domain. As > >> > > > > > >>> > >> suggested in Java SASL doc > >> > > > > > >>> > >> > >> > > > > > >>> > >> > >> > > > > > >>> > >> > > > > > >> > > > > >> > > > >> > > >> > http://docs.oracle.com/javase/7/docs/technotes/guides/security/sasl/sasl-refguide.html > >> > > > > > >>> > >> client > >> > > > > > >>> > >> first sends out a packet to server and server > responds > >> > with > >> > > a > >> > > > > list > >> > > > > > >>> of > >> > > > > > >>> > >> mechanisms it supports. This is very similar to > SSL/TLS > >> > > > > negotiation. > >> > > > > > >>> > >> > >> > > > > > >>> > >> On Tue, 2 Feb 2016 at 06:39 Ismael Juma < > >> > ism...@juma.me.uk> > >> > > > > wrote: > >> > > > > > >>> > >> > >> > > > > > >>> > >> > On Mon, Feb 1, 2016 at 7:04 PM, Gwen Shapira < > >> > > > > g...@confluent.io> > >> > > > > > >>> wrote: > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > > Looking at "existing solutions", it looks like > >> > Zookeeper > >> > > > > allows > >> > > > > > >>> > >> plugging > >> > > > > > >>> > >> > in > >> > > > > > >>> > >> > > any SASL mechanism, but the server will only > support > >> > one > >> > > > > > >>> mechanism at > >> > > > > > >>> > >> a > >> > > > > > >>> > >> > > time. > >> > > > > > >>> > >> > > > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > This was the original proposal from Rajini as that > is > >> > > enough > >> > > > > for > >> > > > > > >>> their > >> > > > > > >>> > >> > needs. > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > > If this is good enough for our use-case (do we > >> > actually > >> > > > > need to > >> > > > > > >>> > >> support > >> > > > > > >>> > >> > > multiple mechanisms at once?), it will simplify > >> life a > >> > > lot > >> > > > > for > >> > > > > > >>> us ( > >> > > > > > >>> > >> > > > >> > > > > > >>> > >> > >> > > > > > >>> > >> > > > > > >> > > > >> > https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zookeeper+and+SASL > >> > > > > > >>> > >> > ) > >> > > > > > >>> > >> > > > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > The current thinking is that it would be useful to > >> > support > >> > > > > > >>> multiple SASL > >> > > > > > >>> > >> > mechanisms simultaneously. In the KIP meeting, Jun > >> > > mentioned > >> > > > > that > >> > > > > > >>> > >> companies > >> > > > > > >>> > >> > sometimes support additional authentication > mechanisms > >> > for > >> > > > > > >>> partners, for > >> > > > > > >>> > >> > example. It does make things more complex, as you > say, > >> > so > >> > > we > >> > > > > need > >> > > > > > >>> to be > >> > > > > > >>> > >> > sure the complexity is worth it. > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > Two more points: > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > 1. It has been suggested that custom security > protocol > >> > > > > support is > >> > > > > > >>> > >> needed by > >> > > > > > >>> > >> > some (KIP-44). Rajini enhanced KIP-43 so that a > SASL > >> > > > mechanism > >> > > > > > >>> with a > >> > > > > > >>> > >> > custom provider can be used for this purpose > instead. > >> > > Given > >> > > > > this, > >> > > > > > >>> it > >> > > > > > >>> > >> seems > >> > > > > > >>> > >> > a bit inconsistent and restrictive not to allow > >> multiple > >> > > > SASL > >> > > > > > >>> mechanisms > >> > > > > > >>> > >> > simultaneously (we do allow SSL and SASL > >> authentication > >> > > > > > >>> simultaneously, > >> > > > > > >>> > >> > after all). > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > 2. The other option would be to support a single > SASL > >> > > > > mechanism > >> > > > > > >>> > >> > simultaneously to start with and then extend this > to > >> > > > multiple > >> > > > > > >>> mechanisms > >> > > > > > >>> > >> > simultaneously later (if and when needed). It seems > >> like > >> > > it > >> > > > > would > >> > > > > > >>> be > >> > > > > > >>> > >> harder > >> > > > > > >>> > >> > to support the latter in the future if we go down > this > >> > > > route, > >> > > > > but > >> > > > > > >>> maybe > >> > > > > > >>> > >> > there are ways around this. > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > Thoughts? > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > Ismael > >> > > > > > >>> > >> > > >> > > > > > >>> > >> > >> > > > > > >>> > > > >> > > > > > >>> > > > >> > > > > > >>> > > > >> > > > > > >>> > > -- > >> > > > > > >>> > > Regards, > >> > > > > > >>> > > > >> > > > > > >>> > > Rajini > >> > > > > > >>> > > > >> > > > > > >>> > > >> > > > > > >>> > > >> > > > > > >>> > > >> > > > > > >>> > -- > >> > > > > > >>> > Regards, > >> > > > > > >>> > > >> > > > > > >>> > Rajini > >> > > > > > >>> > >> > > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > > >> -- > >> > > > > > >> Regards, > >> > > > > > >> > >> > > > > > >> Rajini > >> > > > > > >> > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > -- > >> > > > > > > Regards, > >> > > > > > > > >> > > > > > > Rajini > >> > > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > -- > >> > > > > > Regards, > >> > > > > > > >> > > > > > Rajini > >> > > > > > >> > > > > >> > > > > >> > > > > >> > > > -- > >> > > > Regards, > >> > > > > >> > > > Rajini > >> > > > > >> > > > >> > > >> > > >> > > >> > -- > >> > Regards, > >> > > >> > Rajini > >> > > >> > > > > > > > > -- > > Regards, > > > > Rajini > -- Regards, Rajini
