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

Support for secrets set in ENV variables #504

Closed
kaihendry opened this Issue Sep 10, 2016 · 41 comments

Comments

Projects
None yet
@kaihendry

kaihendry commented Sep 10, 2016

Currently I don't see any support for SMTP_AUTH_PASSWORD to be set via typically Docker env variables. Am I mistaken?

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Sep 10, 2016

The chosen approach is to put them in the config file.

There's many many possible ways to provide configuration, for sanity we have to choose just one of them.

@kaihendry

This comment has been minimized.

kaihendry commented Sep 10, 2016

It's fairly common practice now to provide secrets via ENV variables & it would be good if you could support this approach together with your configuration. Ideally your configuration can be set by an ENV variable like shell.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Sep 10, 2016

You're free to put scripts around our configs to provide this for yourself.

@kaihendry

This comment has been minimized.

kaihendry commented Sep 10, 2016

But that would mean forking the Docker image, right? So I can add a little m4 substitution or something.

@Andor

This comment has been minimized.

Andor commented Sep 11, 2016

@kaihendry you can mount directory with your config into container with alertmanager.

@pydolan

This comment has been minimized.

pydolan commented Sep 13, 2016

@brian-brazil -- Is there a thread somewhere that has the discussion on ending support for environment variables? I'm trying to understand why this design decision was made.

From a programming standpoint, it would be fairly trivial to modify config/config.go to loop over all configuration attributes, looking for a matching environment variable, and overwriting the stored value if one is found. (FWIW, Docker's source appears to have Go code for doing this.)

Given that many organizations store configuration files in source repositories (e.g., git) and that is a highly-recommended practice to not store secrets in source repositories, allowing prometheus/alertmanager users to store secrets in environment variables (without having to use awk hacks) seems like a great idea.

Thank you in advance for any clarification you can provide.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Sep 13, 2016

It was a matter of consistency and allowing for many many secrets to be provided.

We want to have just one way to configure a given setting, and users can then build on that.

@fvigotti

This comment has been minimized.

fvigotti commented Nov 11, 2016

If possible I would like to vote for some kind of env interpolation for config files, this is a common best practice and would be a great feature in prometheus alertmanager..

@seanhoughton

This comment has been minimized.

Contributor

seanhoughton commented Dec 8, 2016

If environment variables aren't going to be supported what about encrypted secrets in the config? The eyaml (https://github.com/TomPoulton/hiera-eyaml) project works well for Puppet.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Dec 8, 2016

That is not something we plan on supporting. Once again, too many ways to do it. You're free to use your own tooling around this.

@groob

This comment has been minimized.

groob commented Dec 10, 2016

I'd like to chime in and say that ENV variables should be supported as well. given that so many of us are using prometheus with docker/k8s deployment, it's very odd to have to deal with the config file and not have builtin support for interpolating secrets/env variables.

@shaneramey

This comment has been minimized.

shaneramey commented Dec 10, 2016

One option is to use a k8s init-container with confd to write your environment-variable-passed secrets to a config file (on a shared emptyDir: {} volumeMount) before the Alertmanager container starts. The Alertmanager container would then mount that same Volume to consume the populated template as its config file.

Supporting environment variables (such as SMTP_AUTH_PASSWORD) natively in the Alertmanager container would be difficult if users wanted multiple alert routing destinations of the same type (like 2+ SMTP servers with different credentials).

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Dec 10, 2016

One option is to use a k8s init-container with confd to write your environment-variable-passed secrets to a config file

That's one option.

@brust

This comment has been minimized.

brust commented Mar 9, 2017

@brian-brazil Sorry, I didn't understand.

How can I get a env-var to use it on my config.yml? What's the options?

My config.yml is versioned and I don't want to put sensitive data there.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Mar 9, 2017

Usage questions are best asked on the mailing list: https://groups.google.com/forum/#!aboutgroup/prometheus-users

@pracucci

This comment has been minimized.

pracucci commented Apr 30, 2017

Thanks for the whole discussion. I'm just not sure about not supporting env variables to keep consistency (that's usually a thing a do appreciate a lot), considering that Prometheus config file supports secrets via environment variables too. I understand it wouldn't work in the same exact way, but it looks some secrets are already passed to Prometheus via env variables. Am I'm wrong?

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Apr 30, 2017

Prometheus config file supports secrets via environment variables too

No it doesn't. Some libraries we happen to use support them.

@pracucci

This comment has been minimized.

pracucci commented Apr 30, 2017

Brian, I do understand it's not Prometheus directly but the libraries Prometheus relies on, but from the user point-of-view, isn't the same thing? As an user, on Prometheus secrets can be passed as env variables, on the Alertmanager not.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Apr 30, 2017

One type of secret for one SD mechanism happens to work via mechanisms other than being in the config file. I wouldn't read much into that.

@TalosThoren

This comment has been minimized.

TalosThoren commented Oct 17, 2017

If you version control your config, placing secrets directly in them is not an option. Not being able to use the standard method of supplying secrets via environment variables is frustrating. It is a standard practice for secrets delivery; and it's actually the preferred method of the Twelve Factor App: 12factor.net

I honestly feel like this level of resistance to supporting such a prevalent industry standard is unprofessional. We need to be able to version control our configs without manually inserting secrets upon deployment; and the answer "we don't support it, write a script"; while doable, is absolutely frustrating to come across.

If you need to consume a library to support this, please do so. This is something your team should absolutely be receptive to.

@balboah

This comment has been minimized.

balboah commented Nov 2, 2017

How about allowing tmpl_string in these places as well? Then you could have a template function env where the configuration file would look like my_secret: '{{ env "MY_SECRET"}}'. I have yet to learn this code base but I see notifications are doing this over secrets like for PagerDuty.ServiceKey

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Nov 2, 2017

If you have somewhere in the notifier config that you believe should be templatable, I'd suggest filing a separate issue for that. Where secrets are templatable is where they are also routing information for the service in question, with PagerDuty keys being the prime example of that.

There are no plans to allow the environment to be accessible in configuration files in Prometheus or the Alertmanager. This is a configuration management issue, and should be resolved within your configuration management system.

@balboah

This comment has been minimized.

balboah commented Nov 3, 2017

It's a configuration management issue for sure, it's just that it's tricky to get right for alertmanager. In my case I use the prometheus operator from coreos running on multiple kubernetes clusters, where I want to configure the alert notifications to contain certain cluster specific variables. There is no obvious way to hack a dynamic setting in there between syncing the config as a secret and alertmanager reading it without forking your own version.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Nov 3, 2017

In the simplest cases, bash variable substitution with a heredoc should be sufficient (or sed). In more sophisticated setups you would be substituting in per-cluster stuff before the config ever hits k8, and environment variables would not help you with these.

@balboah

This comment has been minimized.

balboah commented Nov 3, 2017

The bash or sed substitutions requires a customized secret config file sync & configmap reloader (which coreos operator uses to ping the alertmanager when file is changed). The substituting before hitting k8s could be viable but requires either multiple copies of the same config except the dynamic variable or decode && encode base64 after changing values through helm or other helper script.

If there was support for environment variables, there is a built in way to set them from a secret or config which would be simpler from the operations perspective. This is how the other services are configured and afaik a best practice approach.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Nov 3, 2017

Keep in mind that k8 is only one of many many systems that are used with Prometheus, and the best way to support them is to keep to only one way of doing things and let users build on top of that. If you have a configuration management system, none of this is a problem as this is a normal thing to deal with.

Prometheus presumes that you have a configuration management system, and we do not add features merely because users lack one.

@olvesh

This comment has been minimized.

olvesh commented Nov 6, 2017

The main problem in your reasoning @brian-brazil is that by making the Alertmanager config "simpler than possible", integrations must be more complicated than needed.

Kubernetes has overall good abstractions, and splitting secrets into env-vars is one of them. 12-factor app is another example that favors this way.

In many ways being able to store key variables that often change across environments in environment variables is very much the industry standard.

IMO Prometheus being part of the Cloud Native Computing Foundation has an obligation to follow this industry standard or a similar way of achieving the same end.

I kindly ask if you could consider revising this decision!

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Nov 6, 2017

What you are requesting is that Prometheus become a configuration management system, and implicitly also a secret management system.

Neither of those things are considered in scope, rather that's something for your existing configuration management to tackle. We lack as a team the resources to take on the gargantuan task of developing such a system. Layering configuration management systems also tends to not produce a particularly maintainable result.

This is something we have consensus on as a project team, it is very unlikely that this is going to change. I don't believe we've had any new arguments proposed on this matter in at least a year, maybe two.

This is something to resolve by building on top of the alertmanager, not by changing the alertmanager. Look at the CoreOS Prometheus Operator for example which builds on top of Prometheus, as some of its aims are a bit out of scope of what makes sense in Prometheus itself.

In many ways being able to store key variables that often change across environments in environment variables is very much the industry standard.

I think that's greatly overstating how standard it is, it's only started to become a thing in the past decade or so in my experience. Environment variables are not a great choice, as they weren't designed with security in mind and there's a non-trivial chance they'll get leaked. Without getting into the more advanced setups, I'd say that files protected with unix permissions would be more standard and more secure.

integrations must be more complicated than needed.

This a standard and simple problem to handle with a configuration management system. Put another way, this is the "normal" level of complication. I've integrated hundreds of different pieces of software into configuration management and the simple&dumb approach Prometheus&friends take is extremely refreshing. It avoids days of hassle fighting with over-complicated and bespoke configuration setups.

What I'd suggest doing is finding a good configuration management system for k8, I've heard good things about ksonnet for example, and with a small bit of bash scripting should do the trick.

@olvesh

This comment has been minimized.

olvesh commented Nov 6, 2017

I understand your sentiments, and would like to clarify a few things.

Instead of digging into technical details an arguing "we need env vars for secret" lets rather discuss this use case:
Say when using PagerDuty integrations we will typically have a quite few service keys lying around. Today these will be mixed into the same file providing route, inhibit and template configurations. In our rather simple config management setup we would have to encrypt the whole Alertmanager configuration to be able to safely handle this file.

One possible solution would be the ability to have several config files specified to Alertmanager so that users could have one part with secrets and one with config and AM merging these together. This would allow a clean separation without you guys needing to add much complexity. I could store receivers and global vars in one encrypted file, and routes, templates and inhibitions in another.

I will also dig into ksonnet - thank you for the pointer. I also plan to dig in to Prometheus Operator, but have not had the chance yet.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Nov 6, 2017

One possible solution would be the ability to have several config files specified to Alertmanager so that users could have one part with secrets and one with config and AM merging these together.

That's heading fairly directly into configuration management, as templating is something a CM system pretty much must provide. That's what we're trying to avoid getting into. Any reasonable CM system will allow templating the files (or even running sed on them) to substitute in the secrets as a file is written out, and most of them will also allow combining files together.

@olvesh

This comment has been minimized.

olvesh commented Nov 6, 2017

That's heading fairly directly into configuration management,

So is having a config file :-)

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Nov 6, 2017

So is having a config file :-)

An arbitrary binary string is not configuration management, in the same way that the Prometheus binary is not the source code. If the alertmanager took in the configuration some other way, you'd still have all the same problems. And likely wish we'd had gone with a simple config file :)

@TalosThoren

This comment has been minimized.

TalosThoren commented Dec 5, 2017

A config file that can pull secrets from environment variables is a pretty standard feature. It's a long-standing industry standard that many of us have a real need for.

Why is there so much resistance to this?

We should not have to generate our config file from a template of some kind just to deliver a single secret (or multiple secrets) to alertmanager.

I'm going to end up having to write a janky sed script to place tokens into the config file after deployment, which works, sure, but come on man, this is baby-town-frolics over here. This is basics. This is easy-peasy, everyone expects it, type of functionality. It's beyond silly that your team is so against this common industry standard that so many tools and users expect to see available. It was surprising to learn it's not already available out of box and it's absolutely mind-boggling to watch you argue so strongly against it, @brian-brazil

The notion that providing support for a config file that is capable of pulling a few small pieces of data from environment variables constitutes implementing a full fledged config management tool is also a bit preposterous; recall that many of us are actually experts at infrastructure deployment with a great deal of hands on experience with config management tooling (just like you, presumably) and that we're able to notice exactly how silly that claim is. A great deal of us deliver secrets as environment variables using config management tooling; it's super standard to do.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Dec 5, 2017

I'm going to end up having to write a janky sed script to place tokens into the config file after deployment

I recently became aware of envsubst which is part of gettext and may be of interest.

@jtomell

This comment has been minimized.

jtomell commented Dec 8, 2017

Every other CNCF project supports loading from environment variables. It is one hundred percent part of this industries standard options to configure a program. Ten years is a long time. The go language is only 8 years old. Nobody would be confused about what ${ALERTMANAGER_SLACK_API_URL} would be coming from. No CNCF project except Kubernetes is a CM system. I agree, CM can do everything. That doesn't mean it should.

I took the liberty of compiling examples from every other CNCF project. I left out OpenTracing since it isn't really applicable. There is probably one being used somewhere.

What makes the Prometheus projects different?

notary:     https://github.com/theupdateframework/notary/blob/4995826393ef9fe0416ef9da469f4f359c883364/fips.go
jaeger:     https://github.com/jaegertracing/jaeger/blob/a2ed9b854fa9ef2ce4eb97290d083b73dfbc9bcc/pkg/config/config.go#L46
cni:        https://github.com/containernetworking/cni/blob/e663e98444227cae1adf1214e733f0bbabec037f/pkg/invoke/delegate.go#L26
rkt:        https://github.com/rkt/rkt/blob/d59fc6da36d6a630b2d062fb8e6d70843f4a8a45/stage1_fly/enter/main.go#L117
containerd: https://github.com/containerd/containerd/blob/8ded4fe3a725b74372973b6732feb255f7a4174f/namespaces/context.go#L32
coredns:    https://github.com/coredns/coredns/blob/73d702c05201c44b6c8a9367efd1c12caa896bcf/plugin/metrics/README.md#examples
grpc:       https://github.com/grpc/grpc/blob/master/doc/environment_variables.md
istio :     https://github.com/istio/istio/blob/a1fcda22776603cec99e52da4cc4cf4a2e6e1e76/tools/githubContrib/githubContrib.go#L46
fluentd:    https://github.com/fluent/fluentd/blob/065a0633963fda85a94b70d478e1b0a2f615730d/lib/fluent/env.rb#L20
kubernetes: https://github.com/kubernetes/kubernetes/blob/bf001900376babc9accbac03d47ac76babab9483/pkg/volume/flocker/flocker.go#L242
@shaneramey

This comment has been minimized.

shaneramey commented Dec 8, 2017

Nobody would be confused about what ${ALERTMANAGER_SLACK_API_URL} would be coming from

How would you suggest supporting multiple Slack API URLs?

@jtomell

This comment has been minimized.

jtomell commented Dec 8, 2017

Nobody would be confused about what ${ALERTMANAGER_SLACK_API_URL} would be coming from

How would you suggest supporting multiple Slack API URLs?

https://www.elastic.co/guide/en/elasticsearch/reference/current/settings.html#_environment_variable_substitution

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Dec 8, 2017

No CNCF project except Kubernetes is a CM system.

Kubernetes is not a configuration management system, it's a cluster scheduler. Ksonnet would be a CM system in that space.

Every other CNCF project supports loading from environment variables.

Thank you for collecting these examples. By your definition all Go binaries support environment variables due to GOGC, so I think you're overstating your case a bit.

They's several different use cases there, of which only one (CoreDNS) is relating to a config file. Only one (githubContrib) is for a secret. The reason why ALERTMANAGER_SLACK_API_URL wouldn't work has already been indicated by @shaneramey.

Environment variables are not designed for security, and indeed putting secrets into environment variables is unwise as subprocesses inherit the environment and many debug outputs will dump out environment variables as they are not traditionally somewhere secrets live. This has been the case for much longer than 10 years.

Environment variables are available to all of a process, in the same way EC2 IAM roles are available to all of a machine, so you can't control which part of the process gets to access them. In the case of the alertmanager it is expected that untrusted users will have the ability to control parts of the config file, and thus would be able to gain access to all secrets. Future changes to the alertmanager will likely include many libraries outside our control that may expose environment variables in error situations. See https://prometheus.io/docs/operating/security/#secrets for how the Prometheus project approaches secrets.

Personally I'd suggest filing an issue with istio to ask that secret is read from a config file instead.

For non-secret information, interpolating the environment would make debugging harder as now you have an extra place to check and understand to determine what the actual config is. We already have support challenges with users asking questions without sharing enough of their config, that'd only get worse if the config wasn't self contained. A single place to look at to see how things is configured is much easier to work with, as you've got a nice clean demarcation point. It's also a general principle of Prometheus that we don't add features to make up for a lack of a configuration management system.

@balboah

This comment has been minimized.

balboah commented Dec 8, 2017

I get the point where it's insecure if you have some shared environment with untrusted users that can configure your alertmanager to expose the environment variables in a bad way. I don't not know how common this shared case is, but how can storing clear text passwords in a file be more secure? How will you know that he who installs alertmanager will protect this file or any configuration file substitution hack properly?
I don't get why this is such an important statement to you when obviously the community wants this feature. Make a vote or something, I think the open source community should be able to collaborate and work together without one person's mindset putting it to a halt.

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Dec 8, 2017

I don't not know how common this shared case is, but how can storing clear text passwords in a file be more secure?

Unix file permissions can protect the file. It's quite normal to store secrets plaintext in files, and it's not normal for debug output or systems to dump arbitrary files (which can be a directory traversal attack).

How will you know that he who installs alertmanager will protect this file or any configuration file substitution hack properly?

We don't, we can only try to ensure that the running binary doesn't expose secret fields. Umasks and general security practice should cover this. Environment variables do not have these protections.

when obviously the community wants this feature.

That there is some demand for a feature doesn't mean that it makes sense for the project. For example we've had far far more requests for push and event logging, neither of which make sense for Prometheus.

I appreciate that you really want this, however it is not in line with the project's scope or security model. I've tried to point the various requestors in the right direction on how to approach this, but at this stage we've been going around in circles for over a year. Accordingly I'm going to lock this issue now.

@prometheus prometheus locked and limited conversation to collaborators Dec 8, 2017

@brian-brazil

This comment has been minimized.

Member

brian-brazil commented Dec 8, 2017

Finally found the old article I was looking for on environment variables and secrets: http://movingfast.io/articles/environment-variables-considered-harmful/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.