assign user with given password or check existing one

[piif]

Problem

reset-password subcommand generates a password, but in a CI context is should be interesting to check if an existing user matches a given password, to decide if it must be updated, and force it to a given value if needed

Suggestion

• allow an option --password on reset-password subcommand
• add a check-password subcommand returning a status

Alternatives Considered

source to input required password may be stdin or a prompt

[piif]

After I've taken a look at kafkactl code, it appears it needs a ns4kafka evolution before reporting it in kafkactl.
In fact, I suppose assigning a password should be implemented by applying a "KafkaUser" spec, containing a password or not.

• If a password is given, apply command must check if it's valid, and if yes, return a "unchanged" status, else if must apply new password.
• If not, apply method must return an updated spec containing the password, as it's done by reset-password command today. Then, caller must store back this modified file.

[loicgreffier]

Hey @piif,

To sum up, you'd like to:

• Have the possibility to assign a given password to a Kafka principal, instead of generating a random one ?
• Get the Kafka principal matching a given password ? Or at least checking there is a match ?

Point 1 can be interesting. As you mentioned the whole business logic is held by the Ns4Kafka API. But:

• What do you mean by "apply command must check if it's valid". How do you determine a password is valid ?
• The "unchanged" status in case of a given password not respecting some rules is not appropriate. In Ns4Kafka, the status returned after applying a resource is computed against the last state of the resource. An "unchanged" status means the applied resource is in the same state than the previous one. Currently, we do not store any password in Ns4Kafka (and not sure we really want to do that), so compute a status of a given password against a previous one is not possible. A more appropriate response would be throwing a ResourceValidationException in case of invalidity.
• Handling KafkaUser as a resource is a breaking change and is questionable considering the spec payload can be empty for generating a random password. Maybe giving the password as a query param of the /{user}/reset-password endpoint would be sufficient

For point 2, what would be the point of checking a password matches a Kafka user ?

• The check will be limited to your namespace user anyway
• As mentioned in point 1, passwords are not stored in Ns4Kafka, and not sure the admin client is able to deliver passwords after being generated (for security reasons)

[piif]

Hi @loicgreffier ,
what I mean by "valid password" is "does the given password is the current one or a modified one".
Without storing current password, it should be verified by trying to connect with this password and check if it is accepted by kafka server.
In this way, its possible to return an "unchanged" status when applying a KafkaUser resource.

By the way, when you say KafkaUser resource would be a breaking change, I don't fully agree since it does not exists today.
Thus it's a new feature, but it doesn't break existing code

I agree with the idea of a /{user}/reset-password payload to specify required password, and this PI should return a "not modified" status if given password matches current one (again by simply testing to connect with)

[loicgreffier]

@piif

A KafkaUser resource is a breaking change on the reset-password endpoint path:

• The POST /api/namespaces/{namespace}/users/{user}/reset-password would be changed to give way to a POST /api/namespaces/{namespace}/users callable from the Kafkactl apply subcommand. Unless both endpoints live, but I don't think it's possible considering resource endpoints access is granted by role bindings (which is not currently the case for {user}/reset-password)

Could you provide more information regarding your CI use case for password reset to challenge if a User resource would be valuable ?

Anyway, with the current implementation:

• The password given as query parameter on /{user}/reset-password would be a nice improvement. Backward compatible because optional. The status can be computed as you mentioned:
  • Unchanged if authenticated call returned 200 (meaning the given password is the current one). No reset to perform in this case.
  • Changed if authenticated call is 401 (meaning the password is not the current one). Reset to perform with the new password.
• With Kafkactl, password can be given with a --password option.

➡ Agree with that ?

• Does a check-password subcommand still make any sense if the validation is performed by the reset subcommand ?

[piif]

Hello @loicgreffier , sorry for the delay, I had to work on another task.

• About /reset-password : I fully agree with your proposal .

• About "user resource" idea: Now, I understand the breaking change problem.
  Maybe we could workaround it with a "principal" entrypoint & resource instead.
  In this case, check-password maybe achieved by a call to "diff" command.

• About our CI : We currently use a CI to deploy topics, connectors and so on, containing "diff" and "apply" jobs.
  Our inventory contains passwords in vaulted files.
  I currently work on migrating this thru ns4kafka (at least I'm trying to validate it's possible in our context).
  If there's no "user resource" with ability to call "diff" and "apply" on it, we need to check passwords, thus "check-password" is required.

[loicgreffier]

@piif

Right now I'm reconsidering that a KafkaUser resource could be a good option, especially for permanently aligning the cluster password.

POST /{user}/reset-password will be preserved as an alternative for backward compatibility reason. Role bindings will allow you to deny the access to the endpoint, if you do not want users to use it.

Which approach would suit you the best ? Would you want to contribute to the KafkaUser resource approach ?

[piif]

KafkaUser resource is clearly my first choice, but I don't know micronaut and didn't code in java for several years :-)
I will see if some of my colleagues can help you

[piif]

Hi,
I'm back with this issue and began to work on a first implementation.
Before trying to define and implement the "apply" version (which need a lot a changes in code I fear), I implemented set-password and check-password APIs on ns4kafka : see https://github.com/piif/ns4kafka/tree/issue/91 and michelin/ns4kafka#430

Usage :

• PATCH /api/namespaces/[namespace]/users/[user]/set-password with new password as body ; returns same result than reset-password (open to discussion : it may just return a status ?)
• POST /api/namespaces/[namespace]/users/[user]/check-password with new password as body ; return 200 is password matches current one, 401 else

It seems to work in my basic usage, but I don't have environments with several cluster to check if it's all fine, nor I didn't checked what happened in case of connection failure to target cluster

[piif]

I began to look at "apply" solution, but I've several questions :

• reset-password returns a KafkaUserResetPassword object with a newPassword field
• "apply" should accept a KafkaUser object with an optionnal password field
  Thus, should we duplicate code to handle a KafkaUser class or rename KafkaUserResetPassword one (but this implies a breaking change in object returned by reset-password) ?

Do you think we have to store KafkaUser objects in ns4kafka backend, like other objects, or do we consider there's nothing more to store than the related namespace, thus namespace storage is enough to keep memory about users ?
The second solution implies to create each namespace before its related user, but avoids to create a full data stack just to store a key (since password will not be stored on ns4kafka side, there's nothing more to store).
This question implies also to decide if namespace deletion must imply to cascade user deletion.

Last question : do you think "apply" API must require password field ? if this field is omitted, should it mean to keep current password, or to reset and return a new one ?
My opinion is to require password, since it's the only goal to this API, but I'm open to suggestions.

[loicgreffier]

• PATCH /api/namespaces/[namespace]/users/[user]/set-password with new password as body ; returns same result than reset-password (open to discussion : it may just return a status ?)

@piif I would be in favor of having a single endpoint for that. What I suggest is:

• Keeping the reset-password endpoint.
• Adding a @Body for the given password.
• @Body might be empty. In this case, a random password is generated as it is done right now.

This would minimize changes from Kafkactl.

[loicgreffier]

• POST /api/namespaces/[namespace]/users/[user]/check-password with new password as body ; return 200 is password matches current one, 401 else

A HTTP 401 could let think the user is not allowed to call the check-password endpoint. What do you think about returning a status instead? Something like "SUCCESS/FAIL"?

Suggesting this point in the review.

[twobeeb]

Password check has a much cleaner solution. I strongly recommend using this instead.
Works for SCRAM only.

Step 1: Pull out the SCRAM server fields from Kafka using AdminClient

# kafka-configs --describe --entity-type users
# Configs for user-principal 'post_install_test_producer' are producer_byte_rate=102400,SCRAM-SHA-512=salt=MTZlcmNvbmtoZWpxOWR3OGdzNWFqYm81YnI=,stored_key=JXOkH3OQyWB2mmsYTHkCeTPf9ozDslgzLvinG2z9AJEizOpZDgxCdxkOfmtWyfxXe3vouBAcgtXoV8NU4aC/5Q==,server_key=9oAYdMDGr7g1rH5TertR0vFj6lf0O4U68WMoAV1O2uP7e3hLuP1P0vwzS+skCwzc+Q6GCXQQSYNpIojXkXkPJA==,iterations=4096,consumer_byte_rate=102400
# Configs for user-principal 'perf_test' are producer_byte_rate=104857600,SCRAM-SHA-512=salt=cTFlM3AybWlrOGlrcndpeXJwcWcyNHFtNg==,stored_key=kAVpmVasCKPRrnrxvHgqXxHyxFR2kYlRj5lkCNsjU6YTjbKfb6L41PCWNFUSAv1ge7ENu1s1AucyUzUvTo4U3g==,server_key=USLipj1qt9XjdjZl+VQLJwBBEtIQUEHFwSUNK1v3DmTdrDxl9CqxoH5pZCCsABR3o7BYXOnjQUYrSurBKrmk0A==,iterations=4096,consumer_byte_rate=104857600

Step 2: Compare with clear password

from passlib.hash import scram
import hmac
import hashlib
import base64


class Scram_Check:
    @staticmethod
    def SamePassword(clear_password, broker_password_config, algorithm='sha-512'):
        
        if not broker_password_config:
            return False
        
        _algorithms = {"sha-512": hashlib.sha512, "sha-256": hashlib.sha256}
        
        salt = base64.b64decode(broker_password_config["salt"])
        iterations = int(broker_password_config["iterations"])
        expected_server_key = broker_password_config["server_key"]
        
        salted_hash = scram.derive_digest(clear_password, salt, iterations, algorithm)
        calc_server_key = hmac.new(salted_hash, 
                                'Server Key'.encode('utf-8'), 
                                _algorithms[algorithm]
                                ).digest()
        
        result = base64.b64encode(calc_server_key).decode('utf-8')
        return result == expected_server_key