Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ExtReMLapin
Copy link
Contributor

@ExtReMLapin ExtReMLapin commented Apr 11, 2025

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.

@ExtReMLapin ExtReMLapin marked this pull request as ready for review April 11, 2025 15:28
@@ -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))
Copy link
Contributor Author

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()

@ExtReMLapin
Copy link
Contributor Author

Forgot to run the CI tests

oopsie woopsie

@robfrank
Copy link
Collaborator

hi, so, AFAIU, in this way when I restart arcade passing a new rootPassword, the previous one will be overwritten.
It looks like a possible security hole. An attacker can gai access to the database just restarting it providing a new root password.
That said, I understand your needs. Maybe we can add a flag to enable this feature keeping it disabled by default. WDYT?

@ExtReMLapin
Copy link
Contributor Author

ExtReMLapin commented Apr 12, 2025

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

  • write access to the jsonl file
  • access to env variables or stdin to set the new password

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.

@robfrank
Copy link
Collaborator

Before merging, I need some hints on how to tests this.
AFAIU, to test I should:

  • start with a given password
  • authenticate to verify it works
  • stop
  • ---->>>> do something that I don't know to allow arcadeDb accept a new passwrd
  • start again providing a new root password
  • authenticate to verify it works

Is it right?
What is the "do something that I don't know to allow arcadeDb accept a new passwrd"?
Thanks

@robfrank
Copy link
Collaborator

moreover, how is this related to #2059 ?

@ExtReMLapin
Copy link
Contributor Author

This can be related but more like an alternative proposed here #2058 (comment)

Before merging, I need some hints on how to tests this. AFAIU, to test I should:

* start with a given password

* authenticate to verify it works

* stop

* ---->>>> do something that I don't know to allow arcadeDb accept a new passwrd

* start again providing a new root password

* authenticate to verify it works

Is it right? What is the "do something that I don't know to allow arcadeDb accept a new passwrd"? Thanks

As you said

  1. Start with given password, either stdin or env variable
  2. Auth with given password
  3. Create extra user not called root
  4. kill arcadedb
  5. In the users jsonl file kill the line with root user
  6. Restart arcadedb
  7. Ensure it tries to create root session either using stdin or using env variable (checks if first issue is fixed by this PR)
  8. create a db with random name
  9. list db and ensure it's in there (checks second issue created by this PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants