Fix configuration with env vars

[hypnoglow]

Related issue

• Env vars for jwks_url in id_token mutator not working in versions >v0.19.0-beta.1 #276
• Env vars for client ID/secret in oauth2_introspection don't work anymore in v0.19.0-beta.1 #270

Proposed changes

Currently env vars do not work for any configuration property. One reason is because BindEnv is never called. The other thing that actually the usage of BindEnv is incorrect: this function DOES NOT accept multiple keys (see source for details). Even if BindEnv would be used correctly, it will not resolve env vars like SERVE_PROXY_PORT because it does not replaces dots with underscores.

This MR provides the same way of loading env vars as in hydra.

Checklist

• I have read the contributing guidelines
• I have read the security policy
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got green light (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation within the code base (if appropriate)
• I have documented my changes in the
  developer guide (if appropriate)

[aeneasr]

Removing these will have a detrimental effect. Viper does not automatically check environment variables. You have to bind them explicitly.

[hypnoglow]

Viper does not automatically check environment variables. You have to bind them explicitly.

Why not? It indeed loads envs automatically: https://github.com/spf13/viper/blob/master/viper.go#L1039

In hydra, everything works fine without any explicit bindings.

[aeneasr]

Your analysis is unfortunately incorrect. In Hydra, we do not (yet) use JSON Schema to validate the config. However, validating against the Config JSON Schema requires all keys to be loaded. That's just the way Viper works (it is quirky).

[hypnoglow]

Sorry, I do not understand how we suppose to explicitly bind all keys, since most of them were recently removed for example in #258 . How do envs like AUTHENTICATORS_OAUTH2_INTROSPECTION_CONFIG_PRE_AUTHORIZATION_CLIENT_ID suppose to work?

(also related to the comment below about tests)

[aeneasr]

While the keys have been removed as constants (because we don't explicitly need them any more - except for BindEnvs()) they are still being loaded using viper.Get(). However, I think it would be better to add a patch to our fork to automatically bind all envs when doing AutomaticEnv() instead of doing both AutomaticEnv() as well as BindEnv(). This is a known (and unsolved issue) in viper that has lead to some frustration (me included): spf13/viper#761 (and all linked issues).

Then, we can throw away BindEnvs() which - to be honest - sucks anyways.

[aeneasr]

This however would need some thinking as _ can both be . as well as _. So we'd end up with a lot of keys when "autom-mounting" all environment keys. For example env var FOO_BAR could both be foo_bar as well as foo.bar. That would in turn cause issues when we set additionalProperties: false in the JSON Config Schema - which is not great because with that property set people would immediately find any typos they're doing. Maybe by defining some "categories" that always replace _ with . would be the way to go. Not sure yet, needs some thinking.

[aeneasr]

Actually, I just found my original issue which touched this topic in 2016 :D spf13/viper#212

[hypnoglow]

Oh, I see, things getting complicated, combining viper behaviour and the introduction of json schemas.

If you come up with the idea how should we solve it, let me know.

I've pushed changes that make env vars work for defined keys (e.g. SERVE_PROXY_PORT), but envs like AUTHENTICATORS_OAUTH2_INTROSPECTION_CONFIG_PRE_AUTHORIZATION_CLIENT_ID still do not work. If you feel like we don't need this intermediary improvement, feel free to close this MR.

[aeneasr]

I wrote a small helper that is capable of extracting all configuration keys from the JSON Schema and that binds those keys to viper. This will resolve all of #280 #279 #276 #270 in a way that's future proof and very little effort code-wise and further homogenizes the configuration setup: ory/x#85

[hypnoglow]

@aeneasr Looks like a nice solution to the problem! Thanks