Passing secrets as environment variables

[happysalada]

There is another discussion that I'm not sure where it's the best place to take.
This discussion is around secret environment variables. It seems the consensus is that you cannot have secrets inside environment variables because they can be read by others.
Ideally secrets should be put in files, that will be read by the application.
I'm linking to that discussion in systemd systemd/systemd#19604
I'm not sure how you feel about this. I can open a new issue if you want to discuss more.
Basically the change would look like (multiplied by every secret env var)

secret_key_base =
  cond do
    not is_nil(System.get_env("SECRET_KEY_BASE_FILE) ->
       secret_key_base = File.read(System.get_env("SECRET_KEY_BASE_FILE"))
    not is_nil(System.get_env("SECRET_KEY_BASE)) ->
       secret_key_base = System.get_env("SECRET_KEY_BASE")
   true ->
      raise "SECRET_KEY_BASE or SECRET_KEY_BASE_FILE configuration option is required. See https://plausible.io/docs/self-hosting-configuration#server"
  end

if byte_size(secret_key_base) < 64 do
         raise "SECRET_KEY_BASE must be at least 64 bytes long. See https://plausible.io/docs/self-hosting-configuration#server"
end
...

I'm not sure if you are open to such a change or how you feel about the issue.

Let me know if you need/want more information, and I'll be happy to provide.

[ukutaht]

I don't see this as a big concern since Plausible is designed for Docker. I understand that environment variables get passed along to child processes which makes them risky. However, since we run everything inside Docker, I don't see a way the environment variables leaking outside of the container. The container does not run any child processes on the host machine, it is self-contained.

In our self-hosting setup and also in our cloud version we don't actually pass environment variables. We provide a file like this which is read by docker.

[boucman]

aren't containerized processes visible in the host's /proc ?
other containers wouldn't see your processes, but the host would.

[ukutaht]

Ah yes I was completely wrong. Indeed, the way secrets are managed is currently leaky.

In terms of finding a solution, I think we can come up with something that's also aligned with Docker secrets and Kubernetes secrets. They both seem to have an option to store secrets in files in a certain directory.

Maybe this could work similarly as the STORAGE_DIR - we can provide a SECRETS_DIR configuration from where we look up the files based on filename conventions. It would look a bit weird but the filenames could even be the same as env vars e.g. /run/secret/SECRET_KEY_BASE and /run/secret/DATABASE_URL etc.

[happysalada]

I like the SECRETS_DIR idea. How about doing away with the capitalization since it was coming from ENV_VARS.
perhaps in the runtime.exs module you just have something like

secrets_dir = System.get_env("SECRETS_DIR")
secret_key_base =
   secrets_dir
   |> Path.join("secret_key_base")
   |> File.read()

then the file name could just be the same name as the elixir variable, or option name (that it will passed to).

For docker, it looks like the docker compose file still has to define every single secret.
(it's actually the same for the systemd service, you have to explicitely give permission on each secret)

Just to be sure we would be talking about the following environment variables right?
DATABASE_URL
ADMIN_USER_PWD
GOOGLE_CLIENT_SECRET
TWITTER_CONSUMER_SECRET
TWITTER_ACCESS_TOKEN_SECRET
HCAPTCHA_SECRET

[ukutaht]

Yes, and also CLICKHOUSE_DATABASE_URL should be secret.

What do you think about allowing any variable to be supplied as either a file or env var with one (probably file) having precedence? That would be way more extensible in the future if we decide to add or remove secret variables. I also think it would be easier to document and communicate as opposed to having a list of special variables that can/must be supplied differently from all the rest.

[happysalada]

What about something like the following

defp get_var_from_path_or_env(nil, path, env_var) do
   System.get_env(env_var)
end
defp get_var_from_path_or_env(secrets_dir, path, env_var) do
   var_path = Path.join(secrets_dir, path)
   if File.exists?(var_path) do
       File.read(var_path)
   else
       System.get_env(env_var)
   end
end
secrets_dir = System.get_env("SECRETS_DIR")
secret_key_base = get_var_from_path_or_env(secrets_dir, "secret_key_base", "SECRET_KEY_BASE")
cond do
    is_nil(secret_key_base) ->
        raise "SECRET_KEY_BASE configuration option is required. See https://plausible.io/docs/self-hosting-configuration#server"
    byte_size(secret_key_base) < 64 ->
        raise "SECRET_KEY_BASE must be at least 64 bytes long. See https://plausible.io/docs/self-hosting-configuration#server"
end

You can keep compatibility with existing env var and say that the old format will be deprecated in the release after the next?

[ukutaht]

Yeah I think that would work. SECRETS_DIR should probably default to /run/secret to make it easy for Docker users.

The only thing I don't like about this is having to manually manage the mapping between SECRET_KEY_BASE and secret_key_base. Other than looking weird, is there any reason not to have filenames be uppercase?

Alternatively, we could apply String.downcase in the get_var_from_path_or_env function so the conversion between the two formats happens automatically.

[happysalada]

Oh I see, so that there is only one name. It makes sense. Then we are talking about

defp get_var_from_path_or_env(secrets_dir, var_name) do
   var_path = Path.join(secrets_dir, var_name)
   if File.exists?(var_path) do
       File.read(var_path)
   else
       System.get_env(var_name)
   end
end
secrets_dir = System.get_env("SECRETS_DIR", "/run/secrets')
secret_key_base = get_var_from_path_or_env(secrets_dir, "SECRET_KEY_BASE")
cond do
    is_nil(secret_key_base) ->
        raise "SECRET_KEY_BASE configuration option is required. See https://plausible.io/docs/self-hosting-configuration#server"
    byte_size(secret_key_base) < 64 ->
        raise "SECRET_KEY_BASE must be at least 64 bytes long. See https://plausible.io/docs/self-hosting-configuration#server"
end

[ukutaht]

Right, exactly! Happy to merge a PR for this. Maybe just one final adjustment would be to rename SECRETS_DIR -> CONFIG_DIR since anything can be configured this way (including non-secrets).

[happysalada]

I agree that you could, but do you want to though?
Passing everything as a single file would make things a little cumbersome.
If you name this CONFIG_DIR, there are now 2 different ways to supply config.
This creates confusion over which one should be used in which cases. You need some explanation somewhere.
Making it clear from the start that this is for secrets, alleviate some confusion.
If you still feel that CONFIG_DIR is better, just let me know, I'm flexible on this.

[ukutaht]

Passing everything as a single file would make things a little cumbersome.

Single file? I thought we're talking about a directory where each config option is it's own file

I still prefer CONFIG_DIR because that's a universal solution and we don't need to maintain a list of config options that can be configured differently from all the others. Instead, everything can be configured in one of 2 ways with the file taking precedence.

I also think we can alleviate any potential confusion in our docs. The only officially supported use-case is running Plausible in Docker Compose so people don't even need to know anything about the CONFIG_DIR option. All we need to document is setting secrets via docker secret create for the main container.

[happysalada]

What I meant was that everything in the CONFIG_DIR needs to be in a separate file.
Understood regarding CONFIG_DIR.
So you want to use it for every single environment variable, not just secrets? Correct?

[ukutaht]

Yes

[happysalada]

Opened a PR #1117
I just have erlang 24 on my system so I couldn't compile to test.

Would you be open to adding a shell.nix file in the base of the repo? The idea would be that all the dependencies would be defined in that file so that anybody with nix could just run the application without needing to define anything.