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" && ( +
+ +