From de49c69cbf0299fd990b667dc6d098fd88497459 Mon Sep 17 00:00:00 2001 From: Piotr Janus Date: Fri, 12 Jun 2026 23:44:01 +0200 Subject: [PATCH 1/3] test: prove default/profile client credential conflict via env vars When both the default client and a profile client are configured through environment variables, the profile silently uses the default client's credentials instead of its own. Two coupled defects cause this: - profile client env vars (PROFILES__CLIENT_*) are never read, so the profile has no client of its own - a profile without its own client falls back to the default client by sharing the same pointer, so it inherits the default's credentials and is aliased to it Add two failing tests (and a fixture) that assert the correct behavior to document the bug. --- internal/cac/config/config_test.go | 90 +++++++++++++++++++ .../config/testdata/profile_env_conflict.yaml | 13 +++ 2 files changed, 103 insertions(+) create mode 100644 internal/cac/config/testdata/profile_env_conflict.yaml diff --git a/internal/cac/config/config_test.go b/internal/cac/config/config_test.go index 2cd9d32..364adb6 100644 --- a/internal/cac/config/config_test.go +++ b/internal/cac/config/config_test.go @@ -34,12 +34,102 @@ func TestReadingConfiguration(t *testing.T) { require.NotEmpty(t, "aaaaaaaaaaaaa", profile.Client.ClientID) }) + t.Run("prefers profile config over default config", func(t *testing.T) { + rootConf, err := config.InitConfig("./../../../examples/e2e/config.yaml") + require.NoError(t, err) + + defaultConf, err := rootConf.ForProfile("") + require.NoError(t, err) + + profile, err := rootConf.ForProfile("stage") + require.NoError(t, err) + + // values defined in the profile take precedence over the default config + require.NotNil(t, profile.Client) + require.Equal(t, "https://janus.eu.authz.cloudentity.io/janus/system", profile.Client.IssuerURL.String()) + require.NotEqual(t, defaultConf.Client.IssuerURL.String(), profile.Client.IssuerURL.String()) + + require.Equal(t, "fb346c287c4d4e378cbae39aa0cxxxxx", profile.Client.ClientID) + require.NotEqual(t, defaultConf.Client.ClientID, profile.Client.ClientID) + + require.NotNil(t, profile.Storage) + require.Equal(t, []string{"/tmp/other"}, profile.Storage.DirPath) + require.NotEqual(t, defaultConf.Storage.DirPath, profile.Storage.DirPath) + + // values not overridden in the profile fall back to the default config + require.NotNil(t, profile.Logging) + require.Equal(t, defaultConf.Logging.Level, profile.Logging.Level) + require.Equal(t, defaultConf.Logging.Format, profile.Logging.Format) + }) + t.Run("fail on invalid path", func(t *testing.T) { _, err := config.InitConfig("./invalid.json") require.Error(t, err) require.Equal(t, "open ./invalid.json: no such file or directory", err.Error()) }) + // Reproduces a bug: when both the default client and a profile client are + // configured through environment variables, the profile silently ends up + // using the default client's credentials instead of its own. + t.Run("profile client from env does not collide with default client from env", func(t *testing.T) { + // default client credentials via env + t.Setenv("CLIENT_ISSUER_URL", "https://default.example.com/default/system") + t.Setenv("CLIENT_CLIENT_ID", "env-default-id") + t.Setenv("CLIENT_CLIENT_SECRET", "env-default-secret") + + // stage profile client credentials via env + t.Setenv("PROFILES_STAGE_CLIENT_ISSUER_URL", "https://stage.example.com/stage/system") + t.Setenv("PROFILES_STAGE_CLIENT_CLIENT_ID", "env-stage-id") + t.Setenv("PROFILES_STAGE_CLIENT_CLIENT_SECRET", "env-stage-secret") + + rootConf, err := config.InitConfig("./testdata/profile_env_conflict.yaml") + require.NoError(t, err) + + defaultConf, err := rootConf.ForProfile("") + require.NoError(t, err) + + stage, err := rootConf.ForProfile("stage") + require.NoError(t, err) + + // sanity: default picks up its own env credentials + require.Equal(t, "env-default-id", defaultConf.Client.ClientID) + require.Equal(t, "env-default-secret", defaultConf.Client.ClientSecret) + + // the profile must use ITS OWN env credentials, not the default's + require.Equal(t, "env-stage-id", stage.Client.ClientID) + require.Equal(t, "env-stage-secret", stage.Client.ClientSecret) + require.Equal(t, "https://stage.example.com/stage/system", stage.Client.IssuerURL.String()) + + // the two clients must not be the same backing object + require.NotEqual(t, defaultConf.Client.ClientID, stage.Client.ClientID) + }) + + // Reproduces the mechanism behind the conflict above: a profile that does + // not define its own client falls back to the default client by sharing the + // SAME pointer, so mutating one mutates the other. + t.Run("profile without client is not aliased to the default client", func(t *testing.T) { + t.Setenv("CLIENT_ISSUER_URL", "https://default.example.com/default/system") + t.Setenv("CLIENT_CLIENT_ID", "env-default-id") + t.Setenv("CLIENT_CLIENT_SECRET", "env-default-secret") + + rootConf, err := config.InitConfig("./testdata/profile_env_conflict.yaml") + require.NoError(t, err) + + defaultConf, err := rootConf.ForProfile("") + require.NoError(t, err) + + stage, err := rootConf.ForProfile("stage") + require.NoError(t, err) + + // the profile should fall back to the default values, but as an + // independent copy, not a shared pointer + require.NotSame(t, defaultConf.Client, stage.Client) + + stage.Client.ClientID = "mutated-via-stage" + require.Equal(t, "env-default-id", defaultConf.Client.ClientID, + "mutating the profile client must not affect the default client") + }) + t.Run("read config from env", func(t *testing.T) { t.Setenv("CLIENT_ISSUER_URL", "https://postmance.eu.authz.cloudentity.io/postmance/system") t.Setenv("CLIENT_CLIENT_ID", "test-cid1") diff --git a/internal/cac/config/testdata/profile_env_conflict.yaml b/internal/cac/config/testdata/profile_env_conflict.yaml new file mode 100644 index 0000000..2c9761b --- /dev/null +++ b/internal/cac/config/testdata/profile_env_conflict.yaml @@ -0,0 +1,13 @@ +# Default client and profile client credentials are intentionally NOT set here. +# They are meant to be provided via environment variables (secrets should not +# live in committed config). The profile only declares non-secret settings. +logging: + level: info + format: text +storage: + dir_path: "/tmp/default-data" + +profiles: + stage: + storage: + dir_path: "/tmp/stage-data" From 1190838ed9a75396359036b13ac39cc16f9d0f7f Mon Sep 17 00:00:00 2001 From: Piotr Janus Date: Sat, 13 Jun 2026 00:21:34 +0200 Subject: [PATCH 2/3] fix: resolve default/profile client credential conflict from env vars Two coupled defects caused a profile to silently use the default client's credentials when both were configured via environment variables: - profile client env vars (PROFILES__CLIENT_*) were never read, so a profile had no client of its own - a profile without its own client fell back to the default client by sharing the same pointer, aliasing the two configs Fixes: - bind each default config key per profile and promote any set profile env var into viper's explicit override layer, which is deep-merged on Unmarshal (AutomaticEnv/BindEnv values otherwise do not merge into nested maps that originate from a config file) - deep-copy the default sub-configs on fallback in SetImplicitValues so profiles never share or mutate the default's objects - use an isolated viper instance instead of the global singleton so repeated InitConfig calls do not leak explicit overrides into each other The previously failing tests now pass. --- internal/cac/config/config.go | 44 +++++++++++++++++++++++++----- internal/cac/config/config_test.go | 4 +-- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/internal/cac/config/config.go b/internal/cac/config/config.go index e4abd6e..c7e4500 100644 --- a/internal/cac/config/config.go +++ b/internal/cac/config/config.go @@ -56,16 +56,21 @@ func (c *Configuration) SetImplicitValues(name string, defaultConfig Configurati c.Name = name } - if c.Logging == nil { - c.Logging = defaultConfig.Logging + // Fall back to the default config, but as an independent copy so that + // profiles never share (and accidentally mutate) the default's objects. + if c.Logging == nil && defaultConfig.Logging != nil { + clone := *defaultConfig.Logging + c.Logging = &clone } - if c.Client == nil { - c.Client = defaultConfig.Client + if c.Client == nil && defaultConfig.Client != nil { + clone := *defaultConfig.Client + c.Client = &clone } - if c.Storage == nil { - c.Storage = defaultConfig.Storage + if c.Storage == nil && defaultConfig.Storage != nil { + clone := *defaultConfig.Storage + c.Storage = &clone } } @@ -82,7 +87,7 @@ func InitConfig(path string) (_ *RootConfiguration, err error) { config.Default.SetImplicitValues("default", DefaultConfig()) utils.ConfigureDecoder(&dconf) - v := viper.GetViper() + v := viper.New() v.AutomaticEnv() v.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) @@ -98,6 +103,10 @@ func InitConfig(path string) (_ *RootConfiguration, err error) { v.SetDefault(k, val) } + // Leaf keys of the (squashed) default config, e.g. "client.client_id". + // These are the keys that can also be set per profile via env variables. + defaultKeys := v.AllKeys() + if path != "" { v.SetConfigFile(path) @@ -106,6 +115,27 @@ func InitConfig(path string) (_ *RootConfiguration, err error) { } } + // viper's AutomaticEnv only resolves keys it already knows about, and env + // bindings do not merge into nested maps that originate from a config file + // during Unmarshal. So for every known profile, bind each default config key + // to its env variable (e.g. PROFILES_STAGE_CLIENT_CLIENT_ID) and, when that + // variable is set, promote it into viper's explicit override layer which is + // deep-merged on Unmarshal. Profiles without env overrides are untouched and + // still fall back to the default config in SetImplicitValues. + for name := range v.GetStringMap("profiles") { + for _, key := range defaultKeys { + profileKey := "profiles." + name + "." + key + + if err := v.BindEnv(profileKey); err != nil { + return nil, err + } + + if v.IsSet(profileKey) { + v.Set(profileKey, v.Get(profileKey)) + } + } + } + if err := v.Unmarshal(&config, utils.ConfigureDecoder); err != nil { return nil, err } diff --git a/internal/cac/config/config_test.go b/internal/cac/config/config_test.go index 364adb6..edd0246 100644 --- a/internal/cac/config/config_test.go +++ b/internal/cac/config/config_test.go @@ -135,8 +135,8 @@ func TestReadingConfiguration(t *testing.T) { t.Setenv("CLIENT_CLIENT_ID", "test-cid1") t.Setenv("CLIENT_CLIENT_SECRET", "test-secret") - // FIXME reading profiles from env variables is not yet supported - // t.Setenv("PROFILES_STAGE_CLIENT_CLIENT_SECRET", "test-secret") + // Reading profile values from env variables is covered by + // "profile client from env does not collide with default client from env". rootConf, err := config.InitConfig("") require.NoError(t, err) From 7218abc360029d3dc54fe026bd1a4fa540ce0990 Mon Sep 17 00:00:00 2001 From: Piotr Janus Date: Fri, 3 Jul 2026 16:09:45 +0200 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20deep-?= =?UTF-8?q?copy=20fallback=20configs=20and=20gate=20env=20promotion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve Copilot review comments on the profile/default client env fix: - Deep-copy the client and storage configs on fallback. A plain struct copy left the embedded acpclient.Config's reference fields (the *url.URL endpoints, Scopes slice) and MultiStorageConfiguration.DirPath slice aliased with the default, so mutations could still leak across profiles. Add Clone methods on client.Configuration and MultiStorageConfiguration and use them in SetImplicitValues. Logging holds only value fields, so its plain copy is left as-is. - Gate env promotion on os.LookupEnv instead of v.IsSet. v.IsSet is also true for config-file values, so file-provided profile values were being copied into viper's override layer unnecessarily (contradicting the "untouched" comment). Only actual env overrides are now promoted; file values stay in their native layer. - Assert pointer inequality directly in the collision test, and extend the aliasing test to cover the deep-copied reference fields (IssuerURL, Scopes) and default-storage fallback via a bare profile fixture. --- internal/cac/client/config.go | 41 ++++++++++++++++++- internal/cac/config/config.go | 28 ++++++++----- internal/cac/config/config_test.go | 26 +++++++++++- .../config/testdata/profile_env_conflict.yaml | 3 ++ internal/cac/storage/multi.go | 16 ++++++++ 5 files changed, 102 insertions(+), 12 deletions(-) diff --git a/internal/cac/client/config.go b/internal/cac/client/config.go index fa66840..43c499b 100644 --- a/internal/cac/client/config.go +++ b/internal/cac/client/config.go @@ -1,6 +1,10 @@ package client -import acpclient "github.com/cloudentity/acp-client-go" +import ( + "net/url" + + acpclient "github.com/cloudentity/acp-client-go" +) type Configuration struct { // nolint @@ -9,6 +13,41 @@ type Configuration struct { Insecure bool `json:"insecure"` } +// Clone returns a deep copy of the configuration. The embedded acpclient.Config +// holds reference fields (the *url.URL endpoints and the Scopes slice), so a +// plain struct copy would still alias them with the original. Callers rely on +// Clone to obtain a fully independent value that can be mutated in isolation. +func (c *Configuration) Clone() *Configuration { + if c == nil { + return nil + } + + clone := *c + + if c.Scopes != nil { + clone.Scopes = append([]string(nil), c.Scopes...) + } + + clone.RedirectURL = cloneURL(c.RedirectURL) + clone.IssuerURL = cloneURL(c.IssuerURL) + clone.TokenURL = cloneURL(c.TokenURL) + clone.AuthorizeURL = cloneURL(c.AuthorizeURL) + clone.PushedAuthorizationRequestEndpoint = cloneURL(c.PushedAuthorizationRequestEndpoint) + clone.UserinfoURL = cloneURL(c.UserinfoURL) + + return &clone +} + +func cloneURL(u *url.URL) *url.URL { + if u == nil { + return nil + } + + clone := *u + + return &clone +} + var DefaultConfig = func() *Configuration { return &Configuration{ Insecure: false, diff --git a/internal/cac/config/config.go b/internal/cac/config/config.go index c7e4500..7b9d125 100644 --- a/internal/cac/config/config.go +++ b/internal/cac/config/config.go @@ -1,6 +1,7 @@ package config import ( + "os" "strings" "github.com/cloudentity/cac/internal/cac/client" @@ -58,19 +59,20 @@ func (c *Configuration) SetImplicitValues(name string, defaultConfig Configurati // Fall back to the default config, but as an independent copy so that // profiles never share (and accidentally mutate) the default's objects. + // Logging only holds value fields, so a plain struct copy is enough; the + // client and storage configs carry reference fields and are deep-copied + // via their Clone methods. if c.Logging == nil && defaultConfig.Logging != nil { clone := *defaultConfig.Logging c.Logging = &clone } if c.Client == nil && defaultConfig.Client != nil { - clone := *defaultConfig.Client - c.Client = &clone + c.Client = defaultConfig.Client.Clone() } if c.Storage == nil && defaultConfig.Storage != nil { - clone := *defaultConfig.Storage - c.Storage = &clone + c.Storage = defaultConfig.Storage.Clone() } } @@ -88,8 +90,9 @@ func InitConfig(path string) (_ *RootConfiguration, err error) { utils.ConfigureDecoder(&dconf) v := viper.New() + envKeyReplacer := strings.NewReplacer(".", "_") v.AutomaticEnv() - v.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) + v.SetEnvKeyReplacer(envKeyReplacer) if decoder, err = mapstructure.NewDecoder(&dconf); err != nil { return nil, err @@ -118,10 +121,13 @@ func InitConfig(path string) (_ *RootConfiguration, err error) { // viper's AutomaticEnv only resolves keys it already knows about, and env // bindings do not merge into nested maps that originate from a config file // during Unmarshal. So for every known profile, bind each default config key - // to its env variable (e.g. PROFILES_STAGE_CLIENT_CLIENT_ID) and, when that - // variable is set, promote it into viper's explicit override layer which is - // deep-merged on Unmarshal. Profiles without env overrides are untouched and - // still fall back to the default config in SetImplicitValues. + // to its env variable (e.g. PROFILES_STAGE_CLIENT_CLIENT_ID) and, only when + // that env variable is actually set, promote its value into viper's explicit + // override layer which is deep-merged on Unmarshal. We gate on os.LookupEnv + // (not v.IsSet, which is also true for config-file values) so file-provided + // values stay in their native layer and profiles without env overrides are + // left untouched, still falling back to the default config in + // SetImplicitValues. for name := range v.GetStringMap("profiles") { for _, key := range defaultKeys { profileKey := "profiles." + name + "." + key @@ -130,7 +136,9 @@ func InitConfig(path string) (_ *RootConfiguration, err error) { return nil, err } - if v.IsSet(profileKey) { + envKey := envKeyReplacer.Replace(strings.ToUpper(profileKey)) + + if _, ok := os.LookupEnv(envKey); ok { v.Set(profileKey, v.Get(profileKey)) } } diff --git a/internal/cac/config/config_test.go b/internal/cac/config/config_test.go index edd0246..b533051 100644 --- a/internal/cac/config/config_test.go +++ b/internal/cac/config/config_test.go @@ -100,7 +100,9 @@ func TestReadingConfiguration(t *testing.T) { require.Equal(t, "env-stage-secret", stage.Client.ClientSecret) require.Equal(t, "https://stage.example.com/stage/system", stage.Client.IssuerURL.String()) - // the two clients must not be the same backing object + // the two clients must not be the same backing object; assert pointer + // inequality directly so a regression is caught even if the IDs match + require.NotSame(t, defaultConf.Client, stage.Client) require.NotEqual(t, defaultConf.Client.ClientID, stage.Client.ClientID) }) @@ -128,6 +130,28 @@ func TestReadingConfiguration(t *testing.T) { stage.Client.ClientID = "mutated-via-stage" require.Equal(t, "env-default-id", defaultConf.Client.ClientID, "mutating the profile client must not affect the default client") + + // reference fields inside the embedded acpclient.Config must be + // deep-copied too, not aliased between the profile and the default + require.NotNil(t, defaultConf.Client.IssuerURL) + require.NotSame(t, defaultConf.Client.IssuerURL, stage.Client.IssuerURL, + "the profile client must not share the default's IssuerURL pointer") + + require.NotEmpty(t, defaultConf.Client.Scopes) + stage.Client.Scopes[0] = "mutated-scope" + require.NotEqual(t, "mutated-scope", defaultConf.Client.Scopes[0], + "mutating the profile client scopes must not affect the default client") + + // a profile without its own storage falls back to the default storage, + // which must also be an independent copy (its DirPath slice is not shared) + bare, err := rootConf.ForProfile("bare") + require.NoError(t, err) + + require.NotSame(t, defaultConf.Storage, bare.Storage) + require.NotEmpty(t, defaultConf.Storage.DirPath) + bare.Storage.DirPath[0] = "/tmp/mutated" + require.NotEqual(t, "/tmp/mutated", defaultConf.Storage.DirPath[0], + "mutating the profile storage dir path must not affect the default storage") }) t.Run("read config from env", func(t *testing.T) { diff --git a/internal/cac/config/testdata/profile_env_conflict.yaml b/internal/cac/config/testdata/profile_env_conflict.yaml index 2c9761b..536ea8b 100644 --- a/internal/cac/config/testdata/profile_env_conflict.yaml +++ b/internal/cac/config/testdata/profile_env_conflict.yaml @@ -11,3 +11,6 @@ profiles: stage: storage: dir_path: "/tmp/stage-data" + # a profile that declares nothing of its own, so client and storage both fall + # back to (independent copies of) the default config + bare: {} diff --git a/internal/cac/storage/multi.go b/internal/cac/storage/multi.go index 8888c62..35466c1 100644 --- a/internal/cac/storage/multi.go +++ b/internal/cac/storage/multi.go @@ -14,6 +14,22 @@ type MultiStorageConfiguration struct { DirPath []string `json:"dir_path"` } +// Clone returns a copy with its own backing DirPath slice so callers never +// share (and accidentally mutate) the original's slice. +func (c *MultiStorageConfiguration) Clone() *MultiStorageConfiguration { + if c == nil { + return nil + } + + clone := *c + + if c.DirPath != nil { + clone.DirPath = append([]string(nil), c.DirPath...) + } + + return &clone +} + var DefaultMultiStorageConfig = func() *MultiStorageConfiguration { return &MultiStorageConfiguration{ DirPath: []string{"data"},