diff --git a/pkg/secrets/op_cli_secrets_provider.go b/pkg/secrets/op_cli_secrets_provider.go index 9bcc84a1a..342f7b0eb 100644 --- a/pkg/secrets/op_cli_secrets_provider.go +++ b/pkg/secrets/op_cli_secrets_provider.go @@ -9,18 +9,17 @@ import ( ) // The OnePasswordCLISecretsProvider is an implementation of the SecretsProvider interface -// 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 +// It provides integration with 1Password CLI for secrets management with automatic shell scrubbing registration +// It serves as a bridge between the application and 1Password CLI with built-in security features +// It enables secure storage and retrieval of secrets using 1Password while automatically registering secrets for output scrubbing // ============================================================================= // Types // ============================================================================= -// OnePasswordCLISecretsProvider is an implementation of the SecretsProvider interface -// that uses the 1Password CLI to manage secrets. +// OnePasswordCLISecretsProvider is a struct that implements the SecretsProvider interface using 1Password CLI. type OnePasswordCLISecretsProvider struct { - BaseSecretsProvider + *BaseSecretsProvider vault secretsConfigType.OnePasswordVault } @@ -31,7 +30,7 @@ type OnePasswordCLISecretsProvider struct { // NewOnePasswordCLISecretsProvider creates a new OnePasswordCLISecretsProvider instance func NewOnePasswordCLISecretsProvider(vault secretsConfigType.OnePasswordVault, injector di.Injector) *OnePasswordCLISecretsProvider { return &OnePasswordCLISecretsProvider{ - BaseSecretsProvider: *NewBaseSecretsProvider(injector), + BaseSecretsProvider: NewBaseSecretsProvider(injector), vault: vault, } } @@ -40,9 +39,9 @@ func NewOnePasswordCLISecretsProvider(vault secretsConfigType.OnePasswordVault, // Public Methods // ============================================================================= -// 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. +// GetSecret retrieves a secret value for the given key using 1Password CLI. Registers the secret for shell scrubbing to +// prevent exposure in output. Executes the CLI directly to avoid leaking secrets before registration. If locked, returns +// a masked value. Key format: 'secret.field'. Returns the secret or error on failure. func (s *OnePasswordCLISecretsProvider) GetSecret(key string) (string, error) { if !s.unlocked { return "********", nil @@ -54,8 +53,8 @@ func (s *OnePasswordCLISecretsProvider) GetSecret(key string) (string, error) { } args := []string{"item", "get", parts[0], "--vault", s.vault.Name, "--fields", parts[1], "--reveal", "--account", s.vault.URL} - - output, err := s.shell.ExecSilent("op", args...) + cmd := s.shims.Command("op", args...) + output, err := s.shims.CmdOutput(cmd) if err != nil { return "", fmt.Errorf("failed to retrieve secret from 1Password: %w", err) } diff --git a/pkg/secrets/op_cli_secrets_provider_test.go b/pkg/secrets/op_cli_secrets_provider_test.go index 321a05e95..be5abc600 100644 --- a/pkg/secrets/op_cli_secrets_provider_test.go +++ b/pkg/secrets/op_cli_secrets_provider_test.go @@ -7,6 +7,8 @@ package secrets import ( "errors" + "os/exec" + "strings" "testing" secretsConfigType "github.com/windsorcli/cli/api/v1alpha1/secrets" @@ -65,11 +67,12 @@ func TestOnePasswordCLISecretsProvider_GetSecret(t *testing.T) { } provider.unlocked = true - // And a mock shell that returns a successful result - mocks.Shell.ExecSilentFunc = func(command string, args ...string) (string, error) { + // And mock shims for command execution + mockShims := NewShims() + mockShims.Command = func(name string, args ...string) *exec.Cmd { // Verify the command and arguments - if command != "op" { - t.Errorf("Expected command to be 'op', got %s", command) + if name != "op" { + t.Errorf("Expected command to be 'op', got %s", name) } // Check that the arguments contain the expected values @@ -84,9 +87,13 @@ func TestOnePasswordCLISecretsProvider_GetSecret(t *testing.T) { } } - // Return a successful result - return "secret-value", nil + // Return a mock command + return &exec.Cmd{} + } + mockShims.CmdOutput = func(cmd *exec.Cmd) ([]byte, error) { + return []byte("secret-value"), nil } + provider.shims = mockShims // When GetSecret is called value, err := provider.GetSecret("test-secret.password") @@ -190,10 +197,15 @@ func TestOnePasswordCLISecretsProvider_GetSecret(t *testing.T) { } provider.unlocked = true - // And a mock shell that returns an error - mocks.Shell.ExecSilentFunc = func(command string, args ...string) (string, error) { - return "", errors.New("command execution error") + // And mock shims that return an error + mockShims := NewShims() + mockShims.Command = func(name string, args ...string) *exec.Cmd { + return &exec.Cmd{} + } + mockShims.CmdOutput = func(cmd *exec.Cmd) ([]byte, error) { + return nil, errors.New("command execution error") } + provider.shims = mockShims // When GetSecret is called value, err := provider.GetSecret("test-secret.password") @@ -203,10 +215,12 @@ func TestOnePasswordCLISecretsProvider_GetSecret(t *testing.T) { t.Error("Expected an error, got nil") } - // And the error message should be correct - expectedError := "failed to retrieve secret from 1Password: command execution error" - if err.Error() != expectedError { - t.Errorf("Expected error to be '%s', got '%s'", expectedError, err.Error()) + // And the error message should contain the expected text + if !strings.Contains(err.Error(), "failed to retrieve secret from 1Password") { + t.Errorf("Expected error to contain 'failed to retrieve secret from 1Password', got '%s'", err.Error()) + } + if !strings.Contains(err.Error(), "command execution error") { + t.Errorf("Expected error to contain 'command execution error', got '%s'", err.Error()) } // And the value should be empty @@ -236,10 +250,15 @@ func TestParseSecrets(t *testing.T) { } provider.unlocked = true - // And a mock shell that returns a successful result - mocks.Shell.ExecSilentFunc = func(command string, args ...string) (string, error) { - return "secret-value", nil + // And mock shims for command execution + mockShims := NewShims() + mockShims.Command = func(name string, args ...string) *exec.Cmd { + return &exec.Cmd{} + } + mockShims.CmdOutput = func(cmd *exec.Cmd) ([]byte, error) { + return []byte("secret-value"), nil } + provider.shims = mockShims // When ParseSecrets is called with standard notation input := "This is a secret: ${{ op.test-id.test-secret.password }}" diff --git a/pkg/secrets/shims.go b/pkg/secrets/shims.go index f7ff7c071..aff3f1bad 100644 --- a/pkg/secrets/shims.go +++ b/pkg/secrets/shims.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "os/exec" "github.com/1password/onepassword-sdk-go" "github.com/getsops/sops/v3/decrypt" @@ -26,6 +27,8 @@ type Shims struct { DecryptFile func(string, string) ([]byte, error) NewOnePasswordClient func(context.Context, ...onepassword.ClientOption) (*onepassword.Client, error) ResolveSecret func(*onepassword.Client, context.Context, string) (string, error) + Command func(name string, arg ...string) *exec.Cmd + CmdOutput func(cmd *exec.Cmd) ([]byte, error) } // ============================================================================= @@ -47,5 +50,9 @@ func NewShims() *Shims { } return client.Secrets().Resolve(ctx, secretRef) }, + Command: exec.Command, + CmdOutput: func(cmd *exec.Cmd) ([]byte, error) { + return cmd.Output() + }, } }