-
-
Notifications
You must be signed in to change notification settings - Fork 70
fix: improve root user password check logic in ServerSecurity #2149
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
base: main
Are you sure you want to change the base?
Conversation
@@ -125,7 +125,7 @@ public void loadUsers() { | |||
} | |||
} | |||
|
|||
if (users.isEmpty() || (users.containsKey("root") && users.get("root").getPassword() == null)) | |||
if (users.isEmpty() || !users.containsKey("root") || (users.containsKey("root") && users.get("root").getPassword() == null)) |
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.
I mean we could even remove users.isEmpty()
Forgot to run the CI tests oopsie woopsie |
hi, so, AFAIU, in this way when I restart arcade passing a new rootPassword, the previous one will be overwritten. |
Only if the user root has been removed manually editing the jsonl users file, unlike this PR where just passing as arg on boot changes it If it sounds safer to you feel free to edit the PR to add this check. However I feel like if the attacker already got
I fear the changes from this PR doesn't introduce any security exploit. All it does is allowing to set root password if it's not set. |
Before merging, I need some hints on how to tests this.
Is it right? |
moreover, how is this related to #2059 ? |
This can be related but more like an alternative proposed here #2058 (comment)
As you said
|
as proposed there : #2058 (comment)
Issue is that is we have at least one user registered it will not try to recreate root because the current contition doesn't allow it
this pr fixes it
Thanks to Tom Krawczyk for helping us find this bug.