From a38b310bdef2ee0b02a2e8f4853613ff9bbdddcc Mon Sep 17 00:00:00 2001 From: Ryan VanGundy Date: Sat, 21 Jun 2025 18:36:50 -0400 Subject: [PATCH] Scrub secrets in Exec output --- pkg/secrets/op_cli_secrets_provider.go | 14 +- pkg/secrets/op_sdk_secrets_provider.go | 17 +- pkg/secrets/op_sdk_secrets_provider_test.go | 160 ++----- pkg/secrets/sops_secrets_provider.go | 11 +- pkg/shell/mock_shell.go | 8 + pkg/shell/mock_shell_test.go | 34 ++ pkg/shell/scrubbing_writer.go | 37 ++ pkg/shell/shell.go | 74 ++- pkg/shell/shell_test.go | 469 ++++++++++++++++++-- pkg/shell/unix_shell_test.go | 4 +- 10 files changed, 632 insertions(+), 196 deletions(-) create mode 100644 pkg/shell/scrubbing_writer.go diff --git a/pkg/secrets/op_cli_secrets_provider.go b/pkg/secrets/op_cli_secrets_provider.go index 67751c4fb..e0a194c2e 100644 --- a/pkg/secrets/op_cli_secrets_provider.go +++ b/pkg/secrets/op_cli_secrets_provider.go @@ -9,9 +9,9 @@ import ( ) // The OnePasswordCLISecretsProvider is an implementation of the SecretsProvider interface -// It provides integration with the 1Password CLI for secret management -// It serves as a bridge between the application and 1Password's secure storage -// It enables retrieval and parsing of secrets from 1Password vaults +// It provides integration with the 1Password CLI for secret management with automatic shell scrubbing registration +// It serves as a bridge between the application and 1Password's secure storage with built-in security features +// It enables retrieval and parsing of secrets from 1Password vaults while automatically registering secrets for output scrubbing // ============================================================================= // Types @@ -41,7 +41,9 @@ func NewOnePasswordCLISecretsProvider(vault secretsConfigType.OnePasswordVault, // Public Methods // ============================================================================= -// GetSecret retrieves a secret value for the specified key +// GetSecret retrieves a secret value for the specified key and automatically registers it with the shell for output scrubbing. +// It uses the 1Password CLI to fetch secrets from the configured vault and automatically registers +// the retrieved secret with the shell's scrubbing system to prevent accidental exposure in command output. func (s *OnePasswordCLISecretsProvider) GetSecret(key string) (string, error) { if !s.unlocked { return "********", nil @@ -59,7 +61,9 @@ func (s *OnePasswordCLISecretsProvider) GetSecret(key string) (string, error) { return "", fmt.Errorf("failed to retrieve secret from 1Password: %w", err) } - return strings.TrimSpace(string(output)), nil + value := strings.TrimSpace(string(output)) + s.shell.RegisterSecret(value) + return value, nil } // ParseSecrets identifies and replaces ${{ op... }} patterns in the input diff --git a/pkg/secrets/op_sdk_secrets_provider.go b/pkg/secrets/op_sdk_secrets_provider.go index 118734a5a..6f957abfe 100644 --- a/pkg/secrets/op_sdk_secrets_provider.go +++ b/pkg/secrets/op_sdk_secrets_provider.go @@ -1,7 +1,7 @@ // The OnePasswordSDKSecretsProvider is an implementation of the SecretsProvider interface -// It provides integration with the 1Password SDK for secret management -// It serves as a bridge between the application and 1Password's secure storage -// It enables retrieval and parsing of secrets from 1Password vaults using the official SDK +// It provides integration with the 1Password SDK for secret management with automatic shell scrubbing registration +// It serves as a bridge between the application and 1Password's secure storage with built-in security features +// It enables retrieval and parsing of secrets from 1Password vaults using the official SDK while automatically registering secrets for output scrubbing package secrets @@ -69,11 +69,11 @@ func (s *OnePasswordSDKSecretsProvider) Initialize() error { return nil } -// GetSecret retrieves a secret value for the specified key. It first checks if the provider is unlocked. -// If not, it returns a masked value. It then ensures the 1Password client is initialized using a -// service account token from the environment. The key is split into item and field parts, and the -// item name is sanitized. A secret reference URI is constructed and used to resolve the secret value -// from 1Password. If successful, the secret value is returned; otherwise, an error is reported. +// GetSecret retrieves a secret value for the specified key and automatically registers it with the shell for output scrubbing. +// It first checks if the provider is unlocked. If not, it returns a masked value. It then ensures the 1Password client +// is initialized using a service account token from the environment. The key is split into item and field parts, and the +// item name is sanitized. A secret reference URI is constructed and used to resolve the secret value from 1Password. +// If successful, the secret value is registered with the shell's scrubbing system and returned; otherwise, an error is reported. func (s *OnePasswordSDKSecretsProvider) GetSecret(key string) (string, error) { if !s.unlocked { return "********", nil @@ -120,6 +120,7 @@ func (s *OnePasswordSDKSecretsProvider) GetSecret(key string) (string, error) { return "", fmt.Errorf("failed to resolve secret: %w", err) } + s.shell.RegisterSecret(value) return value, nil } diff --git a/pkg/secrets/op_sdk_secrets_provider_test.go b/pkg/secrets/op_sdk_secrets_provider_test.go index 6c1a59e75..189b74f9b 100644 --- a/pkg/secrets/op_sdk_secrets_provider_test.go +++ b/pkg/secrets/op_sdk_secrets_provider_test.go @@ -447,7 +447,14 @@ func TestOnePasswordSDKSecretsProvider_GetSecret(t *testing.T) { } func TestOnePasswordSDKSecretsProvider_ParseSecrets(t *testing.T) { - t.Run("Success", func(t *testing.T) { + // setup creates and initializes a OnePasswordSDKSecretsProvider for testing + setup := func(t *testing.T) (*Mocks, *OnePasswordSDKSecretsProvider) { + t.Helper() + + // Set environment variable + os.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "test-token") + t.Cleanup(func() { os.Unsetenv("OP_SERVICE_ACCOUNT_TOKEN") }) + // Setup mocks mocks := setupMocks(t) @@ -457,17 +464,20 @@ func TestOnePasswordSDKSecretsProvider_ParseSecrets(t *testing.T) { ID: "test-id", } - // Create the provider + // Create and initialize the provider provider := NewOnePasswordSDKSecretsProvider(vault, mocks.Injector) + err := provider.Initialize() + if err != nil { + t.Fatalf("Failed to initialize provider: %v", err) + } - // Set environment variable - os.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "test-token") - defer os.Unsetenv("OP_SERVICE_ACCOUNT_TOKEN") + return mocks, provider + } - // Set the provider to unlocked state + t.Run("Success", func(t *testing.T) { + // Given a provider with unlocked state and mock 1Password client configured + _, provider := setup(t) provider.unlocked = true - - // Set up the shims to use our mock provider.shims.NewOnePasswordClient = func(ctx context.Context, opts ...onepassword.ClientOption) (*onepassword.Client, error) { return &onepassword.Client{}, nil } @@ -475,173 +485,95 @@ func TestOnePasswordSDKSecretsProvider_ParseSecrets(t *testing.T) { return "secret-value", nil } - // Test with standard notation + // When parsing input containing a valid 1Password secret reference input := "This is a secret: ${{ op.test-id.test-secret.password }}" - expectedOutput := "This is a secret: secret-value" - output, err := provider.ParseSecrets(input) - // Verify the result + // Then the secret reference should be replaced with the actual secret value + expectedOutput := "This is a secret: secret-value" if err != nil { t.Errorf("Expected no error, got %v", err) } - if output != expectedOutput { t.Errorf("Expected output to be '%s', got '%s'", expectedOutput, output) } }) t.Run("EmptyInput", func(t *testing.T) { - // Setup mocks - mocks := setupMocks(t) - - // Create a test vault - vault := secretsConfigType.OnePasswordVault{ - Name: "test-vault", - ID: "test-id", - } - - // Create the provider - provider := NewOnePasswordSDKSecretsProvider(vault, mocks.Injector) - - // Set environment variable - os.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "test-token") - defer os.Unsetenv("OP_SERVICE_ACCOUNT_TOKEN") + // Given a provider with no special configuration + _, provider := setup(t) - // Test with empty input + // When parsing an empty input string input := "" output, err := provider.ParseSecrets(input) - // Verify the result + // Then the output should remain empty and unchanged if err != nil { t.Errorf("Expected no error, got %v", err) } - if output != input { t.Errorf("Expected output to be '%s', got '%s'", input, output) } }) t.Run("InvalidFormat", func(t *testing.T) { - // Setup mocks - mocks := setupMocks(t) - - // Create a test vault - vault := secretsConfigType.OnePasswordVault{ - Name: "test-vault", - ID: "test-id", - } + // Given a provider with no special configuration + _, provider := setup(t) - // Create the provider - provider := NewOnePasswordSDKSecretsProvider(vault, mocks.Injector) - - // Set environment variable - os.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "test-token") - defer os.Unsetenv("OP_SERVICE_ACCOUNT_TOKEN") - - // Test with invalid format (missing field) + // When parsing input with an invalid secret reference format (missing field) input := "This is a secret: ${{ op.test-id.test-secret }}" - expectedOutput := "This is a secret: " - output, err := provider.ParseSecrets(input) - // Verify the result + // Then the invalid reference should be replaced with an error message + expectedOutput := "This is a secret: " if err != nil { t.Errorf("Expected no error, got %v", err) } - if output != expectedOutput { t.Errorf("Expected output to be '%s', got '%s'", expectedOutput, output) } }) t.Run("MalformedJSON", func(t *testing.T) { - // Setup mocks - mocks := setupMocks(t) + // Given a provider with no special configuration + _, provider := setup(t) - // Create a test vault - vault := secretsConfigType.OnePasswordVault{ - Name: "test-vault", - ID: "test-id", - } - - // Create the provider - provider := NewOnePasswordSDKSecretsProvider(vault, mocks.Injector) - - // Set environment variable - os.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "test-token") - defer os.Unsetenv("OP_SERVICE_ACCOUNT_TOKEN") - - // Test with malformed JSON (missing closing brace) + // When parsing input with malformed JSON syntax (missing closing brace) input := "This is a secret: ${{ op.test-id.test-secret.password" - expectedOutput := "This is a secret: ${{ op.test-id.test-secret.password" - output, err := provider.ParseSecrets(input) - // Verify the result + // Then the malformed reference should be left unchanged + expectedOutput := "This is a secret: ${{ op.test-id.test-secret.password" if err != nil { t.Errorf("Expected no error, got %v", err) } - if output != expectedOutput { t.Errorf("Expected output to be '%s', got '%s'", expectedOutput, output) } }) t.Run("MismatchedVaultID", func(t *testing.T) { - // Setup mocks - mocks := setupMocks(t) + // Given a provider configured for vault "test-id" + _, provider := setup(t) - // Create a test vault - vault := secretsConfigType.OnePasswordVault{ - Name: "test-vault", - ID: "test-id", - } - - // Create the provider - provider := NewOnePasswordSDKSecretsProvider(vault, mocks.Injector) - - // Set environment variable - os.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "test-token") - defer os.Unsetenv("OP_SERVICE_ACCOUNT_TOKEN") - - // Test with wrong vault ID + // When parsing input with a secret reference for a different vault ID input := "This is a secret: ${{ op.wrong-id.test-secret.password }}" - expectedOutput := "This is a secret: ${{ op.wrong-id.test-secret.password }}" - output, err := provider.ParseSecrets(input) - // Verify the result + // Then the mismatched reference should be left unchanged + expectedOutput := "This is a secret: ${{ op.wrong-id.test-secret.password }}" if err != nil { t.Errorf("Expected no error, got %v", err) } - if output != expectedOutput { t.Errorf("Expected output to be '%s', got '%s'", expectedOutput, output) } }) t.Run("SecretNotFound", func(t *testing.T) { - // Setup mocks - mocks := setupMocks(t) - - // Create a test vault - vault := secretsConfigType.OnePasswordVault{ - Name: "test-vault", - ID: "test-id", - } - - // Create the provider - provider := NewOnePasswordSDKSecretsProvider(vault, mocks.Injector) - - // Set environment variable - os.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "test-token") - defer os.Unsetenv("OP_SERVICE_ACCOUNT_TOKEN") - - // Set the provider to unlocked state + // Given a provider with unlocked state and mock client that simulates secret not found + _, provider := setup(t) provider.unlocked = true - - // Set up the shims to use our mock provider.shims.NewOnePasswordClient = func(ctx context.Context, opts ...onepassword.ClientOption) (*onepassword.Client, error) { return &onepassword.Client{}, nil } @@ -649,17 +581,15 @@ func TestOnePasswordSDKSecretsProvider_ParseSecrets(t *testing.T) { return "", errors.New("secret not found") } - // Test with a secret that doesn't exist + // When parsing input with a reference to a nonexistent secret input := "This is a secret: ${{ op.test-id.nonexistent-secret.password }}" - expectedOutput := "This is a secret: " - output, err := provider.ParseSecrets(input) - // Verify the result + // Then the reference should be replaced with an error message indicating the secret was not found + expectedOutput := "This is a secret: " if err != nil { t.Errorf("Expected no error, got %v", err) } - if output != expectedOutput { t.Errorf("Expected output to be '%s', got '%s'", expectedOutput, output) } diff --git a/pkg/secrets/sops_secrets_provider.go b/pkg/secrets/sops_secrets_provider.go index 7b1ace00b..e758a7eff 100644 --- a/pkg/secrets/sops_secrets_provider.go +++ b/pkg/secrets/sops_secrets_provider.go @@ -10,9 +10,9 @@ import ( ) // The SopsSecretsProvider is an implementation of the SecretsProvider interface -// It provides integration with Mozilla SOPS for encrypted secrets management -// It serves as a bridge between the application and SOPS-encrypted YAML files -// It enables secure storage and retrieval of secrets using SOPS encryption +// It provides integration with Mozilla SOPS for encrypted secrets management with automatic shell scrubbing registration +// It serves as a bridge between the application and SOPS-encrypted YAML files with built-in security features +// It enables secure storage and retrieval of secrets using SOPS encryption while automatically registering secrets for output scrubbing // ============================================================================= // Constants @@ -116,13 +116,16 @@ func (s *SopsSecretsProvider) LoadSecrets() error { return nil } -// GetSecret retrieves a secret value for the specified key +// GetSecret retrieves a secret value for the specified key and automatically registers it with the shell for output scrubbing. +// If the provider is locked, it returns a masked value. When unlocked, it returns the actual secret value +// and registers it with the shell's scrubbing system to prevent accidental exposure in command output. func (s *SopsSecretsProvider) GetSecret(key string) (string, error) { if !s.unlocked { return "********", nil } if value, ok := s.secrets[key]; ok { + s.shell.RegisterSecret(value) return value, nil } diff --git a/pkg/shell/mock_shell.go b/pkg/shell/mock_shell.go index 668407f6d..5dec98656 100644 --- a/pkg/shell/mock_shell.go +++ b/pkg/shell/mock_shell.go @@ -35,6 +35,7 @@ type MockShell struct { GetSessionTokenFunc func() (string, error) CheckResetFlagsFunc func() (bool, error) ResetFunc func() + RegisterSecretFunc func(value string) } // ============================================================================= @@ -199,5 +200,12 @@ func (s *MockShell) Reset() { } } +// RegisterSecret calls the custom RegisterSecretFunc if provided. +func (s *MockShell) RegisterSecret(value string) { + if s.RegisterSecretFunc != nil { + s.RegisterSecretFunc(value) + } +} + // Ensure MockShell implements the Shell interface var _ Shell = (*MockShell)(nil) diff --git a/pkg/shell/mock_shell_test.go b/pkg/shell/mock_shell_test.go index 9fb2b1062..70126904c 100644 --- a/pkg/shell/mock_shell_test.go +++ b/pkg/shell/mock_shell_test.go @@ -1114,3 +1114,37 @@ func TestMockShell_Reset(t *testing.T) { mockShell.Reset() }) } + +func TestMockShell_RegisterSecret(t *testing.T) { + t.Run("CallsRegisterSecretFunc", func(t *testing.T) { + // Given a mock shell with RegisterSecretFunc configured to track calls + mockShell := setupMockShellMocks(t) + called := false + expectedValue := "test-secret-value" + var receivedValue string + mockShell.RegisterSecretFunc = func(value string) { + called = true + receivedValue = value + } + + // When RegisterSecret is called with a secret value + mockShell.RegisterSecret(expectedValue) + + // Then the mock function should be called with the expected secret value + if !called { + t.Error("Expected RegisterSecretFunc to be called") + } + if receivedValue != expectedValue { + t.Errorf("Expected value %v, got %v", expectedValue, receivedValue) + } + }) + + t.Run("NilFuncDoesNotPanic", func(t *testing.T) { + // Given a mock shell without RegisterSecretFunc configured + mockShell := setupMockShellMocks(t) + + // When RegisterSecret is called with no function set + // Then it should not panic and handle the nil function gracefully + mockShell.RegisterSecret("test-secret") + }) +} diff --git a/pkg/shell/scrubbing_writer.go b/pkg/shell/scrubbing_writer.go new file mode 100644 index 000000000..e86c4c0f8 --- /dev/null +++ b/pkg/shell/scrubbing_writer.go @@ -0,0 +1,37 @@ +package shell + +import "io" + +// scrubbingWriter wraps an io.Writer and scrubs secrets from content before writing +type scrubbingWriter struct { + writer io.Writer + scrubFunc func(string) string +} + +// Write scrubs secrets from content and writes to the underlying writer. +// If scrubbing changes the content length, it pads with spaces or truncates +// to maintain the original byte length for consistent output formatting. +func (sw *scrubbingWriter) Write(p []byte) (n int, err error) { + original := string(p) + scrubbed := sw.scrubFunc(original) + + if scrubbed != original { + scrubbedBytes := []byte(scrubbed) + originalLen := len(p) + scrubbedLen := len(scrubbedBytes) + + if scrubbedLen < originalLen { + padding := make([]byte, originalLen-scrubbedLen) + for i := range padding { + padding[i] = ' ' + } + scrubbedBytes = append(scrubbedBytes, padding...) + } else if scrubbedLen > originalLen { + scrubbedBytes = scrubbedBytes[:originalLen] + } + + return sw.writer.Write(scrubbedBytes) + } + + return sw.writer.Write(p) +} diff --git a/pkg/shell/shell.go b/pkg/shell/shell.go index b2bf2abbf..fea4ed230 100644 --- a/pkg/shell/shell.go +++ b/pkg/shell/shell.go @@ -7,6 +7,7 @@ import ( "os" "path" "path/filepath" + "slices" "strings" "sync" @@ -17,9 +18,9 @@ import ( ) // The Shell package is a unified interface for shell operations across different platforms. -// It provides cross-platform command execution, environment variable management, and session token handling. -// This package serves as the core interface for all shell operations in the Windsor CLI. -// Key features include command execution, environment variable management, and session token handling. +// It provides cross-platform command execution, environment variable management, session token handling, and secret scrubbing. +// This package serves as the core interface for all shell operations in the Windsor CLI with built-in security features. +// Key features include command execution, environment variable management, session token handling, and automatic secret scrubbing from command output. // ============================================================================= // Constants @@ -61,6 +62,7 @@ type Shell interface { GetSessionToken() (string, error) CheckResetFlags() (bool, error) Reset() + RegisterSecret(value string) } // DefaultShell is the default implementation of the Shell interface @@ -71,6 +73,7 @@ type DefaultShell struct { verbose bool sessionToken string shims *Shims + secrets []string } // ============================================================================= @@ -143,11 +146,16 @@ func (s *DefaultShell) GetProjectRoot() (string, error) { // Exec runs a command with args, capturing stdout and stderr. It prints output and returns stdout as a string. // If the command is "sudo", it connects stdin to the terminal for password input. +// All output is scrubbed to remove registered secrets before being displayed or returned. func (s *DefaultShell) Exec(command string, args ...string) (string, error) { cmd := s.shims.Command(command, args...) var stdoutBuf, stderrBuf bytes.Buffer - cmd.Stdout = io.MultiWriter(os.Stdout, &stdoutBuf) - cmd.Stderr = io.MultiWriter(os.Stderr, &stderrBuf) + + scrubbingStdoutWriter := &scrubbingWriter{writer: os.Stdout, scrubFunc: s.scrubString} + scrubbingStderrWriter := &scrubbingWriter{writer: os.Stderr, scrubFunc: s.scrubString} + + cmd.Stdout = io.MultiWriter(scrubbingStdoutWriter, &stdoutBuf) + cmd.Stderr = io.MultiWriter(scrubbingStderrWriter, &stderrBuf) if command == "sudo" { cmd.Stdin = os.Stdin } @@ -155,9 +163,9 @@ func (s *DefaultShell) Exec(command string, args ...string) (string, error) { return stdoutBuf.String(), fmt.Errorf("command start failed: %w", err) } if err := s.shims.CmdWait(cmd); err != nil { - return stdoutBuf.String(), fmt.Errorf("command execution failed: %w", err) + return s.scrubString(stdoutBuf.String()), fmt.Errorf("command execution failed: %w", err) } - return stdoutBuf.String(), nil + return s.scrubString(stdoutBuf.String()), nil } // ExecSudo runs a command with 'sudo', ensuring elevated privileges. It handles password prompts by @@ -205,7 +213,7 @@ func (s *DefaultShell) ExecSudo(message string, command string, args ...string) fmt.Fprintf(os.Stderr, "\033[32m✔\033[0m %s - \033[32mDone\033[0m\n", message) - return stdoutBuf.String(), nil + return s.scrubString(stdoutBuf.String()), nil } // ExecSilent is a method that runs a command quietly, capturing its output. @@ -225,10 +233,10 @@ func (s *DefaultShell) ExecSilent(command string, args ...string) (string, error cmd.Stderr = &stderrBuf if err := s.shims.CmdRun(cmd); err != nil { - return stdoutBuf.String(), fmt.Errorf("command execution failed: %w\n%s", err, stderrBuf.String()) + return s.scrubString(stdoutBuf.String()), fmt.Errorf("command execution failed: %w\n%s", err, s.scrubString(stderrBuf.String())) } - return stdoutBuf.String(), nil + return s.scrubString(stdoutBuf.String()), nil } // ExecProgress is a method of the DefaultShell struct that executes a command with a progress indicator. @@ -323,15 +331,15 @@ func (s *DefaultShell) ExecProgress(message string, command string, args ...stri cmdErr := s.shims.CmdWait(cmd) if firstErr != nil || cmdErr != nil { - fmt.Fprintf(os.Stderr, "\n[ExecProgress ERROR]\nCommand: %s %v\nStdout:\n%s\nStderr:\n%s\nError: %v\n", command, args, stdoutBuf.String(), stderrBuf.String(), firstErr) + fmt.Fprintf(os.Stderr, "\n[ExecProgress ERROR]\nCommand: %s %v\nStdout:\n%s\nStderr:\n%s\nError: %v\n", command, s.scrubString(fmt.Sprintf("%v", args)), s.scrubString(stdoutBuf.String()), s.scrubString(stderrBuf.String()), firstErr) if cmdErr != nil { - return stdoutBuf.String(), fmt.Errorf("command execution failed: %w", cmdErr) + return s.scrubString(stdoutBuf.String()), fmt.Errorf("command execution failed: %w", cmdErr) } - return stdoutBuf.String(), firstErr + return s.scrubString(stdoutBuf.String()), firstErr } fmt.Fprintf(os.Stderr, "\033[32m✔\033[0m %s - \033[32mDone\033[0m\n", message) - return stdoutBuf.String(), nil + return s.scrubString(stdoutBuf.String()), nil } // InstallHook sets up a shell hook for a specified shell using a template with the Windsor path. @@ -496,7 +504,6 @@ func (s *DefaultShell) WriteResetToken() (string, error) { // GetSessionToken retrieves or generates a session token. It first checks if a token is already stored in memory. // If not, it looks for a token in the environment variable. If no token is found in the environment, it generates a new token. func (s *DefaultShell) GetSessionToken() (string, error) { - // If we already have a token in memory, return it if s.sessionToken != "" { return s.sessionToken, nil } @@ -516,6 +523,21 @@ func (s *DefaultShell) GetSessionToken() (string, error) { return token, nil } +// RegisterSecret adds a secret value to the internal list of secrets that will be scrubbed from all command output. +// Empty strings are ignored to prevent unnecessary processing. +// Duplicate values are automatically filtered out to maintain list efficiency. +func (s *DefaultShell) RegisterSecret(value string) { + if value == "" { + return + } + + if slices.Contains(s.secrets, value) { + return + } + + s.secrets = append(s.secrets, value) +} + // Reset removes all managed environment variables and aliases. // It uses the environment variables "WINDSOR_MANAGED_ENV" and "WINDSOR_MANAGED_ALIAS" // to retrieve the previous set of managed environment variables and aliases, respectively. @@ -587,6 +609,11 @@ func (s *DefaultShell) CheckResetFlags() (bool, error) { return tokenFileExists, nil } +// ResetSessionToken resets the session token - used primarily for testing +func (s *DefaultShell) ResetSessionToken() { + s.sessionToken = "" +} + // ============================================================================= // Private Methods // ============================================================================= @@ -604,13 +631,18 @@ func (s *DefaultShell) generateRandomString(length int) (string, error) { return string(b), nil } -// ============================================================================= -// Public Functions -// ============================================================================= +// scrubString replaces all registered secret values with fixed "********" strings for security. +// It processes the input string and replaces any occurrence of registered secrets with asterisks. +// This method is used internally by all command execution methods to prevent secret leakage in output. +func (s *DefaultShell) scrubString(input string) string { + result := input + for _, secret := range s.secrets { + if secret != "" { + result = strings.ReplaceAll(result, secret, "********") + } + } -// ResetSessionToken resets the session token - used primarily for testing -func (s *DefaultShell) ResetSessionToken() { - s.sessionToken = "" + return result } // Ensure DefaultShell implements the Shell interface diff --git a/pkg/shell/shell_test.go b/pkg/shell/shell_test.go index bf7155a74..9497a8ab9 100644 --- a/pkg/shell/shell_test.go +++ b/pkg/shell/shell_test.go @@ -68,10 +68,8 @@ func setupMocks(t *testing.T) *Mocks { // Mock command execution methods with proper cleanup shims.CmdStart = func(cmd *exec.Cmd) error { if cmd.Stdout != nil { - if w, ok := cmd.Stdout.(io.Writer); ok { - if _, err := w.Write([]byte("test\n")); err != nil { - return fmt.Errorf("failed to write to stdout: %v", err) - } + if _, err := cmd.Stdout.Write([]byte("test\n")); err != nil { + return fmt.Errorf("failed to write to stdout: %v", err) } } return nil @@ -83,10 +81,8 @@ func setupMocks(t *testing.T) *Mocks { shims.CmdRun = func(cmd *exec.Cmd) error { if cmd.Stdout != nil { - if w, ok := cmd.Stdout.(io.Writer); ok { - if _, err := w.Write([]byte("test\n")); err != nil { - return fmt.Errorf("failed to write to stdout: %v", err) - } + if _, err := cmd.Stdout.Write([]byte("test\n")); err != nil { + return fmt.Errorf("failed to write to stdout: %v", err) } } return nil @@ -409,8 +405,8 @@ func TestShell_Exec(t *testing.T) { return cmd } mocks.Shims.CmdStart = func(cmd *exec.Cmd) error { - if w, ok := cmd.Stdout.(io.Writer); ok { - if _, err := w.Write([]byte("test\n")); err != nil { + if cmd.Stdout != nil { + if _, err := cmd.Stdout.Write([]byte("test\n")); err != nil { return fmt.Errorf("failed to write to stdout: %v", err) } } @@ -519,10 +515,8 @@ func TestShell_ExecSudo(t *testing.T) { // Mock successful command execution mocks.Shims.CmdStart = func(cmd *exec.Cmd) error { if cmd.Stdout != nil { - if w, ok := cmd.Stdout.(io.Writer); ok { - if _, err := w.Write([]byte("test output")); err != nil { - return fmt.Errorf("failed to write to stdout: %v", err) - } + if _, err := cmd.Stdout.Write([]byte("test output")); err != nil { + return fmt.Errorf("failed to write to stdout: %v", err) } } return nil @@ -562,10 +556,8 @@ func TestShell_ExecSudo(t *testing.T) { // Mock successful command execution mocks.Shims.CmdStart = func(cmd *exec.Cmd) error { if cmd.Stdout != nil { - if w, ok := cmd.Stdout.(io.Writer); ok { - if _, err := w.Write([]byte("test\n")); err != nil { - return fmt.Errorf("failed to write to stdout: %v", err) - } + if _, err := cmd.Stdout.Write([]byte("test\n")); err != nil { + return fmt.Errorf("failed to write to stdout: %v", err) } } return nil @@ -1348,25 +1340,6 @@ func (m *mockReadCloser) Close() error { // Test Helpers // ============================================================================= -// Helper function to create a temporary directory for testing -func createTempDir(t *testing.T, prefix string) string { - t.Helper() - dir, err := os.MkdirTemp("", prefix) - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - return dir -} - -// Helper function to create a file in a directory -func createFile(t *testing.T, dir, name, content string) { - t.Helper() - path := filepath.Join(dir, name) - if err := os.WriteFile(path, []byte(content), 0644); err != nil { - t.Fatalf("Failed to create file %s: %v", path, err) - } -} - // Helper function to capture stdout func captureStdout(t *testing.T, f func()) string { var output bytes.Buffer @@ -2334,8 +2307,8 @@ func TestShell_ExecProgress(t *testing.T) { // Mock command start to write output mocks.Shims.CmdStart = func(cmd *exec.Cmd) error { - if w, ok := cmd.Stdout.(io.Writer); ok { - w.Write([]byte(expectedOutput)) + if cmd.Stdout != nil { + cmd.Stdout.Write([]byte(expectedOutput)) } return nil } @@ -2739,8 +2712,8 @@ func TestShell_ExecProgress(t *testing.T) { // Mock command start to write output mocks.Shims.CmdStart = func(cmd *exec.Cmd) error { - if w, ok := cmd.Stdout.(io.Writer); ok { - w.Write([]byte(expectedOutput)) + if cmd.Stdout != nil { + cmd.Stdout.Write([]byte(expectedOutput)) } return nil } @@ -2801,3 +2774,417 @@ func TestShell_ExecProgress(t *testing.T) { } }) } + +// ============================================================================= +// Secret Management Tests +// ============================================================================= + +func TestShell_RegisterSecret(t *testing.T) { + // setup creates a new shell with mocked dependencies for testing + setup := func(t *testing.T) (*DefaultShell, *Mocks) { + t.Helper() + mocks := setupMocks(t) + shell := NewDefaultShell(mocks.Injector) + shell.Initialize() + return shell, mocks + } + + t.Run("RegisterSingleSecret", func(t *testing.T) { + // Given a shell instance with no registered secrets + shell, _ := setup(t) + + // When registering a single secret value + shell.RegisterSecret("mysecret123") + + // Then the secret should be stored in the secrets list + if len(shell.secrets) != 1 { + t.Errorf("Expected 1 secret, got %d", len(shell.secrets)) + } + if shell.secrets[0] != "mysecret123" { + t.Errorf("Expected secret 'mysecret123', got '%s'", shell.secrets[0]) + } + }) + + t.Run("RegisterMultipleSecrets", func(t *testing.T) { + // Given a shell instance with no registered secrets + shell, _ := setup(t) + + // When registering multiple different secret values + shell.RegisterSecret("secret1") + shell.RegisterSecret("secret2") + shell.RegisterSecret("secret3") + + // Then all secrets should be stored in the secrets list + if len(shell.secrets) != 3 { + t.Errorf("Expected 3 secrets, got %d", len(shell.secrets)) + } + expectedSecrets := []string{"secret1", "secret2", "secret3"} + for i, expected := range expectedSecrets { + if shell.secrets[i] != expected { + t.Errorf("Expected secret[%d] to be '%s', got '%s'", i, expected, shell.secrets[i]) + } + } + }) + + t.Run("RegisterEmptySecret", func(t *testing.T) { + // Given a shell instance with no registered secrets + shell, _ := setup(t) + + // When attempting to register an empty string as a secret + shell.RegisterSecret("") + + // Then the empty secret should be ignored and not stored + if len(shell.secrets) != 0 { + t.Errorf("Expected 0 secrets, got %d", len(shell.secrets)) + } + }) + + t.Run("RegisterDuplicateSecrets", func(t *testing.T) { + // Given a shell instance with no registered secrets + shell, _ := setup(t) + + // When registering the same secret value multiple times + shell.RegisterSecret("duplicate") + shell.RegisterSecret("duplicate") + shell.RegisterSecret("duplicate") + + // Then only one instance should be stored to prevent duplicates + if len(shell.secrets) != 1 { + t.Errorf("Expected 1 secret, got %d", len(shell.secrets)) + } + if shell.secrets[0] != "duplicate" { + t.Errorf("Expected secret to be 'duplicate', got '%s'", shell.secrets[0]) + } + }) +} + +func TestShell_scrubString(t *testing.T) { + // setup creates a new shell with mocked dependencies for testing + setup := func(t *testing.T) (*DefaultShell, *Mocks) { + t.Helper() + mocks := setupMocks(t) + shell := NewDefaultShell(mocks.Injector) + shell.Initialize() + return shell, mocks + } + + t.Run("ScrubSingleSecret", func(t *testing.T) { + // Given a shell with one registered secret value + shell, _ := setup(t) + shell.RegisterSecret("mysecret123") + + // When scrubbing a string that contains the registered secret + input := "The password is mysecret123 and it's confidential" + result := shell.scrubString(input) + + // Then the secret should be replaced with asterisks while preserving the rest of the text + expected := "The password is ******** and it's confidential" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubMultipleSecrets", func(t *testing.T) { + // Given a shell with multiple registered secret values + shell, _ := setup(t) + shell.RegisterSecret("secret1") + shell.RegisterSecret("secret2") + shell.RegisterSecret("secret3") + + // When scrubbing a string that contains multiple registered secrets + input := "First secret1, then secret2, finally secret3" + result := shell.scrubString(input) + + // Then all secrets should be replaced with asterisks + expected := "First ********, then ********, finally ********" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubNoSecrets", func(t *testing.T) { + // Given a shell with no registered secrets + shell, _ := setup(t) + + // When scrubbing a string that contains no sensitive information + input := "This string contains no secrets" + result := shell.scrubString(input) + + // Then the string should remain completely unchanged + if result != input { + t.Errorf("Expected unchanged string '%s', got '%s'", input, result) + } + }) + + t.Run("ScrubEmptyString", func(t *testing.T) { + // Given a shell with registered secrets + shell, _ := setup(t) + shell.RegisterSecret("secret") + + // When scrubbing an empty input string + input := "" + result := shell.scrubString(input) + + // Then the result should remain empty + if result != "" { + t.Errorf("Expected empty string, got '%s'", result) + } + }) + + t.Run("ScrubEmptySecret", func(t *testing.T) { + // Given a shell with an empty string registered as a secret + shell, _ := setup(t) + shell.RegisterSecret("") + + // When scrubbing a normal text string + input := "This is a test string" + result := shell.scrubString(input) + + // Then the string should remain unchanged since empty secrets don't match anything + if result != input { + t.Errorf("Expected unchanged string '%s', got '%s'", input, result) + } + }) + + t.Run("ScrubSecretAtBeginning", func(t *testing.T) { + // Given a shell with a registered secret value + shell, _ := setup(t) + shell.RegisterSecret("secret123") + + // When scrubbing a string where the secret appears at the beginning + input := "secret123 is at the start" + result := shell.scrubString(input) + + // Then the secret should be replaced with asterisks + expected := "******** is at the start" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubSecretAtEnd", func(t *testing.T) { + // Given a shell with a registered secret value + shell, _ := setup(t) + shell.RegisterSecret("secret123") + + // When scrubbing a string where the secret appears at the end + input := "The secret is secret123" + result := shell.scrubString(input) + + // Then the secret should be replaced with asterisks + expected := "The secret is ********" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubSecretMultipleOccurrences", func(t *testing.T) { + // Given a shell with a registered secret value + shell, _ := setup(t) + shell.RegisterSecret("secret") + + // When scrubbing a string where the same secret appears multiple times + input := "secret appears here and secret appears there, secret everywhere" + result := shell.scrubString(input) + + // Then all occurrences of the secret should be replaced with asterisks + expected := "******** appears here and ******** appears there, ******** everywhere" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubSecretPartialMatch", func(t *testing.T) { + // Given a shell with a registered secret value + shell, _ := setup(t) + shell.RegisterSecret("secret") + + // When scrubbing a string containing words that partially match the secret + input := "secretive and secrets contain secret" + result := shell.scrubString(input) + + // Then all occurrences should be replaced using simple string replacement + expected := "********ive and ********s contain ********" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubLongSecret", func(t *testing.T) { + // Given a shell with a very long secret value registered + shell, _ := setup(t) + longSecret := "this-is-a-very-long-secret-key-with-many-characters-1234567890" + shell.RegisterSecret(longSecret) + + // When scrubbing a string containing the long secret + input := fmt.Sprintf("The key is %s and should be hidden", longSecret) + result := shell.scrubString(input) + + // Then the long secret should be replaced with a fixed-length asterisk string + expected := "The key is ******** and should be hidden" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubSpecialCharacters", func(t *testing.T) { + // Given a shell with a secret containing special characters + shell, _ := setup(t) + specialSecret := "p@ssw0rd!#$%^&*()" + shell.RegisterSecret(specialSecret) + + // When scrubbing a string containing the special character secret + input := fmt.Sprintf("Password: %s", specialSecret) + result := shell.scrubString(input) + + // Then the special character secret should be replaced with asterisks + expected := "Password: ********" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubComplexMultipleSecrets", func(t *testing.T) { + // Given a shell with multiple different registered secrets + shell, _ := setup(t) + shell.RegisterSecret("secret123") + shell.RegisterSecret("password456") + shell.RegisterSecret("token789") + + // When scrubbing a complex configuration-like string with multiple secrets + input := "Config: secret123, Auth: password456, API: token789, Normal: text" + result := shell.scrubString(input) + + // Then all secrets should be scrubbed while preserving non-secret text + expected := "Config: ********, Auth: ********, API: ********, Normal: text" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubSecretsInJSON", func(t *testing.T) { + // Given a shell with registered password and token secrets + shell, _ := setup(t) + shell.RegisterSecret("mypassword") + shell.RegisterSecret("mytoken") + + // When scrubbing a JSON-formatted string containing secrets + input := `{"password": "mypassword", "token": "mytoken", "user": "admin"}` + result := shell.scrubString(input) + + // Then the secrets should be scrubbed while preserving JSON structure + expected := `{"password": "********", "token": "********", "user": "admin"}` + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubSecretsInCommandLine", func(t *testing.T) { + // Given a shell with a registered secret value + shell, _ := setup(t) + shell.RegisterSecret("supersecret") + + // When scrubbing a command line string containing the secret + input := "terraform apply -var password=supersecret -var user=admin" + result := shell.scrubString(input) + + // Then the secret should be scrubbed while preserving the command structure + expected := "terraform apply -var password=******** -var user=admin" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) + + t.Run("ScrubSecretsInErrorMessages", func(t *testing.T) { + // Given a shell with a registered password secret + shell, _ := setup(t) + shell.RegisterSecret("badpassword") + + // When scrubbing an error message that accidentally contains the secret + input := "Error: authentication failed with password 'badpassword'" + result := shell.scrubString(input) + + // Then the secret should be scrubbed to prevent leakage in error logs + expected := "Error: authentication failed with password '********'" + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } + }) +} + +func TestScrubbingWriter(t *testing.T) { + setup := func(t *testing.T) (*DefaultShell, *bytes.Buffer) { + t.Helper() + mocks := setupMocks(t) + shell := NewDefaultShell(mocks.Injector) + shell.shims = mocks.Shims + + // Register a test secret + shell.RegisterSecret("secret123") + + // Create a buffer to capture output + var buf bytes.Buffer + return shell, &buf + } + + t.Run("ScrubsSecretsFromOutput", func(t *testing.T) { + // Given a shell with registered secrets and a scrubbing writer configured + shell, buf := setup(t) + writer := &scrubbingWriter{writer: buf, scrubFunc: shell.scrubString} + + // When writing content that contains registered secrets to the scrubbing writer + testContent := "This contains secret123 and other text" + n, err := writer.Write([]byte(testContent)) + + // Then the secrets should be scrubbed from the output while maintaining byte count consistency + if err != nil { + t.Errorf("Expected no error, got: %v", err) + } + + output := buf.String() + if strings.Contains(output, "secret123") { + t.Errorf("Expected secret to be scrubbed, but found in output: %s", output) + } + // Should replace with fixed ******** and pad to maintain length + if !strings.Contains(output, "********") { + t.Errorf("Expected scrubbed output to contain ********, got: %s", output) + } + // Byte counts should match due to padding + if n != len(testContent) { + t.Errorf("Expected bytes written to match content length %d, got %d", len(testContent), n) + } + // Output length should match input length due to padding + if len(output) != len(testContent) { + t.Errorf("Expected output length %d to match input length %d", len(output), len(testContent)) + } + }) + + t.Run("HandlesMultipleSecrets", func(t *testing.T) { + // Given a shell with multiple registered secrets and a scrubbing writer + shell, buf := setup(t) + shell.RegisterSecret("password456") + writer := &scrubbingWriter{writer: buf, scrubFunc: shell.scrubString} + + // When writing content that contains multiple different secrets + testContent := "User secret123 has password456 for access" + _, err := writer.Write([]byte(testContent)) + + // Then all secrets should be scrubbed while maintaining output length consistency + if err != nil { + t.Errorf("Expected no error, got: %v", err) + } + + output := buf.String() + if strings.Contains(output, "secret123") || strings.Contains(output, "password456") { + t.Errorf("Expected all secrets to be scrubbed, got: %s", output) + } + // Should contain fixed ******** replacements + if !strings.Contains(output, "********") { + t.Errorf("Expected scrubbed output to contain ********, got: %s", output) + } + // Output length should match input due to padding + if len(output) != len(testContent) { + t.Errorf("Expected output length %d to match input length %d", len(output), len(testContent)) + } + }) +} diff --git a/pkg/shell/unix_shell_test.go b/pkg/shell/unix_shell_test.go index 2bf026b97..da248b02d 100644 --- a/pkg/shell/unix_shell_test.go +++ b/pkg/shell/unix_shell_test.go @@ -153,8 +153,8 @@ func TestDefaultShell_PrintAlias(t *testing.T) { }) // Then the output should contain the expected alias and unalias commands - expectedAliasLine := fmt.Sprintf("alias ALIAS1=\"command1\"\n") - expectedUnaliasLine := fmt.Sprintf("unalias ALIAS2\n") + expectedAliasLine := "alias ALIAS1=\"command1\"\n" + expectedUnaliasLine := "unalias ALIAS2\n" if !strings.Contains(output, expectedAliasLine) { t.Errorf("PrintAlias() output missing expected line: %v", expectedAliasLine)