edgedb / edgedb-cli

The EdgeDB CLI
https://www.edgedb.com/docs/cli/index
Apache License 2.0
166 stars 23 forks source link

Consider not to store password in plain text #1108

Open MrFoxPro opened 1 year ago

MrFoxPro commented 1 year ago
foxpro:~$ edgedb instance credentials -I dev
┌──────────────┬───────────┐
│ Host         │ localhost │
│ Port         │ 10703     │
│ User         │ edgedb    │
│ Password     │ <hidden>  │
│ Database     │ edgedb    │
│ TLS Security │ Default   │
└──────────────┴───────────┘

it's hidden, but actually stored in plain text. foxpro:~$ cat /home/foxpro/.config/edgedb/credentials/dev.json

{
  "port": 10703,
  "user": "edgedb",
  "password": "Iszh3PvXuq5h8S3eyBgaqoDS", <- wtf?
  "database": "edgedb",
  "tls_cert_data": "...",
  "tls_ca": "...",
  "tls_security": "default"
}

I'm not security expert, but seems like it's bad idea. I can suggest to show prompt for password and let user store it as they wants. For example, it can be done with PGP or age encryption.

elprans commented 1 year ago

I don't think we're very special here, as storing credentials in plain text by default is something that devtools do regularly, take any CLI from awscli to docker to gh.

That said, I agree that we should implement support for credential helpers a-la git (and maybe have an opt-in helper to store secrets in system keychain).

NobbZ commented 1 year ago

I do not know how the current implementation is, though perhaps make sure to create the file with a 600 or 640 mode at most, to avoid other users reading the file after guessing the path (it is a well known location at the end).

Take for example SSH, which requires the individual keys to be 600, for exactly this reason.