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

Allow to disable env variable expansion by setting DEX_EXPAND_ENV = false #1876

Closed
heidemn-faro opened this issue Dec 2, 2020 · 3 comments · Fixed by #1902
Closed

Allow to disable env variable expansion by setting DEX_EXPAND_ENV = false #1876

heidemn-faro opened this issue Dec 2, 2020 · 3 comments · Fixed by #1902
Milestone

Comments

@heidemn-faro
Copy link
Contributor

heidemn-faro commented Dec 2, 2020

Is your feature request related to a problem? Please describe.
This is both a bug and a feature request.
The current way to expand env variables in the Dex config is error-prone, since it happens before the YAML parsing.
Therefore, weird hacks are needed e.g. to support $ characters in the LDAP bindPW.
We're currently using a workaround there by defining an env variable ${DOLLAR} with value $.
#1051
#1382

Describe the solution you'd like to see
Add a toplevel config variable expandEnv which defaults to true (for compatibility with existing config files), but can be set to false by Dex admins who don't need variable expansion at all.

Describe alternatives you've considered
#1838

Additional context
I can make this change, but @sagikazarmark or @bonifaido can you tell me if such a PR would be accepted?

Current implementation:

These parts would be adapted in the PR.

func (s *Storage) UnmarshalJSON(b []byte) error {
	// ...
	if len(store.Config) != 0 {
		data := []byte(os.ExpandEnv(string(store.Config)))
		if err := json.Unmarshal(data, storageConfig); err != nil {
			return fmt.Errorf("parse storage config: %v", err)
		}
	}
	// ...
}

func (c *Connector) UnmarshalJSON(b []byte) error {
	// ...
	if len(conn.Config) != 0 {
		data := []byte(os.ExpandEnv(string(conn.Config)))
		if err := json.Unmarshal(data, connConfig); err != nil {
			return fmt.Errorf("parse connector config: %v", err)
		}
	}
	// ...
}
@sagikazarmark
Copy link
Member

How about an env var instead that can disable this behavior? Then you don't have to parse the configuration upfront. Something like DEX_EXPAND_ENV.

The config layer needs a broader refactor in my opinion, but adding an env var like this would probably improve the situation in the short term.

@heidemn-faro
Copy link
Contributor Author

heidemn-faro commented Dec 3, 2020

@sagikazarmark an env variable is also fine for me. Then I'll implement it like this. 👍
DEX_EXPAND_ENV = 0 -> don't expand
any other value, or unset -> expand

@heidemn-faro heidemn-faro changed the title Add config variable expandEnv so env variable expansion can be disabled Allow to disable env variable expansion by setting DEX_EXPAND_ENV = 0 Dec 29, 2020
@heidemn-faro heidemn-faro changed the title Allow to disable env variable expansion by setting DEX_EXPAND_ENV = 0 Allow to disable env variable expansion by setting DEX_EXPAND_ENV = false Dec 30, 2020
@heidemn-faro
Copy link
Contributor Author

Update:
DEX_EXPAND_ENV = falsy value -> don't expand.
Unset, or truthy value -> expand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants