Hi Ismael, Thank you for the review.
1. I think you had asked earlier for SCRAM-SHA-1 to be removed since it is not secure :-) I am happy to add that back in so that clients which don't have access to a more secure algorithm can use it. But it would be a shame to prevent users who only need Java clients from using more secure mechanisms. Since SHA-1 is not secure, you need a secure Zookeeper installation (or store your credentials in an alternative secure store).. By supporting multiple algorithms, we are giving the choice to users. It doesn't add much additional code, just the additional tests (one integration test per mechanism). As more clients support new mechanisms, users can enable these without any changes to Kafka. 2. OK, since it has come up multiple times, I will update the KIP to use a more verbose format. 3. I am assuming that ZK authentication will be enabled and ZK configuration will be done directly using ZK commands. This is true for ACLs, quotas etc. as well? On Mon, Nov 28, 2016 at 2:04 PM, Ismael Juma <ism...@juma.me.uk> wrote: > Hi Rajini, > > Thanks for the KIP. I am in favour of introducing SCRAM as an additional > SASL mechanism. A few comments: > > 1. Magnus raised the point that cyrus-sasl currently only > implements SCRAM-SHA-1, so having a larger number of variants will involve > more work for non-Java clients. Do we really need all of SCRAM-SHA-224, > SCRAM-SHA-256, SCRAM-SHA-384 and SCRAM-SHA-512? > > 2. Like Radai mentioned, it seems a bit weird to use single letter keys. > The space savings don't seem particularly useful given the size of the > values. You mentioned that it would not be useful for humans to interpret > them, but we do expose them via the configs tool. Since we have to add more > keys, I am also not convinced about the benefit of reusing the same letters > as for SCRAM messages. > > 3. Have we given any thought on whether the znodes storing the > authentication information should have additional access restrictions (even > though they are hashed and salted)? Is the assumption that people would > enable ZK authentication? > > Ismael > > On Tue, Nov 15, 2016 at 7:25 PM, Rajini Sivaram < > rajinisiva...@googlemail.com> wrote: > > > Radai, > > > > I don't have a strong objection to using a more verbose format. But the > > reasons for choosing the cryptic s=<salt>,t=<storedKey>,... format: > > > > 1. Unlike other properties like quotas stored in Zookeeper which need > to > > be human readable in order to query the values, these values only need > > to > > be parsed by code. The base64-encoded array of bytes don't really mean > > anything to a human. You would only ever want to check if the user has > > credentials for a mechanism, you can't really tell what the > credentials > > are > > from the value stored in ZK. > > 2. Single letter keys save space. Agree, it is not much, but since a > > more verbose format doesn't add much value, it feels like wasted space > > in > > ZK to store long key names for each property for each user for each > > mechanism. > > 3. SCRAM authentication messages defined in RFC 5802 > > <https://tools.ietf.org/html/rfc5802> use comma-separated key=value > > pairs with single letter keys. s=<salt> and i=<iterations> appear > > exactly > > like that in SCRAM messages. Server key and stored key are not > > exchanged, > > so I chose two unused letters. The same parser used for SCRAM messages > > is > > used to parse this persisted value as well since the format is the > same. > > > > > > On Tue, Nov 15, 2016 at 5:02 PM, radai <radai.rosenbl...@gmail.com> > wrote: > > > > > small nitpick - given that s,t,k and i are used as part of a rather > large > > > CSV format, what is the gain in having them be single letter aliases? > > > in other words - why not salt=... , serverKey=... , storedKey=... , > > > iterations=... ? > > > > > > On Tue, Nov 15, 2016 at 7:26 AM, Mickael Maison < > > mickael.mai...@gmail.com> > > > wrote: > > > > > > > +1 > > > > > > > > On Tue, Nov 15, 2016 at 10:57 AM, Rajini Sivaram > > > > <rajinisiva...@googlemail.com> wrote: > > > > > Jun, > > > > > > > > > > Thank you, I have made the updates to the KIP. > > > > > > > > > > On Tue, Nov 15, 2016 at 12:34 AM, Jun Rao <j...@confluent.io> > wrote: > > > > > > > > > >> Hi, Rajini, > > > > >> > > > > >> Thanks for the proposal. +1. A few minor comments. > > > > >> > > > > >> 30. Could you add that the broker config sasl.enabled.mechanisms > can > > > now > > > > >> take more values? > > > > >> > > > > >> 31. Could you document the meaning of s,t,k,i used in > > > > /config/users/alice > > > > >> in ZK? > > > > >> > > > > >> 32. In the rejected section, could you document why we decided not > > to > > > > bump > > > > >> up the version of SaslHandshakeRequest? > > > > >> > > > > >> Jun > > > > >> > > > > >> > > > > >> On Mon, Nov 14, 2016 at 5:57 AM, Rajini Sivaram < > > > > >> rajinisiva...@googlemail.com> wrote: > > > > >> > > > > >> > Hi all, > > > > >> > > > > > >> > I would like to initiate the voting process for *KIP-84: Support > > > > >> SASL/SCRAM > > > > >> > mechanisms*: > > > > >> > > > > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > >> > 84%3A+Support+SASL+SCRAM+mechanisms > > > > >> > > > > > >> > This KIP adds support for four SCRAM mechanisms (SHA-224, > SHA-256, > > > > >> SHA-384 > > > > >> > and SHA-512) for SASL authentication, giving more choice for > users > > > to > > > > >> > configure security. When delegation token support is added to > > Kafka, > > > > >> SCRAM > > > > >> > will also support secure authentication using delegation tokens. > > > > >> > > > > > >> > Thank you... > > > > >> > > > > > >> > Regards, > > > > >> > > > > > >> > Rajini > > > > >> > > > > > >> > > > > > > > > > > > > > > > > > > > > -- > > > > > Regards, > > > > > > > > > > Rajini > > > > > > > > > > > > > > > -- > > Regards, > > > > Rajini > > > -- Regards, Rajini
