diff --git a/ASSURANCE_CASE.md b/ASSURANCE_CASE.md index 1b5abf43..5ea69795 100644 --- a/ASSURANCE_CASE.md +++ b/ASSURANCE_CASE.md @@ -59,13 +59,23 @@ OCR is a CLI tool that: | ID | Threat | Boundary | Mitigation | |----|--------|----------|------------| | T1 | Command injection via crafted diff content | 1 | All external commands are `git` only, with hardcoded subcommands; no shell expansion; `--end-of-options` used to prevent flag injection | -| T2 | API key leakage | 2 | Keys read from environment variables only; never logged, written to output files, or transmitted beyond the configured LLM endpoint | +| T2 | API key leakage | 2 | Keys may come from env vars, `api_key_cmd`/`auth_token_cmd`, or the user config file (see Credential sources). Never logged, written to review output files, or transmitted beyond the configured LLM endpoint | | T3 | Path traversal via LLM-suggested file paths | 3 | `pathutil.WithinBase()` validates all file paths against the repository root, both before and after symlink resolution | | T4 | DNS rebinding against local viewer | 4 | Host-header allowlist rejects requests from non-loopback origins; configurable via `OCR_VIEWER_ALLOWED_HOSTS` | | T5 | Man-in-the-middle on API communication | 2 | Go's `net/http` enforces TLS 1.2+ with full certificate verification by default; `InsecureSkipVerify` is never set | | T6 | Malicious LLM response | 2 | JSON schema validation on response structure; line number bounds checking against actual diff ranges | | T7 | Dependency vulnerabilities | All | `govulncheck` runs in CI; Dependabot monitors for updates; `go.sum` provides integrity verification | +### Credential sources + +API keys are never hardcoded in the binary. They may be supplied at runtime from: + +1. Environment variables (`OCR_LLM_TOKEN`, provider-specific variables such as `ANTHROPIC_API_KEY`). Env-only use is supported: a complete `OCR_LLM_*` or Claude Code `ANTHROPIC_*` environment needs no config file. +2. A credential command (`api_key_cmd` / `auth_token_cmd`) whose stdout is the secret. The command string may be stored in config; the secret itself is not. +3. Optional persistence in the user config file (`~/.opencodereview/config.json`) as `api_key` or `auth_token`. `ocr config set` and interactive provider setup write this file in plaintext; `saveConfig` (`cmd/opencodereview/provider_cmd.go`) creates new config files with mode `0600` and tightens existing config files to `0600` before persisting credentials. + +`ocr config set` masks `api_key`/`auth_token` when echoing the value. Sensitive request headers are redacted in raw logs. Keys are not written to review output files. + ## Secure Design Principles The following analysis maps [Saltzer & Schroeder's design principles](https://ieeexplore.ieee.org/document/1451869) to the project's implementation. @@ -73,11 +83,11 @@ The following analysis maps [Saltzer & Schroeder's design principles](https://ie | Principle | How Applied | |-----------|-------------| | **Least privilege** | `CGO_ENABLED=0` eliminates C library attack surface. The CLI requires no elevated permissions. No network listeners except the opt-in viewer. | -| **Fail-safe defaults** | API keys must be explicitly provided via environment variables. The viewer binds to localhost by default; non-loopback hosts require explicit allowlisting. | +| **Fail-safe defaults** | Credentials are not compiled in; they must be supplied via environment variables, `api_key_cmd`/`auth_token_cmd`, or the user config file. The viewer binds to localhost by default; non-loopback hosts require explicit allowlisting. | | **Complete mediation** | Every viewer HTTP request is checked against the host allowlist (`internal/viewer/hostguard.go`). Every file path from agent tools is validated against the repository root (`internal/tool/filereader.go:98`, `internal/pathutil/path.go`). | | **Economy of mechanism** | External process execution is limited to `git` with hardcoded subcommands — no shell invocation, no arbitrary command execution. | | **Open design** | Fully open-source (Apache-2.0). Security relies on TLS, not obscurity. | -| **Separation of privilege** | API authentication (keys) is separated from configuration (files). The viewer's host guard is a distinct middleware layer. | +| **Separation of privilege** | Credential material is never compiled into the binary; optional persistence is limited to the user config file, which `saveConfig` keeps at mode `0600` (pre-write chmod on existing files; new files created as `0600`). The viewer's host guard is a distinct middleware layer. | | **Least common mechanism** | Each review session writes to its own JSONL file. No shared state between sessions. | | **Psychological acceptability** | Security defaults (HTTPS, localhost binding, host allowlist) require no user configuration. Overrides (`OCR_VIEWER_ALLOWED_HOSTS`) are explicit and documented. | @@ -91,7 +101,7 @@ The following maps [OWASP Top 10](https://owasp.org/www-project-top-ten/) and [C | **A03:2021 Injection** (CWE-79 Cross-site Scripting) | Viewer template output is HTML-escaped by `html/template`. Defense-in-depth: every viewer response carries a strict Content-Security-Policy (`default-src 'self'`, no `unsafe-inline`) plus `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, `Referrer-Policy`, and `Permissions-Policy` (`internal/viewer/securityheaders.go`). | Mitigated | | **A01:2021 Broken Access Control** (CWE-22 Path Traversal) | Agent file-read tool validates paths with `pathutil.WithinBase()` before and after symlink resolution (`internal/tool/filereader.go:91-112`). | Mitigated | | **A02:2021 Cryptographic Failures** | All API communication uses HTTPS/TLS 1.2+. Go's default TLS configuration is used without weakening. `InsecureSkipVerify` is never set. | Mitigated | -| **A07:2021 Auth Failures** (CWE-798 Hard-coded Credentials) | API keys are read exclusively from environment variables, never embedded in code or config files, never logged. | Mitigated | +| **A07:2021 Auth Failures** (CWE-798 Hard-coded Credentials) | API keys are never hardcoded in source. They may be read from environment variables, `api_key_cmd`/`auth_token_cmd`, or the user config file (`~/.opencodereview/config.json`); `saveConfig` enforces mode `0600` on create and tightens existing config files to `0600` before persisting credentials. Never logged; `ocr config set` masks them on display. | Mitigated | | **A05:2021 Security Misconfiguration** | Secure defaults: localhost-only viewer, HTTPS-only API calls, `CGO_ENABLED=0`. The `go vet` and `govulncheck` tools run in CI. | Mitigated | | **A06:2021 Vulnerable Components** (CWE-1104) | Dependabot monitors Go modules and GitHub Actions. `govulncheck` runs on every push/PR. Dependencies are locked via `go.sum`. | Mitigated | | **A08:2021 Software Integrity** | Release binaries include SHA-256 checksums. Release tags are cryptographically signed (SSH). `CGO_ENABLED=0` produces static binaries with no external shared library dependencies. | Mitigated | diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index e32338ee..4edf8e15 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -425,12 +425,15 @@ func saveConfig(path string, cfg *Config) error { if err != nil { return fmt.Errorf("marshal config: %w", err) } + // WriteFile applies 0o600 only when creating the file; existing files keep + // their prior permissions. Tighten an existing config before writing any + // credential material; a missing file will be created as 0600 below. + if err := os.Chmod(path, 0o600); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("chmod config: %w", err) + } if err := os.WriteFile(path, data, 0o600); err != nil { return fmt.Errorf("write config: %w", err) } - if err := os.Chmod(path, 0o600); err != nil { - return fmt.Errorf("chmod config: %w", err) - } return nil } diff --git a/cmd/opencodereview/provider_cmd_test.go b/cmd/opencodereview/provider_cmd_test.go index b365c894..7966c6b5 100644 --- a/cmd/opencodereview/provider_cmd_test.go +++ b/cmd/opencodereview/provider_cmd_test.go @@ -102,6 +102,47 @@ func TestSaveConfig(t *testing.T) { } } +func TestSaveConfigEnforcesModeOnExistingFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows does not honor Unix file modes the same way") + } + + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte(`{"provider":"old"}`), 0o644); err != nil { + t.Fatalf("seed config: %v", err) + } + if err := os.Chmod(path, 0o644); err != nil { + t.Fatalf("set seed permissions: %v", err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat seed: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o644 { + t.Fatalf("seed perm = %o, want 644", perm) + } + + cfg := &Config{ + Provider: "anthropic", + Model: "claude-opus-4-6", + Providers: map[string]ProviderEntry{ + "anthropic": {APIKey: "sk-secret"}, + }, + } + if err := saveConfig(path, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + info, err = os.Stat(path) + if err != nil { + t.Fatalf("stat after save: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("perm after saveConfig = %o, want 600 (existing 0644 must be tightened)", perm) + } +} + func TestApplyProviderDeletions(t *testing.T) { dir := t.TempDir() configPath := filepath.Join(dir, "config.json")