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? Also, ScramUserMechanismInfo probably needs a better "about" value (it currently says "The user name.")
Should both responses include ThrottleTimeMs fields? 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 > > >> > > > > > >
