Read db config from environment

[wiltsecarpenter]

• Read db config from environment variables
• Set expiration time on db tokens

db config in environment is done by variables with names in the form:

OBSERVABLEHQ_DB_SECRET_mydbname=secret

This secret is obtained using the observablehq_database_proxy command using the --standalone option

[mbostock]

Should we remove the existing readDatabaseProxyConfig (~/.observablehq) for now, and only look for the database token in process.env? I am inclined to provide fewer ways of doing something, and process.env is the preferred way of doing it in CI.

[wiltsecarpenter]

Simplified, added test, removed reading of config file

[mbostock]

Thanks for making the changes.

Ideally we would have a few more safeguards here: this checks that the environment variable exists, and that it’s base64-encoded JSON, but it doesn’t check that it’s an object, or that the specified fields are present, etc. In particular, we might especially want to check that the secret isn’t empty.

Maybe:

function parseSecret(value: string): DatabaseConfig {
  const obj = JSON.parse(Buffer.from(value, "base64").toString("utf8"));
  if (!obj || typeof obj !== "object") throw new Error("invalid database config");
  if (typeof obj.host !== "string") throw new Error("missing or invalid host");
  if (typeof obj.port !== "number") throw new Error("missing or invalid port");
 // etc.
  return obj;
}

Also given the new API, it seems like we could now just read the environment variable for the requested database, rather than needing to scan process.env for everything that starts with OBSERVABLEHQ_DB_SECRET_. That seems simpler to me, as we wouldn’t need to console.error and continue, only to throw an Error again later.

[wiltsecarpenter]

Okay, further simplified

[mbostock]

Looks good to me!

I think in the future we might want to split these into separate variables so that the values are more easily inspected by the user. The opaque base-64 string is difficult to verify. The Snowflake SDK, for example, uses a variety of fields that are typically fine being public:

• SNOWFLAKE_ACCOUNT
• SNOWFLAKE_DATABASE
• SNOWFLAKE_ROLE
• SNOWFLAKE_SCHEMA
• SNOWFLAKE_USERNAME
• SNOWFLAKE_WAREHOUSE

The only one that is private is:

• SNOWFLAKE_PASSWORD

Our needs are complicated by wanting to support multiple databases, but that’s still addressable with a shared prefix.

[wiltsecarpenter]

The values that we use here are for the proxy, not for direct access, so all we really need is: - proxy url - dialect - token signing secret

…

On Thu, Nov 9, 2023 at 3:10 PM Mike Bostock ***@***.***> wrote: ***@***.**** approved this pull request. Looks good to me! I think in the future we might want to split these into separate variables so that the values are more easily inspected by the user. The opaque base-64 string is difficult to verify. The Snowflake SDK, for example, uses a variety of fields that are typically fine being public: - SNOWFLAKE_ACCOUNT - SNOWFLAKE_DATABASE - SNOWFLAKE_ROLE - SNOWFLAKE_SCHEMA - SNOWFLAKE_USERNAME - SNOWFLAKE_WAREHOUSE The only one that is private is: - SNOWFLAKE_PASSWORD Our needs are complicated by wanting to support multiple databases, but that’s still addressable with a shared prefix. — Reply to this email directly, view it on GitHub <#125 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOM4YQUIDRH4UO2ZGWKHVQTYDVPGTAVCNFSM6AAAAAA7DQJFJWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRTGY2TSNBVHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>