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 e4abd6e..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" @@ -56,16 +57,22 @@ 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. + // 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 { - c.Client = defaultConfig.Client + if c.Client == nil && defaultConfig.Client != nil { + c.Client = defaultConfig.Client.Clone() } - if c.Storage == nil { - c.Storage = defaultConfig.Storage + if c.Storage == nil && defaultConfig.Storage != nil { + c.Storage = defaultConfig.Storage.Clone() } } @@ -82,9 +89,10 @@ func InitConfig(path string) (_ *RootConfiguration, err error) { config.Default.SetImplicitValues("default", DefaultConfig()) utils.ConfigureDecoder(&dconf) - v := viper.GetViper() + 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 @@ -98,6 +106,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 +118,32 @@ 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, 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 + + if err := v.BindEnv(profileKey); err != nil { + return nil, err + } + + envKey := envKeyReplacer.Replace(strings.ToUpper(profileKey)) + + if _, ok := os.LookupEnv(envKey); ok { + 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 2cd9d32..b533051 100644 --- a/internal/cac/config/config_test.go +++ b/internal/cac/config/config_test.go @@ -34,19 +34,133 @@ 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; 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) + }) + + // 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") + + // 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) { t.Setenv("CLIENT_ISSUER_URL", "https://postmance.eu.authz.cloudentity.io/postmance/system") 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) 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..536ea8b --- /dev/null +++ b/internal/cac/config/testdata/profile_env_conflict.yaml @@ -0,0 +1,16 @@ +# 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" + # 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"},