Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi Rajini & Ron, Thanks, this is an interesting discussion. I think Ron made a good point earlier that if you have ALTER on CLUSTER then you can give yourself whatever other ACLs you want. And this is true even if you are authenticated via a delegation token. So splitting the ability to alter scram users off into a separate ACL doesn't really add any security, compared to just letting users with ALTER on CLUSTER do it. In the past, we've usually started with a simple permissions model, and then made it more complex when we got user feedback. The prefix ACLs we added for creating and deleting topics were a great example of a user-driven improvement. On the other hand, in some cases where we started with a simple model, nobody has proposed making it more complex. For example, nobody has proposed more complex permissions for starting partition reassignments. So I'd really like to defer this discussion about adding more complex ACLs for SCRAM until we get feedback from users. Maybe we could have a follow-on KIP to do things like give users the ability to change their own SCRAM passwords, or add a new entity type, and so on. It feels a bit out of scope for this KIP, since the ZooKeeper-based approach we're replacing didn't support fine-grained permissions. So, can we defer this discussion a bit? best, Colin On Wed, Sep 2, 2020, at 13:08, Rajini Sivaram wrote: > Hi Ron, > > Sounds good to me, thank you. > > Regards, > > Rajini > > > On Wed, Sep 2, 2020 at 8:00 PM Ron Dagostino wrote: > > > Thanks, Rajini. That's a good point about authorizing user creation and > > ACL creation separately to enable that separation as an *option* -- I agree > > with that, which I think argues for a separate ALTER_USER operation (or > > ALTER_USER_CREDENTIAL operation; not sure what the best name is, but > > probably the first one since it is shorter and these names tend to be > > short). If we separate it out like this, then I don't think there is a > > need to restrict delegation tokens from being able to > > ALTER_USER_SCRAM_CREDENTIALS -- since it is possible to simply never > > delegate authority to act as a user with those permissions. Do you agree > > with that, or do you still believe we should explicitly restrict delegation > > tokens from ALTER_USER_SCRAM_CREDENTIALS? I personally still agree with > > Colin's point about avoiding second-class citizenry. > > > > That's also a good point about users perhaps wanting to change their own > > password. I don't think that has come up. If we were to add this, then it > > would be the case that a user would be authorized to change their own > > password at all times. But in this case I think we would have to restrict > > delegation tokens from changing the password of the underlying credential > > since the user doesn't know the password to that account -- and due to that > > lack of knowledge I think this is not a case of being a second-class > > citizen and the restriction is justifiable. > > > > So, to summarize, I am tentatively proposing the following: > > > > 1) ALTER_USER_SCRAM_CREDENTIALS is allowed on any credential if a new > > ALTER_USER operation on the CLUSTER resource is authorized -- even if > > authenticated via delegation token > > 2) Assuming (1) does not apply, ALTER_USER_SCRAM_CREDENTIALS is also > > allowed if the only alterations requested are alterations to one or more > > credentials associated with the authenticated user making the request, with > > the added caveat that the authentication must not have been via delegation > > token (i.e. you can't alter credentials, in the absence of (1) above, for > > the user who delegated their authority to you). > > > > Thoughts? > > > > Ron > > > > > > On Wed, Sep 2, 2020 at 2:16 PM Rajini Sivaram > > wrote: > > > > > Hi Ron, > > > > > > Not sure the person who creates/manages users is always the person who > > > controls access to Kafka resources. Separate ACLs gives the flexibility > > to > > > keep them separate while you can still grant both to the user, while a > > > combined ACL means that they can only be granted together. A related > > > question is about password change. In the current model, User:Alice > > > authenticated as Alice cannot change Alice's password without Alter > > > permissions on the Cluster. The delegation token model where a user has > > > more control over their own credential seems more appropriate in this > > case. > > > Not sure if we considered and rejected that approach. > > > > > > > > > On Wed, Sep 2, 2020 at 5:57 PM Ron Dagostino wrote: > > > > > > > Hi Rajini. Thanks for the explanation. > > > > > > > > I think these are the APIs that are authorized via the ALTER CLUSTER > > > > operation, none of which are restricted when authenticating via > > > delegation > > > > token: > > > > > > > > ALTER_PARTITION_REASSIGNMENTS > > > > ALTER_REPLICA_LOG_DIRS > > > > CREATE_ACL > > > > DELETE_ACL > > > > ELECT_LEADERS > > > > > > > > I think if
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi Ron, Sounds good to me, thank you. Regards, Rajini On Wed, Sep 2, 2020 at 8:00 PM Ron Dagostino wrote: > Thanks, Rajini. That's a good point about authorizing user creation and > ACL creation separately to enable that separation as an *option* -- I agree > with that, which I think argues for a separate ALTER_USER operation (or > ALTER_USER_CREDENTIAL operation; not sure what the best name is, but > probably the first one since it is shorter and these names tend to be > short). If we separate it out like this, then I don't think there is a > need to restrict delegation tokens from being able to > ALTER_USER_SCRAM_CREDENTIALS -- since it is possible to simply never > delegate authority to act as a user with those permissions. Do you agree > with that, or do you still believe we should explicitly restrict delegation > tokens from ALTER_USER_SCRAM_CREDENTIALS? I personally still agree with > Colin's point about avoiding second-class citizenry. > > That's also a good point about users perhaps wanting to change their own > password. I don't think that has come up. If we were to add this, then it > would be the case that a user would be authorized to change their own > password at all times. But in this case I think we would have to restrict > delegation tokens from changing the password of the underlying credential > since the user doesn't know the password to that account -- and due to that > lack of knowledge I think this is not a case of being a second-class > citizen and the restriction is justifiable. > > So, to summarize, I am tentatively proposing the following: > > 1) ALTER_USER_SCRAM_CREDENTIALS is allowed on any credential if a new > ALTER_USER operation on the CLUSTER resource is authorized -- even if > authenticated via delegation token > 2) Assuming (1) does not apply, ALTER_USER_SCRAM_CREDENTIALS is also > allowed if the only alterations requested are alterations to one or more > credentials associated with the authenticated user making the request, with > the added caveat that the authentication must not have been via delegation > token (i.e. you can't alter credentials, in the absence of (1) above, for > the user who delegated their authority to you). > > Thoughts? > > Ron > > > On Wed, Sep 2, 2020 at 2:16 PM Rajini Sivaram > wrote: > > > Hi Ron, > > > > Not sure the person who creates/manages users is always the person who > > controls access to Kafka resources. Separate ACLs gives the flexibility > to > > keep them separate while you can still grant both to the user, while a > > combined ACL means that they can only be granted together. A related > > question is about password change. In the current model, User:Alice > > authenticated as Alice cannot change Alice's password without Alter > > permissions on the Cluster. The delegation token model where a user has > > more control over their own credential seems more appropriate in this > case. > > Not sure if we considered and rejected that approach. > > > > > > On Wed, Sep 2, 2020 at 5:57 PM Ron Dagostino wrote: > > > > > Hi Rajini. Thanks for the explanation. > > > > > > I think these are the APIs that are authorized via the ALTER CLUSTER > > > operation, none of which are restricted when authenticating via > > delegation > > > token: > > > > > > ALTER_PARTITION_REASSIGNMENTS > > > ALTER_REPLICA_LOG_DIRS > > > CREATE_ACL > > > DELETE_ACL > > > ELECT_LEADERS > > > > > > I think if we are going to ALTER_USER_SCRAM_CREDENTIALS then we are > > likely > > > going to want to CREATE_ACL as well -- it feels like there's no sense > in > > > creating a user but then not being able to authorize the user to do > > > anything. (Unless I am wrong here?). If this is correct, then > > > following that to its logical conclusion, it feels like we should > > authorize > > > ALTER_USER_SCRAM_CREDENTIALS via the same ALTER CLUSTER operation. And > > > then with respect to delegation tokens, I think we would either need to > > > allow delegation tokens to do both or we should prevent delegation > tokens > > > from altering credentials. And then that gets to Colin's point about > > > whether sessions authenticated via delegation token should be > > second-class > > > in some way, which I am inclined to think they should not. > > > > > > Ron > > > > > > > > > On Wed, Sep 2, 2020 at 11:23 AM Rajini Sivaram < > rajinisiva...@gmail.com> > > > wrote: > > > > > > > Hi Ron/Colin, > > > > > > > > Without any restrictions, if delegation tokens can be used to create > > new > > > > users or change the password of the user you are impersonating, you > > also > > > > get the power to create/renew a new token by authenticating as a > SCRAM > > > user > > > > you just created or updated. It seems like a new power that we are > > > granting > > > > in a new API using an existing ACL. User management is a new class of > > > > operations we are adding to the Kafka protocol. An alternative to > > > > restricting delegation tokens would be to add a new ACL opera
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Thanks, Rajini. That's a good point about authorizing user creation and ACL creation separately to enable that separation as an *option* -- I agree with that, which I think argues for a separate ALTER_USER operation (or ALTER_USER_CREDENTIAL operation; not sure what the best name is, but probably the first one since it is shorter and these names tend to be short). If we separate it out like this, then I don't think there is a need to restrict delegation tokens from being able to ALTER_USER_SCRAM_CREDENTIALS -- since it is possible to simply never delegate authority to act as a user with those permissions. Do you agree with that, or do you still believe we should explicitly restrict delegation tokens from ALTER_USER_SCRAM_CREDENTIALS? I personally still agree with Colin's point about avoiding second-class citizenry. That's also a good point about users perhaps wanting to change their own password. I don't think that has come up. If we were to add this, then it would be the case that a user would be authorized to change their own password at all times. But in this case I think we would have to restrict delegation tokens from changing the password of the underlying credential since the user doesn't know the password to that account -- and due to that lack of knowledge I think this is not a case of being a second-class citizen and the restriction is justifiable. So, to summarize, I am tentatively proposing the following: 1) ALTER_USER_SCRAM_CREDENTIALS is allowed on any credential if a new ALTER_USER operation on the CLUSTER resource is authorized -- even if authenticated via delegation token 2) Assuming (1) does not apply, ALTER_USER_SCRAM_CREDENTIALS is also allowed if the only alterations requested are alterations to one or more credentials associated with the authenticated user making the request, with the added caveat that the authentication must not have been via delegation token (i.e. you can't alter credentials, in the absence of (1) above, for the user who delegated their authority to you). Thoughts? Ron On Wed, Sep 2, 2020 at 2:16 PM Rajini Sivaram wrote: > Hi Ron, > > Not sure the person who creates/manages users is always the person who > controls access to Kafka resources. Separate ACLs gives the flexibility to > keep them separate while you can still grant both to the user, while a > combined ACL means that they can only be granted together. A related > question is about password change. In the current model, User:Alice > authenticated as Alice cannot change Alice's password without Alter > permissions on the Cluster. The delegation token model where a user has > more control over their own credential seems more appropriate in this case. > Not sure if we considered and rejected that approach. > > > On Wed, Sep 2, 2020 at 5:57 PM Ron Dagostino wrote: > > > Hi Rajini. Thanks for the explanation. > > > > I think these are the APIs that are authorized via the ALTER CLUSTER > > operation, none of which are restricted when authenticating via > delegation > > token: > > > > ALTER_PARTITION_REASSIGNMENTS > > ALTER_REPLICA_LOG_DIRS > > CREATE_ACL > > DELETE_ACL > > ELECT_LEADERS > > > > I think if we are going to ALTER_USER_SCRAM_CREDENTIALS then we are > likely > > going to want to CREATE_ACL as well -- it feels like there's no sense in > > creating a user but then not being able to authorize the user to do > > anything. (Unless I am wrong here?). If this is correct, then > > following that to its logical conclusion, it feels like we should > authorize > > ALTER_USER_SCRAM_CREDENTIALS via the same ALTER CLUSTER operation. And > > then with respect to delegation tokens, I think we would either need to > > allow delegation tokens to do both or we should prevent delegation tokens > > from altering credentials. And then that gets to Colin's point about > > whether sessions authenticated via delegation token should be > second-class > > in some way, which I am inclined to think they should not. > > > > Ron > > > > > > On Wed, Sep 2, 2020 at 11:23 AM Rajini Sivaram > > wrote: > > > > > Hi Ron/Colin, > > > > > > Without any restrictions, if delegation tokens can be used to create > new > > > users or change the password of the user you are impersonating, you > also > > > get the power to create/renew a new token by authenticating as a SCRAM > > user > > > you just created or updated. It seems like a new power that we are > > granting > > > in a new API using an existing ACL. User management is a new class of > > > operations we are adding to the Kafka protocol. An alternative to > > > restricting delegation tokens would be to add a new ACL operation > instead > > > of reusing `Alter` for user management : `AlterUsers/DescribeUsers` > (like > > > AlterConfigs/DescribeConfigs). > > > > > > Regards, > > > > > > Rajini > > > > > > > > > On Wed, Sep 2, 2020 at 12:24 AM Colin McCabe > wrote: > > > > > > > Hi Ron, > > > > > > > > Thanks. We can wait for Rajini's reply to finalize this, but for
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi Ron, Not sure the person who creates/manages users is always the person who controls access to Kafka resources. Separate ACLs gives the flexibility to keep them separate while you can still grant both to the user, while a combined ACL means that they can only be granted together. A related question is about password change. In the current model, User:Alice authenticated as Alice cannot change Alice's password without Alter permissions on the Cluster. The delegation token model where a user has more control over their own credential seems more appropriate in this case. Not sure if we considered and rejected that approach. On Wed, Sep 2, 2020 at 5:57 PM Ron Dagostino wrote: > Hi Rajini. Thanks for the explanation. > > I think these are the APIs that are authorized via the ALTER CLUSTER > operation, none of which are restricted when authenticating via delegation > token: > > ALTER_PARTITION_REASSIGNMENTS > ALTER_REPLICA_LOG_DIRS > CREATE_ACL > DELETE_ACL > ELECT_LEADERS > > I think if we are going to ALTER_USER_SCRAM_CREDENTIALS then we are likely > going to want to CREATE_ACL as well -- it feels like there's no sense in > creating a user but then not being able to authorize the user to do > anything. (Unless I am wrong here?). If this is correct, then > following that to its logical conclusion, it feels like we should authorize > ALTER_USER_SCRAM_CREDENTIALS via the same ALTER CLUSTER operation. And > then with respect to delegation tokens, I think we would either need to > allow delegation tokens to do both or we should prevent delegation tokens > from altering credentials. And then that gets to Colin's point about > whether sessions authenticated via delegation token should be second-class > in some way, which I am inclined to think they should not. > > Ron > > > On Wed, Sep 2, 2020 at 11:23 AM Rajini Sivaram > wrote: > > > Hi Ron/Colin, > > > > Without any restrictions, if delegation tokens can be used to create new > > users or change the password of the user you are impersonating, you also > > get the power to create/renew a new token by authenticating as a SCRAM > user > > you just created or updated. It seems like a new power that we are > granting > > in a new API using an existing ACL. User management is a new class of > > operations we are adding to the Kafka protocol. An alternative to > > restricting delegation tokens would be to add a new ACL operation instead > > of reusing `Alter` for user management : `AlterUsers/DescribeUsers` (like > > AlterConfigs/DescribeConfigs). > > > > Regards, > > > > Rajini > > > > > > On Wed, Sep 2, 2020 at 12:24 AM Colin McCabe wrote: > > > > > Hi Ron, > > > > > > Thanks. We can wait for Rajini's reply to finalize this, but for now I > > > guess that will unblock the PR at least. If we do decide we want the > > > restriction we can do a follow-on PR. > > > > > > It's good to see this API moving forward! > > > > > > best, > > > Colin > > > > > > > > > On Tue, Sep 1, 2020, at 12:55, Ron Dagostino wrote: > > > > Hi Colin. I've removed that requirement from the KIP and updated the > > PR > > > > accordingly. > > > > > > > > Ron > > > > > > > > On Fri, Aug 28, 2020 at 2:27 PM Colin McCabe > > wrote: > > > > > > > > > Hi Ron, > > > > > > > > > > Thanks for the update. I agree with all of these changes, except I > > > think > > > > > we should discuss this one further: > > > > > > > > > > On Wed, Aug 26, 2020, at 14:59, Ron Dagostino wrote: > > > > > > > > > > > > 2. We added a restriction to not allow users who authenticated > > using > > > > > > delegation tokens to create or update user SCRAM credentials. We > > > don't > > > > > > allow such authenticated users to create new tokens, and it would > > be > > > odd > > > > > if > > > > > > they could create a new user or change the password of the user > for > > > the > > > > > > token. > > > > > > > > > > > > > > > > I don't think these two restrictions are comparable. It seems to > me > > > that > > > > > we forbid creating a new token based on an existing token in order > to > > > force > > > > > users of delegation tokens to re-authenticate periodically through > > the > > > > > regular auth system. If they could just create a new token based > on > > > their > > > > > old token, there would be an obvious "wishing for more wishes" > > problem > > > and > > > > > they could just sidestep the regular authentication system entirely > > > once > > > > > they had a token. > > > > > > > > > > This issue doesn't exist here, since creating a new SCRAM user > > doesn't > > > do > > > > > anything to extend the life of the existing delegation token. If > the > > > user > > > > > has the permission to change SCRAM users, I don't see any reason > why > > we > > > > > should forbid them from doing just that. Users of delegation > tokens > > > > > shouldn't be second-class citizens. A user with ALTER on CLUSTER > > should > > > > > have all the permissions associated with ALTER on CLUSTER, > regardless > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi Rajini. Thanks for the explanation. I think these are the APIs that are authorized via the ALTER CLUSTER operation, none of which are restricted when authenticating via delegation token: ALTER_PARTITION_REASSIGNMENTS ALTER_REPLICA_LOG_DIRS CREATE_ACL DELETE_ACL ELECT_LEADERS I think if we are going to ALTER_USER_SCRAM_CREDENTIALS then we are likely going to want to CREATE_ACL as well -- it feels like there's no sense in creating a user but then not being able to authorize the user to do anything. (Unless I am wrong here?). If this is correct, then following that to its logical conclusion, it feels like we should authorize ALTER_USER_SCRAM_CREDENTIALS via the same ALTER CLUSTER operation. And then with respect to delegation tokens, I think we would either need to allow delegation tokens to do both or we should prevent delegation tokens from altering credentials. And then that gets to Colin's point about whether sessions authenticated via delegation token should be second-class in some way, which I am inclined to think they should not. Ron On Wed, Sep 2, 2020 at 11:23 AM Rajini Sivaram wrote: > Hi Ron/Colin, > > Without any restrictions, if delegation tokens can be used to create new > users or change the password of the user you are impersonating, you also > get the power to create/renew a new token by authenticating as a SCRAM user > you just created or updated. It seems like a new power that we are granting > in a new API using an existing ACL. User management is a new class of > operations we are adding to the Kafka protocol. An alternative to > restricting delegation tokens would be to add a new ACL operation instead > of reusing `Alter` for user management : `AlterUsers/DescribeUsers` (like > AlterConfigs/DescribeConfigs). > > Regards, > > Rajini > > > On Wed, Sep 2, 2020 at 12:24 AM Colin McCabe wrote: > > > Hi Ron, > > > > Thanks. We can wait for Rajini's reply to finalize this, but for now I > > guess that will unblock the PR at least. If we do decide we want the > > restriction we can do a follow-on PR. > > > > It's good to see this API moving forward! > > > > best, > > Colin > > > > > > On Tue, Sep 1, 2020, at 12:55, Ron Dagostino wrote: > > > Hi Colin. I've removed that requirement from the KIP and updated the > PR > > > accordingly. > > > > > > Ron > > > > > > On Fri, Aug 28, 2020 at 2:27 PM Colin McCabe > wrote: > > > > > > > Hi Ron, > > > > > > > > Thanks for the update. I agree with all of these changes, except I > > think > > > > we should discuss this one further: > > > > > > > > On Wed, Aug 26, 2020, at 14:59, Ron Dagostino wrote: > > > > > > > > > > 2. We added a restriction to not allow users who authenticated > using > > > > > delegation tokens to create or update user SCRAM credentials. We > > don't > > > > > allow such authenticated users to create new tokens, and it would > be > > odd > > > > if > > > > > they could create a new user or change the password of the user for > > the > > > > > token. > > > > > > > > > > > > > I don't think these two restrictions are comparable. It seems to me > > that > > > > we forbid creating a new token based on an existing token in order to > > force > > > > users of delegation tokens to re-authenticate periodically through > the > > > > regular auth system. If they could just create a new token based on > > their > > > > old token, there would be an obvious "wishing for more wishes" > problem > > and > > > > they could just sidestep the regular authentication system entirely > > once > > > > they had a token. > > > > > > > > This issue doesn't exist here, since creating a new SCRAM user > doesn't > > do > > > > anything to extend the life of the existing delegation token. If the > > user > > > > has the permission to change SCRAM users, I don't see any reason why > we > > > > should forbid them from doing just that. Users of delegation tokens > > > > shouldn't be second-class citizens. A user with ALTER on CLUSTER > should > > > > have all the permissions associated with ALTER on CLUSTER, regardless > > of if > > > > they logged in with Kerberos, delegation tokens, SCRAM, etc. etc. I > > don't > > > > think the proposed restriction you mention here is consistent with > > that. > > > > > > > > best, > > > > Colin > > > > > > > > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi Ron/Colin, Without any restrictions, if delegation tokens can be used to create new users or change the password of the user you are impersonating, you also get the power to create/renew a new token by authenticating as a SCRAM user you just created or updated. It seems like a new power that we are granting in a new API using an existing ACL. User management is a new class of operations we are adding to the Kafka protocol. An alternative to restricting delegation tokens would be to add a new ACL operation instead of reusing `Alter` for user management : `AlterUsers/DescribeUsers` (like AlterConfigs/DescribeConfigs). Regards, Rajini On Wed, Sep 2, 2020 at 12:24 AM Colin McCabe wrote: > Hi Ron, > > Thanks. We can wait for Rajini's reply to finalize this, but for now I > guess that will unblock the PR at least. If we do decide we want the > restriction we can do a follow-on PR. > > It's good to see this API moving forward! > > best, > Colin > > > On Tue, Sep 1, 2020, at 12:55, Ron Dagostino wrote: > > Hi Colin. I've removed that requirement from the KIP and updated the PR > > accordingly. > > > > Ron > > > > On Fri, Aug 28, 2020 at 2:27 PM Colin McCabe wrote: > > > > > Hi Ron, > > > > > > Thanks for the update. I agree with all of these changes, except I > think > > > we should discuss this one further: > > > > > > On Wed, Aug 26, 2020, at 14:59, Ron Dagostino wrote: > > > > > > > > 2. We added a restriction to not allow users who authenticated using > > > > delegation tokens to create or update user SCRAM credentials. We > don't > > > > allow such authenticated users to create new tokens, and it would be > odd > > > if > > > > they could create a new user or change the password of the user for > the > > > > token. > > > > > > > > > > I don't think these two restrictions are comparable. It seems to me > that > > > we forbid creating a new token based on an existing token in order to > force > > > users of delegation tokens to re-authenticate periodically through the > > > regular auth system. If they could just create a new token based on > their > > > old token, there would be an obvious "wishing for more wishes" problem > and > > > they could just sidestep the regular authentication system entirely > once > > > they had a token. > > > > > > This issue doesn't exist here, since creating a new SCRAM user doesn't > do > > > anything to extend the life of the existing delegation token. If the > user > > > has the permission to change SCRAM users, I don't see any reason why we > > > should forbid them from doing just that. Users of delegation tokens > > > shouldn't be second-class citizens. A user with ALTER on CLUSTER should > > > have all the permissions associated with ALTER on CLUSTER, regardless > of if > > > they logged in with Kerberos, delegation tokens, SCRAM, etc. etc. I > don't > > > think the proposed restriction you mention here is consistent with > that. > > > > > > best, > > > Colin > > > > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi Ron, Thanks. We can wait for Rajini's reply to finalize this, but for now I guess that will unblock the PR at least. If we do decide we want the restriction we can do a follow-on PR. It's good to see this API moving forward! best, Colin On Tue, Sep 1, 2020, at 12:55, Ron Dagostino wrote: > Hi Colin. I've removed that requirement from the KIP and updated the PR > accordingly. > > Ron > > On Fri, Aug 28, 2020 at 2:27 PM Colin McCabe wrote: > > > Hi Ron, > > > > Thanks for the update. I agree with all of these changes, except I think > > we should discuss this one further: > > > > On Wed, Aug 26, 2020, at 14:59, Ron Dagostino wrote: > > > > > > 2. We added a restriction to not allow users who authenticated using > > > delegation tokens to create or update user SCRAM credentials. We don't > > > allow such authenticated users to create new tokens, and it would be odd > > if > > > they could create a new user or change the password of the user for the > > > token. > > > > > > > I don't think these two restrictions are comparable. It seems to me that > > we forbid creating a new token based on an existing token in order to force > > users of delegation tokens to re-authenticate periodically through the > > regular auth system. If they could just create a new token based on their > > old token, there would be an obvious "wishing for more wishes" problem and > > they could just sidestep the regular authentication system entirely once > > they had a token. > > > > This issue doesn't exist here, since creating a new SCRAM user doesn't do > > anything to extend the life of the existing delegation token. If the user > > has the permission to change SCRAM users, I don't see any reason why we > > should forbid them from doing just that. Users of delegation tokens > > shouldn't be second-class citizens. A user with ALTER on CLUSTER should > > have all the permissions associated with ALTER on CLUSTER, regardless of if > > they logged in with Kerberos, delegation tokens, SCRAM, etc. etc. I don't > > think the proposed restriction you mention here is consistent with that. > > > > best, > > Colin > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi Colin. I've removed that requirement from the KIP and updated the PR accordingly. Ron On Fri, Aug 28, 2020 at 2:27 PM Colin McCabe wrote: > Hi Ron, > > Thanks for the update. I agree with all of these changes, except I think > we should discuss this one further: > > On Wed, Aug 26, 2020, at 14:59, Ron Dagostino wrote: > > > > 2. We added a restriction to not allow users who authenticated using > > delegation tokens to create or update user SCRAM credentials. We don't > > allow such authenticated users to create new tokens, and it would be odd > if > > they could create a new user or change the password of the user for the > > token. > > > > I don't think these two restrictions are comparable. It seems to me that > we forbid creating a new token based on an existing token in order to force > users of delegation tokens to re-authenticate periodically through the > regular auth system. If they could just create a new token based on their > old token, there would be an obvious "wishing for more wishes" problem and > they could just sidestep the regular authentication system entirely once > they had a token. > > This issue doesn't exist here, since creating a new SCRAM user doesn't do > anything to extend the life of the existing delegation token. If the user > has the permission to change SCRAM users, I don't see any reason why we > should forbid them from doing just that. Users of delegation tokens > shouldn't be second-class citizens. A user with ALTER on CLUSTER should > have all the permissions associated with ALTER on CLUSTER, regardless of if > they logged in with Kerberos, delegation tokens, SCRAM, etc. etc. I don't > think the proposed restriction you mention here is consistent with that. > > best, > Colin >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi Ron, Thanks for the update. I agree with all of these changes, except I think we should discuss this one further: On Wed, Aug 26, 2020, at 14:59, Ron Dagostino wrote: > > 2. We added a restriction to not allow users who authenticated using > delegation tokens to create or update user SCRAM credentials. We don't > allow such authenticated users to create new tokens, and it would be odd if > they could create a new user or change the password of the user for the > token. > I don't think these two restrictions are comparable. It seems to me that we forbid creating a new token based on an existing token in order to force users of delegation tokens to re-authenticate periodically through the regular auth system. If they could just create a new token based on their old token, there would be an obvious "wishing for more wishes" problem and they could just sidestep the regular authentication system entirely once they had a token. This issue doesn't exist here, since creating a new SCRAM user doesn't do anything to extend the life of the existing delegation token. If the user has the permission to change SCRAM users, I don't see any reason why we should forbid them from doing just that. Users of delegation tokens shouldn't be second-class citizens. A user with ALTER on CLUSTER should have all the permissions associated with ALTER on CLUSTER, regardless of if they logged in with Kerberos, delegation tokens, SCRAM, etc. etc. I don't think the proposed restriction you mention here is consistent with that. best, Colin
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi everyone. Multiple changes came up during the discussion of the PR for KIP-554 (https://github.com/apache/kafka/pull/9032). I will update the KIP soon (likely tomorrow) but wanted to send notification to this email thread in case anyone wishes to discuss the changes. They are as follows: 1. We removed -1 as a valid iterations value in the code. It was described as meaning "use the server-side default" but it turns out that the client side must know the number of iterations to use when salting the password, and there is no way for it to know any such "server-side default" value. It can't be defined in the client code because the server may be running a different version of that code. 2. We added a restriction to not allow users who authenticated using delegation tokens to create or update user SCRAM credentials. We don't allow such authenticated users to create new tokens, and it would be odd if they could create a new user or change the password of the user for the token. 3. We relaxed the requirement that Describe requests be sent to the controller -- this is no longer necessary. Alter stil must go to the controller, though -- but not Describe. 4. We expanded the Describe response to include user-level errorCode/errorMessage fields. The KIP only specified a top-level error corresponding to the entire request, but we need the flexibility to fail a particular user (as is already possible with Alter). For example, there may be corrupted data that prevents a user from being described (and we decided to fail a user if it is specified multiple times as mentioned previously). If a describe-all request is made and there is only a top-level error then nothing would be described because the entire request will fail. There would also be no place to specify the user(s) that caused the failure aside from the free-form error message. 5. We settled upon the following method signatures for DescribeUserScramCredentialsResult: /** * * @return a future for the results of all described users with map keys (one per user) being consistent with the * contents of the list returned by {@link #users()}. The future will complete successfully only if all such user * descriptions complete successfully. */ public KafkaFuture> all() /** * * @return a future indicating the distinct users that meet the request criteria and that have at least one * credential. The future will not complete successfully if the user is not authorized to perform the describe * operation; otherwise, it will complete successfully as long as the list of users with credentials can be * successfully determined within some hard-coded timeout period. Note that the returned list will not include users * that do not exist/have no credentials: a request to describe an explicit list of users, none of which existed/had * a credential, will result in a future that returns an empty list being returned here. A returned list will * include users that have a credential but that could not be described. */ public KafkaFuture> users() { /** * * @param userName the name of the user description being requested * @return a future indicating the description results for the given user. The future will complete exceptionally if * the future returned by {@link #users()} completes exceptionally. Note that if the given user does not exist in * the list of described users then the returned future will complete exceptionally with * {@link org.apache.kafka.common.errors.ResourceNotFoundException}. */ public KafkaFuture description(String userName) 6. The KIP was silent on the issue of what to do if the client asks to Describe a user that has no credentials. Should we ignore the request for that user and not include it in the results or treat it as an error? We decided to treat it as an error in the response so that we have the flexibility for the client to know that this is what happened, but we decided to have the DescribeUserScramCredentialsResult skip over this and act as though the user was not described for the purposes of its users() and al() methods. Invoking description() for a user that either was not originally requested or that had not credentials will result in a future that completes exceptionally. 7. The KIP was silent on what to do if a single request is sent to describe the same user multiple times -- treat it as though the user was specified only once, or treat it as a failure? We decided to treat it as a failure for that user. 8. We added 3 new Errors/Exceptions as follows: a) Errors.RESOURCE_NOT_FOUND / ResourceNotFoundException: The server returns this error code when a user that has no credentials is described. The exception is thrown if the client calls get() on a future returned by a call to describe(user) any user that was not described. b) Errors.DUPLICATE_RESOURCE / DuplicateResourceException: If an attempt is made to alter or describe a user twice in the same request. c) Errors.UNACCEPTABLE_CREDENTIAL / Uncc
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Thanks, Ron. Sounds good. best, Colin On Tue, Jul 28, 2020, at 15:26, Ron Dagostino wrote: > 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 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 > > 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 a
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 > 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
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 User
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 UserScramCredentialDeletio
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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, ScramUserMechanismInf
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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? 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. 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. 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. 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 Ron On Wed, Jul 15, 2020 at 5:53 PM Colin McCabe 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 > 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 chan
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 wrote: > > > > > > > > > Hi Colin
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 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 con
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Hi again, Colin. I believe the below function should refer to List alterations in its signature and should invoke alterScramUsers(alterations, new AlterScramUsersOptions()), correct? default AlterScramUsersResult alterScramUsers(List alterations) { return alterScramUsers(deletions, alterations, new AlterScramUsersOptions()); } Also, should it return AlterScramUsersResult or AlterScramCredentialResult? AlterScramUsersRequest JSON states "type": "response" -- should be "type": "request". In AlterScramUsersResponse JSON, I think AlterScramUsersResult needs a username? I'm curious why "UNKNOWN" is a valid ScramMechanism. I didn't find anything about it elsewhere in the KIP or in the DISCUSS thread via a quick search. Is it a catch-all so we will always be able to deserialize something regardless of what key actually appears (which of course should only happen if someone goes in and changes the values directly)? Ron On Tue, Jul 14, 2020 at 7:23 PM 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? > 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 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
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 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
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 Da
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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. 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. 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 there are some issues with the response JSO as well. Here is what I think the response JSON should be for listing users (assuming ThrottleTimeMs belongs there, which it may not): { "apiKey": 50, "type": "response", "name": "ListScramUsersResponse", "validVersions": "0", "flexibleVersions": "0+", "fields": [ { "name": "ThrottleTimeMs", "type": "int32", "versions": "0+", "about": "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." }, { "name": "Error", "type": "int16", "versions": "0+", "about": "The message-level error code." }, { "name": "ErrorMessage", "type": "string", "versions": "0+", "nullableVersions": "0+", "about": "The message-level error message." }, { "name": "Users", "type": "[]ScramUser", "versions": "0+", "about": "The SCRAM users.", "fields": [ { "name": "Name", "type": "string", "versions": "0+", "about": "The user name." }, { "name": "MechanismInfos", "type": "[]ScramUserMechanismInfo", "versions": "0+", "about": "The mechanism credentials", "fields": [ { "name": "Mechanism", "type": "int8", "versions": "0+", "about": "The SCRAM mechanism." }, { "name": "Iterations", "type": "int32", "versions": "0+", "about": "The number of iterations used in the SCRAM mechanism." }]} ]} ] } Also, on the delete request that you added, assuming we keep it, there is no way to specify which credential to delete -- it currently deletes everything. The kafka-configs tool requires the specification of which credential to delete. For example: $ bin/kafka-configs.sh --zookeeper localhost:2181 --alter --delete-config SCRAM-SHA-512 --entity-type users --entity-name alice $ bin/kafka-configs.sh --zookeeper localhost:2181 --describe --entity-type users --entity-name alice Configs for user-principal 'alice' are SCRAM-SHA-256=salt=N3g1eTB2aWUyeDlkYXZ5NDljM3h2aTd4dA==,stored_key=zzliLFJtNeQGY07lBAzzxM6jz0dEm5OkpaJUTfRrD+Y=,server_key=punFVKLCKcZymuRCqh6f6Gjp+VU8ZE3qd8iTboMqHbA=,iterations=8192 Ron On Mon, Jul 13, 2020 at 4:21 PM Ron Dagostino 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
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 wrote: > Thanks for the clarification, Colin. +1 binding from me > > -David > > On Mon, Jul 13, 2020 at 3:40 PM Colin McCabe 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 > 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 > > > > 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: > > > > > > > > > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
Thanks for the clarification, Colin. +1 binding from me -David On Mon, Jul 13, 2020 at 3:40 PM Colin McCabe 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 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 > > > 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
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 > > 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 > > > > 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 > > > > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 > 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 > > > 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 > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 > > > wrote: > > > > > > > +1 (binding) > > > > > > > > Thanks for the KIP, Colin! > > > > > > > > Regards, > > > > > > > > Rajini > > > > > > > > > > > > On Thu, Jul 9, 2020 at 8:49 PM Colin McCabe > > 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 >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 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 > > wrote: > > > > > +1 (binding) > > > > > > Thanks for the KIP, Colin! > > > > > > Regards, > > > > > > Rajini > > > > > > > > > On Thu, Jul 9, 2020 at 8:49 PM Colin McCabe > 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
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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 > wrote: > > > +1 (binding) > > > > Thanks for the KIP, Colin! > > > > Regards, > > > > Rajini > > > > > > On Thu, Jul 9, 2020 at 8:49 PM Colin McCabe 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 > > > > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
+1 (non-binding), thanks for the KIP. On Fri, Jul 10, 2020 at 7:02 PM 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). > > For my own education, the salt will be automatically generated by the admin > client when we send the SCRAM requests correct? > > Best, > Boyang > > On Fri, Jul 10, 2020 at 8:10 AM Rajini Sivaram > wrote: > > > +1 (binding) > > > > Thanks for the KIP, Colin! > > > > Regards, > > > > Rajini > > > > > > On Thu, Jul 9, 2020 at 8:49 PM Colin McCabe 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 > > > > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
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). For my own education, the salt will be automatically generated by the admin client when we send the SCRAM requests correct? Best, Boyang On Fri, Jul 10, 2020 at 8:10 AM Rajini Sivaram wrote: > +1 (binding) > > Thanks for the KIP, Colin! > > Regards, > > Rajini > > > On Thu, Jul 9, 2020 at 8:49 PM Colin McCabe 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 > > >
Re: [VOTE] KIP-554: Add Broker-side SCRAM Config API
+1 (binding) Thanks for the KIP, Colin! Regards, Rajini On Thu, Jul 9, 2020 at 8:49 PM Colin McCabe 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 >
[VOTE] KIP-554: Add Broker-side SCRAM Config API
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