Skip to content
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

[TBD] Passwords with $ in them break setup #59

Closed
1 task done
surchs opened this issue May 23, 2024 · 6 comments · Fixed by #84
Closed
1 task done

[TBD] Passwords with $ in them break setup #59

surchs opened this issue May 23, 2024 · 6 comments · Fixed by #84
Assignees
Labels
released This issue/pull request has been released.

Comments

@surchs
Copy link
Contributor

surchs commented May 23, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Expected Behavior

Users should be able to define any random string as their passwords

Current Behavior

@barbarastrasser mentioned that when they created a DB_PASSWORD string with a $ in it, bad things happened.

From my memory, the $ got treated like a BASH variable and expanded, presumably into nothing (since it doesn't exist). The graphdb was set up with the `docker compose --full_stack up -d" but when executing ./add_data_to_graph.sh I couldn't add data. However, the db was accessible via the webinterface.

Error message

No response

Environment

  • OS:
  • Python/Node version:

How to reproduce

We have to first reproduce this ourselves, I haven't yet seen the issue pop up. Again, @barbarastrasser if you have an example please share / comment

  • Step 1: Insert $ in password

  • Step 2: docker compose
    Screenshot from 2024-05-24 14-21-33

  • Step 3: run add data script

Screenshot from 2024-05-24 14-20-18

Seems fine at first BUT after some investigation we found out that anything goes

Screenshot from 2024-05-24 14-18-22

and that there is no password set for the graphdb i.e. any password goes!!!

Anything else?

Atm we recommend openssl rand -hex 16 as a way to create a random password - this specific command will create a string with only alphanumeric characters ([a-z][0-9]). But again, we should allow users to put any valid (ascii?) string as a password. So this still needs addressing

@surchs surchs added flag:detail needed Issue requires additional clarification before a decision can be made or it can be implemented. and removed flag:detail needed Issue requires additional clarification before a decision can be made or it can be implemented. labels May 23, 2024
@surchs
Copy link
Contributor Author

surchs commented May 24, 2024

I think what is happening here is that if the password includes $ something tries to expand the part after the $, fails to do so, and then (maybe) crashes out of the complete setup process

e.g. with

  17   │ # Note: Must not include any spaces or uppercase letters.
  18   │ COMPOSE_PROJECT_NAME=neurobagel_node
  19   │ 
  20   │ # ---- CONFIGURATION FOR graph ----
  21   │ # Replace ADMINPASSWORD with the secure password you want to set for the admin user
  22   │ NB_GRAPH_ADMIN_PASSWORD=ADMINPASSWORD
  23   │ # Replace DBUSER with the username you want to set for your graph database user
  24   │ NB_GRAPH_USERNAME=DBUSER
  25   │ # Replace DBPASSWORD with the secure password you want to set for the created database user
  26   │ NB_GRAPH_PASSWORD=DB$PASSWORD
  27   │ # Replace my_db with the name you want to give your graph database
  28   │ NB_GRAPH_DB=repositories/my_db

I get these logs from the graph

First time setup: on
Setting up a Neurobagel graph backend...
(The GraphDB server is being accessed inside the GraphDB container at http://localhost:7200.)

Setting up GraphDB server...
/usr/src/neurobagel/.env: line 26: PASSWORD: unbound variable
Finished server setup.export FIRST_TIME_SETUP=on
Adding datasets to the database...

[...]

ERROR: Failed to clear repositories/my_db:
HTTP/1.1 404 
Vary: Accept-Encoding
Cache-Control: no-store
Content-Type: text/plain;charset=UTF-8
Content-Language: en-US
Content-Length: 25
Date: Fri, 24 May 2024 15:23:31 GMT
Server: GraphDB/10.3.1 RDF4J/4.3.5

Unknown repository: my_db

@surchs surchs added the flag:discuss Flag issue that needs to be discussed before it can be implemented. label May 24, 2024
Copy link

github-actions bot commented Aug 8, 2024

We want to keep our issues up to date and active. This issue hasn't seen any activity in the last 75 days.
We have applied the _flag:stale label to indicate that this issue should be reviewed again.
When you review, please reread the spec and then apply one of these three options:

  • prioritize: apply the flag:schedule label to suggest moving this issue into the backlog now
  • close: if the issue is no longer relevant, explain why (give others a chance to reply) and then close.
  • archive: sometimes an issue has important information or ideas but we won't work on it soon. In this case
    apply the someday label to show that this won't be prioritized. The stalebot will ignore issues with this
    label in the future. Use sparingly!

@github-actions github-actions bot added the _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again label Aug 8, 2024
@surchs surchs added the flag:schedule Flag issue that should go on the roadmap or backlog. label Aug 9, 2024
@surchs
Copy link
Contributor Author

surchs commented Aug 9, 2024

Let's address this. Fix should be reasonably easy with string escape

@github-actions github-actions bot removed the _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again label Aug 10, 2024
@surchs surchs added phase:research involves researching information on a topic rather than implementing specific feature. and removed flag:schedule Flag issue that should go on the roadmap or backlog. labels Aug 13, 2024
@surchs surchs moved this to Backlog in Neurobagel Aug 13, 2024
@alyssadai
Copy link
Contributor

alyssadai commented Sep 19, 2024

I was able to reproduce this on our fabrique machine, by adding a $ into the NB_GRAPH_DB environment variable.

Seems like single quotes is not sufficient as it results in an extra $ being added, e.g.: 'repositories/my_db$test' becomes repositories/my_db$$test.

We should look into this more to see if there's another syntax / way to escape these special chars.

@alyssadai
Copy link
Contributor

alyssadai commented Sep 19, 2024

As a workaround, we should switch to users storing their passwords in a separate text file and then pass the file itself as a secret to docker compose, a la https://docs.docker.com/compose/how-tos/use-secrets/#examples.

This would entail:

  • Removing the admin & user passwords from the environment section of docker-compose.yml & the .env file template
  • Creating a new template neurobagel_secrets.txt file or similar to hold these credentials
  • Updating the docs
  • Update env var table
  • Update compatibility test

@alyssadai alyssadai removed flag:discuss Flag issue that needs to be discussed before it can be implemented. phase:research involves researching information on a topic rather than implementing specific feature. labels Sep 23, 2024
@alyssadai alyssadai self-assigned this Sep 23, 2024
@alyssadai alyssadai moved this from Implement - Active to Implement - Done in Neurobagel Sep 24, 2024
@github-project-automation github-project-automation bot moved this from Review - Active to Review - Done in Neurobagel Sep 25, 2024
Copy link
Contributor

🚀 Issue was released in v0.2.0 🚀

@neurobagel-bot neurobagel-bot bot added the released This issue/pull request has been released. label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants