diff --git a/go.mod b/go.mod index c6681f60..bd3db496 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( ) require ( + al.essio.dev/pkg/shellescape v1.5.1 // indirect github.com/agnivade/levenshtein v1.2.1 // indirect github.com/alecthomas/chroma/v2 v2.14.0 // indirect github.com/alexflint/go-arg v1.5.1 // indirect @@ -41,10 +42,12 @@ require ( github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf // indirect github.com/charmbracelet/x/exp/strings v0.0.0-20240722160745-212f7b056ed0 // indirect github.com/charmbracelet/x/term v0.2.1 // indirect + github.com/danieljoos/wincred v1.2.2 // indirect github.com/dlclark/regexp2 v1.11.0 // indirect github.com/dustin/go-humanize v1.0.1 // indirect github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect github.com/go-viper/mapstructure/v2 v2.4.0 // indirect + github.com/godbus/dbus/v5 v5.1.0 // indirect github.com/google/uuid v1.6.0 // indirect github.com/gorilla/css v1.0.1 // indirect github.com/microcosm-cc/bluemonday v1.0.27 // indirect @@ -54,6 +57,7 @@ require ( github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect github.com/yuin/goldmark v1.7.8 // indirect github.com/yuin/goldmark-emoji v1.0.5 // indirect + github.com/zalando/go-keyring v0.2.6 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect ) diff --git a/go.sum b/go.sum index ee3c32b2..a4b27fcb 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,5 @@ +al.essio.dev/pkg/shellescape v1.5.1 h1:86HrALUujYS/h+GtqoB26SBEdkWfmMI6FubjXlsXyho= +al.essio.dev/pkg/shellescape v1.5.1/go.mod h1:6sIqp7X2P6mThCQ7twERpZTuigpr6KbZWtls1U8I890= dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk= dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk= github.com/AlecAivazis/survey/v2 v2.3.7 h1:6I/u8FvytdGsgonrYsVn2t8t4QiRnh6QSTqkkhIiSjQ= @@ -97,6 +99,8 @@ github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/cyphar/filepath-securejoin v0.4.1 h1:JyxxyPEaktOD+GAnqIqTf9A8tHyAG22rowi7HkoSU1s= github.com/cyphar/filepath-securejoin v0.4.1/go.mod h1:Sdj7gXlvMcPZsbhwhQ33GguGLDGQL7h7bg04C/+u9jI= +github.com/danieljoos/wincred v1.2.2 h1:774zMFJrqaeYCK2W57BgAem/MLi6mtSE47MB6BOJ0i0= +github.com/danieljoos/wincred v1.2.2/go.mod h1:w7w4Utbrz8lqeMbDAK0lkNJUv5sAOkFi7nd/ogr0Uh8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -130,6 +134,8 @@ github.com/go-git/go-git/v5 v5.16.3 h1:Z8BtvxZ09bYm/yYNgPKCzgWtaRqDTgIKRgIRHBfU6 github.com/go-git/go-git/v5 v5.16.3/go.mod h1:4Ge4alE/5gPs30F2H1esi2gPd69R0C39lolkucHBOp8= github.com/go-viper/mapstructure/v2 v2.4.0 h1:EBsztssimR/CONLSZZ04E8qAkxNYq4Qp9LvH92wZUgs= github.com/go-viper/mapstructure/v2 v2.4.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM= +github.com/godbus/dbus/v5 v5.1.0 h1:4KLkAxT3aOY8Li4FRJe/KvhoNFFxo0m6fNuFUO8QJUk= +github.com/godbus/dbus/v5 v5.1.0/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 h1:f+oWsMOmNPc8JmEHVZIycC7hBoQxHH9pNKQORJNozsQ= github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8/go.mod h1:wcDNUvekVysuuOpQKo3191zZyTpiI6se1N1ULghS0sw= github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= @@ -262,6 +268,8 @@ github.com/yuin/goldmark v1.7.8 h1:iERMLn0/QJeHFhxSt3p6PeN9mGnvIKSpG9YYorDMnic= github.com/yuin/goldmark v1.7.8/go.mod h1:uzxRWxtg69N339t3louHJ7+O03ezfj6PlliRlaOzY1E= github.com/yuin/goldmark-emoji v1.0.5 h1:EMVWyCGPlXJfUXBXpuMu+ii3TIaxbVBnEX9uaDC4cIk= github.com/yuin/goldmark-emoji v1.0.5/go.mod h1:tTkZEbwu5wkPmgTcitqddVxY9osFZiavD+r4AzQrh1U= +github.com/zalando/go-keyring v0.2.6 h1:r7Yc3+H+Ux0+M72zacZoItR3UDxeWfKTcabvkI8ua9s= +github.com/zalando/go-keyring v0.2.6/go.mod h1:2TCrxYrbUNYfNS/Kgy/LSrkSQzZ5UPVH85RwfczwvcI= go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= diff --git a/internal/config/config.go b/internal/config/config.go index a518e568..12f8c0b9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -38,9 +38,9 @@ const ( // selected_org: buildkite // organizations: // buildkite: -// api_token: +// api_token: # (deprecated - tokens now stored in keychain by default) // buildkite-oss: -// api_token: +// api_token: # (deprecated - tokens now stored in keychain by default) // pipelines: # (only in local config) // - first-pipeline // - second-pipeline @@ -51,6 +51,8 @@ type Config struct { localConfig *viper.Viper // userConfig is the configuration stored in the users HOME directory. userConfig *viper.Viper + // tokenStorage is the backend used for storing API tokens (keychain by default, file-based as fallback) + tokenStorage TokenStorage } func New(fs afero.Fs, repo *git.Repository) *Config { @@ -80,10 +82,19 @@ func New(fs afero.Fs, repo *git.Repository) *Config { } _ = localConfig.ReadInConfig() - return &Config{ + conf := &Config{ localConfig: localConfig, userConfig: userConfig, } + + // Initialize token storage backend + if shouldUseKeychain() { + conf.tokenStorage = NewKeychainTokenStorage() + } else { + conf.tokenStorage = NewFileTokenStorage(conf) + } + + return conf } // OrganizationSlug gets the slug for the currently selected organization. This can be configured locally or per user. @@ -108,35 +119,70 @@ func (conf *Config) SelectOrganization(org string, inGitRepo bool) error { } // APIToken gets the API token configured for the currently selected organization +// Priority order: BUILDKITE_API_TOKEN env var ā†’ keychain ā†’ config file (legacy) func (conf *Config) APIToken() string { + // Environment variable takes precedence + if envToken := os.Getenv("BUILDKITE_API_TOKEN"); envToken != "" { + return envToken + } + slug := conf.OrganizationSlug() + if slug == "" { + return "" + } + + // Try keychain first + token, err := conf.tokenStorage.Get(slug) + if err == nil && token != "" { + return token + } + + // Fallback to file-based config for backward compatibility key := fmt.Sprintf("organizations.%s.api_token", slug) - return firstNonEmpty( - os.Getenv("BUILDKITE_API_TOKEN"), - conf.userConfig.GetString(key), - ) + return conf.userConfig.GetString(key) } -// SetTokenForOrg sets the token for the given org in the user configuration file. Tokens are not stored in the local -// configuration file to reduce the likelihood of tokens being committed to VCS +// SetTokenForOrg sets the token for the given org in the token storage (keychain by default). +// Tokens are not stored in the local configuration file to reduce the likelihood of tokens being committed to VCS func (conf *Config) SetTokenForOrg(org, token string) error { - key := fmt.Sprintf("organizations.%s.api_token", org) - conf.userConfig.Set(key, token) - return conf.userConfig.WriteConfig() + return conf.tokenStorage.Set(org, token) } -// GetTokenForOrg gets the API token for a specific organization from the user configuration +// GetTokenForOrg gets the API token for a specific organization from the token storage +// Falls back to file-based config for backward compatibility func (conf *Config) GetTokenForOrg(org string) string { + // Try token storage first (keychain by default) + token, err := conf.tokenStorage.Get(org) + if err == nil && token != "" { + return token + } + + // Fallback to file-based config key := fmt.Sprintf("organizations.%s.api_token", org) return conf.userConfig.GetString(key) } func (conf *Config) ConfiguredOrganizations() []string { + // Get orgs from file config m := conf.userConfig.GetStringMap("organizations") orgs := slices.Collect(maps.Keys(m)) + + // Add orgs from keychain storage (if using keychain) + if keychainOrgs, err := conf.tokenStorage.List(); err == nil { + for _, org := range keychainOrgs { + if !slices.Contains(orgs, org) { + orgs = append(orgs, org) + } + } + } + + // Add org from environment variable if set if o := os.Getenv("BUILDKITE_ORGANIZATION_SLUG"); o != "" { - orgs = append(orgs, o) + if !slices.Contains(orgs, o) { + orgs = append(orgs, o) + } } + return orgs } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ce802cff..2ba50875 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -34,10 +34,9 @@ func prepareTestDirectory(fs afero.Fs, fixturePath, configPath string) error { } func TestConfig(t *testing.T) { - t.Parallel() - t.Run("read in local config", func(t *testing.T) { - t.Parallel() + // Use file-based storage for tests by setting the environment variable + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") fs := afero.NewMemMapFs() err := prepareTestDirectory(fs, "local.basic.yaml", localConfigFilePath) @@ -62,7 +61,8 @@ func TestConfig(t *testing.T) { }) t.Run("GetTokenForOrg returns token for specific organization", func(t *testing.T) { - t.Parallel() + // Use file-based storage for tests + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") fs := afero.NewMemMapFs() conf := New(fs, nil) diff --git a/internal/config/keychain.go b/internal/config/keychain.go new file mode 100644 index 00000000..d46bcebb --- /dev/null +++ b/internal/config/keychain.go @@ -0,0 +1,141 @@ +package config + +import ( + "fmt" + "os" + + "github.com/zalando/go-keyring" +) + +const ( + // KeychainServiceName is the service name used when storing tokens in the system keychain + KeychainServiceName = "com.buildkite.cli" + + // EnvVarTokenStorage allows users to override token storage mechanism + // Valid values: "keychain" (default), "file" + EnvVarTokenStorage = "BUILDKITE_TOKEN_STORAGE" +) + +// TokenStorage defines the interface for storing and retrieving API tokens +type TokenStorage interface { + Get(org string) (string, error) + Set(org, token string) error + Delete(org string) error + List() ([]string, error) +} + +// KeychainTokenStorage stores tokens in the system keychain (macOS Keychain, Windows Credential Manager, Linux Secret Service) +type KeychainTokenStorage struct { + serviceName string +} + +// NewKeychainTokenStorage creates a new keychain-based token storage +func NewKeychainTokenStorage() *KeychainTokenStorage { + return &KeychainTokenStorage{ + serviceName: KeychainServiceName, + } +} + +// Get retrieves a token for the given organization from the keychain +func (k *KeychainTokenStorage) Get(org string) (string, error) { + token, err := keyring.Get(k.serviceName, org) + if err == keyring.ErrNotFound { + return "", fmt.Errorf("no token found for organization %q", org) + } + if err != nil { + return "", fmt.Errorf("failed to get token from keychain: %w", err) + } + return token, nil +} + +// Set stores a token for the given organization in the keychain +func (k *KeychainTokenStorage) Set(org, token string) error { + if err := keyring.Set(k.serviceName, org, token); err != nil { + return fmt.Errorf("failed to set token in keychain: %w", err) + } + return nil +} + +// Delete removes a token for the given organization from the keychain +func (k *KeychainTokenStorage) Delete(org string) error { + if err := keyring.Delete(k.serviceName, org); err != nil { + return fmt.Errorf("failed to delete token from keychain: %w", err) + } + return nil +} + +// List returns all organizations that have tokens stored in the keychain +// Note: This is not directly supported by the keychain API, so we'll return an empty list +// and rely on the file-based config for listing organizations +func (k *KeychainTokenStorage) List() ([]string, error) { + return []string{}, nil +} + +// FileTokenStorage stores tokens in the configuration file (legacy/fallback mode) +type FileTokenStorage struct { + conf *Config +} + +// NewFileTokenStorage creates a new file-based token storage +func NewFileTokenStorage(conf *Config) *FileTokenStorage { + return &FileTokenStorage{ + conf: conf, + } +} + +// Get retrieves a token for the given organization from the config file +func (f *FileTokenStorage) Get(org string) (string, error) { + key := fmt.Sprintf("organizations.%s.api_token", org) + token := f.conf.userConfig.GetString(key) + if token == "" { + return "", fmt.Errorf("no token found for organization %q", org) + } + return token, nil +} + +// Set stores a token for the given organization in the config file +func (f *FileTokenStorage) Set(org, token string) error { + key := fmt.Sprintf("organizations.%s.api_token", org) + f.conf.userConfig.Set(key, token) + return f.conf.userConfig.WriteConfig() +} + +// Delete removes a token for the given organization from the config file +func (f *FileTokenStorage) Delete(org string) error { + key := fmt.Sprintf("organizations.%s.api_token", org) + f.conf.userConfig.Set(key, nil) + return f.conf.userConfig.WriteConfig() +} + +// List returns all organizations that have tokens stored in the config file +func (f *FileTokenStorage) List() ([]string, error) { + orgsMap := f.conf.userConfig.GetStringMap("organizations") + orgs := make([]string, 0, len(orgsMap)) + for org := range orgsMap { + // Check if this org actually has a token + key := fmt.Sprintf("organizations.%s.api_token", org) + if f.conf.userConfig.GetString(key) != "" { + orgs = append(orgs, org) + } + } + return orgs, nil +} + +// getTokenStorageBackend determines which token storage backend to use based on environment variables +func getTokenStorageBackend() string { + backend := os.Getenv(EnvVarTokenStorage) + if backend == "" { + return "keychain" // Default to keychain + } + return backend +} + +// ShouldUseKeychain returns true if keychain storage should be used +func ShouldUseKeychain() bool { + return getTokenStorageBackend() == "keychain" +} + +// Deprecated: use ShouldUseKeychain instead +func shouldUseKeychain() bool { + return ShouldUseKeychain() +} diff --git a/internal/config/keychain_test.go b/internal/config/keychain_test.go new file mode 100644 index 00000000..0ab92340 --- /dev/null +++ b/internal/config/keychain_test.go @@ -0,0 +1,175 @@ +package config + +import ( + "testing" + + "github.com/spf13/afero" + "github.com/zalando/go-keyring" +) + +func TestKeychainTokenStorage(t *testing.T) { + t.Run("set and get token from keychain", func(t *testing.T) { + // Mock the keyring for testing + keyring.MockInit() + + storage := NewKeychainTokenStorage() + + // Set a token + err := storage.Set("test-org", "test-token-123") + if err != nil { + t.Fatalf("failed to set token: %v", err) + } + + // Get the token back + token, err := storage.Get("test-org") + if err != nil { + t.Fatalf("failed to get token: %v", err) + } + + if token != "test-token-123" { + t.Errorf("expected token 'test-token-123', got '%s'", token) + } + }) + + t.Run("get non-existent token returns error", func(t *testing.T) { + keyring.MockInit() + + storage := NewKeychainTokenStorage() + + _, err := storage.Get("nonexistent-org") + if err == nil { + t.Error("expected error when getting non-existent token") + } + }) + + t.Run("delete token from keychain", func(t *testing.T) { + keyring.MockInit() + + storage := NewKeychainTokenStorage() + + // Set a token + err := storage.Set("test-org", "test-token-123") + if err != nil { + t.Fatalf("failed to set token: %v", err) + } + + // Delete the token + err = storage.Delete("test-org") + if err != nil { + t.Fatalf("failed to delete token: %v", err) + } + + // Verify it's gone + _, err = storage.Get("test-org") + if err == nil { + t.Error("expected error after deleting token") + } + }) +} + +func TestConfigWithKeychain(t *testing.T) { + t.Run("config uses keychain by default", func(t *testing.T) { + // Mock the keyring + keyring.MockInit() + + // Ensure keychain is enabled + t.Setenv("BUILDKITE_TOKEN_STORAGE", "keychain") + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + + // Set a token + err := conf.SetTokenForOrg("my-org", "my-token-456") + if err != nil { + t.Fatalf("failed to set token: %v", err) + } + + // Get the token back + token := conf.GetTokenForOrg("my-org") + if token != "my-token-456" { + t.Errorf("expected token 'my-token-456', got '%s'", token) + } + }) + + t.Run("config falls back to file when keychain fails", func(t *testing.T) { + // Use file-based storage + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + + // Set a token (will go to file) + err := conf.SetTokenForOrg("file-org", "file-token-789") + if err != nil { + t.Fatalf("failed to set token: %v", err) + } + + // Get the token back (should read from file) + token := conf.GetTokenForOrg("file-org") + if token != "file-token-789" { + t.Errorf("expected token 'file-token-789', got '%s'", token) + } + }) +} + +func TestMigration(t *testing.T) { + t.Run("migrate tokens from file to keychain", func(t *testing.T) { + keyring.MockInit() + t.Setenv("BUILDKITE_TOKEN_STORAGE", "keychain") + + fs := afero.NewMemMapFs() + + // First create config with file-based storage + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") + conf := New(fs, nil) + + // Add some tokens to the file + conf.SetTokenForOrg("org1", "token1") + conf.SetTokenForOrg("org2", "token2") + + // Now switch to keychain mode + t.Setenv("BUILDKITE_TOKEN_STORAGE", "keychain") + conf = New(fs, nil) + + // Migrate + migrated, err := conf.MigrateTokensToKeychain() + if err != nil { + t.Fatalf("migration failed: %v", err) + } + + if migrated != 2 { + t.Errorf("expected 2 tokens migrated, got %d", migrated) + } + + // Verify tokens are in keychain + token1 := conf.GetTokenForOrg("org1") + if token1 != "token1" { + t.Errorf("expected token1, got %s", token1) + } + + token2 := conf.GetTokenForOrg("org2") + if token2 != "token2" { + t.Errorf("expected token2, got %s", token2) + } + }) + + t.Run("HasFileTokens detects tokens in file", func(t *testing.T) { + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") + + fs := afero.NewMemMapFs() + conf := New(fs, nil) + + // Initially no tokens + if conf.HasFileTokens() { + t.Error("expected no file tokens initially") + } + + // Add a token + conf.SetTokenForOrg("org1", "token1") + + // Now should detect it + if !conf.HasFileTokens() { + t.Error("expected to detect file tokens") + } + }) +} diff --git a/internal/config/migration.go b/internal/config/migration.go new file mode 100644 index 00000000..8934ade4 --- /dev/null +++ b/internal/config/migration.go @@ -0,0 +1,72 @@ +package config + +import ( + "fmt" +) + +// MigrateTokensToKeychain migrates all API tokens from the config file to the keychain +// Returns the number of tokens migrated and any error encountered +func (conf *Config) MigrateTokensToKeychain() (int, error) { + if !shouldUseKeychain() { + return 0, fmt.Errorf("keychain storage is not enabled") + } + + // Get all organizations from the config file + orgsMap := conf.userConfig.GetStringMap("organizations") + if len(orgsMap) == 0 { + return 0, nil // No organizations to migrate + } + + migrated := 0 + keychainStorage := NewKeychainTokenStorage() + + for org := range orgsMap { + // Get token from file + key := fmt.Sprintf("organizations.%s.api_token", org) + token := conf.userConfig.GetString(key) + + if token == "" { + continue // No token for this org + } + + // Store in keychain + if err := keychainStorage.Set(org, token); err != nil { + return migrated, fmt.Errorf("failed to migrate token for %q: %w", org, err) + } + + migrated++ + } + + return migrated, nil +} + +// RemoveTokensFromFile removes all API tokens from the config file after successful migration +// This should only be called after MigrateTokensToKeychain succeeds +func (conf *Config) RemoveTokensFromFile() error { + orgsMap := conf.userConfig.GetStringMap("organizations") + if len(orgsMap) == 0 { + return nil + } + + for org := range orgsMap { + key := fmt.Sprintf("organizations.%s.api_token", org) + if conf.userConfig.GetString(key) != "" { + // Set to empty string to remove the token + conf.userConfig.Set(key, "") + } + } + + return conf.userConfig.WriteConfig() +} + +// HasFileTokens checks if there are any tokens stored in the config file +func (conf *Config) HasFileTokens() bool { + orgsMap := conf.userConfig.GetStringMap("organizations") + for org := range orgsMap { + key := fmt.Sprintf("organizations.%s.api_token", org) + if conf.userConfig.GetString(key) != "" { + return true + } + } + return false +} diff --git a/pkg/cmd/configure/add/add_test.go b/pkg/cmd/configure/add/add_test.go index 26e37eb1..c9e2ed28 100644 --- a/pkg/cmd/configure/add/add_test.go +++ b/pkg/cmd/configure/add/add_test.go @@ -9,10 +9,8 @@ import ( ) func TestGetTokenForOrg(t *testing.T) { - t.Parallel() - t.Run("returns empty string when no token exists", func(t *testing.T) { - t.Parallel() + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") fs := afero.NewMemMapFs() conf := config.New(fs, nil) f := &factory.Factory{Config: conf} @@ -24,7 +22,7 @@ func TestGetTokenForOrg(t *testing.T) { }) t.Run("returns token when it exists for organization", func(t *testing.T) { - t.Parallel() + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") fs := afero.NewMemMapFs() conf := config.New(fs, nil) f := &factory.Factory{Config: conf} @@ -40,7 +38,7 @@ func TestGetTokenForOrg(t *testing.T) { }) t.Run("returns different tokens for different organizations", func(t *testing.T) { - t.Parallel() + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") fs := afero.NewMemMapFs() conf := config.New(fs, nil) f := &factory.Factory{Config: conf} @@ -61,10 +59,8 @@ func TestGetTokenForOrg(t *testing.T) { } func TestConfigureWithCredentials(t *testing.T) { - t.Parallel() - t.Run("configures organization and token", func(t *testing.T) { - t.Parallel() + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") fs := afero.NewMemMapFs() conf := config.New(fs, nil) f := &factory.Factory{Config: conf} @@ -88,10 +84,8 @@ func TestConfigureWithCredentials(t *testing.T) { } func TestConfigureTokenReuse(t *testing.T) { - t.Parallel() - t.Run("reuses existing token when available", func(t *testing.T) { - t.Parallel() + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") fs := afero.NewMemMapFs() conf := config.New(fs, nil) f := &factory.Factory{Config: conf} @@ -126,10 +120,8 @@ func TestConfigureTokenReuse(t *testing.T) { } func TestConfigureRequiresGitRepository(t *testing.T) { - t.Parallel() - t.Run("fails when not in a git repository", func(t *testing.T) { - t.Parallel() + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") fs := afero.NewMemMapFs() conf := config.New(fs, nil) diff --git a/pkg/cmd/configure/configure.go b/pkg/cmd/configure/configure.go index 6d1c1564..3f5d6d1d 100644 --- a/pkg/cmd/configure/configure.go +++ b/pkg/cmd/configure/configure.go @@ -4,6 +4,7 @@ import ( "errors" addCmd "github.com/buildkite/cli/v3/pkg/cmd/configure/add" + migrateCmd "github.com/buildkite/cli/v3/pkg/cmd/configure/migrate" "github.com/buildkite/cli/v3/pkg/cmd/factory" "github.com/spf13/cobra" ) @@ -41,6 +42,7 @@ func NewCmdConfigure(f *factory.Factory) *cobra.Command { cmd.Flags().StringVar(&token, "token", "", "API token") cmd.AddCommand(addCmd.NewCmdAdd(f)) + cmd.AddCommand(migrateCmd.NewCmdMigrate(f)) return cmd } diff --git a/pkg/cmd/configure/migrate/migrate.go b/pkg/cmd/configure/migrate/migrate.go new file mode 100644 index 00000000..cfb69281 --- /dev/null +++ b/pkg/cmd/configure/migrate/migrate.go @@ -0,0 +1,102 @@ +package migrate + +import ( + "fmt" + + "github.com/buildkite/cli/v3/internal/config" + "github.com/buildkite/cli/v3/pkg/cmd/factory" + "github.com/spf13/cobra" +) + +func NewCmdMigrate(f *factory.Factory) *cobra.Command { + var removeFromFile bool + + cmd := &cobra.Command{ + Use: "migrate", + Args: cobra.NoArgs, + Short: "Migrate API tokens from config file to secure keychain storage", + Long: `Migrate API tokens from the plain text config file to secure keychain storage. + +This command will: + 1. Detect any tokens stored in ~/.config/bk.yaml (or equivalent) + 2. Copy them to your system's secure credential storage + - macOS: Keychain + - Windows: Credential Manager + - Linux: Secret Service (GNOME Keyring / KWallet) + 3. Optionally remove tokens from the config file (use --remove-from-file) + +After migration, tokens will be read from the keychain by default, with the config +file serving as a fallback. You can verify keychain storage is being used by checking +that your config file no longer contains plain text tokens. + +To force file-based storage instead, set: BUILDKITE_TOKEN_STORAGE=file`, + RunE: func(cmd *cobra.Command, args []string) error { + return migrateRun(f, removeFromFile) + }, + } + + cmd.Flags().BoolVar(&removeFromFile, "remove-from-file", false, "Remove tokens from config file after successful migration") + + return cmd +} + +func migrateRun(f *factory.Factory, removeFromFile bool) error { + conf := f.Config + + // Check if keychain storage is enabled + if !config.ShouldUseKeychain() { + return fmt.Errorf("keychain storage is not enabled\n\nKeychain storage is disabled via BUILDKITE_TOKEN_STORAGE environment variable.\nTo use keychain storage, either unset this variable or set it to 'keychain'") + } + + // Check if there are any tokens to migrate + if !conf.HasFileTokens() { + fmt.Println("āœ“ No tokens found in config file - nothing to migrate") + fmt.Println("\nYour tokens are likely already in the keychain, or you haven't configured any organizations yet.") + fmt.Println("Use 'bk configure add' to add an organization and token.") + return nil + } + + // Show what will be migrated + fmt.Println("šŸ” Checking for tokens in config file...") + + // Get all configured organizations (this is a best effort - we'll migrate what we can) + orgs := conf.ConfiguredOrganizations() + if len(orgs) > 0 { + fmt.Printf("\nFound organization(s) in config:\n") + for _, org := range orgs { + fmt.Printf(" ā€¢ %s\n", org) + } + } + + // Perform migration + fmt.Println("\nšŸ” Migrating tokens to keychain...") + migrated, err := conf.MigrateTokensToKeychain() + if err != nil { + return fmt.Errorf("migration failed: %w", err) + } + + if migrated == 0 { + fmt.Println("āš ļø No tokens were migrated") + return nil + } + + fmt.Printf("āœ“ Successfully migrated %d token(s) to keychain\n", migrated) + + // Optionally remove from file + if removeFromFile { + fmt.Println("\nšŸ—‘ļø Removing tokens from config file...") + if err := conf.RemoveTokensFromFile(); err != nil { + fmt.Printf("āš ļø Warning: Could not remove tokens from config file: %v\n", err) + fmt.Println("You may need to manually edit ~/.config/bk.yaml") + } else { + fmt.Println("āœ“ Tokens removed from config file") + } + } else { + fmt.Println("\nšŸ’” Tip: Use --remove-from-file to clean up tokens from the config file") + fmt.Println(" Tokens will remain in the file as a backup, but the keychain will be used first.") + } + + fmt.Println("\nāœ… Migration complete! Your tokens are now stored securely in the system keychain.") + + return nil +} diff --git a/pkg/cmd/configure/migrate/migrate_test.go b/pkg/cmd/configure/migrate/migrate_test.go new file mode 100644 index 00000000..14f2f93d --- /dev/null +++ b/pkg/cmd/configure/migrate/migrate_test.go @@ -0,0 +1,117 @@ +package migrate + +import ( + "testing" + + "github.com/buildkite/cli/v3/internal/config" + "github.com/buildkite/cli/v3/pkg/cmd/factory" + "github.com/spf13/afero" + "github.com/zalando/go-keyring" +) + +func TestMigrateCommand(t *testing.T) { + t.Run("migrates tokens from file to keychain", func(t *testing.T) { + // Initialize mock keyring + keyring.MockInit() + + // Start with file-based storage + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") + + fs := afero.NewMemMapFs() + conf := config.New(fs, nil) + + // Add some tokens to the file + conf.SetTokenForOrg("org1", "token1") + conf.SetTokenForOrg("org2", "token2") + + // Verify tokens are in file + if !conf.HasFileTokens() { + t.Fatal("expected tokens to be in file") + } + + // Now switch to keychain mode + t.Setenv("BUILDKITE_TOKEN_STORAGE", "keychain") + conf = config.New(fs, nil) + + f := &factory.Factory{Config: conf} + + // Run migration + err := migrateRun(f, false) + if err != nil { + t.Fatalf("migration failed: %v", err) + } + + // Verify tokens are accessible (should be in keychain now) + token1 := conf.GetTokenForOrg("org1") + if token1 != "token1" { + t.Errorf("expected token1, got %s", token1) + } + + token2 := conf.GetTokenForOrg("org2") + if token2 != "token2" { + t.Errorf("expected token2, got %s", token2) + } + }) + + t.Run("reports when no tokens to migrate", func(t *testing.T) { + keyring.MockInit() + t.Setenv("BUILDKITE_TOKEN_STORAGE", "keychain") + + fs := afero.NewMemMapFs() + conf := config.New(fs, nil) + f := &factory.Factory{Config: conf} + + // Should not error when no tokens to migrate + err := migrateRun(f, false) + if err != nil { + t.Errorf("expected no error when no tokens to migrate, got: %v", err) + } + }) + + t.Run("fails when keychain storage is disabled", func(t *testing.T) { + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") + + fs := afero.NewMemMapFs() + conf := config.New(fs, nil) + f := &factory.Factory{Config: conf} + + // Should error when trying to migrate with keychain disabled + err := migrateRun(f, false) + if err == nil { + t.Error("expected error when keychain storage is disabled") + } + }) + + t.Run("removes tokens from file when flag is set", func(t *testing.T) { + keyring.MockInit() + + // Start with file-based storage + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") + + fs := afero.NewMemMapFs() + conf := config.New(fs, nil) + + // Add a token to the file + conf.SetTokenForOrg("org1", "token1") + + // Switch to keychain mode + t.Setenv("BUILDKITE_TOKEN_STORAGE", "keychain") + conf = config.New(fs, nil) + + f := &factory.Factory{Config: conf} + + // Run migration with remove flag + err := migrateRun(f, true) + if err != nil { + t.Fatalf("migration failed: %v", err) + } + + // Verify tokens are removed from file + // We need to check with file storage mode + t.Setenv("BUILDKITE_TOKEN_STORAGE", "file") + confFile := config.New(fs, nil) + if confFile.HasFileTokens() { + t.Error("expected tokens to be removed from file") + } + }) +}