arcadedb icon indicating copy to clipboard operation
arcadedb copied to clipboard

fix: improve root user password check logic in ServerSecurity

Open ExtReMLapin opened this issue 8 months ago • 7 comments

as proposed there : https://github.com/ArcadeData/arcadedb/issues/2058#issuecomment-2718427974

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 avatar Apr 11 '25 15:04 ExtReMLapin

Forgot to run the CI tests

oopsie woopsie

ExtReMLapin avatar Apr 11 '25 17:04 ExtReMLapin

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?

robfrank avatar Apr 11 '25 21:04 robfrank

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.

ExtReMLapin avatar Apr 12 '25 01:04 ExtReMLapin

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 avatar Apr 14 '25 08:04 robfrank

moreover, how is this related to #2059 ?

robfrank avatar Apr 14 '25 08:04 robfrank

This can be related but more like an alternative proposed here https://github.com/ArcadeData/arcadedb/issues/2058#issuecomment-2718427974

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)

ExtReMLapin avatar Apr 14 '25 09:04 ExtReMLapin

@robfrank What's the status of this issue?

lvca avatar May 30 '25 16:05 lvca

I'm coming back to this. What happens now is that if you remove the "root" user, but there are other users configured, the root user is not created. With this PR, if the root user is removed, it is always recreated.

I don't think it is good.

Suppose to provide users/groups configuration without the root user: with this PR it is not possible anymore. At the first start a new root user if created.

@lvca WDYT?

robfrank avatar Jul 10 '25 12:07 robfrank

I agree, if somebody decides to remove the root user, it shouldn't be created automatically at the next restart, unless root is internally needed for some reason, but I can't think of anything right now.

lvca avatar Sep 10 '25 06:09 lvca

Closing this PR not merging it. The user must be able to remove "root" as user.

lvca avatar Dec 09 '25 19:12 lvca