From 7072cb2038ea38741a899abf88f5d648ddcd4df0 Mon Sep 17 00:00:00 2001 From: thibaud-lclr Date: Mon, 20 Apr 2026 09:39:05 +0200 Subject: [PATCH] feat(secretstore): harden bitwarden readiness and secret verification --- cli/doctor.go | 54 ++++++ cli/doctor_test.go | 56 ++++++ docs/cli-helpers.md | 12 ++ docs/secrets.md | 34 ++++ scaffold/scaffold.go | 87 ++++++++-- secretstore/bitwarden.go | 246 +++++++++++++++++++++++--- secretstore/bitwarden_test.go | 317 ++++++++++++++++++++++++++++++---- secretstore/store.go | 37 ++++ secretstore/store_test.go | 85 +++++++++ 9 files changed, 859 insertions(+), 69 deletions(-) diff --git a/cli/doctor.go b/cli/doctor.go index 27b15a8..ec859e7 100644 --- a/cli/doctor.go +++ b/cli/doctor.go @@ -60,6 +60,13 @@ type DoctorOptions struct { ExtraChecks []DoctorCheck } +type BitwardenDoctorOptions struct { + Command string + LookupEnv func(string) (string, bool) +} + +var checkBitwardenReady = secretstore.EnsureBitwardenReady + func RunDoctor(ctx context.Context, options DoctorOptions) DoctorReport { checks := make([]DoctorCheck, 0, 5+len(options.ExtraChecks)) @@ -220,6 +227,53 @@ func SecretStoreAvailabilityCheck(factory func() (secretstore.Store, error)) Doc } } +func BitwardenReadyCheck(options BitwardenDoctorOptions) DoctorCheck { + return func(context.Context) DoctorResult { + err := checkBitwardenReady(secretstore.Options{ + BitwardenCommand: strings.TrimSpace(options.Command), + LookupEnv: options.LookupEnv, + }) + if err == nil { + return DoctorResult{ + Name: "bitwarden", + Status: DoctorStatusOK, + Summary: "bitwarden CLI is ready", + } + } + + switch { + case errors.Is(err, secretstore.ErrBWNotLoggedIn): + return DoctorResult{ + Name: "bitwarden", + Status: DoctorStatusFail, + Summary: "bitwarden login is required", + Detail: err.Error(), + } + case errors.Is(err, secretstore.ErrBWLocked): + return DoctorResult{ + Name: "bitwarden", + Status: DoctorStatusFail, + Summary: "bitwarden vault is locked or BW_SESSION is missing", + Detail: err.Error(), + } + case errors.Is(err, secretstore.ErrBWUnavailable): + return DoctorResult{ + Name: "bitwarden", + Status: DoctorStatusFail, + Summary: "bitwarden CLI is unavailable", + Detail: err.Error(), + } + default: + return DoctorResult{ + Name: "bitwarden", + Status: DoctorStatusFail, + Summary: "bitwarden readiness check failed", + Detail: err.Error(), + } + } + } +} + func RequiredSecretsCheck(factory func() (secretstore.Store, error), required []DoctorSecret) DoctorCheck { return func(context.Context) DoctorResult { store, err := factory() diff --git a/cli/doctor_test.go b/cli/doctor_test.go index 04aeb99..698cd74 100644 --- a/cli/doctor_test.go +++ b/cli/doctor_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "fmt" "os" "path/filepath" "strings" @@ -197,6 +198,61 @@ func TestSecretStoreAvailabilityCheckFailsWhenFactoryFails(t *testing.T) { } } +func TestBitwardenReadyCheckMapsTypedErrorsToActionableDiagnostics(t *testing.T) { + prev := checkBitwardenReady + t.Cleanup(func() { + checkBitwardenReady = prev + }) + + t.Run("not logged in", func(t *testing.T) { + checkBitwardenReady = func(options secretstore.Options) error { + return fmt.Errorf("%w: run `bw login`", secretstore.ErrBWNotLoggedIn) + } + + result := BitwardenReadyCheck(BitwardenDoctorOptions{})(context.Background()) + if result.Status != DoctorStatusFail { + t.Fatalf("status = %q, want fail", result.Status) + } + if result.Summary != "bitwarden login is required" { + t.Fatalf("summary = %q", result.Summary) + } + if !strings.Contains(result.Detail, "bw login") { + t.Fatalf("detail = %q, want login remediation", result.Detail) + } + }) + + t.Run("locked", func(t *testing.T) { + checkBitwardenReady = func(options secretstore.Options) error { + return fmt.Errorf("%w: run `bw unlock --raw`", secretstore.ErrBWLocked) + } + + result := BitwardenReadyCheck(BitwardenDoctorOptions{})(context.Background()) + if result.Status != DoctorStatusFail { + t.Fatalf("status = %q, want fail", result.Status) + } + if result.Summary != "bitwarden vault is locked or BW_SESSION is missing" { + t.Fatalf("summary = %q", result.Summary) + } + if !strings.Contains(result.Detail, "bw unlock --raw") { + t.Fatalf("detail = %q, want unlock remediation", result.Detail) + } + }) + + t.Run("ready", func(t *testing.T) { + checkBitwardenReady = func(options secretstore.Options) error { + return nil + } + + result := BitwardenReadyCheck(BitwardenDoctorOptions{})(context.Background()) + if result.Status != DoctorStatusOK { + t.Fatalf("status = %q, want ok", result.Status) + } + if result.Summary != "bitwarden CLI is ready" { + t.Fatalf("summary = %q", result.Summary) + } + }) +} + func TestRequiredResolvedFieldsCheckReportsSources(t *testing.T) { check := RequiredResolvedFieldsCheck(ResolveOptions{ Fields: []FieldSpec{ diff --git a/docs/cli-helpers.md b/docs/cli-helpers.md index 4077f15..69d97b3 100644 --- a/docs/cli-helpers.md +++ b/docs/cli-helpers.md @@ -158,6 +158,18 @@ if report.HasFailures() { } ``` +Pour une policy `bitwarden-cli`, tu peux ajouter un check dédié avec remédiations : + +```go +report := cli.RunDoctor(context.Background(), cli.DoctorOptions{ + ExtraChecks: []cli.DoctorCheck{ + cli.BitwardenReadyCheck(cli.BitwardenDoctorOptions{ + LookupEnv: os.LookupEnv, + }), + }, +}) +``` + Pour éviter des checks custom répétitifs sur les profils, `doctor` expose aussi un helper basé sur `FieldSpec` : ```go diff --git a/docs/secrets.md b/docs/secrets.md index 9fcbf66..9d9e683 100644 --- a/docs/secrets.md +++ b/docs/secrets.md @@ -72,6 +72,25 @@ store, err := secretstore.Open(secretstore.Options{ }) ``` +Pour vérifier explicitement que Bitwarden est prêt (login + unlock + `BW_SESSION`) : + +```go +if err := secretstore.EnsureBitwardenReady(secretstore.Options{ + BitwardenCommand: "bw", + LookupEnv: os.LookupEnv, +}); err != nil { + switch { + case errors.Is(err, secretstore.ErrBWNotLoggedIn): + // guider vers `bw login` + case errors.Is(err, secretstore.ErrBWLocked): + // guider vers `bw unlock --raw` puis export BW_SESSION + default: + // indisponibilité CLI/réseau + } + return err +} +``` + Pour stocker un secret structuré en JSON : ```go @@ -97,5 +116,20 @@ if err != nil { _ = creds ``` +Pour écrire puis confirmer immédiatement une relecture : + +```go +if err := secretstore.SetSecretVerified(store, "api-token", "My MCP API token", token); err != nil { + return err +} +``` + +Pour connaître le backend effectif utilisé : + +```go +effective := secretstore.EffectiveBackendPolicy(store) +fmt.Println("backend effectif:", effective) // bitwarden-cli, env-only, keyring-any... +``` + En mode `env-only`, `GetSecret("API_TOKEN")` lit la variable d'environnement `API_TOKEN`. Les opérations d'écriture et de suppression retournent `secretstore.ErrReadOnly`. diff --git a/scaffold/scaffold.go b/scaffold/scaffold.go index 135f4af..a94b97c 100644 --- a/scaffold/scaffold.go +++ b/scaffold/scaffold.go @@ -1677,6 +1677,10 @@ func (r Runtime) runSetup(_ context.Context, inv bootstrap.Invocation) error { stdout = os.Stdout } + if _, err := r.openSecretStore(); err != nil { + return fmt.Errorf("secret backend is not ready: %w", err) + } + cfg, _, err := r.ConfigStore.LoadDefault() if err != nil { return err @@ -1684,7 +1688,14 @@ func (r Runtime) runSetup(_ context.Context, inv bootstrap.Invocation) error { profileName := r.resolveProfileName(cfg.CurrentProfile) profile := cfg.Profiles[profileName] - storedToken, _ := r.readToken() + storedToken, err := r.readToken() + switch { + case err == nil: + case errors.Is(err, secretstore.ErrNotFound): + storedToken = "" + default: + return err + } result, err := cli.RunSetup(cli.SetupOptions{ Stdin: stdin, @@ -1726,16 +1737,34 @@ func (r Runtime) runSetup(_ context.Context, inv bootstrap.Invocation) error { return err } - if err := store.SetSecret(r.SecretName, "API token", tokenValue.String); err != nil { + if err := secretstore.SetSecretVerified(store, r.SecretName, "API token", tokenValue.String); err != nil { if errors.Is(err, secretstore.ErrReadOnly) { - fmt.Fprintf(stdout, "Secret store en lecture seule, exporte %s pour fournir le token.\n", r.TokenEnv) + return fmt.Errorf( + "secret store is read-only, export %s and retry setup", + r.TokenEnv, + ) } else { return err } } } - _, err = fmt.Fprintf(stdout, "Configuration sauvegardée pour le profil %q.\n", profileName) + verifiedToken, err := r.readToken() + if err != nil { + if errors.Is(err, secretstore.ErrNotFound) { + return fmt.Errorf( + "secret %q is not readable after setup, export %s and retry", + r.SecretName, + r.TokenEnv, + ) + } + return err + } + if strings.TrimSpace(verifiedToken) == "" { + return fmt.Errorf("secret %q is empty after setup", r.SecretName) + } + + _, err = fmt.Fprintf(stdout, "Configuration saved for profile %q. Secret readability confirmed.\n", profileName) return err } @@ -1789,6 +1818,12 @@ func (r Runtime) runConfigShow(_ context.Context, inv bootstrap.Invocation) erro if _, err := fmt.Fprintf(stdout, "Config: %s\n", path); err != nil { return err } + if _, err := fmt.Fprintf(stdout, "Manifest source: %s\n", r.manifestSourceLabel()); err != nil { + return err + } + if _, err := fmt.Fprintf(stdout, "Secret backend policy (active): %s\n", r.activeBackendPolicy()); err != nil { + return err + } _, err = fmt.Fprintf(stdout, "%s\n", payload) return err } @@ -1807,6 +1842,9 @@ func (r Runtime) runConfigTest(ctx context.Context, inv bootstrap.Invocation) er }, SecretStoreFactory: r.openSecretStore, ManifestCheck: r.manifestDoctorCheck(), + ExtraChecks: []cli.DoctorCheck{ + r.bitwardenDoctorCheck(), + }, }) if err := cli.RenderDoctorReport(stdout, report); err != nil { @@ -1835,14 +1873,9 @@ func (r Runtime) runUpdate(ctx context.Context, inv bootstrap.Invocation) error } func (r Runtime) openSecretStore() (secretstore.Store, error) { - backendPolicy := secretstore.BackendPolicy(strings.TrimSpace(r.Manifest.SecretStore.BackendPolicy)) - if backendPolicy == "" { - backendPolicy = secretstore.BackendAuto - } - return secretstore.Open(secretstore.Options{ - ServiceName: r.BinaryName, - BackendPolicy: backendPolicy, + ServiceName: r.BinaryName, + BackendPolicy: r.activeBackendPolicy(), LookupEnv: func(name string) (string, bool) { if name == r.SecretName { return os.LookupEnv(r.TokenEnv) @@ -1852,6 +1885,22 @@ func (r Runtime) openSecretStore() (secretstore.Store, error) { }) } +func (r Runtime) activeBackendPolicy() secretstore.BackendPolicy { + policy := secretstore.BackendPolicy(strings.TrimSpace(r.Manifest.SecretStore.BackendPolicy)) + if policy == "" { + return secretstore.BackendAuto + } + return policy +} + +func (r Runtime) manifestSourceLabel() string { + source := strings.TrimSpace(r.ManifestSource) + if source == "" { + return "embedded defaults" + } + return source +} + func (r Runtime) readToken() (string, error) { store, err := r.openSecretStore() if err != nil { @@ -1881,12 +1930,12 @@ func firstNonEmpty(values ...string) string { func (r Runtime) manifestDoctorCheck() cli.DoctorCheck { return func(context.Context) cli.DoctorResult { - source := strings.TrimSpace(r.ManifestSource) - if source == "" { + if strings.TrimSpace(r.ManifestSource) == "" { return cli.DoctorResult{ Name: "manifest", Status: cli.DoctorStatusWarn, Summary: "manifest is missing, using built-in defaults", + Detail: fmt.Sprintf("source=%s policy=%s", r.manifestSourceLabel(), r.activeBackendPolicy()), } } @@ -1894,10 +1943,20 @@ func (r Runtime) manifestDoctorCheck() cli.DoctorCheck { Name: "manifest", Status: cli.DoctorStatusOK, Summary: "manifest is valid", - Detail: source, + Detail: fmt.Sprintf("source=%s policy=%s", r.manifestSourceLabel(), r.activeBackendPolicy()), } } } + +func (r Runtime) bitwardenDoctorCheck() cli.DoctorCheck { + if r.activeBackendPolicy() != secretstore.BackendBitwardenCLI { + return nil + } + + return cli.BitwardenReadyCheck(cli.BitwardenDoctorOptions{ + LookupEnv: os.LookupEnv, + }) +} ` const manifestTemplate = `binary_name = "{{.BinaryName}}" diff --git a/secretstore/bitwarden.go b/secretstore/bitwarden.go index 55497c7..2a29cee 100644 --- a/secretstore/bitwarden.go +++ b/secretstore/bitwarden.go @@ -5,13 +5,17 @@ import ( "encoding/json" "errors" "fmt" + "os" "os/exec" "strings" ) const ( - defaultBitwardenCommand = "bw" - bitwardenSecretFieldName = "mcp-secret" + defaultBitwardenCommand = "bw" + bitwardenSessionEnvName = "BW_SESSION" + bitwardenSecretFieldName = "mcp-secret" + bitwardenServiceFieldName = "mcp-service" + bitwardenSecretNameFieldName = "mcp-secret-name" ) type bitwardenRunner func(command string, stdin []byte, args ...string) ([]byte, error) @@ -28,6 +32,10 @@ type bitwardenListItem struct { Name string `json:"name"` } +type bitwardenStatusOutput struct { + Status string `json:"status"` +} + func newBitwardenStore(options Options, policy BackendPolicy, serviceName string) (Store, error) { command := strings.TrimSpace(options.BitwardenCommand) if command == "" { @@ -57,12 +65,79 @@ func newBitwardenStore(options Options, policy BackendPolicy, serviceName string ) } + if err := EnsureBitwardenReady(Options{ + BitwardenCommand: command, + LookupEnv: options.LookupEnv, + }); err != nil { + return nil, fmt.Errorf( + "secret backend policy %q cannot use bitwarden CLI command %q right now: %w", + policy, + command, + errors.Join(ErrBackendUnavailable, err), + ) + } + return store, nil } +func EnsureBitwardenReady(options Options) error { + command := strings.TrimSpace(options.BitwardenCommand) + if command == "" { + command = defaultBitwardenCommand + } + + lookupEnv := options.LookupEnv + if lookupEnv == nil { + lookupEnv = os.LookupEnv + } + + output, err := runBitwardenCLI(command, nil, "status") + if err != nil { + return fmt.Errorf("check bitwarden CLI status: %w", err) + } + + trimmed := strings.TrimSpace(string(output)) + if trimmed == "" { + return fmt.Errorf("%w: bitwarden CLI returned an empty status", ErrBWUnavailable) + } + + var status bitwardenStatusOutput + if err := json.Unmarshal([]byte(trimmed), &status); err != nil { + return fmt.Errorf("decode bitwarden CLI status: %w", errors.Join(ErrBWUnavailable, err)) + } + + switch strings.ToLower(strings.TrimSpace(status.Status)) { + case "unauthenticated": + return fmt.Errorf("%w: run `bw login` then retry", ErrBWNotLoggedIn) + case "locked": + return fmt.Errorf( + "%w: run `export %s=\"$(bw unlock --raw)\"` then retry", + ErrBWLocked, + bitwardenSessionEnvName, + ) + case "unlocked": + session, ok := lookupEnv(bitwardenSessionEnvName) + if !ok || strings.TrimSpace(session) == "" { + return fmt.Errorf( + "%w: environment variable %q is missing; run `export %s=\"$(bw unlock --raw)\"` then retry", + ErrBWLocked, + bitwardenSessionEnvName, + bitwardenSessionEnvName, + ) + } + return nil + default: + return fmt.Errorf( + "%w: unsupported bitwarden status %q", + ErrBWUnavailable, + strings.TrimSpace(status.Status), + ) + } +} + func (s *bitwardenStore) SetSecret(name, label, secret string) error { secretName := s.scopedName(name) - item, err := s.findItem(secretName) + item, err := s.findItem(secretName, name) switch { case errors.Is(err, ErrNotFound): template, err := s.itemTemplate() @@ -70,7 +145,7 @@ func (s *bitwardenStore) SetSecret(name, label, secret string) error { return err } - setBitwardenSecretPayload(template, secretName, label, secret) + setBitwardenSecretPayload(template, s.serviceName, name, secretName, label, secret) encoded, err := s.encodePayload(template) if err != nil { return err @@ -95,7 +170,7 @@ func (s *bitwardenStore) SetSecret(name, label, secret string) error { return err } - setBitwardenSecretPayload(payload, secretName, label, secret) + setBitwardenSecretPayload(payload, s.serviceName, name, secretName, label, secret) encoded, err := s.encodePayload(payload) if err != nil { return err @@ -117,7 +192,7 @@ func (s *bitwardenStore) SetSecret(name, label, secret string) error { func (s *bitwardenStore) GetSecret(name string) (string, error) { secretName := s.scopedName(name) - item, err := s.findItem(secretName) + item, err := s.findItem(secretName, name) if err != nil { return "", err } @@ -137,7 +212,7 @@ func (s *bitwardenStore) GetSecret(name string) (string, error) { func (s *bitwardenStore) DeleteSecret(name string) error { secretName := s.scopedName(name) - item, err := s.findItem(secretName) + item, err := s.findItem(secretName, name) if errors.Is(err, ErrNotFound) { return nil } @@ -162,7 +237,7 @@ func (s *bitwardenStore) scopedName(name string) string { return fmt.Sprintf("%s/%s", s.serviceName, name) } -func (s *bitwardenStore) findItem(secretName string) (bitwardenListItem, error) { +func (s *bitwardenStore) findItem(secretName, rawSecretName string) (bitwardenListItem, error) { output, err := s.execute( fmt.Sprintf("list bitwarden items for secret %q", secretName), nil, @@ -174,6 +249,9 @@ func (s *bitwardenStore) findItem(secretName string) (bitwardenListItem, error) if err != nil { return bitwardenListItem{}, err } + if strings.TrimSpace(string(output)) == "" { + return bitwardenListItem{}, ErrNotFound + } var items []bitwardenListItem if err := json.Unmarshal(output, &items); err != nil { @@ -191,14 +269,44 @@ func (s *bitwardenStore) findItem(secretName string) (bitwardenListItem, error) matches = append(matches, item) } - switch len(matches) { - case 0: + if len(matches) == 0 { return bitwardenListItem{}, ErrNotFound + } + + markedMatches := make([]bitwardenListItem, 0, len(matches)) + legacyMatches := make([]bitwardenListItem, 0, len(matches)) + for _, item := range matches { + payload, err := s.itemByID(item.ID) + if err != nil { + return bitwardenListItem{}, err + } + + if bitwardenItemMatchesMarkers(payload, s.serviceName, rawSecretName) { + markedMatches = append(markedMatches, item) + continue + } + legacyMatches = append(legacyMatches, item) + } + + switch len(markedMatches) { + case 0: + switch len(legacyMatches) { + case 0: + return bitwardenListItem{}, ErrNotFound + case 1: + return legacyMatches[0], nil + default: + return bitwardenListItem{}, fmt.Errorf( + "multiple legacy bitwarden items match secret %q for service %q", + secretName, + s.serviceName, + ) + } case 1: - return matches[0], nil + return markedMatches[0], nil default: return bitwardenListItem{}, fmt.Errorf( - "multiple bitwarden items match secret %q for service %q", + "multiple bitwarden items share marker for secret %q and service %q", secretName, s.serviceName, ) @@ -272,7 +380,7 @@ func (s *bitwardenStore) execute(operation string, stdin []byte, args ...string) return output, nil } -func setBitwardenSecretPayload(payload map[string]any, secretName, label, secret string) { +func setBitwardenSecretPayload(payload map[string]any, serviceName, rawSecretName, secretName, label, secret string) { payload["type"] = 2 payload["name"] = secretName payload["notes"] = strings.TrimSpace(label) @@ -283,10 +391,24 @@ func setBitwardenSecretPayload(payload map[string]any, secretName, label, secret "value": secret, "type": 1, }, + { + "name": bitwardenServiceFieldName, + "value": strings.TrimSpace(serviceName), + "type": 0, + }, + { + "name": bitwardenSecretNameFieldName, + "value": strings.TrimSpace(rawSecretName), + "type": 0, + }, } } func readBitwardenSecret(payload map[string]any) (string, bool) { + return readBitwardenField(payload, bitwardenSecretFieldName) +} + +func readBitwardenField(payload map[string]any, fieldName string) (string, bool) { rawFields, ok := payload["fields"] if !ok { return "", false @@ -304,7 +426,7 @@ func readBitwardenSecret(payload map[string]any) (string, bool) { } name, _ := field["name"].(string) - if strings.TrimSpace(name) != bitwardenSecretFieldName { + if strings.TrimSpace(name) != strings.TrimSpace(fieldName) { continue } @@ -319,6 +441,20 @@ func readBitwardenSecret(payload map[string]any) (string, bool) { return "", false } +func bitwardenItemMatchesMarkers(payload map[string]any, serviceName, secretName string) bool { + markedService, ok := readBitwardenField(payload, bitwardenServiceFieldName) + if !ok { + return false + } + markedSecretName, ok := readBitwardenField(payload, bitwardenSecretNameFieldName) + if !ok { + return false + } + + return strings.TrimSpace(markedService) == strings.TrimSpace(serviceName) && + strings.TrimSpace(markedSecretName) == strings.TrimSpace(secretName) +} + func executeBitwardenCLI(command string, stdin []byte, args ...string) ([]byte, error) { cmd := exec.Command(command, args...) if stdin != nil { @@ -331,15 +467,81 @@ func executeBitwardenCLI(command string, stdin []byte, args ...string) ([]byte, cmd.Stderr = &stderr if err := cmd.Run(); err != nil { - detail := strings.TrimSpace(stderr.String()) - if detail == "" { - detail = strings.TrimSpace(stdout.String()) - } - if detail == "" { - return nil, err - } - return nil, fmt.Errorf("%w: %s", err, detail) + return nil, normalizeBitwardenExecutionError(err, stderr.String(), stdout.String()) } return stdout.Bytes(), nil } + +func normalizeBitwardenExecutionError(err error, stderrText, stdoutText string) error { + detail := sanitizeBitwardenErrorDetail(stderrText, stdoutText) + classification := classifyBitwardenError(detail) + if classification == nil { + classification = ErrBWUnavailable + } + + wrapped := errors.Join(classification, err) + if strings.TrimSpace(detail) == "" { + return wrapped + } + + return fmt.Errorf("%w: %s", wrapped, detail) +} + +func sanitizeBitwardenErrorDetail(stderrText, stdoutText string) string { + raw := strings.TrimSpace(stderrText) + if raw == "" { + raw = strings.TrimSpace(stdoutText) + } + if raw == "" { + return "" + } + + lines := strings.Split(raw, "\n") + cleaned := make([]string, 0, len(lines)) + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + continue + } + + lower := strings.ToLower(trimmed) + if strings.HasPrefix(trimmed, "at ") || + strings.HasPrefix(lower, "node:internal") || + strings.HasPrefix(lower, "internal/") || + strings.HasPrefix(lower, "npm ") { + continue + } + + cleaned = append(cleaned, trimmed) + } + + if len(cleaned) == 0 { + return "" + } + + if len(cleaned) == 1 { + return cleaned[0] + } + + return cleaned[0] + " | " + cleaned[1] +} + +func classifyBitwardenError(detail string) error { + lower := strings.ToLower(strings.TrimSpace(detail)) + + switch { + case strings.Contains(lower, "not logged in"), strings.Contains(lower, "unauthenticated"): + return ErrBWNotLoggedIn + case strings.Contains(lower, "vault is locked"), strings.Contains(lower, "is locked"): + return ErrBWLocked + case strings.Contains(lower, "failed to fetch"), + strings.Contains(lower, "econnrefused"), + strings.Contains(lower, "etimedout"), + strings.Contains(lower, "unable to connect"), + strings.Contains(lower, "network"): + return ErrBWUnavailable + default: + return nil + } +} diff --git a/secretstore/bitwarden_test.go b/secretstore/bitwarden_test.go index 8f6e2f0..9994fb5 100644 --- a/secretstore/bitwarden_test.go +++ b/secretstore/bitwarden_test.go @@ -14,6 +14,7 @@ import ( ) func TestOpenSupportsBitwardenCLIBackendPolicy(t *testing.T) { + withBitwardenSession(t) fakeCLI := newFakeBitwardenCLI("bw") withBitwardenRunner(t, fakeCLI.run) @@ -31,9 +32,116 @@ func TestOpenSupportsBitwardenCLIBackendPolicy(t *testing.T) { if !fakeCLI.versionChecked { t.Fatal("expected bitwarden CLI version check") } + if !fakeCLI.statusChecked { + t.Fatal("expected bitwarden CLI status check") + } +} + +func TestOpenBitwardenCLIReturnsUnavailableWhenCommandIsMissing(t *testing.T) { + withBitwardenRunner(t, func(command string, stdin []byte, args ...string) ([]byte, error) { + return nil, &exec.Error{Name: command, Err: exec.ErrNotFound} + }) + + _, err := Open(Options{ + ServiceName: "email-mcp", + BackendPolicy: BackendBitwardenCLI, + }) + if err == nil { + t.Fatal("expected error") + } + if !errors.Is(err, ErrBackendUnavailable) { + t.Fatalf("error = %v, want ErrBackendUnavailable", err) + } +} + +func TestOpenBitwardenCLIFailsWhenSessionIsMissing(t *testing.T) { + fakeCLI := newFakeBitwardenCLI("bw") + withBitwardenRunner(t, fakeCLI.run) + + _, err := Open(Options{ + ServiceName: "email-mcp", + BackendPolicy: BackendBitwardenCLI, + LookupEnv: func(name string) (string, bool) { + return "", false + }, + }) + if err == nil { + t.Fatal("expected error") + } + if !errors.Is(err, ErrBWLocked) { + t.Fatalf("error = %v, want ErrBWLocked", err) + } + if !errors.Is(err, ErrBackendUnavailable) { + t.Fatalf("error = %v, want ErrBackendUnavailable", err) + } +} + +func TestEnsureBitwardenReadyGuidesLoginAndUnlock(t *testing.T) { + t.Run("unauthenticated", func(t *testing.T) { + withBitwardenRunner(t, func(command string, stdin []byte, args ...string) ([]byte, error) { + if len(args) == 1 && args[0] == "status" { + return []byte(`{"status":"unauthenticated"}`), nil + } + return nil, fmt.Errorf("unexpected args: %v", args) + }) + + err := EnsureBitwardenReady(Options{BitwardenCommand: "bw"}) + if err == nil { + t.Fatal("expected error") + } + if !errors.Is(err, ErrBWNotLoggedIn) { + t.Fatalf("error = %v, want ErrBWNotLoggedIn", err) + } + if !strings.Contains(err.Error(), "bw login") { + t.Fatalf("error = %v, want guidance with bw login", err) + } + }) + + t.Run("locked", func(t *testing.T) { + withBitwardenRunner(t, func(command string, stdin []byte, args ...string) ([]byte, error) { + if len(args) == 1 && args[0] == "status" { + return []byte(`{"status":"locked"}`), nil + } + return nil, fmt.Errorf("unexpected args: %v", args) + }) + + err := EnsureBitwardenReady(Options{BitwardenCommand: "bw"}) + if err == nil { + t.Fatal("expected error") + } + if !errors.Is(err, ErrBWLocked) { + t.Fatalf("error = %v, want ErrBWLocked", err) + } + if !strings.Contains(err.Error(), "bw unlock --raw") { + t.Fatalf("error = %v, want guidance with bw unlock", err) + } + }) +} + +func TestEnsureBitwardenReadyAcceptsUnlockedSession(t *testing.T) { + withBitwardenRunner(t, func(command string, stdin []byte, args ...string) ([]byte, error) { + if len(args) == 1 && args[0] == "status" { + return []byte(`{"status":"unlocked"}`), nil + } + return nil, fmt.Errorf("unexpected args: %v", args) + }) + + err := EnsureBitwardenReady(Options{ + BitwardenCommand: "bw", + LookupEnv: func(name string) (string, bool) { + if name == "BW_SESSION" { + return "session-token", true + } + return "", false + }, + }) + if err != nil { + t.Fatalf("EnsureBitwardenReady returned error: %v", err) + } } func TestBitwardenStoreSetGetDeleteSecret(t *testing.T) { + withBitwardenSession(t) fakeCLI := newFakeBitwardenCLI("bw") withBitwardenRunner(t, fakeCLI.run) @@ -79,24 +187,138 @@ func TestBitwardenStoreSetGetDeleteSecret(t *testing.T) { } } -func TestOpenBitwardenCLIReturnsUnavailableWhenCommandIsMissing(t *testing.T) { - withBitwardenRunner(t, func(command string, stdin []byte, args ...string) ([]byte, error) { - return nil, &exec.Error{Name: command, Err: exec.ErrNotFound} - }) +func TestBitwardenStoreWritesMarkerFields(t *testing.T) { + withBitwardenSession(t) + fakeCLI := newFakeBitwardenCLI("bw") + withBitwardenRunner(t, fakeCLI.run) - _, err := Open(Options{ + store, err := Open(Options{ ServiceName: "email-mcp", BackendPolicy: BackendBitwardenCLI, }) + if err != nil { + t.Fatalf("Open returned error: %v", err) + } + + if err := store.SetSecret("api-token", "API token", "secret-v1"); err != nil { + t.Fatalf("SetSecret returned error: %v", err) + } + + var found fakeBitwardenItem + for _, item := range fakeCLI.itemsByID { + if item.Name == "email-mcp/api-token" { + found = item + break + } + } + if found.ID == "" { + t.Fatal("expected bitwarden item to be created") + } + if found.MarkerService != "email-mcp" { + t.Fatalf("marker service = %q, want email-mcp", found.MarkerService) + } + if found.MarkerSecretName != "api-token" { + t.Fatalf("marker secret = %q, want api-token", found.MarkerSecretName) + } +} + +func TestBitwardenStorePrefersStrictMarkerMatchWhenNameCollides(t *testing.T) { + withBitwardenSession(t) + fakeCLI := newFakeBitwardenCLI("bw") + fakeCLI.itemsByID["item-1"] = fakeBitwardenItem{ + ID: "item-1", + Name: "email-mcp/api-token", + Secret: "wrong-secret", + MarkerService: "other-service", + MarkerSecretName: "api-token", + } + fakeCLI.itemsByID["item-2"] = fakeBitwardenItem{ + ID: "item-2", + Name: "email-mcp/api-token", + Secret: "good-secret", + MarkerService: "email-mcp", + MarkerSecretName: "api-token", + } + withBitwardenRunner(t, fakeCLI.run) + + store, err := Open(Options{ + ServiceName: "email-mcp", + BackendPolicy: BackendBitwardenCLI, + }) + if err != nil { + t.Fatalf("Open returned error: %v", err) + } + + value, err := store.GetSecret("api-token") + if err != nil { + t.Fatalf("GetSecret returned error: %v", err) + } + if value != "good-secret" { + t.Fatalf("GetSecret = %q, want good-secret", value) + } +} + +func TestBitwardenStoreFallsBackToSingleLegacyItemWithoutMarkers(t *testing.T) { + withBitwardenSession(t) + fakeCLI := newFakeBitwardenCLI("bw") + fakeCLI.itemsByID["legacy-1"] = fakeBitwardenItem{ + ID: "legacy-1", + Name: "email-mcp/api-token", + Secret: "legacy-secret", + } + withBitwardenRunner(t, fakeCLI.run) + + store, err := Open(Options{ + ServiceName: "email-mcp", + BackendPolicy: BackendBitwardenCLI, + }) + if err != nil { + t.Fatalf("Open returned error: %v", err) + } + + value, err := store.GetSecret("api-token") + if err != nil { + t.Fatalf("GetSecret returned error: %v", err) + } + if value != "legacy-secret" { + t.Fatalf("GetSecret = %q, want legacy-secret", value) + } +} + +func TestBitwardenFindItemTreatsEmptyListOutputAsNotFound(t *testing.T) { + store := &bitwardenStore{command: "bw", serviceName: "email-mcp"} + withBitwardenRunner(t, func(command string, stdin []byte, args ...string) ([]byte, error) { + if len(args) == 4 && args[0] == "list" && args[1] == "items" && args[2] == "--search" { + return []byte(""), nil + } + return nil, fmt.Errorf("unexpected args: %v", args) + }) + + _, err := store.findItem("email-mcp/api-token", "api-token") + if !errors.Is(err, ErrNotFound) { + t.Fatalf("error = %v, want ErrNotFound", err) + } +} + +func TestExecuteBitwardenCLIClassifiesLoginError(t *testing.T) { + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh is required for this test") + } + + _, err := executeBitwardenCLI("sh", nil, "-c", "echo 'You are not logged in.' 1>&2; echo ' at Foo (node:internal/x)' 1>&2; exit 1") if err == nil { t.Fatal("expected error") } - if !errors.Is(err, ErrBackendUnavailable) { - t.Fatalf("error = %v, want ErrBackendUnavailable", err) + if !errors.Is(err, ErrBWNotLoggedIn) { + t.Fatalf("error = %v, want ErrBWNotLoggedIn", err) + } + if strings.Contains(err.Error(), "node:internal") { + t.Fatalf("error = %v, stack trace should be stripped", err) } } func TestOpenFromManifestSupportsBitwardenCLIBackendPolicy(t *testing.T) { + withBitwardenSession(t) fakeCLI := newFakeBitwardenCLI("bw") withBitwardenRunner(t, fakeCLI.run) @@ -128,6 +350,11 @@ func TestOpenFromManifestSupportsBitwardenCLIBackendPolicy(t *testing.T) { } } +func withBitwardenSession(t *testing.T) { + t.Helper() + t.Setenv("BW_SESSION", "test-session") +} + func withBitwardenRunner( t *testing.T, runner func(command string, stdin []byte, args ...string) ([]byte, error), @@ -145,14 +372,18 @@ type fakeBitwardenCLI struct { command string itemsByID map[string]fakeBitwardenItem nextID int + status string versionChecked bool + statusChecked bool } type fakeBitwardenItem struct { - ID string - Name string - Notes string - Secret string + ID string + Name string + Notes string + Secret string + MarkerService string + MarkerSecretName string } func newFakeBitwardenCLI(command string) *fakeBitwardenCLI { @@ -160,6 +391,7 @@ func newFakeBitwardenCLI(command string) *fakeBitwardenCLI { command: strings.TrimSpace(command), itemsByID: map[string]fakeBitwardenItem{}, nextID: 1, + status: "unlocked", } } @@ -175,6 +407,10 @@ func (f *fakeBitwardenCLI) run(command string, stdin []byte, args ...string) ([] f.versionChecked = true return []byte("2026.1.0\n"), nil } + if len(args) == 1 && args[0] == "status" { + f.statusChecked = true + return []byte(fmt.Sprintf(`{"status":%q}`, strings.TrimSpace(f.status))), nil + } switch { case len(args) == 4 && args[0] == "list" && args[1] == "items" && args[2] == "--search": @@ -205,12 +441,10 @@ func (f *fakeBitwardenCLI) handleListItems(search string) ([]byte, error) { continue } items = append(items, map[string]any{ - "id": item.ID, - "name": item.Name, - "notes": item.Notes, - "fields": []map[string]any{ - {"name": bitwardenSecretFieldName, "value": item.Secret, "type": 1}, - }, + "id": item.ID, + "name": item.Name, + "notes": item.Notes, + "fields": item.fieldsPayload(), }) } @@ -228,12 +462,10 @@ func (f *fakeBitwardenCLI) handleGetItem(id string) ([]byte, error) { } payload, err := json.Marshal(map[string]any{ - "id": item.ID, - "name": item.Name, - "notes": item.Notes, - "fields": []map[string]any{ - {"name": bitwardenSecretFieldName, "value": item.Secret, "type": 1}, - }, + "id": item.ID, + "name": item.Name, + "notes": item.Notes, + "fields": item.fieldsPayload(), "secureNote": map[string]any{"type": 0}, }) if err != nil { @@ -250,10 +482,12 @@ func (f *fakeBitwardenCLI) handleCreateItem(encoded string) ([]byte, error) { } item := fakeBitwardenItem{ - ID: fmt.Sprintf("item-%d", f.nextID), - Name: readString(payload, "name"), - Notes: readString(payload, "notes"), - Secret: readFakeBitwardenSecret(payload), + ID: fmt.Sprintf("item-%d", f.nextID), + Name: readString(payload, "name"), + Notes: readString(payload, "notes"), + Secret: readFakeBitwardenField(payload, bitwardenSecretFieldName), + MarkerService: readFakeBitwardenField(payload, bitwardenServiceFieldName), + MarkerSecretName: readFakeBitwardenField(payload, bitwardenSecretNameFieldName), } f.nextID++ f.itemsByID[item.ID] = item @@ -278,10 +512,12 @@ func (f *fakeBitwardenCLI) handleEditItem(id, encoded string) ([]byte, error) { } item := fakeBitwardenItem{ - ID: trimmedID, - Name: readString(payload, "name"), - Notes: readString(payload, "notes"), - Secret: readFakeBitwardenSecret(payload), + ID: trimmedID, + Name: readString(payload, "name"), + Notes: readString(payload, "notes"), + Secret: readFakeBitwardenField(payload, bitwardenSecretFieldName), + MarkerService: readFakeBitwardenField(payload, bitwardenServiceFieldName), + MarkerSecretName: readFakeBitwardenField(payload, bitwardenSecretNameFieldName), } f.itemsByID[trimmedID] = item @@ -307,7 +543,7 @@ func decodeBitwardenPayload(encoded string) (map[string]any, error) { return payload, nil } -func readFakeBitwardenSecret(payload map[string]any) string { +func readFakeBitwardenField(payload map[string]any, fieldName string) string { rawFields, ok := payload["fields"] if !ok { return "" @@ -325,7 +561,7 @@ func readFakeBitwardenSecret(payload map[string]any) string { } name := strings.TrimSpace(readString(field, "name")) - if name != bitwardenSecretFieldName { + if name != fieldName { continue } @@ -335,6 +571,21 @@ func readFakeBitwardenSecret(payload map[string]any) string { return "" } +func (i fakeBitwardenItem) fieldsPayload() []map[string]any { + fields := []map[string]any{ + {"name": bitwardenSecretFieldName, "value": i.Secret, "type": 1}, + } + + if strings.TrimSpace(i.MarkerService) != "" { + fields = append(fields, map[string]any{"name": bitwardenServiceFieldName, "value": i.MarkerService, "type": 0}) + } + if strings.TrimSpace(i.MarkerSecretName) != "" { + fields = append(fields, map[string]any{"name": bitwardenSecretNameFieldName, "value": i.MarkerSecretName, "type": 0}) + } + + return fields +} + func readString(payload map[string]any, key string) string { value, _ := payload[key].(string) return strings.TrimSpace(value) diff --git a/secretstore/store.go b/secretstore/store.go index d957562..2b9bb5f 100644 --- a/secretstore/store.go +++ b/secretstore/store.go @@ -16,6 +16,9 @@ var ErrNotFound = errors.New("secret not found") var ErrBackendUnavailable = errors.New("secret backend unavailable") var ErrReadOnly = errors.New("secret backend is read-only") var ErrInvalidBackendPolicy = errors.New("invalid secret backend policy") +var ErrBWNotLoggedIn = errors.New("bitwarden is not logged in") +var ErrBWLocked = errors.New("bitwarden vault is locked or BW_SESSION is missing") +var ErrBWUnavailable = errors.New("bitwarden CLI unavailable") type BackendPolicy string @@ -140,6 +143,40 @@ func BackendName() string { } } +func EffectiveBackendPolicy(store Store) BackendPolicy { + switch store.(type) { + case *bitwardenStore: + return BackendBitwardenCLI + case *envStore: + return BackendEnvOnly + case *keyringStore: + return BackendKeyringAny + default: + return "" + } +} + +func SetSecretVerified(store Store, name, label, secret string) error { + if store == nil { + return errors.New("secret store must not be nil") + } + + if err := store.SetSecret(name, label, secret); err != nil { + return err + } + + verified, err := store.GetSecret(name) + if err != nil { + return fmt.Errorf("verify secret %q after write: %w", name, err) + } + + if verified != secret { + return fmt.Errorf("verify secret %q after write: read-back mismatch", name) + } + + return nil +} + func SetJSON[T any](store Store, name, label string, value T) error { data, err := json.Marshal(value) if err != nil { diff --git a/secretstore/store_test.go b/secretstore/store_test.go index 818ea41..1ddd6c6 100644 --- a/secretstore/store_test.go +++ b/secretstore/store_test.go @@ -248,3 +248,88 @@ func TestJSONHelpersRoundTrip(t *testing.T) { t.Fatalf("GetJSON = %#v, want %#v", output, input) } } + +func TestEffectiveBackendPolicyReportsConcreteBackend(t *testing.T) { + t.Run("env-only", func(t *testing.T) { + store, err := Open(Options{ + BackendPolicy: BackendEnvOnly, + LookupEnv: func(string) (string, bool) { return "", false }, + }) + if err != nil { + t.Fatalf("Open returned error: %v", err) + } + + if got := EffectiveBackendPolicy(store); got != BackendEnvOnly { + t.Fatalf("EffectiveBackendPolicy = %q, want %q", got, BackendEnvOnly) + } + }) + + t.Run("keyring", func(t *testing.T) { + withKeyringHooks(t, []keyring.BackendType{keyring.SecretServiceBackend}, func(cfg keyring.Config) (keyring.Keyring, error) { + return &stubKeyring{}, nil + }) + + store, err := Open(Options{ + ServiceName: "mcp-framework-test", + BackendPolicy: BackendAuto, + }) + if err != nil { + t.Fatalf("Open returned error: %v", err) + } + + if got := EffectiveBackendPolicy(store); got != BackendKeyringAny { + t.Fatalf("EffectiveBackendPolicy = %q, want %q", got, BackendKeyringAny) + } + }) +} + +func TestSetSecretVerifiedWritesThenReadsBack(t *testing.T) { + ring := &stubKeyring{} + withKeyringHooks(t, []keyring.BackendType{keyring.SecretServiceBackend}, func(cfg keyring.Config) (keyring.Keyring, error) { + return ring, nil + }) + + store, err := Open(Options{ + ServiceName: "mcp-framework-test", + BackendPolicy: BackendAuto, + }) + if err != nil { + t.Fatalf("Open returned error: %v", err) + } + + if err := SetSecretVerified(store, "token", "API token", "secret-value"); err != nil { + t.Fatalf("SetSecretVerified returned error: %v", err) + } +} + +func TestSetSecretVerifiedFailsOnReadBackMismatch(t *testing.T) { + store := &mismatchSecretStore{} + + err := SetSecretVerified(store, "token", "API token", "secret-value") + if err == nil { + t.Fatal("expected error") + } + if !errors.Is(err, ErrNotFound) { + t.Fatalf("error = %v, want wrapped ErrNotFound", err) + } + if store.setCalls != 1 { + t.Fatalf("setCalls = %d, want 1", store.setCalls) + } +} + +type mismatchSecretStore struct { + setCalls int +} + +func (s *mismatchSecretStore) SetSecret(name, label, secret string) error { + s.setCalls++ + return nil +} + +func (s *mismatchSecretStore) GetSecret(name string) (string, error) { + return "", ErrNotFound +} + +func (s *mismatchSecretStore) DeleteSecret(name string) error { + return nil +}