From cca6992764703141e0fb81fba85d731314bee156 Mon Sep 17 00:00:00 2001 From: Ivanna Lisetska Date: Fri, 5 Jun 2026 16:42:34 -0600 Subject: [PATCH] feat(config): add credential_store key - Add credential_store config key (auto, keyring, shm) with precedence env > local > user - Propagate the configured value into BUILDKITE_CREDENTIAL_STORE at startup so bk auth login uses it without --credential-store - Update help text on bk config set/get and bk auth login TASK: SUP-7238 --- cmd/auth/login.go | 4 ++ cmd/config/config.go | 22 +++--- cmd/config/config_test.go | 19 +++++ cmd/config/get.go | 20 +++--- cmd/config/list.go | 4 ++ cmd/config/set.go | 20 +++--- internal/config/config.go | 48 ++++++++++--- internal/config/config_test.go | 104 +++++++++++++++++++++++++++ main.go | 5 ++ pkg/cmd/factory/factory.go | 20 ++++++ pkg/cmd/factory/factory_test.go | 120 ++++++++++++++++++++++++++++++++ 11 files changed, 352 insertions(+), 34 deletions(-) diff --git a/cmd/auth/login.go b/cmd/auth/login.go index 9feac9cf..352d6801 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -65,6 +65,10 @@ Examples: # Login on a headless Linux host using an in-memory /dev/shm credential store $ bk auth login --device --credential-store shm + # Or set the default once so 'bk auth login' uses /dev/shm without flags: + $ bk config set credential_store shm + $ bk auth login --device + # Login with read-only access $ bk auth login --scopes read_only diff --git a/cmd/config/config.go b/cmd/config/config.go index 5c0a0831..7c91ffde 100644 --- a/cmd/config/config.go +++ b/cmd/config/config.go @@ -38,14 +38,15 @@ Examples: type ConfigKey string const ( - KeySelectedOrg ConfigKey = "selected_org" - KeyOutputFormat ConfigKey = "output_format" - KeyNoPager ConfigKey = "no_pager" - KeyQuiet ConfigKey = "quiet" - KeyNoInput ConfigKey = "no_input" - KeyPager ConfigKey = "pager" - KeyTelemetry ConfigKey = "telemetry" - KeyExperiments ConfigKey = "experiments" + KeySelectedOrg ConfigKey = "selected_org" + KeyOutputFormat ConfigKey = "output_format" + KeyNoPager ConfigKey = "no_pager" + KeyQuiet ConfigKey = "quiet" + KeyNoInput ConfigKey = "no_input" + KeyPager ConfigKey = "pager" + KeyTelemetry ConfigKey = "telemetry" + KeyExperiments ConfigKey = "experiments" + KeyCredentialStore ConfigKey = "credential_store" ) // AllKeys returns all valid configuration keys @@ -59,6 +60,7 @@ func AllKeys() []ConfigKey { KeyPager, KeyTelemetry, KeyExperiments, + KeyCredentialStore, } } @@ -103,6 +105,8 @@ func (k ConfigKey) ValidValues() []string { return []string{"json", "yaml", "text"} case KeyNoPager, KeyQuiet, KeyNoInput, KeyTelemetry: return []string{"true", "false"} + case KeyCredentialStore: + return []string{"auto", "keyring", "shm"} default: return nil } @@ -150,6 +154,8 @@ func SetConfigValue(conf *config.Config, key ConfigKey, value string, local bool return conf.SetTelemetry(v) case KeyExperiments: return conf.SetExperiments(value) + case KeyCredentialStore: + return conf.SetCredentialStore(value, local) } return nil diff --git a/cmd/config/config_test.go b/cmd/config/config_test.go index 0ea6df7b..6f4a123e 100644 --- a/cmd/config/config_test.go +++ b/cmd/config/config_test.go @@ -18,6 +18,7 @@ func TestValidateKey(t *testing.T) { "no_input", "pager", "experiments", + "credential_store", } for _, key := range validKeys { @@ -131,4 +132,22 @@ func TestConfigKeyValidValues(t *testing.T) { t.Errorf("KeyPager.ValidValues() = %v, want nil", values) } }) + + t.Run("credential_store has valid values", func(t *testing.T) { + t.Parallel() + + values := KeyCredentialStore.ValidValues() + if values == nil { + t.Fatal("expected valid values for credential_store") + } + expected := []string{"auto", "keyring", "shm"} + if len(values) != len(expected) { + t.Fatalf("got %d values, want %d", len(values), len(expected)) + } + for i, want := range expected { + if values[i] != want { + t.Errorf("values[%d] = %q, want %q", i, values[i], want) + } + } + }) } diff --git a/cmd/config/get.go b/cmd/config/get.go index 32b0f2e5..235fbedd 100644 --- a/cmd/config/get.go +++ b/cmd/config/get.go @@ -17,17 +17,19 @@ Returns the effective value after applying precedence rules: Environment variable > Local config (.bk.yaml) > User config (~/.config/bk.yaml) > Default Valid keys: - selected_org Organization slug to use - output_format Default output format (json, yaml, text) - no_pager Disable pager for text output (true, false) - quiet Suppress progress output (true, false) - no_input Disable interactive prompts (true, false) - pager Custom pager command - experiments Enabled experiment flags + selected_org Organization slug to use + output_format Default output format (json, yaml, text) + no_pager Disable pager for text output (true, false) + quiet Suppress progress output (true, false) + no_input Disable interactive prompts (true, false) + pager Custom pager command + experiments Enabled experiment flags + credential_store Default credential store for tokens (auto, keyring, shm) Examples: $ bk config get output_format - $ bk config get pager` + $ bk config get pager + $ bk config get credential_store` } func (c *GetCmd) Run() error { @@ -71,6 +73,8 @@ func (c *GetCmd) Run() error { value = conf.Pager() case KeyExperiments: value = conf.Experiments() + case KeyCredentialStore: + value = conf.CredentialStore() } if value != "" { diff --git a/cmd/config/list.go b/cmd/config/list.go index 283dbd20..24f62d9e 100644 --- a/cmd/config/list.go +++ b/cmd/config/list.go @@ -5,6 +5,7 @@ import ( "os" "github.com/buildkite/cli/v3/pkg/cmd/factory" + "github.com/buildkite/cli/v3/pkg/keyring" ) type ListCmd struct { @@ -63,6 +64,9 @@ func (c *ListCmd) Run() error { if v := conf.Experiments(); v != "" { items = append(items, configItem{string(KeyExperiments), v, "effective"}) } + if v := conf.CredentialStore(); v != "" && v != keyring.StoreAuto { + items = append(items, configItem{string(KeyCredentialStore), v, "effective"}) + } } if c.Local && !inGitRepo { diff --git a/cmd/config/set.go b/cmd/config/set.go index 87341643..09e1ba06 100644 --- a/cmd/config/set.go +++ b/cmd/config/set.go @@ -17,13 +17,14 @@ func (c *SetCmd) Help() string { return `Set a configuration value. Valid keys: - selected_org Organization slug to use - output_format Default output format (json, yaml, text) - no_pager Disable pager for text output (true, false) - quiet Suppress progress output (true, false) - no_input Disable interactive prompts (true, false) [user config only] - pager Custom pager command [user config only] - telemetry Enable anonymous usage telemetry (true, false) [user config only] + selected_org Organization slug to use + output_format Default output format (json, yaml, text) + no_pager Disable pager for text output (true, false) + quiet Suppress progress output (true, false) + no_input Disable interactive prompts (true, false) [user config only] + pager Custom pager command [user config only] + telemetry Enable anonymous usage telemetry (true, false) [user config only] + credential_store Default credential store for tokens (auto, keyring, shm) Examples: # Set default output format to YAML @@ -36,7 +37,10 @@ Examples: $ bk config set output_format text --local # Set a custom pager - $ bk config set pager "less -RS"` + $ bk config set pager "less -RS" + + # Pin token storage to /dev/shm (recommended for headless Linux dev hosts) + $ bk config set credential_store shm` } func (c *SetCmd) Run() error { diff --git a/internal/config/config.go b/internal/config/config.go index 48e07d78..b20d5650 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -50,16 +50,17 @@ type orgConfig struct { } type fileConfig struct { - SelectedOrg string `yaml:"selected_org"` - Organizations map[string]orgConfig `yaml:"organizations,omitempty"` - Pipelines []string `yaml:"pipelines,omitempty"` - NoPager bool `yaml:"no_pager,omitempty"` - OutputFormat string `yaml:"output_format,omitempty"` - Quiet bool `yaml:"quiet,omitempty"` - NoInput bool `yaml:"no_input,omitempty"` - Pager string `yaml:"pager,omitempty"` - Telemetry *bool `yaml:"telemetry,omitempty"` - Experiments string `yaml:"experiments,omitempty"` + SelectedOrg string `yaml:"selected_org"` + Organizations map[string]orgConfig `yaml:"organizations,omitempty"` + Pipelines []string `yaml:"pipelines,omitempty"` + NoPager bool `yaml:"no_pager,omitempty"` + OutputFormat string `yaml:"output_format,omitempty"` + Quiet bool `yaml:"quiet,omitempty"` + NoInput bool `yaml:"no_input,omitempty"` + Pager string `yaml:"pager,omitempty"` + Telemetry *bool `yaml:"telemetry,omitempty"` + Experiments string `yaml:"experiments,omitempty"` + CredentialStore string `yaml:"credential_store,omitempty"` } // Config contains the configuration for the currently selected organization @@ -393,6 +394,33 @@ func (conf *Config) SetExperiments(v string) error { return conf.writeUser() } +// CredentialStore returns the configured credential store for token storage. +// Precedence: env > local > user > "auto". +func (conf *Config) CredentialStore() string { + return firstNonEmpty( + os.Getenv(keyring.CredentialStoreEnv), + conf.local.CredentialStore, + conf.user.CredentialStore, + keyring.StoreAuto, + ) +} + +// SetCredentialStore writes the credential store preference. An empty value +// clears it. Invalid stores are rejected before any write. +func (conf *Config) SetCredentialStore(v string, saveLocal bool) error { + if v != "" { + if err := keyring.ValidateCredentialStore(v); err != nil { + return err + } + } + if !saveLocal { + conf.user.CredentialStore = v + return conf.writeUser() + } + conf.local.CredentialStore = v + return conf.writeLocal() +} + func lookupBoolEnv(key string) (bool, bool) { v := os.Getenv(key) if v == "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index dba6c53d..5d725344 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -462,3 +462,107 @@ func TestHasExperiment(t *testing.T) { }) } } + +func TestCredentialStore(t *testing.T) { + t.Run("defaults to auto", func(t *testing.T) { + unsetEnv(t, keyring.CredentialStoreEnv) + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + + if got := conf.CredentialStore(); got != keyring.StoreAuto { + t.Errorf("CredentialStore() = %q, want %q", got, keyring.StoreAuto) + } + }) + + t.Run("env overrides user config", func(t *testing.T) { + setEnv(t, keyring.CredentialStoreEnv, keyring.StoreKeyring) + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + if err := conf.SetCredentialStore(keyring.StoreSHM, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + + if got := conf.CredentialStore(); got != keyring.StoreKeyring { + t.Errorf("CredentialStore() = %q, want %q (env should override)", got, keyring.StoreKeyring) + } + }) + + t.Run("user config overrides default", func(t *testing.T) { + unsetEnv(t, keyring.CredentialStoreEnv) + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + if err := conf.SetCredentialStore(keyring.StoreSHM, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + + if got := conf.CredentialStore(); got != keyring.StoreSHM { + t.Errorf("CredentialStore() = %q, want %q", got, keyring.StoreSHM) + } + }) + + t.Run("local config overrides user config", func(t *testing.T) { + unsetEnv(t, keyring.CredentialStoreEnv) + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + if err := conf.SetCredentialStore(keyring.StoreKeyring, false); err != nil { + t.Fatalf("SetCredentialStore(user): %v", err) + } + if err := conf.SetCredentialStore(keyring.StoreSHM, true); err != nil { + t.Fatalf("SetCredentialStore(local): %v", err) + } + + if got := conf.CredentialStore(); got != keyring.StoreSHM { + t.Errorf("CredentialStore() = %q, want %q (local should win over user)", got, keyring.StoreSHM) + } + }) + + t.Run("rejects unknown values", func(t *testing.T) { + unsetEnv(t, keyring.CredentialStoreEnv) + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + + if err := conf.SetCredentialStore("vault", false); err == nil { + t.Error("SetCredentialStore(\"vault\") expected error, got nil") + } + if got := conf.CredentialStore(); got != keyring.StoreAuto { + t.Errorf("CredentialStore() after rejected write = %q, want %q", got, keyring.StoreAuto) + } + }) + + t.Run("empty value clears the preference", func(t *testing.T) { + unsetEnv(t, keyring.CredentialStoreEnv) + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + if err := conf.SetCredentialStore(keyring.StoreSHM, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + if err := conf.SetCredentialStore("", false); err != nil { + t.Fatalf("SetCredentialStore(\"\") error: %v", err) + } + + if got := conf.CredentialStore(); got != keyring.StoreAuto { + t.Errorf("CredentialStore() after clear = %q, want %q", got, keyring.StoreAuto) + } + }) + + t.Run("setter persists across reload", func(t *testing.T) { + unsetEnv(t, keyring.CredentialStoreEnv) + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + if err := conf.SetCredentialStore(keyring.StoreSHM, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + + conf2 := New(fs, nil) + if got := conf2.CredentialStore(); got != keyring.StoreSHM { + t.Errorf("CredentialStore() after reload = %q, want %q", got, keyring.StoreSHM) + } + }) +} diff --git a/main.go b/main.go index c812937b..59178454 100644 --- a/main.go +++ b/main.go @@ -36,6 +36,7 @@ import ( "github.com/buildkite/cli/v3/internal/config" bkErrors "github.com/buildkite/cli/v3/internal/errors" "github.com/buildkite/cli/v3/pkg/analytics" + "github.com/buildkite/cli/v3/pkg/cmd/factory" ) // Kong CLI structure, with base commands defined as additional commands are defined in their respective files @@ -302,6 +303,10 @@ func run() int { conf := config.New(nil, nil) + // Must run before kong dispatches: bk auth login builds its keyring + // before factory.New() does. + factory.ApplyCredentialStoreFromConfig(conf) + tracker := analytics.Init("dev", conf.TelemetryEnabled()) defer tracker.Close() tracker.SetOrg(conf.OrganizationSlug()) diff --git a/pkg/cmd/factory/factory.go b/pkg/cmd/factory/factory.go index 0d0cfd63..2709d6b9 100644 --- a/pkg/cmd/factory/factory.go +++ b/pkg/cmd/factory/factory.go @@ -174,6 +174,24 @@ func buildUserAgent(suffix string) string { return fmt.Sprintf("%s %s", baseUserAgent, suffix) } +// ApplyCredentialStoreFromConfig propagates the configured credential store +// into BUILDKITE_CREDENTIAL_STORE so subsequent keyring.New() calls pick it +// up. A non-empty env wins; the helper is idempotent and safe to call from +// both main() and factory.New(). +func ApplyCredentialStoreFromConfig(conf *config.Config) { + if os.Getenv(keyring.CredentialStoreEnv) != "" { + return + } + store := conf.CredentialStore() + if store == "" || store == keyring.StoreAuto { + return + } + if err := keyring.ValidateCredentialStore(store); err != nil { + return + } + _ = os.Setenv(keyring.CredentialStoreEnv, store) +} + func (a *gqlHTTPClient) Do(req *http.Request) (*http.Response, error) { // Auth and User-Agent are injected by AuthTransport in the // shared HTTP transport chain, so we don't set them here. @@ -195,6 +213,8 @@ func New(opts ...FactoryOpt) (*Factory, error) { conf := config.New(nil, repo) + ApplyCredentialStoreFromConfig(conf) + token := conf.APIToken() if cfg.orgOverride != "" { if t := conf.APITokenForOrg(cfg.orgOverride); t != "" { diff --git a/pkg/cmd/factory/factory_test.go b/pkg/cmd/factory/factory_test.go index 0de383f5..c1a74b5a 100644 --- a/pkg/cmd/factory/factory_test.go +++ b/pkg/cmd/factory/factory_test.go @@ -4,12 +4,35 @@ import ( "io" "net/http" "net/http/httptest" + "os" "strings" "testing" + "github.com/buildkite/cli/v3/internal/config" + "github.com/buildkite/cli/v3/pkg/keyring" buildkite "github.com/buildkite/go-buildkite/v4" + "github.com/spf13/afero" ) +// withCredentialStoreEnv sets BUILDKITE_CREDENTIAL_STORE for the test (or +// unsets it when value is empty) and restores prior state via t.Cleanup. +func withCredentialStoreEnv(t *testing.T, value string) { + t.Helper() + original, had := os.LookupEnv(keyring.CredentialStoreEnv) + t.Cleanup(func() { + if had { + _ = os.Setenv(keyring.CredentialStoreEnv, original) + return + } + _ = os.Unsetenv(keyring.CredentialStoreEnv) + }) + if value == "" { + _ = os.Unsetenv(keyring.CredentialStoreEnv) + return + } + _ = os.Setenv(keyring.CredentialStoreEnv, value) +} + func TestRedactHeaders(t *testing.T) { tests := []struct { name string @@ -177,6 +200,103 @@ func TestBuildUserAgent(t *testing.T) { }) } +func TestApplyCredentialStoreFromConfig(t *testing.T) { + t.Run("exports configured store when env unset", func(t *testing.T) { + withCredentialStoreEnv(t, "") + + conf := config.New(afero.NewMemMapFs(), nil) + if err := conf.SetCredentialStore(keyring.StoreSHM, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + + ApplyCredentialStoreFromConfig(conf) + + if got := os.Getenv(keyring.CredentialStoreEnv); got != keyring.StoreSHM { + t.Errorf("env = %q, want %q", got, keyring.StoreSHM) + } + }) + + t.Run("env value is preserved", func(t *testing.T) { + withCredentialStoreEnv(t, keyring.StoreKeyring) + + conf := config.New(afero.NewMemMapFs(), nil) + if err := conf.SetCredentialStore(keyring.StoreSHM, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + + ApplyCredentialStoreFromConfig(conf) + + if got := os.Getenv(keyring.CredentialStoreEnv); got != keyring.StoreKeyring { + t.Errorf("env = %q, want %q (env should win)", got, keyring.StoreKeyring) + } + }) + + t.Run("auto in config does not export env", func(t *testing.T) { + withCredentialStoreEnv(t, "") + + conf := config.New(afero.NewMemMapFs(), nil) + if err := conf.SetCredentialStore(keyring.StoreAuto, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + + ApplyCredentialStoreFromConfig(conf) + + if got, set := os.LookupEnv(keyring.CredentialStoreEnv); set { + t.Errorf("env should remain unset for auto, got %q", got) + } + }) + + t.Run("missing config does not export env", func(t *testing.T) { + withCredentialStoreEnv(t, "") + + conf := config.New(afero.NewMemMapFs(), nil) + + ApplyCredentialStoreFromConfig(conf) + + if got, set := os.LookupEnv(keyring.CredentialStoreEnv); set { + t.Errorf("env should remain unset, got %q", got) + } + }) + + t.Run("idempotent across multiple calls", func(t *testing.T) { + // main() and factory.New() both invoke the bridge. + withCredentialStoreEnv(t, "") + + conf := config.New(afero.NewMemMapFs(), nil) + if err := conf.SetCredentialStore(keyring.StoreSHM, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + + ApplyCredentialStoreFromConfig(conf) + first := os.Getenv(keyring.CredentialStoreEnv) + ApplyCredentialStoreFromConfig(conf) + second := os.Getenv(keyring.CredentialStoreEnv) + + if first != keyring.StoreSHM || second != keyring.StoreSHM { + t.Errorf("expected stable env=%q across calls, got first=%q second=%q", + keyring.StoreSHM, first, second) + } + }) + + t.Run("empty env falls through to config", func(t *testing.T) { + // keyring.New() treats empty env as auto. + withCredentialStoreEnv(t, "") + _ = os.Setenv(keyring.CredentialStoreEnv, "") + t.Cleanup(func() { _ = os.Unsetenv(keyring.CredentialStoreEnv) }) + + conf := config.New(afero.NewMemMapFs(), nil) + if err := conf.SetCredentialStore(keyring.StoreSHM, false); err != nil { + t.Fatalf("SetCredentialStore: %v", err) + } + + ApplyCredentialStoreFromConfig(conf) + + if got := os.Getenv(keyring.CredentialStoreEnv); got != keyring.StoreSHM { + t.Errorf("env = %q, want %q", got, keyring.StoreSHM) + } + }) +} + func TestNewUserAgent(t *testing.T) { t.Chdir(t.TempDir())