Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Picking up database config from environment variables #196

Closed
kizzie opened this issue Jan 25, 2023 · 11 comments · Fixed by #197
Closed

Picking up database config from environment variables #196

kizzie opened this issue Jan 25, 2023 · 11 comments · Fixed by #197

Comments

@kizzie
Copy link

kizzie commented Jan 25, 2023

So this is probably just me being too rusty at PHP but I am struggling to get the config.php (well.. really its an ini file...) to pick up environment variables. I can connect to the database when I hardcode the username, password and host, but if I try and use environment variables at all such as

dsn = "pgsql:host=db;dbname=privatebin"
tbl = "privatebin_"     ; table prefix
usr = $_ENV['DB_USER']
pwd = $_ENV['DB_PASSWORD']

They are not picked up at all, either you get the full variable name parsed as if it were a string (and I don't have a user called $_ENV['DB_USER'] oddly), or if I try the version which works with php.ini files and just have ${DB_USER} they are blank. Do you have any guides for this? or is it one where I should update the code to not get this from the config.php file and instead just read in direct?

@kizzie
Copy link
Author

kizzie commented Jan 25, 2023

Oh I think I might have it, I needed to overwrite /etc/php81/php-fpm.d/zz-docker.conf to pass the environment variables from the system environment to the PHP environment adding

env[DB_HOST] = $DB_HOST
env[DB_USER] = $DB_USER
env[DB_NAME] = $DB_NAME
env[DB_PASSWORD] = $DB_PASSWORD

to the end of the list. Now it seems to be working when I do

[model]
class = Database
[model_options]
dsn = "pgsql:host=${DB_HOST};dbname=${DB_NAME}"
tbl = "privatebin_"     ; table prefix
usr = ${DB_USER}
pwd = ${DB_PASSWORD}
opt[12] = true    ; PDO::ATTR_PERSISTENT

I guess my main question is whether this is a terrible plan, or if there is a better way? else you can just close this as I am happy now I can not have a plain text password for my container!

@elrido
Copy link
Contributor

elrido commented Jan 26, 2023

I guess my main question is whether this is a terrible plan, or if there is a better way?

I do think this is a legit use case, especially in the container world. It obviously does allow injecting these specific environment variables to PHP, but we don't use these in our code and therefore it would only work with your specific configuration.

Generally environment variables are considered more risky than secrets in files, as they can be extracted or leak into unintended places (see for example this trendmicro blog post). If you were to use kubernetes, you could consider storing these environment variables or the configuration file itself in a secret that gets injected into the container - other container products may offer similar mechanisms. The main benefit of that is to avoid having either the environment variable or files being part of the configuration of your container.


What I'm not sure about is how this could be introduced as a feature in a backwards compatible way. It would be simple enough to pick some variable names (yours sound sensible) and expose these to php-fpm as you did.

One would then still have to include a custom conf.php file that configures pgsql or mysql as the driver and injects the variables in the appropriate locations. Given that we recommend to mount the root filesystem read-only, we can't really dynamically generate a configuration file, if the environment variables are present. Or we would only do that if the file is not present and can be written to (i.e. /srv/cfg is a tmpfs mount or such).

@smktpd
Copy link

smktpd commented Jun 6, 2024

Installing PrivateBin into Kubernetes from a Helm chart was awful: we store our secrets in Vault and use a kubernetes operator to bring those secrets from vault into k8s Secrets, this is an industrial standard approach nowadays.
In k8s a Secret (or a ConfigMap) can be mounted into pods either as an env (and this is a more common way) or as a file (less common approach).
Unlike secrets, configs do not belong to Vault.

And both: configuring non-secret settings and integrating secrets into privatebin's settings file was a major PITA, especially given that existing helm chart is heavily undercooked and doesn't discern configs from secrets and just assumes that you give them all together as a single file.

IMO the settings file needs to be chopped into 2 parts: ([model] + [model_options]) vs the rest of the config.
The next step should be to introduce envs to configure the most important settings, so [model] + [model_options] parts as a must + maybe something else.

I don't claim to be a security expert but I find the stance that having sensitive stuff in envs being less secure than having it on filesystem laughably wrong.

p.s.: thank you @kizzie for providing a workaround, I've used it to have things way less ugly than they would be without it. (But still ugly).

@elrido
Copy link
Contributor

elrido commented Jun 7, 2024

Thank you for sharing your experience.

I don't claim to be a security expert but I find the stance that having sensitive stuff in envs being less secure than having it on filesystem laughably wrong.

May I suggest you have a look at the /proc/*/environ "files" in one of your deployments? At least these are mode 0600 and owned by the respective users the processes run under, but in Linux, your secrets injected as environment variables will show up in the filesystem API and are readable like a config file.

I do take note of your argument that different configuration options have different sensitivity. Which template is configured is not a secret, while your backend access settings likely are. If we are talking about only making it possible to inject certain sensitive values from environment, that would certainly limit the scope of the necessary changes, than having to support all options.

I'm happy for anyone to take a stab at it.

@smktpd
Copy link

smktpd commented Jun 7, 2024

I know about procfs of course and that was my point exactly: using secrets from FS rather than from ENV is not 'more secure' ~ at all, that was a comment about your earlier

environment variables are considered more risky than secrets in files, as they can be extracted or leak into unintended places

Now, as for

If we are talking about only making it possible to inject certain sensitive values from environment, that would certainly limit the scope of the necessary changes, than having to support all options.

PrivateBin supports multiple possible backends. There's some difference in how they are configured. But rather than introducing unique env for each of backends options - I'd suggest introducing a set of 4 common envs (re-)used by all backends (think login, password, host and db_or_bucket (maybe call it container?)). The rest can (at least for now) be left as is as config options manually set in conf file.
And now I realise that there's even no need in chopping settings file into 2 then, because we could just reference envs from there where needed.

The patch would basically mean changing 2 files:

  1. adding 4 lines to the end of /etc/php81/php-fpm.d/zz-docker.conf:
env[STORAGE_HOST] = $STORAGE_HOST
env[STORAGE_LOGIN] = $STORAGE_LOGIN
env[STORAGE_PASSWORD] = $STORAGE_PASSWORD
env[STORAGE_CONTAINER] = $STORAGE_CONTAINER
  1. editing cfg/conf.sample.php to reference those envs everywhere, adding explanation that "you may use ENV references like this to conceal sensitive data or hardcode those values as plaintext right in this confif file, if you feel like doing so"

What do you think?

@elrido
Copy link
Contributor

elrido commented Jun 7, 2024

The model_options are probably the main area with sensitive information, and possibly some others, like:

  • exempted / created: IP addresses or subnets
  • [yourls]->signature: access key to YOURLS

The model_options have pretty different formats, depending on backend, though:

https:/PrivateBin/PrivateBin/blob/3c6bf26dc817cd582e572e72e4b7f6d65114f485/cfg/conf.sample.php#L183-L260

How would you feel about an env variable PRIVATEBIN_MODEL_OPTIONS (to use a unique prefix) containing a JSON encoded object?

i.e. as an env var, compacted and on one line:

export PRIVATEBIN_MODEL_OPTIONS='{"dsn":"mysql:host=localhost;dbname=privatebin;charset=UTF8","tbl":"privatebin_","usr":"privatebin","pwd": "...", "opt": {"12":true}}'

What I'm not sure is if env vars should take precedence over configured values, if both are set. Probably we should only take env vars into account if configurations are commented / unset, so there are no risks of admins getting odd settings injected on them and getting confused why their configuration files don't work?

@smktpd
Copy link

smktpd commented Jun 11, 2024

That looks weird, to be honest. What is the problem in different models having different formats? You just have 4 envs and reference them in those options like this and that's it:

[model] 
  class = S3Storage 
[model_options] 
  region = "whatever" 
  version = "what"
  endpoint = "${STORAGE_HOST}"
  bucket = "${STORAGE_CONTAINER}"
  accesskey = "${STORAGE_LOGIN}"
  secretkey = "${STORAGE_PASSWORD}"

or

[model] 
  class = Database 
[model_options] 
  dsn = "mysql:host=${STORAGE_HOST};dbname=${STORAGE_CONTAINER};charset=UTF8" 
  tbl = "privatebin_"	; table prefix 
  usr = "${STORAGE_LOGIN}" 
  pwd = "${STORAGE_PASSWORD}"
  opt[12] = true ;PDO::ATTR_PERSISTENT 

My idea was that those 4 envs should be universally applicable to any model and that's the bare minimum which seems to cover the private parts (see how I specify region, version, tbl or opt[12] in plain text).

@elrido
Copy link
Contributor

elrido commented Jun 11, 2024

Oh, hang on, I might have missed a crucial bit here: PHP already supports environment variables in INI files? If yes, we don't even need to change anything in the application, right? It would only require we allow passing these through in the container image into php-fpm and someone would need to extend the helm chart (not maintained by me)... Is this it?

I can take care of the container image configuration, for sure. I mainly pushed back because I feared we'd have to maintain something extra in our PHP code, write unit tests for it, etc.

@rugk
Copy link
Member

rugk commented Jun 12, 2024

Yeah, if somehow possible with low effort, I would also argue if we can easily make it configurable, then let's just try to somehow adjust the settings system to read all settings from an env first, and fallback to the ini file or somehow like that. E.g. Grafana also has a lot of settings and allows such a configuration using a schema, which one needs to understand first, but which is understandable: https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#override-configuration-with-environment-variables

@smktpd
Copy link

smktpd commented Jun 12, 2024

@elrido Well, yes and no: it turns out you can't directly reference envs like so from your conf.php:

[model] 
  class = S3Storage 
[model_options] 
  region = "whatever" 
  version = "what"
  endpoint = $_ENV["STORAGE_HOST"]
  bucket = $_ENV["STORAGE_CONTAINER"]
  accesskey = $_ENV["STORAGE_LOGIN"]
  secretkey = $_ENV["STORAGE_PASSWORD"]

@kizzie in his second message in this thread shows how to make envs work in that file, that's why I said the patch would consist of 2 changes, first of which would be adding 4 lines to zz-docker.conf.
Basically, for all the envs you'd like to make references to from within conf.php, you need to list them first in zz-docker.conf.
And then you'll be able to reference them as ${STORAGE_HOST} right from conf.php.

I suggested adding the bare minimum of 4 envs, but you aren't really limited to adding a whole lot more, if you like to close PrivateBin/PrivateBin#1073, for example.

@smktpd
Copy link

smktpd commented Jun 12, 2024

@rugk if you decide to introduce envs for all settings (which I'm not against at all) just don't make a mistake by introducing different envs for, say, secretkeys for different models, this will make the docs / example configuration way more complex for no good reason.

@rugk rugk transferred this issue from PrivateBin/PrivateBin Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants