diff --git a/go.mod b/go.mod index 27a6d5f8e..3ab21c2e6 100644 --- a/go.mod +++ b/go.mod @@ -29,6 +29,7 @@ require ( github.com/stretchr/testify v1.11.1 github.com/tomnomnom/linkheader v0.0.0-20180905144013-02ca5825eb80 github.com/xeipuuv/gojsonschema v1.2.0 + github.com/zalando/go-keyring v0.2.8 golang.org/x/crypto v0.55.0 golang.org/x/sync v0.22.0 golang.org/x/text v0.41.0 @@ -46,10 +47,12 @@ require ( github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/clipperhouse/displaywidth v0.10.0 // indirect github.com/clipperhouse/uax29/v2 v2.6.0 // indirect + github.com/danieljoos/wincred v1.2.3 // indirect github.com/distribution/distribution/v3 v3.1.1 // indirect github.com/docker/docker v28.0.3+incompatible // indirect github.com/docker/go-events v0.0.0-20250808211157-605354379745 // indirect github.com/edsrzf/mmap-go v1.2.0 // indirect + github.com/godbus/dbus/v5 v5.2.2 // indirect github.com/golang/snappy v1.0.0 // indirect github.com/google/jsonschema-go v0.4.3 // indirect github.com/klauspost/cpuid/v2 v2.3.0 // indirect @@ -335,3 +338,5 @@ replace github.com/docker/go-connections => github.com/docker/go-connections v0. replace github.com/opencontainers/selinux => github.com/opencontainers/selinux v1.13.0 replace github.com/opencontainers/runtime-spec => github.com/opencontainers/runtime-spec v1.2.0 + +replace github.com/zalando/go-keyring => github.com/checkmarx/go-keyring v0.0.0-20260812105354-1168609d33ac diff --git a/go.sum b/go.sum index 34b4cffec..411935706 100644 --- a/go.sum +++ b/go.sum @@ -223,6 +223,8 @@ github.com/charmbracelet/x/term v0.2.2 h1:xVRT/S2ZcKdhhOuSP4t5cLi5o+JxklsoEObBSg github.com/charmbracelet/x/term v0.2.2/go.mod h1:kF8CY5RddLWrsgVwpw4kAa6TESp6EB5y3uxGLeCqzAI= github.com/checkmarx/2ms/v3 v3.21.0 h1:EcabeDypNMsSidISQbziZ062HjMZQ+Hm/uOJ5AOxK8o= github.com/checkmarx/2ms/v3 v3.21.0/go.mod h1:e8f4F94MZ+iCetR/G3aw7nXdPe6TgPI92Zzk/NG1l0o= +github.com/checkmarx/go-keyring v0.0.0-20260812105354-1168609d33ac h1:q/EiuayTYiRZdeCSeauE5+G8kd3SkMfLEmMoaZDtcLI= +github.com/checkmarx/go-keyring v0.0.0-20260812105354-1168609d33ac/go.mod h1:tsMo+VpRq5NGyKfxoBVjCuMrG47yj8cmakZDO5QGii0= github.com/chromedp/cdproto v0.0.0-20230802225258-3cf4e6d46a89/go.mod h1:GKljq0VrfU4D5yc+2qA6OVr8pmO/MBbPEWqWQ/oqGEs= github.com/chromedp/chromedp v0.9.2/go.mod h1:LkSXJKONWTCHAfQasKFUZI+mxqS4tZqhmtGzzhLsnLs= github.com/chromedp/sysutil v1.0.0/go.mod h1:kgWmDdq8fTzXYcKIBqIYvRRTnYb9aNS9moAV0xufSww= @@ -284,6 +286,8 @@ github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= github.com/cyphar/filepath-securejoin v0.6.1 h1:5CeZ1jPXEiYt3+Z6zqprSAgSWiggmpVyciv8syjIpVE= github.com/cyphar/filepath-securejoin v0.6.1/go.mod h1:A8hd4EnAeyujCJRrICiOWqjS1AX0a9kM5XL+NwKoYSc= +github.com/danieljoos/wincred v1.2.3 h1:v7dZC2x32Ut3nEfRH+vhoZGvN72+dQ/snVXo/vMFLdQ= +github.com/danieljoos/wincred v1.2.3/go.mod h1:6qqX0WNrS4RzPZ1tnroDzq9kY3fu1KwE7MRLQK4X0bs= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= @@ -433,6 +437,8 @@ github.com/gobwas/httphead v0.1.0/go.mod h1:O/RXo79gxV8G+RqlR/otEwx4Q36zl9rqC5u1 github.com/gobwas/pool v0.2.1/go.mod h1:q8bcK0KcYlCgd9e7WYLm9LpyS+YeLd8JVDW6WezmKEw= github.com/gobwas/ws v1.2.1/go.mod h1:hRKAFb8wOxFROYNsT1bqfWnhX+b5MFeJM9r2ZSwg/KY= github.com/godbus/dbus/v5 v5.0.4/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= +github.com/godbus/dbus/v5 v5.2.2 h1:TUR3TgtSVDmjiXOgAAyaZbYmIeP3DPkld3jgKGV8mXQ= +github.com/godbus/dbus/v5 v5.2.2/go.mod h1:3AAv2+hPq5rdnr5txxxRwiGjPXamgoIHgz9FPBfOp3c= github.com/gofrs/flock v0.13.0 h1:95JolYOvGMqeH31+FC7D2+uULf6mG61mEZ/A8dRYMzw= github.com/gofrs/flock v0.13.0/go.mod h1:jxeyy9R1auM5S6JYDBhDt+E2TCo7DkratH4Pgi8P+Z0= github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ= diff --git a/internal/commands/agenthooks/mcp/bridge.go b/internal/commands/agenthooks/mcp/bridge.go index 1e6ee214b..251a31094 100644 --- a/internal/commands/agenthooks/mcp/bridge.go +++ b/internal/commands/agenthooks/mcp/bridge.go @@ -14,6 +14,7 @@ import ( "sync" "time" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/logger" commonParams "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/wrappers" @@ -28,8 +29,8 @@ import ( // binary guaranteed present, instead of bash/node/python (none of which are // guaranteed across Windows/macOS/Linux or on a native, Bun-based Claude install). // -// It reads the credential cx already resolved (env CX_APIKEY / cx config, loaded -// at startup), derives the realm-scoped URL from the credential's JWT `iss` +// It reads the credential cx already resolved (OS keyring / env CX_APIKEY / cx +// config, loaded at startup), derives the realm-scoped URL from the credential's JWT `iss` // claim, and forwards newline-delimited JSON-RPC between stdin/stdout and the // remote MCP's Streamable HTTP endpoint (application/json + text/event-stream, // Mcp-Session-Id). The credential is sent ONLY in the Authorization header (the @@ -135,17 +136,22 @@ var ( // productionResolveAPIKey reads it. viper itself has no internal locking. var configMu sync.Mutex -// productionResolveAPIKey returns the credential cx resolved (CX_APIKEY env / cx -// config / active session), falling back to CHECKMARX_API_KEY for parity with the -// previous Python bridge. Callers that need a credential written AFTER startup must -// call reloadConfig() first (viper is a one-shot startup snapshot). The viper read -// is guarded by configMu so it never races a concurrent reloadConfig. +// productionResolveAPIKey returns the credential cx resolved (keyring / +// CX_APIKEY env / cx config), falling back to CHECKMARX_API_KEY for parity with +// the previous Python bridge. The credentialstore read covers the keyring and +// env layers; reloadConfig() keeps the viper snapshot fresh for other keys. func productionResolveAPIKey() string { + k, err := credentialstore.Resolve(credentialstore.CredentialAPIKey) + if err == nil { + if k = strings.TrimSpace(k); k != "" { + return k + } + } configMu.Lock() - k := strings.TrimSpace(viper.GetString(commonParams.AstAPIKey)) + v := strings.TrimSpace(viper.GetString(commonParams.AstAPIKey)) configMu.Unlock() - if k != "" { - return k + if v != "" { + return v } if k := strings.TrimSpace(os.Getenv("CHECKMARX_API_KEY")); k != "" { return k @@ -169,7 +175,7 @@ Intended to be launched by an AI coding assistant as an MCP server: } } -The credential is read from cx config (or CX_APIKEY). The realm-scoped URL is +The credential is read from the OS keyring, CX_APIKEY, or cx config. The realm-scoped URL is resolved by, in order: the --mcp-url flag, the CX_MCP_URL env var, the authoritative "ast-base-url" claim from the exchanged access token (works for any region/on-prem), then an offline IAM->AST host swap. Override with --mcp-url diff --git a/internal/commands/agenthooks/mcp/main_test.go b/internal/commands/agenthooks/mcp/main_test.go new file mode 100644 index 000000000..df7e73309 --- /dev/null +++ b/internal/commands/agenthooks/mcp/main_test.go @@ -0,0 +1,29 @@ +package mcp + +import ( + "log" + "os" + "path/filepath" + "testing" + + "github.com/checkmarx/ast-cli/internal/credentialstore" + "github.com/checkmarx/ast-cli/internal/params" +) + +// TestMain isolates bridge tests from the real OS keyring and config file. +func TestMain(m *testing.M) { + testConfigDir, err := os.MkdirTemp("", "cx-mcp-test-config") + if err != nil { + log.Fatalf("failed to create test config dir: %v", err) + } + configPath := filepath.Join(testConfigDir, "checkmarxcli.yaml") + if err := os.WriteFile(configPath, nil, 0o600); err != nil { + log.Fatalf("failed to seed test config file: %v", err) + } + _ = os.Setenv(credentialstore.KeyringModeEnvVar, "disabled") + _ = os.Setenv(params.ConfigFilePathEnv, configPath) + credentialstore.ResetForTest() + exitVal := m.Run() + _ = os.RemoveAll(testConfigDir) + os.Exit(exitVal) +} diff --git a/internal/commands/auth.go b/internal/commands/auth.go index dea970040..3e6c36c96 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -5,6 +5,7 @@ import ( "log" "github.com/MakeNowJust/heredoc" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/logger" "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/wrappers" @@ -120,8 +121,16 @@ func NewAuthCommand(authWrapper wrappers.AuthWrapper, telemetryWrapper wrappers. func validLogin(telemetryWrapper wrappers.TelemetryWrapper) func(cmd *cobra.Command, args []string) error { return func(cmd *cobra.Command, args []string) error { clientID := viper.GetString(params.AccessKeyIDConfigKey) - clientSecret := viper.GetString(params.AccessKeySecretConfigKey) - apiKey := viper.GetString(params.AstAPIKey) + clientSecret, secretErr := credentialstore.Resolve(credentialstore.CredentialClientSecret) + if secretErr != nil { + logger.PrintIfVerbose(fmt.Sprintf("auth validate: resolving client secret: %v", secretErr)) + clientSecret = "" + } + apiKey, keyErr := credentialstore.Resolve(credentialstore.CredentialAPIKey) + if keyErr != nil { + logger.PrintIfVerbose(fmt.Sprintf("auth validate: resolving api key: %v", keyErr)) + apiKey = "" + } if (clientID != "" && clientSecret != "") || apiKey != "" { authWrapper := wrappers.NewAuthHTTPWrapper() authWrapper.SetPath(viper.GetString(params.ScansPathKey)) diff --git a/internal/commands/auth_login.go b/internal/commands/auth_login.go index 92daf3b83..da85c0695 100644 --- a/internal/commands/auth_login.go +++ b/internal/commands/auth_login.go @@ -6,6 +6,7 @@ import ( "os" "github.com/MakeNowJust/heredoc" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/logger" "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/wrappers" @@ -27,13 +28,13 @@ func newAuthLoginCommand() *cobra.Command { Use: "login", Short: "Authenticate to Checkmarx One via browser-based OAuth", Long: "Opens the default browser, walks the user through the Checkmarx One IAM login " + - "(including MFA), and saves the resulting refresh token to the config file's cx_apikey " + - "field — the same credential slot cx configure writes to, so every other command picks " + - "it up automatically.\n\n" + + "(including MFA), and stores the resulting refresh token securely in the OS keyring " + + "under the api-key credential slot — the same slot cx configure writes to, so every " + + "other command picks it up automatically.\n\n" + "Requires --tenant and --base-uri (or --base-auth-uri). Pass them as flags, or run " + "cx auth login with none and it prompts for the missing ones like cx configure.", Example: heredoc.Doc(` - # With flags — saves the refresh token to ~/.checkmarx/checkmarxcli.yaml + # With flags — stores the refresh token in the OS keyring $ cx auth login --base-uri https://.ast.checkmarx.net --tenant my-tenant # No flags — prompts for base URI / tenant, then opens the browser @@ -83,7 +84,7 @@ func runAuthLogin(cmd *cobra.Command, _ []string) error { return err } - return persistYamlLogin(cmd, tokens.RefreshToken) + return persistLogin(cmd, tokens.RefreshToken) } // connectionFlagsProvided reports whether any connection detail was passed as a flag. @@ -93,18 +94,24 @@ func connectionFlagsProvided(cmd *cobra.Command) bool { cmd.Flags().Changed(params.TenantFlag) } -// persistYamlLogin saves the refresh token to cx_apikey; never echoes it to stdout. -func persistYamlLogin(cmd *cobra.Command, refreshToken string) error { - configPath, err := configuration.GetConfigFilePath() - if err != nil { - return errors.Wrap(err, "failed to resolve config file path") +// persistLogin stores the refresh token under the api-key credential slot; +// never echoes it to stdout. +func persistLogin(cmd *cobra.Command, refreshToken string) error { + if err := credentialstore.Default().Store(context.Background(), credentialstore.CredentialAPIKey, refreshToken); err != nil { + return errors.Wrap(err, "failed to save refresh token") } - if err := configuration.SafeWriteSingleConfigKeyString(configPath, params.AstAPIKey, refreshToken); err != nil { - return errors.Wrap(err, "failed to save refresh token to config file") + if !credentialstore.Default().StoresInConfigFile() { + if removeErr := credentialstore.Default().RemoveConfigFileEntry(credentialstore.CredentialAPIKey); removeErr != nil { + logger.PrintIfVerbose(fmt.Sprintf("failed to remove old refresh token from config file: %v", removeErr)) + } } - // Restrict to owner-only; best-effort no-op on Windows. - if chErr := os.Chmod(configPath, configFilePerm); chErr != nil { - logger.PrintIfVerbose(fmt.Sprintf("failed to restrict config file permissions: %v", chErr)) + // Restrict a pre-existing config file to owner-only; best-effort no-op on + // Windows and when the file does not exist. + configPath, err := configuration.GetConfigFilePath() + if err == nil { + if chErr := os.Chmod(configPath, configFilePerm); chErr != nil { + logger.PrintIfVerbose(fmt.Sprintf("failed to restrict config file permissions: %v", chErr)) + } } _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Successfully authenticated to Checkmarx One server!") return nil diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index 988df38c2..cdb514cce 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -8,8 +8,10 @@ import ( "strings" "testing" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/wrappers/configuration" + "github.com/checkmarx/ast-cli/internal/wrappers/mock" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -18,8 +20,6 @@ import ( // The full runAuthLogin (browser + network) is out of scope; these cover the // deterministic pieces: persistLogin and runAuthLogout. -// swapDefaultStore swaps credentialstore.Default for a mock and restores it. - // withTempConfigDir sandboxes viper at a temp config file and clears CX_APIKEY. func withTempConfigDir(t *testing.T) string { t.Helper() @@ -31,6 +31,20 @@ func withTempConfigDir(t *testing.T) string { return dir } +// swapCredentialResolver binds a mock-backed resolver to the sandbox config +// path so credential reads/writes never reach the real OS keyring. +func swapCredentialResolver(t *testing.T) *mock.CredentialStoreMock { + t.Helper() + configPath, err := configuration.GetConfigFilePath() + if err != nil { + t.Fatalf("GetConfigFilePath failed: %v", err) + } + store := mock.NewCredentialStoreMock() + credentialstore.SetDefaultResolverForTest(credentialstore.NewResolver(configPath, credentialstore.PolicyAuto, store)) + t.Cleanup(credentialstore.ResetForTest) + return store +} + // newBufferedCmd returns a cobra command whose stdout/stderr are captured. func newBufferedCmd() (*cobra.Command, *bytes.Buffer, *bytes.Buffer) { cmd := &cobra.Command{} @@ -100,6 +114,7 @@ func TestConnectionFlagsProvided(t *testing.T) { // Logout clears cx_apikey and is idempotent. func TestRunAuthLogout_ClearsYaml(t *testing.T) { dir := withTempConfigDir(t) + swapCredentialResolver(t) configPath := filepath.Join(dir, "checkmarxcli.yaml") if err := configuration.SafeWriteSingleConfigKeyString(configPath, params.AstAPIKey, "stored-token"); err != nil { t.Fatalf("setup yaml write failed: %v", err) @@ -145,35 +160,37 @@ func TestRunAuthLogout_DoesNotClearClientCredentials(t *testing.T) { } } -// persistYamlLogin saves the refresh token to the config file. -func TestPersistYamlLogin_SavesTokenAndPrintsSuccess(t *testing.T) { +// persistLogin saves the refresh token to the credential store. +func TestPersistLogin_SavesTokenAndPrintsSuccess(t *testing.T) { _ = withTempConfigDir(t) + store := swapCredentialResolver(t) cmd, out, _ := newBufferedCmd() refreshToken := "refresh-token-abc123" - if err := persistYamlLogin(cmd, refreshToken); err != nil { - t.Fatalf("persistYamlLogin failed: %v", err) + if err := persistLogin(cmd, refreshToken); err != nil { + t.Fatalf("persistLogin failed: %v", err) } - // Check token was saved to YAML - if got := readYamlAPIKey(t); got != refreshToken { - t.Errorf("expected token saved to yaml, got %q want %q", got, refreshToken) + if got := store.Store[credentialstore.CredentialAPIKey]; got != refreshToken { + t.Errorf("expected token saved to credential store, got %q want %q", got, refreshToken) + } + if got := readYamlAPIKey(t); got != "" { + t.Errorf("expected legacy yaml entry scrubbed, got %q", got) } - // Check success message was printed if !strings.Contains(out.String(), "Successfully authenticated to Checkmarx One server!") { t.Errorf("expected success message, got: %q", out.String()) } } -// persistYamlLogin does not echo the token to stdout -func TestPersistYamlLogin_DoesNotEchoToken(t *testing.T) { +// persistLogin does not echo the token to stdout +func TestPersistLogin_DoesNotEchoToken(t *testing.T) { _ = withTempConfigDir(t) cmd, out, _ := newBufferedCmd() refreshToken := "secret-refresh-token-12345" - if err := persistYamlLogin(cmd, refreshToken); err != nil { - t.Fatalf("persistYamlLogin failed: %v", err) + if err := persistLogin(cmd, refreshToken); err != nil { + t.Fatalf("persistLogin failed: %v", err) } output := out.String() @@ -182,8 +199,8 @@ func TestPersistYamlLogin_DoesNotEchoToken(t *testing.T) { } } -// persistYamlLogin handles different token formats -func TestPersistYamlLogin_DifferentTokenFormats(t *testing.T) { +// persistLogin handles different token formats +func TestPersistLogin_DifferentTokenFormats(t *testing.T) { testTokens := []string{ "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9...", "simple-token", @@ -193,27 +210,28 @@ func TestPersistYamlLogin_DifferentTokenFormats(t *testing.T) { for _, token := range testTokens { t.Run("token format", func(t *testing.T) { _ = withTempConfigDir(t) + store := swapCredentialResolver(t) cmd, _, _ := newBufferedCmd() - if err := persistYamlLogin(cmd, token); err != nil { - t.Fatalf("persistYamlLogin failed for token %q: %v", token, err) + if err := persistLogin(cmd, token); err != nil { + t.Fatalf("persistLogin failed for token %q: %v", token, err) } - if got := readYamlAPIKey(t); got != token { + if got := store.Store[credentialstore.CredentialAPIKey]; got != token { t.Errorf("token mismatch for %q: got %q", token, got) } }) } } -// persistYamlLogin prints success message to stdout -func TestPersistYamlLogin_PrintsSuccessMessage(t *testing.T) { +// persistLogin prints success message to stdout +func TestPersistLogin_PrintsSuccessMessage(t *testing.T) { _ = withTempConfigDir(t) cmd, out, _ := newBufferedCmd() refreshToken := "test-token-456" - if err := persistYamlLogin(cmd, refreshToken); err != nil { - t.Fatalf("persistYamlLogin failed: %v", err) + if err := persistLogin(cmd, refreshToken); err != nil { + t.Fatalf("persistLogin failed: %v", err) } output := out.String() diff --git a/internal/commands/auth_logout.go b/internal/commands/auth_logout.go index b32a4d1e8..a7b686607 100644 --- a/internal/commands/auth_logout.go +++ b/internal/commands/auth_logout.go @@ -1,11 +1,13 @@ package commands import ( + "context" + stderrors "errors" "fmt" "github.com/MakeNowJust/heredoc" - "github.com/checkmarx/ast-cli/internal/params" - "github.com/checkmarx/ast-cli/internal/wrappers/configuration" + "github.com/checkmarx/ast-cli/internal/credentialstore" + "github.com/checkmarx/ast-cli/internal/logger" "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -24,16 +26,19 @@ func newAuthLogoutCommand() *cobra.Command { } } -// runAuthLogout clears the cx_apikey field in the yaml config file. The -// client-credentials and env-provided credentials are intentionally left alone. +// runAuthLogout clears the stored api-key credential (keyring and any leftover +// plaintext config-file entry). The client-credentials and env-provided credentials are +// intentionally left alone. func runAuthLogout(cmd *cobra.Command, _ []string) error { - configPath, err := configuration.GetConfigFilePath() - if err != nil { - return errors.Wrap(err, "failed to resolve config file path") - } - if err := configuration.SafeWriteSingleConfigKeyString(configPath, params.AstAPIKey, ""); err != nil { + err := credentialstore.Default().Clear(context.Background(), credentialstore.CredentialAPIKey) + if err != nil && !stderrors.Is(err, credentialstore.ErrNotFound) { return errors.Wrap(err, "failed to clear stored credential") } + if !credentialstore.Default().StoresInConfigFile() { + if removeErr := credentialstore.Default().RemoveConfigFileEntry(credentialstore.CredentialAPIKey); removeErr != nil { + logger.PrintIfVerbose(fmt.Sprintf("failed to remove old credential from config file: %v", removeErr)) + } + } _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Successfully logged out of Checkmarx One server!") return nil } diff --git a/internal/commands/chat-kics_test.go b/internal/commands/chat-kics_test.go index c110cee33..ae05d8c1c 100644 --- a/internal/commands/chat-kics_test.go +++ b/internal/commands/chat-kics_test.go @@ -6,14 +6,15 @@ import ( "strings" "testing" - "github.com/checkmarx/ast-cli/internal/params" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/wrappers" "github.com/checkmarx/ast-cli/internal/wrappers/mock" "github.com/google/uuid" - "github.com/spf13/viper" "gotest.tools/assert" ) +const testAPIKeyValue = "SomeKey" + func TestChatKicsHelp(t *testing.T) { execCmdNilAssertion(t, "help", "chat", "kics") } @@ -80,8 +81,8 @@ func TestChatKicsAzureAICorrectResponse(t *testing.T) { Value: "azureai", }, } - origAPIKey := viper.GetString(params.AstAPIKey) - viper.Set(params.AstAPIKey, "SomeKey") + store := swapCredentialResolver(t) + store.Store[credentialstore.CredentialAPIKey] = testAPIKeyValue buffer, err := executeRedirectedTestCommand("chat", "kics", "--conversation-id", uuid.New().String(), @@ -96,7 +97,6 @@ func TestChatKicsAzureAICorrectResponse(t *testing.T) { s := strings.ToLower(string(output)) mock.TenantConfiguration = []*wrappers.TenantConfigurationResponse{} - viper.Set(params.AstAPIKey, origAPIKey) assert.Assert(t, strings.Contains(s, "mock message from securecall"), s) } @@ -116,8 +116,8 @@ func TestChatKicsCheckmarxAICorrectResponse(t *testing.T) { Value: "checkmarxai", }, } - origAPIKey := viper.GetString(params.AstAPIKey) - viper.Set(params.AstAPIKey, "SomeKey") + store2 := swapCredentialResolver(t) + store2.Store[credentialstore.CredentialAPIKey] = testAPIKeyValue buffer, err := executeRedirectedTestCommand("chat", "kics", "--conversation-id", uuid.New().String(), @@ -133,7 +133,6 @@ func TestChatKicsCheckmarxAICorrectResponse(t *testing.T) { s := strings.ToLower(string(output)) mock.TenantConfiguration = []*wrappers.TenantConfigurationResponse{} - viper.Set(params.AstAPIKey, origAPIKey) assert.Assert(t, strings.Contains(s, "mock message from securecall"), s) } diff --git a/internal/commands/chat-sast.go b/internal/commands/chat-sast.go index 37f881630..cbad07744 100644 --- a/internal/commands/chat-sast.go +++ b/internal/commands/chat-sast.go @@ -11,6 +11,7 @@ import ( "github.com/Checkmarx/gen-ai-wrapper/pkg/role" "github.com/Checkmarx/gen-ai-wrapper/pkg/wrapper" "github.com/checkmarx/ast-cli/internal/commands/util/printer" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/logger" "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/wrappers" @@ -202,8 +203,11 @@ func isAiGuidedRemediationEnabled(tenantConfigurationResponses *[]*wrappers.Tena } func isCxOneAPIKeyAvailable() bool { - apiKey := viper.GetString(params.AstAPIKey) - return apiKey != "" + apiKey, err := credentialstore.Resolve(credentialstore.CredentialAPIKey) + if err != nil { + logger.PrintIfVerbose(fmt.Sprintf("chat: resolving cx one api key: %v", err)) + } + return err == nil && apiKey != "" } func isAzureAiGuidedRemediationEnabled(tenantConfigurationResponses *[]*wrappers.TenantConfigurationResponse) bool { diff --git a/internal/commands/root.go b/internal/commands/root.go index d3390e430..4bfd9f4fd 100644 --- a/internal/commands/root.go +++ b/internal/commands/root.go @@ -14,6 +14,7 @@ import ( "github.com/checkmarx/ast-cli/internal/commands/dast" "github.com/checkmarx/ast-cli/internal/commands/util" "github.com/checkmarx/ast-cli/internal/commands/util/printer" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/logger" "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/wrappers/bitbucketserver" @@ -121,6 +122,8 @@ func NewAstCLI( // This monitors and traps situations where "extra/garbage" commands // are passed to Cobra. rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { + seedExplicitCredentials(cmd) + credentialstore.Migrate() CheckPreferredCredentials(cmd) err := extractOptionalFlags(cmd) if err != nil { @@ -144,7 +147,6 @@ func NewAstCLI( } // Link the environment variable to the CLI argument(s). _ = viper.BindPFlag(params.AccessKeyIDConfigKey, rootCmd.PersistentFlags().Lookup(params.AccessKeyIDFlag)) - _ = viper.BindPFlag(params.AccessKeySecretConfigKey, rootCmd.PersistentFlags().Lookup(params.AccessKeySecretFlag)) _ = viper.BindPFlag(params.BaseURIKey, rootCmd.PersistentFlags().Lookup(params.BaseURIFlag)) _ = viper.BindPFlag(params.TenantKey, rootCmd.PersistentFlags().Lookup(params.TenantFlag)) _ = viper.BindPFlag(params.ProxyKey, rootCmd.PersistentFlags().Lookup(params.ProxyFlag)) @@ -155,7 +157,6 @@ func NewAstCLI( _ = viper.BindPFlag(params.ProxyKerberosCcacheKey, rootCmd.PersistentFlags().Lookup(params.KerberosCcacheFlag)) _ = viper.BindPFlag(params.ClientTimeoutKey, rootCmd.PersistentFlags().Lookup(params.TimeoutFlag)) _ = viper.BindPFlag(params.BaseAuthURIKey, rootCmd.PersistentFlags().Lookup(params.BaseAuthURIFlag)) - _ = viper.BindPFlag(params.AstAPIKey, rootCmd.PersistentFlags().Lookup(params.AstAPIKeyFlag)) _ = viper.BindPFlag(params.AgentNameKey, rootCmd.PersistentFlags().Lookup(params.AgentFlag)) _ = viper.BindPFlag(params.OriginKey, rootCmd.PersistentFlags().Lookup(params.OriginFlag)) _ = viper.BindPFlag(params.IgnoreProxyKey, rootCmd.PersistentFlags().Lookup(params.IgnoreProxyFlag)) @@ -296,10 +297,28 @@ func PrintConfiguration() { logger.PrintfIfVerbose("CLI Version: %s", params.Version) logger.PrintIfVerbose("CLI Configuration:") for param := range util.Properties { + if credentialstore.IsSecret(param) { + continue + } logger.PrintIfVerbose(fmt.Sprintf(configFormatString, param, viper.GetString(param))) } } +// seedExplicitCredentials captures secret flag values for this invocation only; +// they are resolved in-memory and never written to viper or the config file. +func seedExplicitCredentials(cmd *cobra.Command) { + if cmd.Flags().Changed(params.AstAPIKeyFlag) { + if value, err := cmd.Flags().GetString(params.AstAPIKeyFlag); err == nil { + credentialstore.SetExplicitCredential(credentialstore.CredentialAPIKey, value) + } + } + if cmd.Flags().Changed(params.AccessKeySecretFlag) { + if value, err := cmd.Flags().GetString(params.AccessKeySecretFlag); err == nil { + credentialstore.SetExplicitCredential(credentialstore.CredentialClientSecret, value) + } + } +} + func getFilters(cmd *cobra.Command) (map[string]string, error) { filters, _ := cmd.Flags().GetStringSlice(params.FilterFlag) allFilters := make(map[string]string) diff --git a/internal/commands/root_test.go b/internal/commands/root_test.go index 9553f3245..2803ba487 100644 --- a/internal/commands/root_test.go +++ b/internal/commands/root_test.go @@ -6,12 +6,14 @@ import ( "io" "log" "os" + "path/filepath" "reflect" "strings" "testing" "github.com/checkmarx/ast-cli/internal/params" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/wrappers" "github.com/checkmarx/ast-cli/internal/wrappers/mock" "github.com/spf13/viper" @@ -28,9 +30,22 @@ const ( func TestMain(m *testing.M) { log.Println("Commands tests started") - // Run all tests + // Isolate credential storage: unit tests must never touch the real OS + // keyring or the user's actual config file. + testConfigDir, err := os.MkdirTemp("", "cx-test-config") + if err != nil { + log.Fatalf("failed to create test config dir: %v", err) + } + configPath := filepath.Join(testConfigDir, "checkmarxcli.yaml") + if err := os.WriteFile(configPath, nil, 0o600); err != nil { + log.Fatalf("failed to seed test config file: %v", err) + } + _ = os.Setenv(credentialstore.KeyringModeEnvVar, "disabled") + _ = os.Setenv(params.ConfigFilePathEnv, configPath) + credentialstore.ResetForTest() exitVal := m.Run() viper.SetDefault(resolverEnvVar, resolverEnvVarDefault) + _ = os.RemoveAll(testConfigDir) log.Println("Commands tests done") os.Exit(exitVal) } diff --git a/internal/commands/util/configuration.go b/internal/commands/util/configuration.go index 6fa586329..abbec718b 100644 --- a/internal/commands/util/configuration.go +++ b/internal/commands/util/configuration.go @@ -119,10 +119,8 @@ func runSetValue() func(cmd *cobra.Command, args []string) error { propName, _ := cmd.Flags().GetString(propNameFlag) propValue, _ := cmd.Flags().GetString(propValFlag) if Properties[strings.ToLower(propName)] { - configuration.SetConfigProperty(propName, propValue) - } else { - return errors.Errorf("%s: unknown property or bad value", failedSettingProp) + return configuration.SetConfigProperty(propName, propValue) } - return nil + return errors.Errorf("%s: unknown property or bad value", failedSettingProp) } } diff --git a/internal/commands/util/configuration_test.go b/internal/commands/util/configuration_test.go index 560cd84f6..3c97e7972 100644 --- a/internal/commands/util/configuration_test.go +++ b/internal/commands/util/configuration_test.go @@ -39,6 +39,7 @@ func TestNewConfigCommand(t *testing.T) { func TestGetConfigFilePath_CheckmarxConfigFileExists_Success(t *testing.T) { want := ".checkmarx/checkmarxcli.yaml" + t.Setenv(params.ConfigFilePathEnv, "") got, err := configuration.GetConfigFilePath() if err != nil { diff --git a/internal/commands/util/env.go b/internal/commands/util/env.go index d3e4b167b..efb90989b 100644 --- a/internal/commands/util/env.go +++ b/internal/commands/util/env.go @@ -2,14 +2,21 @@ package util import ( "fmt" - "os" "github.com/MakeNowJust/heredoc" + "github.com/checkmarx/ast-cli/internal/credentialstore" + "github.com/checkmarx/ast-cli/internal/params" + "github.com/checkmarx/ast-cli/internal/wrappers/configuration" "github.com/spf13/cobra" + "github.com/spf13/viper" ) const formatString = "%30v: %s\n" +// proxyEnvName is used because the proxy is bound outside EnvVarsBinds. +const proxyEnvName = params.ProxyEnv + +// NewEnvCheckCommand returns the `cx utils env` command. func NewEnvCheckCommand() *cobra.Command { cmd := &cobra.Command{ Use: "env", @@ -31,12 +38,64 @@ func NewEnvCheckCommand() *cobra.Command { return cmd } +type envBinding struct { + key string + env string +} + +// configurableEnvBindings maps each property in Properties to its uppercase +// environment variable name, in declaration order. Secret properties are +// appended explicitly: they are no longer bound to environment variables but +// remain listed (obfuscated) configuration entries. +// extraEnvBindings accounts for the apikey/client-secret/proxy bindings +// appended below, which live outside params.EnvVarsBinds. +const extraEnvBindings = 3 + +func configurableEnvBindings() []envBinding { + bindings := make([]envBinding, 0, len(Properties)+extraEnvBindings) + for _, bind := range params.EnvVarsBinds { + if Properties[bind.Key] { + bindings = append(bindings, envBinding{key: bind.Key, env: bind.Env}) + } + } + for _, bind := range []envBinding{ + {key: params.AstAPIKey, env: params.AstAPIKeyEnv}, + {key: params.AccessKeySecretConfigKey, env: params.AccessKeySecretEnv}, + } { + if Properties[bind.key] { + bindings = append(bindings, bind) + } + } + if Properties[params.ProxyKey] { + bindings = append(bindings, envBinding{key: params.ProxyKey, env: proxyEnvName}) + } + return bindings +} + func runEnvChecks() func(cmd *cobra.Command, args []string) error { return func(cmd *cobra.Command, args []string) error { fmt.Printf("\nDetected Environment Variables:\n\n") - for param := range Properties { - fmt.Printf(formatString, param, os.Getenv(param)) + for _, bind := range configurableEnvBindings() { + value := effectivePropertyValue(bind.key) + if _, isSecret := credentialstore.CredentialForViperKey(bind.key); isSecret { + value = configuration.ObfuscateString(value) + } + fmt.Printf(formatString, bind.env, value) } return nil } } + +// effectivePropertyValue returns the current value of a configuration +// property: secrets resolve through the credential store, the rest via viper. +func effectivePropertyValue(viperKey string) string { + credentialName, ok := credentialstore.CredentialForViperKey(viperKey) + if !ok { + return viper.GetString(viperKey) + } + value, err := credentialstore.Resolve(credentialName) + if err != nil { + return "" + } + return value +} diff --git a/internal/commands/util/env_test.go b/internal/commands/util/env_test.go index 7bce20867..077a4ecc0 100644 --- a/internal/commands/util/env_test.go +++ b/internal/commands/util/env_test.go @@ -1,11 +1,32 @@ package util import ( + "bytes" + "fmt" + "io" + "os" + "path/filepath" + "strings" "testing" + "github.com/checkmarx/ast-cli/internal/credentialstore" + "github.com/checkmarx/ast-cli/internal/params" + "github.com/checkmarx/ast-cli/internal/wrappers/mock" + "github.com/spf13/viper" "gotest.tools/assert" ) +// swapCredentialResolver binds a mock-backed resolver so secret lookups never +// reach the real OS keyring. +func swapCredentialResolver(t *testing.T) *mock.CredentialStoreMock { + t.Helper() + store := mock.NewCredentialStoreMock() + path := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + credentialstore.SetDefaultResolverForTest(credentialstore.NewResolver(path, credentialstore.PolicyAuto, store)) + t.Cleanup(credentialstore.ResetForTest) + return store +} + func TestNewEnvCheckCommand(t *testing.T) { cmd := NewEnvCheckCommand() assert.Assert(t, cmd != nil, "Env check command must exist") @@ -13,3 +34,61 @@ func TestNewEnvCheckCommand(t *testing.T) { err := cmd.Execute() assert.NilError(t, err, "Env check command should run with no errors") } + +func captureEnvCheckOutput(t *testing.T, fn func()) string { + t.Helper() + + original := os.Stdout + r, w, err := os.Pipe() + assert.NilError(t, err) + os.Stdout = w + + fn() + + assert.NilError(t, w.Close()) + os.Stdout = original + + var buf bytes.Buffer + _, err = io.Copy(&buf, r) + assert.NilError(t, err) + return buf.String() +} + +func TestRunEnvChecksShowsEffectiveValuesAndRedactsSecrets(t *testing.T) { + defer viper.Reset() + + store := swapCredentialResolver(t) + store.Store[credentialstore.CredentialAPIKey] = "supersecretvalue123" + store.Store[credentialstore.CredentialClientSecret] = "topsecretabcd" + viper.Set(params.AccessKeyIDConfigKey, "plain-client-id") + viper.Set(params.BaseURIKey, "https://example.api.test") + + output := captureEnvCheckOutput(t, func() { + err := runEnvChecks()(nil, nil) + assert.NilError(t, err) + }) + + assert.Assert(t, strings.Contains(output, fmt.Sprintf("%30v: %s\n", "CX_APIKEY", "******e123")), + "CX_APIKEY must be printed obfuscated, got:\n%s", output) + assert.Assert(t, strings.Contains(output, fmt.Sprintf("%30v: %s\n", "CX_CLIENT_SECRET", "******abcd")), + "CX_CLIENT_SECRET must be printed obfuscated, got:\n%s", output) + assert.Assert(t, !strings.Contains(output, "supersecretvalue123"), "plaintext api key leaked:\n%s", output) + assert.Assert(t, !strings.Contains(output, "topsecretabcd"), "plaintext client secret leaked:\n%s", output) + + assert.Assert(t, strings.Contains(output, fmt.Sprintf("%30v: %s\n", "CX_CLIENT_ID", "plain-client-id"))) + assert.Assert(t, strings.Contains(output, fmt.Sprintf("%30v: %s\n", "CX_BASE_URI", "https://example.api.test"))) +} + +func TestRunEnvChecksOutputIsDeterministic(t *testing.T) { + swapCredentialResolver(t) + + first := captureEnvCheckOutput(t, func() { + err := runEnvChecks()(nil, nil) + assert.NilError(t, err) + }) + second := captureEnvCheckOutput(t, func() { + err := runEnvChecks()(nil, nil) + assert.NilError(t, err) + }) + assert.Equal(t, first, second, "utils env output must be deterministic across runs") +} diff --git a/internal/commands/util/keyring_masking_test.go b/internal/commands/util/keyring_masking_test.go new file mode 100644 index 000000000..289c67a2c --- /dev/null +++ b/internal/commands/util/keyring_masking_test.go @@ -0,0 +1,47 @@ +package util + +import ( + "errors" + "log" + "strings" + "testing" + + "github.com/checkmarx/ast-cli/internal/credentialstore" + "github.com/checkmarx/ast-cli/internal/logger" + "gotest.tools/assert" +) + +// Resolved credentials are registered with the logger sanitizer at the +// resolver choke point, so debug request dumps never print them even though +// they no longer live in viper. +func TestResolvedSecretIsMaskedInLogs(t *testing.T) { + store := swapCredentialResolver(t) + const secret = "qa-secret-abcdef123456" + store.Store[credentialstore.CredentialAPIKey] = secret + + got, err := credentialstore.Resolve(credentialstore.CredentialAPIKey) + assert.NilError(t, err) + assert.Equal(t, secret, got) + + var buf strings.Builder + original := log.Writer() + log.SetOutput(&buf) + defer log.SetOutput(original) + + logger.Print("grant_type=refresh_token&client_id=qa&refresh_token=" + secret) + + assert.Assert(t, !strings.Contains(buf.String(), secret), "plaintext secret leaked to logs: %s", buf.String()) + assert.Assert(t, strings.Contains(buf.String(), "***"), "expected masked log output: %s", buf.String()) +} + +// A failed secret write must fail the command (fail-hard), not print and +// pretend success. +func TestConfigureSetSecretWriteFailureFailsCommand(t *testing.T) { + store := swapCredentialResolver(t) + store.SetErr = errors.New("keyring write failed") + + err := executeTestCommand(NewConfigCommand(), "set", "--prop-name", "cx_apikey", "--prop-value", "whatever") + + assert.Assert(t, err != nil, "expected non-nil error on keyring write failure") + assert.ErrorContains(t, err, "storing cx_apikey") +} diff --git a/internal/commands/util/main_test.go b/internal/commands/util/main_test.go new file mode 100644 index 000000000..ae91867f1 --- /dev/null +++ b/internal/commands/util/main_test.go @@ -0,0 +1,29 @@ +package util + +import ( + "log" + "os" + "path/filepath" + "testing" + + "github.com/checkmarx/ast-cli/internal/credentialstore" + "github.com/checkmarx/ast-cli/internal/params" +) + +// TestMain isolates configuration tests from the user's real config file. +func TestMain(m *testing.M) { + testConfigDir, err := os.MkdirTemp("", "cx-util-test-config") + if err != nil { + log.Fatalf("failed to create test config dir: %v", err) + } + configPath := filepath.Join(testConfigDir, "checkmarxcli.yaml") + if err := os.WriteFile(configPath, nil, 0o600); err != nil { + log.Fatalf("failed to seed test config file: %v", err) + } + _ = os.Setenv(credentialstore.KeyringModeEnvVar, "disabled") + _ = os.Setenv(params.ConfigFilePathEnv, configPath) + credentialstore.ResetForTest() + exitVal := m.Run() + _ = os.RemoveAll(testConfigDir) + os.Exit(exitVal) +} diff --git a/internal/configfile/configfile.go b/internal/configfile/configfile.go new file mode 100644 index 000000000..3bc51d0b1 --- /dev/null +++ b/internal/configfile/configfile.go @@ -0,0 +1,126 @@ +// Package configfile owns reading and writing the CLI's flat YAML +// configuration file. All mutations happen under a file lock so concurrent +// CLI processes cannot corrupt the file. +package configfile + +import ( + "fmt" + "io" + "os" + + "github.com/gofrs/flock" + "gopkg.in/yaml.v3" +) + +const ( + lockFileSuffix = ".lock" + // ownerOnlyFilePerm keeps credential-bearing config files private on Unix; + // Windows stores the equivalent in ACLs instead of POSIX modes. + ownerOnlyFilePerm = 0o600 +) + +// Load reads the configuration map. A missing or empty file yields an empty +// map, which callers treat as "not configured yet". +func Load(path string) (map[string]interface{}, error) { + config := make(map[string]interface{}) + file, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + return config, nil + } + return nil, fmt.Errorf("reading config file: %w", err) + } + defer func() { + _ = file.Close() + }() + + if err := yaml.NewDecoder(file).Decode(&config); err != nil && err != io.EOF { + // A genuinely corrupt file must error with a nil map so callers can + // never mistake partial data for a valid config. + return nil, fmt.Errorf("error decoding YAML: %w", err) + } + return config, nil +} + +// Save writes the configuration map atomically: the content is written to a +// temporary sibling restricted to owner-only (0600) and renamed over the +// target. This keeps concurrent readers from seeing a truncated file and +// stops repeated writes from widening an existing file's permissions. +func Save(path string, config map[string]interface{}) error { + tmpPath := path + ".tmp" + file, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, ownerOnlyFilePerm) + if err != nil { + return fmt.Errorf("opening config file for writing: %w", err) + } + + encoder := yaml.NewEncoder(file) + encodeErr := encoder.Encode(config) + closeErr := encoder.Close() + closeFileErr := file.Close() + if encodeErr != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("encoding YAML: %w", encodeErr) + } + if closeErr != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("writing config file: %w", closeErr) + } + if closeFileErr != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("writing config file: %w", closeFileErr) + } + if err := os.Rename(tmpPath, path); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("replacing config file: %w", err) + } + return nil +} + +// RemoveKey removes key from the config file under a file lock. Removing an +// absent key is a no-op. +func RemoveKey(path, key string) error { + fileLock := flock.New(path + lockFileSuffix) + locked, err := fileLock.TryLock() + if err != nil { + return fmt.Errorf("locking config file: %w", err) + } + if !locked { + return fmt.Errorf("config file lock is held by another process") + } + defer func() { + _ = fileLock.Unlock() + }() + + config, err := Load(path) + if err != nil { + return err + } + if _, ok := config[key]; !ok { + return nil + } + delete(config, key) + return Save(path, config) +} + +// SetKey assigns key to value in the config file under a file lock. +// SafeWriteSingleConfigKey-style helpers build on it. +func SetKey(path, key string, value interface{}) error { + fileLock := flock.New(path + lockFileSuffix) + locked, err := fileLock.TryLock() + if err != nil { + return fmt.Errorf("locking config file: %w", err) + } + if !locked { + return fmt.Errorf("config file lock is held by another process") + } + defer func() { + _ = fileLock.Unlock() + }() + + config, err := Load(path) + if err != nil { + return err + } + config[key] = value + return Save(path, config) +} diff --git a/internal/configfile/configfile_test.go b/internal/configfile/configfile_test.go new file mode 100644 index 000000000..ed513c9f0 --- /dev/null +++ b/internal/configfile/configfile_test.go @@ -0,0 +1,170 @@ +package configfile + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/gofrs/flock" + "github.com/stretchr/testify/assert" +) + +func TestLoadMissingFileReturnsEmptyMap(t *testing.T) { + config, err := Load(filepath.Join(t.TempDir(), "absent.yaml")) + assert.NoError(t, err) + assert.Empty(t, config) +} + +func TestLoadZeroByteFileReturnsEmptyMap(t *testing.T) { + path := filepath.Join(t.TempDir(), "empty.yaml") + assert.NoError(t, os.WriteFile(path, nil, 0o600)) + + config, err := Load(path) + assert.NoError(t, err) + assert.Empty(t, config) +} + +func TestLoadCommentOnlyFileReturnsEmptyMap(t *testing.T) { + path := filepath.Join(t.TempDir(), "comments.yaml") + assert.NoError(t, os.WriteFile(path, []byte("# nothing but a comment\n"), 0o600)) + + config, err := Load(path) + assert.NoError(t, err) + assert.Empty(t, config) +} + +func TestLoadCorruptYamlReturnsNilMapAndError(t *testing.T) { + path := filepath.Join(t.TempDir(), "corrupt.yaml") + assert.NoError(t, os.WriteFile(path, []byte("foo: [bar"), 0o600)) + + config, err := Load(path) + assert.Error(t, err) + assert.Contains(t, err.Error(), "error decoding YAML") + assert.Nil(t, config) +} + +func TestSetKeyRoundTripAndOverwrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yaml") + + assert.NoError(t, SetKey(path, "cx_apikey", "first")) + config, err := Load(path) + assert.NoError(t, err) + assert.Equal(t, "first", config["cx_apikey"]) + + assert.NoError(t, SetKey(path, "cx_apikey", "second")) + assert.NoError(t, SetKey(path, "cx_tenant", "qa")) + config, err = Load(path) + assert.NoError(t, err) + assert.Equal(t, "second", config["cx_apikey"]) + assert.Equal(t, "qa", config["cx_tenant"]) +} + +func TestRemoveKeyRemovesOnlyTargetKey(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yaml") + assert.NoError(t, SetKey(path, "cx_apikey", "gone")) + assert.NoError(t, SetKey(path, "cx_base_uri", "https://keep")) + + assert.NoError(t, RemoveKey(path, "cx_apikey")) + config, err := Load(path) + assert.NoError(t, err) + assert.NotContains(t, config, "cx_apikey") + assert.Equal(t, "https://keep", config["cx_base_uri"]) +} + +func TestRemoveKeyAbsentIsNoOp(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yaml") + assert.NoError(t, SetKey(path, "cx_branch", "main")) + + assert.NoError(t, RemoveKey(path, "cx_apikey")) + config, err := Load(path) + assert.NoError(t, err) + assert.Equal(t, "main", config["cx_branch"]) +} + +// Save must not widen permissions on an existing file and must leave no +// temporary file behind. +func TestSavePreservesPermissionsAndCleansTempFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + assert.NoError(t, os.WriteFile(path, []byte("cx_apikey: original\n"), 0o600)) + + assert.NoError(t, Save(path, map[string]interface{}{"cx_apikey": "updated"})) + + info, err := os.Stat(path) + assert.NoError(t, err) + if runtime.GOOS != "windows" { + // Windows emulates the POSIX read/write bits (any writable file + // reports 0666), so owner-only persistence is only assertable on Unix. + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "save must keep owner-only permissions") + } + + entries, err := os.ReadDir(dir) + assert.NoError(t, err) + for _, entry := range entries { + assert.NotContains(t, entry.Name(), ".tmp") + } +} + +// Renaming the temp file over an existing directory must fail and clean up +// the temp file rather than leaving it behind. +func TestSaveRenameOntoDirectoryFailsAndCleansTemp(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + assert.NoError(t, os.Mkdir(path, 0o700)) + + err := Save(path, map[string]interface{}{"k": "v"}) + assert.Error(t, err) + assert.Contains(t, err.Error(), "replacing config file") + + entries, err := os.ReadDir(dir) + assert.NoError(t, err) + assert.Len(t, entries, 1) + assert.True(t, entries[0].IsDir()) +} + +func TestLoadDirectoryPathReturnsError(t *testing.T) { + _, err := Load(t.TempDir()) + assert.Error(t, err) +} + +func TestSaveInvalidTargetPathReturnsErrorAndLeavesNoTemp(t *testing.T) { + dir := t.TempDir() + bad := filepath.Join(dir, "no-such-dir", "config.yaml") + assert.Error(t, Save(bad, map[string]interface{}{"k": "v"})) + entries, err := os.ReadDir(dir) + assert.NoError(t, err) + assert.Empty(t, entries) +} + +func TestSetKeyMissingParentDirFails(t *testing.T) { + bad := filepath.Join(t.TempDir(), "no-such-dir", "config.yaml") + assert.Error(t, SetKey(bad, "k", "v")) +} + +func TestRemoveKeyMissingParentDirFails(t *testing.T) { + bad := filepath.Join(t.TempDir(), "no-such-dir", "config.yaml") + assert.Error(t, RemoveKey(bad, "k")) +} + +func TestSetKeyLockHeldByOtherProcessFails(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yaml") + lock := flock.New(path + ".lock") + locked, lockErr := lock.TryLock() + assert.NoError(t, lockErr) + assert.True(t, locked) + t.Cleanup(func() { _ = lock.Unlock() }) + + assert.ErrorContains(t, SetKey(path, "k", "v"), "held by another process") +} + +func TestRemoveKeyLockHeldByOtherProcessFails(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yaml") + lock := flock.New(path + ".lock") + locked, lockErr := lock.TryLock() + assert.NoError(t, lockErr) + assert.True(t, locked) + t.Cleanup(func() { _ = lock.Unlock() }) + + assert.ErrorContains(t, RemoveKey(path, "k"), "held by another process") +} diff --git a/internal/credentialstore/errors.go b/internal/credentialstore/errors.go new file mode 100644 index 000000000..47a2601c7 --- /dev/null +++ b/internal/credentialstore/errors.go @@ -0,0 +1,24 @@ +// Package credentialstore stores CLI credentials in the OS keyring with a plaintext +// config-file fallback. +// +// Note: unlike the HTTP wrappers, this package exposes package-level seams +// (Default/Resolve/Store/Clear) instead of constructor injection. Credential +// consumers include non-cobra entrypoints (agenthook dispatch, MCP bridge +// polling) where threading a wrapper through every constructor is impractical; +// the testing.go file provides the injection/reset seams this requires. +package credentialstore + +import "errors" + +var ( + // ErrNotFound is returned when a credential does not exist in any layer. + ErrNotFound = errors.New("credential not found") + // ErrKeyringUnavailable is returned when the OS keyring cannot be reached. + ErrKeyringUnavailable = errors.New("credential keyring unavailable") + // ErrAccessDenied is returned when the keyring refuses access to a credential. + ErrAccessDenied = errors.New("access to credential denied") + // ErrInvalidName is returned for an unknown credential name. + ErrInvalidName = errors.New("unknown credential name") + // ErrKeyringFailure wraps keyring errors that do not fit a known category. + ErrKeyringFailure = errors.New("credential keyring failure") +) diff --git a/internal/credentialstore/keyring.go b/internal/credentialstore/keyring.go new file mode 100644 index 000000000..2ace0a3cc --- /dev/null +++ b/internal/credentialstore/keyring.go @@ -0,0 +1,103 @@ +package credentialstore + +import ( + "context" + "errors" + "fmt" + "strings" + "time" + + keyring "github.com/zalando/go-keyring" +) + +// keyringOpTimeout bounds each blocking keyring call. Generous enough for +// interactive keychain prompts (macOS re-prompts after every binary upgrade +// when access ACLs reset) while still failing closed eventually. +const keyringOpTimeout = 30 * time.Second + +var ( + deniedIndicators = []string{"denied", "authentication"} + unavailableIndicators = []string{"not available", "unavailable", "dbus", "failed to connect", "timeout"} +) + +type osKeyring struct{} + +func (osKeyring) Get(ctx context.Context, service, account string) (string, error) { + type result struct { + value string + err error + } + ch := make(chan result, 1) + ctx, cancel := context.WithTimeout(ctx, keyringOpTimeout) + defer cancel() + go func() { + value, err := keyring.Get(service, account) + ch <- result{value: value, err: err} + }() + select { + case res := <-ch: + if errors.Is(res.err, keyring.ErrNotFound) || (res.err == nil && res.value == "") { + return "", ErrNotFound + } + if res.err != nil { + return "", mapKeyringError(res.err) + } + return res.value, nil + case <-ctx.Done(): + return "", fmt.Errorf("%w: %w", ErrKeyringUnavailable, ctx.Err()) + } +} + +func (osKeyring) Set(ctx context.Context, service, account, value string) error { + return runKeyringWrite(ctx, func() error { + return keyring.Set(service, account, value) + }) +} + +func (osKeyring) Delete(ctx context.Context, service, account string) error { + return runKeyringWrite(ctx, func() error { + return keyring.Delete(service, account) + }) +} + +func runKeyringWrite(ctx context.Context, op func() error) error { + errCh := make(chan error, 1) + ctx, cancel := context.WithTimeout(ctx, keyringOpTimeout) + defer cancel() + go func() { + errCh <- op() + }() + select { + case err := <-errCh: + return mapKeyringError(err) + case <-ctx.Done(): + return fmt.Errorf("%w: %w", ErrKeyringUnavailable, ctx.Err()) + } +} + +func mapKeyringError(err error) error { + if err == nil { + return nil + } + if errors.Is(err, keyring.ErrNotFound) { + return ErrNotFound + } + message := strings.ToLower(err.Error()) + switch { + case containsAny(message, deniedIndicators): + return fmt.Errorf("%w: %w", ErrAccessDenied, err) + case containsAny(message, unavailableIndicators): + return fmt.Errorf("%w: %w", ErrKeyringUnavailable, err) + default: + return fmt.Errorf("%w: %w", ErrKeyringFailure, err) + } +} + +func containsAny(message string, indicators []string) bool { + for _, indicator := range indicators { + if strings.Contains(message, indicator) { + return true + } + } + return false +} diff --git a/internal/credentialstore/keyring_test.go b/internal/credentialstore/keyring_test.go new file mode 100644 index 000000000..404d06473 --- /dev/null +++ b/internal/credentialstore/keyring_test.go @@ -0,0 +1,114 @@ +package credentialstore + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + keyring "github.com/zalando/go-keyring" +) + +func TestOSKeyringRoundtrip(t *testing.T) { + keyring.MockInit() + provider := osKeyring{} + ctx := context.Background() + service := fmt.Sprintf("ast-cli-test-%s", t.Name()) + account := "account" + + _, err := provider.Get(ctx, service, account) + assert.ErrorIs(t, err, ErrNotFound) + + assert.NoError(t, provider.Set(ctx, service, account, "secret-value")) + got, err := provider.Get(ctx, service, account) + assert.NoError(t, err) + assert.Equal(t, "secret-value", got) + + assert.NoError(t, provider.Delete(ctx, service, account)) + _, err = provider.Get(ctx, service, account) + assert.ErrorIs(t, err, ErrNotFound) + assert.ErrorIs(t, provider.Delete(ctx, service, account), ErrNotFound) +} + +func TestOSKeyringEmptyValueTreatedAsNotFound(t *testing.T) { + keyring.MockInit() + provider := osKeyring{} + ctx := context.Background() + service := fmt.Sprintf("ast-cli-test-%s", t.Name()) + account := "empty" + + keyringSetErr := keyring.Set(service, account, "") + assert.NoError(t, keyringSetErr) + _, err := provider.Get(ctx, service, account) + assert.ErrorIs(t, err, ErrNotFound) +} + +func TestOSKeyringErrorClassification(t *testing.T) { + cases := []struct { + raw error + want error + }{ + {raw: errors.New("dbus: failed to connect to socket"), want: ErrKeyringUnavailable}, + {raw: errors.New("service not available"), want: ErrKeyringUnavailable}, + {raw: errors.New("access denied by policy"), want: ErrAccessDenied}, + {raw: errors.New("authentication failed"), want: ErrAccessDenied}, + {raw: errors.New("something entirely unexpected"), want: ErrKeyringFailure}, + } + for _, tc := range cases { + t.Run(tc.raw.Error(), func(t *testing.T) { + keyring.MockInitWithError(tc.raw) + t.Cleanup(keyring.MockInit) + provider := osKeyring{} + ctx := context.Background() + + _, err := provider.Get(ctx, "svc", "acct") + assert.ErrorIs(t, err, tc.want) + assert.NotErrorIs(t, err, ErrNotFound) + + err = provider.Set(ctx, "svc", "acct", "value") + assert.ErrorIs(t, err, tc.want) + }) + } +} + +// TestKeyringBackendContextCanceled exercises the guard's ctx branch of every +// operation without waiting on the fixed op timeout: a pre-canceled context +// must surface as ErrKeyringUnavailable wrapping the context error. +func TestKeyringBackendContextCanceled(t *testing.T) { + keyring.MockInit() + provider := osKeyring{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := provider.Get(ctx, "svc", "acct") + assert.ErrorIs(t, err, ErrKeyringUnavailable) + assert.ErrorIs(t, err, context.Canceled) + + err = provider.Set(ctx, "svc", "acct", "value") + assert.ErrorIs(t, err, ErrKeyringUnavailable) + assert.ErrorIs(t, err, context.Canceled) + + err = provider.Delete(ctx, "svc", "acct") + assert.ErrorIs(t, err, ErrKeyringUnavailable) + assert.ErrorIs(t, err, context.Canceled) +} + +func TestOSKeyringClassificationSweepsAllIndicators(t *testing.T) { + cases := []struct { + raw error + want error + }{ + {raw: errors.New("service not available"), want: ErrKeyringUnavailable}, + {raw: errors.New("keyring unavailable"), want: ErrKeyringUnavailable}, + {raw: errors.New("connection timeout"), want: ErrKeyringUnavailable}, + {raw: errors.New("failed to connect to bus"), want: ErrKeyringUnavailable}, + } + for _, tc := range cases { + keyring.MockInitWithError(tc.raw) + t.Cleanup(keyring.MockInit) + provider := osKeyring{} + err := provider.Set(context.Background(), "svc", "acct", "v") + assert.ErrorIs(t, err, tc.want) + } +} diff --git a/internal/credentialstore/migration.go b/internal/credentialstore/migration.go new file mode 100644 index 000000000..ddd10e2d3 --- /dev/null +++ b/internal/credentialstore/migration.go @@ -0,0 +1,85 @@ +package credentialstore + +import ( + "context" + "errors" + + "github.com/checkmarx/ast-cli/internal/configfile" + "github.com/checkmarx/ast-cli/internal/logger" + "github.com/checkmarx/ast-cli/internal/params" +) + +// RunMigration moves plaintext config-file credentials into the keyring and removes them from +// the file. It exits cheaply when the active config file holds neither secret, +// so per-invocation calls are a single file read on the steady state. +func (r *Resolver) RunMigration(ctx context.Context) { + if r.policy == PolicyDisabled { + return + } + config, err := configfile.Load(r.filePath) + if err != nil || len(config) == 0 { + return + } + for _, name := range []string{CredentialAPIKey, CredentialClientSecret} { + if stringValue(config[viperKeyFor(name)]) != "" { + r.migrateOne(ctx, name, config) + } + } +} + +// Migrate runs migration on the default resolver. +func Migrate() { + Default().RunMigration(context.Background()) +} + +func (r *Resolver) migrateOne(ctx context.Context, credentialName string, config map[string]interface{}) { + yamlValue := stringValue(config[viperKeyFor(credentialName)]) + if yamlValue == "" { + return + } + current, err := r.store.Get(ctx, credentialName) + switch { + case errors.Is(err, ErrNotFound): + r.migrateAndRemove(ctx, credentialName, yamlValue) + case err != nil: + logger.PrintfIfVerbose("credentialstore: skipping migration of %s, keyring unavailable: %v", credentialName, err) + case current == yamlValue: + r.removeConfigFileEntryQuietly(credentialName) + default: + logger.PrintfIfVerbose( + "credentialstore: keyring already holds a different %s; keeping existing config file entry", + credentialName, + ) + } +} + +func (r *Resolver) migrateAndRemove(ctx context.Context, credentialName, value string) { + if err := r.store.Set(ctx, credentialName, value); err != nil { + logger.PrintfIfVerbose("credentialstore: could not store %s in keyring, keeping config file entry: %v", credentialName, err) + return + } + stored, err := r.store.Get(ctx, credentialName) + if err != nil || stored != value { + logger.PrintfIfVerbose("credentialstore: verification of %s failed, keeping config file entry", credentialName) + return + } + r.removeConfigFileEntryQuietly(credentialName) + logger.PrintfIfVerbose("credentialstore: migrated %s to OS keyring", credentialName) +} + +func (r *Resolver) removeConfigFileEntryQuietly(credentialName string) { + if err := configfile.RemoveKey(r.filePath, viperKeyFor(credentialName)); err != nil { + logger.PrintfIfVerbose("credentialstore: could not remove %s from config file: %v", credentialName, err) + } +} + +func viperKeyFor(credentialName string) string { + switch credentialName { + case CredentialAPIKey: + return params.AstAPIKey + case CredentialClientSecret: + return params.AccessKeySecretConfigKey + default: + return "" + } +} diff --git a/internal/credentialstore/migration_test.go b/internal/credentialstore/migration_test.go new file mode 100644 index 000000000..cb75558bf --- /dev/null +++ b/internal/credentialstore/migration_test.go @@ -0,0 +1,276 @@ +package credentialstore + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/checkmarx/ast-cli/internal/configfile" + "github.com/checkmarx/ast-cli/internal/params" + "github.com/gofrs/flock" + "github.com/stretchr/testify/assert" +) + +const migratedAPIKey = "old-api-key" + +// readStoredValueForTest reads a key directly from the config file on disk. +func readStoredValueForTest(t *testing.T, configPath, key string) string { + t.Helper() + config, err := configfile.Load(configPath) + assert.NoError(t, err) + return stringValue(config[key]) +} + +func TestRunMigrationFirstRunMigratesAndRemoves(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\ncx_base_uri: https://keep.example.com\n") + store := newFakeStore() + resolver := NewResolver(configPath, PolicyAuto, store) + + resolver.RunMigration(context.Background()) + + value := readStoredValueForTest(t, configPath, params.AstAPIKey) + assert.Empty(t, value) + stored, err := store.Get(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, migratedAPIKey, stored) + + baseURI := readStoredValueForTest(t, configPath, "cx_base_uri") + assert.Equal(t, "https://keep.example.com", baseURI) +} + +func TestRunMigrationIdempotentSecondRun(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\n") + resolver := NewResolver(configPath, PolicyAuto, newFakeStore()) + + resolver.RunMigration(context.Background()) + resolver.RunMigration(context.Background()) + + content := readStoredValueForTest(t, configPath, params.AstAPIKey) + assert.Empty(t, content) +} + +func TestRunMigrationAlreadyMigratedRemovesOnly(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\n") + store := newFakeStore() + store.values[CredentialAPIKey] = migratedAPIKey + resolver := NewResolver(configPath, PolicyAuto, store) + + resolver.RunMigration(context.Background()) + + value := readStoredValueForTest(t, configPath, params.AstAPIKey) + assert.Empty(t, value) + stored, err := store.Get(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, migratedAPIKey, stored) +} + +func TestRunMigrationConflictKeepsYAMLAndStore(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\n") + store := newFakeStore() + store.values[CredentialAPIKey] = "different-key" + resolver := NewResolver(configPath, PolicyAuto, store) + + resolver.RunMigration(context.Background()) + + value := readStoredValueForTest(t, configPath, params.AstAPIKey) + assert.Equal(t, migratedAPIKey, value) + stored, err := store.Get(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "different-key", stored) +} + +func TestRunMigrationSetFailureKeepsYAML(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\n") + store := newFakeStore() + store.setErr = errors.New("keyring write refused") + resolver := NewResolver(configPath, PolicyAuto, store) + + resolver.RunMigration(context.Background()) + + value := readStoredValueForTest(t, configPath, params.AstAPIKey) + assert.Equal(t, migratedAPIKey, value) +} + +// verifyFailStore reports ErrNotFound on the first Get (so migrateOne routes +// into migrateAndRemove) then fails every subsequent Get, so the +// post-Set verification read in migrateAndRemove observes an error. +type verifyFailStore struct { + gets int +} + +func (s *verifyFailStore) Get(context.Context, string) (string, error) { + s.gets++ + if s.gets == 1 { + return "", ErrNotFound + } + return "", errors.New("keyring read refused") +} + +func (s *verifyFailStore) Set(context.Context, string, string) error { return nil } +func (s *verifyFailStore) Delete(context.Context, string) error { return nil } + +func TestRunMigrationVerificationFailureKeepsYAML(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\n") + resolver := NewResolver(configPath, PolicyAuto, &verifyFailStore{}) + + resolver.RunMigration(context.Background()) + + value := readStoredValueForTest(t, configPath, params.AstAPIKey) + assert.Equal(t, migratedAPIKey, value) +} + +// A config file lock held by another process must not fail migration outright: +// the keyring write already succeeded, so the leftover YAML entry is a +// cosmetic cleanup failure logged verbosely, not a hard error. +func TestRunMigrationConfigFileRemovalLockedIsQuiet(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\n") + lock := flock.New(configPath + ".lock") + locked, lockErr := lock.TryLock() + assert.NoError(t, lockErr) + assert.True(t, locked) + t.Cleanup(func() { _ = lock.Unlock() }) + + resolver := NewResolver(configPath, PolicyAuto, newFakeStore()) + assert.NotPanics(t, func() { resolver.RunMigration(context.Background()) }) + + stored, err := resolver.store.Get(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, migratedAPIKey, stored) +} + +func TestRunMigrationDisabledIsNoOp(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\n") + store := newFakeStore() + resolver := NewResolver(configPath, PolicyDisabled, store) + + resolver.RunMigration(context.Background()) + + value := readStoredValueForTest(t, configPath, params.AstAPIKey) + assert.Equal(t, migratedAPIKey, value) + assert.Equal(t, 0, store.calls()) +} + +type recordingProvider struct { + mu sync.Mutex + accounts []string + values map[string]string +} + +func (r *recordingProvider) Get(_ context.Context, _, account string) (string, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.accounts = append(r.accounts, account) + value, ok := r.values[account] + if !ok { + return "", ErrNotFound + } + return value, nil +} + +func (r *recordingProvider) Set(_ context.Context, _, account, value string) error { + r.mu.Lock() + defer r.mu.Unlock() + if r.values == nil { + r.values = make(map[string]string) + } + r.values[account] = value + return nil +} + +func (r *recordingProvider) Delete(_ context.Context, _, _ string) error { + return nil +} + +func TestMigrationAccountsIsolatedPerConfigFile(t *testing.T) { + pathA := filepath.Join(t.TempDir(), "a", "checkmarxcli.yaml") + pathB := filepath.Join(t.TempDir(), "b", "checkmarxcli.yaml") + canonicalA := CanonicalConfigPath(pathA) + canonicalB := CanonicalConfigPath(pathB) + + assert.NotEqual( + t, + AccountFor(canonicalA, CredentialAPIKey), + AccountFor(canonicalB, CredentialAPIKey), + ) + + backendA := &recordingProvider{} + backendB := &recordingProvider{} + storeA := &keyCredentialStore{canonicalPath: canonicalA, backend: backendA} + storeB := &keyCredentialStore{canonicalPath: canonicalB, backend: backendB} + + _, _ = storeA.Get(context.Background(), CredentialAPIKey) + _, _ = storeB.Get(context.Background(), CredentialAPIKey) + + accountA := backendA.accounts[0] + accountB := backendB.accounts[0] + assert.NotEqual(t, accountA, accountB) + assert.True(t, strings.HasSuffix(accountA, accountSeparator+CredentialAPIKey)) + assert.NotContains(t, accountA, canonicalA) + assert.NotContains(t, accountB, canonicalB) +} + +// A numeric YAML scalar must surface through stringValue's default branch. +func TestResolveNumericPlaintextScalar(t *testing.T) { + dir := t.TempDir() + yamlPath := filepath.Join(dir, "checkmarxcli.yaml") + assert.NoError(t, os.WriteFile(yamlPath, []byte("cx_apikey: 12345\n"), 0o600)) + resolver := NewResolver(yamlPath, PolicyAuto, newFakeStore()) + + value, err := resolver.Resolve(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "12345", value) +} + +func TestViperKeyForUnknownCredentialReturnsEmpty(t *testing.T) { + assert.Equal(t, "", viperKeyFor("unknown")) +} + +// migrateOne is only ever called by RunMigration after it has already +// verified the YAML value is non-empty; this pins that guard as belt-and- +// suspenders for any future direct caller. +func TestMigrateOneEmptyYAMLValueIsNoOp(t *testing.T) { + store := newFakeStore() + resolver := NewResolver(filepath.Join(t.TempDir(), "checkmarxcli.yaml"), PolicyAuto, store) + + resolver.migrateOne(context.Background(), CredentialAPIKey, map[string]interface{}{}) + + assert.Equal(t, 0, store.calls()) +} + +// An unreadable config file (a directory path) must no-op migration quietly. +func TestRunMigrationUnreadableConfigIsNoop(t *testing.T) { + resolver := NewResolver(t.TempDir(), PolicyAuto, newFakeStore()) + assert.NotPanics(t, func() { resolver.RunMigration(context.Background()) }) +} + +// Migrate is the package-level wrapper over Default().RunMigration; this +// pins it to the injected default resolver so it never touches the real +// keyring or the user's actual config file. +func TestMigratePackageLevelWrapper(t *testing.T) { + ResetForTest() + defer ResetForTest() + + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, "cx_apikey: "+migratedAPIKey+"\n") + store := newFakeStore() + SetDefaultResolverForTest(NewResolver(configPath, PolicyAuto, store)) + + Migrate() + + stored, err := store.Get(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, migratedAPIKey, stored) + assert.Empty(t, readStoredValueForTest(t, configPath, params.AstAPIKey)) +} diff --git a/internal/credentialstore/names.go b/internal/credentialstore/names.go new file mode 100644 index 000000000..4f2b5e445 --- /dev/null +++ b/internal/credentialstore/names.go @@ -0,0 +1,46 @@ +package credentialstore + +import ( + "crypto/sha256" + "encoding/hex" + "path/filepath" + "runtime" + "strings" +) + +// KeyringServiceName is the service name used for all keyring entries. +const KeyringServiceName = "checkmarx-ast-cli" + +const ( + accountHashBytes = 16 + accountSeparator = ":" +) + +// CanonicalConfigPath returns an absolute cleaned path, lowercased on Windows +// so casing variants of the same file map to one keyring account. +func CanonicalConfigPath(path string) string { + if path == "" { + return "" + } + abs, err := filepath.Abs(path) + if err != nil { + abs = filepath.Clean(path) + } + return normalizeWindowsPath(abs) +} + +// normalizeWindowsPath lowercases the whole path on Windows (NTFS is +// case-insensitive, including UNC shares) and leaves paths on other +// platforms untouched. +func normalizeWindowsPath(path string) string { + if runtime.GOOS != "windows" { + return path + } + return strings.ToLower(path) +} + +// AccountFor derives the keyring account from the canonical config path and logical name. +func AccountFor(canonicalConfigPath, credentialName string) string { + sum := sha256.Sum256([]byte(canonicalConfigPath)) + return hex.EncodeToString(sum[:accountHashBytes]) + accountSeparator + credentialName +} diff --git a/internal/credentialstore/names_test.go b/internal/credentialstore/names_test.go new file mode 100644 index 000000000..0f2413e34 --- /dev/null +++ b/internal/credentialstore/names_test.go @@ -0,0 +1,102 @@ +package credentialstore + +import ( + "encoding/hex" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/checkmarx/ast-cli/internal/params" + "github.com/stretchr/testify/assert" +) + +func TestCanonicalConfigPathDriveLetterEquivalence(t *testing.T) { + const goosWindows = "windows" + if runtime.GOOS != goosWindows { + t.Skip("drive letter normalization applies to Windows only") + } + upper := CanonicalConfigPath(`C:\Users\TEST\.checkmarx\checkmarxcli.yaml`) + lower := CanonicalConfigPath(`c:\users\test\.checkmarx\checkmarxcli.yaml`) + assert.Equal(t, upper, lower) + assert.True(t, strings.HasPrefix(upper, "c:")) +} + +func TestCanonicalConfigPathEmptyReturnsEmpty(t *testing.T) { + assert.Equal(t, "", CanonicalConfigPath("")) +} + +func TestCanonicalConfigPathRelativeEqualsAbsolute(t *testing.T) { + tmp := t.TempDir() + absolute := CanonicalConfigPath(tmp) + relative := filepath.Join(tmp, "..", filepath.Base(tmp)) + assert.Equal(t, absolute, CanonicalConfigPath(relative)) + assert.Equal(t, absolute, CanonicalConfigPath(absolute)) +} + +func TestAccountForDeterministic(t *testing.T) { + canonical := CanonicalConfigPath(filepath.Join(t.TempDir(), "checkmarxcli.yaml")) + first := AccountFor(canonical, CredentialAPIKey) + second := AccountFor(canonical, CredentialAPIKey) + assert.Equal(t, first, second) +} + +func TestAccountForDiffersAcrossPathsAndNames(t *testing.T) { + pathA := CanonicalConfigPath(filepath.Join(t.TempDir(), "a.yaml")) + pathB := CanonicalConfigPath(filepath.Join(t.TempDir(), "b.yaml")) + assert.NotEqual(t, AccountFor(pathA, CredentialAPIKey), AccountFor(pathB, CredentialAPIKey)) + assert.NotEqual(t, AccountFor(pathA, CredentialAPIKey), AccountFor(pathA, CredentialClientSecret)) +} + +func TestAccountForNeverExposesRawPath(t *testing.T) { + raw := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + canonical := CanonicalConfigPath(raw) + account := AccountFor(canonical, CredentialAPIKey) + assert.NotContains(t, account, canonical) + assert.NotContains(t, account, raw) + parts := strings.Split(account, accountSeparator) + assert.Len(t, parts, 2) + assert.Equal(t, CredentialAPIKey, parts[1]) + decoded, err := hex.DecodeString(parts[0]) + assert.NoError(t, err) + assert.Len(t, decoded, accountHashBytes) +} + +func TestIsValidCredentialName(t *testing.T) { + assert.True(t, IsValidCredentialName(CredentialAPIKey)) + assert.True(t, IsValidCredentialName(CredentialClientSecret)) + assert.False(t, IsValidCredentialName("")) + assert.False(t, IsValidCredentialName("api-key ")) + assert.False(t, IsValidCredentialName("API-KEY")) + assert.False(t, IsValidCredentialName("totally-unrelated")) +} + +func TestIsSecret(t *testing.T) { + assert.True(t, IsSecret(params.AstAPIKey)) + assert.True(t, IsSecret(params.AccessKeySecretConfigKey)) + assert.False(t, IsSecret(params.ConfigFilePathKey)) + assert.False(t, IsSecret("")) +} + +func TestEnvVarForUnknownCredentialReturnsFalse(t *testing.T) { + envVar, ok := envVarFor("unknown") + assert.False(t, ok) + assert.Equal(t, "", envVar) +} + +func TestEnvValueUnknownCredentialReturnsEmpty(t *testing.T) { + assert.Equal(t, "", envValue("unknown")) +} + +func TestCredentialForViperKey(t *testing.T) { + name, ok := CredentialForViperKey(params.AstAPIKey) + assert.True(t, ok) + assert.Equal(t, CredentialAPIKey, name) + + name, ok = CredentialForViperKey(params.AccessKeySecretConfigKey) + assert.True(t, ok) + assert.Equal(t, CredentialClientSecret, name) + + _, ok = CredentialForViperKey(params.ConfigFilePathKey) + assert.False(t, ok) +} diff --git a/internal/credentialstore/policy.go b/internal/credentialstore/policy.go new file mode 100644 index 000000000..f00ad6be1 --- /dev/null +++ b/internal/credentialstore/policy.go @@ -0,0 +1,56 @@ +package credentialstore + +import ( + "fmt" + "os" + + "github.com/checkmarx/ast-cli/internal/logger" +) + +// Policy controls how the resolver combines keyring, environment and YAML layers. +type Policy int + +const ( + // PolicyAuto prefers the keyring and falls back to the config-file layer. + PolicyAuto Policy = iota + // PolicyRequired uses only the keyring, never YAML. + PolicyRequired + // PolicyDisabled ignores the keyring entirely. + PolicyDisabled +) + +// KeyringModeEnvVar selects the keyring policy via environment variable. +const KeyringModeEnvVar = "CX_KEYRING_MODE" + +const ( + policyValueAuto = "auto" + policyValueRequired = "required" + policyValueDisabled = "disabled" +) + +// ParsePolicy converts a raw mode string into a Policy. +func ParsePolicy(raw string) (Policy, error) { + switch raw { + case "", policyValueAuto: + return PolicyAuto, nil + case policyValueRequired: + return PolicyRequired, nil + case policyValueDisabled: + return PolicyDisabled, nil + default: + return PolicyAuto, fmt.Errorf( + "invalid keyring mode %q, valid values: %s, %s, %s", + raw, policyValueAuto, policyValueRequired, policyValueDisabled, + ) + } +} + +// PolicyFromEnv reads KeyringModeEnvVar, falling back to PolicyAuto on invalid values. +func PolicyFromEnv() Policy { + policy, err := ParsePolicy(os.Getenv(KeyringModeEnvVar)) + if err != nil { + logger.PrintfIfVerbose("credentialstore: %v, falling back to auto mode", err) + return PolicyAuto + } + return policy +} diff --git a/internal/credentialstore/policy_test.go b/internal/credentialstore/policy_test.go new file mode 100644 index 000000000..ad8506a7f --- /dev/null +++ b/internal/credentialstore/policy_test.go @@ -0,0 +1,56 @@ +package credentialstore + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParsePolicy(t *testing.T) { + cases := []struct { + raw string + want Policy + wantErr bool + }{ + {raw: "", want: PolicyAuto}, + {raw: "auto", want: PolicyAuto}, + {raw: "required", want: PolicyRequired}, + {raw: "disabled", want: PolicyDisabled}, + {raw: "AUTO", wantErr: true}, + {raw: "bogus", wantErr: true}, + } + for _, tc := range cases { + got, err := ParsePolicy(tc.raw) + if tc.wantErr { + assert.Error(t, err, tc.raw) + continue + } + assert.NoError(t, err, tc.raw) + assert.Equal(t, tc.want, got, tc.raw) + } +} + +func TestParsePolicyInvalidListsValidValues(t *testing.T) { + _, err := ParsePolicy("bogus") + assert.ErrorContains(t, err, policyValueAuto) + assert.ErrorContains(t, err, policyValueRequired) + assert.ErrorContains(t, err, policyValueDisabled) +} + +func TestPolicyFromEnv(t *testing.T) { + t.Setenv(KeyringModeEnvVar, "required") + assert.Equal(t, PolicyRequired, PolicyFromEnv()) + + t.Setenv(KeyringModeEnvVar, "disabled") + assert.Equal(t, PolicyDisabled, PolicyFromEnv()) +} + +func TestPolicyFromEnvUnsetDefaultsToAuto(t *testing.T) { + t.Setenv(KeyringModeEnvVar, "") + assert.Equal(t, PolicyAuto, PolicyFromEnv()) +} + +func TestPolicyFromEnvInvalidFallsBackToAuto(t *testing.T) { + t.Setenv(KeyringModeEnvVar, "sometimes") + assert.Equal(t, PolicyAuto, PolicyFromEnv()) +} diff --git a/internal/credentialstore/resolver.go b/internal/credentialstore/resolver.go new file mode 100644 index 000000000..478ae565d --- /dev/null +++ b/internal/credentialstore/resolver.go @@ -0,0 +1,233 @@ +package credentialstore + +import ( + "context" + "errors" + "fmt" + "os" + "os/user" + "path/filepath" + "sync" + + "github.com/checkmarx/ast-cli/internal/configfile" + "github.com/checkmarx/ast-cli/internal/logger" + "github.com/checkmarx/ast-cli/internal/params" + "github.com/spf13/viper" +) + +const ( + checkmarxDirName = ".checkmarx" + checkmarxFileName = "checkmarxcli.yaml" +) + +// Resolver resolves credential values across explicit, env, keyring and config-file layers. +type Resolver struct { + mu sync.Mutex + canonicalPath string + filePath string + policy Policy + store CredentialStore + explicit map[string]string +} + +// NewResolver builds a resolver for configFilePath; a nil store defaults to the keyring. +func NewResolver(configFilePath string, policy Policy, store CredentialStore) *Resolver { + canonical := CanonicalConfigPath(configFilePath) + if store == nil { + store = NewCredentialStore(canonical) + } + return &Resolver{ + canonicalPath: canonical, + filePath: configFilePath, + policy: policy, + store: store, + explicit: make(map[string]string), + } +} + +// SetExplicit registers an in-process override for a credential. +func (r *Resolver) SetExplicit(credentialName, value string) { + r.mu.Lock() + defer r.mu.Unlock() + r.explicit[credentialName] = value +} + +// Resolve returns the credential value following the configured policy precedence. +// Every non-empty result is registered with the logger's sanitizer: callers may +// embed the value in HTTP bodies or headers that debug mode dumps verbatim. +func (r *Resolver) Resolve(ctx context.Context, credentialName string) (string, error) { + if !IsValidCredentialName(credentialName) { + return "", ErrInvalidName + } + value, err := r.resolve(ctx, credentialName) + if err == nil && value != "" { + logger.RegisterSensitiveValue(value) + } + return value, err +} + +// Store persists a credential following the policy: the OS keyring by +// default, the config-file layer under PolicyDisabled — so writes and reads +// always land in the same layer. +func (r *Resolver) Store(ctx context.Context, credentialName, value string) error { + if !IsValidCredentialName(credentialName) { + return ErrInvalidName + } + if r.policy == PolicyDisabled { + return configfile.SetKey(r.filePath, viperKeyFor(credentialName), value) + } + return r.store.Set(ctx, credentialName, value) +} + +// Clear removes a credential following the policy, mirroring Store. A missing +// credential is reported as ErrNotFound. +func (r *Resolver) Clear(ctx context.Context, credentialName string) error { + if !IsValidCredentialName(credentialName) { + return ErrInvalidName + } + if r.policy == PolicyDisabled { + config, err := configfile.Load(r.filePath) + if err != nil { + return err + } + if stringValue(config[viperKeyFor(credentialName)]) == "" { + return ErrNotFound + } + return configfile.RemoveKey(r.filePath, viperKeyFor(credentialName)) + } + return r.store.Delete(ctx, credentialName) +} + +// StoresInConfigFile reports whether this policy persists credentials in the +// config file (PolicyDisabled) instead of the OS keyring. Callers use it to +// decide whether a leftover plaintext entry must be removed after a keyring write. +func (r *Resolver) StoresInConfigFile() bool { + return r.policy == PolicyDisabled +} + +// The explicit layer distinguishes "flag passed" from "flag absent" by map +// presence, so --apikey "" wins over env/keyring/config for this invocation +// (matching pre-keyring viper flag-over-env precedence) without ever writing +// through to the persisted credential — Store/Clear are separate calls. +// The environment layer has no such distinction: an empty env var is +// indistinguishable from an unset one, so it is treated as absent. Name +// validity is checked by the exported entry points. +func (r *Resolver) resolve(ctx context.Context, credentialName string) (string, error) { + if value, ok := r.explicitValue(credentialName); ok { + return value, nil + } + if value := envValue(credentialName); value != "" { + return value, nil + } + switch r.policy { + case PolicyDisabled: + return r.resolveFromConfigFile(credentialName) + case PolicyRequired: + return r.store.Get(ctx, credentialName) + default: + return r.resolveAuto(ctx, credentialName) + } +} + +func (r *Resolver) explicitValue(credentialName string) (string, bool) { + r.mu.Lock() + defer r.mu.Unlock() + value, ok := r.explicit[credentialName] + return value, ok +} + +func (r *Resolver) resolveFromConfigFile(credentialName string) (string, error) { + config, err := configfile.Load(r.filePath) + if err != nil { + return "", err + } + value := stringValue(config[viperKeyFor(credentialName)]) + if value == "" { + return "", ErrNotFound + } + return value, nil +} + +func (r *Resolver) resolveAuto(ctx context.Context, credentialName string) (string, error) { + current, err := r.store.Get(ctx, credentialName) + if err == nil { + return current, nil + } + // An unreachable keyring must not break an otherwise working plaintext + // configuration: degrade to the config-file layer exactly like a missing + // entry does. Access-denied stays loud — a locked keychain needs the user. + if !errors.Is(err, ErrNotFound) && !errors.Is(err, ErrKeyringUnavailable) { + return "", err + } + value, cfgErr := r.resolveFromConfigFile(credentialName) + if cfgErr == nil { + if errors.Is(err, ErrKeyringUnavailable) { + logger.PrintIfVerbose("credentialstore: keyring unavailable, using config-file credential") + } + return value, nil + } + if errors.Is(err, ErrKeyringUnavailable) { + return "", fmt.Errorf("%w; set CX_APIKEY or CX_KEYRING_MODE=disabled to keep using the config file", err) + } + return "", err +} + +// RemoveConfigFileEntry removes the plaintext entry for credentialName from the config file. +func (r *Resolver) RemoveConfigFileEntry(credentialName string) error { + return configfile.RemoveKey(r.filePath, viperKeyFor(credentialName)) +} + +var ( + defaultResolver *Resolver + resolverOnce sync.Once +) + +// Default returns the process-wide resolver built from the active CLI configuration. +func Default() *Resolver { + resolverOnce.Do(func() { + path := viper.GetString(params.ConfigFilePathKey) + if path == "" { + // Read directly so isolation (and any consumer without viper + // bindings) still honors the environment variable. + path = os.Getenv(params.ConfigFilePathEnv) + } + if path == "" { + path = filepath.Join(homeDir(), checkmarxDirName, checkmarxFileName) + } + defaultResolver = NewResolver(path, PolicyFromEnv(), nil) + }) + return defaultResolver +} + +// Resolve looks up a credential using the default resolver. +func Resolve(credentialName string) (string, error) { + return Default().Resolve(context.Background(), credentialName) +} + +// SetExplicitCredential registers an override on the default resolver. +func SetExplicitCredential(credentialName, value string) { + Default().SetExplicit(credentialName, value) +} + +// stringValue renders a decoded YAML value as its plaintext form. +func stringValue(value interface{}) string { + switch v := value.(type) { + case nil: + return "" + case string: + return v + default: + return fmt.Sprint(v) + } +} + +func homeDir() string { + if currentUser, err := user.Current(); err == nil && currentUser.HomeDir != "" { + return currentUser.HomeDir + } + dir, err := os.UserHomeDir() + if err != nil || dir == "" { + return "." + } + return dir +} diff --git a/internal/credentialstore/resolver_test.go b/internal/credentialstore/resolver_test.go new file mode 100644 index 000000000..11ca4bf32 --- /dev/null +++ b/internal/credentialstore/resolver_test.go @@ -0,0 +1,220 @@ +package credentialstore + +import ( + "context" + "errors" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/checkmarx/ast-cli/internal/params" + "github.com/stretchr/testify/assert" +) + +type fakeStore struct { + mu sync.Mutex + values map[string]string + getCalls int + getErr error + setErr error +} + +func newFakeStore() *fakeStore { + return &fakeStore{values: make(map[string]string)} +} + +func (f *fakeStore) Get(_ context.Context, credentialName string) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.getCalls++ + if f.getErr != nil { + return "", f.getErr + } + value, ok := f.values[credentialName] + if !ok { + return "", ErrNotFound + } + return value, nil +} + +func (f *fakeStore) Set(_ context.Context, credentialName, value string) error { + f.mu.Lock() + defer f.mu.Unlock() + if f.setErr != nil { + return f.setErr + } + f.values[credentialName] = value + return nil +} + +func (f *fakeStore) Delete(_ context.Context, credentialName string) error { + f.mu.Lock() + defer f.mu.Unlock() + delete(f.values, credentialName) + return nil +} + +func (f *fakeStore) calls() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.getCalls +} + +var errBackendDown = errors.New("dbus: failed to connect to socket") + +func writePlaintextConfig(t *testing.T, path, content string) { + t.Helper() + assert.NoError(t, os.WriteFile(path, []byte(content), 0o600)) +} + +const yamlWithAPIKey = "cx_apikey: yaml-secret\n" + +func TestResolvePrecedence(t *testing.T) { + cases := []struct { + name string + policy Policy + explicit string + envValue string + storeVal string + storeErr error + yaml string + want string + wantErr error + }{ + {name: "explicit wins over everything", policy: PolicyAuto, explicit: "expl", envValue: "env", storeVal: "store", yaml: yamlWithAPIKey, want: "expl"}, + {name: "env beats store", policy: PolicyAuto, envValue: "env", storeVal: "store", yaml: yamlWithAPIKey, want: "env"}, + {name: "store beats yaml", policy: PolicyAuto, storeVal: "store", yaml: yamlWithAPIKey, want: "store"}, + {name: "auto falls back to yaml on store miss", policy: PolicyAuto, yaml: yamlWithAPIKey, want: "yaml-secret"}, + {name: "auto empty env ignored", policy: PolicyAuto, envValue: "", storeVal: "store", want: "store"}, + {name: "required store hit", policy: PolicyRequired, storeVal: "store", yaml: yamlWithAPIKey, want: "store"}, + {name: "required ignores yaml on miss", policy: PolicyRequired, yaml: yamlWithAPIKey, wantErr: ErrNotFound}, + {name: "required backend error propagates", policy: PolicyRequired, storeErr: errBackendDown, wantErr: errBackendDown}, + {name: "auto backend error propagates without yaml fallback", policy: PolicyAuto, storeErr: errBackendDown, yaml: yamlWithAPIKey, wantErr: errBackendDown}, + {name: "disabled uses yaml", policy: PolicyDisabled, storeVal: "store", yaml: yamlWithAPIKey, want: "yaml-secret"}, + {name: "disabled env used", policy: PolicyDisabled, envValue: "env", yaml: yamlWithAPIKey, want: "env"}, + {name: "disabled nothing anywhere", policy: PolicyDisabled, wantErr: ErrNotFound}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + if tc.yaml != "" { + writePlaintextConfig(t, configPath, tc.yaml) + } + if tc.envValue != "" || (tc.name == "auto empty env ignored") { + t.Setenv(params.AstAPIKeyEnv, tc.envValue) + } + store := newFakeStore() + if tc.storeVal != "" { + store.values[CredentialAPIKey] = tc.storeVal + } + store.getErr = tc.storeErr + + resolver := NewResolver(configPath, tc.policy, store) + if tc.explicit != "" { + resolver.SetExplicit(CredentialAPIKey, tc.explicit) + } + + got, err := resolver.Resolve(context.Background(), CredentialAPIKey) + if tc.wantErr != nil { + assert.ErrorIs(t, err, tc.wantErr) + return + } + assert.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +// TestExplicitEmptyOverridesEnvAndStore pins the fix for --apikey "" (a +// flag the user actually typed) losing to CX_APIKEY/keyring: an explicitly +// set credential, even empty, must win over every lower layer for this +// invocation, matching pre-keyring viper flag-over-env precedence. +func TestExplicitEmptyOverridesEnvAndStore(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, yamlWithAPIKey) + t.Setenv(params.AstAPIKeyEnv, "env-secret") + store := newFakeStore() + store.values[CredentialAPIKey] = "store-secret" + + resolver := NewResolver(configPath, PolicyAuto, store) + resolver.SetExplicit(CredentialAPIKey, "") + + got, err := resolver.Resolve(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "", got) +} + +// SetExplicitCredential is the package-level wrapper over +// Default().SetExplicit, exercised through the same injected-resolver seam +// TestMigratePackageLevelWrapper uses. +func TestSetExplicitCredentialPackageLevelWrapper(t *testing.T) { + ResetForTest() + defer ResetForTest() + + store := newFakeStore() + SetDefaultResolverForTest(NewResolver(filepath.Join(t.TempDir(), "checkmarxcli.yaml"), PolicyAuto, store)) + + SetExplicitCredential(CredentialAPIKey, "explicit-secret") + + got, err := Resolve(CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "explicit-secret", got) +} + +// An unreachable keyring degrades to the config-file layer instead of failing +// the command outright. +func TestResolveAutoKeyringUnavailableFallsBackToYAML(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, yamlWithAPIKey) + store := newFakeStore() + store.getErr = ErrKeyringUnavailable + resolver := NewResolver(configPath, PolicyAuto, store) + + value, err := resolver.Resolve(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "yaml-secret", value) +} + +// When the keyring is unreachable and the config file has nothing either, the +// error must name the escape hatches rather than a bare "not found". +func TestResolveAutoKeyringUnavailableAndConfigMissingNamesEscapeHatch(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + store := newFakeStore() + store.getErr = ErrKeyringUnavailable + resolver := NewResolver(configPath, PolicyAuto, store) + + _, err := resolver.Resolve(context.Background(), CredentialAPIKey) + assert.ErrorIs(t, err, ErrKeyringUnavailable) + assert.ErrorContains(t, err, "CX_APIKEY") + assert.ErrorContains(t, err, "CX_KEYRING_MODE") +} + +func TestResolveDisabledNeverConsultsStore(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + writePlaintextConfig(t, configPath, yamlWithAPIKey) + store := newFakeStore() + resolver := NewResolver(configPath, PolicyDisabled, store) + + got, err := resolver.Resolve(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "yaml-secret", got) + assert.Equal(t, 0, store.calls()) +} + +func TestResolveInvalidName(t *testing.T) { + resolver := NewResolver(filepath.Join(t.TempDir(), "checkmarxcli.yaml"), PolicyAuto, newFakeStore()) + _, err := resolver.Resolve(context.Background(), "nope") + assert.ErrorIs(t, err, ErrInvalidName) +} + +func TestDefaultResolverSingletonAndReset(t *testing.T) { + ResetForTest() + defer ResetForTest() + + first := Default() + second := Default() + assert.Same(t, first, second) + assert.NotEmpty(t, first.filePath) + assert.NotNil(t, Default()) +} diff --git a/internal/credentialstore/resolver_write_test.go b/internal/credentialstore/resolver_write_test.go new file mode 100644 index 000000000..8fda75ee2 --- /dev/null +++ b/internal/credentialstore/resolver_write_test.go @@ -0,0 +1,154 @@ +package credentialstore + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/checkmarx/ast-cli/internal/configfile" + "github.com/stretchr/testify/assert" + "github.com/zalando/go-keyring" +) + +// PolicyDisabled must route writes to the config-file layer so reads and +// writes land in the same layer — the keyring is never touched. +func TestStoreDisabledWritesYAMLNotKeyring(t *testing.T) { + keyring.MockInit() + t.Cleanup(keyring.MockInit) + dir := t.TempDir() + yamlPath := filepath.Join(dir, "checkmarxcli.yaml") + resolver := NewResolver(yamlPath, PolicyDisabled, nil) + + assert.NoError(t, resolver.Store(context.Background(), CredentialAPIKey, "yaml-value")) + + data, err := os.ReadFile(yamlPath) + assert.NoError(t, err) + assert.Contains(t, string(data), "cx_apikey: yaml-value") + + value, err := resolver.Resolve(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "yaml-value", value) +} + +func TestClearDisabledRemovesYAMLAndReportsMissing(t *testing.T) { + dir := t.TempDir() + yamlPath := filepath.Join(dir, "checkmarxcli.yaml") + resolver := NewResolver(yamlPath, PolicyDisabled, nil) + ctx := context.Background() + + assert.ErrorIs(t, resolver.Clear(ctx, CredentialClientSecret), ErrNotFound) + + assert.NoError(t, resolver.Store(ctx, CredentialClientSecret, "to-be-cleared")) + assert.NoError(t, resolver.Clear(ctx, CredentialClientSecret)) + + value, err := resolver.Resolve(ctx, CredentialClientSecret) + assert.ErrorIs(t, err, ErrNotFound) + assert.Empty(t, value) +} + +// PolicyRequired round-trips through the keyring store only; the YAML layer +// is neither read nor written. +func TestStoreRequiredRoundTripIgnoresYAML(t *testing.T) { + keyring.MockInit() + t.Cleanup(keyring.MockInit) + store := NewCredentialStore(CanonicalConfigPath(t.TempDir())) + resolver := NewResolver(filepath.Join(t.TempDir(), "checkmarxcli.yaml"), PolicyRequired, store) + ctx := context.Background() + + assert.NoError(t, resolver.Store(ctx, CredentialAPIKey, "required-value")) + + if _, err := os.Stat(resolver.filePath); !os.IsNotExist(err) { + t.Fatalf("required mode must not create the YAML file") + } + value, err := resolver.Resolve(ctx, CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "required-value", value) +} + +// The default (auto) policy keeps writing to the keyring; plaintext removal is +// the caller's responsibility (setConfigProperty/persistLogin gate it on +// StoresInConfigFile). +func TestStoreAutoWritesKeyringOnly(t *testing.T) { + keyring.MockInit() + t.Cleanup(keyring.MockInit) + dir := t.TempDir() + yamlPath := filepath.Join(dir, "checkmarxcli.yaml") + assert.NoError(t, os.WriteFile(yamlPath, []byte("cx_apikey: old\n"), 0o600)) + resolver := NewResolver(yamlPath, PolicyAuto, nil) + + assert.NoError(t, resolver.Store(context.Background(), CredentialAPIKey, "auto-value")) + assert.False(t, resolver.StoresInConfigFile()) + + value, err := resolver.Resolve(context.Background(), CredentialAPIKey) + assert.NoError(t, err) + assert.Equal(t, "auto-value", value) +} + +func TestStoresInConfigFileMatrix(t *testing.T) { + dir := t.TempDir() + assert.False(t, NewResolver(filepath.Join(dir, "a.yaml"), PolicyAuto, nil).StoresInConfigFile()) + assert.False(t, NewResolver(filepath.Join(dir, "b.yaml"), PolicyRequired, nil).StoresInConfigFile()) + assert.True(t, NewResolver(filepath.Join(dir, "c.yaml"), PolicyDisabled, nil).StoresInConfigFile()) +} + +func TestStoreAndClearRejectUnknownCredentialName(t *testing.T) { + resolver := NewResolver(filepath.Join(t.TempDir(), "checkmarxcli.yaml"), PolicyAuto, nil) + ctx := context.Background() + + assert.ErrorIs(t, resolver.Store(ctx, "not-a-slot", "v"), ErrInvalidName) + assert.ErrorIs(t, resolver.Clear(ctx, "not-a-slot"), ErrInvalidName) +} + +func TestPackageLevelResolveRejectsUnknownCredentialName(t *testing.T) { + t.Setenv("CX_CONFIG_FILE_PATH", filepath.Join(t.TempDir(), "checkmarxcli.yaml")) + ResetForTest() + t.Cleanup(ResetForTest) + + _, err := Resolve("nope") + assert.ErrorIs(t, err, ErrInvalidName) +} + +func TestRemoveConfigFileEntryRemovesPlaintextSlot(t *testing.T) { + dir := t.TempDir() + yamlPath := filepath.Join(dir, "checkmarxcli.yaml") + assert.NoError(t, os.WriteFile(yamlPath, []byte("cx_client_secret: plaintext\n"), 0o600)) + resolver := NewResolver(yamlPath, PolicyRequired, nil) + + assert.NoError(t, resolver.RemoveConfigFileEntry(CredentialClientSecret)) + + config, err := configfile.Load(yamlPath) + assert.NoError(t, err) + assert.NotContains(t, config, "cx_client_secret") +} + +func TestStoreDisabledMissingParentDirFails(t *testing.T) { + resolver := NewResolver(filepath.Join(t.TempDir(), "no-such-dir", "checkmarxcli.yaml"), PolicyDisabled, nil) + assert.Error(t, resolver.Store(context.Background(), CredentialAPIKey, "v")) +} + +func TestClearDisabledMissingParentDirFails(t *testing.T) { + resolver := NewResolver(filepath.Join(t.TempDir(), "no-such-dir", "checkmarxcli.yaml"), PolicyDisabled, nil) + err := resolver.Clear(context.Background(), CredentialAPIKey) + assert.ErrorIs(t, err, ErrNotFound) +} + +// An unreadable (as opposed to merely missing) config file must propagate the +// read error rather than reporting a plain ErrNotFound. +func TestClearDisabledUnreadableConfigPropagatesError(t *testing.T) { + resolver := NewResolver(t.TempDir(), PolicyDisabled, nil) + err := resolver.Clear(context.Background(), CredentialAPIKey) + assert.Error(t, err) + assert.NotErrorIs(t, err, ErrNotFound) +} + +// Auto mode surfaces a config-file read failure instead of masking it as +// not-found once the keyring layer also misses. +func TestResolveAutoConfigFileReadErrorSurfaces(t *testing.T) { + dir := t.TempDir() + resolver := NewResolver(filepath.Join(dir, "checkmarxcli.yaml"), PolicyAuto, newFakeStore()) + _ = os.MkdirAll(resolver.filePath, 0o700) // path exists but is a directory + + _, err := resolver.Resolve(context.Background(), CredentialAPIKey) + assert.Error(t, err) +} diff --git a/internal/credentialstore/secrets.go b/internal/credentialstore/secrets.go new file mode 100644 index 000000000..64a1d79fd --- /dev/null +++ b/internal/credentialstore/secrets.go @@ -0,0 +1,56 @@ +package credentialstore + +import ( + "os" + + "github.com/checkmarx/ast-cli/internal/params" +) + +const ( + // CredentialAPIKey identifies the cx_apikey credential slot, which holds + // either a classic API key or an auth-login refresh token. + CredentialAPIKey = "cx_apikey" + // CredentialClientSecret identifies the cx_client_secret credential slot. + CredentialClientSecret = "cx_client_secret" +) + +// IsValidCredentialName reports whether name is a known logical credential. +func IsValidCredentialName(name string) bool { + return name == CredentialAPIKey || name == CredentialClientSecret +} + +// IsSecret reports whether viperKey maps to a secret config value. +func IsSecret(viperKey string) bool { + return viperKey == params.AstAPIKey || viperKey == params.AccessKeySecretConfigKey +} + +// CredentialForViperKey maps a secret viper key to its logical credential name. +func CredentialForViperKey(viperKey string) (string, bool) { + switch viperKey { + case params.AstAPIKey: + return CredentialAPIKey, true + case params.AccessKeySecretConfigKey: + return CredentialClientSecret, true + default: + return "", false + } +} + +func envVarFor(credentialName string) (string, bool) { + switch credentialName { + case CredentialAPIKey: + return params.AstAPIKeyEnv, true + case CredentialClientSecret: + return params.AccessKeySecretEnv, true + default: + return "", false + } +} + +func envValue(credentialName string) string { + envVar, ok := envVarFor(credentialName) + if !ok { + return "" + } + return os.Getenv(envVar) +} diff --git a/internal/credentialstore/store.go b/internal/credentialstore/store.go new file mode 100644 index 000000000..eee702a7f --- /dev/null +++ b/internal/credentialstore/store.go @@ -0,0 +1,57 @@ +package credentialstore + +import "context" + +// CredentialStore persists logical credentials in the OS keyring. +type CredentialStore interface { + Get(ctx context.Context, credentialName string) (string, error) + Set(ctx context.Context, credentialName, value string) error + Delete(ctx context.Context, credentialName string) error +} + +type keyringProvider interface { + Get(ctx context.Context, service, account string) (string, error) + Set(ctx context.Context, service, account, value string) error + Delete(ctx context.Context, service, account string) error +} + +type keyCredentialStore struct { + canonicalPath string + backend keyringProvider +} + +// NewCredentialStore returns a keyring-backed store scoped to canonicalConfigPath. +func NewCredentialStore(canonicalConfigPath string) CredentialStore { + return &keyCredentialStore{canonicalPath: canonicalConfigPath, backend: osKeyring{}} +} + +func (s *keyCredentialStore) Get(ctx context.Context, credentialName string) (string, error) { + account, err := s.account(credentialName) + if err != nil { + return "", err + } + return s.backend.Get(ctx, KeyringServiceName, account) +} + +func (s *keyCredentialStore) Set(ctx context.Context, credentialName, value string) error { + account, err := s.account(credentialName) + if err != nil { + return err + } + return s.backend.Set(ctx, KeyringServiceName, account, value) +} + +func (s *keyCredentialStore) Delete(ctx context.Context, credentialName string) error { + account, err := s.account(credentialName) + if err != nil { + return err + } + return s.backend.Delete(ctx, KeyringServiceName, account) +} + +func (s *keyCredentialStore) account(credentialName string) (string, error) { + if !IsValidCredentialName(credentialName) { + return "", ErrInvalidName + } + return AccountFor(s.canonicalPath, credentialName), nil +} diff --git a/internal/credentialstore/store_test.go b/internal/credentialstore/store_test.go new file mode 100644 index 000000000..8cbb6b1b2 --- /dev/null +++ b/internal/credentialstore/store_test.go @@ -0,0 +1,36 @@ +package credentialstore + +import ( + "context" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/zalando/go-keyring" +) + +func TestCredentialStoreDeleteRoundTrip(t *testing.T) { + keyring.MockInit() + t.Cleanup(keyring.MockInit) + store := NewCredentialStore(CanonicalConfigPath(filepath.Join(t.TempDir(), "checkmarxcli.yaml"))) + ctx := context.Background() + + assert.ErrorIs(t, store.Delete(ctx, CredentialAPIKey), ErrNotFound) + + assert.NoError(t, store.Set(ctx, CredentialAPIKey, "value")) + assert.NoError(t, store.Delete(ctx, CredentialAPIKey)) + + _, err := store.Get(ctx, CredentialAPIKey) + assert.ErrorIs(t, err, ErrNotFound) +} + +func TestCredentialStoreRejectsUnknownCredentialName(t *testing.T) { + store := NewCredentialStore(CanonicalConfigPath(filepath.Join(t.TempDir(), "checkmarxcli.yaml"))) + ctx := context.Background() + + _, err := store.Get(ctx, "unknown") + assert.ErrorIs(t, err, ErrInvalidName) + + assert.ErrorIs(t, store.Set(ctx, "unknown", "value"), ErrInvalidName) + assert.ErrorIs(t, store.Delete(ctx, "unknown"), ErrInvalidName) +} diff --git a/internal/credentialstore/testing.go b/internal/credentialstore/testing.go new file mode 100644 index 000000000..d5bdd1e45 --- /dev/null +++ b/internal/credentialstore/testing.go @@ -0,0 +1,22 @@ +package credentialstore + +import "sync" + +// Test-only seams for the default-resolver singleton. They live in the +// production package (not _test.go) because harnesses in other packages — +// commands, util, agenthooks/mcp, wrappers — must inject fakes, and Go cannot +// export symbols from test files to external test packages. + +// ResetForTest discards the default resolver singleton; tests only. +func ResetForTest() { + resolverOnce = sync.Once{} + defaultResolver = nil +} + +// SetDefaultResolverForTest installs r as the default resolver; tests only. +// The no-op Once marks initialization as done so a later Default() call cannot +// overwrite the injected resolver. +func SetDefaultResolverForTest(r *Resolver) { + resolverOnce.Do(func() {}) + defaultResolver = r +} diff --git a/internal/logger/utils.go b/internal/logger/utils.go index 3134184d7..66b5c6bb8 100644 --- a/internal/logger/utils.go +++ b/internal/logger/utils.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httputil" "strings" + "sync" "unicode/utf8" "github.com/checkmarx/ast-cli/internal/params" @@ -16,15 +17,31 @@ import ( const ContentLengthLimit = 1000000 // 1mb in bytes var sanitizeFlags = []string{ - params.AstAPIKey, params.AccessKeyIDConfigKey, params.AccessKeySecretConfigKey, + params.AccessKeyIDConfigKey, params.UsernameFlag, params.PasswordFlag, - params.AstToken, params.SSHValue, + params.SSHValue, params.SCMTokenFlag, params.ProxyKey, params.UploadURLEnv, params.SCSRepoTokenFlag, params.SCSRepoURLFlag, } +var ( + sensitiveValuesMu sync.RWMutex + sensitiveValues = map[string]struct{}{} +) + +// RegisterSensitiveValue masks a runtime value (e.g. the OAuth access token) +// in all logger output, for values that no longer live in viper. +func RegisterSensitiveValue(value string) { + if value == "" { + return + } + sensitiveValuesMu.Lock() + defer sensitiveValuesMu.Unlock() + sensitiveValues[value] = struct{}{} +} + func Print(msg string) { if utf8.Valid([]byte(msg)) { log.Print(sanitizeLogs(msg)) @@ -70,10 +87,15 @@ func PrintResponse(r *http.Response, body bool) { func sanitizeLogs(msg string) string { for _, flag := range sanitizeFlags { value := viper.GetString(flag) - if len(value) > 0 { + if value != "" { msg = strings.ReplaceAll(msg, value, "***") } } + sensitiveValuesMu.RLock() + defer sensitiveValuesMu.RUnlock() + for value := range sensitiveValues { + msg = strings.ReplaceAll(msg, value, "***") + } return msg } diff --git a/internal/params/binds.go b/internal/params/binds.go index bc19e4383..4cc649b74 100644 --- a/internal/params/binds.go +++ b/internal/params/binds.go @@ -12,7 +12,6 @@ var EnvVarsBinds = []struct { {ProxyKerberosKrb5ConfKey, ProxyKerberosKrb5ConfEnv, ""}, {ProxyKerberosCcacheKey, ProxyKerberosCcacheEnv, ""}, {BaseAuthURIKey, BaseAuthURIEnv, ""}, - {AstAPIKey, AstAPIKeyEnv, ""}, {IgnoreProxyKey, IgnoreProxyEnv, ""}, {AgentNameKey, AgentNameEnv, "ASTCLI"}, {OriginKey, OriginEnv, "CLI"}, @@ -62,7 +61,6 @@ var EnvVarsBinds = []struct { {LogsPathKey, LogsPathEnv, "api/logs"}, {LogsEngineLogPathKey, LogsEngineLogPathEnv, "/%s/%s"}, {AccessKeyIDConfigKey, AccessKeyIDEnv, ""}, - {AccessKeySecretConfigKey, AccessKeySecretEnv, ""}, {TenantKey, TenantEnv, ""}, {BranchKey, BranchEnv, ""}, {AstRoleKey, AstRoleEnv, ScaAgent}, diff --git a/internal/params/flags.go b/internal/params/flags.go index 9415101b8..0dd7e1089 100644 --- a/internal/params/flags.go +++ b/internal/params/flags.go @@ -167,7 +167,6 @@ const ( QueryIDFlag = "query-id" SSHKeyFlag = "ssh-key" RepoURLFlag = "repo-url" - AstToken = "ast-token" SSHValue = "ssh-value" KicsContainerNameKey = "kics-container-name" KicsPlatformsFlag = "kics-platforms" diff --git a/internal/wrappers/client.go b/internal/wrappers/client.go index 142afe190..030967e65 100644 --- a/internal/wrappers/client.go +++ b/internal/wrappers/client.go @@ -24,6 +24,7 @@ import ( "github.com/pkg/errors" "github.com/spf13/viper" + "github.com/checkmarx/ast-cli/internal/credentialstore" commonParams "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/wrappers/kerberos" "github.com/checkmarx/ast-cli/internal/wrappers/ntlm" @@ -606,8 +607,14 @@ func enrichWithPasswordCredentials( func configureClientCredentialsAndGetNewToken() (string, error) { accessKeyID := viper.GetString(commonParams.AccessKeyIDConfigKey) - accessKeySecret := viper.GetString(commonParams.AccessKeySecretConfigKey) - astAPIKey := viper.GetString(commonParams.AstAPIKey) + accessKeySecret, err := credentialstore.Resolve(credentialstore.CredentialClientSecret) + if err != nil && !errors.Is(err, credentialstore.ErrNotFound) { + return "", err + } + astAPIKey, err := credentialstore.Resolve(credentialstore.CredentialAPIKey) + if err != nil && !errors.Is(err, credentialstore.ErrNotFound) { + return "", err + } var accessToken string credType := viper.GetString(commonParams.PreferredCredentialTypeKey) @@ -672,7 +679,7 @@ func writeCredentialsToCache(accessToken string) { defer credentialsMutex.Unlock() logger.PrintIfVerbose("Storing API access token to cache.") - viper.Set(commonParams.AstToken, accessToken) + logger.RegisterSensitiveValue(accessToken) cachedAccessToken = accessToken cachedAccessTime = time.Now() } @@ -915,7 +922,10 @@ func GetRealmURL() (string, error) { var err error override := viper.GetBool(commonParams.ApikeyOverrideFlag) - apiKey := viper.GetString(commonParams.AstAPIKey) + apiKey, err := credentialstore.Resolve(credentialstore.CredentialAPIKey) + if err != nil && !errors.Is(err, credentialstore.ErrNotFound) { + return "", err + } // On override, skip decoding the stored key so the flags win and a stale key // can't block login with a decode error. if len(apiKey) > 0 && !override { diff --git a/internal/wrappers/client_test.go b/internal/wrappers/client_test.go index e2f4fb670..2dabf1b0d 100644 --- a/internal/wrappers/client_test.go +++ b/internal/wrappers/client_test.go @@ -1,23 +1,60 @@ package wrappers import ( + "context" "encoding/base64" "encoding/json" "errors" "fmt" "net/http" "net/http/httptest" + "path/filepath" "strconv" "strings" "sync" "testing" "time" + "github.com/checkmarx/ast-cli/internal/credentialstore" commonParams "github.com/checkmarx/ast-cli/internal/params" "github.com/spf13/viper" "github.com/stretchr/testify/assert" ) +type fakeCredentialStore struct { + data map[string]string + getErr error + setErr error + deleteErr error +} + +func (f *fakeCredentialStore) Get(_ context.Context, credentialName string) (string, error) { + if f.getErr != nil { + return "", f.getErr + } + value, ok := f.data[credentialName] + if !ok { + return "", credentialstore.ErrNotFound + } + return value, nil +} + +func (f *fakeCredentialStore) Set(_ context.Context, credentialName, value string) error { + if f.setErr != nil { + return f.setErr + } + f.data[credentialName] = value + return nil +} + +func (f *fakeCredentialStore) Delete(_ context.Context, credentialName string) error { + if f.deleteErr != nil { + return f.deleteErr + } + delete(f.data, credentialName) + return nil +} + type mockReadCloser struct{} func (m *mockReadCloser) Read(p []byte) (n int, err error) { @@ -101,11 +138,10 @@ func TestConcurrentWriteCredentialsToCache(t *testing.T) { } wg.Wait() - token := viper.Get(commonParams.AstToken) - assert.NotNil(t, token, "Token should not be nil") - - tokenStr, ok := token.(string) - assert.True(t, ok, "Token should be a string") + credentialsMutex.Lock() + tokenStr := cachedAccessToken + credentialsMutex.Unlock() + assert.NotEmpty(t, tokenStr, "Token should not be empty") splitToken := strings.Split(tokenStr, "_") assert.Equal(t, 2, len(splitToken), "Token should split into 2 parts") @@ -187,6 +223,12 @@ func TestGetAPIKeyPayload(t *testing.T) { } } +// TestGetRealmURL_LoginOverrideSkipsStoredAPIKey guards the `cx auth login` fix: +// when ApikeyOverrideFlag is set, GetRealmURL must build the realm from the +// explicit --base-auth-uri/--tenant flags and must NOT decode the stored +// cx_apikey. A stale/malformed stored key previously surfaced here as a hard +// "failed to resolve IAM realm URL" error, making login impossible until the bad +// key was manually cleared. // TestGetRealmURL_LoginOverrideSkipsStoredAPIKey guards the `cx auth login` fix: // when ApikeyOverrideFlag is set, GetRealmURL must build the realm from the // explicit --base-auth-uri/--tenant flags and must NOT decode the stored @@ -196,7 +238,6 @@ func TestGetAPIKeyPayload(t *testing.T) { func TestGetRealmURL_LoginOverrideSkipsStoredAPIKey(t *testing.T) { keys := []string{ commonParams.ApikeyOverrideFlag, - commonParams.AstAPIKey, commonParams.BaseAuthURIKey, commonParams.TenantKey, } @@ -210,11 +251,16 @@ func TestGetRealmURL_LoginOverrideSkipsStoredAPIKey(t *testing.T) { } }) + store := &fakeCredentialStore{data: map[string]string{}} + t.Setenv("CX_CONFIG_FILE_PATH", filepath.Join(t.TempDir(), "checkmarxcli.yaml")) + credentialstore.SetDefaultResolverForTest(credentialstore.NewResolver("checkmarxcli.yaml", credentialstore.PolicyAuto, store)) + t.Cleanup(credentialstore.ResetForTest) + const malformedKey = "not-a-jwt" // single segment -> ExtractFromTokenClaims fails t.Run("override builds realm from flags despite a malformed stored key", func(t *testing.T) { viper.Set(commonParams.ApikeyOverrideFlag, true) - viper.Set(commonParams.AstAPIKey, malformedKey) + store.data[credentialstore.CredentialAPIKey] = malformedKey viper.Set(commonParams.BaseAuthURIKey, "https://eu.iam.checkmarx.net") viper.Set(commonParams.TenantKey, "cx_seg") @@ -226,7 +272,7 @@ func TestGetRealmURL_LoginOverrideSkipsStoredAPIKey(t *testing.T) { t.Run("without override a malformed stored key still errors (unchanged)", func(t *testing.T) { viper.Set(commonParams.ApikeyOverrideFlag, false) - viper.Set(commonParams.AstAPIKey, malformedKey) + store.data[credentialstore.CredentialAPIKey] = malformedKey viper.Set(commonParams.BaseAuthURIKey, "https://eu.iam.checkmarx.net") viper.Set(commonParams.TenantKey, "cx_seg") @@ -369,3 +415,40 @@ func TestRetryHTTPIAMRequest_Fail(t *testing.T) { assert.Error(t, err) assert.Nil(t, resp) } + +// An unreachable OS keyring must surface as a keyring failure, never as a +// silent no-credentials path. +func TestConfigureClientCredentialsPropagatesKeyringUnavailable(t *testing.T) { + t.Setenv("CX_CONFIG_FILE_PATH", filepath.Join(t.TempDir(), "checkmarxcli.yaml")) + store := &fakeCredentialStore{ + data: map[string]string{}, + getErr: fmt.Errorf("%w: dbus: failed to connect to socket", credentialstore.ErrKeyringUnavailable), + } + credentialstore.SetDefaultResolverForTest(credentialstore.NewResolver("checkmarxcli.yaml", credentialstore.PolicyAuto, store)) + t.Cleanup(credentialstore.ResetForTest) + + viper.Set(commonParams.PreferredCredentialTypeKey, "") + + _, err := configureClientCredentialsAndGetNewToken() + assert.Error(t, err) + assert.ErrorIs(t, err, credentialstore.ErrKeyringUnavailable) +} + +func TestGetRealmURLPropagatesKeyringUnavailable(t *testing.T) { + savedOverride := viper.Get(commonParams.ApikeyOverrideFlag) + t.Cleanup(func() { viper.Set(commonParams.ApikeyOverrideFlag, savedOverride) }) + + t.Setenv("CX_CONFIG_FILE_PATH", filepath.Join(t.TempDir(), "checkmarxcli.yaml")) + store := &fakeCredentialStore{ + data: map[string]string{}, + getErr: fmt.Errorf("%w: dbus: failed to connect to socket", credentialstore.ErrKeyringUnavailable), + } + credentialstore.SetDefaultResolverForTest(credentialstore.NewResolver("checkmarxcli.yaml", credentialstore.PolicyAuto, store)) + t.Cleanup(credentialstore.ResetForTest) + + viper.Set(commonParams.ApikeyOverrideFlag, false) + + _, err := GetRealmURL() + assert.Error(t, err) + assert.ErrorIs(t, err, credentialstore.ErrKeyringUnavailable) +} diff --git a/internal/wrappers/configuration/configuration.go b/internal/wrappers/configuration/configuration.go index 57509ef62..28f902e0e 100644 --- a/internal/wrappers/configuration/configuration.go +++ b/internal/wrappers/configuration/configuration.go @@ -2,19 +2,20 @@ package configuration import ( "bufio" + "context" + stderrors "errors" "fmt" - "io" "log" "os" "os/user" "strings" + "github.com/checkmarx/ast-cli/internal/configfile" + "github.com/checkmarx/ast-cli/internal/credentialstore" "github.com/checkmarx/ast-cli/internal/logger" "github.com/checkmarx/ast-cli/internal/params" - "github.com/gofrs/flock" "github.com/pkg/errors" "github.com/spf13/viper" - "gopkg.in/yaml.v3" ) const configDirName = "/.checkmarx" @@ -26,9 +27,9 @@ func PromptConfiguration() { baseURI := viper.GetString(params.BaseURIKey) baseURISrc := viper.GetString(params.BaseURIKey) baseAuthURI := viper.GetString(params.BaseAuthURIKey) - accessKeySecret := viper.GetString(params.AccessKeySecretConfigKey) + accessKeySecret := resolveSecretForPrompt(params.AccessKeySecretConfigKey) accessKey := viper.GetString(params.AccessKeyIDConfigKey) - accessAPIKey := viper.GetString(params.AstAPIKey) + accessAPIKey := resolveSecretForPrompt(params.AstAPIKey) tenant := viper.GetString(params.TenantKey) fmt.Print("Setup guide: https://checkmarx.com/resource/documents/en/34965-68621-checkmarx-one-cli-quick-start-guide.html\n\n") // Prompt for Base URI @@ -64,31 +65,31 @@ func PromptConfiguration() { authType = strings.Replace(authType, "\n", "", -1) authType = strings.Replace(authType, "\r", "", -1) if strings.EqualFold(authType, "Y") { - fmt.Printf("AST API Key [%s]: ", obfuscateString(accessAPIKey)) + fmt.Printf("AST API Key [%s]: ", ObfuscateString(accessAPIKey)) accessAPIKey, _ = reader.ReadString('\n') accessAPIKey = strings.Replace(accessAPIKey, "\n", "", -1) accessAPIKey = strings.Replace(accessAPIKey, "\r", "", -1) if len(accessAPIKey) > 0 { - setConfigPropertyQuiet(params.AstAPIKey, accessAPIKey) + storeProperty(params.AstAPIKey, accessAPIKey) setConfigPropertyQuiet(params.AccessKeyIDConfigKey, "") - setConfigPropertyQuiet(params.AccessKeySecretConfigKey, "") + storeProperty(params.AccessKeySecretConfigKey, "") } } else { - fmt.Printf("Checkmarx One Client ID [%s]: ", obfuscateString(accessKey)) + fmt.Printf("Checkmarx One Client ID [%s]: ", ObfuscateString(accessKey)) accessKey, _ = reader.ReadString('\n') accessKey = strings.Replace(accessKey, "\n", "", -1) accessKey = strings.Replace(accessKey, "\r", "", -1) if len(accessKey) > 0 { setConfigPropertyQuiet(params.AccessKeyIDConfigKey, accessKey) - setConfigPropertyQuiet(params.AstAPIKey, "") + storeProperty(params.AstAPIKey, "") } - fmt.Printf("Client Secret [%s]: ", obfuscateString(accessKeySecret)) + fmt.Printf("Client Secret [%s]: ", ObfuscateString(accessKeySecret)) accessKeySecret, _ = reader.ReadString('\n') accessKeySecret = strings.Replace(accessKeySecret, "\n", "", -1) accessKeySecret = strings.Replace(accessKeySecret, "\r", "", -1) if len(accessKeySecret) > 0 { - setConfigPropertyQuiet(params.AccessKeySecretConfigKey, accessKeySecret) - setConfigPropertyQuiet(params.AstAPIKey, "") + storeProperty(params.AccessKeySecretConfigKey, accessKeySecret) + storeProperty(params.AstAPIKey, "") } } } @@ -124,7 +125,8 @@ func readLine(reader *bufio.Reader) string { return strings.TrimSpace(s) } -func obfuscateString(str string) string { +// ObfuscateString masks all but the last four characters of a secret value. +func ObfuscateString(str string) string { if len(str) > obfuscateLimit { return "******" + str[len(str)-4:] } else if len(str) > 1 { @@ -148,13 +150,77 @@ func setConfigPropertyQuiet(propName, propValue string) { } } -func SetConfigProperty(propName, propValue string) { - fmt.Println("Setting property [", propName, "] to value [", propValue, "]") - setConfigPropertyQuiet(propName, propValue) +// SetConfigProperty stores propValue for propName via the credential resolver +// or config file, masking the echoed value when propName is a secret. +func SetConfigProperty(propName, propValue string) error { + displayValue := propValue + if credentialstore.IsSecret(strings.ToLower(propName)) { + displayValue = ObfuscateString(propValue) + } + fmt.Println("Setting property [", propName, "] to value [", displayValue, "]") + return setConfigProperty(strings.ToLower(propName), propValue) +} + +func storeProperty(propName, propValue string) { + if err := setConfigProperty(propName, propValue); err != nil { + fmt.Println("Error storing property", propName, err) + } +} + +func setConfigProperty(propName, propValue string) error { + credentialName, ok := credentialstore.CredentialForViperKey(propName) + if !ok { + setConfigPropertyQuiet(propName, propValue) + return nil + } + resolver := credentialstore.Default() + var err error + if propValue == "" { + err = resolver.Clear(context.Background(), credentialName) + if stderrors.Is(err, credentialstore.ErrNotFound) { + err = nil + } + } else { + err = resolver.Store(context.Background(), credentialName, propValue) + } + if err != nil { + return fmt.Errorf("storing %s: %w", propName, err) + } + if !resolver.StoresInConfigFile() { + removePlaintextEntryQuietly(propName) + } + return nil +} + +func removePlaintextEntryQuietly(propName string) { + credentialName, ok := credentialstore.CredentialForViperKey(propName) + if !ok { + return + } + if err := credentialstore.Default().RemoveConfigFileEntry(credentialName); err != nil { + logger.PrintfIfVerbose("could not remove %s from config file: %v", propName, err) + } +} + +func resolveSecretForPrompt(viperKey string) string { + credentialName, ok := credentialstore.CredentialForViperKey(viperKey) + if !ok { + return "" + } + value, err := credentialstore.Resolve(credentialName) + if err != nil { + return "" + } + return value } func LoadConfiguration() error { configFilePath := viper.GetString(params.ConfigFilePathKey) + if configFilePath == "" { + // Read directly so consumers without viper bindings (tests, embedded + // runs) still honor the environment variable. + configFilePath = os.Getenv(params.ConfigFilePathEnv) + } if configFilePath != "" { err := validateConfigFile(configFilePath) @@ -201,114 +267,30 @@ func validateConfigFile(configFilePath string) error { } func SafeWriteSingleConfigKey(configFilePath, key string, value int) error { - // Create a file lock - lock := flock.New(configFilePath + ".lock") - locked, err := lock.TryLock() - if err != nil { - return errors.Errorf("error acquiring lock: %s", err.Error()) - } - if !locked { - return errors.Errorf("could not acquire lock") - } - defer func() { - _ = lock.Unlock() - }() - - // Load existing configuration or initialize a new one - config, err := LoadConfig(configFilePath) - if err != nil { - return errors.Errorf("error loading config: %s", err.Error()) - } - - // Update the configuration key - config[key] = value - - // Save the updated configuration back to the file - if err = SaveConfig(configFilePath, config); err != nil { - return errors.Errorf("error saving config: %s", err.Error()) - } - return nil + return configfile.SetKey(configFilePath, key, value) } func SafeWriteSingleConfigKeyString(configFilePath, key string, value string) error { - // Create a file lock - lock := flock.New(configFilePath + ".lock") - locked, err := lock.TryLock() - if err != nil { - return errors.Errorf("error acquiring lock: %s", err.Error()) - } - if !locked { - return errors.Errorf("could not acquire lock") - } - defer func() { - _ = lock.Unlock() - }() - - // Load existing configuration or initialize a new one - config, err := LoadConfig(configFilePath) - if err != nil { - return errors.Errorf("error loading config: %s", err.Error()) - } - - // Update the configuration key - config[key] = value - - // Save the updated configuration back to the file - if err = SaveConfig(configFilePath, config); err != nil { - return errors.Errorf("error saving config: %s", err.Error()) - } - return nil + return configfile.SetKey(configFilePath, key, value) } // LoadConfig loads the configuration from a file. If the file does not exist // or is empty, it returns an empty map. func LoadConfig(path string) (map[string]interface{}, error) { - config := make(map[string]interface{}) - file, err := os.Open(path) - if err != nil { - if os.IsNotExist(err) { - return config, nil // Return an empty config if the file doesn't exist - } - return nil, err - } - defer func(file *os.File) { - _ = file.Close() - }(file) - - decoder := yaml.NewDecoder(file) - if err = decoder.Decode(&config); err != nil { - if err == io.EOF { - // An empty (zero-byte) config file is a valid "no config yet" - // state, not corruption. Treat it like a missing file and return - // an empty config so callers (e.g. cx auth login persisting a - // fresh token) can populate it instead of failing. - return config, nil - } - return nil, fmt.Errorf("error decoding YAML: %w", err) - } - return config, nil + return configfile.Load(path) } // SaveConfig writes the configuration to a file. func SaveConfig(path string, config map[string]interface{}) error { - file, err := os.Create(path) - if err != nil { - return err - } - - defer func(file *os.File) { - _ = file.Close() - }(file) - - encoder := yaml.NewEncoder(file) - if err = encoder.Encode(config); err != nil { - return fmt.Errorf("error encoding YAML: %w", err) - } - return nil + return configfile.Save(path, config) } func GetConfigFilePath() (string, error) { configFilePath := viper.GetString(params.ConfigFilePathKey) + if configFilePath == "" { + // Test-isolation seam: harnesses set the env var without viper bindings. + configFilePath = os.Getenv(params.ConfigFilePathEnv) + } if configFilePath == "" { usr, err := user.Current() @@ -342,9 +324,9 @@ func ShowConfiguration() { fmt.Printf("%30v", "Client ID: ") fmt.Println(viper.GetString(params.AccessKeyIDConfigKey)) fmt.Printf("%30v", "Client Secret: ") - fmt.Println(obfuscateString(viper.GetString(params.AccessKeySecretConfigKey))) + fmt.Println(ObfuscateString(resolveSecretForPrompt(params.AccessKeySecretConfigKey))) fmt.Printf("%30v", "APIKey: ") - fmt.Println(obfuscateString(viper.GetString(params.AstAPIKey))) + fmt.Println(ObfuscateString(resolveSecretForPrompt(params.AstAPIKey))) fmt.Printf("%30v", "Proxy: ") fmt.Println(viper.GetString(params.ProxyKey)) } diff --git a/internal/wrappers/mock/credential-store-mock.go b/internal/wrappers/mock/credential-store-mock.go index 7a740fbff..d971783f5 100644 --- a/internal/wrappers/mock/credential-store-mock.go +++ b/internal/wrappers/mock/credential-store-mock.go @@ -1,34 +1,61 @@ package mock -// CredentialStoreMock is an in-memory CredentialStore for unit tests. +import ( + "context" + "sync" + + "github.com/checkmarx/ast-cli/internal/credentialstore" +) + type CredentialStoreMock struct { - Store map[string]string + mu sync.Mutex + Store map[string]string + GetErr error + SetErr error + DeleteErr error } -// NewCredentialStoreMock returns an empty in-memory credential store. func NewCredentialStoreMock() *CredentialStoreMock { - return &CredentialStoreMock{Store: map[string]string{}} + return &CredentialStoreMock{Store: make(map[string]string)} } -// GetSecret retrieves a secret value from the in-memory store. -func (m *CredentialStoreMock) GetSecret(key string) (string, error) { - if m.Store == nil { - return "", nil +// Get returns the stored value for credentialName, or GetErr/ErrNotFound. +func (m *CredentialStoreMock) Get(_ context.Context, credentialName string) (string, error) { + m.mu.Lock() + defer m.mu.Unlock() + if m.GetErr != nil { + return "", m.GetErr + } + value, ok := m.Store[credentialName] + if !ok { + return "", credentialstore.ErrNotFound } - return m.Store[key], nil + return value, nil } -// SetSecret stores a secret value in the in-memory store. -func (m *CredentialStoreMock) SetSecret(key, value string) error { - if m.Store == nil { - m.Store = map[string]string{} +// Set stores value under credentialName, or returns SetErr. +func (m *CredentialStoreMock) Set(_ context.Context, credentialName, value string) error { + m.mu.Lock() + defer m.mu.Unlock() + if m.SetErr != nil { + return m.SetErr } - m.Store[key] = value + m.Store[credentialName] = value return nil } -// DeleteSecret removes a secret value from the in-memory store. -func (m *CredentialStoreMock) DeleteSecret(key string) error { - delete(m.Store, key) +// Delete removes credentialName, or returns DeleteErr/ErrNotFound. +func (m *CredentialStoreMock) Delete(_ context.Context, credentialName string) error { + m.mu.Lock() + defer m.mu.Unlock() + if m.DeleteErr != nil { + return m.DeleteErr + } + if _, ok := m.Store[credentialName]; !ok { + return credentialstore.ErrNotFound + } + delete(m.Store, credentialName) return nil } + +var _ credentialstore.CredentialStore = (*CredentialStoreMock)(nil) diff --git a/test/integration/root_test.go b/test/integration/root_test.go index 13a229f89..2e3d1fcf5 100644 --- a/test/integration/root_test.go +++ b/test/integration/root_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/checkmarx/ast-cli/internal/credentialstore" commonParams "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/wrappers" "github.com/spf13/viper" @@ -48,6 +49,11 @@ var rootProjectName string func TestMain(m *testing.M) { log.Println("CLI integration tests started") viper.SetDefault(resolverEnvVar, resolverEnvVarDefault) + // CI runners have no user keyring; credentials arrive via environment + // variables, which the resolver serves in every policy mode. + if os.Getenv(credentialstore.KeyringModeEnvVar) == "" { + _ = os.Setenv(credentialstore.KeyringModeEnvVar, "disabled") + } exitVal := m.Run() //deleteScanAndProject() log.Println("CLI integration tests done")