edgedb-cli icon indicating copy to clipboard operation
edgedb-cli copied to clipboard

secret_key should be included in instance credentials output

Open fmoor opened this issue 3 years ago • 4 comments

Currently (EdgeDB CLI 2.3.0-dev.902+e29ccdb) secret_key doesn't appear in the output from edgedb instance credentials but it probably should.

fmoor avatar Jan 04 '23 21:01 fmoor

We have to discuss this a bit:

  1. For table mode we can only mark existence of the secret key, unless we want some flags for revealing password/secret.
  2. For DSN mode we already print password, so maybe it's fine. Although, I'm not sure it is. I can easily see users want simpler DSN and password/secret key provided by the environment variables (using their platform secrets management facility).
  3. For JSON we must either support reading secret key from JSON or don't output it. Where @fantix did something in the middle: outputting non-valid credentials JSON.

What was your reason for this feature in the first place?

My take would probably be:

  1. By default skip both password and secret and print something like:

    > edgedb instance credentials -I inst2
    ┌──────────────┬───────────┐
    │ Host         │ localhost │
    │ Port         │ 5656      │
    │ User         │ edgedb    │
    │ Password     │ <hidden>  │
    │ Database     │ edgedb    │
    │ TLS Security │ Default   │
    └──────────────┴───────────┘
    > edgedb instance credentials -I inst2 --json
    {
      "host": "localhost",
      "port": 5656,
      "user": "edgedb",
      "database": "edgedb"
    }
    stderr: WARN: use EDGEDB_SECRET_KEY=xxx to provide secret key, or add `-s` to output that as part of JSON
    > edgedb instance credentials -I mycloud/inst1 --json
    {
      "host": "aws.edgedb.cloud",
      "port": 5656,
      "user": "edgedb",
      "database": "edgedb",
    }
    stderr: WARN: use EDGEDB_PASSWORD=xxx to provide password, use `-s` to output that as part of JSON
    > edgedb instance credentials -I mycloud/inst1 --dsn
    edgedb://[email protected]/inst1
    stderr: EDGEDB_SECRET_KEY=xxx is used for secret key, use `-s` to output that as part of JSON
    > edgedb instance credentials -I inst2 --dsn
    edgedb://[email protected]/inst1
    stderr: EDGEDB_PASSWORD=xxx is used for password, use `-s` to output that as part of JSON
    
  2. Add -s/--secrets argument to print password/secret-key in all modes:

    > edgedb instance credentials -I inst2 -s
    ┌──────────────┬───────────┐
    │ Host         │ localhost │
    │ Port         │ 5656      │
    │ User         │ edgedb    │
    │ Password     │ a8C9sNoD  │
    │ Database     │ edgedb    │
    │ TLS Security │ Default   │
    └──────────────┴───────────┘
    > edgedb instance credentials -I inst2 --json -s
    {
      "host": "localhost",
      "port": 5656,
      "user": "edgedb",
      "database": "edgedb",
      "password": "a8C9sNoD"
    }
    > edgedb instance credentials -I mycloud/inst1 --json -s
    {
      "host": "aws.edgedb.cloud",
      "port": 5656,
      "user": "edgedb",
      "database": "edgedb",
      "secret_key": "alasdfiusenHYUDHSekkduSHJHJDUWksjsJDHIWejJKdhUsjH",
    }
    > edgedb instance credentials -I mycloud/inst1 --dsn -s
    edgedb://[email protected]/inst1?secret_key=alasdfiusenHYUDHSekkduSHJHJDUWksjsJDHIWejJKdhUsjH
    > edgedb instance credentials -I inst2 --dsn -s
    edgedb://edgedb:[email protected]/inst1
    
  3. And yes supporting secret_key in credentials JSON. Which I don't see a reason of why not.

The downside of it is breaking backwards compatibility, though.

tailhook avatar Feb 28 '23 10:02 tailhook

Ah, well there is the issue with my proposal. --dsn is used for input parameters so we actually use --insecure-dsn for outputting DSN. Which means it's kinda okay to put secret_key there, but also means we don't give user an option to have DSN without secrets.

tailhook avatar Feb 28 '23 11:02 tailhook

And I'm also thinking that we need some real-use mode something like:

> edgedb instance credentials -I mycloud/inst1 --env -s
EDGEDB_INSTANCE=mycloud/inst1
EDGEDB_SECRET_KEY=alasdfiusenHYUDHSekkduSHJHJDUWksjsJDHIWejJKdhUsjH

Because it makes little sense to use DSN or JSON for cloud. Although for other remote instances there is no comfortable mode for using environment variables because of certificate. So the following will not work without extra steps:

> edgedb instance credentials -I inst2 --env -s
EDGEDB_DSN=edgedb://edgedb@localhost/edgedb
EDGEDB_PASSWORD=a8C9sNoD

tailhook avatar Feb 28 '23 12:02 tailhook

Decided on the call:

  1. edgedb instance credentials for cloud will fail in JSON an DSN mode
  2. We shouldn't show any extra info about cloud instance except it's name (no hostname, etc.)
  3. Other command will be created to create a token for cloud instance

tailhook avatar Mar 02 '23 10:03 tailhook