Allow users to bypass keychain reading for headless (#757)

Author: mcncl

cmd/auth/login.go

@@ -55,6 +55,9 @@ Examples:
 }
 
 // LoginWithToken stores a token for an organization in the system keychain.
+// When the keychain is unavailable (e.g. BUILDKITE_NO_KEYRING=1 is set), it
+// still registers the org and selects it in config so that commands resolve the
+// org correctly; the caller is expected to supply the token via BUILDKITE_API_TOKEN.
 func LoginWithToken(f *factory.Factory, org, token string) error {
 	if org == "" {
 		return errors.New("--org is required when --token is provided")
@@ -64,13 +67,14 @@ func LoginWithToken(f *factory.Factory, org, token string) error {
 	}
 
 	kr := keyring.New()
-	if !kr.IsAvailable() {
-		return errors.New("system keychain is not available; cannot store token")
-	}
-	if err := kr.Set(org, token); err != nil {
-		return fmt.Errorf("failed to store token in keychain: %w", err)
+	if kr.IsAvailable() {
+		if err := kr.Set(org, token); err != nil {
+			return fmt.Errorf("failed to store token in keychain: %w", err)
+		}
+		fmt.Println("Token stored securely in system keychain.")
+	} else {
+		fmt.Println("Keychain unavailable; token not stored. Use BUILDKITE_API_TOKEN to supply your token at runtime.")
 	}
-	fmt.Println("Token stored securely in system keychain.")
 
 	if err := f.Config.EnsureOrganization(org); err != nil {
 		return fmt.Errorf("failed to register organization in config: %w", err)

internal/config/config_test.go

@@ -5,6 +5,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/buildkite/cli/v3/pkg/keyring"
 	"github.com/spf13/afero"
 )
 
@@ -322,6 +323,37 @@ func TestConfig(t *testing.T) {
 	})
 }
 
+func TestAPITokenForOrgNoKeyring(t *testing.T) {
+	// Ensure BUILDKITE_NO_KEYRING disables keychain access entirely and that
+	// APITokenForOrg falls through to the config file (legacy) path without
+	// attempting to call the OS keychain.
+	setEnv(t, "BUILDKITE_NO_KEYRING", "1")
+	setEnv(t, "CI", "")
+	setEnv(t, "BUILDKITE", "")
+	setEnv(t, "BUILDKITE_API_TOKEN", "")
+	keyring.ResetForTesting()
+	t.Cleanup(keyring.ResetForTesting)
+
+	fs := afero.NewMemMapFs()
+	content := []byte("organizations:\n  my-org:\n    api_token: legacy-token\n")
+	if err := afero.WriteFile(fs, configFile(), content, 0o600); err != nil {
+		t.Fatalf("failed to write config: %v", err)
+	}
+
+	conf := New(fs, nil)
+
+	// Should return the legacy file token without touching the keychain.
+	if got := conf.APITokenForOrg("my-org"); got != "legacy-token" {
+		t.Errorf("APITokenForOrg() = %q, want %q", got, "legacy-token")
+	}
+
+	// Keyring must report unavailable.
+	kr := keyring.New()
+	if kr.IsAvailable() {
+		t.Error("expected keyring to be unavailable when BUILDKITE_NO_KEYRING=1")
+	}
+}
+
 func TestExperiments(t *testing.T) {
 	t.Run("defaults to empty", func(t *testing.T) {
 		unsetEnv(t, "BUILDKITE_EXPERIMENTS")

pkg/keyring/keyring.go

@@ -70,9 +70,22 @@ func MockForTesting() {
 	})
 }
 
+// ResetForTesting resets the availability cache so that the next call to
+// New() re-evaluates the environment. Intended for use in tests only.
+func ResetForTesting() {
+	keyringAvailableOnce = sync.Once{}
+	keyringAvailable = false
+}
+
 // isKeyringAvailable checks if the system keyring can be used
 func isKeyringAvailable() bool {
 	keyringAvailableOnce.Do(func() {
+		// Disable keyring if explicitly opted out
+		if os.Getenv("BUILDKITE_NO_KEYRING") != "" {
+			keyringAvailable = false
+			return
+		}
+
 		// Disable keyring in CI environments
 		if os.Getenv("CI") != "" || os.Getenv("BUILDKITE") != "" {
 			keyringAvailable = false

pkg/keyring/keyring_test.go

@@ -0,0 +1,102 @@
+package keyring
+
+import (
+	"os"
+	"testing"
+)
+
+// setEnv sets an environment variable for the duration of the test and
+// restores the original value (or unsets it) via t.Cleanup.
+func setEnv(t *testing.T, key, value string) {
+	t.Helper()
+	original, had := os.LookupEnv(key)
+	if err := os.Setenv(key, value); err != nil {
+		t.Fatalf("failed to set env %s: %v", key, err)
+	}
+	t.Cleanup(func() {
+		if had {
+			os.Setenv(key, original)
+		} else {
+			os.Unsetenv(key)
+		}
+		// Reset the once so the next test starts fresh.
+		ResetForTesting()
+	})
+	// Reset now so this test sees the new env value.
+	ResetForTesting()
+}
+
+func TestIsKeyringAvailable(t *testing.T) {
+	// These tests manipulate package-level state (sync.Once) so must not run
+	// in parallel with each other.
+
+	t.Run("disabled by BUILDKITE_NO_KEYRING", func(t *testing.T) {
+		setEnv(t, "BUILDKITE_NO_KEYRING", "1")
+		setEnv(t, "CI", "")
+		setEnv(t, "BUILDKITE", "")
+
+		kr := New()
+		if kr.IsAvailable() {
+			t.Error("expected keyring to be unavailable when BUILDKITE_NO_KEYRING is set")
+		}
+	})
+
+	t.Run("disabled by CI", func(t *testing.T) {
+		setEnv(t, "CI", "true")
+		setEnv(t, "BUILDKITE_NO_KEYRING", "")
+		setEnv(t, "BUILDKITE", "")
+
+		kr := New()
+		if kr.IsAvailable() {
+			t.Error("expected keyring to be unavailable when CI is set")
+		}
+	})
+
+	t.Run("disabled by BUILDKITE", func(t *testing.T) {
+		setEnv(t, "BUILDKITE", "true")
+		setEnv(t, "BUILDKITE_NO_KEYRING", "")
+		setEnv(t, "CI", "")
+
+		kr := New()
+		if kr.IsAvailable() {
+			t.Error("expected keyring to be unavailable when BUILDKITE is set")
+		}
+	})
+}
+
+func TestNoKeyringGet(t *testing.T) {
+	setEnv(t, "BUILDKITE_NO_KEYRING", "1")
+	setEnv(t, "CI", "")
+	setEnv(t, "BUILDKITE", "")
+
+	kr := New()
+	token, err := kr.Get("my-org")
+	if token != "" {
+		t.Errorf("Get() returned non-empty token with keyring disabled, got %q", token)
+	}
+	if err == nil {
+		t.Error("Get() expected ErrNotFound when keyring is disabled, got nil")
+	}
+}
+
+func TestNoKeyringSet(t *testing.T) {
+	setEnv(t, "BUILDKITE_NO_KEYRING", "1")
+	setEnv(t, "CI", "")
+	setEnv(t, "BUILDKITE", "")
+
+	kr := New()
+	if err := kr.Set("my-org", "token-123"); err != nil {
+		t.Errorf("Set() returned unexpected error with keyring disabled: %v", err)
+	}
+}
+
+func TestNoKeyringDelete(t *testing.T) {
+	setEnv(t, "BUILDKITE_NO_KEYRING", "1")
+	setEnv(t, "CI", "")
+	setEnv(t, "BUILDKITE", "")
+
+	kr := New()
+	if err := kr.Delete("my-org"); err != nil {
+		t.Errorf("Delete() returned unexpected error with keyring disabled: %v", err)
+	}
+}