Read config variables from files (e.g. docker secrets) and environment variables.

[RandomExplosion]

Hi All, I've been setting up my own homeserver recently and I've come across an issue that many seem to have raised before me;
For the purpose of storing and backing up configuration it is not ideal to have secrets embedded within the config files themselves e.g. private keys, database passwords etc.

Description:
I would like to propose that support be added for reading particular config variables (and perhaps any config variable) from a file or environment variable by adding a suffix to the config option such as "_file" or "_env". Similar to how authelia handles the same problem (see here)

Suggested Implementation
As for how this would be implemented, the easiest (albeit least efficient) approach I can think of would be to check all the attributes in a given config object (using something like .vars()) for a suffix such as "_file" or "_env" suffix (using something like python's string endswith() method)
Then whenever a match is found it could just attempt to read the file or environment variable into a new attribute with the suffix removed the contents of the file or environment variable.

The recursive nature of that approach is definitely slower but I figure since afaik it will only be happening at server startup it shouldn't be that bad as a first implementation. (The search could also be disabled by default as to not affect those who aren't going to use the feature.)

I'm be more than happy to implement this and make a PR myself. I just want to confirm that such a feature would be accepted.

[RandomExplosion]

An example use case:

• Reading postgres connection password from a docker secret
• Reading connection port from environment variable (e.g set in docker-compose file)

Current:

database:
  name: psycopg2
  txn_limit: 10000
  args:
    user: synapse
    password: "sensitiveSecretInConfigFile"
    database: synapse
    host: infra-synapse_db-1
    port: 5432
    cp_min: 5
    cp_max: 10

Proposed:

Suffixed

database:
  name: psycopg2
  txn_limit: 10000
  args:
    user: synapse
    password_file: "/run/secrets/SYNAPSE_POSTGRES_PASSWORD"
    database: synapse
    host: infra-synapse_db-1
    port_env: "SYNAPSE_POSTGRES_PORT"
    cp_min: 5
    cp_max: 10

Advantages:

• Arguably closer to standard yaml syntax

Disadvantages:

• Potentially undesirable if applicable to all config variables
• pid_file and worker_pid_file would need code patches to not clash with an automatic system to handle this

Bash-Like Interpolation Syntax (Proposed by @richvdh:sw1v.org)

database:
  name: psycopg2
  txn_limit: 10000
  args:
    user: synapse
    password: "${file:/run/secrets/SYNAPSE_POSTGRES_PASSWORD}"
    database: synapse
    host: infra-synapse_db-1
    port: "${SYNAPSE_POSTGRES_PORT}"
    cp_min: 5
    cp_max: 10

Advantages:

• Follows well known standard for string interpolation

Disadvantages:

• Brings an external convention into a yaml config file
• File syntax is a non-standard adaptation

[mazzz1y]

Or like Grafana does: https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#override-configuration-with-environment-variables

[DMRobertson]

There is history here, e.g.

• Deprecate the env-var way of running under docker #5518 (comment)
• Configuration files should be more templatable in the standard images #11489; Configuration files should be more templatable in the standard images #11489 (comment) is an example of a case that env vars don't handle well.

If we did want to do something like this, I have some thoughts:

• I'd want to see a systematic approach, not an ad-hoc approach
• We should clearly document the names of env vars
• We should clearly specify who "wins" if there's a conflict between config and/or env and/or file

Speaking personally, I'd like to see us have systematic validation of config files (#12651). We might use Pydantic for that to kill two birds with one stone: it claims to have something which automatically reads from env vars.

[RandomExplosion]

There is history here, e.g.

* [Deprecate the env-var way of running under docker #5518 (comment)](https://github.com/matrix-org/synapse/issues/5518#issuecomment-586288446)

* [Configuration files should be more templatable in the standard images #11489](https://github.com/matrix-org/synapse/issues/11489); [Configuration files should be more templatable in the standard images #11489 (comment)](https://github.com/matrix-org/synapse/issues/11489#issuecomment-1004865079) is an example of a case that env vars don't handle well.

If we did want to do something like this, I have some thoughts:

* I'd want to see a systematic approach, not an ad-hoc approach

* We should clearly document the names of env vars

* We should clearly specify who "wins" if there's a conflict between config and/or env and/or file

Speaking personally, I'd like to see us have systematic validation of config files (#12651). We might use Pydantic for that to kill two birds with one stone: it claims to have something which automatically reads from env vars.

Afaik the problem with environment variables was that they weren't getting resolved before they were needed in the case of some options. Honestly I don't think environment are necessary.

I do agree that a systematic approach for handling this would be ideal e.g having the suffix system work for all configuration options (e.g. have the files be resolved at the end of the function that initially loads the config). Realistically the files approach isn't going to get used for anything beyond integrating with docker secrets or similar mechanisms so even if there are edge cases where things break (as there were in the previous env-var implementation), those edge cases won't get hit (in theory but we'll find that out through testing pre-merge).

It looks like pydantic does support docker secrets but I don't know enough about how it works to make a call on whether it'd work in this case.
EDIT: Looks like it just matches by the setting name.

[RandomExplosion]

@DMRobertson How much rewriting do you think would be needed to switch to pydantic? The main breaking point I see is whether or the change only needs to be made in the init functions or not? (Whether the resulting data structure will be the same)

I don't have a strong enough knowledge of the synapse codebase to implement pydantic if that is the case. (However I'm still happy to try)

[DMRobertson]

I think it would require some nontrivial surgery on Synapses's config machinery.

The main breaking point I see is whether or the change only needs to be made in the init functions or not? (Whether the resulting data structure will be the same)

A systematic approach would likely require data structure changes and changes to Synapses's general config machinery.

[RandomExplosion]

A systematic approach would likely require data structure changes and changes to Synapses's general config machinery.

Yeah that's what I was afraid of, If it were the same from a post-init standpoint it'd be much easier but since synapse makes heavy use of typed variables that probably won't be the case :/