fix unmarshaling of expanded environment variables with special characters

[tuminoid]

Overview

If we expand environment values directly with os.ExpandEnv() over whole config, we might end up in a situation where the environment variable has escape characters that break the resulting JSON, and unmarshalling fails. Instead of expanding the entire config with single call, we recurse through the config and expand the values in leaves one by one.

What this PR does / why we need it

Closes #3689

[tuminoid]

/cc @nabokihms
Here is the PR for fixing the escaping of special characters from #3689 using the approach we agreed on.

Suggested label is kind/bug.

It should be noted that this may cause issues for existing configs that have passwords with escapes due previous implementation requiring them. This must not be many installations, but maybe good to note regardless.

[nabokihms]

LGTM! Could you also add it to the storage config?

dex/cmd/dex/config.go

Line 316 in 6c77662

data = []byte(os.ExpandEnv(string(store.Config)))

[tuminoid]

LGTM! Could you also add it to the storage config?

dex/cmd/dex/config.go

Line 316 in 6c77662

data = []byte(os.ExpandEnv(string(store.Config)))

I implemented this and then started adding test for it, but there doesn't seem to be any value that can actually hold escaped strings, like there is on connector side. PostgresHost seems to be only string there, and there is AFAIK no legit case of putting escapes in. If we fix the parsing, it means the parsed URL will be invalid as a host string, instead of parsing the config failing, thus deferring the inevitable failure from parsing to connecting. Which I'd say is not what we want, would you agree?

[tuminoid]

LGTM! Could you also add it to the storage config?

dex/cmd/dex/config.go

Line 316 in 6c77662

data = []byte(os.ExpandEnv(string(store.Config)))

I implemented this and then started adding test for it, but there doesn't seem to be any value that can actually hold escaped strings, like there is on connector side. PostgresHost seems to be only string there, and there is AFAIK no legit case of putting escapes in. If we fix the parsing, it means the parsed URL will be invalid as a host string, instead of parsing the config failing, thus deferring the inevitable failure from parsing to connecting. Which I'd say is not what we want, would you agree?

Ah nevermind. Of course there is passwords in storage side as well, not just present in current config.

Edit: added similar test password to Postgres as the simplest option to verify this as I had on the connector side for LDAP.

[nabokihms]

@tuminoid thank you for your patience

[tuminoid]

@tuminoid thank you for your patience

No worries, thanks for taking in the fix!