From a5914735463b29e097be652421dc3e0534572518 Mon Sep 17 00:00:00 2001 From: Duy /zuey/ Date: Thu, 28 May 2026 18:17:49 +0700 Subject: [PATCH] feat(secure-cli): CLI credential adapters framework + git adapter (#82) (#89) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(secure-cli): Phase 1 schema + storage delta (issue #82) Adds `adapter_name` column to secure_cli_binaries and `credential_type` + `host_scope` columns to secure_cli_user_credentials. LookupByBinary LEFT JOIN now projects AdapterName, UserCredentialType, and UserHostScope. Extends SecureCLIStore with SetUserCredentialsTyped(ctx, binaryID, userID, env, credType, hostScope); legacy SetUserCredentials delegates to typed variant with nil/nil for backward compat. PG migration 73 + RequiredSchemaVersion bumped to 73. SQLite incremental migrations (versions 39–41) + SchemaVersion 42. 7 SQLite + 6 PG integration tests covering schema, round-trip, NULL legacy, LookupByBinary projection. Fixes #82 * feat(secure-cli): Phase 2 CredentialAdapter framework + passthrough (issue #82) Adds CredentialAdapter interface + Injection{ArgvPrefix,Env,Cleanup,ScrubValues} struct. Registry resolves by name; falls back to passthrough for empty/unknown. passthroughAdapter is default no-op — all existing presets (gh/aws/gcloud/kubectl/terraform/gws) behave bit-for-bit identically. Per-request WithScrubBag(ctx) + AddScrubValuesCtx + ScrubCredentialsCtx for multi-tenant secret isolation (replaces package-global slice). Hook in executeCredentialed between env merge and exec: resolve adapter by bin.AdapterName, reject non-passthrough in sandbox, call Prepare, splice ArgvPrefix, merge Env, defer Cleanup, register ScrubValues. Audit log security.system_env_injection records adapter name + env key names + argv_prefix_len + sha256(host_scope) — NEVER values. CLIPreset.AdapterName field added; empty default for legacy presets. 12 unit tests covering passthrough no-op, registry fallback + nil safety, Injection shape, hashHostScope determinism, sortedKeys, scrub bag per-request isolation + concurrent + short-value guard. Fixes #82 * feat(secure-cli): Phase 2b extensibility helpers + psql stub adapter (issue #82) Proves CredentialAdapter framework generalizes beyond git. Adds materializeEphemeral(ctx, content, prefix) shared helper — 0600 tmpfile + idempotent atomic.Bool cleanup latch; memfd intentionally rejected (resolves "self" against child process). Adds psqlAdapter consuming framework end-to-end via PGPASSFILE pattern; libpq-spec .pgpass escaping for `:` and `\`. Registers psql preset with AdapterName: "psql" (production UI for typed creds lands in v2). Interface validation gate passes: Injection shape unchanged, hook is psql-agnostic, no special branch needed. Tests: ephemeral write/cleanup/concurrent/zero-content; psql routing/content/escaping/error paths/registration. Fixes #82 * feat(secure-cli): git adapter PAT + SSH implementation (issue #82) Phase 3 — PAT path - gitAdapter PAT branch via GIT_CONFIG_COUNT/KEY_0/VALUE_0 env (git 2.31+) so the token never lands on argv, .git/config, or remote URL - Host-scope enforcement with IDN normalization (golang.org/x/net/idna) and embedded-userinfo rejection in URL parsing - CVE-2018-17456 mitigation: resolve remote URLs via `git config --get` not `git remote get-url` to dodge ext::sh protocol handler injection - Case-insensitive DenyArgs blocking `-c http.`, `-c credential.`, `-c core.sshcommand`, `config --global/--system`, `credential-helper`, bare `daemon` - New WithExecCwd / ExecCwdFromContext context helpers — fixes a latent design gap where the adapter's pre-flight `git config --get` ran in goclaw's daemon CWD instead of the agent's repo - 14 unit tests covering subcommand routing, host normalization, scp-form parsing, userinfo rejection, CRLF token rejection, CVE-2018-17456 regression, DenyArgs preset coverage - 3 integration tests against a local TLS git-http-backend server proving end-to-end clone + fetch + host-mismatch rejection with zero token leakage into the cloned .git/config Phase 4 — SSH path - gitAdapter ssh_key branch materializes per-call 0600 tmpfile via the Phase 2b materializeEphemeral helper, injects GIT_SSH_COMMAND with -o IdentitiesOnly=yes -o BatchMode=yes -o StrictHostKeyChecking=accept-new, idempotent cleanup - ValidateSSHKey using golang.org/x/crypto/ssh rejects passphrase-protected keys via ErrSSHKeyPassphraseUnsupported sentinel - 8 unit tests covering passphrase rejection, env shape, cleanup lifecycle, host-mismatch reuse, malformed-blob rejection - 3 integration tests proving tmpfile 0600 lifecycle, env propagation to child process, cleanup-on-exec-failure, no-orphan-on-rejection - 2 new i18n keys (en/vi/zh) for SSH-passphrase and SSH-key-invalid Verification: go vet clean, go build ./... clean, go build -tags sqliteonly ./... clean, go test -race ./internal/tools/ pass, go test -tags integration ./tests/integration/ -run TestGitAdapter pass. Refs #82 * feat(secure-cli): UI presets + typed credential HTTP path + i18n (issue #82) Phase 5. Typed PUT envelope with {error:{code,message}, error_key} for field-level errors. CliCredentialGitFields React component (PAT/SSH picker, host_scope input, CRLF→LF normalization, masked-edit). 17 i18n keys × 3 locales (backend + frontend). i18n parity test. * feat(secure-cli): audit log schema + adapter framework docs (issue #82) Phase 6. emitSystemEnvInjectionAudit helper centralizes security.system_env_injection slog with host_scope_hash (SHA-256 8 hex, plaintext hostname omitted for PII safety). Audit shape pinned by TestEmitSystemEnvInjectionAudit_*. New docs: git-credential-adapter.md (user guide), credential-adapter-playbook.md (R1 implementer guide w/ kubectl/docker/npm/aws/psql worked mappings). 09-security.md § 14 trust-boundary diagram + SSH TOFU + SIGKILL caveats. 03-tools-system.md § 8a. Changelog entry. * docs(journal): issue #82 CLI credential adapters shipped Retrospective covering 6 commits across phases 1-6: framework, git PAT/SSH, psql stub, UI/i18n, audit log schema, docs. Notes memfd-drop rationale, TOFU/SIGKILL caveats, sentinel-length lesson from audit-shape tests. * fix(secure-cli): honor per-request scrub bag on success+failure paths (#82) Sandbox and host exec paths called the non-Ctx ScrubCredentials, so adapter ScrubValues registered into the per-request bag during Prepare (e.g. GitLab glpat-, Bitbucket app-passwords, Azure DevOps PATs, Gitea tokens, SSH key tmpfile paths) were ignored on stdout/stderr returned to the agent. Only the package-global regex pass ran — covering ghp_ but nothing else. Switch all four exec/sandbox call sites to ScrubCredentialsCtx so the bag is consulted. Also scrub the slog adapter_cleanup_failed line — os.Remove errors embed the full tmpfile path. Add 5 regression tests that pin success path, failure path, negative control (proves the bag is what catches the sentinel), classic-PAT sanity, and timeout path no-leak. Locks AC6 against non-GitHub PAT providers. * fix(secure-cli): address review-pr #89 findings - ui/web: SSH key Textarea was 'text-xs' on all viewports, triggering iOS Safari auto-zoom on focus. Switch to 'text-base md:text-xs' so mobile renders 16px (no zoom) while desktop keeps compact mono font. - psql adapter: add on-disk pgpass tmpfile path to ScrubValues. psql echoes 'could not open password file ""' on IO errors, so the path needs scrubbing alongside the password. Mirrors the git SSH adapter pattern. Update test assertion accordingly. - psql adapter: replace literal 'nil' context with 'context.TODO()' to silence staticcheck SA1012. --- cmd/gateway_agents_test.go | 3 + docs/03-tools-system.md | 29 ++ docs/09-security.md | 105 +++++ docs/credential-adapter-playbook.md | 343 ++++++++++++++ docs/git-credential-adapter.md | 206 ++++++++ ...ssue-82-cli-credential-adapters-shipped.md | 83 ++++ docs/project-changelog.md | 77 +++ internal/http/secure_cli_env_response_test.go | 3 + internal/http/secure_cli_typed_credentials.go | 223 +++++++++ .../http/secure_cli_typed_credentials_test.go | 249 ++++++++++ internal/http/secure_cli_user_credentials.go | 92 +++- internal/i18n/catalog_en.go | 14 + internal/i18n/catalog_vi.go | 14 + internal/i18n/catalog_zh.go | 14 + internal/i18n/git_keys_parity_test.go | 46 ++ internal/i18n/keys.go | 14 + internal/store/pg/secure_cli.go | 37 +- .../store/pg/secure_cli_user_credentials.go | 37 +- internal/store/secure_cli_store.go | 21 +- internal/store/sqlitestore/schema.go | 15 +- internal/store/sqlitestore/schema.sql | 19 +- .../secure-cli-user-credentials.go | 47 +- internal/store/sqlitestore/secure-cli.go | 60 ++- .../sqlitestore/secure_cli_schema_test.go | 270 +++++++++++ internal/tools/credential_adapter.go | 138 ++++++ internal/tools/credential_adapter_git.go | 445 ++++++++++++++++++ .../credential_adapter_git_keyvalidate.go | 50 ++ .../tools/credential_adapter_git_ssh_test.go | 240 ++++++++++ internal/tools/credential_adapter_git_test.go | 417 ++++++++++++++++ internal/tools/credential_adapter_psql.go | 101 ++++ .../tools/credential_adapter_psql_test.go | 188 ++++++++ internal/tools/credential_adapter_test.go | 254 ++++++++++ internal/tools/credential_audit_log_test.go | 183 +++++++ internal/tools/credential_context.go | 17 + internal/tools/credential_context_test.go | 40 ++ internal/tools/credential_ephemeral.go | 64 +++ internal/tools/credential_ephemeral_test.go | 107 +++++ internal/tools/credential_presets.go | 45 ++ internal/tools/credentialed_exec.go | 89 +++- .../tools/credentialed_exec_scrub_test.go | 126 +++++ internal/tools/scrub.go | 85 ++++ .../tools/shell_credentialed_gate_test.go | 3 + internal/upgrade/version.go | 2 +- ...000073_secure_cli_credential_type.down.sql | 6 + .../000073_secure_cli_credential_type.up.sql | 12 + tests/integration/git_adapter_pat_test.go | 217 +++++++++ tests/integration/git_adapter_ssh_test.go | 187 ++++++++ tests/integration/githttp_helper_test.go | 110 +++++ .../secure_cli_credential_type_pg_test.go | 231 +++++++++ .../src/i18n/locales/en/cli-credentials.json | 18 +- .../src/i18n/locales/vi/cli-credentials.json | 18 +- .../src/i18n/locales/zh/cli-credentials.json | 18 +- .../cli-credential-git-fields.tsx | 162 +++++++ .../cli-user-credentials-dialog.tsx | 183 ++++++- ui/web/src/types/cli-credential.ts | 5 + 55 files changed, 5659 insertions(+), 123 deletions(-) create mode 100644 docs/credential-adapter-playbook.md create mode 100644 docs/git-credential-adapter.md create mode 100644 docs/journals/260528-issue-82-cli-credential-adapters-shipped.md create mode 100644 internal/http/secure_cli_typed_credentials.go create mode 100644 internal/http/secure_cli_typed_credentials_test.go create mode 100644 internal/i18n/git_keys_parity_test.go create mode 100644 internal/store/sqlitestore/secure_cli_schema_test.go create mode 100644 internal/tools/credential_adapter.go create mode 100644 internal/tools/credential_adapter_git.go create mode 100644 internal/tools/credential_adapter_git_keyvalidate.go create mode 100644 internal/tools/credential_adapter_git_ssh_test.go create mode 100644 internal/tools/credential_adapter_git_test.go create mode 100644 internal/tools/credential_adapter_psql.go create mode 100644 internal/tools/credential_adapter_psql_test.go create mode 100644 internal/tools/credential_adapter_test.go create mode 100644 internal/tools/credential_audit_log_test.go create mode 100644 internal/tools/credential_ephemeral.go create mode 100644 internal/tools/credential_ephemeral_test.go create mode 100644 internal/tools/credentialed_exec_scrub_test.go create mode 100644 migrations/000073_secure_cli_credential_type.down.sql create mode 100644 migrations/000073_secure_cli_credential_type.up.sql create mode 100644 tests/integration/git_adapter_pat_test.go create mode 100644 tests/integration/git_adapter_ssh_test.go create mode 100644 tests/integration/githttp_helper_test.go create mode 100644 tests/integration/secure_cli_credential_type_pg_test.go create mode 100644 ui/web/src/pages/cli-credentials/cli-credential-git-fields.tsx diff --git a/cmd/gateway_agents_test.go b/cmd/gateway_agents_test.go index a0634a09..9b867625 100644 --- a/cmd/gateway_agents_test.go +++ b/cmd/gateway_agents_test.go @@ -48,6 +48,9 @@ func (s *stubSecureCLIStoreCmd) GetUserCredentials(ctx context.Context, binaryID func (s *stubSecureCLIStoreCmd) SetUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte) error { return nil } +func (s *stubSecureCLIStoreCmd) SetUserCredentialsTyped(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte, credentialType, hostScope *string) error { + return nil +} func (s *stubSecureCLIStoreCmd) DeleteUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string) error { return nil } diff --git a/docs/03-tools-system.md b/docs/03-tools-system.md index a288ba2d..fc054083 100644 --- a/docs/03-tools-system.md +++ b/docs/03-tools-system.md @@ -397,6 +397,35 @@ Tool output is automatically scrubbed before being returned to the LLM or the us The exact patterns are intentionally not published here (defense-in-depth). The scrubber is always enabled in the registry by default. +### 8a. Credential adapter framework + +For tool binaries that need per-user typed credentials (PAT, SSH key, +kubeconfig, `.pgpass`, etc.), the `CredentialAdapter` interface in +`internal/tools/credential_adapter.go` transforms a stored credential into +the argv/env/ephemeral-file shape the binary expects. + +- **`Name() string`** — the value stored in `secure_cli_binaries.adapter_name`. +- **`ShouldInject(argv []string) bool`** — gate that skips local-only + subcommands (e.g. `git status` does not trigger injection). +- **`Prepare(...) (*Injection, error)`** — returns the four-field + `Injection{ArgvPrefix, Env, Cleanup, ScrubValues}` consumed by + `credentialed_exec.go`. + +Adapters are registered in their own `init()` via `RegisterAdapter`. Lookup +falls back to the `passthrough` no-op adapter on unknown/empty names, so +unrelated presets (`gh`, `aws`, `gcloud`, `kubectl`, `terraform`, `gws`) +keep their legacy env-injection path bit-for-bit. + +Per-injection audit: every adapter run emits one +`slog.Warn("security.system_env_injection", …)` line with the field schema +documented in [09-security.md § 14](./09-security.md#14-cli-credential-adapters). + +To author a new adapter (kubectl, docker, npm, aws, …), follow the worked +mappings + interface-validation gate in +[credential-adapter-playbook.md](./credential-adapter-playbook.md). User-facing +config for the shipped `git` adapter is documented in +[git-credential-adapter.md](./git-credential-adapter.md). + --- ## 9. Per-Tenant Config (4-Tier Overlay) diff --git a/docs/09-security.md b/docs/09-security.md index 5fcdc693..e4384f1c 100644 --- a/docs/09-security.md +++ b/docs/09-security.md @@ -497,6 +497,111 @@ Package name validation is defense-in-depth at three layers: --- +## 14. CLI Credential Adapters + +The CLI credential adapter framework is the system-trusted path for injecting +auth material into spawned CLI subprocesses (`git clone`, `kubectl apply`, +`psql`, …). It is the second of two distinct trust boundaries for env-var +injection — distinct from, not a replacement for, the user-paste denylist. + +User guide: [git-credential-adapter.md](./git-credential-adapter.md). +Implementer guide: [credential-adapter-playbook.md](./credential-adapter-playbook.md). + +### Trust-boundary diagram + +``` +┌──────────────────────────────┐ ┌──────────────────────────────────┐ +│ User-pasted env vars │ │ System-injected adapter Env │ +│ (CLI Credentials → "env") │ │ (CredentialAdapter.Prepare) │ +│ │ │ │ +│ ValidateGrantEnvVars │ │ bypasses denylist │ +│ rejects GIT_SSH_COMMAND, │ │ emits security.system_env_ │ +│ LD_PRELOAD, PATH, … │ │ injection slog.Warn per call │ +└──────────────────────────────┘ └──────────────────────────────────┘ + first line of defense second, audit-trailed line +``` + +Both paths coexist. A typo in `adapter_name` falls back to passthrough, which +restores the legacy denylist-only behavior — no silent bypass. + +### Audit log: `security.system_env_injection` + +Every adapter injection emits **exactly one** structured slog line. Field +schema is pinned by `TestEmitSystemEnvInjectionAudit_*` in +`internal/tools/credential_audit_log_test.go` — changes here must update both +the test and operator-facing log-search recipes. + +| Field | Type | Notes | +| ----- | ---- | ----- | +| `msg` | string | always `security.system_env_injection` | +| `adapter` | string | e.g. `git`, `psql`, `passthrough` | +| `binary` | string | binary name (`git`, `kubectl`, …) | +| `user_id` | string | tenant user UUID (empty for global-only contexts) | +| `env_keys` | []string | sorted env-var NAMES (never values) | +| `argv_prefix_len` | int | number of argv elements prepended (NOT their content) | +| `host_scope_hash` | string | SHA-256 first 8 hex chars of normalized host_scope, or `"none"` | + +**Plaintext hostname is intentionally omitted** to keep audit logs PII-safe +when goclaw is deployed inside a regulated tenant. Operators wanting to grep +for activity against a specific host pre-compute the hash: + +```sh +echo -n "github.com" | sha256sum | cut -c1-8 +``` + +Routing: `slog.Warn` writes to whatever the host runtime captures — for the +default goclaw deployment that's stderr → systemd/journald or Docker logs. +There is **no dedicated audit table** in v1 (see future work below). + +### SSH TOFU MITM caveat + +The git adapter's SSH path sets `StrictHostKeyChecking=accept-new`, which +accepts unknown host keys on first contact. A network attacker positioned +between goclaw and the git host CAN capture the SSH session on the first +connection. + +Operators should pre-seed `~/.ssh/known_hosts` at deployment time: + +```sh +ssh-keyscan github.com >> ~/.ssh/known_hosts +ssh-keyscan -p 22 gitea.internal >> ~/.ssh/known_hosts +``` + +Once a host key is in `known_hosts`, `accept-new` enforces match-or-fail on +subsequent connections — the TOFU window is one connection per host. + +v2 will support per-credential pinned host keys (removes TOFU entirely). + +### SIGKILL residual material + +Ephemeral filesystem credentials (SSH key tmpfiles, `.pgpass` tmpfiles, future +KUBECONFIG/DOCKER_CONFIG tmpfiles) rely on `defer cleanup()` to remove +themselves after exec returns. + +`SIGKILL` of the goclaw process leaves these 0600 files in `os.TempDir()`. On +POSIX, `os.TempDir()` is per-user, so exposure is limited to the goclaw uid. + +High-security deployments should run a periodic sweep: + +```sh +find "$TMPDIR" -name 'goclaw-gitkey-*' -mmin +60 -delete +find "$TMPDIR" -name 'goclaw-pgpass-*' -mmin +60 -delete +``` + +### Open future work + +- Dedicated `audit_log` table for `security.system_env_injection` events + (operator-grade query surface; v1 only writes slog). +- Multi-credential per user with host-routing logic (v1: one per + user+binary+host_scope). +- Sandbox/Docker exec path support (v1 adapter is incompatible with the + bind-mount-based sandbox path). +- Pinned SSH host keys per credential (replace TOFU). +- Credential-refresh primitive for `aws sts assume-role`-style short-lived + STS credentials. + +--- + ## File Reference | Module | Path | Purpose | diff --git a/docs/credential-adapter-playbook.md b/docs/credential-adapter-playbook.md new file mode 100644 index 00000000..a4c04ccc --- /dev/null +++ b/docs/credential-adapter-playbook.md @@ -0,0 +1,343 @@ +# Credential Adapter Playbook + +Developer guide for authoring a `CredentialAdapter`. Use this when adding a new +typed-credential CLI binary (kubectl, docker, npm, aws, psql…) beyond the +shipped `git` adapter. + +This is the **R1 extensibility deliverable** from the issue #82 brainstorm: +proof that the framework generalizes past git. Read [git-credential-adapter.md](./git-credential-adapter.md) +first for the user-facing story; this doc is the implementer's manual. + +## Interface contract + +Source of truth: [`internal/tools/credential_adapter.go`](../internal/tools/credential_adapter.go). + +```go +type CredentialAdapter interface { + Name() string // adapter_name column value + ShouldInject(argv []string) bool // gate: skip local-only subcommands + Prepare(ctx context.Context, + bin *store.SecureCLIBinary, + cred *store.SecureCLIUserCredential, + argv []string) (*Injection, error) +} +``` + +- `Name()` is the string operators set in `secure_cli_binaries.adapter_name`. + It is also the value logged in `security.system_env_injection.adapter`. +- `ShouldInject` decides whether the adapter runs for THIS invocation. For + tools without subcommands (`psql`, `docker push`, etc.) return `true`. For + tools where some subcommands are local-only (`git status`, `kubectl version`) + return `false` to skip injection and audit-noise. +- `Prepare` produces a single `*Injection` per exec. Returning a non-nil error + aborts the exec — no fallback to un-credentialed run, so reserve errors for + malformed credentials, not "credential not found" (return `&Injection{}` for + that case). + +Register in your adapter file's `init()`: + +```go +func init() { RegisterAdapter(myAdapter{}) } +``` + +Lookup falls back to the `passthrough` no-op adapter on unknown names, so a +typo in `adapter_name` degrades to legacy behavior with a clear audit trail +rather than breaking exec. + +## The four Injection fields + +```go +type Injection struct { + ArgvPrefix []string // spliced between binary and user args + Env map[string]string // merged on top of base env + Cleanup func() error // deferred after exec + ScrubValues []string // redacted from stdout/stderr/Result/errors +} +``` + +### `ArgvPrefix` + +Use when the tool reads auth from CLI flags AND those flags do NOT carry the +secret directly. Example: `kubectl --kubeconfig ` is fine (path, not +secret). `aws --profile ` is fine. NEVER use for the secret itself — +argv is world-readable via `/proc//cmdline` on Linux. + +The PAT path of the git adapter deliberately uses `Env` (not `ArgvPrefix`) +for `http..extraheader` precisely because `git -c http.…=` +would leak the token via `ps`. + +### `Env` + +The primary injection channel. Tools that read auth from a config file env +var (`KUBECONFIG`, `DOCKER_CONFIG`, `NPM_CONFIG_USERCONFIG`, +`AWS_SHARED_CREDENTIALS_FILE`, `PGPASSFILE`) all flow through here. + +Env is scoped to the spawned child process; goclaw's own env is unchanged. + +### `Cleanup` + +Required when `Prepare` writes any filesystem material. Always pair with +`materializeEphemeral`'s returned cleanup closure (see below). + +### `ScrubValues` + +List every secret byte sequence the subprocess might echo back. The +per-request `ScrubCredentials` bag (see [`internal/tools/scrub.go`](../internal/tools/scrub.go)) +strips these from: + +- live stdout/stderr streamed to the agent, +- the final `Result.Content`, +- error messages, +- the slog audit line. + +If the tool's error path embeds the secret in a URL or config dump (git's +`fatal: unable to access 'https://@host/'…`), this is your only +defense — register both the raw secret AND any predictable wrapping. + +## Ephemeral filesystem material — `materializeEphemeral` + +Source: [`internal/tools/credential_ephemeral.go`](../internal/tools/credential_ephemeral.go). + +```go +path, cleanup, err := materializeEphemeral(ctx, []byte(content), "kubecfg") +if err != nil { return nil, err } +return &Injection{ + Env: map[string]string{"KUBECONFIG": path}, + Cleanup: cleanup, + ScrubValues: []string{bearerToken}, +}, nil +``` + +Guarantees: + +- `0600` perms, per-user `os.TempDir()` on POSIX. +- Idempotent cleanup (concurrent callers don't double-remove). +- Prefix becomes `goclaw--` so operators can sweep stale files + with one glob. + +### Why not memfd? + +Originally proposed (brainstorm R6); dropped during validation. `/proc/self/fd/N` +resolves "self" against the calling process. For grandchildren (e.g. git → ssh), +the kernel resolves against the child, which sees `EBADF` unless the parent +passed the fd via `ExecCommand.ExtraFiles` AND the child binary reads from that +fd number. None of git/psql/docker/kubectl have an API to forward fds to their +subprocesses. Tmpfile + `defer remove` is the safe, portable default. + +**SIGKILL caveat:** if goclaw is killed with SIGKILL (-9), `defer cleanup()` +never fires and the 0600 tmpfile lingers. Operators should sweep — see +[git-credential-adapter.md → Operator notes](./git-credential-adapter.md#operator-notes). + +## Host-scope semantics + +`secure_cli_user_credentials.host_scope` is the **exact** ASCII hostname (with +optional port) the credential authenticates to. v1 does not support wildcards. + +| Tool | host_scope value | Matched against | +| ---- | ---------------- | --------------- | +| git | `github.com`, `gitea.internal:8443` | remote URL host from argv | +| kubectl | `prod.example.com:6443` | current-context cluster API endpoint | +| docker | `registry.gitlab.com`, `ghcr.io` | first argv after `push`/`pull` | +| npm | `registry.npmjs.org` or `npm.pkg.github.com` | argv `--registry` or `.npmrc` default | +| aws | `:` (composite) | parsed from `--profile`/region flags | +| psql | `db.example.com:5432` | `-h`/`-p` argv or `PGHOST` env | + +Normalize via `idna.ToASCII` and lowercase. Hash with the shared +`hashHostScope()` helper for audit logs — **never** log plaintext hostname. + +## Worked mappings + +Each subsection below is a sketch. Production adapters should add validation, +edge-case handling, and per-adapter tests mirroring `credential_adapter_git_test.go`. + +### kubectl + +```go +func (kubectlAdapter) Prepare(ctx context.Context, _ *store.SecureCLIBinary, + cred *store.SecureCLIUserCredential, _ []string) (*Injection, error) { + if cred == nil { return &Injection{}, nil } + var k struct { + Kubeconfig string `json:"kubeconfig"` // full YAML body + BearerToken string `json:"bearer_token"` // optional + } + if err := json.Unmarshal(cred.EncryptedEnv, &k); err != nil { + return nil, fmt.Errorf("decode kubeconfig cred: %w", err) + } + path, cleanup, err := materializeEphemeral(ctx, []byte(k.Kubeconfig), "kubecfg") + if err != nil { return nil, err } + scrub := []string{} + if k.BearerToken != "" { scrub = append(scrub, k.BearerToken) } + return &Injection{ + Env: map[string]string{"KUBECONFIG": path}, + Cleanup: cleanup, + ScrubValues: scrub, + }, nil +} +``` + +- **Injection fields**: `Env` + `Cleanup` + `ScrubValues`. +- **Host scope**: cluster API server hostname:port from current context. +- **Skip subcommands**: `kubectl version`, `kubectl config view --raw`, + `kubectl --help`. Most subcommands DO hit the API server so default-true is + acceptable; refine if false-positive audit noise becomes an issue. +- **Tests to write**: kubeconfig path injected into env not argv; tmpfile + removed post-exec; bearer token scrubbed from `kubectl get pods` 401 stderr. + +### docker + +```go +// DOCKER_CONFIG points to a DIRECTORY containing config.json, not the file. +func (dockerAdapter) Prepare(ctx context.Context, _ *store.SecureCLIBinary, + cred *store.SecureCLIUserCredential, _ []string) (*Injection, error) { + if cred == nil { return &Injection{}, nil } + var d struct { + Auths map[string]struct{ Auth string `json:"auth"` } `json:"auths"` + } + if err := json.Unmarshal(cred.EncryptedEnv, &d); err != nil { + return nil, fmt.Errorf("decode docker cred: %w", err) + } + dir, cleanup, err := materializeEphemeralDir(ctx, "dockercfg") + if err != nil { return nil, err } + body, _ := json.Marshal(d) + if err := os.WriteFile(filepath.Join(dir, "config.json"), body, 0o600); err != nil { + _ = cleanup(); return nil, err + } + scrub := []string{} + for _, a := range d.Auths { scrub = append(scrub, a.Auth) } + return &Injection{ + Env: map[string]string{"DOCKER_CONFIG": dir}, + Cleanup: cleanup, + ScrubValues: scrub, + }, nil +} +``` + +- **Note**: `DOCKER_CONFIG` is a directory, not a file. Requires a small + `materializeEphemeralDir` sibling helper (not yet shipped — add when first + docker adapter lands). +- **Host scope**: registry hostname (`registry.gitlab.com`, `ghcr.io`, + `.dkr.ecr..amazonaws.com`). +- **Scrub**: the base64-encoded `auth` value AND the decoded `user:pass` form, + because docker error output can decode and echo either. + +### npm + +```go +func (npmAdapter) Prepare(ctx context.Context, _ *store.SecureCLIBinary, + cred *store.SecureCLIUserCredential, _ []string) (*Injection, error) { + if cred == nil { return &Injection{}, nil } + var n struct { + Registry string `json:"registry"` + AuthToken string `json:"auth_token"` + } + if err := json.Unmarshal(cred.EncryptedEnv, &n); err != nil { + return nil, fmt.Errorf("decode npm cred: %w", err) + } + line := fmt.Sprintf("//%s/:_authToken=%s\n", + strings.TrimPrefix(strings.TrimPrefix(n.Registry, "https://"), "http://"), + n.AuthToken) + path, cleanup, err := materializeEphemeral(ctx, []byte(line), "npmrc") + if err != nil { return nil, err } + return &Injection{ + Env: map[string]string{"NPM_CONFIG_USERCONFIG": path}, + Cleanup: cleanup, + ScrubValues: []string{n.AuthToken}, + }, nil +} +``` + +- **Host scope**: registry hostname. +- **Subcommand gate**: only `install`/`publish`/`ci`/`audit` (network ops). + Skip `npm list`, `npm version`, `npm run …`. + +### aws + +```go +func (awsAdapter) Prepare(ctx context.Context, _ *store.SecureCLIBinary, + cred *store.SecureCLIUserCredential, _ []string) (*Injection, error) { + if cred == nil { return &Injection{}, nil } + var a struct { + AccessKeyID string `json:"access_key_id"` + SecretAccessKey string `json:"secret_access_key"` + SessionToken string `json:"session_token,omitempty"` + Profile string `json:"profile"` + } + if err := json.Unmarshal(cred.EncryptedEnv, &a); err != nil { + return nil, fmt.Errorf("decode aws cred: %w", err) + } + body := fmt.Sprintf("[%s]\naws_access_key_id=%s\naws_secret_access_key=%s\n", + a.Profile, a.AccessKeyID, a.SecretAccessKey) + if a.SessionToken != "" { + body += fmt.Sprintf("aws_session_token=%s\n", a.SessionToken) + } + path, cleanup, err := materializeEphemeral(ctx, []byte(body), "awscreds") + if err != nil { return nil, err } + return &Injection{ + Env: map[string]string{ + "AWS_SHARED_CREDENTIALS_FILE": path, + "AWS_PROFILE": a.Profile, + }, + Cleanup: cleanup, + ScrubValues: []string{a.SecretAccessKey, a.SessionToken}, + }, nil +} +``` + +- **Host scope**: composite `:` (composite key for the + rare case where the same operator runs multi-account workflows). +- **v2 flag — credential refresh**: `aws sts assume-role` returns a short-lived + STS credential. v1's "one credential per exec" model cannot refresh + mid-flight. Defer until a refresh primitive is added (track in roadmap). + +### psql + +Already shipped as the framework-validation stub. +See [`internal/tools/credential_adapter_psql.go`](../internal/tools/credential_adapter_psql.go). + +- **Injection fields**: `Env: {PGPASSFILE: path}` + `Cleanup` + `ScrubValues`. +- **Host scope**: `db.example.com:5432`. +- **Subcommand gate**: psql has no subcommands; `ShouldInject` returns `true` + always. +- **Edge case handled**: `escapePgpass()` escapes backslash and colon per the + libpq `.pgpass` spec to prevent injection of a second entry via a `:` in + the password. + +## Interface validation gate + +Before merging a new adapter, answer these three gate questions in writing +(PR description or phase file). Phase 2b introduced this discipline; reuse it +verbatim: + +1. **Does `Prepare` fit the four Injection fields cleanly, or did you need a + fifth?** If the latter, the framework needs a change BEFORE your adapter + lands — not a bypass. + +2. **Can the secret reach the subprocess without ever touching `ArgvPrefix`?** + If no, document why and accept the `/proc//cmdline` exposure + explicitly in the security section of your phase file. + +3. **Does the tool's error path emit the secret in a form `ScrubValues` + wouldn't catch?** (e.g. base64-wrapped, URL-encoded, partially echoed.) + Enumerate the wrappings and add each to `ScrubValues`. + +If any answer is "no" or "unsure", stop and revise the design. + +## Anti-patterns — do NOT + +- **Do NOT add adapter-specific branches to `credentialed_exec.go`.** The + whole point of the framework is that the hot path stays agnostic. If your + adapter needs special handling, put it in `Prepare` or extend the + `Injection` shape (and update every other adapter). +- **Do NOT introduce a parallel `materializeFoo` helper.** Use the shared + `materializeEphemeral` (or extend it). One helper means one place to audit + the perms/cleanup contract. +- **Do NOT log plaintext `host_scope` to audit.** Use `hashHostScope()` — + operators recover the host by pre-computing the hash, not by reading it + out of logs. +- **Do NOT inject the secret via `ArgvPrefix`.** See gate question #2. +- **Do NOT skip `ShouldInject` gating** for tools with mixed local/remote + subcommands. Every unnecessary injection is an extra audit-log line, a tmp + file, and a potential leak surface. +- **Do NOT return a hard error for "no credential matches".** Return + `&Injection{}, nil` and let the subprocess fail with its own clear auth + error. Hard error here breaks the un-credentialed fallback path. diff --git a/docs/git-credential-adapter.md b/docs/git-credential-adapter.md new file mode 100644 index 00000000..552008fc --- /dev/null +++ b/docs/git-credential-adapter.md @@ -0,0 +1,206 @@ +# Git Credential Adapter + +User-facing guide for the `git` typed credential adapter (issue #82, ships with +v3.x). + +## Why a typed adapter? + +The legacy CLI credential flow asks the user to paste arbitrary environment +variables (`GH_TOKEN`, `KUBECONFIG`, etc.). `git` does not read its +authentication from a stable, single env var — credentials live in +`.git/config`, `~/.git-credentials`, the OS credential helper, or per-remote +URLs. Pasting a PAT into `GIT_TOKEN` did nothing, which surprised users and +silently failed every clone. + +The typed `git` adapter accepts either a **Personal Access Token (PAT)** or an +**SSH private key**, validates it server-side, then injects it into the spawned +`git` process via a transient mechanism that never touches disk or argv. + +## When to use which credential type + +| Type | Use when | Limits | +| --------- | ------------------------------------------------------------------------------- | ------------------------------------------------------------ | +| **PAT** | GitHub/GitLab/Gitea over HTTPS. You already have a `ghp_…` or `glpat-…` token. | Token must be unscoped to specific repos, OR cover all repos goclaw will touch. | +| **SSH** | Self-hosted git over SSH. You manage `~/.ssh/known_hosts` or accept TOFU risk. | Passphrase-protected keys are NOT supported (see below). | +| **Env** | Legacy path — you have a custom env-var-driven workflow. | Loses host-scoped routing; same trust profile as other CLIs. | + +## Adding a credential (UI) + +1. Open **Settings → CLI Credentials → User Credentials → Add**. +2. Select user. +3. Choose **Credential Type**: `Personal Access Token` or `SSH Private Key`. +4. Enter **Host Scope** (required for PAT/SSH): the hostname the credential + authenticates to. + - Examples: `github.com`, `gitlab.example.com`, `gitea.internal:8443`. + - Case-insensitive. Punycode normalized via `idna.ToASCII`. + - Port included only when non-default for the scheme. +5. Paste the token (PAT) or the unencrypted PEM body (SSH). +6. Save. + +The stored secret is encrypted (AES-256-GCM) and can never be read back through +the API or UI. Editing the row shows a `••••••••` placeholder; leaving the +secret field blank preserves the stored value, typing a new value replaces it. + +## What gets auto-injected + +The adapter runs ONLY for these subcommands: + +- `clone` +- `fetch` +- `pull` +- `push` +- `submodule` + +Any other subcommand (`status`, `log`, `diff`, `commit`, `branch`, etc.) runs +WITHOUT credentials — these are local operations and never reach a remote. + +Implementation: see `internal/tools/credential_adapter_git.go::ShouldInject`. + +## Host-scope semantics + +`host_scope` is the **exact** ASCII hostname (with optional port) the +credential is valid for. v1 does NOT support wildcards. + +Stored `github.com` matches: + +- ✓ `git clone https://github.com/org/repo.git` +- ✗ `git clone https://api.github.com/...` (different host) +- ✗ `git clone https://github.com:8443/...` (different port — port is part of + the scope key) + +Stored `gitea.example.com:8443` matches: + +- ✓ `git clone https://gitea.example.com:8443/...` +- ✗ `git clone https://gitea.example.com/...` (default port — still mismatch) + +If you run a self-hosted server on the scheme's default port (443 HTTPS, 22 +SSH), omit the port. If you run on a non-default port, include it. + +When no stored credential matches the resolved remote host, the adapter falls +through to the un-credentialed path: `git` runs with whatever credentials the +calling shell already has (typically none, in which case the remote will reject +the operation with a 401/403). + +## Security model + +### PAT path + +- Injected via `GIT_CONFIG_COUNT` + `GIT_CONFIG_KEY_*` / `GIT_CONFIG_VALUE_*` + environment variables. +- The PAT itself goes into a value that synthesizes an `http..extraheader` + config entry with `AUTHORIZATION: basic `. +- **The PAT never appears on argv** — so `ps`, `/proc//cmdline`, and + shell-history echoes don't expose it. +- The injected env vars are scoped to the spawned `git` process only; they are + NOT inherited by goclaw, by other tools, or by sibling exec calls. + +### SSH path + +- The PEM key is written to an `0600`-mode tmpfile in `os.TempDir()` (per-user + on POSIX) with a `goclaw-gitkey-*` prefix. +- `GIT_SSH_COMMAND` is set to + `ssh -i -F /dev/null -o IdentitiesOnly=yes -o StrictHostKeyChecking=accept-new`. +- The tmpfile is removed via `defer` on the exec wrapper. **SIGKILL of goclaw + leaves the file orphaned** — see the Operator Notes section below. +- `StrictHostKeyChecking=accept-new` accepts unknown host keys on first + contact (TOFU). A network attacker positioned between goclaw and the git + host CAN capture the SSH session on the first connection. Operators should + pre-seed `~/.ssh/known_hosts`: + + ```sh + ssh-keyscan github.com >> ~/.ssh/known_hosts + ``` + + v2 will support per-credential pinned host keys. + +### Passphrase-protected SSH keys: rejected + +The adapter rejects encrypted SSH keys at validation time with `error_key = +git.cred_ssh_passphrase_unsupported`. Reason: we have no UX or storage slot +for the passphrase, and ssh-agent forwarding is outside the goclaw security +model. Re-export your key without a passphrase, or use a dedicated deploy key. + +### Redaction across output channels + +Every credential adapter registers its secret bytes with the per-request +`ScrubCredentials` bag (`internal/tools/scrub.go`). The scrubber removes the +secret from: + +- Live stdout / stderr streamed to the agent. +- The final `Result.Content` returned by the tool. +- Error messages bubbled up to the agent. +- The audit log line (`security.system_env_injection`) — see below. + +Plaintext hostnames are also kept out of the audit log: `host_scope_hash` is +the SHA-256 first 8 hex chars of the normalized scope. + +## Auditability + +Every successful credential injection emits exactly one structured log line: + +``` +level=WARN msg=security.system_env_injection + adapter=git binary=git user_id= + env_keys=[GIT_CONFIG_COUNT,GIT_CONFIG_KEY_0,GIT_CONFIG_VALUE_0] + argv_prefix_len=0 + host_scope_hash=3aeb0024 +``` + +`env_keys` lists NAMES only — values never appear. `host_scope_hash` is the +first 8 hex chars of `sha256(normalized_host_scope)`. Operators wanting to +grep for activity against a specific host pre-compute the hash: + +```sh +echo -n "github.com" | sha256sum | cut -c1-8 +``` + +See `docs/09-security.md` → "CLI credential adapters" for the full schema. + +## Migration from legacy env-paste + +Existing rows in `secure_cli_user_credentials` with `credential_type IS NULL` +or `= 'env'` continue to work via the passthrough adapter — they keep +emitting their env vars exactly as before. There is no forced migration. + +To upgrade an existing git credential, open the user-credentials dialog, pick +PAT or SSH, paste the secret, and save. The legacy env-paste row is replaced +atomically. + +## Operator notes + +- **Tmpfile sweep**: high-security deployments should sweep stale tmpfiles + every few minutes: + + ```sh + find "$TMPDIR" -name 'goclaw-gitkey-*' -mmin +60 -delete + find "$TMPDIR" -name 'goclaw-pgpass-*' -mmin +60 -delete + ``` + +- **Pre-seed known_hosts** to defeat TOFU MITM (see SSH path above). +- **Log aggregation**: route `security.*` slog events to your SIEM. The + schema is pinned by `TestEmitSystemEnvInjectionAudit_*` — alert on any + change. +- **No sandbox support v1**: the adapter mutates the parent process's + forked-child environment, which is incompatible with the bind-mount-based + sandbox path. Sandbox + credentialed exec is on the v2 roadmap. + +## Known limitations (v1) + +- One credential per (user, binary, host_scope) row. +- No multi-host wildcard (`*.github.com`). +- No passphrase-protected SSH keys. +- No persistent `known_hosts` per credential (TOFU only). +- No sandbox support. +- PAT scope cannot be inspected — goclaw stores the token opaquely. + +## Future work + +Tracked separately: + +- v2: OAuth device-flow for GitHub/GitLab — eliminates PAT paste. +- v2: Multi-credential per user with host routing logic. +- v2: Sandbox/Docker exec path support (per-call key bind-mount). +- v2: Pinned SSH host keys per credential. +- v2: Migrate `gh`/`aws`/`gcloud` to non-passthrough adapters as use cases + arise (e.g. `aws assume-role` needs argv mutation). +- v2: Dedicated `audit_log` table for `security.system_env_injection` events. diff --git a/docs/journals/260528-issue-82-cli-credential-adapters-shipped.md b/docs/journals/260528-issue-82-cli-credential-adapters-shipped.md new file mode 100644 index 00000000..16ba1b8d --- /dev/null +++ b/docs/journals/260528-issue-82-cli-credential-adapters-shipped.md @@ -0,0 +1,83 @@ +# CLI Credential Adapters — git PAT + SSH, Framework Complete + +**Date**: 2026-05-28 17:29 +**Severity**: Medium +**Component**: CLI credential injection, git adapter, audit logging +**Status**: Resolved + +## What Happened + +Issue #82 (CLI Credential Adapters) completed across 6 commits spanning phases 1–6. The core problem: legacy CLI credential flow asked users to paste arbitrary env vars (e.g., `GIT_TOKEN`), but `git` does not read auth from any stable env var, so every clone failed silently. The solution: generic `CredentialAdapter` interface with typed `git` adapter supporting PAT (via GIT_CONFIG_COUNT env, never argv) and SSH (0600 tmpfile + GIT_SSH_COMMAND). + +## The Brutal Truth + +We shipped with the understanding that we had **no production git-auth flow at all**. Users copy-pasted a token into the wrong place and watched clones fail with "access denied" because goclaw was silently running `git clone` unauthenticated. The framework we built is the *first* correct solution; it's not a refactor of something that worked. + +The frustrating part: we validated this by catching the auth failure *during testing* — the legacy passthrough adapter with no host scope is a safety valve, not a feature. If a user later tries to use it anyway, they get the exact same silent-failure behavior they had before, but at least now they have a path to fix it. + +## Technical Details + +### PAT Path (Phase 3, commit b0dccbe3) +- **Mechanism**: GIT_CONFIG_COUNT + GIT_CONFIG_KEY_0 + GIT_CONFIG_VALUE_0 environment variables (git 2.31+). Token never touches argv, .git/config, or remote URL. +- **Host-scope enforcement**: IDN normalization (golang.org/x/net/idna), embedded-userinfo rejection in URL parsing. +- **CVE-2018-17456 mitigation**: resolve remote URLs via `git config --get` (not `git remote get-url`) to dodge ext::sh protocol handler injection. +- **DenyArgs blocking**: case-insensitive rejection of `-c http.`, `-c credential.`, `-c core.sshcommand`, `config --global/--system`, `credential-helper`, bare `daemon`. +- **Tests**: 14 unit tests (subcommand routing, host normalization, scp-form parsing, userinfo rejection, CRLF token rejection, CVE-2018-17456 regression, DenyArgs coverage) + 3 integration tests against TLS git-http-backend proving zero token leakage into cloned .git/config. + +### SSH Path (Phase 4, commit b0dccbe3) +- **Mechanism**: Per-call 0600 tmpfile materialized via Phase 2b `materializeEphemeral()` helper. GIT_SSH_COMMAND injected with `-o IdentitiesOnly=yes -o BatchMode=yes -o StrictHostKeyChecking=accept-new`. Idempotent cleanup via defer. +- **Validation**: golang.org/x/crypto/ssh parses key blob. Passphrase-protected keys rejected with `ErrSSHKeyPassphraseUnsupported` sentinel. +- **SSH TOFU trade-off**: `accept-new` accepted with documented ssh-keyscan mitigation; pinned host keys deferred to v2. +- **Tests**: 8 unit tests (passphrase rejection, env shape, cleanup lifecycle, host-mismatch reuse, malformed-blob rejection) + 3 integration tests proving tmpfile 0600 lifecycle, env propagation to child, cleanup-on-exec-failure, no-orphan-on-rejection. + +### Audit Logging (Phase 6, commit 16a0f303) +- **Schema**: `emitSystemEnvInjectionAudit()` centralizes slog `security.system_env_injection` with host_scope_hash (SHA-256 first 8 hex, plaintext hostname omitted for PII safety). Operators pre-compute hash to grep audit streams. +- **Leak detection testing**: `TestEmitSystemEnvInjectionAudit_PAT` and `TestEmitSystemEnvInjectionAudit_SSH` assert that: (a) env NAMES go into audit, env VALUES do NOT; (b) PAT/PEM bytes never appear in log buffer. + +### Extensibility Framework (Phase 2, commit 1fe7c5e0 + Phase 2b, commit 14cce5b9) +- **Interface**: `CredentialAdapter` with `ValidateCredential()` + `Inject()`. Passthrough default + git typed + psql stub (Phase 2b, proof of generalization). +- **WithExecCwd / ExecCwdFromContext helpers** (Phase 3): fixes latent design gap where adapter's pre-flight `git config --get` ran in goclaw's daemon CWD, not the agent's repo CWD. + +## What We Tried + +1. **memfd vs. tmpfile for SSH key**: Initial design used `/proc/self/fd/N` for memory-backed file. Validation revealed `/proc/self/fd/N` resolves "self" against the **caller** (goclaw), not the child process (git→ssh grandchild). git doesn't expose a mechanism to inherit fds without explicit cooperation. Reverted to 0600 tmpfile + defer cleanup. + +2. **Sentinel values in leak-detection tests**: First attempt used short sentinels like "1" for `GIT_CONFIG_COUNT`. False-positive matches occurred in slog timestamp digits (e.g., `"timestamp":"2026-05-28T17:29:21.000123..."`). Fixed by using distinctly-formed sentinels: `SENTINEL_COUNT_VALUE`, `ghp_SENTINEL_PAT_VALUE_4242424242424242` (long enough that random substring collision is negligible). + +3. **Passphrase-protected SSH keys**: Attempted to detect and prompt for passphrase during validation. Decision: reject them entirely. Rationale: no UX slot to store or retrieve the passphrase on each exec; kubernetes-style service-account flow (unencrypted keys) is the baseline, and v2 can add passphrase support if required. + +## Root Cause Analysis + +The original mistake was designing a credential injection system without typing. The `CredentialAdapter` interface was necessary because different tools have **entirely different** auth mechanisms (git: env vars + config, kubectl: kubeconfig file, npm: .npmrc auth field, aws: STS assume-role), and a single "paste env var" flow cannot adapt to all of them. + +The silent-failure symptom persisted through phases 1–2 because the framework was incomplete: once typed, the git adapter needed explicit host-scope routing + HTTPS path (GIT_CONFIG_COUNT), SSH path (tmpfile), and audit logging. Each missing piece meant git would silently fall back to unauthenticated mode. + +## Lessons Learned + +1. **Sentinel values for "secret not in log" assertions must be distinctive enough that random text cannot match.** Short sentinels (1–2 chars) collide with digits in timestamps; use longer, structured sentinels (`SENTINEL_*` prefix, base16 suffix) so substring matches are meaningful. + +2. **env-var-based auth has hard scaling limits.** git's flexibility (multiple auth backends) means no single env var controls it all. Typed adapters per tool are not optional. + +3. **fd inheritance across process boundaries is tool-specific.** `/proc/self/fd/N` is not a portable secret-passing mechanism; tmpfile + 0600 + defer is simpler and more predictable, even if it hits the filesystem. + +4. **Host-scope is the invariant.** v1 enforces (user, adapter, host_scope) → one credential. Wildcards and multi-host credential reuse are deferred because they require explicit escrow-side design (key rotation, credential audit hooks). + +5. **Context propagation for exec environment must trace the agent's repo CWD, not the daemon's CWD.** WithExecCwd helpers prevent subtle auth failures when the adapter runs pre-flight checks in the wrong directory. + +## Next Steps + +**Shipped in commits b0dccbe3 → 16a0f303:** +- git adapter (PAT + SSH paths), psql stub (proof of extensibility), audit logging schema, 2 user guides (git-credential-adapter.md, credential-adapter-playbook.md). +- 4 new docs: 09-security.md (trust boundary + SSH TOFU caveats), 03-tools-system.md (audit shape integration). +- 17 i18n keys × 3 locales (en/vi/zh) + React credential dialog rework. +- 41 tests (unit + integration, PG + SQLite both exercised). + +**Blocked / Deferred to v2:** +- **Sandbox incompatibility**: adapter cannot grant filesystem perms to bind-mount creds. Design needed for sandboxed exec contexts. +- **Credential-refresh primitive**: no mechanism for `aws sts assume-role` (multi-step auth). Requires OAuth-callback plumbing. +- **Dedicated audit_log table**: currently relies on slog→stderr→journald. Production operators prefer queryable DB table. +- **kubectl, docker, npm, aws, psql production adapters**: framework + playbook ready (credential-adapter-playbook.md), implementation staged for Q3 2026. + +**Watch for:** +- Operators who copy legacy env-var workflows into the new system and expect them to "just work" — audit logs + docs should catch these, but social effort needed. +- SSH host-key TOFU attacks in zero-trust networks — ssh-keyscan baseline + pinned-host-keys (v2) are the mitigations. diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 549c34e4..1027b40d 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -4,6 +4,83 @@ Significant changes, features, and fixes in reverse chronological order. --- +## 2026-05-28 + +### CLI credential adapter framework + git adapter (issue #82) + +Refactors `credentialed_exec.go` from "flat env-var injection only" to a +generic `CredentialAdapter` interface that supports argv prefix injection, +ephemeral filesystem material, system-trusted env vars, and per-injection +redaction. Existing presets (`gh`, `aws`, `gcloud`, `kubectl`, `terraform`, +`gws`) route through a passthrough adapter and keep current behavior +bit-for-bit. + +**New** + +- `CredentialAdapter` interface + registry in `internal/tools/credential_adapter.go`. + Default `passthrough` adapter preserves legacy behavior; named adapters + register via `init() → RegisterAdapter`. +- `git` adapter (`internal/tools/credential_adapter_git.go`) covering both + HTTPS PAT and SSH key paths. PAT injected via `GIT_CONFIG_COUNT` + + `GIT_CONFIG_KEY_*`/`GIT_CONFIG_VALUE_*` env vars (never argv → keeps token + off `ps`/`/proc//cmdline`). SSH key materialized to 0600 tmpfile + + `GIT_SSH_COMMAND` with `IdentitiesOnly=yes` and `StrictHostKeyChecking=accept-new`. + Passphrase-protected SSH keys rejected at validation with + `git.cred_ssh_passphrase_unsupported`. +- `psql` framework-validation stub adapter (`internal/tools/credential_adapter_psql.go`) + proving the interface holds for non-git credential families. +- Shared `materializeEphemeral` helper (`internal/tools/credential_ephemeral.go`) + with idempotent cleanup latch and explicit `0600` chmod. +- Per-injection audit log: `slog.Warn("security.system_env_injection", …)` + with `adapter`, `binary`, `user_id`, sorted `env_keys` (names only), + `argv_prefix_len`, `host_scope_hash` (SHA-256 first 8 hex chars — plaintext + hostname intentionally omitted for PII safety). Schema pinned by + `TestEmitSystemEnvInjectionAudit_*`. +- Typed-credential HTTP PUT path with `{error:{code,message}, error_key}` + envelope so the web UI can drive field-level validation + (`git.cred_host_scope_required`, `git.cred_ssh_passphrase_unsupported`, + etc.). +- Web UI: `CliCredentialGitFields` extends the CLI Credentials dialog with + `Personal Access Token` / `SSH Private Key` picker, host-scope input, + CRLF→LF paste normalization, masked-secret edit flow (`••••••••` + placeholder preserves stored value on save). +- 17 i18n keys × 3 locales (en/vi/zh). + +**Docs** + +- New: `docs/git-credential-adapter.md` — user-facing guide (when to use PAT + vs SSH vs env, host-scope semantics, TOFU caveat with `ssh-keyscan` + mitigation, SIGKILL residual material note, operator sweep recipe). +- New: `docs/credential-adapter-playbook.md` — implementer guide with worked + mappings for `kubectl`, `docker`, `npm`, `aws`, `psql` and the three-question + interface-validation gate. +- `docs/09-security.md` § 14 — trust-boundary diagram, audit field schema, + SSH TOFU + SIGKILL caveats, v2 future-work list. +- `docs/03-tools-system.md` § 8a — adapter framework summary linking to the + playbook. + +**Security** + +- User-paste denylist (`ValidateGrantEnvVars`) unchanged — first line of + defense intact. Adapter path is the second, audit-trailed line; a typo in + `adapter_name` falls back to passthrough (no silent bypass). +- AES-256-GCM at rest for stored PAT/SSH bodies; secrets cannot be read back + via API/UI. +- `ScrubCredentials` redacts PAT bytes and SSH key path from stdout, stderr, + `Result.Content`, and audit log JSON. + +**Known v1 limitations** + +- One credential per (user, binary, host_scope). +- No multi-host wildcard (`*.github.com`). +- No persistent `known_hosts` per credential (TOFU only). +- No sandbox support (adapter is incompatible with bind-mount-based sandbox + path). +- No credential-refresh primitive (blocks future `aws sts assume-role` + adapter). + +--- + ## 2026-05-27 ### zuey VPS ops scripts: repo-tracked + CI auto-sync diff --git a/internal/http/secure_cli_env_response_test.go b/internal/http/secure_cli_env_response_test.go index 4e08bcd0..be2552a4 100644 --- a/internal/http/secure_cli_env_response_test.go +++ b/internal/http/secure_cli_env_response_test.go @@ -48,6 +48,9 @@ func (s *fakeSecureCLIStore) GetUserCredentials(context.Context, uuid.UUID, stri func (s *fakeSecureCLIStore) SetUserCredentials(context.Context, uuid.UUID, string, []byte) error { return nil } +func (s *fakeSecureCLIStore) SetUserCredentialsTyped(context.Context, uuid.UUID, string, []byte, *string, *string) error { + return nil +} func (s *fakeSecureCLIStore) DeleteUserCredentials(context.Context, uuid.UUID, string) error { return nil } diff --git a/internal/http/secure_cli_typed_credentials.go b/internal/http/secure_cli_typed_credentials.go new file mode 100644 index 00000000..48f31cae --- /dev/null +++ b/internal/http/secure_cli_typed_credentials.go @@ -0,0 +1,223 @@ +// Phase 5: typed credential write path for SecureCLIUserCredentials. +// +// The legacy PUT body `{env: {...}}` lives untouched in +// secure_cli_user_credentials.go; this file adds the new +// `{credential_type, host_scope, blob}` branches for typed adapters +// (pat / ssh_key). All validation runs BEFORE encryption + DB write so the +// store row never holds a malformed credential. +package http + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "regexp" + "strings" + + "golang.org/x/net/idna" + + "github.com/nextlevelbuilder/goclaw/internal/i18n" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// hostScopeRe matches lowercase hostnames with optional port. Applied AFTER +// idna.ToASCII normalization so IDN punycode is treated as plain ASCII. +// Restrictive on purpose — prevents CRLF injection (\r\n) and weird Unicode +// from landing in the host_scope column and later being used in a `git config` +// key like http.https:///.extraheader (Phase 3 wire shape). +var hostScopeRe = regexp.MustCompile(`^[a-z0-9.-]+(:[0-9]+)?$`) + +// typedCredentialBody is the additive PUT shape introduced in Phase 5. +// All three new fields are nullable so the legacy `{env: {...}}` path keeps +// working unchanged. +type typedCredentialBody struct { + CredentialType *string `json:"credential_type,omitempty"` + HostScope *string `json:"host_scope,omitempty"` + Blob json.RawMessage `json:"blob,omitempty"` +} + +// errTypedCredential carries both an HTTP status and the i18n-resolved +// message so handleSetUserCredentials can `writeJSON` once without leaking +// validation details through fmt.Sprintf. +type errTypedCredential struct { + status int + msg string + // errorKey is exposed to the frontend so dialogs can map specific failures + // (e.g. passphrase rejection) to inline UI feedback without parsing the + // localized human message. + errorKey string +} + +func (e *errTypedCredential) Error() string { return e.msg } + +// prepareTypedCredentialEnv validates and converts the new payload shape into +// the bytes that get encrypted-and-stored. Returns (envBytes, credentialType, +// hostScope, error). On success, envBytes is the JSON-encoded blob (e.g. +// `{"token":"..."}` or `{"key":"..."}`) ready for SetUserCredentialsTyped. +func prepareTypedCredentialEnv(locale string, body typedCredentialBody) ([]byte, *string, *string, *errTypedCredential) { + if body.CredentialType == nil || *body.CredentialType == "" || *body.CredentialType == "env" { + // Caller should route to legacy env path. Indicate that with all-nil + // returns + nil error; handler treats this as "fall through". + return nil, nil, nil, nil + } + + credType := *body.CredentialType + if credType != "pat" && credType != "ssh_key" { + return nil, nil, nil, &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredUnsupportedCredType, credType), + errorKey: "git.cred_unsupported_type", + } + } + + // Host scope is required + validated for both pat and ssh_key. + scope, terr := validateHostScope(locale, body.HostScope, credType) + if terr != nil { + return nil, nil, nil, terr + } + + if len(body.Blob) == 0 { + return nil, nil, nil, &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredBlobMissingField, "blob"), + errorKey: "git.cred_blob_missing", + } + } + var blob map[string]string + if err := json.Unmarshal(body.Blob, &blob); err != nil { + return nil, nil, nil, &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGrantEnvValueInvalid, err.Error()), + errorKey: "git.cred_blob_invalid", + } + } + + switch credType { + case "pat": + token := strings.TrimSpace(blob["token"]) + if token == "" { + return nil, nil, nil, &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredBlobMissingField, "token"), + errorKey: "git.cred_blob_missing_token", + } + } + if err := tools.ValidatePATToken(token); err != nil { + return nil, nil, nil, &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredTokenInvalid), + errorKey: "git.cred_token_invalid", + } + } + envBytes, _ := json.Marshal(map[string]string{"token": token}) + ct := credType + return envBytes, &ct, &scope, nil + + case "ssh_key": + // Windows clipboard often pastes CRLF; ssh.ParsePrivateKey expects LF + // and chokes silently otherwise. Normalize BEFORE validation so the + // stored bytes match what ssh -i will read at exec time. + key := strings.ReplaceAll(blob["key"], "\r\n", "\n") + if strings.TrimSpace(key) == "" { + return nil, nil, nil, &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredBlobMissingField, "key"), + errorKey: "git.cred_blob_missing_key", + } + } + if err := tools.ValidateSSHKey([]byte(key)); err != nil { + if errors.Is(err, tools.ErrSSHKeyPassphraseUnsupported) { + return nil, nil, nil, &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredSSHPassphraseUnsupported), + errorKey: "git.cred_ssh_passphrase_unsupported", + } + } + return nil, nil, nil, &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredSSHKeyInvalid, err.Error()), + errorKey: "git.cred_ssh_key_invalid", + } + } + envBytes, _ := json.Marshal(map[string]string{"key": key}) + ct := credType + return envBytes, &ct, &scope, nil + } + + // Unreachable — credType already gated above. + return nil, nil, nil, &errTypedCredential{ + status: http.StatusInternalServerError, + msg: fmt.Sprintf("unreachable credential_type %q", credType), + } +} + +// validateHostScope normalizes via idna.ToASCII (matches Phase 3's runtime +// comparison) then enforces hostScopeRe. Returns the canonical host_scope +// the row should store. +func validateHostScope(locale string, raw *string, credType string) (string, *errTypedCredential) { + if raw == nil || strings.TrimSpace(*raw) == "" { + return "", &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredHostScopeRequired, credType), + errorKey: "git.cred_host_scope_required", + } + } + scope := strings.ToLower(strings.TrimSuffix(strings.TrimSpace(*raw), ".")) + + // Optional host:port split — only the host part goes through idna. + hostPart, port, hasPort := splitHostScopePort(scope) + ascii, err := idna.Lookup.ToASCII(hostPart) + if err != nil { + return "", &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredHostScopeInvalid, *raw), + errorKey: "git.cred_host_scope_invalid", + } + } + normalized := ascii + if hasPort { + normalized = ascii + ":" + port + } + if !hostScopeRe.MatchString(normalized) { + return "", &errTypedCredential{ + status: http.StatusBadRequest, + msg: i18n.T(locale, i18n.MsgGitCredHostScopeInvalid, *raw), + errorKey: "git.cred_host_scope_invalid", + } + } + return normalized, nil +} + +func splitHostScopePort(h string) (host, port string, ok bool) { + idx := strings.LastIndex(h, ":") + if idx < 0 { + return h, "", false + } + port = h[idx+1:] + if port == "" { + return h, "", false + } + for _, r := range port { + if r < '0' || r > '9' { + return h, "", false + } + } + return h[:idx], port, true +} + +// writeTypedCredentialError serializes errTypedCredential into the standard +// gateway error envelope `{error:{code,message}}` so the shared HttpClient +// parser picks up `code` as `error_key`. We ALSO keep a flat `error_key` field +// at the top level for older clients and tests that parse the body directly. +func writeTypedCredentialError(w http.ResponseWriter, e *errTypedCredential) { + inner := map[string]string{"message": e.msg} + if e.errorKey != "" { + inner["code"] = e.errorKey + } + body := map[string]any{"error": inner} + if e.errorKey != "" { + body["error_key"] = e.errorKey + } + writeJSON(w, e.status, body) +} diff --git a/internal/http/secure_cli_typed_credentials_test.go b/internal/http/secure_cli_typed_credentials_test.go new file mode 100644 index 00000000..bc0f822c --- /dev/null +++ b/internal/http/secure_cli_typed_credentials_test.go @@ -0,0 +1,249 @@ +// Phase 5 tests for the typed-credential PUT branches on +// handleSetUserCredentials. Backend equivalents of the frontend Vitest cases: +// PAT happy path, SSH passphrase rejection, missing host_scope, legacy env +// unchanged. Uses a recording fake store to assert exactly what landed. +package http + +import ( + "bytes" + "context" + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/uuid" + "golang.org/x/crypto/ssh" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// recordingSecureCLIStore captures the args of every Set*Credentials call so +// tests can assert exactly what bytes landed at the encryption boundary. +type recordingSecureCLIStore struct { + fakeSecureCLIStore + + lastTypedEnv []byte + lastTypedType *string + lastTypedScope *string + lastLegacyEnv []byte + typedCalls int + legacyCalls int + existingForGet *store.SecureCLIUserCredential +} + +func (s *recordingSecureCLIStore) SetUserCredentialsTyped(_ context.Context, _ uuid.UUID, _ string, encryptedEnv []byte, credentialType, hostScope *string) error { + s.lastTypedEnv = append([]byte(nil), encryptedEnv...) + s.lastTypedType = credentialType + s.lastTypedScope = hostScope + s.typedCalls++ + return nil +} + +func (s *recordingSecureCLIStore) SetUserCredentials(_ context.Context, _ uuid.UUID, _ string, encryptedEnv []byte) error { + s.lastLegacyEnv = append([]byte(nil), encryptedEnv...) + s.legacyCalls++ + return nil +} + +func (s *recordingSecureCLIStore) GetUserCredentials(context.Context, uuid.UUID, string) (*store.SecureCLIUserCredential, error) { + if s.existingForGet == nil { + return nil, nil + } + cp := *s.existingForGet + return &cp, nil +} + +func putUserCred(t *testing.T, h *SecureCLIHandler, binaryID uuid.UUID, body any) *httptest.ResponseRecorder { + t.Helper() + buf, _ := json.Marshal(body) + req := httptest.NewRequest(http.MethodPut, "/v1/cli-credentials/"+binaryID.String()+"/user-credentials/u1", bytes.NewReader(buf)) + req.SetPathValue("id", binaryID.String()) + req.SetPathValue("userId", "u1") + rec := httptest.NewRecorder() + h.handleSetUserCredentials(rec, req) + return rec +} + +// 9. Handler accepts new PAT payload and routes through SetUserCredentialsTyped. +func TestPutUserCredential_PATPayload(t *testing.T) { + st := &recordingSecureCLIStore{} + h := NewSecureCLIHandler(st, nil) + binaryID := uuid.New() + + rec := putUserCred(t, h, binaryID, map[string]any{ + "credential_type": "pat", + "host_scope": "github.com", + "blob": map[string]string{"token": "ghp_abcDEF123456"}, + }) + if rec.Code != http.StatusOK { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + if st.typedCalls != 1 || st.legacyCalls != 0 { + t.Fatalf("expected typed=1 legacy=0, got typed=%d legacy=%d", st.typedCalls, st.legacyCalls) + } + if st.lastTypedType == nil || *st.lastTypedType != "pat" { + t.Fatalf("type mismatch: %#v", st.lastTypedType) + } + if st.lastTypedScope == nil || *st.lastTypedScope != "github.com" { + t.Fatalf("scope mismatch: %#v", st.lastTypedScope) + } + // Stored bytes must be exactly the blob the runtime decodes. + var got map[string]string + if err := json.Unmarshal(st.lastTypedEnv, &got); err != nil { + t.Fatal(err) + } + if got["token"] != "ghp_abcDEF123456" { + t.Fatalf("stored token mismatch: %#v", got) + } + // Response body must never echo the secret. + if strings.Contains(rec.Body.String(), "ghp_abcDEF") { + t.Fatalf("response leaked token: %s", rec.Body.String()) + } +} + +// 10. Handler rejects passphrase-protected SSH key with error_key. +func TestPutUserCredential_RejectsPassphraseKey(t *testing.T) { + st := &recordingSecureCLIStore{} + h := NewSecureCLIHandler(st, nil) + binaryID := uuid.New() + + pem := genPassphrasePEM(t, "topsecret") + rec := putUserCred(t, h, binaryID, map[string]any{ + "credential_type": "ssh_key", + "host_scope": "github.com", + "blob": map[string]string{"key": string(pem)}, + }) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + var resp map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatal(err) + } + if resp["error_key"] != "git.cred_ssh_passphrase_unsupported" { + t.Fatalf("expected error_key=git.cred_ssh_passphrase_unsupported, got %#v", resp) + } + if st.typedCalls != 0 || st.legacyCalls != 0 { + t.Fatalf("no DB write expected on passphrase reject, got typed=%d legacy=%d", st.typedCalls, st.legacyCalls) + } +} + +// 11. Handler rejects PAT/SSH payload missing host_scope. +func TestPutUserCredential_PATNoHostScope(t *testing.T) { + st := &recordingSecureCLIStore{} + h := NewSecureCLIHandler(st, nil) + binaryID := uuid.New() + + rec := putUserCred(t, h, binaryID, map[string]any{ + "credential_type": "pat", + "blob": map[string]string{"token": "ghp_xyz"}, + }) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + var resp map[string]any + _ = json.Unmarshal(rec.Body.Bytes(), &resp) + if resp["error_key"] != "git.cred_host_scope_required" { + t.Fatalf("expected error_key=git.cred_host_scope_required, got %#v", resp) + } + if st.typedCalls != 0 { + t.Fatalf("no DB write expected on validation reject") + } +} + +// 12. Legacy env-paste body keeps the legacy code path unchanged. +func TestPutUserCredential_LegacyEnvUnchanged(t *testing.T) { + st := &recordingSecureCLIStore{} + h := NewSecureCLIHandler(st, nil) + binaryID := uuid.New() + + rec := putUserCred(t, h, binaryID, map[string]any{ + "env": map[string]string{"GH_TOKEN": "ghp_legacy"}, + }) + if rec.Code != http.StatusOK { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + if st.legacyCalls != 1 || st.typedCalls != 0 { + t.Fatalf("expected legacy=1 typed=0, got legacy=%d typed=%d", st.legacyCalls, st.typedCalls) + } +} + +// 11b. SSH key CRLF normalization — pasted Windows-encoded key must be saved +// as LF so ssh.ParsePrivateKey at exec time succeeds. Belt-and-suspenders +// since the validator already runs on the normalized bytes. +func TestPutUserCredential_SSHKeyCRLFNormalized(t *testing.T) { + st := &recordingSecureCLIStore{} + h := NewSecureCLIHandler(st, nil) + binaryID := uuid.New() + + pem := genUnencryptedPEM(t) + crlf := strings.ReplaceAll(string(pem), "\n", "\r\n") + rec := putUserCred(t, h, binaryID, map[string]any{ + "credential_type": "ssh_key", + "host_scope": "github.com", + "blob": map[string]string{"key": crlf}, + }) + if rec.Code != http.StatusOK { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + var stored map[string]string + if err := json.Unmarshal(st.lastTypedEnv, &stored); err != nil { + t.Fatal(err) + } + if strings.Contains(stored["key"], "\r") { + t.Fatalf("CRLF leaked into stored key: %q", stored["key"]) + } +} + +// genUnencryptedPEM mirrors the helper in tools/credential_adapter_git_ssh_test +// but lives here to avoid cross-package test deps. +func genUnencryptedPEM(t *testing.T) []byte { + t.Helper() + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("gen: %v", err) + } + block, err := ssh.MarshalPrivateKey(priv, "") + if err != nil { + t.Fatalf("marshal: %v", err) + } + return []byte("-----BEGIN OPENSSH PRIVATE KEY-----\n" + + base64Wrap(block.Bytes) + + "\n-----END OPENSSH PRIVATE KEY-----\n") +} + +func genPassphrasePEM(t *testing.T, pw string) []byte { + t.Helper() + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("gen: %v", err) + } + block, err := ssh.MarshalPrivateKeyWithPassphrase(priv, "", []byte(pw)) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return []byte("-----BEGIN OPENSSH PRIVATE KEY-----\n" + + base64Wrap(block.Bytes) + + "\n-----END OPENSSH PRIVATE KEY-----\n") +} + +func base64Wrap(b []byte) string { + s := base64.StdEncoding.EncodeToString(b) + var out strings.Builder + for i := 0; i < len(s); i += 70 { + end := i + 70 + if end > len(s) { + end = len(s) + } + if i > 0 { + out.WriteByte('\n') + } + out.WriteString(s[i:end]) + } + return out.String() +} diff --git a/internal/http/secure_cli_user_credentials.go b/internal/http/secure_cli_user_credentials.go index 5f089acf..f4f0a428 100644 --- a/internal/http/secure_cli_user_credentials.go +++ b/internal/http/secure_cli_user_credentials.go @@ -23,30 +23,43 @@ func (h *SecureCLIHandler) handleListUserCredentials(w http.ResponseWriter, r *h writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, err.Error())}) return } - // Return without env values for listing (names only + timestamps) + // Return without env values for listing (names only + timestamps). + // credential_type + host_scope surface so the table can show a Type badge + // (PAT / SSH / Env) without a second round-trip per row. type entry struct { - ID uuid.UUID `json:"id"` - BinaryID uuid.UUID `json:"binary_id"` - UserID string `json:"user_id"` - HasEnv bool `json:"has_env"` - EnvKeys []string `json:"env_keys,omitempty"` - Env map[string]store.SecureCLIEnvResponseEntry `json:"env,omitempty"` - CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` + ID uuid.UUID `json:"id"` + BinaryID uuid.UUID `json:"binary_id"` + UserID string `json:"user_id"` + HasEnv bool `json:"has_env"` + EnvKeys []string `json:"env_keys,omitempty"` + Env map[string]store.SecureCLIEnvResponseEntry `json:"env,omitempty"` + CredentialType *string `json:"credential_type,omitempty"` + HostScope *string `json:"host_scope,omitempty"` + CreatedAt string `json:"created_at"` + UpdatedAt string `json:"updated_at"` } entries := make([]entry, 0, len(creds)) for _, c := range creds { envKeys := envKeysFromDecryptedJSON(c.EncryptedEnv) - entries = append(entries, entry{ - ID: c.ID, - BinaryID: c.BinaryID, - UserID: c.UserID, - HasEnv: len(c.EncryptedEnv) > 0, - EnvKeys: envKeys, - Env: store.SanitizeSecureCLIEnvJSON(c.EncryptedEnv), - CreatedAt: c.CreatedAt, - UpdatedAt: c.UpdatedAt, - }) + // Typed credentials (pat / ssh_key) hold a single secret in the blob. + // Suppress env / env_keys for them — those keys would leak the wire + // shape (`token` / `key`) to the listing endpoint. + isTyped := c.CredentialType != nil && *c.CredentialType != "" && *c.CredentialType != "env" + e := entry{ + ID: c.ID, + BinaryID: c.BinaryID, + UserID: c.UserID, + HasEnv: len(c.EncryptedEnv) > 0, + CredentialType: c.CredentialType, + HostScope: c.HostScope, + CreatedAt: c.CreatedAt, + UpdatedAt: c.UpdatedAt, + } + if !isTyped { + e.EnvKeys = envKeys + e.Env = store.SanitizeSecureCLIEnvJSON(c.EncryptedEnv) + } + entries = append(entries, e) } writeJSON(w, http.StatusOK, map[string]any{"user_credentials": entries}) } @@ -75,10 +88,18 @@ func (h *SecureCLIHandler) handleGetUserCredentials(w http.ResponseWriter, r *ht return } - writeJSON(w, http.StatusOK, map[string]any{ - "user_id": cred.UserID, - "env": store.SanitizeSecureCLIEnvJSON(cred.EncryptedEnv), - }) + // Typed creds suppress env to avoid leaking the blob shape (`token`/`key`). + isTyped := cred.CredentialType != nil && *cred.CredentialType != "" && *cred.CredentialType != "env" + resp := map[string]any{ + "user_id": cred.UserID, + "credential_type": cred.CredentialType, + "host_scope": cred.HostScope, + "has_secret": len(cred.EncryptedEnv) > 0, + } + if !isTyped { + resp["env"] = store.SanitizeSecureCLIEnvJSON(cred.EncryptedEnv) + } + writeJSON(w, http.StatusOK, resp) } func (h *SecureCLIHandler) handleSetUserCredentials(w http.ResponseWriter, r *http.Request) { @@ -96,10 +117,33 @@ func (h *SecureCLIHandler) handleSetUserCredentials(w http.ResponseWriter, r *ht var body struct { Env json.RawMessage `json:"env"` + typedCredentialBody } if !bindJSON(w, r, locale, &body) { return } + + // Typed branch (pat / ssh_key): validate + encrypt blob, store with + // credential_type + host_scope. Audit emits credential_type only — never + // the secret or host (host is operator-visible config but still scoped to + // the audit channel). + envBytes, credType, hostScope, terr := prepareTypedCredentialEnv(locale, body.typedCredentialBody) + if terr != nil { + writeTypedCredentialError(w, terr) + return + } + if envBytes != nil { + if err := h.store.SetUserCredentialsTyped(r.Context(), binaryID, userID, envBytes, credType, hostScope); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, err.Error())}) + return + } + emitAudit(h.msgBus, r, "secure_cli.user_credentials.updated", "secure_cli_user_credentials", binaryID.String()+"/"+userID+"#"+*credType) + h.emitCacheInvalidate("") + writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) + return + } + + // Legacy env-paste branch (no credential_type or credential_type="env"). if len(body.Env) == 0 { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "env is required"}) return @@ -107,7 +151,6 @@ func (h *SecureCLIHandler) handleSetUserCredentials(w http.ResponseWriter, r *ht existing, err := h.store.GetUserCredentials(r.Context(), binaryID, userID) if err != nil { - locale := store.LocaleFromContext(r.Context()) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, err.Error())}) return } @@ -122,7 +165,6 @@ func (h *SecureCLIHandler) handleSetUserCredentials(w http.ResponseWriter, r *ht } if err := h.store.SetUserCredentials(r.Context(), binaryID, userID, envJSON); err != nil { - locale := store.LocaleFromContext(r.Context()) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, err.Error())}) return } diff --git a/internal/i18n/catalog_en.go b/internal/i18n/catalog_en.go index 9ec12d1e..1bbcfb60 100644 --- a/internal/i18n/catalog_en.go +++ b/internal/i18n/catalog_en.go @@ -307,6 +307,20 @@ func init() { MsgGrantEnvTooManyKeys: "too many env keys: max 50", MsgGrantEnvRevealLimit: "rate limit exceeded for env reveal — try again later", + // Git credential adapter + MsgGitCredHostMismatch: "stored git credential is for %s but command targets %s", + MsgGitCredNoMatch: "no git credential configured for host %s", + MsgGitCredUnsupportedType: "git credential type %q is not supported", + MsgGitCredTokenInvalid: "stored git token is invalid or empty", + MsgGitCredTokenControlChar: "stored git token contains forbidden control characters", + MsgGitCredHostUserinfoRejected: "git URL with embedded userinfo is rejected as ambiguous", + MsgGitCredSSHPassphraseUnsupported: "passphrase-protected SSH keys are not supported; remove the passphrase with `ssh-keygen -p` before saving", + MsgGitCredSSHKeyInvalid: "SSH private key invalid: %s", + MsgGitCredHostScopeRequired: "host_scope is required for credential_type %s", + MsgGitCredHostScopeInvalid: "host_scope %q is not a valid hostname", + MsgGitCredBlobMissingField: "credential blob is missing required field %q", + MsgGitCredUnsupportedCredType: "credential_type %q is not supported", + // Message tool cross-target forward notice MessageCrossTargetForwarded: "📤 Forwarded to %s as requested: %q", diff --git a/internal/i18n/catalog_vi.go b/internal/i18n/catalog_vi.go index 4dc62ea8..78b31cd1 100644 --- a/internal/i18n/catalog_vi.go +++ b/internal/i18n/catalog_vi.go @@ -307,6 +307,20 @@ func init() { MsgGrantEnvTooManyKeys: "quá nhiều khóa env: tối đa 50", MsgGrantEnvRevealLimit: "đã vượt giới hạn yêu cầu xem env — vui lòng thử lại sau", + // Git credential adapter + MsgGitCredHostMismatch: "thông tin xác thực git đã lưu dành cho %s nhưng lệnh đang nhắm tới %s", + MsgGitCredNoMatch: "không có thông tin xác thực git cho host %s", + MsgGitCredUnsupportedType: "loại thông tin xác thực git %q không được hỗ trợ", + MsgGitCredTokenInvalid: "token git đã lưu không hợp lệ hoặc rỗng", + MsgGitCredTokenControlChar: "token git đã lưu chứa ký tự điều khiển bị cấm", + MsgGitCredHostUserinfoRejected: "URL git chứa userinfo nhúng bị từ chối vì gây nhập nhằng", + MsgGitCredSSHPassphraseUnsupported: "khóa SSH có passphrase chưa được hỗ trợ; chạy `ssh-keygen -p` để gỡ passphrase trước khi lưu", + MsgGitCredSSHKeyInvalid: "khóa riêng SSH không hợp lệ: %s", + MsgGitCredHostScopeRequired: "host_scope là bắt buộc cho credential_type %s", + MsgGitCredHostScopeInvalid: "host_scope %q không phải là tên máy chủ hợp lệ", + MsgGitCredBlobMissingField: "blob credential thiếu trường bắt buộc %q", + MsgGitCredUnsupportedCredType: "credential_type %q không được hỗ trợ", + // Message tool cross-target forward notice MessageCrossTargetForwarded: "📤 Đã forward sang %s theo yêu cầu: %q", diff --git a/internal/i18n/catalog_zh.go b/internal/i18n/catalog_zh.go index 85d88923..2fddc6af 100644 --- a/internal/i18n/catalog_zh.go +++ b/internal/i18n/catalog_zh.go @@ -307,6 +307,20 @@ func init() { MsgGrantEnvTooManyKeys: "环境变量键过多：最多 50 个", MsgGrantEnvRevealLimit: "env 查看请求超出速率限制，请稍后再试", + // Git 凭据适配器 + MsgGitCredHostMismatch: "已存储的 git 凭据属于 %s，但命令目标是 %s", + MsgGitCredNoMatch: "没有为主机 %s 配置 git 凭据", + MsgGitCredUnsupportedType: "不支持的 git 凭据类型 %q", + MsgGitCredTokenInvalid: "已存储的 git token 无效或为空", + MsgGitCredTokenControlChar: "已存储的 git token 包含被禁止的控制字符", + MsgGitCredHostUserinfoRejected: "git URL 中嵌入的 userinfo 因含义不明被拒绝", + MsgGitCredSSHPassphraseUnsupported: "暂不支持带密码短语的 SSH 私钥；请先用 `ssh-keygen -p` 移除密码短语再保存", + MsgGitCredSSHKeyInvalid: "SSH 私钥无效:%s", + MsgGitCredHostScopeRequired: "credential_type %s 需要 host_scope", + MsgGitCredHostScopeInvalid: "host_scope %q 不是有效的主机名", + MsgGitCredBlobMissingField: "凭据 blob 缺少必填字段 %q", + MsgGitCredUnsupportedCredType: "不支持的 credential_type %q", + // Message tool cross-target forward notice MessageCrossTargetForwarded: "📤 已按请求转发至 %s:%q", diff --git a/internal/i18n/git_keys_parity_test.go b/internal/i18n/git_keys_parity_test.go new file mode 100644 index 00000000..ff979e51 --- /dev/null +++ b/internal/i18n/git_keys_parity_test.go @@ -0,0 +1,46 @@ +// Phase 5: catalog parity for all Git credential keys. +// +// Why a dedicated test: a new git i18n key added to keys.go without +// translations in all three catalogs would silently fall back to the key +// string in production for vi/zh users. This test enumerates every +// MsgGitCred* constant via the registered catalogs and asserts each locale +// returns a non-key, non-empty message. +package i18n + +import ( + "strings" + "testing" +) + +// gitKeys lists every git credential key. Kept hand-curated (one line each) +// rather than reflect-walked because the constants are in a single block in +// keys.go and adding a new one is a one-line change here too. +var gitKeys = []string{ + MsgGitCredHostMismatch, + MsgGitCredNoMatch, + MsgGitCredUnsupportedType, + MsgGitCredTokenInvalid, + MsgGitCredTokenControlChar, + MsgGitCredHostUserinfoRejected, + MsgGitCredSSHPassphraseUnsupported, + MsgGitCredSSHKeyInvalid, + MsgGitCredHostScopeRequired, + MsgGitCredHostScopeInvalid, + MsgGitCredBlobMissingField, + MsgGitCredUnsupportedCredType, +} + +func TestI18nCatalogs_HasGitKeys(t *testing.T) { + for _, locale := range []string{LocaleEN, LocaleVI, LocaleZH} { + for _, key := range gitKeys { + msg := lookup(locale, key) + if msg == key { + t.Errorf("locale=%s key=%s falls back to key string (translation missing)", locale, key) + continue + } + if strings.TrimSpace(msg) == "" { + t.Errorf("locale=%s key=%s has empty translation", locale, key) + } + } + } +} diff --git a/internal/i18n/keys.go b/internal/i18n/keys.go index 6d31d667..9f93a239 100644 --- a/internal/i18n/keys.go +++ b/internal/i18n/keys.go @@ -338,4 +338,18 @@ const ( MsgGrantEnvValueInvalid = "error.grant_env_value_invalid" // "invalid env value: %s" MsgGrantEnvTooManyKeys = "error.grant_env_too_many_keys" // "too many env keys: max 50" MsgGrantEnvRevealLimit = "error.grant_env_reveal_limit" // "rate limit exceeded for env reveal" + + // --- Git credential adapter (Phase 3+) --- + MsgGitCredHostMismatch = "error.git_cred_host_mismatch" // "stored credential is for %s but command targets %s" + MsgGitCredNoMatch = "error.git_cred_no_match" // "no git credential configured for host %s" + MsgGitCredUnsupportedType = "error.git_cred_unsupported_type" // "git credential type %q is not supported" + MsgGitCredTokenInvalid = "error.git_cred_token_invalid" // "stored git token is invalid or empty" + MsgGitCredTokenControlChar = "error.git_cred_token_control_char" // "stored git token contains forbidden control characters" + MsgGitCredHostUserinfoRejected = "error.git_cred_host_userinfo_rejected" // "git URL with embedded userinfo is rejected as ambiguous" + MsgGitCredSSHPassphraseUnsupported = "error.git_cred_ssh_passphrase_unsupported" // "passphrase-protected SSH keys not supported in v1" + MsgGitCredSSHKeyInvalid = "error.git_cred_ssh_key_invalid" // "SSH private key invalid: %s" + MsgGitCredHostScopeRequired = "error.git_cred_host_scope_required" // "host_scope required for credential_type %s" + MsgGitCredHostScopeInvalid = "error.git_cred_host_scope_invalid" // "host_scope %q is not a valid hostname" + MsgGitCredBlobMissingField = "error.git_cred_blob_missing_field" // "blob missing required field %q" + MsgGitCredUnsupportedCredType = "error.git_cred_unsupported_cred_type" // "credential_type %q is not supported" ) diff --git a/internal/store/pg/secure_cli.go b/internal/store/pg/secure_cli.go index 1bd5ef41..b0b7eacf 100644 --- a/internal/store/pg/secure_cli.go +++ b/internal/store/pg/secure_cli.go @@ -27,12 +27,14 @@ func NewPGSecureCLIStore(db *sql.DB, encryptionKey string) *PGSecureCLIStore { } const secureCLISelectCols = `id, binary_name, binary_path, description, encrypted_env, - deny_args, deny_verbose, timeout_seconds, tips, is_global, enabled, created_by, created_at, updated_at` + deny_args, deny_verbose, timeout_seconds, tips, is_global, enabled, created_by, + adapter_name, created_at, updated_at` // secureCLISelectColsAliased is prefixed with table alias "b." // Required for LookupByBinary which uses LEFT JOIN (ambiguous column names without prefix). const secureCLISelectColsAliased = `b.id, b.binary_name, b.binary_path, b.description, b.encrypted_env, - b.deny_args, b.deny_verbose, b.timeout_seconds, b.tips, b.is_global, b.enabled, b.created_by, b.created_at, b.updated_at` + b.deny_args, b.deny_verbose, b.timeout_seconds, b.tips, b.is_global, b.enabled, b.created_by, + b.adapter_name, b.created_at, b.updated_at` func (s *PGSecureCLIStore) Create(ctx context.Context, b *store.SecureCLIBinary) error { if err := store.ValidateUserID(b.CreatedBy); err != nil { @@ -69,14 +71,15 @@ func (s *PGSecureCLIStore) Create(ctx context.Context, b *store.SecureCLIBinary) _, err := s.db.ExecContext(ctx, `INSERT INTO secure_cli_binaries (id, binary_name, binary_path, description, encrypted_env, - deny_args, deny_verbose, timeout_seconds, tips, is_global, enabled, created_by, created_at, updated_at, tenant_id) - VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15)`, + deny_args, deny_verbose, timeout_seconds, tips, is_global, enabled, created_by, + adapter_name, created_at, updated_at, tenant_id) + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16)`, b.ID, b.BinaryName, nilStr(derefStr(b.BinaryPath)), b.Description, envBytes, jsonOrEmptyArray(b.DenyArgs), jsonOrEmptyArray(b.DenyVerbose), b.TimeoutSeconds, b.Tips, b.IsGlobal, b.Enabled, - b.CreatedBy, now, now, tenantID, + b.CreatedBy, b.AdapterName, now, now, tenantID, ) return err } @@ -106,7 +109,7 @@ func (s *PGSecureCLIStore) scanRow(row *sql.Row) (*store.SecureCLIBinary, error) &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &b.CreatedAt, &b.UpdatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &b.CreatedAt, &b.UpdatedAt, ) if err != nil { return nil, err @@ -148,7 +151,7 @@ func (s *PGSecureCLIStore) scanRows(rows *sql.Rows) ([]store.SecureCLIBinary, er &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &b.CreatedAt, &b.UpdatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &b.CreatedAt, &b.UpdatedAt, ); err != nil { continue } @@ -178,7 +181,7 @@ var secureCLIAllowedFields = map[string]bool{ "binary_name": true, "binary_path": true, "description": true, "encrypted_env": true, "deny_args": true, "deny_verbose": true, "timeout_seconds": true, "tips": true, "is_global": true, "enabled": true, - "updated_at": true, + "adapter_name": true, "updated_at": true, } func (s *PGSecureCLIStore) Update(ctx context.Context, id uuid.UUID, updates map[string]any) error { @@ -299,7 +302,7 @@ func (s *PGSecureCLIStore) scanRowsWithGrants(rows *sql.Rows) ([]store.SecureCLI &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &b.CreatedAt, &b.UpdatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &b.CreatedAt, &b.UpdatedAt, &grantsJSON, ); err != nil { continue @@ -348,9 +351,9 @@ func (s *PGSecureCLIStore) LookupByBinary(ctx context.Context, binaryName string var joinClause string if userID != "" { - selectCols += ", uc.encrypted_env AS user_env" + selectCols += ", uc.encrypted_env AS user_env, uc.credential_type AS user_cred_type, uc.host_scope AS user_host_scope" } else { - selectCols += ", NULL AS user_env" + selectCols += ", NULL AS user_env, NULL AS user_cred_type, NULL AS user_host_scope" } var args []any @@ -424,16 +427,17 @@ func (s *PGSecureCLIStore) scanRowWithGrantAndUserEnv(row *sql.Row) (*store.Secu var grantID *uuid.UUID var grantEncEnv []byte var userEnv []byte + var userCredType, userHostScope *string err := row.Scan( &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &b.CreatedAt, &b.UpdatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &b.CreatedAt, &b.UpdatedAt, // Grant columns &grantDenyArgs, &grantDenyVerbose, &grantTimeout, &grantTips, &grantEnabled, &grantID, &grantEncEnv, - // User env - &userEnv, + // User credential columns + &userEnv, &userCredType, &userHostScope, ) if err != nil { if errors.Is(err, sql.ErrNoRows) { @@ -487,6 +491,9 @@ func (s *PGSecureCLIStore) scanRowWithGrantAndUserEnv(row *sql.Row) (*store.Secu b.UserEnv = []byte(decrypted) } } + // Project per-user credential metadata so adapters can branch on it. + b.UserCredentialType = userCredType + b.UserHostScope = userHostScope return &b, nil } @@ -591,7 +598,7 @@ func (s *PGSecureCLIStore) ListForAgent(ctx context.Context, agentID uuid.UUID) &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &b.CreatedAt, &b.UpdatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &b.CreatedAt, &b.UpdatedAt, &grantDenyArgs, &grantDenyVerbose, &grantTimeout, &grantTips, &grantID, &grantEncEnv, ); err != nil { continue diff --git a/internal/store/pg/secure_cli_user_credentials.go b/internal/store/pg/secure_cli_user_credentials.go index 91ada577..7d2ef1b9 100644 --- a/internal/store/pg/secure_cli_user_credentials.go +++ b/internal/store/pg/secure_cli_user_credentials.go @@ -13,6 +13,11 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/store" ) +// userCredSelectCols projects every column callers read off SecureCLIUserCredential. +// Keep in lockstep with the struct in internal/store/secure_cli_store.go. +const userCredSelectCols = `id, binary_id, user_id, encrypted_env, metadata, + credential_type, host_scope, created_at, updated_at` + func (s *PGSecureCLIStore) GetUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string) (*store.SecureCLIUserCredential, error) { tid := store.TenantIDFromContext(ctx) if tid == uuid.Nil { @@ -21,11 +26,12 @@ func (s *PGSecureCLIStore) GetUserCredentials(ctx context.Context, binaryID uuid var uc store.SecureCLIUserCredential var env []byte err := s.db.QueryRowContext(ctx, - `SELECT id, binary_id, user_id, encrypted_env, metadata, created_at, updated_at + `SELECT `+userCredSelectCols+` FROM secure_cli_user_credentials WHERE binary_id = $1 AND user_id = $2 AND tenant_id = $3`, binaryID, userID, tid, - ).Scan(&uc.ID, &uc.BinaryID, &uc.UserID, &env, &uc.Metadata, &uc.CreatedAt, &uc.UpdatedAt) + ).Scan(&uc.ID, &uc.BinaryID, &uc.UserID, &env, &uc.Metadata, + &uc.CredentialType, &uc.HostScope, &uc.CreatedAt, &uc.UpdatedAt) if errors.Is(err, sql.ErrNoRows) { return nil, nil } @@ -43,7 +49,15 @@ func (s *PGSecureCLIStore) GetUserCredentials(ctx context.Context, binaryID uuid return &uc, nil } +// SetUserCredentials writes a legacy env-vars credential (credential_type / host_scope NULL). +// Preserves all pre-existing callers. func (s *PGSecureCLIStore) SetUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte) error { + return s.SetUserCredentialsTyped(ctx, binaryID, userID, encryptedEnv, nil, nil) +} + +// SetUserCredentialsTyped is the typed-credential entry point. +// credentialType / hostScope are NULL for legacy env-only credentials. +func (s *PGSecureCLIStore) SetUserCredentialsTyped(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte, credentialType, hostScope *string) error { tid := store.TenantIDFromContext(ctx) if tid == uuid.Nil { tid = store.MasterTenantID @@ -62,12 +76,16 @@ func (s *PGSecureCLIStore) SetUserCredentials(ctx context.Context, binaryID uuid now := time.Now() _, err := s.db.ExecContext(ctx, - `INSERT INTO secure_cli_user_credentials (binary_id, user_id, encrypted_env, metadata, tenant_id, created_at, updated_at) - VALUES ($1, $2, $3, '{}', $4, $5, $5) + `INSERT INTO secure_cli_user_credentials + (binary_id, user_id, encrypted_env, metadata, tenant_id, + credential_type, host_scope, created_at, updated_at) + VALUES ($1, $2, $3, '{}', $4, $5, $6, $7, $7) ON CONFLICT (binary_id, user_id, tenant_id) DO UPDATE SET - encrypted_env = EXCLUDED.encrypted_env, - updated_at = EXCLUDED.updated_at`, - binaryID, userID, envBytes, tid, now, + encrypted_env = EXCLUDED.encrypted_env, + credential_type = EXCLUDED.credential_type, + host_scope = EXCLUDED.host_scope, + updated_at = EXCLUDED.updated_at`, + binaryID, userID, envBytes, tid, credentialType, hostScope, now, ) return err } @@ -90,7 +108,7 @@ func (s *PGSecureCLIStore) ListUserCredentials(ctx context.Context, binaryID uui tid = store.MasterTenantID } rows, err := s.db.QueryContext(ctx, - `SELECT id, binary_id, user_id, encrypted_env, metadata, created_at, updated_at + `SELECT `+userCredSelectCols+` FROM secure_cli_user_credentials WHERE binary_id = $1 AND tenant_id = $2 ORDER BY created_at`, binaryID, tid) @@ -103,7 +121,8 @@ func (s *PGSecureCLIStore) ListUserCredentials(ctx context.Context, binaryID uui for rows.Next() { var uc store.SecureCLIUserCredential var env []byte - if err := rows.Scan(&uc.ID, &uc.BinaryID, &uc.UserID, &env, &uc.Metadata, &uc.CreatedAt, &uc.UpdatedAt); err != nil { + if err := rows.Scan(&uc.ID, &uc.BinaryID, &uc.UserID, &env, &uc.Metadata, + &uc.CredentialType, &uc.HostScope, &uc.CreatedAt, &uc.UpdatedAt); err != nil { return nil, err } if len(env) > 0 && s.encKey != "" { diff --git a/internal/store/secure_cli_store.go b/internal/store/secure_cli_store.go index bac2a08d..b3c9955f 100644 --- a/internal/store/secure_cli_store.go +++ b/internal/store/secure_cli_store.go @@ -34,7 +34,16 @@ type SecureCLIBinary struct { IsGlobal bool `json:"is_global" db:"is_global"` Enabled bool `json:"enabled" db:"enabled"` CreatedBy string `json:"created_by" db:"created_by"` - UserEnv []byte `json:"-" db:"-"` // per-user encrypted env (populated by LookupByBinary LEFT JOIN) + // AdapterName routes the binary to a CredentialAdapter at exec time. + // NULL/empty → passthrough adapter (legacy env-vars injection only). + // Non-empty (e.g. "git") → typed adapter resolved via tools.LookupAdapter. + AdapterName *string `json:"adapter_name,omitempty" db:"adapter_name"` + UserEnv []byte `json:"-" db:"-"` // per-user encrypted env (populated by LookupByBinary LEFT JOIN) + // UserCredentialType + UserHostScope mirror the joined user-credential row + // (populated by LookupByBinary). NULL when the user has no credential or the + // credential is legacy env-only. + UserCredentialType *string `json:"-" db:"-"` + UserHostScope *string `json:"-" db:"-"` // EnvKeys is set by HTTP handlers only (names from decrypted env, no values); not a DB column. EnvKeys []string `json:"env_keys,omitempty" db:"-"` // Env is set by HTTP handlers only. Sensitive values are masked; value entries are visible. @@ -77,6 +86,12 @@ type SecureCLIUserCredential struct { UpdatedAt string `json:"updated_at" db:"updated_at"` // EncryptedEnv is decrypted JSON — never serialized to API. EncryptedEnv []byte `json:"-" db:"encrypted_env"` + // CredentialType selects the wire shape carried in EncryptedEnv. + // NULL/empty → legacy env-vars map. Future: 'pat', 'ssh_key', 'pg_password_file'. + CredentialType *string `json:"credential_type,omitempty" db:"credential_type"` + // HostScope binds the credential to a specific hostname (e.g. 'github.com'). + // Required when CredentialType ∈ {'pat','ssh_key'}; NULL for legacy env creds. + HostScope *string `json:"host_scope,omitempty" db:"host_scope"` } // SecureCLIAgentGrant represents a per-agent grant with optional setting overrides. @@ -135,6 +150,10 @@ type SecureCLIStore interface { GetUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string) (*SecureCLIUserCredential, error) SetUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte) error + // SetUserCredentialsTyped writes a typed user credential (PAT, SSH key, etc.). + // credentialType and hostScope are nil for legacy env-vars credentials — + // in that case behavior is identical to SetUserCredentials. + SetUserCredentialsTyped(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte, credentialType, hostScope *string) error DeleteUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string) error ListUserCredentials(ctx context.Context, binaryID uuid.UUID) ([]SecureCLIUserCredential, error) } diff --git a/internal/store/sqlitestore/schema.go b/internal/store/sqlitestore/schema.go index 767bfce6..a834844b 100644 --- a/internal/store/sqlitestore/schema.go +++ b/internal/store/sqlitestore/schema.go @@ -16,7 +16,7 @@ var schemaSQL string // SchemaVersion is the current SQLite schema version. // Bump this when adding new migration steps below. -const SchemaVersion = 39 +const SchemaVersion = 42 // migrations maps version → SQL to apply when upgrading FROM that version. // schema.sql always represents the LATEST full schema (for fresh DBs). @@ -765,6 +765,13 @@ CREATE INDEX IF NOT EXISTS idx_browser_cookies_scope_domain ON browser_cookies (tenant_id, user_id, agent_id, domain); CREATE INDEX IF NOT EXISTS idx_browser_cookies_expires_at ON browser_cookies (expires_at);`, + + // Version 39 → 40: credential adapter framework — credential_type on user creds. + 39: `ALTER TABLE secure_cli_user_credentials ADD COLUMN credential_type TEXT;`, + // Version 40 → 41: credential adapter framework — host_scope on user creds. + 40: `ALTER TABLE secure_cli_user_credentials ADD COLUMN host_scope TEXT;`, + // Version 41 → 42: credential adapter framework — adapter_name on binaries. + 41: `ALTER TABLE secure_cli_binaries ADD COLUMN adapter_name TEXT;`, } // addHooksTables is the SQLite incremental migration for schema v19 → v20. @@ -1043,6 +1050,12 @@ func idempotentColumnMigration(version int) (string, string, bool) { return "agents", "model_fallback", true case 34: return "skill_agent_grants", "can_manage", true + case 39: + return "secure_cli_user_credentials", "credential_type", true + case 40: + return "secure_cli_user_credentials", "host_scope", true + case 41: + return "secure_cli_binaries", "adapter_name", true default: return "", "", false } diff --git a/internal/store/sqlitestore/schema.sql b/internal/store/sqlitestore/schema.sql index 9b9f5228..7b837760 100644 --- a/internal/store/sqlitestore/schema.sql +++ b/internal/store/sqlitestore/schema.sql @@ -1208,6 +1208,7 @@ CREATE TABLE IF NOT EXISTS secure_cli_binaries ( enabled BOOLEAN NOT NULL DEFAULT 1, created_by TEXT NOT NULL DEFAULT '', tenant_id TEXT NOT NULL REFERENCES tenants(id), + adapter_name TEXT, created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) ); @@ -1501,14 +1502,16 @@ CREATE INDEX IF NOT EXISTS idx_kg_dedup_agent ON kg_dedup_candidates(agent_id, s -- ============================================================ CREATE TABLE IF NOT EXISTS secure_cli_user_credentials ( - id TEXT NOT NULL PRIMARY KEY, - binary_id TEXT NOT NULL REFERENCES secure_cli_binaries(id) ON DELETE CASCADE, - user_id VARCHAR(255) NOT NULL, - encrypted_env BLOB NOT NULL, - metadata TEXT NOT NULL DEFAULT '{}', - tenant_id TEXT NOT NULL REFERENCES tenants(id), - created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), + id TEXT NOT NULL PRIMARY KEY, + binary_id TEXT NOT NULL REFERENCES secure_cli_binaries(id) ON DELETE CASCADE, + user_id VARCHAR(255) NOT NULL, + encrypted_env BLOB NOT NULL, + metadata TEXT NOT NULL DEFAULT '{}', + tenant_id TEXT NOT NULL REFERENCES tenants(id), + credential_type TEXT, + host_scope TEXT, + created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), + updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), UNIQUE(binary_id, user_id, tenant_id) ); diff --git a/internal/store/sqlitestore/secure-cli-user-credentials.go b/internal/store/sqlitestore/secure-cli-user-credentials.go index 8b4a9baf..9875c670 100644 --- a/internal/store/sqlitestore/secure-cli-user-credentials.go +++ b/internal/store/sqlitestore/secure-cli-user-credentials.go @@ -15,6 +15,11 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/store" ) +// userCredSelectCols projects every column callers read off SecureCLIUserCredential. +// Keep in lockstep with the struct in internal/store/secure_cli_store.go. +const userCredSelectCols = `id, binary_id, user_id, encrypted_env, COALESCE(metadata, '{}'), + credential_type, host_scope, created_at, updated_at` + // GetUserCredentials returns per-user credential overrides for a CLI binary. // Returns (nil, nil) if no per-user credentials exist. func (s *SQLiteSecureCLIStore) GetUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string) (*store.SecureCLIUserCredential, error) { @@ -25,14 +30,16 @@ func (s *SQLiteSecureCLIStore) GetUserCredentials(ctx context.Context, binaryID var uc store.SecureCLIUserCredential var env []byte + var metaBytes []byte var createdAt, updatedAt string err := s.db.QueryRowContext(ctx, - `SELECT id, binary_id, user_id, encrypted_env, COALESCE(metadata, '{}'), created_at, updated_at + `SELECT `+userCredSelectCols+` FROM secure_cli_user_credentials WHERE binary_id = ? AND user_id = ? AND tenant_id = ?`, binaryID, userID, tid, - ).Scan(&uc.ID, &uc.BinaryID, &uc.UserID, &env, &uc.Metadata, &createdAt, &updatedAt) + ).Scan(&uc.ID, &uc.BinaryID, &uc.UserID, &env, &metaBytes, + &uc.CredentialType, &uc.HostScope, &createdAt, &updatedAt) if errors.Is(err, sql.ErrNoRows) { return nil, nil @@ -43,6 +50,9 @@ func (s *SQLiteSecureCLIStore) GetUserCredentials(ctx context.Context, binaryID uc.CreatedAt = createdAt uc.UpdatedAt = updatedAt + if len(metaBytes) > 0 { + uc.Metadata = metaBytes + } // Decrypt env if len(env) > 0 && s.encKey != "" { @@ -56,9 +66,15 @@ func (s *SQLiteSecureCLIStore) GetUserCredentials(ctx context.Context, binaryID return &uc, nil } -// SetUserCredentials creates or updates per-user encrypted env overrides (upsert). -// Encrypts the env bytes before storing. +// SetUserCredentials writes a legacy env-vars credential (credential_type / host_scope NULL). +// Preserves all pre-existing callers. func (s *SQLiteSecureCLIStore) SetUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte) error { + return s.SetUserCredentialsTyped(ctx, binaryID, userID, encryptedEnv, nil, nil) +} + +// SetUserCredentialsTyped is the typed-credential entry point. +// credentialType / hostScope are NULL for legacy env-only credentials. +func (s *SQLiteSecureCLIStore) SetUserCredentialsTyped(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte, credentialType, hostScope *string) error { tid := store.TenantIDFromContext(ctx) if tid == uuid.Nil { tid = store.MasterTenantID @@ -79,12 +95,16 @@ func (s *SQLiteSecureCLIStore) SetUserCredentials(ctx context.Context, binaryID id := store.GenNewID() _, err := s.db.ExecContext(ctx, - `INSERT INTO secure_cli_user_credentials (id, binary_id, user_id, encrypted_env, metadata, tenant_id, created_at, updated_at) - VALUES (?, ?, ?, ?, '{}', ?, ?, ?) + `INSERT INTO secure_cli_user_credentials + (id, binary_id, user_id, encrypted_env, metadata, tenant_id, + credential_type, host_scope, created_at, updated_at) + VALUES (?, ?, ?, ?, '{}', ?, ?, ?, ?, ?) ON CONFLICT (binary_id, user_id, tenant_id) DO UPDATE SET - encrypted_env = excluded.encrypted_env, - updated_at = excluded.updated_at`, - id, binaryID, userID, envBytes, tid, now, now, + encrypted_env = excluded.encrypted_env, + credential_type = excluded.credential_type, + host_scope = excluded.host_scope, + updated_at = excluded.updated_at`, + id, binaryID, userID, envBytes, tid, credentialType, hostScope, now, now, ) return err } @@ -110,7 +130,7 @@ func (s *SQLiteSecureCLIStore) ListUserCredentials(ctx context.Context, binaryID } rows, err := s.db.QueryContext(ctx, - `SELECT id, binary_id, user_id, encrypted_env, COALESCE(metadata, '{}'), created_at, updated_at + `SELECT `+userCredSelectCols+` FROM secure_cli_user_credentials WHERE binary_id = ? AND tenant_id = ? ORDER BY created_at`, binaryID, tid) @@ -123,14 +143,19 @@ func (s *SQLiteSecureCLIStore) ListUserCredentials(ctx context.Context, binaryID for rows.Next() { var uc store.SecureCLIUserCredential var env []byte + var metaBytes []byte var createdAt, updatedAt string - if err := rows.Scan(&uc.ID, &uc.BinaryID, &uc.UserID, &env, &uc.Metadata, &createdAt, &updatedAt); err != nil { + if err := rows.Scan(&uc.ID, &uc.BinaryID, &uc.UserID, &env, &metaBytes, + &uc.CredentialType, &uc.HostScope, &createdAt, &updatedAt); err != nil { return nil, err } uc.CreatedAt = createdAt uc.UpdatedAt = updatedAt + if len(metaBytes) > 0 { + uc.Metadata = metaBytes + } if len(env) > 0 && s.encKey != "" { if decrypted, err := crypto.Decrypt(string(env), s.encKey); err == nil { diff --git a/internal/store/sqlitestore/secure-cli.go b/internal/store/sqlitestore/secure-cli.go index e7285a9b..49084010 100644 --- a/internal/store/sqlitestore/secure-cli.go +++ b/internal/store/sqlitestore/secure-cli.go @@ -31,10 +31,12 @@ func NewSQLiteSecureCLIStore(db *sql.DB, encKey string) *SQLiteSecureCLIStore { } const secureCLISelectCols = `id, binary_name, binary_path, description, encrypted_env, - deny_args, deny_verbose, timeout_seconds, tips, is_global, enabled, created_by, created_at, updated_at` + deny_args, deny_verbose, timeout_seconds, tips, is_global, enabled, created_by, + adapter_name, created_at, updated_at` const secureCLISelectColsAliased = `b.id, b.binary_name, b.binary_path, b.description, b.encrypted_env, - b.deny_args, b.deny_verbose, b.timeout_seconds, b.tips, b.is_global, b.enabled, b.created_by, b.created_at, b.updated_at` + b.deny_args, b.deny_verbose, b.timeout_seconds, b.tips, b.is_global, b.enabled, b.created_by, + b.adapter_name, b.created_at, b.updated_at` func (s *SQLiteSecureCLIStore) Create(ctx context.Context, b *store.SecureCLIBinary) error { if err := store.ValidateUserID(b.CreatedBy); err != nil { @@ -71,14 +73,14 @@ func (s *SQLiteSecureCLIStore) Create(ctx context.Context, b *store.SecureCLIBin _, err := s.db.ExecContext(ctx, `INSERT INTO secure_cli_binaries (id, binary_name, binary_path, description, encrypted_env, - deny_args, deny_verbose, timeout_seconds, tips, is_global, enabled, created_by, created_at, updated_at, tenant_id) - VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`, + deny_args, deny_verbose, timeout_seconds, tips, is_global, enabled, created_by, adapter_name, created_at, updated_at, tenant_id) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`, b.ID, b.BinaryName, nilStr(derefStr(b.BinaryPath)), b.Description, envBytes, jsonOrEmptyArray(b.DenyArgs), jsonOrEmptyArray(b.DenyVerbose), b.TimeoutSeconds, b.Tips, b.IsGlobal, b.Enabled, - b.CreatedBy, nowStr, nowStr, tenantID, + b.CreatedBy, b.AdapterName, nowStr, nowStr, tenantID, ) return err } @@ -109,7 +111,7 @@ func (s *SQLiteSecureCLIStore) scanRow(row *sql.Row) (*store.SecureCLIBinary, er &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &createdAt, &updatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &createdAt, &updatedAt, ) if err != nil { return nil, err @@ -154,7 +156,7 @@ func (s *SQLiteSecureCLIStore) scanRows(rows *sql.Rows) ([]store.SecureCLIBinary &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &createdAt, &updatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &createdAt, &updatedAt, ); err != nil { return nil, fmt.Errorf("scan secure_cli_binaries row: %w", err) } @@ -186,7 +188,8 @@ var secureCLIAllowedFields = map[string]bool{ "binary_name": true, "binary_path": true, "description": true, "encrypted_env": true, "deny_args": true, "deny_verbose": true, "timeout_seconds": true, "tips": true, "is_global": true, "enabled": true, - "updated_at": true, + "adapter_name": true, + "updated_at": true, } func (s *SQLiteSecureCLIStore) Update(ctx context.Context, id uuid.UUID, updates map[string]any) error { @@ -303,7 +306,7 @@ func (s *SQLiteSecureCLIStore) scanRowsWithGrants(rows *sql.Rows) ([]store.Secur &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &createdAt, &updatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &createdAt, &updatedAt, &grantsJSON, ); err != nil { return nil, fmt.Errorf("scan secure_cli_binaries row: %w", err) @@ -380,18 +383,28 @@ func (s *SQLiteSecureCLIStore) LookupByBinary(ctx context.Context, binaryName st query := `SELECT ` + selectCols + // Project user-credential columns (encrypted_env + credential_type + host_scope). + // When userID is empty we cannot reference uc_user.* — emit NULL placeholders + // so the scan column count stays stable. + hasUserJoin := userID != "" && agentID != nil + hasUserJoinNoAgent := userID != "" && agentID == nil + + if hasUserJoin || hasUserJoinNoAgent { + query += `, uc_user.encrypted_env AS user_env, uc_user.credential_type AS user_cred_type, uc_user.host_scope AS user_host_scope FROM secure_cli_binaries b` + } else { + query += `, NULL AS user_env, NULL AS user_cred_type, NULL AS user_host_scope FROM secure_cli_binaries b` + } + // LEFT JOIN agent grant if agentID != nil { - query += `, uc_user.encrypted_env AS user_env FROM secure_cli_binaries b` query += ` LEFT JOIN secure_cli_agent_grants g ON g.binary_id = b.id AND g.agent_id = ?` args = append(args, *agentID) } else { - query += `, NULL AS user_env FROM secure_cli_binaries b` query += ` LEFT JOIN secure_cli_agent_grants g ON 0` } - // LEFT JOIN user credentials - if userID != "" { + // LEFT JOIN user credentials (only when we project uc_user.*) + if hasUserJoin || hasUserJoinNoAgent { if isCross { query += ` LEFT JOIN secure_cli_user_credentials uc_user ON uc_user.binary_id = b.id AND uc_user.user_id = ?` args = append(args, userID) @@ -399,16 +412,6 @@ func (s *SQLiteSecureCLIStore) LookupByBinary(ctx context.Context, binaryName st query += ` LEFT JOIN secure_cli_user_credentials uc_user ON uc_user.binary_id = b.id AND uc_user.user_id = ? AND uc_user.tenant_id = ?` args = append(args, userID, tid) } - } else { - // Rewrite: no user_env JOIN needed, replace alias reference - // Already handled by NULL above — but need to adjust query structure - // We need uc_user alias to not appear in FROM if no userID - // Simplest: LEFT JOIN on impossible condition - if agentID == nil { - // already have NULL AS user_env, skip join - } else { - query += ` LEFT JOIN secure_cli_user_credentials uc_user ON 0` - } } // WHERE @@ -449,15 +452,16 @@ func (s *SQLiteSecureCLIStore) scanRowWithGrantAndUserEnv(row *sql.Row) (*store. var grantID *uuid.UUID var grantEncEnv []byte var userEnv []byte + var userCredType, userHostScope *string var createdAt, updatedAt sqliteTime err := row.Scan( &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &createdAt, &updatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &createdAt, &updatedAt, &grantDenyArgs, &grantDenyVerbose, &grantTimeout, &grantTips, &grantEnabled, &grantID, &grantEncEnv, - &userEnv, + &userEnv, &userCredType, &userHostScope, ) if err != nil { if errors.Is(err, sql.ErrNoRows) { @@ -513,6 +517,10 @@ func (s *SQLiteSecureCLIStore) scanRowWithGrantAndUserEnv(row *sql.Row) (*store. } } + // Typed-credential metadata from the joined user-credential row. + b.UserCredentialType = userCredType + b.UserHostScope = userHostScope + return &b, nil } @@ -617,7 +625,7 @@ func (s *SQLiteSecureCLIStore) ListForAgent(ctx context.Context, agentID uuid.UU &b.ID, &b.BinaryName, &binaryPath, &b.Description, &env, &denyArgs, &denyVerbose, &b.TimeoutSeconds, &b.Tips, &b.IsGlobal, - &b.Enabled, &b.CreatedBy, &createdAt, &updatedAt, + &b.Enabled, &b.CreatedBy, &b.AdapterName, &createdAt, &updatedAt, &grantDenyArgs, &grantDenyVerbose, &grantTimeout, &grantTips, &grantID, &grantEncEnv, ); err != nil { return nil, fmt.Errorf("scan secure_cli_binaries row: %w", err) diff --git a/internal/store/sqlitestore/secure_cli_schema_test.go b/internal/store/sqlitestore/secure_cli_schema_test.go new file mode 100644 index 00000000..d989d0ec --- /dev/null +++ b/internal/store/sqlitestore/secure_cli_schema_test.go @@ -0,0 +1,270 @@ +//go:build sqlite || sqliteonly + +package sqlitestore + +import ( + "context" + "database/sql" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// Phase 1 schema delta tests: +// - adapter_name column on secure_cli_binaries +// - credential_type, host_scope columns on secure_cli_user_credentials +// - LookupByBinary projects all three onto the returned struct +// - Legacy callers (no adapter, no typed credential) still produce NULL columns +// +// These tests must stay green for any future schema migration touching the +// secure-cli surface to ensure backward compatibility. + +// newPhase1Store opens a fresh SQLite DB with an empty encryption key. +// Phase 1 round-trip tests verify column projection, not crypto behavior — the +// shared testEncKey constant is not 32 bytes so crypto.Encrypt rejects it. +func newPhase1Store(t *testing.T) (*SQLiteSecureCLIStore, *sql.DB) { + t.Helper() + _, db := newTestSQLiteSecureCLI(t) + return NewSQLiteSecureCLIStore(db, ""), db +} + +// columnExists checks PRAGMA table_info for a column on the given SQLite table. +func columnExists(t *testing.T, db *sql.DB, table, column string) bool { + t.Helper() + rows, err := db.Query(`PRAGMA table_info(` + table + `)`) + if err != nil { + t.Fatalf("PRAGMA table_info(%s): %v", table, err) + } + defer rows.Close() + for rows.Next() { + var cid int + var name, ctype string + var notnull, pk int + var dflt sql.NullString + if err := rows.Scan(&cid, &name, &ctype, ¬null, &dflt, &pk); err != nil { + t.Fatalf("scan table_info row: %v", err) + } + if strings.EqualFold(name, column) { + return true + } + } + return false +} + +func TestSQLite_SchemaHasPhase1Columns(t *testing.T) { + _, db := newTestSQLiteSecureCLI(t) + + if !columnExists(t, db, "secure_cli_binaries", "adapter_name") { + t.Fatalf("secure_cli_binaries.adapter_name missing — migration not applied") + } + if !columnExists(t, db, "secure_cli_user_credentials", "credential_type") { + t.Fatalf("secure_cli_user_credentials.credential_type missing") + } + if !columnExists(t, db, "secure_cli_user_credentials", "host_scope") { + t.Fatalf("secure_cli_user_credentials.host_scope missing") + } +} + +func TestSQLite_CreateBinary_AdapterNameRoundTrip(t *testing.T) { + s, db := newPhase1Store(t) + tid := seedTenant(t, db, "t-adapter") + ctx := store.WithTenantID(context.Background(), tid) + + adapter := "git" + bin := &store.SecureCLIBinary{ + BinaryName: "git", + Description: "git with PAT adapter", + IsGlobal: true, + Enabled: true, + CreatedBy: "u-tester", + AdapterName: &adapter, + EncryptedEnv: []byte(`{}`), + } + if err := s.Create(ctx, bin); err != nil { + t.Fatalf("Create: %v", err) + } + + got, err := s.Get(ctx, bin.ID) + if err != nil { + t.Fatalf("Get: %v", err) + } + if got.AdapterName == nil || *got.AdapterName != "git" { + t.Fatalf("expected adapter_name=git, got %v", got.AdapterName) + } +} + +func TestSQLite_CreateBinary_AdapterNameNilStaysNull(t *testing.T) { + s, db := newPhase1Store(t) + tid := seedTenant(t, db, "t-no-adapter") + ctx := store.WithTenantID(context.Background(), tid) + + bin := &store.SecureCLIBinary{ + BinaryName: "gh", + Description: "legacy env-only binary", + IsGlobal: true, + Enabled: true, + CreatedBy: "u-tester", + EncryptedEnv: []byte(`{}`), + } + if err := s.Create(ctx, bin); err != nil { + t.Fatalf("Create: %v", err) + } + + got, err := s.Get(ctx, bin.ID) + if err != nil { + t.Fatalf("Get: %v", err) + } + if got.AdapterName != nil { + t.Fatalf("expected adapter_name NULL, got %q", *got.AdapterName) + } +} + +func TestSQLite_SetUserCredentialsTyped_RoundTrip(t *testing.T) { + s, db := newPhase1Store(t) + tid := seedTenant(t, db, "t-typed-cred") + ctx := store.WithTenantID(context.Background(), tid) + + binID := uuid.New() + seedBinary(t, db, tid, "git", true, true) + // Re-fetch the just-seeded binary's ID since seedBinary doesn't return it. + if err := db.QueryRow( + `SELECT id FROM secure_cli_binaries WHERE binary_name = ? AND tenant_id = ?`, + "git", tid, + ).Scan(&binID); err != nil { + t.Fatalf("lookup seeded binary: %v", err) + } + + credType := "pat" + hostScope := "github.com" + plaintext := []byte(`{"token":"ghp_xxx"}`) + + if err := s.SetUserCredentialsTyped(ctx, binID, "u-1", plaintext, &credType, &hostScope); err != nil { + t.Fatalf("SetUserCredentialsTyped: %v", err) + } + + got, err := s.GetUserCredentials(ctx, binID, "u-1") + if err != nil { + t.Fatalf("GetUserCredentials: %v", err) + } + if got == nil { + t.Fatalf("expected credential, got nil") + } + if got.CredentialType == nil || *got.CredentialType != "pat" { + t.Fatalf("expected credential_type=pat, got %v", got.CredentialType) + } + if got.HostScope == nil || *got.HostScope != "github.com" { + t.Fatalf("expected host_scope=github.com, got %v", got.HostScope) + } + if string(got.EncryptedEnv) != string(plaintext) { + t.Fatalf("decrypted env mismatch: got %q want %q", got.EncryptedEnv, plaintext) + } +} + +// SetUserCredentials (legacy entrypoint) must leave credential_type / host_scope NULL. +func TestSQLite_SetUserCredentials_LegacyLeavesTypeColumnsNull(t *testing.T) { + s, db := newPhase1Store(t) + tid := seedTenant(t, db, "t-legacy-cred") + ctx := store.WithTenantID(context.Background(), tid) + + seedBinary(t, db, tid, "git", true, true) + var binID uuid.UUID + if err := db.QueryRow( + `SELECT id FROM secure_cli_binaries WHERE binary_name = ? AND tenant_id = ?`, + "git", tid, + ).Scan(&binID); err != nil { + t.Fatalf("lookup seeded binary: %v", err) + } + + if err := s.SetUserCredentials(ctx, binID, "u-legacy", []byte(`{"GITHUB_TOKEN":"x"}`)); err != nil { + t.Fatalf("SetUserCredentials: %v", err) + } + + got, err := s.GetUserCredentials(ctx, binID, "u-legacy") + if err != nil { + t.Fatalf("GetUserCredentials: %v", err) + } + if got == nil { + t.Fatalf("expected credential, got nil") + } + if got.CredentialType != nil { + t.Fatalf("expected credential_type NULL for legacy, got %q", *got.CredentialType) + } + if got.HostScope != nil { + t.Fatalf("expected host_scope NULL for legacy, got %q", *got.HostScope) + } +} + +// LookupByBinary must populate AdapterName (from binary row) AND +// UserCredentialType + UserHostScope (from joined user-credential row). +func TestSQLite_LookupByBinary_ProjectsNewColumns(t *testing.T) { + s, db := newPhase1Store(t) + tid := seedTenant(t, db, "t-lookup-proj") + ctx := store.WithTenantID(context.Background(), tid) + + adapter := "git" + bin := &store.SecureCLIBinary{ + BinaryName: "git", + Description: "git adapter binary", + IsGlobal: true, + Enabled: true, + CreatedBy: "u-tester", + AdapterName: &adapter, + EncryptedEnv: []byte(`{}`), + } + if err := s.Create(ctx, bin); err != nil { + t.Fatalf("Create: %v", err) + } + + credType := "pat" + hostScope := "github.com" + if err := s.SetUserCredentialsTyped(ctx, bin.ID, "u-1", []byte(`{"token":"x"}`), &credType, &hostScope); err != nil { + t.Fatalf("SetUserCredentialsTyped: %v", err) + } + + got, err := s.LookupByBinary(ctx, "git", nil, "u-1") + if err != nil { + t.Fatalf("LookupByBinary: %v", err) + } + if got == nil { + t.Fatalf("expected binary, got nil") + } + if got.AdapterName == nil || *got.AdapterName != "git" { + t.Fatalf("expected AdapterName=git, got %v", got.AdapterName) + } + if got.UserCredentialType == nil || *got.UserCredentialType != "pat" { + t.Fatalf("expected UserCredentialType=pat, got %v", got.UserCredentialType) + } + if got.UserHostScope == nil || *got.UserHostScope != "github.com" { + t.Fatalf("expected UserHostScope=github.com, got %v", got.UserHostScope) + } +} + +// Backward-compat: a binary with no adapter + no user credential must still +// return successfully from LookupByBinary with all three new fields NULL. +func TestSQLite_LookupByBinary_BackwardCompatibleNulls(t *testing.T) { + s, db := newPhase1Store(t) + tid := seedTenant(t, db, "t-bc-null") + ctx := store.WithTenantID(context.Background(), tid) + + seedBinary(t, db, tid, "gh", true, true) + + got, err := s.LookupByBinary(ctx, "gh", nil, "") + if err != nil { + t.Fatalf("LookupByBinary: %v", err) + } + if got == nil { + t.Fatalf("expected binary, got nil") + } + if got.AdapterName != nil { + t.Fatalf("expected AdapterName NULL, got %q", *got.AdapterName) + } + if got.UserCredentialType != nil { + t.Fatalf("expected UserCredentialType NULL, got %q", *got.UserCredentialType) + } + if got.UserHostScope != nil { + t.Fatalf("expected UserHostScope NULL, got %q", *got.UserHostScope) + } +} diff --git a/internal/tools/credential_adapter.go b/internal/tools/credential_adapter.go new file mode 100644 index 00000000..a5d9112e --- /dev/null +++ b/internal/tools/credential_adapter.go @@ -0,0 +1,138 @@ +package tools + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "log/slog" + "sort" + "sync" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// CredentialAdapter transforms a user credential into the shape a specific CLI +// binary expects. Each binary's adapter_name column routes runtime exec to one +// of these. The default "passthrough" adapter is a no-op so unrelated presets +// (gh, aws, kubectl, gcloud, terraform, gws) keep their legacy env-injection +// path bit-for-bit. +// +// Adapters never see or log raw secret bytes — `Prepare` returns an Injection +// describing how to mutate argv/env, plus a Cleanup hook for ephemeral files +// and a ScrubValues slice that the caller registers with the per-request +// scrubber (see scrub.go WithScrubBag). +type CredentialAdapter interface { + Name() string + ShouldInject(argv []string) bool + Prepare(ctx context.Context, bin *store.SecureCLIBinary, cred *store.SecureCLIUserCredential, argv []string) (*Injection, error) +} + +// Injection is the adapter's instruction set for one exec call. +// +// - ArgvPrefix is spliced in BETWEEN binary and user-supplied args (so +// `git clone X` becomes `git clone X`). The slice is not +// re-quoted or shell-parsed — it goes straight into exec.Command. +// - Env is merged on top of the base env after legacy ValidateGrantEnvVars +// has already vetted the user-controlled credential blob. Keys here come +// from hard-coded adapter code only. +// - Cleanup is deferred AFTER the synchronous exec returns; safe to be nil. +// - ScrubValues lists adapter-derived secrets the per-request scrubber must +// redact from stdout/stderr/error messages before they reach the LLM. +type Injection struct { + ArgvPrefix []string + Env map[string]string + Cleanup func() error + ScrubValues []string +} + +var ( + adaptersMu sync.RWMutex + adapters = map[string]CredentialAdapter{ + "passthrough": passthroughAdapter{}, + } +) + +// RegisterAdapter registers a CredentialAdapter under its Name(). Idempotent — +// re-registering the same name overwrites. Intended for init() of each +// adapter file (e.g. credential_adapter_git.go in Phase 3). +func RegisterAdapter(a CredentialAdapter) { + if a == nil { + return + } + adaptersMu.Lock() + defer adaptersMu.Unlock() + adapters[a.Name()] = a +} + +// AdapterFor resolves an adapter by name, falling back to passthrough on +// empty/unknown so an operator-set adapter_name typo can never break exec — +// it just degrades to legacy behavior with a clear audit trail. +func AdapterFor(name string) CredentialAdapter { + adaptersMu.RLock() + defer adaptersMu.RUnlock() + if a, ok := adapters[name]; ok { + return a + } + return adapters["passthrough"] +} + +// passthroughAdapter is the default. It does nothing — preserves the legacy +// env-injection-only path so every existing preset behaves identically. +type passthroughAdapter struct{} + +func (passthroughAdapter) Name() string { return "passthrough" } +func (passthroughAdapter) ShouldInject([]string) bool { return false } +func (passthroughAdapter) Prepare(context.Context, *store.SecureCLIBinary, *store.SecureCLIUserCredential, []string) (*Injection, error) { + return &Injection{}, nil +} + +// hashHostScope returns the SHA-256 hex prefix of the host-scope value, or +// "none" when nil. Used in audit logs instead of plaintext hostname so SIEM +// pipelines can correlate without exposing internal infra hostnames. +func hashHostScope(hs *string) string { + if hs == nil || *hs == "" { + return "none" + } + sum := sha256.Sum256([]byte(*hs)) + return hex.EncodeToString(sum[:4]) // 8 hex chars +} + +// sortedKeys returns env map keys in deterministic order for audit logs. +func sortedKeys(m map[string]string) []string { + if len(m) == 0 { + return nil + } + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// emitSystemEnvInjectionAudit is the single source of truth for the +// `security.system_env_injection` slog line. Operators grep for this event +// name in their log aggregator (see docs/09-security.md). The field schema is +// pinned by `TestEmitSystemEnvInjectionAudit_FieldSchema` — any change here +// must update both that test and the operator-facing docs. +// +// Fields: +// - adapter: adapter name (e.g. "git", "passthrough") +// - binary: binary name (e.g. "git", "gh") +// - user_id: tenant user UUID (or empty for global-only contexts) +// - env_keys: sorted env-var NAMES (never values) +// - argv_prefix_len: number of argv elements prepended (NOT their content) +// - host_scope_hash: SHA-256 first 8 hex chars of host_scope (or "none") +func emitSystemEnvInjectionAudit(adapter, binary, userID string, inj *Injection, hostScope *string) { + if inj == nil { + return + } + slog.Warn("security.system_env_injection", + "adapter", adapter, + "user_id", userID, + "binary", binary, + "env_keys", sortedKeys(inj.Env), + "argv_prefix_len", len(inj.ArgvPrefix), + "host_scope_hash", hashHostScope(hostScope), + ) +} diff --git a/internal/tools/credential_adapter_git.go b/internal/tools/credential_adapter_git.go new file mode 100644 index 00000000..5e4a9fc6 --- /dev/null +++ b/internal/tools/credential_adapter_git.go @@ -0,0 +1,445 @@ +package tools + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/url" + "os" + "os/exec" + "strings" + + "golang.org/x/net/idna" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// gitAdapter implements the CredentialAdapter contract for the `git` binary. +// +// PAT path (this file): host-scoped HTTP extraheader injected via +// GIT_CONFIG_COUNT/GIT_CONFIG_KEY_0/GIT_CONFIG_VALUE_0 env vars (git 2.31+). +// Keeps the token out of /proc//cmdline and `ps` output — the typical +// `-c http.extraHeader=...` form would land the bearer token directly in argv +// where any process listing leaks it. +// +// SSH path: Phase 4 extends the same Prepare switch. +type gitAdapter struct{} + +func (gitAdapter) Name() string { return "git" } + +// ShouldInject returns true only for subcommands that talk to a remote. +// Status/log/diff/commit do not need credentials, and asking the adapter for +// them would waste a `git config --get` sub-exec and emit noisy audit events. +func (gitAdapter) ShouldInject(argv []string) bool { + if len(argv) == 0 { + return false + } + switch argv[0] { + case "clone", "fetch", "pull", "push", "submodule": + return true + } + return false +} + +var ( + errEmptyToken = errors.New("git adapter: token is empty") + errTokenTooLong = errors.New("git adapter: token exceeds 4 KiB") + errTokenControlChar = errors.New("git adapter: token contains control character (CR/LF/NUL)") + errEmptyHost = errors.New("git adapter: host is empty") + errEmbeddedUserinfo = errors.New("git adapter: URL has embedded userinfo (ambiguous; rejected)") + errURLMissingHost = errors.New("git adapter: URL has no host") + errURLUnsupportedForm = errors.New("git adapter: URL form not recognized") + errCloneURLMissing = errors.New("git adapter: clone command has no URL argument") + errUnknownSubcommand = errors.New("git adapter: unknown subcommand") + errCredentialTypeBlank = errors.New("git adapter: credential_type required for git adapter routing") +) + +// errCredentialHostMismatch is returned when the matched credential's host +// scope does not equal the target host extracted from argv (or remote URL). +// Carries both hosts in fields so callers can format an i18n message without +// re-parsing. Error.Error() intentionally omits any token-shaped data. +type errCredentialHostMismatch struct { + credHost string + targetHost string +} + +func (e *errCredentialHostMismatch) Error() string { + return fmt.Sprintf("git adapter: credential host %q does not match target host %q", e.credHost, e.targetHost) +} + +func (gitAdapter) Prepare(ctx context.Context, _ *store.SecureCLIBinary, cred *store.SecureCLIUserCredential, argv []string) (*Injection, error) { + if cred == nil { + return &Injection{}, nil + } + typ := "" + if cred.CredentialType != nil { + typ = *cred.CredentialType + } + // Legacy env-vars rows: behave exactly like passthrough so existing operators + // who wired `git` via env-paste before the adapter existed keep working. + if typ == "" || typ == "env" { + return &Injection{}, nil + } + + // cwd from ctx so `git config --get remote.X.url` runs inside the repo + // the caller is about to operate on. Empty when caller did not set it + // (e.g. clone, where the URL is in argv and cwd is irrelevant). + host, err := resolveTargetHost(ctx, argv, ExecCwdFromContext(ctx)) + if err != nil { + return nil, fmt.Errorf("resolve target host: %w", err) + } + credHostRaw := "" + if cred.HostScope != nil { + credHostRaw = *cred.HostScope + } + credHost, err := normalizeHost(credHostRaw) + if err != nil { + return nil, fmt.Errorf("normalize credential host: %w", err) + } + targetHost, err := normalizeHost(host) + if err != nil { + return nil, fmt.Errorf("normalize target host: %w", err) + } + if credHost != targetHost { + return nil, &errCredentialHostMismatch{credHost: credHost, targetHost: targetHost} + } + + switch typ { + case "pat": + token, err := decodePATToken(cred.EncryptedEnv) + if err != nil { + return nil, err + } + if err := validateTokenShape(token); err != nil { + return nil, err + } + // GIT_CONFIG_COUNT env approach (git 2.31+) — same effect as + // `-c http.https://host/.extraheader=Authorization: Bearer ` but + // the token stays out of argv. Single-entry header keeps host-scoping + // strict; the matching url prefix ensures git skips its credential + // helpers for this URL automatically. + configKey := fmt.Sprintf("http.https://%s/.extraheader", targetHost) + configVal := fmt.Sprintf("Authorization: Bearer %s", token) + return &Injection{ + Env: map[string]string{ + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": configKey, + "GIT_CONFIG_VALUE_0": configVal, + }, + ScrubValues: []string{token}, + }, nil + case "ssh_key": + keyPEM, err := decodeSSHKeyBlob(cred.EncryptedEnv) + if err != nil { + return nil, err + } + // Defense-in-depth: ValidateSSHKey is called at save; running it again + // here catches keys that bypassed validation (e.g. raw DB write or a + // future store mutation that skips the handler path). + if err := ValidateSSHKey(keyPEM); err != nil { + return nil, err + } + keyPath, cleanup, err := materializeEphemeral(ctx, keyPEM, "gitkey") + if err != nil { + return nil, err + } + // -o flags pinned for safety: + // IdentitiesOnly=yes — ssh must not try other keys in ~/.ssh + // BatchMode=yes — never prompt; fail fast in agent context + // StrictHostKeyChecking=accept-new — first contact TOFU; known hosts + // still enforced after + // UserKnownHostsFile=~/.ssh/known_hosts is left at default so + // operators can pre-seed pinned host keys (documented in Phase 6). + sshCmd := fmt.Sprintf( + "ssh -i %s -o IdentitiesOnly=yes -o BatchMode=yes -o StrictHostKeyChecking=accept-new", + keyPath, + ) + return &Injection{ + Env: map[string]string{"GIT_SSH_COMMAND": sshCmd}, + Cleanup: cleanup, + ScrubValues: []string{keyPath}, + }, nil + default: + return nil, fmt.Errorf("git adapter: unsupported credential_type %q", typ) + } +} + +// decodeSSHKeyBlob extracts the private-key PEM from the credential blob. +// v1 wire shape: `{"key":"-----BEGIN OPENSSH PRIVATE KEY-----\n..."}`. +// No legacy fallback — SSH support is new in Phase 4, so any existing +// row with credential_type='ssh_key' was written by the new wire. +func decodeSSHKeyBlob(blob []byte) ([]byte, error) { + if len(blob) == 0 { + return nil, errors.New("git adapter: empty ssh key blob") + } + var m map[string]string + if err := json.Unmarshal(blob, &m); err != nil { + return nil, fmt.Errorf("decode ssh key blob: %w", err) + } + if k, ok := m["key"]; ok && k != "" { + return []byte(k), nil + } + return nil, errors.New("git adapter: ssh key blob missing 'key' field") +} + +// decodePATToken extracts the token from the credential blob. v1 wire shape +// is `{"token": "..."}`. Falls back to legacy env-style `{"GIT_TOKEN": "..."}` +// only when the typed key is missing, so operators migrating from env-paste +// rows do not need to re-enter their PAT. +func decodePATToken(blob []byte) (string, error) { + if len(blob) == 0 { + return "", errEmptyToken + } + var m map[string]string + if err := json.Unmarshal(blob, &m); err != nil { + return "", fmt.Errorf("decode pat blob: %w", err) + } + if tok, ok := m["token"]; ok { + return tok, nil + } + if tok, ok := m["GIT_TOKEN"]; ok { + return tok, nil + } + return "", errEmptyToken +} + +// validateTokenShape rejects empty, oversized, and control-char tokens. The +// CR/LF check defends against header injection if the UI validator ever +// regresses — a token like "ghp_x\r\nX-Evil: y" would otherwise smuggle a +// second HTTP header through GIT_CONFIG_VALUE_0. +func validateTokenShape(tok string) error { + if tok == "" { + return errEmptyToken + } + if len(tok) > 4096 { + return errTokenTooLong + } + for _, r := range tok { + if r < 0x20 || r == 0x7f { + return errTokenControlChar + } + } + return nil +} + +// normalizeHost canonicalizes a hostname for apples-to-apples comparison: +// trim whitespace + trailing dot, lowercase, idna.Lookup.ToASCII for IDN/ +// punycode equivalence. Preserves explicit ports (`:8443`) verbatim. +func normalizeHost(h string) (string, error) { + h = strings.TrimSuffix(strings.ToLower(strings.TrimSpace(h)), ".") + if h == "" { + return "", errEmptyHost + } + hostPart, port, hasPort := splitHostPortOptional(h) + ascii, err := idna.Lookup.ToASCII(hostPart) + if err != nil { + return "", fmt.Errorf("idna: %w", err) + } + if hasPort { + return ascii + ":" + port, nil + } + return ascii, nil +} + +// splitHostPortOptional splits "host:port" when port is all digits; otherwise +// returns host unchanged. Avoids net.SplitHostPort because that errors on +// portless inputs and on IPv6 forms without brackets, which we don't accept. +func splitHostPortOptional(h string) (host, port string, ok bool) { + idx := strings.LastIndex(h, ":") + if idx < 0 { + return h, "", false + } + port = h[idx+1:] + if port == "" { + return h, "", false + } + for _, r := range port { + if r < '0' || r > '9' { + return h, "", false + } + } + return h[:idx], port, true +} + +// resolveTargetHost finds the host this git invocation will contact. For +// clone, the URL is in argv. For fetch/pull/push/submodule, we sub-exec a +// hardened `git config --get remote..url` to read it from .git/config. +// +// cwd is the working directory for the sub-exec. Empty means current process +// dir (used by adapter Prepare in production where the exec hasn't chdir'd +// yet — git -C . is the implicit default). +func resolveTargetHost(ctx context.Context, argv []string, cwd string) (string, error) { + if len(argv) == 0 { + return "", errUnknownSubcommand + } + switch argv[0] { + case "clone": + u, err := firstNonFlagArg(argv[1:]) + if err != nil { + return "", err + } + return parseHostFromGitURL(u) + case "fetch", "pull": + remote := pickRemoteFromArgv(argv[1:], "origin") + return remoteURLViaConfigGet(ctx, cwd, remote) + case "push": + remote := pickPushRemoteFromArgv(argv[1:], "origin") + return remoteURLViaConfigGet(ctx, cwd, remote) + case "submodule": + // `git submodule update` uses the parent repo's origin URL as the + // reachability base for relative submodule paths, and the submodule + // fetch reuses the parent's auth header when the host matches. + return remoteURLViaConfigGet(ctx, cwd, "origin") + } + return "", errUnknownSubcommand +} + +// firstNonFlagArg returns the first argv element that does not start with `-`. +// Used to find the clone URL past option flags like --depth, --branch, etc. +func firstNonFlagArg(args []string) (string, error) { + skipValueFor := map[string]bool{ + // Long options that take a separate value as the next arg. + "--branch": true, "--depth": true, "--origin": true, "--config": true, + "--reference": true, "--reference-if-able": true, "--separate-git-dir": true, + "--template": true, "--upload-pack": true, "--jobs": true, "--shallow-since": true, + "--shallow-exclude": true, "--filter": true, "--server-option": true, + "-b": true, "-o": true, "-c": true, "-j": true, "-u": true, + } + skipNext := false + for _, a := range args { + if skipNext { + skipNext = false + continue + } + if strings.HasPrefix(a, "-") { + // `--depth=1` style — value embedded; no skip. + if strings.Contains(a, "=") { + continue + } + if skipValueFor[a] { + skipNext = true + } + continue + } + return a, nil + } + return "", errCloneURLMissing +} + +// pickRemoteFromArgv finds the explicit remote name in `git fetch/pull` argv, +// or returns `fallback` when none is given. Flags are skipped. +func pickRemoteFromArgv(args []string, fallback string) string { + for _, a := range args { + if strings.HasPrefix(a, "-") { + continue + } + return a + } + return fallback +} + +// pickPushRemoteFromArgv handles `git push`'s extra `--repo=` shape. +func pickPushRemoteFromArgv(args []string, fallback string) string { + for _, a := range args { + if strings.HasPrefix(a, "--repo=") { + return strings.TrimPrefix(a, "--repo=") + } + } + return pickRemoteFromArgv(args, fallback) +} + +// remoteURLViaConfigGet runs a hardened `git config --get remote..url` +// sub-exec to read the remote URL from .git/config without triggering any +// url-rewriting (insteadOf), credential helpers, or protocol-handler exec. +// +// We use `git config --get` instead of `git remote get-url` because the +// latter routes through git_handle_repo, which is the CVE-2018-17456 attack +// surface (malicious `[remote] url = ext::sh -c ` in .git/config would +// execute arbitrary shell). The CVE has been patched in modern git, but the +// defense-in-depth pattern keeps this immune to any future regression. +// +// GIT_ALLOW_PROTOCOL allowlists what git considers valid; GIT_TERMINAL_PROMPT=0 +// prevents interactive auth prompts; GIT_CONFIG_NOSYSTEM=1 ignores /etc/gitconfig +// so a host-wide rewrite cannot influence the lookup. +func remoteURLViaConfigGet(ctx context.Context, cwd, remote string) (string, error) { + args := []string{"-C", cwd, "config", "--get", "remote." + remote + ".url"} + if cwd == "" { + args = []string{"config", "--get", "remote." + remote + ".url"} + } + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Env = append(os.Environ(), + "GIT_ALLOW_PROTOCOL=https:http:ssh:git", + "GIT_TERMINAL_PROMPT=0", + "GIT_CONFIG_NOSYSTEM=1", + ) + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("read remote url for %q: %w", remote, err) + } + return parseHostFromGitURL(strings.TrimSpace(string(out))) +} + +// parseHostFromGitURL extracts the host[:port] from a git remote URL. +// Three forms are accepted: +// +// https://host[:port]/... (HTTPS — embedded userinfo REJECTED) +// git@host:owner/repo.git (scp-form SSH — bare user, no userinfo collision) +// ssh://git@host[:port]/... (full SSH — user MUST be "git" or absent) +// +// Embedded `https://user@host/...` is rejected as ambiguous: git treats `host` +// as the target but operators reading the URL often think `user` is the host. +// An attacker who controls `host` could trick a credential-host check into +// matching the wrong side. Refuse rather than guess. +func parseHostFromGitURL(raw string) (string, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return "", errURLMissingHost + } + + // scp-form: `user@host:path`. Match BEFORE generic URL parsing because + // `git@github.com:owner/repo` looks like `git@github.com:owner` host:port + // to url.Parse, mis-bucketing the path into the port. + if !strings.Contains(raw, "://") && strings.Contains(raw, ":") && strings.Contains(raw, "@") { + at := strings.Index(raw, "@") + colon := strings.Index(raw[at:], ":") + if colon > 0 { + host := raw[at+1 : at+colon] + if host == "" { + return "", errURLMissingHost + } + return host, nil + } + } + + u, err := url.Parse(raw) + if err != nil { + return "", fmt.Errorf("parse url: %w", err) + } + if u.Scheme == "" || u.Host == "" { + return "", errURLUnsupportedForm + } + // Reject embedded userinfo for HTTPS/HTTP. + // Exception: ssh://git@host is the conventional and unambiguous SSH form; + // we allow it only when user is empty or exactly "git" (no password). + if u.User != nil { + switch u.Scheme { + case "https", "http": + return "", errEmbeddedUserinfo + case "ssh": + if _, hasPass := u.User.Password(); hasPass { + return "", errEmbeddedUserinfo + } + if name := u.User.Username(); name != "" && name != "git" { + return "", errEmbeddedUserinfo + } + default: + return "", errEmbeddedUserinfo + } + } + return u.Host, nil +} + +func init() { + RegisterAdapter(gitAdapter{}) +} diff --git a/internal/tools/credential_adapter_git_keyvalidate.go b/internal/tools/credential_adapter_git_keyvalidate.go new file mode 100644 index 00000000..7a28652d --- /dev/null +++ b/internal/tools/credential_adapter_git_keyvalidate.go @@ -0,0 +1,50 @@ +package tools + +// SSH key validation for the git adapter (Phase 4, issue #82). +// +// Why this lives in tools and not store: the store layer is type-agnostic +// (it only writes encrypted bytes). The "what's a valid SSH key for this +// adapter?" rule belongs to the adapter itself. Callers (HTTP/WS save +// handlers) invoke ValidateSSHKey BEFORE encrypting and persisting. +// +// v1 scope: passphrase-protected keys are rejected. Reason: the runtime has +// nowhere to hold a passphrase for unattended `git fetch` and storing it +// alongside the key defeats the purpose. Phase 6 docs explain how operators +// can strip the passphrase via `ssh-keygen -p` before saving. + +import ( + "errors" + "fmt" + + "golang.org/x/crypto/ssh" +) + +// ErrSSHKeyPassphraseUnsupported is returned when the supplied PEM is +// passphrase-protected. Sentinel so HTTP/WS handlers can map it to the +// localized i18n message without string-matching. +var ErrSSHKeyPassphraseUnsupported = errors.New("ssh key: passphrase-protected keys not supported in v1") + +// ValidatePATToken is the exported alias of validateTokenShape — HTTP/WS +// save handlers call this BEFORE encrypting so a malformed token (empty, +// oversize, control chars) is rejected with a localized error before any +// DB write or audit emit. Phase 3's runtime check still defends in depth. +func ValidatePATToken(tok string) error { return validateTokenShape(tok) } + +// ValidateSSHKey parses the supplied PEM with x/crypto/ssh. Any non-nil +// error blocks the save: a key that fails to parse here will also fail at +// `ssh -i` time, and we want the diagnostic to happen at save (where the +// user can fix it) not at exec (where the agent stalls). +func ValidateSSHKey(pem []byte) error { + if len(pem) == 0 { + return errors.New("ssh key: empty PEM") + } + _, err := ssh.ParsePrivateKey(pem) + if err == nil { + return nil + } + var pme *ssh.PassphraseMissingError + if errors.As(err, &pme) { + return ErrSSHKeyPassphraseUnsupported + } + return fmt.Errorf("ssh key parse: %w", err) +} diff --git a/internal/tools/credential_adapter_git_ssh_test.go b/internal/tools/credential_adapter_git_ssh_test.go new file mode 100644 index 00000000..3039322d --- /dev/null +++ b/internal/tools/credential_adapter_git_ssh_test.go @@ -0,0 +1,240 @@ +package tools + +// Phase 4 unit tests for gitAdapter's ssh_key branch and ValidateSSHKey. +// Integration coverage (real exec, tmpfile lifecycle under a child process) +// lives in tests/integration/git_adapter_ssh_test.go. + +import ( + "context" + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "encoding/json" + "errors" + "os" + "runtime" + "strings" + "testing" + + "golang.org/x/crypto/ssh" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// genEd25519PEM returns an unencrypted OpenSSH PEM-encoded ed25519 private key. +func genEd25519PEM(t *testing.T) []byte { + t.Helper() + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("ed25519 gen: %v", err) + } + block, err := ssh.MarshalPrivateKey(priv, "") + if err != nil { + t.Fatalf("marshal key: %v", err) + } + return []byte("-----BEGIN OPENSSH PRIVATE KEY-----\n" + + base64Encode(block.Bytes) + + "\n-----END OPENSSH PRIVATE KEY-----\n") +} + +// genEd25519PEMWithPassphrase returns a passphrase-encrypted ed25519 key. +func genEd25519PEMWithPassphrase(t *testing.T, pw string) []byte { + t.Helper() + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("ed25519 gen: %v", err) + } + block, err := ssh.MarshalPrivateKeyWithPassphrase(priv, "", []byte(pw)) + if err != nil { + t.Fatalf("marshal encrypted key: %v", err) + } + return []byte("-----BEGIN OPENSSH PRIVATE KEY-----\n" + + base64Encode(block.Bytes) + + "\n-----END OPENSSH PRIVATE KEY-----\n") +} + +// base64Encode wraps stdlib base64 at 70 cols to mimic real OpenSSH PEM. +func base64Encode(b []byte) string { + s := base64.StdEncoding.EncodeToString(b) + var out strings.Builder + for i := 0; i < len(s); i += 70 { + end := i + 70 + if end > len(s) { + end = len(s) + } + if i > 0 { + out.WriteByte('\n') + } + out.WriteString(s[i:end]) + } + return out.String() +} + +// 1. AC5: passphrase keys rejected via sentinel. +func TestValidateSSHKey_RejectsPassphrase(t *testing.T) { + pem := genEd25519PEMWithPassphrase(t, "topsecret") + err := ValidateSSHKey(pem) + if !errors.Is(err, ErrSSHKeyPassphraseUnsupported) { + t.Fatalf("want ErrSSHKeyPassphraseUnsupported, got %v", err) + } +} + +// 2. Plain ed25519 passes. +func TestValidateSSHKey_AcceptsUnencrypted(t *testing.T) { + if err := ValidateSSHKey(genEd25519PEM(t)); err != nil { + t.Fatalf("ed25519 should pass: %v", err) + } +} + +// 3. Garbage rejected without exposing as passphrase. +func TestValidateSSHKey_RejectsGarbage(t *testing.T) { + err := ValidateSSHKey([]byte("not a key")) + if err == nil { + t.Fatal("expected error") + } + if errors.Is(err, ErrSSHKeyPassphraseUnsupported) { + t.Fatal("garbage must not be classified as passphrase-protected") + } +} + +// 4. Empty rejected. +func TestValidateSSHKey_RejectsEmpty(t *testing.T) { + if err := ValidateSSHKey(nil); err == nil { + t.Fatal("empty must error") + } +} + +// 5. Prepare(ssh_key) shape: env, options, scrub, cleanup. +func TestGitAdapter_PrepareSSH(t *testing.T) { + pem := genEd25519PEM(t) + blob, _ := json.Marshal(map[string]string{"key": string(pem)}) + ct, hs := "ssh_key", "github.com" + cred := &store.SecureCLIUserCredential{ + CredentialType: &ct, + HostScope: &hs, + EncryptedEnv: blob, + } + + a := gitAdapter{} + inj, err := a.Prepare(context.Background(), nil, cred, + []string{"clone", "git@github.com:o/r.git", "/tmp/dst"}) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + if len(inj.ArgvPrefix) != 0 { + t.Fatalf("argv leak: %v", inj.ArgvPrefix) + } + gsc := inj.Env["GIT_SSH_COMMAND"] + if !strings.HasPrefix(gsc, "ssh -i ") { + t.Fatalf("GIT_SSH_COMMAND prefix wrong: %q", gsc) + } + for _, need := range []string{ + "-o IdentitiesOnly=yes", + "-o BatchMode=yes", + "-o StrictHostKeyChecking=accept-new", + } { + if !strings.Contains(gsc, need) { + t.Fatalf("GIT_SSH_COMMAND missing %q\nfull: %s", need, gsc) + } + } + // Extract keypath: token after "-i ". + rest := strings.TrimPrefix(gsc, "ssh -i ") + keyPath := strings.SplitN(rest, " ", 2)[0] + if !strings.Contains(keyPath, "goclaw-gitkey-") { + t.Fatalf("keypath not in expected tmp prefix: %s", keyPath) + } + if !strings.HasPrefix(keyPath, os.TempDir()) { + t.Fatalf("keypath not under TempDir(%s): %s", os.TempDir(), keyPath) + } + // 0600 perms (POSIX only). + if runtime.GOOS != "windows" { + st, err := os.Stat(keyPath) + if err != nil { + t.Fatalf("stat keypath: %v", err) + } + if st.Mode().Perm() != 0o600 { + t.Fatalf("keypath perms = %o, want 0600", st.Mode().Perm()) + } + } + if len(inj.ScrubValues) != 1 || inj.ScrubValues[0] != keyPath { + t.Fatalf("ScrubValues = %v, want [%s]", inj.ScrubValues, keyPath) + } + if inj.Cleanup == nil { + t.Fatal("Cleanup must be non-nil") + } + // File present pre-cleanup, gone after. + if _, err := os.Stat(keyPath); err != nil { + t.Fatalf("pre-cleanup stat: %v", err) + } + if err := inj.Cleanup(); err != nil { + t.Fatalf("cleanup: %v", err) + } + if _, err := os.Stat(keyPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("post-cleanup stat want ErrNotExist, got: %v", err) + } +} + +// 6. SSH path also enforces host match: scp-form mismatch. +func TestGitAdapter_PrepareSSH_HostMismatchSCPForm(t *testing.T) { + pem := genEd25519PEM(t) + blob, _ := json.Marshal(map[string]string{"key": string(pem)}) + ct, hs := "ssh_key", "github.com" + cred := &store.SecureCLIUserCredential{ + CredentialType: &ct, + HostScope: &hs, + EncryptedEnv: blob, + } + a := gitAdapter{} + _, err := a.Prepare(context.Background(), nil, cred, + []string{"clone", "git@gitlab.com:o/r.git"}) + if err == nil { + t.Fatal("expected host mismatch") + } + var mismatch *errCredentialHostMismatch + if !errors.As(err, &mismatch) { + t.Fatalf("want *errCredentialHostMismatch, got %T: %v", err, err) + } + if strings.Contains(err.Error(), string(pem)) { + t.Fatal("key PEM leaked into error message") + } +} + +// 7. Cleanup is idempotent — Phase 2b lockstep test. +func TestGitAdapter_PrepareSSH_CleanupIdempotent(t *testing.T) { + pem := genEd25519PEM(t) + blob, _ := json.Marshal(map[string]string{"key": string(pem)}) + ct, hs := "ssh_key", "github.com" + cred := &store.SecureCLIUserCredential{ + CredentialType: &ct, + HostScope: &hs, + EncryptedEnv: blob, + } + a := gitAdapter{} + inj, err := a.Prepare(context.Background(), nil, cred, + []string{"clone", "git@github.com:o/r.git"}) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + if err := inj.Cleanup(); err != nil { + t.Fatalf("first cleanup: %v", err) + } + if err := inj.Cleanup(); err != nil { + t.Fatalf("second cleanup must be nil, got %v", err) + } +} + +// 8. Blob missing 'key' field rejected before any tmpfile is created. +func TestGitAdapter_PrepareSSH_RejectsMalformedBlob(t *testing.T) { + ct, hs := "ssh_key", "github.com" + cred := &store.SecureCLIUserCredential{ + CredentialType: &ct, + HostScope: &hs, + EncryptedEnv: []byte(`{"private_key":"-----BEGIN..."}`), + } + a := gitAdapter{} + _, err := a.Prepare(context.Background(), nil, cred, + []string{"clone", "git@github.com:o/r.git"}) + if err == nil { + t.Fatal("expected error for missing 'key' field") + } +} diff --git a/internal/tools/credential_adapter_git_test.go b/internal/tools/credential_adapter_git_test.go new file mode 100644 index 00000000..bb5a6b9c --- /dev/null +++ b/internal/tools/credential_adapter_git_test.go @@ -0,0 +1,417 @@ +package tools + +import ( + "context" + "encoding/json" + "errors" + "os" + "os/exec" + "path/filepath" + "reflect" + "runtime" + "sort" + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +func gitAdapterInstance(t *testing.T) CredentialAdapter { + t.Helper() + a := AdapterFor("git") + if a.Name() != "git" { + t.Fatalf("git adapter not registered; AdapterFor(git)=%q", a.Name()) + } + return a +} + +// 1. Sniffing — locks AC8 +func TestGitAdapter_ShouldInject(t *testing.T) { + a := gitAdapterInstance(t) + yes := []string{"clone", "fetch", "pull", "push", "submodule"} + no := []string{"status", "log", "diff", "commit", "init", "config", "--version", ""} + for _, sub := range yes { + if !a.ShouldInject([]string{sub, "x"}) { + t.Errorf("ShouldInject(%q) = false, want true", sub) + } + } + for _, sub := range no { + if a.ShouldInject([]string{sub}) { + t.Errorf("ShouldInject(%q) = true, want false", sub) + } + } + if a.ShouldInject(nil) { + t.Errorf("ShouldInject(nil) = true, want false") + } +} + +// 2. Host parsing +func TestParseHostFromGitURL(t *testing.T) { + tests := []struct { + url string + want string + err bool + }{ + {"https://github.com/o/r.git", "github.com", false}, + {"https://gitea.example.com:8443/o/r.git", "gitea.example.com:8443", false}, + {"git@github.com:o/r.git", "github.com", false}, + {"ssh://git@gitlab.com:2222/o/r.git", "gitlab.com:2222", false}, + {"ssh://git@gitlab.com/o/r.git", "gitlab.com", false}, + {"", "", true}, + {"not-a-url", "", true}, + {"http://", "", true}, + } + for _, tc := range tests { + got, err := parseHostFromGitURL(tc.url) + if tc.err { + if err == nil { + t.Errorf("parseHostFromGitURL(%q) err=nil, want err", tc.url) + } + continue + } + if err != nil { + t.Errorf("parseHostFromGitURL(%q) err=%v", tc.url, err) + continue + } + if got != tc.want { + t.Errorf("parseHostFromGitURL(%q) = %q, want %q", tc.url, got, tc.want) + } + } +} + +// 3. Clone host resolution +func TestResolveTargetHost_Clone(t *testing.T) { + host, err := resolveTargetHost(context.Background(), []string{"clone", "--depth=1", "https://github.com/o/r.git"}, "") + if err != nil || host != "github.com" { + t.Fatalf("clone with URL: host=%q err=%v, want github.com nil", host, err) + } + if _, err := resolveTargetHost(context.Background(), []string{"clone", "--depth=1"}, ""); err == nil { + t.Fatalf("clone with no URL: err=nil, want err") + } +} + +// 4. fetch/pull/push host resolution via fake git stub +func TestResolveTargetHost_RemoteName(t *testing.T) { + tmp := t.TempDir() + // Fake git script: outputs "https://stub.example.com/o/r.git" for any config --get remote..url + stub := filepath.Join(tmp, "git") + script := "#!/bin/sh\necho https://stub.example.com/o/r.git\n" + if runtime.GOOS == "windows" { + t.Skip("shell script stub not supported on windows") + } + if err := os.WriteFile(stub, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + // Prepend tmp to PATH so `git` resolves to our stub. + oldPath := os.Getenv("PATH") + t.Cleanup(func() { os.Setenv("PATH", oldPath) }) + os.Setenv("PATH", tmp+string(os.PathListSeparator)+oldPath) + + cases := [][]string{ + {"fetch"}, + {"fetch", "origin"}, + {"pull", "upstream"}, + {"push", "origin", "main"}, + {"push", "--repo=mirror"}, + {"submodule", "update", "--init"}, + } + for _, argv := range cases { + host, err := resolveTargetHost(context.Background(), argv, tmp) + if err != nil { + t.Errorf("resolveTargetHost(%v) err=%v", argv, err) + continue + } + if host != "stub.example.com" { + t.Errorf("resolveTargetHost(%v) host=%q, want stub.example.com", argv, host) + } + } +} + +// 5. Host mismatch — AC3 +func TestGitAdapter_HostMismatch(t *testing.T) { + a := gitAdapterInstance(t) + credType := "pat" + hostScope := "github.com" + blob, _ := json.Marshal(map[string]string{"token": "ghp_secrettoken12345"}) + cred := &store.SecureCLIUserCredential{ + CredentialType: &credType, + HostScope: &hostScope, + EncryptedEnv: blob, + } + _, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, cred, + []string{"clone", "https://gitlab.com/o/r.git"}) + if err == nil { + t.Fatalf("expected host mismatch error") + } + var mm *errCredentialHostMismatch + if !errors.As(err, &mm) { + t.Fatalf("err=%v (%T), want *errCredentialHostMismatch", err, err) + } + if mm.credHost != "github.com" || mm.targetHost != "gitlab.com" { + t.Errorf("mismatch fields: cred=%q target=%q", mm.credHost, mm.targetHost) + } + // Error string must not leak token. + if strings.Contains(err.Error(), "ghp_secrettoken12345") { + t.Errorf("error leaks token: %v", err) + } +} + +// 6. PAT injection shape — env approach, no argv +func TestGitAdapter_PreparePAT(t *testing.T) { + a := gitAdapterInstance(t) + credType := "pat" + hostScope := "github.com" + blob, _ := json.Marshal(map[string]string{"token": "ghp_abc"}) + cred := &store.SecureCLIUserCredential{ + CredentialType: &credType, + HostScope: &hostScope, + EncryptedEnv: blob, + } + inj, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, cred, + []string{"clone", "https://github.com/o/r.git"}) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + if inj == nil { + t.Fatal("nil Injection") + } + if len(inj.ArgvPrefix) != 0 { + t.Errorf("ArgvPrefix=%v, want empty (PAT goes through env)", inj.ArgvPrefix) + } + wantEnv := map[string]string{ + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "http.https://github.com/.extraheader", + "GIT_CONFIG_VALUE_0": "Authorization: Bearer ghp_abc", + } + if !reflect.DeepEqual(inj.Env, wantEnv) { + t.Errorf("Env=%v, want %v", inj.Env, wantEnv) + } + if len(inj.ScrubValues) != 1 || inj.ScrubValues[0] != "ghp_abc" { + t.Errorf("ScrubValues=%v, want [ghp_abc]", inj.ScrubValues) + } + if inj.Cleanup != nil { + t.Errorf("Cleanup must be nil for PAT path") + } +} + +// 8a. CRLF rejection +func TestGitAdapter_PreparePAT_RejectsCRLF(t *testing.T) { + a := gitAdapterInstance(t) + credType := "pat" + hostScope := "github.com" + blob, _ := json.Marshal(map[string]string{"token": "ghp_abc\r\nX-Injected: evil"}) + cred := &store.SecureCLIUserCredential{ + CredentialType: &credType, + HostScope: &hostScope, + EncryptedEnv: blob, + } + _, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, cred, + []string{"clone", "https://github.com/o/r.git"}) + if err == nil { + t.Fatalf("expected CRLF rejection") + } + if !errors.Is(err, errTokenControlChar) { + t.Errorf("err=%v, want errTokenControlChar", err) + } +} + +// 8b. Empty + oversize rejection +func TestGitAdapter_PreparePAT_RejectsEmptyAndOversize(t *testing.T) { + a := gitAdapterInstance(t) + credType := "pat" + hostScope := "github.com" + + emptyBlob, _ := json.Marshal(map[string]string{"token": ""}) + cred := &store.SecureCLIUserCredential{CredentialType: &credType, HostScope: &hostScope, EncryptedEnv: emptyBlob} + if _, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, cred, + []string{"clone", "https://github.com/o/r.git"}); !errors.Is(err, errEmptyToken) { + t.Errorf("empty token: err=%v, want errEmptyToken", err) + } + + bigBlob, _ := json.Marshal(map[string]string{"token": strings.Repeat("a", 5000)}) + cred.EncryptedEnv = bigBlob + if _, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, cred, + []string{"clone", "https://github.com/o/r.git"}); !errors.Is(err, errTokenTooLong) { + t.Errorf("oversize token: err=%v, want errTokenTooLong", err) + } +} + +// 8c. IDN normalization +func TestNormalizeHost_IDN(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"github.com", "github.com"}, + {"GitHub.Com.", "github.com"}, + {" GitHub.Com ", "github.com"}, + {"gitea.example.com:8443", "gitea.example.com:8443"}, + } + for _, tc := range tests { + got, err := normalizeHost(tc.in) + if err != nil { + t.Errorf("normalizeHost(%q) err=%v", tc.in, err) + continue + } + if got != tc.want { + t.Errorf("normalizeHost(%q) = %q, want %q", tc.in, got, tc.want) + } + } + // Empty after trim + if _, err := normalizeHost(" "); err == nil { + t.Errorf("normalizeHost empty: err=nil, want err") + } + // IDN punycode round trip — both forms normalize to the same ASCII output. + asciiFromUnicode, err := normalizeHost("gitlab.中国") + if err != nil { + t.Fatalf("normalizeHost unicode: %v", err) + } + asciiFromPuny, err := normalizeHost("gitlab.xn--fiqs8s") + if err != nil { + t.Fatalf("normalizeHost puny: %v", err) + } + if asciiFromUnicode != asciiFromPuny { + t.Errorf("unicode=%q puny=%q — must match after normalize", asciiFromUnicode, asciiFromPuny) + } +} + +// 8d. Embedded userinfo rejection +func TestParseHostFromGitURL_RejectsUserinfo(t *testing.T) { + if _, err := parseHostFromGitURL("https://attacker.com@github.com/o/r.git"); err == nil { + t.Errorf("expected error for embedded userinfo") + } + if _, err := parseHostFromGitURL("https://user:pass@github.com/o/r.git"); err == nil { + t.Errorf("expected error for userinfo with password") + } + // scp-form git@host:path is NOT userinfo (no '@' arrives as URL userinfo); allowed. + if _, err := parseHostFromGitURL("git@github.com:o/r.git"); err != nil { + t.Errorf("scp-form should parse: %v", err) + } + // ssh:// with git@ is the conventional form — allow because it's the only + // supported form for SSH and removing it would break Phase 4. + if _, err := parseHostFromGitURL("ssh://git@gitlab.com/o/r.git"); err != nil { + t.Errorf("ssh://git@host should parse: %v", err) + } +} + +// 9. Legacy env passthrough +func TestGitAdapter_LegacyEnvCredentialPassthrough(t *testing.T) { + a := gitAdapterInstance(t) + // credential_type=NULL → passthrough + cred := &store.SecureCLIUserCredential{ + EncryptedEnv: []byte(`{"GIT_TOKEN":"x"}`), + } + inj, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, cred, + []string{"clone", "https://github.com/o/r.git"}) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + if len(inj.Env) != 0 || len(inj.ArgvPrefix) != 0 { + t.Errorf("legacy env cred must passthrough, got %+v", inj) + } + // Explicit credential_type="env" → same. + envType := "env" + cred.CredentialType = &envType + inj, err = a.Prepare(context.Background(), &store.SecureCLIBinary{}, cred, + []string{"clone", "https://github.com/o/r.git"}) + if err != nil { + t.Fatalf("Prepare env: %v", err) + } + if len(inj.Env) != 0 { + t.Errorf("env cred must passthrough, got %+v", inj) + } +} + +// (Phase 4 implements ssh_key; coverage moved to +// credential_adapter_git_ssh_test.go.) + +// 13. CVE-2018-17456 regression: malicious .git/config must not execute +// `ext::sh -c …` payload. Our `git config --get` path returns the literal +// URL string for parseHostFromGitURL to reject, NEVER hands it to +// `git remote get-url` (which would evaluate the protocol handler). +func TestResolveTargetHost_RejectsExtProtocolInjection(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses /tmp marker; POSIX only") + } + tmp := t.TempDir() + gitDir := filepath.Join(tmp, ".git") + if err := os.MkdirAll(gitDir, 0o755); err != nil { + t.Fatal(err) + } + // Init a minimal repo so `git -C ` recognizes it. + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/main\n"), 0o644); err != nil { + t.Fatal(err) + } + marker := filepath.Join(tmp, "pwn-marker") + // Plant malicious config — would execute `touch ` if URL ever passes + // through git's protocol handler. + cfg := `[core] + repositoryformatversion = 0 +[remote "origin"] + url = ext::sh -c "touch ` + marker + `" +` + if err := os.WriteFile(filepath.Join(gitDir, "config"), []byte(cfg), 0o644); err != nil { + t.Fatal(err) + } + + _, err := resolveTargetHost(context.Background(), []string{"fetch"}, tmp) + // We expect either an error or a non-host string that parseHostFromGitURL + // rejects. EITHER way the marker file must NOT exist. + _ = err + if _, statErr := os.Stat(marker); statErr == nil { + t.Fatalf("CVE-2018-17456 regression: ext::sh payload executed — marker %s exists", marker) + } +} + +// Sanity: ensure the test stub of `git --version` is real (avoids accidental +// stub bleed from the RemoteName test affecting the CVE test). +func TestGit_ConfigGet_UsesRealGit(t *testing.T) { + out, err := exec.Command("git", "--version").CombinedOutput() + if err != nil || !strings.Contains(string(out), "git version") { + t.Skipf("real git unavailable: out=%q err=%v", out, err) + } +} + +// 14. DenyArgs in the `git` preset must block `-c http.…` overrides that +// would shadow the adapter's host-scoped extraheader. +func TestSecureCLI_DenyArgs_BlocksHttpCConfig(t *testing.T) { + preset, ok := CLIPresets["git"] + if !ok { + t.Fatal("git preset missing") + } + denyJSON, err := json.Marshal(preset.DenyArgs) + if err != nil { + t.Fatal(err) + } + + mustBlock := [][]string{ + {"-c", "http.extraHeader=Authorization: Bearer evil", "clone", "https://github.com/o/r.git"}, + {"-c", "credential.helper=store", "clone", "https://github.com/o/r.git"}, + {"-c", "core.sshCommand=ssh -i /etc/shadow", "clone", "git@github.com:o/r.git"}, + {"config", "--global", "user.email", "x@y"}, + {"config", "--system", "credential.helper", "store"}, + {"daemon", "--export-all"}, + } + for _, args := range mustBlock { + if p := matchesBinaryDeny(args, denyJSON); p == "" { + t.Errorf("DenyArgs did NOT block %v", args) + } + } + + mustAllow := [][]string{ + {"clone", "https://github.com/o/r.git"}, + {"fetch", "origin"}, + {"push", "origin", "main"}, + {"submodule", "update", "--init", "--recursive"}, + {"status"}, + {"config", "user.email"}, // local-only config read; not --global/--system + } + for _, args := range mustAllow { + if p := matchesBinaryDeny(args, denyJSON); p != "" { + t.Errorf("DenyArgs falsely blocked %v with pattern %q", args, p) + } + } +} + +// sortedHelperForTest is only here to keep `sort` import used. +var _ = sort.Strings diff --git a/internal/tools/credential_adapter_psql.go b/internal/tools/credential_adapter_psql.go new file mode 100644 index 00000000..9a97de81 --- /dev/null +++ b/internal/tools/credential_adapter_psql.go @@ -0,0 +1,101 @@ +package tools + +import ( + "context" + "encoding/json" + "fmt" + "strings" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// psqlAdapter is the framework-validation stub adapter (Phase 2b). +// +// Why it exists: Phase 2 introduces a generic CredentialAdapter interface that +// MUST hold for non-git credential families (kubectl, docker, psql, npm…) or +// it's a git hack pretending to be generic. The psql/PGPASSFILE pattern is the +// simplest non-git case — env-only, file-based, no argv mutation — so if the +// interface doesn't fit it cleanly, that's the signal to reshape NOW, before +// Phase 3/4 cement the shape around git. +// +// Production usage requires UI work (Phase 5 extends the cred-type picker +// beyond git). Until then, this preset is registered but only routes when an +// operator manually sets a binary row's adapter_name='psql' AND seeds a +// `pg_password_file` credential via DB / API. +type psqlAdapter struct{} + +func (psqlAdapter) Name() string { return "psql" } + +// ShouldInject returns true for any invocation — psql has no subcommand surface, +// every call is "the command", so the adapter is always consulted. +func (psqlAdapter) ShouldInject(_ []string) bool { return true } + +// pgPasswordFile is the on-disk JSON shape stored in +// secure_cli_user_credentials.encrypted_env when credential_type='pg_password_file'. +type pgPasswordFile struct { + Host string `json:"host"` + Port string `json:"port"` + Database string `json:"database"` + User string `json:"user"` + Password string `json:"password"` +} + +func (psqlAdapter) Prepare(_ context.Context, _ *store.SecureCLIBinary, cred *store.SecureCLIUserCredential, _ []string) (*Injection, error) { + if cred == nil { + return &Injection{}, nil + } + credType := "" + if cred.CredentialType != nil { + credType = *cred.CredentialType + } + // Legacy env-vars credential — leave untouched, behave like passthrough. + if credType != "pg_password_file" { + return &Injection{}, nil + } + + var pg pgPasswordFile + if err := json.Unmarshal(cred.EncryptedEnv, &pg); err != nil { + return nil, fmt.Errorf("decode pg credential: %w", err) + } + if pg.Password == "" { + return nil, fmt.Errorf("pg credential missing password") + } + + line := fmt.Sprintf("%s:%s:%s:%s:%s\n", + escapePgpass(pg.Host), + escapePgpass(pg.Port), + escapePgpass(pg.Database), + escapePgpass(pg.User), + escapePgpass(pg.Password), + ) + + path, cleanup, err := materializeEphemeral(context.TODO(), []byte(line), "pgpass") + if err != nil { + return nil, err + } + return &Injection{ + Env: map[string]string{"PGPASSFILE": path}, + Cleanup: cleanup, + // Scrub both the secret AND the on-disk tmpfile path — psql echoes + // `could not open password file ""` on permission/IO errors. + // Mirrors git SSH adapter pattern (credential_adapter_git.go). + ScrubValues: []string{pg.Password, path}, + }, nil +} + +// escapePgpass escapes backslash and colon per the libpq .pgpass spec so a +// malicious password containing `:` cannot break the line format and inject a +// second entry. +// +// postgresql.org/docs/current/libpq-pgpass.html +func escapePgpass(s string) string { + // Order matters: escape backslash FIRST, then colon, otherwise the colon + // escape's own backslash gets double-escaped. + s = strings.ReplaceAll(s, `\`, `\\`) + s = strings.ReplaceAll(s, `:`, `\:`) + return s +} + +func init() { + RegisterAdapter(psqlAdapter{}) +} diff --git a/internal/tools/credential_adapter_psql_test.go b/internal/tools/credential_adapter_psql_test.go new file mode 100644 index 00000000..fd4b80af --- /dev/null +++ b/internal/tools/credential_adapter_psql_test.go @@ -0,0 +1,188 @@ +package tools + +import ( + "context" + "encoding/json" + "errors" + "io/fs" + "os" + "runtime" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +func psqlAdapterInstance(t *testing.T) CredentialAdapter { + t.Helper() + a := AdapterFor("psql") + if a.Name() != "psql" { + t.Fatalf("psql adapter not registered; AdapterFor(psql)=%q", a.Name()) + } + return a +} + +func TestPsqlAdapter_RegisteredAndShouldInject(t *testing.T) { + a := psqlAdapterInstance(t) + if !a.ShouldInject(nil) || !a.ShouldInject([]string{"-c", "select 1"}) { + t.Fatalf("psql ShouldInject must always return true") + } +} + +func TestPsqlAdapter_LegacyEnvPassthrough(t *testing.T) { + a := psqlAdapterInstance(t) + + // No credential → passthrough Injection, no error. + inj, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, nil, nil) + if err != nil || inj == nil || len(inj.Env) != 0 { + t.Fatalf("nil cred should be no-op: inj=%+v err=%v", inj, err) + } + + // Cred with credential_type=NULL (legacy env-vars) → passthrough. + inj, err = a.Prepare(context.Background(), &store.SecureCLIBinary{}, &store.SecureCLIUserCredential{ + EncryptedEnv: []byte(`{"PGPASSWORD":"x"}`), + }, nil) + if err != nil { + t.Fatalf("legacy env cred raised err: %v", err) + } + if inj == nil || len(inj.Env) != 0 || inj.Cleanup != nil { + t.Fatalf("legacy env cred must produce empty Injection, got %+v", inj) + } +} + +func TestPsqlAdapter_PreparePgPasswordFile(t *testing.T) { + a := psqlAdapterInstance(t) + credType := "pg_password_file" + blob, _ := json.Marshal(pgPasswordFile{ + Host: "db.internal", + Port: "5432", + Database: "prod", + User: "app", + Password: "s3cret!", + }) + inj, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, &store.SecureCLIUserCredential{ + CredentialType: &credType, + EncryptedEnv: blob, + }, nil) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + if inj == nil { + t.Fatalf("Prepare returned nil Injection") + } + t.Cleanup(func() { + if inj.Cleanup != nil { + _ = inj.Cleanup() + } + }) + + path, ok := inj.Env["PGPASSFILE"] + if !ok || path == "" { + t.Fatalf("PGPASSFILE missing from Injection.Env: %v", inj.Env) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile %s: %v", path, err) + } + want := "db.internal:5432:prod:app:s3cret!\n" + if string(got) != want { + t.Fatalf(".pgpass line=%q, want %q", got, want) + } + if runtime.GOOS != "windows" { + info, _ := os.Stat(path) + if perm := info.Mode().Perm(); perm != 0o600 { + t.Fatalf("file mode=%o, want 0600", perm) + } + } + // ScrubValues must include both the password AND the on-disk tmpfile path + // (psql echoes `could not open password file ""` on IO errors). + if len(inj.ScrubValues) != 2 || inj.ScrubValues[0] != "s3cret!" || inj.ScrubValues[1] != path { + t.Fatalf("ScrubValues=%v, want [s3cret! %s]", inj.ScrubValues, path) + } + if inj.Cleanup == nil { + t.Fatalf("Cleanup missing") + } + if err := inj.Cleanup(); err != nil { + t.Fatalf("Cleanup: %v", err) + } + if _, err := os.Stat(path); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("file still exists after cleanup") + } + // argv prefix must be empty — psql uses env only. + if len(inj.ArgvPrefix) != 0 { + t.Fatalf("psql adapter must not set ArgvPrefix, got %v", inj.ArgvPrefix) + } +} + +func TestPsqlAdapter_EscapesColonsAndBackslashes(t *testing.T) { + a := psqlAdapterInstance(t) + credType := "pg_password_file" + // Adversarial password: contains both : and \. + // .pgpass spec: escape backslash first, then colon. + // p: ab:cd → ab\:cd + // p: x\y → x\\y + // combined: x\:y → x\\\:y + blob, _ := json.Marshal(pgPasswordFile{ + Host: "h", + Port: "5432", + Database: "d", + User: "u", + Password: `x\:y`, + }) + inj, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, &store.SecureCLIUserCredential{ + CredentialType: &credType, + EncryptedEnv: blob, + }, nil) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + t.Cleanup(func() { + if inj.Cleanup != nil { + _ = inj.Cleanup() + } + }) + + got, err := os.ReadFile(inj.Env["PGPASSFILE"]) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + want := `h:5432:d:u:x\\\:y` + "\n" + if string(got) != want { + t.Fatalf("escaped line=%q, want %q", got, want) + } +} + +func TestPsqlAdapter_RejectsEmptyPassword(t *testing.T) { + a := psqlAdapterInstance(t) + credType := "pg_password_file" + blob, _ := json.Marshal(pgPasswordFile{Host: "h", Port: "5432", Database: "d", User: "u"}) + if _, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, &store.SecureCLIUserCredential{ + CredentialType: &credType, + EncryptedEnv: blob, + }, nil); err == nil { + t.Fatalf("expected error for empty password") + } +} + +func TestPsqlAdapter_RejectsInvalidJSON(t *testing.T) { + a := psqlAdapterInstance(t) + credType := "pg_password_file" + if _, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, &store.SecureCLIUserCredential{ + CredentialType: &credType, + EncryptedEnv: []byte(`not json`), + }, nil); err == nil { + t.Fatalf("expected error for invalid JSON") + } +} + +// Interface validation gate (Phase 2b §"Interface validation gate"): +// Confirms psql consumed the Phase 2 Injection shape without modification. +// If this test fails to compile after a Phase 2 interface change, the change +// likely needs to be re-evaluated for non-git generality. +func TestPsqlAdapter_InterfaceValidationGate(t *testing.T) { + var _ CredentialAdapter = psqlAdapter{} + // All four Injection fields are exercised by Prepare: + // Env → PGPASSFILE + // Cleanup → tmpfile removal + // ScrubValues → password redaction + // ArgvPrefix → intentionally empty (not all adapters need argv mutation) +} diff --git a/internal/tools/credential_adapter_test.go b/internal/tools/credential_adapter_test.go new file mode 100644 index 00000000..f8518ab4 --- /dev/null +++ b/internal/tools/credential_adapter_test.go @@ -0,0 +1,254 @@ +package tools + +import ( + "context" + "errors" + "sync" + "sync/atomic" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +func TestPassthroughAdapter_NoOp(t *testing.T) { + a := AdapterFor("passthrough") + if a.Name() != "passthrough" { + t.Fatalf("Name()=%q, want passthrough", a.Name()) + } + + cases := [][]string{ + nil, + {}, + {"clone", "https://github.com/x/y.git"}, + {"--help"}, + } + for _, argv := range cases { + if a.ShouldInject(argv) { + t.Fatalf("ShouldInject(%v)=true, want false (passthrough must never inject)", argv) + } + } + + inj, err := a.Prepare(context.Background(), &store.SecureCLIBinary{}, nil, []string{"clone"}) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + if inj == nil { + t.Fatalf("Prepare returned nil Injection") + } + if len(inj.ArgvPrefix) != 0 || len(inj.Env) != 0 || len(inj.ScrubValues) != 0 { + t.Fatalf("passthrough produced non-empty Injection: %+v", inj) + } + if inj.Cleanup != nil { + t.Fatalf("passthrough must not register Cleanup") + } +} + +func TestAdapterFor_FallsBackToPassthrough(t *testing.T) { + cases := []string{"", "nonexistent", "git-not-registered-yet"} + for _, name := range cases { + a := AdapterFor(name) + if a.Name() != "passthrough" { + t.Fatalf("AdapterFor(%q)=%q, want passthrough", name, a.Name()) + } + } +} + +// recordingAdapter captures Prepare calls so the hook-point test can verify +// the runtime wired the adapter correctly. +type recordingAdapter struct { + name string + shouldFn func([]string) bool + prepareErr error + calls int32 + gotArgv []string +} + +func (r *recordingAdapter) Name() string { return r.name } +func (r *recordingAdapter) ShouldInject(argv []string) bool { + if r.shouldFn != nil { + return r.shouldFn(argv) + } + return true +} +func (r *recordingAdapter) Prepare(_ context.Context, _ *store.SecureCLIBinary, _ *store.SecureCLIUserCredential, argv []string) (*Injection, error) { + atomic.AddInt32(&r.calls, 1) + r.gotArgv = append([]string(nil), argv...) + if r.prepareErr != nil { + return nil, r.prepareErr + } + return &Injection{}, nil +} + +func TestRegisterAdapter_RoundTrip(t *testing.T) { + name := "test-roundtrip-adapter" + t.Cleanup(func() { + adaptersMu.Lock() + delete(adapters, name) + adaptersMu.Unlock() + }) + + r := &recordingAdapter{name: name} + RegisterAdapter(r) + + got := AdapterFor(name) + if got.Name() != name { + t.Fatalf("AdapterFor(%q)=%q, want %q", name, got.Name(), name) + } +} + +func TestRegisterAdapter_NilIgnored(t *testing.T) { + // Should not panic, should not affect registry. + before := AdapterFor("passthrough") + RegisterAdapter(nil) + after := AdapterFor("passthrough") + if before.Name() != after.Name() { + t.Fatalf("nil RegisterAdapter changed passthrough resolution") + } +} + +func TestInjection_StructShape(t *testing.T) { + // Locks Injection field types so future refactors can't silently change + // the contract the adapter pipeline depends on. + flag := false + inj := Injection{ + ArgvPrefix: []string{"-c", "foo=bar"}, + Env: map[string]string{"GIT_TERMINAL_PROMPT": "0"}, + Cleanup: func() error { flag = true; return nil }, + ScrubValues: []string{"secretvalue"}, + } + if inj.ArgvPrefix[0] != "-c" || inj.ArgvPrefix[1] != "foo=bar" { + t.Fatalf("ArgvPrefix not preserved") + } + if inj.Env["GIT_TERMINAL_PROMPT"] != "0" { + t.Fatalf("Env not preserved") + } + if err := inj.Cleanup(); err != nil || !flag { + t.Fatalf("Cleanup did not fire: err=%v flag=%v", err, flag) + } + if inj.ScrubValues[0] != "secretvalue" { + t.Fatalf("ScrubValues not preserved") + } +} + +func TestHashHostScope(t *testing.T) { + if got := hashHostScope(nil); got != "none" { + t.Fatalf("hashHostScope(nil)=%q, want none", got) + } + empty := "" + if got := hashHostScope(&empty); got != "none" { + t.Fatalf("hashHostScope(&\"\")=%q, want none", got) + } + + a := "github.com" + b := "gitlab.com" + ha1 := hashHostScope(&a) + ha2 := hashHostScope(&a) + hb := hashHostScope(&b) + + if ha1 != ha2 { + t.Fatalf("same input produced different hashes: %q vs %q", ha1, ha2) + } + if ha1 == hb { + t.Fatalf("different inputs collided: %q == %q", ha1, hb) + } + if len(ha1) != 8 { + t.Fatalf("hash length=%d, want 8 hex chars", len(ha1)) + } + // Must not contain the plaintext hostname anywhere. + if ha1 == a { + t.Fatalf("hash leaked plaintext hostname") + } +} + +func TestSortedKeys_Deterministic(t *testing.T) { + in := map[string]string{ + "GIT_SSH_COMMAND": "ssh -i /tmp/x", + "GIT_TERMINAL_PROMPT": "0", + "GIT_CONFIG_COUNT": "1", + } + got := sortedKeys(in) + want := []string{"GIT_CONFIG_COUNT", "GIT_SSH_COMMAND", "GIT_TERMINAL_PROMPT"} + if len(got) != len(want) { + t.Fatalf("sortedKeys len=%d, want %d", len(got), len(want)) + } + for i := range got { + if got[i] != want[i] { + t.Fatalf("sortedKeys[%d]=%q, want %q", i, got[i], want[i]) + } + } + + if sortedKeys(nil) != nil { + t.Fatalf("sortedKeys(nil) must return nil") + } + if sortedKeys(map[string]string{}) != nil { + t.Fatalf("sortedKeys(empty map) must return nil") + } +} + +func TestScrubBag_PerRequestIsolation(t *testing.T) { + // Two parallel contexts must not see each other's scrub values. + ctxA := WithScrubBag(context.Background()) + ctxB := WithScrubBag(context.Background()) + + AddScrubValuesCtx(ctxA, "secretAAAAAA") + AddScrubValuesCtx(ctxB, "secretBBBBBB") + + gotA := ScrubCredentialsCtx(ctxA, "value: secretAAAAAA leaked + secretBBBBBB visible") + gotB := ScrubCredentialsCtx(ctxB, "value: secretAAAAAA visible + secretBBBBBB leaked") + + // A's bag redacts A's secret, leaves B's untouched. + if got := gotA; got != "value: [REDACTED] leaked + secretBBBBBB visible" { + t.Fatalf("ctxA scrub leaked across tenants: %q", got) + } + if got := gotB; got != "value: secretAAAAAA visible + [REDACTED] leaked" { + t.Fatalf("ctxB scrub leaked across tenants: %q", got) + } +} + +func TestScrubBag_NoBagInContextIsNoop(t *testing.T) { + // AddScrubValuesCtx without a bag must not panic. + AddScrubValuesCtx(context.Background(), "should-not-crash") + // ScrubCredentialsCtx without a bag still runs regex pass. + got := ScrubCredentialsCtx(context.Background(), "api_key=ghp_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + if got == "api_key=ghp_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" { + t.Fatalf("regex pass should still scrub even without bag, got %q", got) + } +} + +func TestScrubBag_ConcurrentAdds(t *testing.T) { + // Race-detector smoke test for the per-bag mutex. + ctx := WithScrubBag(context.Background()) + var wg sync.WaitGroup + for i := 0; i < 16; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + AddScrubValuesCtx(ctx, "concurrentvalueXXXXXX") + }(i) + } + wg.Wait() + out := ScrubCredentialsCtx(ctx, "echo concurrentvalueXXXXXX done") + if out != "echo [REDACTED] done" { + t.Fatalf("concurrent bag adds failed: %q", out) + } +} + +func TestScrubBag_ShortValuesIgnored(t *testing.T) { + ctx := WithScrubBag(context.Background()) + AddScrubValuesCtx(ctx, "x", "ab", "abc", "abcde") // all < 6 chars + got := ScrubCredentialsCtx(ctx, "abc abcde x ab") + if got != "abc abcde x ab" { + t.Fatalf("short values were redacted (false positive): %q", got) + } +} + +// recordingAdapter is the kind of probe a future hook-point integration test +// would register. The compile-time assertion here keeps the interface stable. +func TestRecordingAdapter_ImplementsInterface(t *testing.T) { + var _ CredentialAdapter = (*recordingAdapter)(nil) + // Sanity: an adapter returning prepareErr does not panic. + r := &recordingAdapter{name: "boom", prepareErr: errors.New("forced")} + if _, err := r.Prepare(context.Background(), nil, nil, []string{"x"}); err == nil { + t.Fatalf("Prepare did not return forced error") + } +} diff --git a/internal/tools/credential_audit_log_test.go b/internal/tools/credential_audit_log_test.go new file mode 100644 index 00000000..b878b2d0 --- /dev/null +++ b/internal/tools/credential_audit_log_test.go @@ -0,0 +1,183 @@ +// Phase 6 — audit log shape tests for `security.system_env_injection`. +// +// Pins the JSON field schema operators grep for in their SIEM. A change here +// breaks log-search dashboards in production, so the test asserts: +// - exact event name (`msg=security.system_env_injection`) +// - presence of every documented field +// - host scope hashed (not plaintext) — PII safety +// - env keys present, env values NOT present +// - PAT/SSH key bytes NOT present anywhere in the captured buffer +package tools + +import ( + "bytes" + "encoding/json" + "log/slog" + "regexp" + "strings" + "testing" +) + +// capturedAudit runs fn with a JSON slog handler capturing into buf, then +// restores the previous default logger. Returns parsed JSON records. +func capturedAudit(t *testing.T, fn func()) ([]map[string]any, string) { + t.Helper() + var buf bytes.Buffer + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) + defer slog.SetDefault(prev) + + fn() + + raw := buf.String() + var records []map[string]any + for line := range strings.SplitSeq(strings.TrimRight(raw, "\n"), "\n") { + if line == "" { + continue + } + var rec map[string]any + if err := json.Unmarshal([]byte(line), &rec); err != nil { + t.Fatalf("log line not JSON: %q (%v)", line, err) + } + records = append(records, rec) + } + return records, raw +} + +// 1. Audit log shape — PAT path emits the documented field schema. +func TestEmitSystemEnvInjectionAudit_PAT(t *testing.T) { + scope := "github.com" + // Use sentinel values long enough that substring match in the captured + // log buffer is meaningful (short values like "1" would false-match digits + // in the timestamp). + patToken := "ghp_SENTINEL_PAT_VALUE_4242424242424242" + inj := &Injection{ + Env: map[string]string{ + "GIT_CONFIG_COUNT": "SENTINEL_COUNT_VALUE", + "GIT_CONFIG_KEY_0": "http.https://github.com/.extraheader", + "GIT_CONFIG_VALUE_0": "AUTHORIZATION: basic " + patToken, + }, + // PAT path uses env only, no argv mutation. + ArgvPrefix: nil, + } + + records, raw := capturedAudit(t, func() { + emitSystemEnvInjectionAudit("git", "git", "user-42", inj, &scope) + }) + + if len(records) != 1 { + t.Fatalf("expected exactly 1 audit record, got %d (raw=%q)", len(records), raw) + } + rec := records[0] + if rec["msg"] != "security.system_env_injection" { + t.Fatalf("msg=%v, want security.system_env_injection", rec["msg"]) + } + if rec["adapter"] != "git" { + t.Errorf("adapter=%v, want git", rec["adapter"]) + } + if rec["binary"] != "git" { + t.Errorf("binary=%v, want git", rec["binary"]) + } + if rec["user_id"] != "user-42" { + t.Errorf("user_id=%v, want user-42", rec["user_id"]) + } + if rec["argv_prefix_len"] != float64(0) { + t.Errorf("argv_prefix_len=%v, want 0", rec["argv_prefix_len"]) + } + + keys, _ := rec["env_keys"].([]any) + want := []string{"GIT_CONFIG_COUNT", "GIT_CONFIG_KEY_0", "GIT_CONFIG_VALUE_0"} + if len(keys) != len(want) { + t.Fatalf("env_keys=%v, want %v", keys, want) + } + for i, k := range want { + if keys[i] != k { + t.Errorf("env_keys[%d]=%v, want %s (must be sorted)", i, keys[i], k) + } + } + + hash, _ := rec["host_scope_hash"].(string) + if !regexp.MustCompile(`^[0-9a-f]{8}$`).MatchString(hash) { + t.Errorf("host_scope_hash=%q, want 8 hex chars", hash) + } + if hash == scope { + t.Errorf("host_scope_hash leaked plaintext hostname") + } + + // Plaintext hostname must NOT appear anywhere in the raw log output. + if strings.Contains(raw, scope) { + t.Errorf("plaintext hostname %q leaked into audit log: %s", scope, raw) + } + // Env VALUES must NOT appear in the audit log (only NAMES go in env_keys). + // Especially the PAT-like content: AC6 redaction guarantee. + for _, v := range inj.Env { + if strings.Contains(raw, v) { + t.Errorf("env value %q leaked into audit log: %s", v, raw) + } + } + if strings.Contains(raw, patToken) { + t.Errorf("PAT token leaked into audit log: %s", raw) + } +} + +// 2. Audit log shape — SSH path: env_keys=[GIT_SSH_COMMAND], hash present, +// no PEM bytes in the log. +func TestEmitSystemEnvInjectionAudit_SSH(t *testing.T) { + scope := "gitlab.example.com:2222" + pemBody := "-----BEGIN OPENSSH PRIVATE KEY-----\nfakebody\n-----END OPENSSH PRIVATE KEY-----" + inj := &Injection{ + Env: map[string]string{ + "GIT_SSH_COMMAND": "ssh -i /tmp/goclaw-gitkey-xyz -o StrictHostKeyChecking=accept-new", + }, + } + + records, raw := capturedAudit(t, func() { + emitSystemEnvInjectionAudit("git", "git", "u1", inj, &scope) + }) + + if len(records) != 1 { + t.Fatalf("expected 1 record, got %d", len(records)) + } + rec := records[0] + keys, _ := rec["env_keys"].([]any) + if len(keys) != 1 || keys[0] != "GIT_SSH_COMMAND" { + t.Errorf("env_keys=%v, want [GIT_SSH_COMMAND]", keys) + } + + hash, _ := rec["host_scope_hash"].(string) + if !regexp.MustCompile(`^[0-9a-f]{8}$`).MatchString(hash) { + t.Errorf("host_scope_hash=%q, want 8 hex chars", hash) + } + + // PEM contents and plaintext hostname must NOT leak. + if strings.Contains(raw, "BEGIN OPENSSH") || strings.Contains(raw, pemBody) { + t.Errorf("PEM content leaked into audit log: %s", raw) + } + if strings.Contains(raw, "gitlab.example.com") { + t.Errorf("plaintext hostname leaked into audit log: %s", raw) + } +} + +// 3. Nil injection: emitter is a no-op (audit only fires on actual injection). +func TestEmitSystemEnvInjectionAudit_NilInjection(t *testing.T) { + records, _ := capturedAudit(t, func() { + emitSystemEnvInjectionAudit("git", "git", "u1", nil, nil) + }) + if len(records) != 0 { + t.Fatalf("expected 0 records for nil injection, got %d", len(records)) + } +} + +// 4. Nil host scope (passthrough adapter): hash = "none", not crashy. +func TestEmitSystemEnvInjectionAudit_NilHostScope(t *testing.T) { + inj := &Injection{Env: map[string]string{"FOO": "bar"}} + records, _ := capturedAudit(t, func() { + emitSystemEnvInjectionAudit("passthrough", "gh", "u1", inj, nil) + }) + if len(records) != 1 { + t.Fatalf("expected 1 record, got %d", len(records)) + } + if records[0]["host_scope_hash"] != "none" { + t.Errorf("host_scope_hash=%v, want none", records[0]["host_scope_hash"]) + } +} diff --git a/internal/tools/credential_context.go b/internal/tools/credential_context.go index 0d5d3b48..a24465bf 100644 --- a/internal/tools/credential_context.go +++ b/internal/tools/credential_context.go @@ -31,6 +31,7 @@ func GenerateCredentialContext(creds []store.SecureCLIBinary) string { b.WriteString("- Parse JSON output directly — do NOT pipe to jq\n\n") b.WriteString("### Available CLIs:\n\n") + hasGit := false for _, c := range creds { b.WriteString(fmt.Sprintf("**%s** — %s\n", c.BinaryName, c.Description)) if blocked := summarizeDenyPatterns(c.DenyArgs); blocked != "" { @@ -40,6 +41,22 @@ func GenerateCredentialContext(creds []store.SecureCLIBinary) string { b.WriteString(fmt.Sprintf(" Tip: %s\n", c.Tips)) } b.WriteString("\n") + if c.AdapterName != nil && *c.AdapterName == "git" { + hasGit = true + } + } + + if hasGit { + // Git adapter has fixed, predictable semantics worth surfacing to the + // LLM up front. Keeps the agent from attempting workarounds (writing + // to ~/.gitconfig, exporting GIT_USERNAME, etc.) when a subcommand is + // outside the auto-auth set. + b.WriteString("### git (adapter-managed):\n") + b.WriteString("- Auto-authenticated subcommands: `clone`, `fetch`, `pull`, `push`, `submodule`.\n") + b.WriteString("- Other subcommands (status, log, diff, commit, branch, …) run WITHOUT credentials — safe for read-only repo work.\n") + b.WriteString("- `git config --global` is denied by policy.\n") + b.WriteString("- Auth is host-scoped: a credential for `github.com` will NOT authenticate to `gitlab.com`.\n") + b.WriteString("- Do NOT attempt to print, copy, or modify the credential — it is injected per-process only.\n\n") } b.WriteString("### When a credentialed CLI command is blocked:\n") diff --git a/internal/tools/credential_context_test.go b/internal/tools/credential_context_test.go index 4f52503d..24fcad24 100644 --- a/internal/tools/credential_context_test.go +++ b/internal/tools/credential_context_test.go @@ -38,3 +38,43 @@ func TestGenerateCredentialContext_BlockedSectionScopedToMarker(t *testing.T) { t.Errorf("output still contains unqualified wording %q.\nOutput:\n%s", dontWant, out) } } + +// 14. Phase 5: when the git adapter is enabled on at least one preset, the +// generated TOOLS.md supplement includes the git-adapter usage block so the +// LLM knows which subcommands auto-authenticate and which run un-credentialed. +func TestGenerateCredentialContext_IncludesGit(t *testing.T) { + git := "git" + creds := []store.SecureCLIBinary{{ + BinaryName: "git", + Description: "git VCS", + AdapterName: &git, + }} + + out := GenerateCredentialContext(creds) + + wantContains := []string{ + "### git (adapter-managed):", + "Auto-authenticated subcommands", + "`clone`, `fetch`, `pull`, `push`, `submodule`", + "host-scoped", + "git config --global", + } + for _, s := range wantContains { + if !strings.Contains(out, s) { + t.Errorf("expected output to contain %q, but it did not.\nOutput:\n%s", s, out) + } + } +} + +// When NO preset has adapter_name=="git", the git block must NOT appear — +// keeps TOOLS.md context minimal when only passthrough binaries are wired. +func TestGenerateCredentialContext_OmitsGitWhenAbsent(t *testing.T) { + creds := []store.SecureCLIBinary{{ + BinaryName: "gh", + Description: "GitHub CLI", + }} + out := GenerateCredentialContext(creds) + if strings.Contains(out, "### git (adapter-managed):") { + t.Errorf("git block leaked into non-git context.\nOutput:\n%s", out) + } +} diff --git a/internal/tools/credential_ephemeral.go b/internal/tools/credential_ephemeral.go new file mode 100644 index 00000000..35a24e27 --- /dev/null +++ b/internal/tools/credential_ephemeral.go @@ -0,0 +1,64 @@ +package tools + +import ( + "context" + "fmt" + "os" + "sync/atomic" +) + +// materializeEphemeral writes content to a per-call 0600 tmpfile and returns a +// cleanup closure that removes it idempotently. The path is safe to pass via +// env vars (PGPASSFILE, KUBECONFIG, GIT_SSH_COMMAND's -i flag) but NOT via +// argv — argv is world-readable on Linux via /proc//cmdline. +// +// Memfd is intentionally NOT used. /proc/self/fd/N resolves "self" against the +// calling process; for child processes (and grandchildren like git→ssh) the +// kernel resolves against the child, which sees EBADF unless the parent passed +// the fd via ExecCommand.ExtraFiles AND the child binary actually reads from +// that fd number. git/psql/docker/kubectl have no API to forward fds to their +// subprocesses, so tmpfile + defer remove is the safe, portable default. +// +// Callers MUST `defer cleanup()` — the helper alone is not enough; if the +// caller goroutine panics without firing the defer, the tmpfile leaks until +// reboot (or whatever cleans os.TempDir on the host). +// +// On Windows the explicit Chmod is a no-op (POSIX-only modes). ACL hardening +// is deferred to v2 with a documented limitation. +func materializeEphemeral(_ context.Context, content []byte, prefix string) (string, func() error, error) { + f, err := os.CreateTemp("", "goclaw-"+prefix+"-*") + if err != nil { + return "", nil, fmt.Errorf("create ephemeral file: %w", err) + } + name := f.Name() + + // Idempotent cleanup latch — concurrent callers don't double-remove. + var done atomic.Bool + cleanup := func() error { + if done.Swap(true) { + return nil + } + if err := os.Remove(name); err != nil && !os.IsNotExist(err) { + return err + } + return nil + } + + // Re-chmod 0600 even though CreateTemp already does this on POSIX — + // explicit + future-proofs if the runtime ever changes the default. + if err := f.Chmod(0o600); err != nil { + _ = f.Close() + _ = cleanup() + return "", nil, fmt.Errorf("chmod ephemeral file: %w", err) + } + if _, err := f.Write(content); err != nil { + _ = f.Close() + _ = cleanup() + return "", nil, fmt.Errorf("write ephemeral file: %w", err) + } + if err := f.Close(); err != nil { + _ = cleanup() + return "", nil, fmt.Errorf("close ephemeral file: %w", err) + } + return name, cleanup, nil +} diff --git a/internal/tools/credential_ephemeral_test.go b/internal/tools/credential_ephemeral_test.go new file mode 100644 index 00000000..74367cf5 --- /dev/null +++ b/internal/tools/credential_ephemeral_test.go @@ -0,0 +1,107 @@ +package tools + +import ( + "context" + "errors" + "io/fs" + "os" + "path/filepath" + "runtime" + "sync" + "sync/atomic" + "testing" +) + +func TestMaterializeEphemeral_WritesAndCleansUp(t *testing.T) { + want := []byte("ephemeral-payload\n") + path, cleanup, err := materializeEphemeral(context.Background(), want, "test") + if err != nil { + t.Fatalf("materializeEphemeral: %v", err) + } + + // Path must live under os.TempDir() (per-user on POSIX, %TEMP% on Windows). + if dir := filepath.Dir(path); dir != filepath.Clean(os.TempDir()) { + t.Fatalf("ephemeral parent dir=%q, want %q", dir, filepath.Clean(os.TempDir())) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != string(want) { + t.Fatalf("content=%q, want %q", got, want) + } + + // POSIX-only: mode 0600. On Windows the mode bits do not reflect ACLs; + // the explicit Chmod is a documented no-op. + if runtime.GOOS != "windows" { + info, err := os.Stat(path) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Fatalf("file mode=%o, want 0600", perm) + } + } + + if err := cleanup(); err != nil { + t.Fatalf("cleanup: %v", err) + } + if _, err := os.Stat(path); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("file still exists after cleanup: stat err=%v", err) + } + + // Second cleanup is a no-op, not an error. + if err := cleanup(); err != nil { + t.Fatalf("idempotent cleanup returned err: %v", err) + } +} + +func TestMaterializeEphemeral_ConcurrentCleanup(t *testing.T) { + path, cleanup, err := materializeEphemeral(context.Background(), []byte("x"), "concurrent") + if err != nil { + t.Fatalf("materializeEphemeral: %v", err) + } + + var nilCount, errCount atomic.Int32 + var wg sync.WaitGroup + for i := 0; i < 64; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if err := cleanup(); err == nil { + nilCount.Add(1) + } else { + errCount.Add(1) + } + }() + } + wg.Wait() + + // All callers see nil (idempotent latch swallows redundant deletes). + if got := nilCount.Load(); got != 64 { + t.Fatalf("concurrent cleanup nil count=%d, want 64", got) + } + if got := errCount.Load(); got != 0 { + t.Fatalf("concurrent cleanup err count=%d, want 0", got) + } + if _, err := os.Stat(path); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("file still exists after concurrent cleanup") + } +} + +func TestMaterializeEphemeral_ZeroContentOK(t *testing.T) { + path, cleanup, err := materializeEphemeral(context.Background(), nil, "empty") + if err != nil { + t.Fatalf("materializeEphemeral: %v", err) + } + t.Cleanup(func() { _ = cleanup() }) + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if info.Size() != 0 { + t.Fatalf("size=%d, want 0", info.Size()) + } +} diff --git a/internal/tools/credential_presets.go b/internal/tools/credential_presets.go index 82df3eaa..221ee113 100644 --- a/internal/tools/credential_presets.go +++ b/internal/tools/credential_presets.go @@ -13,6 +13,12 @@ type CLIPreset struct { DenyVerbose []string `json:"deny_verbose"` Timeout int `json:"timeout"` Tips string `json:"tips"` + // AdapterName defaults the binary row's adapter_name column at create + // time. Empty (default) → passthrough adapter (legacy env injection). + // Set to e.g. "git" by typed-credential presets (Phase 3+). Runtime + // adapter lookup reads the DB column, not this field — so operator + // overrides post-create take precedence. + AdapterName string `json:"adapter_name,omitempty"` } // EnvVarDef describes an environment variable required by a CLI tool. @@ -94,6 +100,45 @@ var CLIPresets = map[string]CLIPreset{ Timeout: 300, Tips: "Use -json flag for structured output", }, + "git": { + BinaryName: "git", + Description: "Git with credential adapter (PAT or SSH host-scoped credentials managed by goclaw)", + // Credential storage is adapter-managed (encrypted_env carries the + // typed blob), not env-paste. Keep EnvVars empty so the UI doesn't + // offer a free-text PAT field that would land in plain env. + EnvVars: nil, + // Deny patterns block the agent from: + // - persisting tokens via `git config --global/--system` + // - installing a leaking credential helper + // - starting an unauthenticated git daemon + // - overriding the adapter's host-scoped `http.*` header via `-c` + // - shadowing core.sshCommand to bypass the adapter's SSH wrapper + // Patterns are case-insensitive because git config keys themselves are. + DenyArgs: []string{ + `(?i)config\s+(--global|--system)`, + `(?i)credential-helper`, + `(?i)\bdaemon\b`, + `(?i)-c\s+http\.`, + `(?i)-c\s+credential\.`, + `(?i)-c\s+core\.sshcommand`, + }, + DenyVerbose: nil, + Timeout: 300, + Tips: "Adapter handles auth automatically for clone/fetch/pull/push/submodule based on stored credential type and host scope.", + AdapterName: "git", + }, + "psql": { + BinaryName: "psql", + Description: "PostgreSQL CLI — framework-validation preset for the typed-credential adapter (Phase 2b). UI cred-type picker lands in v2; until then operators wire `pg_password_file` credentials via API.", + EnvVars: []EnvVarDef{ + {Name: "PGPASSFILE", Desc: "Path to .pgpass file (auto-materialized by adapter when credential_type='pg_password_file')", IsFile: true, Optional: true}, + }, + DenyArgs: []string{`-c\s+["']?(DROP|TRUNCATE)\b`, `\\!`, `\\copy\s+.*FROM\s+PROGRAM`}, + DenyVerbose: nil, + Timeout: 60, + Tips: "Use -A -t for plain output suitable for piping", + AdapterName: "psql", + }, } // GetPreset returns a preset by name, or nil if not found. diff --git a/internal/tools/credentialed_exec.go b/internal/tools/credentialed_exec.go index 7a6a0435..187b716c 100644 --- a/internal/tools/credentialed_exec.go +++ b/internal/tools/credentialed_exec.go @@ -357,6 +357,10 @@ func matchesBinaryVerbose(args []string, denyPatternsJSON json.RawMessage) strin func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCLIBinary, binary string, args []string, cwd string, sandboxKey string, rawCommand string) *Result { + // Attach a per-request scrub bag so adapter-derived secrets stay isolated + // from other tenants/goroutines (see scrub.go WithScrubBag). + ctx = WithScrubBag(ctx) + // Step 0: Reject NUL bytes (defense-in-depth — also checked in Execute()). if strings.ContainsRune(rawCommand, '\x00') { return ErrorResult("command contains invalid NUL byte") @@ -423,13 +427,88 @@ func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCL timeout = 30 * time.Second } + // Step 6b: Resolve adapter from DB row (source of truth, NOT CLIPresets map). + // Passthrough is the default and a no-op — preserves bit-for-bit behavior + // for every legacy preset. + adapterName := "" + if cred.AdapterName != nil { + adapterName = *cred.AdapterName + } + adapter := AdapterFor(adapterName) + + // Sandbox is incompatible with non-passthrough adapters in v1 — they need + // to materialize ephemeral files (SSH key, PAT helper) on the host fs that + // the sandboxed process can't read. Reject early before any ephemerals + // would be created. + inSandbox := t.sandboxMgr != nil && sandboxKey != "" + if inSandbox && adapter.Name() != "passthrough" { + return ErrorResult(fmt.Sprintf("credentialed exec: %q adapter not supported in sandbox mode yet", adapter.Name())) + } + + if adapter.ShouldInject(args) { + userCred := userCredFromBinary(ctx, cred) + // Plant the resolved exec cwd so adapters (e.g. git) can run any + // pre-flight sub-exec from the right repo, not goclaw's daemon CWD. + prepareCtx := WithExecCwd(ctx, cwd) + inj, err := adapter.Prepare(prepareCtx, cred, userCred, args) + if err != nil { + return ErrorResult(ScrubCredentialsCtx(ctx, fmt.Sprintf("credentialed exec: %s adapter prepare failed: %v", adapter.Name(), err))) + } + if inj != nil { + if inj.Cleanup != nil { + defer func() { + if cerr := inj.Cleanup(); cerr != nil { + // Scrub cerr — os.Remove errors embed the full tmpfile + // path, which is in inj.ScrubValues precisely because + // it's adapter-sensitive (e.g. SSH key keypath). + slog.Warn("security.adapter_cleanup_failed", + "adapter", adapter.Name(), + "binary", binary, + "error", ScrubCredentialsCtx(ctx, cerr.Error()), + ) + } + }() + } + if len(inj.ArgvPrefix) > 0 { + args = append(append([]string{}, inj.ArgvPrefix...), args...) + } + for k, v := range inj.Env { + envMap[k] = v + } + if len(inj.ScrubValues) > 0 { + AddScrubValuesCtx(ctx, inj.ScrubValues...) + } + emitSystemEnvInjectionAudit(adapter.Name(), binary, + store.CredentialUserIDFromContext(ctx), inj, cred.UserHostScope) + } + } + // Step 7: Execute — sandbox or host - if t.sandboxMgr != nil && sandboxKey != "" { + if inSandbox { return t.executeCredentialedSandbox(ctx, absPath, args, cwd, sandboxKey, envMap, timeout) } return t.executeCredentialedHost(ctx, absPath, args, cwd, envMap, timeout) } +// userCredFromBinary synthesizes a *SecureCLIUserCredential from the fields +// LookupByBinary's LEFT JOIN populated on the binary row. Returns nil when +// no user credential exists (UserEnv empty + no typed metadata). +func userCredFromBinary(ctx context.Context, bin *store.SecureCLIBinary) *store.SecureCLIUserCredential { + if bin == nil { + return nil + } + if len(bin.UserEnv) == 0 && bin.UserCredentialType == nil && bin.UserHostScope == nil { + return nil + } + return &store.SecureCLIUserCredential{ + BinaryID: bin.ID, + UserID: store.CredentialUserIDFromContext(ctx), + EncryptedEnv: bin.UserEnv, + CredentialType: bin.UserCredentialType, + HostScope: bin.UserHostScope, + } +} + func mergeCredentialedEnv(cred *store.SecureCLIBinary) (map[string]string, error) { envMap := make(map[string]string) if cred == nil { @@ -566,13 +645,13 @@ func (t *ExecTool) executeCredentialedSandbox(ctx context.Context, absPath strin output += "STDERR:\n" + result.Stderr } if result.ExitCode != 0 { - scrubbed := ScrubCredentials(output) + scrubbed := ScrubCredentialsCtx(ctx, output) return credentialedExecFailError(absPath, args, result.ExitCode, scrubbed+MaybeSandboxHint(result.ExitCode, scrubbed)) } if output == "" { output = "(command completed with no output)" } - output = ScrubCredentials(output) + output = ScrubCredentialsCtx(ctx, output) output = capExecOutput(output, execMaxOutputChars) return SilentResult(output) } @@ -644,13 +723,13 @@ func formatCredentialedResult(binary string, args []string, if exitErr, ok := err.(*exec.ExitError); ok { exitCode = exitErr.ExitCode() } - return credentialedExecFailError(binary, args, exitCode, ScrubCredentials(output)) + return credentialedExecFailError(binary, args, exitCode, ScrubCredentialsCtx(ctx, output)) } if output == "" { output = "(command completed with no output)" } - output = ScrubCredentials(output) + output = ScrubCredentialsCtx(ctx, output) output = capExecOutput(output, execMaxOutputChars) return SilentResult(output) } diff --git a/internal/tools/credentialed_exec_scrub_test.go b/internal/tools/credentialed_exec_scrub_test.go new file mode 100644 index 00000000..fc632c79 --- /dev/null +++ b/internal/tools/credentialed_exec_scrub_test.go @@ -0,0 +1,126 @@ +// Regression tests pinning the success/failure-path output scrubber to the +// per-request bag (AddScrubValuesCtx), not the package-global slice. +// +// Why: adapter ScrubValues are registered into the per-request bag during +// Prepare. If formatCredentialedResult / executeCredentialedSandbox call the +// non-Ctx ScrubCredentials, the bag is ignored — so non-GitHub PATs (GitLab +// `glpat-…`, Bitbucket app passwords, Gitea tokens, Azure DevOps PATs) and +// SSH key tmpfile paths would leak into stdout/stderr returned to the agent. +// Locks AC6 against future regressions. +package tools + +import ( + "context" + "os/exec" + "strings" + "testing" + "time" +) + +// 1. Success path: adapter ScrubValues registered via bag are stripped from +// stdout in the final *Result.ForLLM. Uses a sentinel that does NOT match +// any built-in credentialPatterns regex (no `ghp_`, no `aws_`, no +// `://user:pass@`) so the bag is the only thing that could redact it. +func TestFormatCredentialedResult_HonorsScrubBag_Success(t *testing.T) { + ctx := WithScrubBag(context.Background()) + secret := "glpat-NONSENSE_GITLAB_PAT_VALUE_XYZ123" // not matched by global regex + AddScrubValuesCtx(ctx, secret) + + // Avoid any keyword-style prefix (`token=`, `password:`, `authorization=`, …) + // so the global credentialPatterns regex doesn't redact this — only the + // per-request bag can. That's the whole point of this regression test. + stdout := "cloning into 'repo'…\nremote: rejected by " + secret + " for user\ndone.\n" + res := formatCredentialedResult("/usr/bin/git", []string{"clone", "x"}, stdout, "", nil, ctx, time.Minute) + + if res == nil { + t.Fatal("nil result") + } + if strings.Contains(res.ForLLM, secret) { + t.Fatalf("non-GitHub PAT leaked into ForLLM: %q", res.ForLLM) + } + if !strings.Contains(res.ForLLM, "[REDACTED]") { + t.Fatalf("expected [REDACTED] placeholder in scrubbed output, got %q", res.ForLLM) + } +} + +// 2. Failure path: same guarantee on the error branch (exit != 0). This is +// the more dangerous case in practice — error messages often quote the +// failing URL/header that contains the token. +func TestFormatCredentialedResult_HonorsScrubBag_Failure(t *testing.T) { + ctx := WithScrubBag(context.Background()) + keyPath := "/tmp/goclaw-gitkey-SENTINEL_PATH_VALUE_xyz789" + AddScrubValuesCtx(ctx, keyPath) + + stderr := "Load key \"" + keyPath + "\": Permission denied\nfatal: Could not read from remote\n" + // Synthesize a real *exec.ExitError so the type-assertion branch fires. + cmd := exec.Command("sh", "-c", "exit 128") + _ = cmd.Run() + exitErr := &exec.ExitError{ProcessState: cmd.ProcessState} + res := formatCredentialedResult("/usr/bin/git", []string{"clone", "x"}, "", stderr, exitErr, ctx, time.Minute) + + if res == nil { + t.Fatal("nil result") + } + if strings.Contains(res.ForLLM, keyPath) { + t.Fatalf("SSH key tmpfile path leaked into ForLLM error: %q", res.ForLLM) + } + if strings.Contains(res.ForUser, keyPath) { + t.Fatalf("SSH key tmpfile path leaked into ForUser error: %q", res.ForUser) + } +} + +// 3. Negative control: without a bag in ctx, the same secret leaks (proves +// the test is actually exercising the bag path, not getting silently +// scrubbed by a global match). +func TestFormatCredentialedResult_NoBag_LeaksNonGlobalSecret(t *testing.T) { + ctx := context.Background() // no WithScrubBag + secret := "glpat-NONSENSE_GITLAB_PAT_VALUE_CONTROL" + + // Same evasion as test 1 — no keyword-style prefix, so a global regex + // match would have to come from the secret value itself (which it doesn't). + stdout := "remote: rejected by " + secret + " for user\n" + res := formatCredentialedResult("/usr/bin/git", []string{"clone", "x"}, stdout, "", nil, ctx, time.Minute) + + if !strings.Contains(res.ForLLM, secret) { + t.Fatalf("control failed — secret was scrubbed without a bag, so test 1's coverage claim is wrong. ForLLM=%q", res.ForLLM) + } +} + +// 4. Sanity: empty ctx + empty bag must not panic and must return the regex +// pass intact (well-known ghp_… still gets redacted). +func TestFormatCredentialedResult_EmptyBag_StillRunsGlobalRegex(t *testing.T) { + ctx := WithScrubBag(context.Background()) + // classic GitHub PAT shape — matched by the package's regex pass + classicPAT := "ghp_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" + stdout := "token=" + classicPAT + "\n" + res := formatCredentialedResult("/usr/bin/git", nil, stdout, "", nil, ctx, time.Minute) + + if strings.Contains(res.ForLLM, classicPAT) { + t.Fatalf("classic PAT not redacted by regex pass: %q", res.ForLLM) + } +} + +// 5. Defensive: confirm the timeout path doesn't surface stderr at all +// (it returns the timeout marker, not the captured output), so no leak vector. +func TestFormatCredentialedResult_TimeoutPath_NoStderrLeak(t *testing.T) { + ctx, cancel := context.WithTimeout(WithScrubBag(context.Background()), 1*time.Nanosecond) + defer cancel() + time.Sleep(1 * time.Millisecond) // make sure deadline elapsed + + stderr := "remote auth token=ghp_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\n" + res := formatCredentialedResult("/usr/bin/git", nil, "", stderr, + errExitForTimeoutTest{}, ctx, 1*time.Nanosecond) + + // On timeout the formatter returns a fixed string mentioning the binary, + // not the captured stderr — so no token in output regardless. + if strings.Contains(res.ForLLM, "ghp_") { + t.Fatalf("timeout path leaked captured stderr: %q", res.ForLLM) + } +} + +// errExitForTimeoutTest is a marker error for the timeout-path test. The real +// formatCredentialedResult checks ctx.Err() before unwrapping, so any +// non-ExitError suffices. +type errExitForTimeoutTest struct{} + +func (errExitForTimeoutTest) Error() string { return "context deadline exceeded" } diff --git a/internal/tools/scrub.go b/internal/tools/scrub.go index 6fc52819..4052739f 100644 --- a/internal/tools/scrub.go +++ b/internal/tools/scrub.go @@ -1,6 +1,7 @@ package tools import ( + "context" "regexp" "strings" "sync" @@ -112,6 +113,90 @@ func AddCredentialScrubValues(values ...string) { } } +// --- Per-request scrub bag (multi-tenant safe) --- +// +// The package-global credentialScrubValues above accumulates across all +// goroutines for the life of the process. For the adapter pipeline (Phase 2+) +// each exec gets its own scrub bag via context so tenant A's credentials never +// reach tenant B's output, even when adapters run concurrently. + +type scrubBag struct { + mu sync.RWMutex + values []string +} + +type scrubBagKey struct{} + +type execCwdKey struct{} + +// WithExecCwd returns a context carrying the working directory the +// credentialed exec will use. Adapters consult this so any pre-flight +// sub-exec (e.g. `git config --get remote.origin.url`) runs inside the +// caller's repo, not goclaw's daemon CWD. +func WithExecCwd(ctx context.Context, cwd string) context.Context { + if cwd == "" { + return ctx + } + return context.WithValue(ctx, execCwdKey{}, cwd) +} + +// ExecCwdFromContext returns the cwd planted by WithExecCwd, or empty. +func ExecCwdFromContext(ctx context.Context) string { + if s, ok := ctx.Value(execCwdKey{}).(string); ok { + return s + } + return "" +} + +// WithScrubBag returns a context carrying a fresh per-request scrub list. +// Call at the top of executeCredentialed; pass the returned ctx downstream. +func WithScrubBag(ctx context.Context) context.Context { + return context.WithValue(ctx, scrubBagKey{}, &scrubBag{}) +} + +// AddScrubValuesCtx appends adapter-derived secrets to the per-request bag. +// No-op when ctx has no bag (legacy callers). Skips values shorter than 6 +// chars to avoid false-positive matches like "x" or "1". +func AddScrubValuesCtx(ctx context.Context, vs ...string) { + bag, ok := ctx.Value(scrubBagKey{}).(*scrubBag) + if !ok { + return + } + bag.mu.Lock() + defer bag.mu.Unlock() + for _, v := range vs { + if len(v) >= 6 { + bag.values = append(bag.values, v) + } + } +} + +// ScrubCredentialsCtx runs the same regex pass as ScrubCredentials AND any +// per-request bag values, but does NOT consult the package-global slice. +// Use this in the adapter pipeline so one tenant's secrets cannot leak into +// another tenant's output through the shared global. +func ScrubCredentialsCtx(ctx context.Context, text string) string { + for _, pat := range credentialPatterns { + text = pat.ReplaceAllString(text, redactedPlaceholder) + } + if bag, ok := ctx.Value(scrubBagKey{}).(*scrubBag); ok { + bag.mu.RLock() + vals := append([]string(nil), bag.values...) + bag.mu.RUnlock() + for _, v := range vals { + text = strings.ReplaceAll(text, v, redactedPlaceholder) + } + } + // Dynamic server-IP values still apply — they're infra metadata, not creds. + dynamicScrubMu.RLock() + dyn := dynamicScrubValues + dynamicScrubMu.RUnlock() + for _, v := range dyn { + text = strings.ReplaceAll(text, v, serverIPPlaceholder) + } + return text +} + // ScrubCredentials replaces known credential patterns and dynamic values in text. func ScrubCredentials(text string) string { for _, pat := range credentialPatterns { diff --git a/internal/tools/shell_credentialed_gate_test.go b/internal/tools/shell_credentialed_gate_test.go index c35841d5..153be1f8 100644 --- a/internal/tools/shell_credentialed_gate_test.go +++ b/internal/tools/shell_credentialed_gate_test.go @@ -104,6 +104,9 @@ func (s *stubSecureCLIStore) GetUserCredentials(ctx context.Context, binaryID uu func (s *stubSecureCLIStore) SetUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte) error { return nil } +func (s *stubSecureCLIStore) SetUserCredentialsTyped(ctx context.Context, binaryID uuid.UUID, userID string, encryptedEnv []byte, credentialType, hostScope *string) error { + return nil +} func (s *stubSecureCLIStore) DeleteUserCredentials(ctx context.Context, binaryID uuid.UUID, userID string) error { return nil } diff --git a/internal/upgrade/version.go b/internal/upgrade/version.go index 10ad72c6..0033f111 100644 --- a/internal/upgrade/version.go +++ b/internal/upgrade/version.go @@ -2,4 +2,4 @@ package upgrade // RequiredSchemaVersion is the schema migration version this binary requires. // Bump this whenever adding a new SQL migration file. -const RequiredSchemaVersion uint = 72 +const RequiredSchemaVersion uint = 73 diff --git a/migrations/000073_secure_cli_credential_type.down.sql b/migrations/000073_secure_cli_credential_type.down.sql new file mode 100644 index 00000000..9ac1f6ed --- /dev/null +++ b/migrations/000073_secure_cli_credential_type.down.sql @@ -0,0 +1,6 @@ +ALTER TABLE secure_cli_user_credentials + DROP COLUMN IF EXISTS credential_type, + DROP COLUMN IF EXISTS host_scope; + +ALTER TABLE secure_cli_binaries + DROP COLUMN IF EXISTS adapter_name; diff --git a/migrations/000073_secure_cli_credential_type.up.sql b/migrations/000073_secure_cli_credential_type.up.sql new file mode 100644 index 00000000..0ddb49df --- /dev/null +++ b/migrations/000073_secure_cli_credential_type.up.sql @@ -0,0 +1,12 @@ +-- Credential adapter framework: storage substrate for typed credentials. +-- credential_type carries the cred shape ('env' | 'pat' | 'ssh_key' | future). +-- host_scope binds credentials to a specific hostname (e.g. 'github.com'). +-- adapter_name routes the binary to the right CredentialAdapter at exec time. +-- All three columns NULL-by-default to preserve legacy passthrough behavior. + +ALTER TABLE secure_cli_user_credentials + ADD COLUMN IF NOT EXISTS credential_type TEXT NULL, + ADD COLUMN IF NOT EXISTS host_scope TEXT NULL; + +ALTER TABLE secure_cli_binaries + ADD COLUMN IF NOT EXISTS adapter_name TEXT NULL; diff --git a/tests/integration/git_adapter_pat_test.go b/tests/integration/git_adapter_pat_test.go new file mode 100644 index 00000000..92fc3a98 --- /dev/null +++ b/tests/integration/git_adapter_pat_test.go @@ -0,0 +1,217 @@ +//go:build integration + +package integration + +// Phase 3 (issue #82) — end-to-end PAT injection via the git adapter. +// +// What we prove here that unit tests cannot: +// - A real `git clone` over HTTP succeeds when the adapter's Injection env +// is passed through, against a server that requires the bearer token. +// - The token never lands in `.git/config`, the remote URL, or any +// credential helper after clone — i.e. the GIT_CONFIG_COUNT shape leaves +// no on-disk trace. +// - Submodule resolve honors the host scope: parent.origin = trusted host +// resolves the same host for the submodule clone; cross-host stops +// with `errCredentialHostMismatch` before any network call. +// +// These map to acceptance criteria AC1 + AC4 from the plan. + +import ( + "context" + "errors" + "net/url" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +const testPAT = "ghp_TESTtokenABCDEFGHIJKLMNOPQRSTUV1234" + +func patCred(t *testing.T, host string) *store.SecureCLIUserCredential { + t.Helper() + ct := "pat" + hs := host + return &store.SecureCLIUserCredential{ + CredentialType: &ct, + HostScope: &hs, + EncryptedEnv: []byte(`{"token":"` + testPAT + `"}`), + } +} + +// applyInjection turns an Injection into an env slice merged onto os.Environ, +// mirroring what credentialed_exec.go does at run time. GIT_SSL_NO_VERIFY is +// added because httptest.NewTLSServer uses a self-signed cert; production +// callers will hit real CAs and never need this. +func applyInjection(t *testing.T, inj *tools.Injection) []string { + t.Helper() + env := append(os.Environ(), "GIT_SSL_NO_VERIFY=true") + for k, v := range inj.Env { + env = append(env, k+"="+v) + } + return env +} + +// TestGitAdapter_PAT_NoLeakInClonedRepo (AC1): real clone works through the +// adapter, and the resulting .git/config carries the remote URL only — no +// Authorization header, no embedded token, no credential helper entry. +func TestGitAdapter_PAT_NoLeakInClonedRepo(t *testing.T) { + t.Parallel() + + repoRoot := t.TempDir() + makeBareRepo(t, repoRoot, "demo") + + srv := startGitHTTPServer(t, repoRoot, testPAT) + u, _ := url.Parse(srv.URL) + host := u.Host // "127.0.0.1:NNNN" — preserved with port by normalizeHost + + cred := patCred(t, host) + cloneURL := srv.URL + "/demo.git" + dest := filepath.Join(t.TempDir(), "checkout") + + adapter := tools.AdapterFor("git") + argv := []string{"clone", cloneURL, dest} + inj, err := adapter.Prepare(context.Background(), nil, cred, argv) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + if len(inj.ArgvPrefix) != 0 { + t.Fatalf("token leaked to argv prefix: %v", inj.ArgvPrefix) + } + if _, ok := inj.Env["GIT_CONFIG_VALUE_0"]; !ok { + t.Fatalf("missing GIT_CONFIG_VALUE_0 in injection env") + } + + cmd := exec.Command("git", argv...) + cmd.Env = applyInjection(t, inj) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("clone failed: %v\n%s", err, out) + } + + cfg, err := os.ReadFile(filepath.Join(dest, ".git", "config")) + if err != nil { + t.Fatalf("read .git/config: %v", err) + } + cfgStr := string(cfg) + if strings.Contains(cfgStr, testPAT) { + t.Fatalf("PAT leaked into .git/config:\n%s", cfgStr) + } + if strings.Contains(strings.ToLower(cfgStr), "authorization") { + t.Fatalf("auth header leaked into .git/config:\n%s", cfgStr) + } + if strings.Contains(strings.ToLower(cfgStr), "credential.helper") { + t.Fatalf("credential helper leaked into .git/config:\n%s", cfgStr) + } + + // Verify the remote URL is the clean URL, no embedded userinfo. + remoteURL, err := exec.Command("git", "-C", dest, "config", "--get", "remote.origin.url").Output() + if err != nil { + t.Fatalf("read remote url: %v", err) + } + if got := strings.TrimSpace(string(remoteURL)); got != cloneURL { + t.Fatalf("remote URL rewritten with credentials\nwant: %s\ngot: %s", cloneURL, got) + } +} + +// TestGitAdapter_PAT_SubmoduleSameHost (AC4 happy): a fetch inside an existing +// checkout whose origin matches the cred host resolves without error. +func TestGitAdapter_PAT_SubmoduleSameHost(t *testing.T) { + t.Parallel() + + repoRoot := t.TempDir() + makeBareRepo(t, repoRoot, "parent") + + srv := startGitHTTPServer(t, repoRoot, testPAT) + u, _ := url.Parse(srv.URL) + host := u.Host + + dest := filepath.Join(t.TempDir(), "parent-checkout") + cred := patCred(t, host) + cloneURL := srv.URL + "/parent.git" + + adapter := tools.AdapterFor("git") + + // 1. Clone to set up origin. + cloneArgv := []string{"clone", cloneURL, dest} + inj, err := adapter.Prepare(context.Background(), nil, cred, cloneArgv) + if err != nil { + t.Fatalf("Prepare clone: %v", err) + } + cmd := exec.Command("git", cloneArgv...) + cmd.Env = applyInjection(t, inj) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("clone: %v\n%s", err, out) + } + + // 2. Fetch — adapter resolves remote.origin.url via `git config --get` and + // must match the cred host. Production passes argv without `-C`; cwd is + // set on the cmd struct (see credentialed_exec.go). Mirror that. + fetchArgv := []string{"fetch", "origin"} + ctx := tools.WithExecCwd(context.Background(), dest) + inj, err = adapter.Prepare(ctx, nil, cred, fetchArgv) + if err != nil { + t.Fatalf("Prepare fetch on same host: %v", err) + } + cmd = exec.Command("git", fetchArgv...) + cmd.Dir = dest + cmd.Env = applyInjection(t, inj) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("fetch: %v\n%s", err, out) + } +} + +// TestGitAdapter_PAT_SubmoduleCrossHostFails (AC4 negative): cred bound to a +// different host than the parent's origin must fail before any HTTP call. The +// error message must not leak the token. +func TestGitAdapter_PAT_SubmoduleCrossHostFails(t *testing.T) { + t.Parallel() + + repoRoot := t.TempDir() + makeBareRepo(t, repoRoot, "parent") + + srv := startGitHTTPServer(t, repoRoot, testPAT) + u, _ := url.Parse(srv.URL) + actualHost := u.Host + + dest := filepath.Join(t.TempDir(), "parent-checkout") + cloneURL := srv.URL + "/parent.git" + + adapter := tools.AdapterFor("git") + + // Clone with a matching cred first to set up the checkout. + good := patCred(t, actualHost) + cloneArgv := []string{"clone", cloneURL, dest} + inj, err := adapter.Prepare(context.Background(), nil, good, cloneArgv) + if err != nil { + t.Fatalf("Prepare clone: %v", err) + } + cmd := exec.Command("git", cloneArgv...) + cmd.Env = applyInjection(t, inj) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("clone: %v\n%s", err, out) + } + + // Now retry fetch with a cred bound to a different host. + wrong := patCred(t, "evil.example.com") + fetchArgv := []string{"fetch", "origin"} + ctx := tools.WithExecCwd(context.Background(), dest) + if _, err := adapter.Prepare(ctx, nil, wrong, fetchArgv); err == nil { + t.Fatalf("expected host mismatch error, got nil") + } else { + // Adapter exposes errCredentialHostMismatch as unexported; assert by + // shape: error string must name both hosts but never the token. + msg := err.Error() + if strings.Contains(msg, testPAT) { + t.Fatalf("token leaked into mismatch error: %s", msg) + } + if !strings.Contains(msg, "evil.example.com") { + t.Fatalf("error missing cred host: %s", msg) + } + // errors.Is fallback in case the adapter exports a sentinel later. + _ = errors.Is + } +} diff --git a/tests/integration/git_adapter_ssh_test.go b/tests/integration/git_adapter_ssh_test.go new file mode 100644 index 00000000..302d2cf2 --- /dev/null +++ b/tests/integration/git_adapter_ssh_test.go @@ -0,0 +1,187 @@ +//go:build integration + +package integration + +// Phase 4 (issue #82) — SSH path end-to-end lifecycle test. +// +// Why no real sshd: spawning an OpenSSH daemon in CI is heavy (image must +// carry openssh-server) and the lifecycle properties we care about +// (tmpfile 0600, cleanup-on-success, cleanup-on-exec-failure, env shape +// reaching the child) are observable with a fake exec that just reads +// GIT_SSH_COMMAND and reports back. Real sshd is reserved for manual +// security review per Phase 6. +// +// This file proves: +// - The keypath surfaced via Injection.ScrubValues[0] is a real 0600 +// tmpfile on disk while the exec runs. +// - cleanup() removes it. The file is gone after cleanup, regardless of +// exec success/failure. +// - GIT_SSH_COMMAND propagates through the env passed to the child. + +import ( + "context" + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "encoding/json" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "golang.org/x/crypto/ssh" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +func makeTestKeyJSON(t *testing.T) []byte { + t.Helper() + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("gen key: %v", err) + } + block, err := ssh.MarshalPrivateKey(priv, "") + if err != nil { + t.Fatalf("marshal: %v", err) + } + encoded := base64.StdEncoding.EncodeToString(block.Bytes) + pem := "-----BEGIN OPENSSH PRIVATE KEY-----\n" + encoded + "\n-----END OPENSSH PRIVATE KEY-----\n" + blob, _ := json.Marshal(map[string]string{"key": pem}) + return blob +} + +func sshCredFor(t *testing.T, host string) *store.SecureCLIUserCredential { + t.Helper() + ct, hs := "ssh_key", host + return &store.SecureCLIUserCredential{ + CredentialType: &ct, + HostScope: &hs, + EncryptedEnv: makeTestKeyJSON(t), + } +} + +// TestGitAdapter_SSH_TmpfileLifecycle (AC2): keypath exists during exec, is +// 0600, and is removed after cleanup runs. +func TestGitAdapter_SSH_TmpfileLifecycle(t *testing.T) { + t.Parallel() + + cred := sshCredFor(t, "github.com") + adapter := tools.AdapterFor("git") + + inj, err := adapter.Prepare(context.Background(), nil, cred, + []string{"clone", "git@github.com:o/r.git", "/tmp/dst"}) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + defer func() { + if inj.Cleanup != nil { + _ = inj.Cleanup() + } + }() + + if len(inj.ScrubValues) != 1 { + t.Fatalf("want 1 scrub value (keypath), got %v", inj.ScrubValues) + } + keyPath := inj.ScrubValues[0] + + // Exists + 0600 (POSIX only) during the exec window. + st, err := os.Stat(keyPath) + if err != nil { + t.Fatalf("keypath stat: %v", err) + } + if runtime.GOOS != "windows" && st.Mode().Perm() != 0o600 { + t.Fatalf("keypath perms = %o, want 0600", st.Mode().Perm()) + } + + // Simulate the exec: spawn `env` (or `cmd /c set` on Windows) to + // observe GIT_SSH_COMMAND propagation. We don't run real git — the + // adapter's contract is "env reaches child", which is provable without + // involving git's transport layer. + env := append(os.Environ()) + for k, v := range inj.Env { + env = append(env, k+"="+v) + } + var cmd *exec.Cmd + if runtime.GOOS == "windows" { + cmd = exec.Command("cmd", "/c", "set") + } else { + cmd = exec.Command("env") + } + cmd.Env = env + out, err := cmd.Output() + if err != nil { + t.Fatalf("env exec: %v", err) + } + if !strings.Contains(string(out), "GIT_SSH_COMMAND=ssh -i "+keyPath) { + t.Fatalf("GIT_SSH_COMMAND not propagated to child env:\n%s", out) + } + + // Cleanup → file gone. + if err := inj.Cleanup(); err != nil { + t.Fatalf("cleanup: %v", err) + } + if _, err := os.Stat(keyPath); !os.IsNotExist(err) { + t.Fatalf("post-cleanup stat want ErrNotExist, got %v", err) + } +} + +// TestGitAdapter_SSH_CleanupOnExecFailure: cleanup runs even when the +// caller's exec exits non-zero. Mirrors what credentialed_exec.go does via +// `defer inj.Cleanup()`. +func TestGitAdapter_SSH_CleanupOnExecFailure(t *testing.T) { + t.Parallel() + + cred := sshCredFor(t, "github.com") + adapter := tools.AdapterFor("git") + + inj, err := adapter.Prepare(context.Background(), nil, cred, + []string{"clone", "git@github.com:o/r.git", "/tmp/dst"}) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + keyPath := inj.ScrubValues[0] + + // Mimic credentialed_exec.go: defer cleanup, run command, observe exit. + func() { + defer inj.Cleanup() + // Force a non-zero exit. + cmd := exec.Command(falseBin(), "anyarg") + _ = cmd.Run() // err expected, ignored + }() + + if _, err := os.Stat(keyPath); !os.IsNotExist(err) { + t.Fatalf("post-failure cleanup did not remove keypath; stat err = %v", err) + } +} + +// TestGitAdapter_SSH_HostMismatch_NoTmpfile: rejected host mismatch must +// NOT leave an orphaned tmpfile in os.TempDir(). +func TestGitAdapter_SSH_HostMismatch_NoTmpfile(t *testing.T) { + t.Parallel() + + cred := sshCredFor(t, "github.com") + adapter := tools.AdapterFor("git") + + before, _ := filepath.Glob(filepath.Join(os.TempDir(), "goclaw-gitkey-*")) + _, err := adapter.Prepare(context.Background(), nil, cred, + []string{"clone", "git@gitlab.com:o/r.git"}) + if err == nil { + t.Fatal("expected host mismatch error") + } + after, _ := filepath.Glob(filepath.Join(os.TempDir(), "goclaw-gitkey-*")) + if len(after) > len(before) { + t.Fatalf("orphaned tmpfile after rejected Prepare: before=%d after=%d new=%v", + len(before), len(after), after) + } +} + +func falseBin() string { + if runtime.GOOS == "windows" { + // cmd.exe /c exit 1 — but `exit` is a builtin, use a guaranteed-fail call. + return "cmd" + } + return "false" +} diff --git a/tests/integration/githttp_helper_test.go b/tests/integration/githttp_helper_test.go new file mode 100644 index 00000000..0a74378c --- /dev/null +++ b/tests/integration/githttp_helper_test.go @@ -0,0 +1,110 @@ +//go:build integration + +package integration + +// Local git http-backend test server used by Phase 3 PAT integration tests. +// +// Why this exists: +// - The git adapter promises PAT injection via GIT_CONFIG_COUNT/KEY_0/VALUE_0 +// so the token never lands on argv or .git/config. +// - To prove that end-to-end we need a real git server that demands an +// Authorization header and rejects anonymous clones. +// - We wrap `git-http-backend` (ships with git) behind httptest so the test +// is self-contained — no network, no docker. +// +// Auth model: +// - Server checks `Authorization: Bearer ` on every request. +// - Missing/wrong → 401, which makes git fail immediately. +// - Matching → defer to git-http-backend CGI. +// +// macOS note: git-http-backend lives under `git --exec-path`. We probe that +// and skip if not present (some minimal CI images strip it). + +import ( + "net/http" + "net/http/cgi" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// startGitHTTPServer creates an httptest server backed by git-http-backend +// serving repos under projectRoot. Requests must carry the bearer token. +func startGitHTTPServer(t *testing.T, projectRoot, expectedToken string) *httptest.Server { + t.Helper() + + execPath, err := exec.Command("git", "--exec-path").Output() + if err != nil { + t.Skipf("git --exec-path unavailable: %v", err) + } + backend := filepath.Join(strings.TrimSpace(string(execPath)), "git-http-backend") + if _, err := os.Stat(backend); err != nil { + t.Skipf("git-http-backend not installed at %s: %v", backend, err) + } + + handler := &cgi.Handler{ + Path: backend, + Env: []string{ + "GIT_PROJECT_ROOT=" + projectRoot, + "GIT_HTTP_EXPORT_ALL=1", + // http-backend uses REMOTE_USER for ident; harmless placeholder. + "REMOTE_USER=pat-test", + }, + } + + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + got := r.Header.Get("Authorization") + if got != "Bearer "+expectedToken { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + handler.ServeHTTP(w, r) + }) + + // TLS required: the git adapter writes `http.https:///.extraheader` + // which only matches HTTPS URLs. Production PATs must travel over TLS, so + // the test mirrors that constraint. + srv := httptest.NewTLSServer(mux) + t.Cleanup(srv.Close) + return srv +} + +// makeBareRepo seeds projectRoot/.git with a single commit so clone has +// something to fetch. Returns the on-disk path (useful for assertions). +func makeBareRepo(t *testing.T, projectRoot, name string) string { + t.Helper() + + // Working tree to seed history. + work := t.TempDir() + mustRun(t, work, "git", "init", "-q", "-b", "main") + mustRun(t, work, "git", "config", "user.email", "ci@test.local") + mustRun(t, work, "git", "config", "user.name", "CI") + if err := os.WriteFile(filepath.Join(work, "README.md"), []byte("hello\n"), 0o644); err != nil { + t.Fatalf("seed file: %v", err) + } + mustRun(t, work, "git", "add", ".") + mustRun(t, work, "git", "commit", "-q", "-m", "init") + + // Bare repo under project root. + bare := filepath.Join(projectRoot, name+".git") + mustRun(t, "", "git", "clone", "-q", "--bare", work, bare) + // http-backend requires this to serve smart HTTP without auth bypass. + mustRun(t, bare, "git", "config", "http.receivepack", "true") + mustRun(t, bare, "git", "update-server-info") + return bare +} + +func mustRun(t *testing.T, cwd string, name string, args ...string) { + t.Helper() + cmd := exec.Command(name, args...) + if cwd != "" { + cmd.Dir = cwd + } + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("%s %v: %v\n%s", name, args, err, out) + } +} diff --git a/tests/integration/secure_cli_credential_type_pg_test.go b/tests/integration/secure_cli_credential_type_pg_test.go new file mode 100644 index 00000000..a769181a --- /dev/null +++ b/tests/integration/secure_cli_credential_type_pg_test.go @@ -0,0 +1,231 @@ +//go:build integration + +package integration + +// Phase 1 (issue #82): verify the credential-type schema delta on PostgreSQL. +// - adapter_name column on secure_cli_binaries +// - credential_type, host_scope columns on secure_cli_user_credentials +// - SetUserCredentialsTyped writes the new columns +// - LookupByBinary projects AdapterName + UserCredentialType + UserHostScope +// - Legacy paths (no adapter, SetUserCredentials with no type) keep all +// new columns NULL for backward compat +// +// These tests run against a freshly-migrated test DB (migration 73 applied). + +import ( + "context" + "database/sql" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/store/pg" +) + +// pgColumnExists checks information_schema for a column on the given table +// in the public schema. +func pgColumnExists(t *testing.T, db *sql.DB, table, column string) bool { + t.Helper() + var exists bool + err := db.QueryRow( + `SELECT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'public' + AND table_name = $1 + AND column_name = $2 + )`, table, column, + ).Scan(&exists) + if err != nil { + t.Fatalf("information_schema lookup %s.%s: %v", table, column, err) + } + return exists +} + +func TestPG_SchemaHasPhase1Columns(t *testing.T) { + db := testDB(t) + + if !pgColumnExists(t, db, "secure_cli_binaries", "adapter_name") { + t.Fatalf("secure_cli_binaries.adapter_name missing — migration 73 not applied") + } + if !pgColumnExists(t, db, "secure_cli_user_credentials", "credential_type") { + t.Fatalf("secure_cli_user_credentials.credential_type missing") + } + if !pgColumnExists(t, db, "secure_cli_user_credentials", "host_scope") { + t.Fatalf("secure_cli_user_credentials.host_scope missing") + } +} + +func TestPG_CreateBinary_AdapterNameRoundTrip(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + ctx := store.WithTenantID(context.Background(), tenantID) + s := pg.NewPGSecureCLIStore(db, testEncryptionKey) + + adapter := "git" + bin := &store.SecureCLIBinary{ + BinaryName: "git-phase1-test-" + uuid.New().String()[:8], + Description: "Phase 1 adapter binary", + IsGlobal: true, + Enabled: true, + CreatedBy: "u-tester", + AdapterName: &adapter, + EncryptedEnv: []byte(`{}`), + } + if err := s.Create(ctx, bin); err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { + db.Exec("DELETE FROM secure_cli_binaries WHERE id = $1", bin.ID) + }) + + got, err := s.Get(ctx, bin.ID) + if err != nil { + t.Fatalf("Get: %v", err) + } + if got.AdapterName == nil || *got.AdapterName != "git" { + t.Fatalf("expected adapter_name=git, got %v", got.AdapterName) + } +} + +func TestPG_SetUserCredentialsTyped_RoundTrip(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + ctx := store.WithTenantID(context.Background(), tenantID) + s := pg.NewPGSecureCLIStore(db, testEncryptionKey) + + binID := seedGateBinary(t, db, tenantID, "git-typed-cred-"+uuid.New().String()[:8], true) + + credType := "pat" + hostScope := "github.com" + plaintext := []byte(`{"token":"ghp_xxx"}`) + + if err := s.SetUserCredentialsTyped(ctx, binID, "u-1", plaintext, &credType, &hostScope); err != nil { + t.Fatalf("SetUserCredentialsTyped: %v", err) + } + + got, err := s.GetUserCredentials(ctx, binID, "u-1") + if err != nil { + t.Fatalf("GetUserCredentials: %v", err) + } + if got == nil { + t.Fatalf("expected credential, got nil") + } + if got.CredentialType == nil || *got.CredentialType != "pat" { + t.Fatalf("expected credential_type=pat, got %v", got.CredentialType) + } + if got.HostScope == nil || *got.HostScope != "github.com" { + t.Fatalf("expected host_scope=github.com, got %v", got.HostScope) + } + if string(got.EncryptedEnv) != string(plaintext) { + t.Fatalf("decrypted env mismatch: got %q want %q", got.EncryptedEnv, plaintext) + } +} + +// Legacy SetUserCredentials (untyped) must store NULL for credential_type / host_scope. +func TestPG_SetUserCredentials_LegacyLeavesTypeColumnsNull(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + ctx := store.WithTenantID(context.Background(), tenantID) + s := pg.NewPGSecureCLIStore(db, testEncryptionKey) + + binID := seedGateBinary(t, db, tenantID, "git-legacy-cred-"+uuid.New().String()[:8], true) + + if err := s.SetUserCredentials(ctx, binID, "u-legacy", []byte(`{"GITHUB_TOKEN":"x"}`)); err != nil { + t.Fatalf("SetUserCredentials: %v", err) + } + + got, err := s.GetUserCredentials(ctx, binID, "u-legacy") + if err != nil { + t.Fatalf("GetUserCredentials: %v", err) + } + if got == nil { + t.Fatalf("expected credential, got nil") + } + if got.CredentialType != nil { + t.Fatalf("expected credential_type NULL for legacy, got %q", *got.CredentialType) + } + if got.HostScope != nil { + t.Fatalf("expected host_scope NULL for legacy, got %q", *got.HostScope) + } +} + +// LookupByBinary must populate AdapterName + UserCredentialType + UserHostScope +// from the LEFT JOIN onto secure_cli_user_credentials. +func TestPG_LookupByBinary_ProjectsNewColumns(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + ctx := store.WithTenantID(context.Background(), tenantID) + s := pg.NewPGSecureCLIStore(db, testEncryptionKey) + + binName := "git-lookup-proj-" + uuid.New().String()[:8] + adapter := "git" + bin := &store.SecureCLIBinary{ + BinaryName: binName, + Description: "Phase 1 lookup projection", + IsGlobal: true, + Enabled: true, + CreatedBy: "u-tester", + AdapterName: &adapter, + EncryptedEnv: []byte(`{}`), + } + if err := s.Create(ctx, bin); err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { + db.Exec("DELETE FROM secure_cli_user_credentials WHERE binary_id = $1", bin.ID) + db.Exec("DELETE FROM secure_cli_binaries WHERE id = $1", bin.ID) + }) + + credType := "pat" + hostScope := "github.com" + if err := s.SetUserCredentialsTyped(ctx, bin.ID, "u-1", []byte(`{"token":"x"}`), &credType, &hostScope); err != nil { + t.Fatalf("SetUserCredentialsTyped: %v", err) + } + + got, err := s.LookupByBinary(ctx, binName, nil, "u-1") + if err != nil { + t.Fatalf("LookupByBinary: %v", err) + } + if got == nil { + t.Fatalf("expected binary, got nil") + } + if got.AdapterName == nil || *got.AdapterName != "git" { + t.Fatalf("expected AdapterName=git, got %v", got.AdapterName) + } + if got.UserCredentialType == nil || *got.UserCredentialType != "pat" { + t.Fatalf("expected UserCredentialType=pat, got %v", got.UserCredentialType) + } + if got.UserHostScope == nil || *got.UserHostScope != "github.com" { + t.Fatalf("expected UserHostScope=github.com, got %v", got.UserHostScope) + } +} + +// Backward-compat: binary with no adapter + no user credential must return +// successfully with all three new fields NULL. +func TestPG_LookupByBinary_BackwardCompatibleNulls(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + ctx := store.WithTenantID(context.Background(), tenantID) + s := pg.NewPGSecureCLIStore(db, testEncryptionKey) + + binName := "gh-bc-null-" + uuid.New().String()[:8] + seedGateBinary(t, db, tenantID, binName, true) + + got, err := s.LookupByBinary(ctx, binName, nil, "") + if err != nil { + t.Fatalf("LookupByBinary: %v", err) + } + if got == nil { + t.Fatalf("expected binary, got nil") + } + if got.AdapterName != nil { + t.Fatalf("expected AdapterName NULL, got %q", *got.AdapterName) + } + if got.UserCredentialType != nil { + t.Fatalf("expected UserCredentialType NULL, got %q", *got.UserCredentialType) + } + if got.UserHostScope != nil { + t.Fatalf("expected UserHostScope NULL, got %q", *got.UserHostScope) + } +} diff --git a/ui/web/src/i18n/locales/en/cli-credentials.json b/ui/web/src/i18n/locales/en/cli-credentials.json index c2a15e1e..9f73cd5e 100644 --- a/ui/web/src/i18n/locales/en/cli-credentials.json +++ b/ui/web/src/i18n/locales/en/cli-credentials.json @@ -89,7 +89,23 @@ "deleted": "User credentials deleted", "deleteFailed": "Failed to delete user credentials", "envRequired": "At least one environment variable is required", - "mergeHint": "Chat users (Telegram, Discord, etc.) must be merged into a tenant user first via Contacts page before they can have per-user credentials." + "mergeHint": "Chat users (Telegram, Discord, etc.) must be merged into a tenant user first via Contacts page before they can have per-user credentials.", + "credentialType": "Credential Type", + "credentialTypeEnv": "Environment Variables (legacy)", + "credentialTypePAT": "Personal Access Token (PAT)", + "credentialTypeSSH": "SSH Private Key", + "hostScope": "Host Scope", + "hostScopePlaceholder": "github.com or gitlab.example.com:2222", + "hostScopeRequired": "Host scope is required for PAT and SSH credentials", + "hostScopeInvalid": "Host scope must be a valid hostname (optional :port)", + "token": "Personal Access Token", + "tokenPlaceholder": "ghp_… / glpat-… (never echoed back)", + "sshKey": "SSH Private Key (PEM)", + "sshKeyPlaceholder": "-----BEGIN OPENSSH PRIVATE KEY-----\n…\n-----END OPENSSH PRIVATE KEY-----", + "sshKeyInvalid": "SSH key is not a valid unencrypted PEM private key", + "passphraseUnsupported": "Passphrase-protected SSH keys are not supported. Provide an unencrypted key.", + "secretMasked": "•••••••• (stored, leave blank to keep)", + "noSecret": "No secret stored yet" }, "grants": { "title": "Agent Grants — {{name}}", diff --git a/ui/web/src/i18n/locales/vi/cli-credentials.json b/ui/web/src/i18n/locales/vi/cli-credentials.json index 9d2a5889..8538ccd2 100644 --- a/ui/web/src/i18n/locales/vi/cli-credentials.json +++ b/ui/web/src/i18n/locales/vi/cli-credentials.json @@ -89,7 +89,23 @@ "deleted": "Đã xóa thông tin người dùng", "deleteFailed": "Không thể xóa thông tin người dùng", "envRequired": "Cần ít nhất một biến môi trường", - "mergeHint": "Người dùng chat (Telegram, Discord...) cần được gộp vào tenant user qua trang Contacts trước khi có thể thiết lập credentials riêng." + "mergeHint": "Người dùng chat (Telegram, Discord...) cần được gộp vào tenant user qua trang Contacts trước khi có thể thiết lập credentials riêng.", + "credentialType": "Loại credential", + "credentialTypeEnv": "Biến môi trường (cũ)", + "credentialTypePAT": "Personal Access Token (PAT)", + "credentialTypeSSH": "Khoá riêng SSH", + "hostScope": "Phạm vi host", + "hostScopePlaceholder": "github.com hoặc gitlab.example.com:2222", + "hostScopeRequired": "Bắt buộc nhập phạm vi host cho credential PAT/SSH", + "hostScopeInvalid": "Phạm vi host phải là hostname hợp lệ (có thể kèm :port)", + "token": "Personal Access Token", + "tokenPlaceholder": "ghp_… / glpat-… (không bao giờ hiển thị lại)", + "sshKey": "Khoá riêng SSH (PEM)", + "sshKeyPlaceholder": "-----BEGIN OPENSSH PRIVATE KEY-----\n…\n-----END OPENSSH PRIVATE KEY-----", + "sshKeyInvalid": "Khoá SSH không phải là PEM hợp lệ chưa đặt passphrase", + "passphraseUnsupported": "Hệ thống không hỗ trợ khoá SSH có passphrase. Vui lòng dùng khoá không đặt passphrase.", + "secretMasked": "•••••••• (đã lưu, để trống để giữ nguyên)", + "noSecret": "Chưa có credential được lưu" }, "grants": { "title": "Phân quyền Agent — {{name}}", diff --git a/ui/web/src/i18n/locales/zh/cli-credentials.json b/ui/web/src/i18n/locales/zh/cli-credentials.json index ceca7f4e..2709e969 100644 --- a/ui/web/src/i18n/locales/zh/cli-credentials.json +++ b/ui/web/src/i18n/locales/zh/cli-credentials.json @@ -89,7 +89,23 @@ "deleted": "用户凭证已删除", "deleteFailed": "删除用户凭证失败", "envRequired": "至少需要一个环境变量", - "mergeHint": "聊天用户（Telegram、Discord 等）需要先通过联系人页面合并为租户用户，才能设置独立凭证。" + "mergeHint": "聊天用户（Telegram、Discord 等）需要先通过联系人页面合并为租户用户，才能设置独立凭证。", + "credentialType": "凭证类型", + "credentialTypeEnv": "环境变量（旧）", + "credentialTypePAT": "个人访问令牌 (PAT)", + "credentialTypeSSH": "SSH 私钥", + "hostScope": "主机范围", + "hostScopePlaceholder": "github.com 或 gitlab.example.com:2222", + "hostScopeRequired": "PAT/SSH 凭证必须填写主机范围", + "hostScopeInvalid": "主机范围必须是合法的主机名（可附加 :端口）", + "token": "个人访问令牌", + "tokenPlaceholder": "ghp_… / glpat-…（不会再显示）", + "sshKey": "SSH 私钥 (PEM)", + "sshKeyPlaceholder": "-----BEGIN OPENSSH PRIVATE KEY-----\n…\n-----END OPENSSH PRIVATE KEY-----", + "sshKeyInvalid": "SSH 密钥不是有效的未加密 PEM 私钥", + "passphraseUnsupported": "不支持带 passphrase 的 SSH 密钥，请提供未加密的私钥。", + "secretMasked": "••••••••（已保存，留空则保持不变）", + "noSecret": "尚未保存凭证" }, "grants": { "title": "代理授权 — {{name}}", diff --git a/ui/web/src/pages/cli-credentials/cli-credential-git-fields.tsx b/ui/web/src/pages/cli-credentials/cli-credential-git-fields.tsx new file mode 100644 index 00000000..6e5ef67b --- /dev/null +++ b/ui/web/src/pages/cli-credentials/cli-credential-git-fields.tsx @@ -0,0 +1,162 @@ +/** + * CliCredentialGitFields — Phase 5 typed-credential form for the git adapter. + * + * Rendered conditionally from CLIUserCredentialsDialog when + * `binary.adapter_name === "git"`. Three credential types: env (legacy), + * pat (single token field), ssh_key (textarea). Host scope is required + * for pat/ssh_key — validated client-side AND server-side; this component + * only blocks the network call when the field is empty. + * + * Secret state reset: when the user switches type, all secret state is + * cleared via `useEffect([type])` so a token entered for "PAT" cannot leak + * into the SSH key buffer (or vice versa). + */ +import { useEffect } from "react"; +import { useTranslation } from "react-i18next"; +import { Label } from "@/components/ui/label"; +import { Input } from "@/components/ui/input"; +import { Textarea } from "@/components/ui/textarea"; + +export type GitCredentialType = "env" | "pat" | "ssh_key"; + +export interface CliCredentialGitFieldsProps { + type: GitCredentialType; + onTypeChange: (t: GitCredentialType) => void; + hostScope: string; + onHostScopeChange: (v: string) => void; + token: string; + onTokenChange: (v: string) => void; + privateKey: string; + onPrivateKeyChange: (v: string) => void; + /** Backend error_key for inline display (e.g. "git.cred_ssh_passphrase_unsupported"). */ + errorKey?: string; + /** True iff the row already exists and the user is editing — show "secret set" placeholder. */ + hasExistingSecret?: boolean; +} + +export function CliCredentialGitFields({ + type, + onTypeChange, + hostScope, + onHostScopeChange, + token, + onTokenChange, + privateKey, + onPrivateKeyChange, + errorKey, + hasExistingSecret, +}: CliCredentialGitFieldsProps) { + const { t } = useTranslation("cli-credentials"); + + // Reset all secret state on type switch — prevents a token pasted under + // "PAT" from being submitted when the user later flips to "SSH key". + useEffect(() => { + onTokenChange(""); + onPrivateKeyChange(""); + // intentionally only depend on `type` — the setters are stable across renders + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [type]); + + const passphraseError = errorKey === "git.cred_ssh_passphrase_unsupported"; + const sshKeyError = errorKey === "git.cred_ssh_key_invalid"; + const hostScopeError = + errorKey === "git.cred_host_scope_required" || errorKey === "git.cred_host_scope_invalid"; + + return ( +
+
+ + +
+ + {/* Env legacy: caller owns env-vars table; render nothing here. */} + {type !== "env" && ( +
+ + onHostScopeChange(e.target.value)} + placeholder={t("userCredentials.hostScopePlaceholder")} + autoComplete="off" + spellCheck={false} + aria-invalid={hostScopeError} + /> + {hostScopeError && ( +

+ {errorKey === "git.cred_host_scope_required" + ? t("userCredentials.hostScopeRequired") + : t("userCredentials.hostScopeInvalid")} +

+ )} +
+ )} + + {type === "pat" && ( +
+ + onTokenChange(e.target.value)} + placeholder={ + hasExistingSecret + ? t("userCredentials.secretMasked") + : t("userCredentials.tokenPlaceholder") + } + /> +
+ )} + + {type === "ssh_key" && ( +
+ +