When using DEX_EXPAND_ENV, environment values with " or \ break the resulting JSON

tuminoid commented 1 month ago

Preflight Checklist

[X] I agree to follow the Code of Conduct that this project adheres to.
[X] I have searched the issue tracker for an issue that matches the one I want to file, without success.
[X] I am not looking for support or already pursued the available support channels without success.

Version

2.29.0, main, doesn't matter

Storage Type

etcd

Installation Type

Binary

Expected Behavior

User can supply password (or other config via environment variable, while using DEX_EXPAND_ENV) in the config YAML, and the values would be safely converted into JSON.

Actual Behavior

Unmashaling JSON converted from connector YAML config fails:

=== RUN   TestUnmarshalConfigWithEnvExpand
    config_test.go:445: failed to decode config: error unmarshaling JSON: parse connector config: invalid character 'd' after object key:value pair

Steps To Reproduce

Apply the following diff to config_test.go:

diff --git a/cmd/dex/config_test.go b/cmd/dex/config_test.go
index c6d37cb0..87a78f80 100644
--- a/cmd/dex/config_test.go
+++ b/cmd/dex/config_test.go
@@ -273,7 +273,7 @@ func checkUnmarshalConfigWithEnv(t *testing.T, dexExpandEnv string, wantExpandEn
    os.Setenv("DEX_FOO_USER_PASSWORD", "$2a$10$33EMT0cVYVlPy6WAMCLsceLYjWhuHpbz5yuZxu/GAFj03J9Lytjuy")
    // For os.ExpandEnv ($VAR -> value_of_VAR):
    os.Setenv("DEX_FOO_POSTGRES_HOST", "10.0.0.1")
-   os.Setenv("DEX_FOO_OIDC_CLIENT_SECRET", "bar")
+   os.Setenv("DEX_FOO_OIDC_CLIENT_SECRET", `abc"def\ghi`)
    if dexExpandEnv != "UNSET" {
        os.Setenv("DEX_EXPAND_ENV", dexExpandEnv)
    } else {

It will fail in make test with:

=== RUN   TestUnmarshalConfigWithEnvExpand
    config_test.go:445: failed to decode config: error unmarshaling JSON: parse connector config: invalid character 'd' after object key:value pair

The root cause is in function at cmd/dex/config.go:341. The call to os.ExpandEnv on L362 is unaware of the JSON context in which the variables are being expanded, and has a comment about it already. JSON enforces that values are enclosed in quotes, hence extra unescaped quotes or escape characters in the fields make the resulting JSON invalid.

Additional Information

These JSON illegal characters can appear in passwords, so the real use-case this came up was LDAP connector bindPW field. OIDC secret is used for reproduction as triggering it has a single line diff.

Configuration

config_test.go has the config.

Logs

=== RUN   TestUnmarshalConfigWithEnvExpand
    config_test.go:443: failed to decode config: error unmarshaling JSON: parse connector config: invalid character 'd' after object key:value pair
--- FAIL: TestUnmarshalConfigWithEnvExpand (0.00s)

tuminoid commented 1 month ago

Since this is more or less known issue based on the comment in the code, it means you maintainers have probably an idea how it would be good to be fixed. Based on my debug conn.Config is a structure, so just checking up a keyname for bindPW and doing JSON illegal character replacing, but would at least need some sort of structure parsing etc, which feels hacky.

Let me know which solution would feel right here, I'll try fixing it!

tuminoid commented 3 weeks ago

Ping @nabokihms and @sagikazarmark, would need your advise here on the preferred solution.

nabokihms commented 3 days ago

Sorry for the long delay. I think it is ok to encode values using json marshal before templating.

dexidp / dex