Open piif opened 9 months ago
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.
Hey @piif,
To sum up, you'd like to:
Point 1 can be interesting. As you mentioned the whole business logic is held by the Ns4Kafka API. But:
ResourceValidationException
in case of invalidity.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 sufficientFor point 2, what would be the point of checking a password matches a Kafka user ?
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)
@piif
A KafkaUser
resource is a breaking change on the reset-password
endpoint path:
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:
/{user}/reset-password
would be a nice improvement. Backward compatible because optional. The status can be computed as you mentioned:
--password
option.➡ Agree with that ?
check-password
subcommand still make any sense if the validation is performed by the reset subcommand ?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.
@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 ?
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
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 https://github.com/michelin/ns4kafka/pull/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 elseIt 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
I began to look at "apply" solution, but I've several questions :
KafkaUserResetPassword
object with a newPassword
fieldKafkaUser
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.
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:
reset-password
endpoint.This would minimize changes from Kafkactl.
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.
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
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 neededSuggestion
--password
onreset-password
subcommandcheck-password
subcommand returning a statusAlternatives Considered
source to input required password may be stdin or a prompt