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 > > > >> > > > > > > > > > >
