Hi all, Thanks, everyone, for reviewing.
Since we made a few changes to the RPCs in the last few days, I'm going to extend the vote until Monday and close it out then if it looks good. best, Colin On Wed, Jul 15, 2020, at 14:47, Colin McCabe wrote: > On Tue, Jul 14, 2020, at 16:23, Ron Dagostino wrote: > > Thanks, Colin. > > > > DescribeScramUsersResponse returns a list of ScramUser instances, which > > makes sense, but then each of the ScramUser instances only has a single > > ScramUserMechanismInfo instance. I believe it needs a List of those? > > Hi Ron, > > Sorry, that was a typo in the response JSON. Fixed. > > > > > Also, ScramUserMechanismInfo probably needs a better "about" value (it > > currently says "The user name.") > > > > Also fixed :) > > > > > Should both responses include ThrottleTimeMs fields? > > > > Good call. I added this. > > best, > Colin > > > > > I haven't looked at the AlterScramUsers stuff yet; I'll look at that in > > detail tomorrow. > > > > Ron > > > > > > On Tue, Jul 14, 2020 at 4:11 PM Colin McCabe <cmcc...@apache.org> wrote: > > > > > On Tue, Jul 14, 2020, at 07:57, Ron Dagostino wrote: > > > > Hi again, Colin. I also just realized a couple of other > > > incompatibilities > > > > with the way kafka-configs works today that prevents --bootstrap-server > > > > from being a drop-in replacement. This may or may not be a hard > > > > requirement, but we should explicitly decide on these one way or the > > > other. > > > > > > > > One issue is that it is legal to list the SCRAM credentials for a single > > > > user with kafka-configs (e.g. bin/kafka-configs.sh --zookeeper > > > > localhost:2181 --describe --entity-type users --entity-name alice). The > > > > current ListScramUsersRequest API does not support specifying an > > > (optional) > > > > user name, so it always returns all users' SCRAM credentials. We could > > > > filter the lst on the client side, of course, but that seems > > > > inefficient. > > > > > > > > > > Hi Ron, > > > > > > Yes, I think we should allow listing just a particular scram user or > > > users. I will change this to "describe" and add a list of user names > > > which > > > can be supplied, or null to list all. > > > > > > (more responses below) > > > > > > > > > > > The second issue is that the content of the data returned by the new API > > > > does not match the content that kafka-configs currently returns. Here > > > > is > > > > what the tool currently returns: > > > > > > > > # add a user with a pair of SCRAM credentials > > > > $ bin/kafka-configs.sh --zookeeper localhost:2181 --alter --add-config > > > > 'SCRAM-SHA-256=[iterations=8192,password=alice-secret], > > > > SCRAM-SHA-512=[password=alice-secret]' --entity-type users --entity-name > > > > alice > > > > # list that user's SCRAM credentials > > > > $ bin/kafka-configs.sh --zookeeper localhost:2181 --describe > > > > --entity-type > > > > users --entity-name alice > > > > Configs for user-principal 'alice' are > > > > > > > SCRAM-SHA-512=salt=MXNpcXViZmwxcmN4ZzhjeDkwam51c2I3Yw==,stored_key=V3IYnwwC5qjrzoRMyLrzgBstrJVDimQkFfAnJbVrG5mIEaJ/W6j5iV6xITF6HWvtsYrhGDIxsNSZbvVxAIck1w==,server_key=/BpX9JtxqzqyQS6NQcvyksN8Hs8XV65f2G7jx9PMy8Z9s22qCQrqCAtpvLPReYIMMDYRZ9/5x4aSlU/1rfLvVA==,iterations=4096,SCRAM-SHA-256=salt=N3g1eTB2aWUyeDlkYXZ5NDljM3h2aTd4dA==,stored_key=zzliLFJtNeQGY07lBAzzxM6jz0dEm5OkpaJUTfRrD+Y=,server_key=punFVKLCKcZymuRCqh6f6Gjp+VU8ZE3qd8iTboMqHbA=,iterations=8192 > > > > > > > > The API as currently defined only returns the number of iterations. I > > > > would like to confirm that this particular lack of drop-in compatibility > > > is > > > > acceptable. > > > > > > > > > > Yes, this is expected. The argument was that returning the salted > > > password and hash was not secure, so we elected not to return this > > > information. > > > > > > > > > > > Even if the difference in content is acceptable, I think the inability > > > > to > > > > list a single user is probably something we should fix, and the previous > > > > issue I raised about kafka-configs being able to specify alterations and > > > > deletions simultaneously also still stands as something we need to > > > > decide > > > > about -- perhaps drop-in compatibility is not a requirement given the > > > > content difference, in which case we could make it an error to specify > > > both > > > > alterations and deletions when using --bootstrap-server. > > > > [...] > > > > > > > > > > I think you brought up some very good points. I got rid of the delete > > > operation and replaced it with an Alter that can remove individual > > > credentials as needed. We certainly need this, given what the command > > > line > > > tool needs to be able to do. > > > > > > Thanks for the comments. Take a look and see if the latest changes fix > > > it... > > > > > > best, > > > Colin > > > > > > > > > > > > > > > Ron > > > > > > > > > > > > On Mon, Jul 13, 2020 at 4:21 PM Ron Dagostino <rndg...@gmail.com> wrote: > > > > > > > > > Hi Colin. I wanted to explicitly identify a side-effect that I think > > > > > derives from having deletions separated out from the > > > AlterScramUsersRequest > > > > > and put in their own DeleteScramUsersRequest. The command line > > > invocation > > > > > of kafka-configs can specify alterations and deletions simultaneously: > > > it > > > > > is entirely legal for that tool to accept and process both > > > --add-config and > > > > > --delete-config (the current code removes any keys from the added > > > configs > > > > > that are also supplied in the deleted configs, it grabs the > > > > > currently-defined keys, deletes the keys to be deleted, adds the ones > > > to be > > > > > added, and then sets the JSON for the user accordingly). If we split > > > these > > > > > two operations into separate APIs then an invocation of kafka-configs > > > that > > > > > specifies both operations can't complete atomically and could possibly > > > have > > > > > one of them succeed but the other fail. I am wondering if splitting > > > the > > > > > deletions out into a separate API is acceptable given this > > > possibility, and > > > > > if so, what the behavior should be. Maybe the kafka-configs command > > > would > > > > > have to prevent both from being specified simultaneously when > > > > > --bootstrap-server is used. That would create an inconsistency with > > > how > > > > > the tool works with --zookeeper, meaning it is conceivable that > > > switching > > > > > over to --bootstrap-server would not necessarily be a drop-in > > > replacement. > > > > > Am I missing/misunderstanding something? Thoughts? > > > > > > > > > > Also, separately, should the responses include a ThrottleTimeMs > > > field? I > > > > > believe so but would like to confirm. > > > > > > > > > > Ron > > > > > > > > > > On Mon, Jul 13, 2020 at 3:44 PM David Arthur <mum...@gmail.com> wrote: > > > > > > > > > >> Thanks for the clarification, Colin. +1 binding from me > > > > >> > > > > >> -David > > > > >> > > > > >> On Mon, Jul 13, 2020 at 3:40 PM Colin McCabe <cmcc...@apache.org> > > > wrote: > > > > >> > > > > >> > Thanks, Boyang. Fixed. > > > > >> > > > > > >> > best, > > > > >> > Colin > > > > >> > > > > > >> > On Mon, Jul 13, 2020, at 08:43, Boyang Chen wrote: > > > > >> > > Thanks for the update Colin. One nit comment to fix the RPC type > > > > >> > > for AlterScramUsersRequest as: > > > > >> > > "apiKey": 51, > > > > >> > > "type": "request", > > > > >> > > "name": "AlterScramUsersRequest", > > > > >> > > Other than that, +1 (binding) from me. > > > > >> > > > > > > >> > > > > > > >> > > On Mon, Jul 13, 2020 at 8:38 AM Colin McCabe <cmcc...@apache.org> > > > > >> wrote: > > > > >> > > > > > > >> > > > Hi David, > > > > >> > > > > > > > >> > > > The API is for clients. Brokers will still listen to ZooKeeper > > > to > > > > >> load > > > > >> > > > the SCRAM information. > > > > >> > > > > > > > >> > > > best, > > > > >> > > > Colin > > > > >> > > > > > > > >> > > > > > > > >> > > > On Mon, Jul 13, 2020, at 08:30, David Arthur wrote: > > > > >> > > > > Thanks for the KIP, Colin. The new RPCs look good to me, just > > > one > > > > >> > > > question: > > > > >> > > > > since we don't return the password info through the RPC, how > > > will > > > > >> > brokers > > > > >> > > > > load this info? (I'm presuming that they need it to configure > > > > >> > > > > authentication) > > > > >> > > > > > > > > >> > > > > -David > > > > >> > > > > > > > > >> > > > > On Mon, Jul 13, 2020 at 10:57 AM Colin McCabe < > > > cmcc...@apache.org > > > > >> > > > > > >> > > > wrote: > > > > >> > > > > > > > > >> > > > > > On Fri, Jul 10, 2020, at 10:55, Boyang Chen wrote: > > > > >> > > > > > > Hey Colin, thanks for the KIP. One question I have about > > > > >> > > > AlterScramUsers > > > > >> > > > > > > RPC is whether we could consolidate the deletion list and > > > > >> > alteration > > > > >> > > > > > list, > > > > >> > > > > > > since in response we only have a single list of results. > > > The > > > > >> > further > > > > >> > > > > > > benefit is to reduce unintentional duplicate entries for > > > both > > > > >> > > > deletion > > > > >> > > > > > and > > > > >> > > > > > > alteration, which makes the broker side handling logic > > > easier. > > > > >> > > > Another > > > > >> > > > > > > alternative is to add DeleteScramUsers RPC to align what > > > we > > > > >> > currently > > > > >> > > > > > have > > > > >> > > > > > > with other user provided data such as delegation tokens > > > > >> (create, > > > > >> > > > change, > > > > >> > > > > > > delete). > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > Hi Boyang, > > > > >> > > > > > > > > > >> > > > > > It can't really be consolidated without some awkwardness. > > > It's > > > > >> > > > probably > > > > >> > > > > > better just to create a DeleteScramUsers function and RPC. > > > I've > > > > >> > > > changed > > > > >> > > > > > the KIP. > > > > >> > > > > > > > > > >> > > > > > > > > > > >> > > > > > > For my own education, the salt will be automatically > > > generated > > > > >> > by the > > > > >> > > > > > admin > > > > >> > > > > > > client when we send the SCRAM requests correct? > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > Yes, the client generates the salt before sending the > > > request. > > > > >> > > > > > > > > > >> > > > > > best, > > > > >> > > > > > Colin > > > > >> > > > > > > > > > >> > > > > > > Best, > > > > >> > > > > > > Boyang > > > > >> > > > > > > > > > > >> > > > > > > On Fri, Jul 10, 2020 at 8:10 AM Rajini Sivaram < > > > > >> > > > rajinisiva...@gmail.com> > > > > >> > > > > > > wrote: > > > > >> > > > > > > > > > > >> > > > > > > > +1 (binding) > > > > >> > > > > > > > > > > > >> > > > > > > > Thanks for the KIP, Colin! > > > > >> > > > > > > > > > > > >> > > > > > > > Regards, > > > > >> > > > > > > > > > > > >> > > > > > > > Rajini > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > On Thu, Jul 9, 2020 at 8:49 PM Colin McCabe < > > > > >> > cmcc...@apache.org> > > > > >> > > > > > wrote: > > > > >> > > > > > > > > > > > >> > > > > > > > > Hi all, > > > > >> > > > > > > > > > > > > >> > > > > > > > > I'd like to call a vote for KIP-554: Add a > > > > >> > > > > > > > > broker-side > > > > >> SCRAM > > > > >> > > > > > > > configuration > > > > >> > > > > > > > > API. The KIP is here: > > > > >> > > > https://cwiki.apache.org/confluence/x/ihERCQ > > > > >> > > > > > > > > > > > > >> > > > > > > > > The previous discussion thread is here: > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > >> > > > > > >> > > > https://lists.apache.org/thread.html/r69bdc65bdf58f5576944a551ff249d759073ecbf5daa441cff680ab0%40%3Cdev.kafka.apache.org%3E > > > > >> > > > > > > > > > > > > >> > > > > > > > > best, > > > > >> > > > > > > > > Colin > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > -- > > > > >> > > > > David Arthur > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > >> -- > > > > >> David Arthur > > > > >> > > > > > > > > > > > > > > >
