Hi everyone. I just wanted to notify that while implementing this I discovered that we had declared the ScramMechanism enum to have the values HMAC_SHA_{256,512} instead of SCRAM_SHA_{256,512}. I believe Rajini had indicated that this should be changed back on May 7th, and the change makes sense to me given that these are the formal SASL SCRAM mechanism names (with dash replaced by underscore so they are valid Java identifiers). I have updated the KIP. Let me know if you have any questions/concerns, otherwise we can assume this change is acceptable.
Ron On Tue, Jul 21, 2020 at 1:57 PM Colin McCabe <cmcc...@apache.org> wrote: > Hi all, > > With binding +1s from Rajini Sivaram, David Arthur, and Boyang Chen, and > non-binding +1s from Tom Bentley, the vote passes. > > Thanks to everyone who commented and helped to improve the proposal, > especially Ron Dagostino, David, and Boyang. > > best, > Colin > > > On Thu, Jul 16, 2020, at 16:02, Ron Dagostino wrote: > > Hi Colin. I updated the KIP with various renames. I've also created a > > draft PR at https://github.com/apache/kafka/pull/9032 that still needs a > > bunch of real implementation but nonetheless represents the renames in > code. > > > > The biggest changes are that there are now derived classes public class > > UserScramCredentialAdditionUpsertion and public class > > UserScramCredentialDeletion. I don't know what the reaction to the use > of > > the term "upsertion" will be, but that's the best thing I could come up > > with to reflect that these requests are "upserts" (update if there, > > otherwise insert). It was referred to as an "Addition" before, which I > > felt was not technically correct. If you diff the most recent two > versions > > of the KIP it diffs pretty cleanly and makes the changes pretty apparent. > > > > Ron > > > > > > On Thu, Jul 16, 2020 at 11:38 AM Colin McCabe <cmcc...@apache.org> > wrote: > > > > > On Thu, Jul 16, 2020, at 08:06, Ron Dagostino wrote: > > > > Thanks, Colin. The standard "about" message for ThrottleTimeMs seems > > > > to be "The duration in milliseconds for which the request was > throttled > > > > due to a quota violation, or zero if the request did not violate any > > > quota." > > > > as opposed to "The time spent waiting for quota." Should we adjust to > > > > match the typical definition? > > > > > > > > > > Hi Ron, > > > > > > Good point. Let's keep the "about" text consistent. I changed it. > > > > > > > > > > > I'm wondering if describing Scram credentials should require READ > > > privilege > > > > rather than ALTER on the cluster? Altering SCRAM credentials of > course > > > > requires ALTER privilege, and I can see the argument for requiring > ALTER > > > > privilege to describe them as well, but it did catch my eye as > something > > > > worth questioning/confirming. > > > > > > > > > > Also a good point. I spoke with Rajini about this offline, and she > > > pointed out that we can already see user names in ACLs if we have > DESCRIBE > > > on CLUSTER. So it should be fine to have describeScramUsers require > > > DESCRIBE on CLUSTER as well. > > > > > > > > > > > I'm also now thinking that "UNKNOWN" shouldn't be listed in the > > > > ScramMechanism enum. I thought maybe it was a catch-all so we will > > > always > > > > be able to deserialize something regardless of what key actually > appears, > > > > but I just realized that SCRAM credentials and Client Quotas are > mixed > > > > together in the same JSON, so it will be up to the corresponding API > to > > > > ignore what it doesn't recognize -- i.e. if both client quotas and > SCRAM > > > > credentials are defined for a user, then invoking > DescribeClientQuotas > > > must > > > > only describe the quota configs and invoking DescribeScramUsers must > only > > > > describe the SCRAM configs. > > > > > > > > > > The reason to have the UNKNOWN enum is so that we can add new SCRAM > > > mechanisms in the future. If we don't have it, then we're basically > saying > > > we can never add new mechanisms. > > > > > > I agree that the decision to put SCRAM users under the same ZK path as > > > client quotas makes this more complex than we'd like it to be, but all > is > > > not lost. For one thing, we could always just add a new ZK path for > SCRAM > > > users in the future if we really need to. With a new path we wouldn't > have > > > to worry about namespace collisions. For another thing, in the > > > post-KIP-500 world this won't be an issue. > > > > > > In the short term, a simpler solution might work here. For example, > can > > > we just assume that any key that starts with "SCRAM-" is not a quota, > but a > > > scram user? (Or look at some other aspect of the key). > > > > > > > > > > > Also, note that invoking kafka-configs.sh > > > > --bootstrap-server ... --entity-type user --describe will require the > > > > invocation of two separate APIs -- one to describe quotas and one to > > > > describe SCRAM credentials; I don't think this is a problem, but I > did > > > want > > > > to call it out explicitly. > > > > > > > > > > That should be OK. > > > > > > > > > > > Finally, there is a lack of consistency regarding how we name things. > > > For > > > > example, we are calling these APIs {Describe,Alter}ScramUsers and we > have > > > > declared the classes {Describe,Alter}ScramUserOptions, which matches > up > > > > fine. We also have public class DescribeScramUsersResult, which also > > > > matches. But then we have public class AlterScramCredentialsResult, > > > > interface ScramCredentialAlteration, public class > > > ScramCredentialAddition, > > > > and public class ScramCredentialDeletion, none of which match up in > terms > > > > of consistency of naming. I wonder if we should always use > > > > "UserScramCredential" everywhere since that is technically what these > > > API's > > > > are about: describing/altering Users' SCRAM credentials. So the APis > > > would > > > > be {Describe,Alter}UserScramCredentials, and everything else that is > > > > publiuc that now refers inconsistently to either ScramUsers or > > > > ScramCredential would instead refer to UserScramCredentials > (sometimes > > > > singular rather than plural if warranted). For example: public > class { > > > > Describe,Alter}UserScramCredentialsResult, interface User > > > > ScramCredentialAlteration, public class UserScramCredentialAddition, > and > > > > public class UserScramCredentialDeletion > > > > > > > > > > Yeah, there is a bit of a mismatch between "credentials" and "users." > > > Really, these APIs are about credentials, not users. So I agree -- > let's > > > rename it. > > > > > > best, > > > Colin > > > > > > > > > > > On Wed, Jul 15, 2020 at 5:53 PM Colin McCabe <cmcc...@apache.org> > wrote: > > > > > > > > > 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 > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >