I think Magnus brought up the point of bumping up the version of SaslHandshakeRequest during the discussion of KIP-35. A broker may be capable of supporting a list of SASL mechanisms X and may be enabling a list of SASL mechanisms Y. Y will be a subset of X. Now, from a client's perspective, it may be useful to differentiate the two cases. For example, the client could at least give different error messages. If a broker is not capable of supporting a SASL mechanism, the error could say "please upgrade the broker to a particular version". If the broker is capable, but not enabled with a particular SASL mechanism, the error could just say "please enable this SASL mechanism in the broker". We will be able to distinguish these two cases if we bump up the version of SaslHandshakeRequest every time we add the support for a new SASL mechanism. Not sure how critical this is, but that's my understanding from the KIP-35 discussion. Perhaps Magnus could chime in.
Ewen, I am not sure that I get your comment in the brackets. ApiVersionRequest happens before SaslHandshakeRequest and doesn't need SASL authentication to be completed. Thanks, Jun On Thu, Nov 3, 2016 at 8:18 AM, Harsha Chintalapani <ka...@harsha.io> wrote: > Agree with Rajini on not incrementing the protocol version. As brokers are > returning the list of supported mechanisms don't think it warrants a > protocol version bump. > > Thanks, > Harsha > > On Thu, Nov 3, 2016 at 7:59 AM Rajini Sivaram < > rajinisiva...@googlemail.com> > wrote: > > > Ismael, > > > > 2) I left out MD5 because it is insecure, but thought of keeping SHA-1 > > since it is supported by most services that support SCRAM today. Since > > there is no actual requirement to support SHA-1 and we don't really want > to > > store credentials in Zookeeper using SHA-1, it makes sense not to support > > SHA-1. Will update the KIP. Thanks. > > > > On Thu, Nov 3, 2016 at 1:25 PM, Ismael Juma <isma...@gmail.com> wrote: > > > > > Hi all, > > > > > > I'm currently with limited internet access so I have not done a proper > > > review of the KIP, my apologies. A couple of thoughts: > > > > > > 1. I agree with Rajini and I don't see the value in bumping the > protocol > > > version in this case. As Rajini said, the enabled mechanisms are > > > independent of the broker version. If and when we have a feature API, > we > > > can consider exposing it that way. > > > > > > 2. Do we really want to support sha1? The security of it has been > > > questioned and the general recommendation is to avoid it these days. > > > > > > Ismael > > > > > > On 3 Nov 2016 6:51 am, "Rajini Sivaram" <rajinisiva...@googlemail.com> > > > wrote: > > > > > > > I don't have a strong objection to bumping up SaslHandshakeRequest > > > version, > > > > though I am still not convinced of the usefulness. I do agree that > > KIP-35 > > > > should be standard way to determine client/broker compatibility. But > I > > am > > > > not sure ApiVersionsRequest is a good fit for clients to choose a > SASL > > > > mechanism. > > > > > > > > 1. SaslHandshakeRequest is the only way a client can figure out > if a > > > > SASL mechanism is actually enabled in the broker. The fact that > > broker > > > > version n supports SCRAM doesn't imply that a broker of version n > > > > actually > > > > has the mechanism enabled. Since enabling a mechanism involves > some > > > > effort > > > > like installing an authentication server and/or configuring > > > credentials, > > > > SASL mechanisms are a feature of a broker installation very unlike > > > > versions. As you said, "features" would have worked better. > > > > 2. New SASL mechanisms can be added to older Kafka broker > versions. > > > With > > > > some tweaking, the SCRAM implementation from KIP-84 can be enabled > > in > > > a > > > > 0.10.0 broker without changing any broker code. KIP-86 would make > > this > > > > even > > > > easier, but it is already possible to add new or custom mechanisms > > to > > > > existing broker versions. A client using ApiVersionsRequest to > > choose > > > a > > > > SASL mechanism is only checking when a mechanism was included in a > > > > broker > > > > release, not the "capability" of a broker to support the > mechanism. > > I > > > am > > > > not sure we should encourage clients to choose mechanisms based on > > > > versions. > > > > 3. Clients need additional configuration based on the chosen > > > mechanism. > > > > One of the reasons I couldn't see any value in using > > > ApiVersionsRequest > > > > in > > > > the Java client was because clients are configured with a single > > SASL > > > > mechanism and a JAAS configuration corresponding to that > mechanism. > > > If a > > > > client wants to choose between Kerberos and SCRAM, the client > would > > > need > > > > keytab/principal for kerberos and username/password for SCRAM. > > Clients > > > > that > > > > possess multiple credentials without knowing what the broker > > supports > > > - > > > > that sounds like a rather unusual scenario. > > > > 4. For some mechanisms, clients may want to choose mechanism at > > > runtime. > > > > For example, a client is configured with username/ password and > > wants > > > to > > > > choose between SCRAM-SHA-1/SCRAM-SHA-256/PLAIN depending on which > > > > mechanisms are enabled in the broker. To choose between > SCRAM-SHA-1 > > > and > > > > SCRAM-SHA-256, clients have to use SaslHandshakeRequest since they > > > were > > > > added in the same version. To choose between PLAIN and > > SCRAM-SHA-256, > > > a > > > > versions based approach would work, but wouldn't it be better for > > > > clients > > > > to rely on just SaslHandshakeRequest rather than > > > > ApiVersionsRequest+SaslHandshakeRequest so that the solution > works > > in > > > > all > > > > scenarios? > > > > > > > > > > > > > > > > > > > > On Thu, Nov 3, 2016 at 4:33 AM, Ewen Cheslack-Postava < > > e...@confluent.io > > > > > > > > wrote: > > > > > > > > > I think the bump isn't strictly required, but if the client is > KIP-35 > > > > > aware, it can proactively choose a compatible SASL mechanism based > on > > > its > > > > > initial ApiVersionRequest and avoid an extra connection round trip > > when > > > > > there are client/broker version differences. Without this, a newer > > > client > > > > > would have to do 2 set of requests since the first SASL mechanism > > might > > > > not > > > > > be compatible. > > > > > > > > > > I don't think this is a deal breaker, but I do think it would be > good > > > to > > > > > just standardize on KIP-35 as the way we figure out client/broker > > > > > compatibility. The SASL stuff happened in parallel (maybe before?) > > > KIP-35 > > > > > and ended up with its own mechanism, but I'm in favor of trying to > > > > simplify > > > > > everything by centralizing those considerations into a single API > > call. > > > > (By > > > > > the way, dredging up now ancient history in the KIP-35 discussion, > > this > > > > is > > > > > also why "features" vs "API version" is relevant. If we wanted to > > > > configure > > > > > a newer broker to disable SASL mechanisms we no longer want to > allow > > > use > > > > > of, this isn't really possible to express via API versions unless > we > > > also > > > > > explicitly add an API version that doesn't support that mechanism > > > whereas > > > > > features would make this easier to toggle on/off. The > > > > SaslHandshakeRequest > > > > > probably makes it easier to keep thing secure compared to the > current > > > > state > > > > > of ApiVersionRequest). > > > > > > > > > > -Ewen > > > > > > > > > > On Tue, Nov 1, 2016 at 2:09 PM, Rajini Sivaram < > > > > > rajinisiva...@googlemail.com > > > > > > wrote: > > > > > > > > > > > Gwen, > > > > > > > > > > > > I had thought the same too and hence I am assuming that Java > > clients > > > > > could > > > > > > simply use SaslHandshakeRequest. SaslHandshakeRequest returns the > > > list > > > > of > > > > > > mechanisms enabled in the broker. I think Jun's point was that by > > > > > > incrementing the version of SaslHandshakeRequest, clients can use > > > > > > ApiVersionsRequest to figure out the mechanisms the broker is > > capable > > > > of > > > > > > supporting and use that information to choose a mechanism to send > > in > > > > > > SaslHandshakeRequest. Not sure how useful this actually is, so > will > > > > wait > > > > > > for Jun's response. > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 1, 2016 at 8:18 PM, Gwen Shapira <g...@confluent.io> > > > > wrote: > > > > > > > > > > > > > Wait, I thought SaslHandshakeResponse includes a list of > > mechanisms > > > > > > > supported, so I'm not sure why we need to bump the version? > > > > > > > > > > > > > > I expect clients will send SaslHandshakeRequest_V0, see which > > > > > mechanisms > > > > > > > are supported and make a call based on that? Which means KIP-35 > > is > > > > not > > > > > > > required in that case? Am I missing something? > > > > > > > > > > > > > > On Tue, Nov 1, 2016 at 1:07 PM, Rajini Sivaram < > > > > > > > rajinisiva...@googlemail.com > > > > > > > > wrote: > > > > > > > > > > > > > > > Jun, > > > > > > > > > > > > > > > > I have added the following text to the KIP. Does this match > > your > > > > > > > > expectation? > > > > > > > > > > > > > > > > *SaslHandshakeRequest version will be increased from 0 to 1 > so > > > that > > > > > > > clients > > > > > > > > can determine if the broker is capable of supporting SCRAM > > > > mechanisms > > > > > > > using > > > > > > > > ApiVersionsRequest. Java clients will not be updated to use > > > > > > > > ApiVersionsRequest to choose SASL mechanism under this KIP. > > Java > > > > > > clients > > > > > > > > will continue to use their configured SASL mechanism and will > > > fail > > > > > > > > connection if the requested mechanism is not enabled in the > > > > broker.* > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > On Tue, Nov 1, 2016 at 4:54 PM, Jun Rao <j...@confluent.io> > > > wrote: > > > > > > > > > > > > > > > > > Hi, Rajini, > > > > > > > > > > > > > > > > > > One more thing. It seems that we should bump up the version > > of > > > > > > > > > SaslHandshakeRequest? This way, the client can figure out > > which > > > > > SASL > > > > > > > > > mechanisms the broker is capable of supporting through > > > > > > > ApiVersionRequest. > > > > > > > > > We discussed this briefly as part of KIP-43. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 1, 2016 at 7:41 AM, Rajini Sivaram < > > > > > > > > > rajinisiva...@googlemail.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > If there are no more comments, I will start vote on this > > KIP > > > > > later > > > > > > > this > > > > > > > > > > week. In the meantime, please feel free to post any > > feedback > > > or > > > > > > > > > > suggestions. Initial implementation is here: > > > > > > > > > > https://github.com/apache/kafka/pull/2086. > > > > > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > On Thu, Oct 27, 2016 at 11:18 AM, Rajini Sivaram < > > > > > > > > > > rajinisiva...@googlemail.com> wrote: > > > > > > > > > > > > > > > > > > > > > Jun, > > > > > > > > > > > > > > > > > > > > > > 4) Agree, it does make the implementation simpler. > > Updated > > > > KIP. > > > > > > > > > > > 5) Thank you, that looks neater. Updated KIP. > > > > > > > > > > > > > > > > > > > > > > On Wed, Oct 26, 2016 at 6:59 PM, Jun Rao < > > j...@confluent.io > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > >> Hi, Rajini, > > > > > > > > > > >> > > > > > > > > > > >> Thanks for the reply. > > > > > > > > > > >> > > > > > > > > > > >> 4. Implementation wise, it seems to me that it's > simpler > > > to > > > > > read > > > > > > > > from > > > > > > > > > > the > > > > > > > > > > >> cache than reading directly from ZK since the config > > > manager > > > > > > > already > > > > > > > > > > >> propagates all config changes through ZK. Also, it's > > > > probably > > > > > a > > > > > > > good > > > > > > > > > > idea > > > > > > > > > > >> to limit the places in the code base that directly > > > accesses > > > > > ZK. > > > > > > > > > > >> > > > > > > > > > > >> 5. Yes, it seems that it makes sense to add the new > > SCRAM > > > > > > > > > configurations > > > > > > > > > > >> to > > > > > > > > > > >> the existing /config/users/<encoded-user>. I am not > sure > > > how > > > > > one > > > > > > > > would > > > > > > > > > > >> remove the SCRAM configurations in the example though > > > since > > > > > the > > > > > > > > > > properties > > > > > > > > > > >> specified in add-config is not the ones actually being > > > > stored. > > > > > > An > > > > > > > > > > >> alternative is to doing sth like the following. It may > > > still > > > > > > feel > > > > > > > a > > > > > > > > > bit > > > > > > > > > > >> weird and I am wondering if there is a clearer > approach. > > > > > > > > > > >> > > > > > > > > > > >> bin/kafka-configs.sh --zookeeper localhost:2181 > --alter > > > > > > > --add-config > > > > > > > > > > >> 'scram_sha-256=[password= > alice-secret,iterations=4096], > > > > > > > scram_sha-1= > > > > > > > > > > >> [password=alice-secret,iterations=4096]' > --entity-type > > > > users > > > > > > > > > > >> --entity-name > > > > > > > > > > >> alice > > > > > > > > > > >> > > > > > > > > > > >> bin/kafka-configs.sh --zookeeper localhost:2181 > --alter > > > > > > > > > --delete-config > > > > > > > > > > >> 'scram_sha-256,scram_sha-1' --entity-type users > > > > --entity-name > > > > > > > alice > > > > > > > > > > >> > > > > > > > > > > >> Thanks, > > > > > > > > > > >> > > > > > > > > > > >> Jun > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> On Wed, Oct 26, 2016 at 4:35 AM, Rajini Sivaram < > > > > > > > > > > >> rajinisiva...@googlemail.com> wrote: > > > > > > > > > > >> > > > > > > > > > > >> > Hi Jun, > > > > > > > > > > >> > > > > > > > > > > > >> > Thank you for reviewing the KIP. Answers below: > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > 1. Yes, agree, Updated KIP. > > > > > > > > > > >> > 2. User specifies a password and iteration count. > > > > > > > > kaka-configs.sh > > > > > > > > > > >> > generates a random salt and then generates > > StoredKey > > > > and > > > > > > > > > ServerKey > > > > > > > > > > >> for > > > > > > > > > > >> > that > > > > > > > > > > >> > password using the same message formatter > > > > implementation > > > > > > used > > > > > > > > for > > > > > > > > > > >> SCRAM > > > > > > > > > > >> > authentication. I have added some more detail to > > the > > > > KIP > > > > > ( > > > > > > > > > > >> > https://cwiki.apache.org/ > > > confluence/display/KAFKA/KIP- > > > > > > > > > > >> > 84%3A+Support+SASL+SCRAM+mechanisms#KIP-84: > > > > > > > > > > SupportSASLSCRAMmechanisms- > > > > > > > > > > >> > Tools). > > > > > > > > > > >> > Does that answer the question? > > > > > > > > > > >> > 3. I started off thinking just one > (SCRAM-SHA-256) > > > and > > > > > then > > > > > > > > > thought > > > > > > > > > > >> > another one is required to make sure that the > > > > > > implementation > > > > > > > > can > > > > > > > > > > cope > > > > > > > > > > >> > with > > > > > > > > > > >> > multiple SCRAM mechanisms. But actually you are > > > right, > > > > we > > > > > > can > > > > > > > > > > support > > > > > > > > > > >> > all. > > > > > > > > > > >> > I haven't added the old md2/md5 mechanisms that > > > aren't > > > > > very > > > > > > > > > secure, > > > > > > > > > > >> but > > > > > > > > > > >> > I > > > > > > > > > > >> > have included all the SHA algorithms. > > > > > > > > > > >> > 4. Since credentials are only required when a > > > > connection > > > > > is > > > > > > > > made, > > > > > > > > > > it > > > > > > > > > > >> > feels like we can just read the latest value from > > ZK > > > > > rather > > > > > > > > than > > > > > > > > > > >> cache > > > > > > > > > > >> > all > > > > > > > > > > >> > users and keep them updated. Having said that, we > > can > > > > > > always > > > > > > > > add > > > > > > > > > > >> caching > > > > > > > > > > >> > later if we find that the overhead of reading > from > > ZK > > > > > every > > > > > > > > time > > > > > > > > > is > > > > > > > > > > >> too > > > > > > > > > > >> > expensive. Since caching doesn't change any > > > externals, > > > > > this > > > > > > > can > > > > > > > > > be > > > > > > > > > > >> done > > > > > > > > > > >> > in > > > > > > > > > > >> > a JIRA later - would that be ok? > > > > > > > > > > >> > 5. Thanks, updated. I have changed the property > > names > > > > to > > > > > > > > include > > > > > > > > > > >> > mechanism. To avoid four separate properties per > > > > > mechanism > > > > > > in > > > > > > > > > ZK, I > > > > > > > > > > >> have > > > > > > > > > > >> > changed the format to use a single property > > > (lower-case > > > > > > > > mechanism > > > > > > > > > > >> name) > > > > > > > > > > >> > with four values concatenated in a format similar > > to > > > > > SCRAM > > > > > > > > > > messages. > > > > > > > > > > >> > > > > > > > > > > > >> > Do you think storing SCRAM credentials in > > > > > > > > > /config/users/<encoded-user> > > > > > > > > > > >> > along with existing quota properties is okay? Or > > should > > > > they > > > > > > be > > > > > > > > > under > > > > > > > > > > a > > > > > > > > > > >> > different path (eg. /config/credentials/<encoded- > > > user>)? > > > > Or > > > > > > > > should > > > > > > > > > > >> they be > > > > > > > > > > >> > on a completely different path like ACLs with a > > separate > > > > > tool > > > > > > > > > instead > > > > > > > > > > of > > > > > > > > > > >> > reusing kaka-configs.sh? > > > > > > > > > > >> > > > > > > > > > > > >> > Thank you, > > > > > > > > > > >> > > > > > > > > > > > >> > Rajini > > > > > > > > > > >> > > > > > > > > > > > >> > On Tue, Oct 25, 2016 at 11:55 PM, Jun Rao < > > > > j...@confluent.io > > > > > > > > > > > > > > wrote: > > > > > > > > > > >> > > > > > > > > > > > >> > > Hi, Rajini, > > > > > > > > > > >> > > > > > > > > > > > > >> > > Thanks for the proposal. Looks good overall and > > seems > > > > > quite > > > > > > > > useful > > > > > > > > > > >> (e.g. > > > > > > > > > > >> > > for supporting delegation tokens). A few > > > > > comments/questions > > > > > > > > below. > > > > > > > > > > >> > > > > > > > > > > > > >> > > 1. For the ZK data format change, should we use > the > > > same > > > > > > > > > convention > > > > > > > > > > >> as in > > > > > > > > > > >> > > KIP-55 to use encoded user name (i.e., > > > > > > > > > > /config/users/<encoded-user1>) > > > > > > > > > > >> ? > > > > > > > > > > >> > > > > > > > > > > > > >> > > 2. For tooling, could you describe how user > > typically > > > > > > > generates > > > > > > > > > > >> > > scam_server_key and scram_stored_key to be used by > > > > > > > > > kafka-config.sh? > > > > > > > > > > >> > > > > > > > > > > > > >> > > 3. Is there a particular reason to only support > sha1 > > > and > > > > > > > sha128? > > > > > > > > > > >> Should > > > > > > > > > > >> > we > > > > > > > > > > >> > > support more hashes listed below in the future? > > > > > > > > > > >> > > http://www.iana.org/assignments/hash-function- > > > > > > > > > > >> > > text-names/hash-function-text-names.xhtml > > > > > > > > > > >> > > > > > > > > > > > > >> > > 4. Is there a reason not to cache user credentials > > in > > > > the > > > > > > > > broker? > > > > > > > > > > The > > > > > > > > > > >> > > dynamic config mechanism already supports loading > > > > configs > > > > > > into > > > > > > > > > > >> broker's > > > > > > > > > > >> > > cache. Checking credentials from broker's cache is > > > more > > > > > > > > efficient > > > > > > > > > > than > > > > > > > > > > >> > > reading from ZK each time. > > > > > > > > > > >> > > > > > > > > > > > > >> > > 5. Typo "scram_iteration-4096" (= instead of -). > > > > > > > > > > >> > > > > > > > > > > > > >> > > Thanks, > > > > > > > > > > >> > > > > > > > > > > > > >> > > Jun > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > On Tue, Oct 4, 2016 at 6:43 AM, Rajini Sivaram < > > > > > > > > > > >> > > rajinisiva...@googlemail.com > > > > > > > > > > >> > > > wrote: > > > > > > > > > > >> > > > > > > > > > > > > >> > > > Hi all, > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > I have just created KIP-84 to add SCRAM-SHA-1 > and > > > > > > > > SCRAM-SHA-256 > > > > > > > > > > SASL > > > > > > > > > > >> > > > mechanisms to Kafka: > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > https://cwiki.apache.org/ > > > > confluence/display/KAFKA/KIP- > > > > > > > > > > >> > > > 84%3A+Support+SASL+SCRAM+mechanisms > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > Comments and suggestions are welcome. > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > Thank you... > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > Regards, > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > Rajini > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > -- > > > > > > > > > > >> > Regards, > > > > > > > > > > >> > > > > > > > > > > > >> > Rajini > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Regards, > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > *Gwen Shapira* > > > > > > > Product Manager | Confluent > > > > > > > 650.450.2760 <(650)%20450-2760> | @gwenshap > > > > > > > Follow us: Twitter <https://twitter.com/ConfluentInc> | blog > > > > > > > <http://www.confluent.io/blog> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Regards, > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Thanks, > > > > > Ewen > > > > > > > > > > > > > > > > > > > > > -- > > > > Regards, > > > > > > > > Rajini > > > > > > > > > > > > > > > -- > > Regards, > > > > Rajini > > >
