-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configurable default user & password #919
base: main
Are you sure you want to change the base?
Conversation
@@ -118,8 +121,8 @@ module LavinMQ | |||
p.on("-h", "--help", "Show this help") { puts p; exit 0 } | |||
p.on("-v", "--version", "Show version") { puts LavinMQ::VERSION; exit 0 } | |||
p.on("--build-info", "Show build information") { puts LavinMQ::BUILD_INFO; exit 0 } | |||
p.on("--guest-only-loopback=BOOL", "Limit guest user to only connect from loopback address") do |v| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how much this is used but this is considered breaking change.
If we keep the other flag and add this new we get around it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Maybe we should just keep the guest_only_loopback name instead of changing it. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to handle changes is to add the new flag and keep the old, the old still works but logs a deprecation warning and then we remove it next major release
@@ -119,6 +119,13 @@ module LavinMQ | |||
end | |||
context | |||
end | |||
|
|||
put "/api/auth/hash_password/:password" do |context, params| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we use a hashed password I think there should be an easy way to generate that hash. I've taken some inspiration from https://www.rabbitmq.com/docs/passwords#computing-password-hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think it's better to send the password in body, since it's a PUT, URLs tends to show up in logs.
Like so: curl -d my_strong_password -XPUT http://localhost:15672/api/auth/hash_password
yes, it's a different API but maybe better? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah that should be a little safer. Maybe we can support both ways? Password in the URL for compatibility, but we recommend/document sending password as body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Lets add that the endpoint with password in body as well. But I can open another issue for that and we can put that in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to achive compatibility, it should be GET, not PUT?
I think this could be a thing where we intentionally does not aim for compatibility, as @snichme states, web server logs will contain the plain text password.
…lt user and password is set after specs have run, remove spec for env variables since it does not work
…t default user and password is set after specs have run, remove spec for env variables since it does not work
edb8be9
to
e29b981
Compare
WHAT is this pull request doing?
Makes it possible to configure default user and password. Takes a hashed value for password.
Also adds api endpoint
/api/auth/hash_password/:password
and lavinmqctl commandlavinmqctl hash_password password
to hash passwords.Also changes
guest-only-loopback
todefault-user-only-loopback
, since the default user no longer has to beguest
.Fixes #877
Configuration
Values can be configured by environment variables, in the config file or by command line arguments.
Env
Config file
Command line
--default-user=USER --default-password=PASSWORD
HOW can this pull request be tested?
Run specs.