diff --git a/docs/history.md b/docs/history.md index 976ef36..64e17e9 100644 --- a/docs/history.md +++ b/docs/history.md @@ -161,17 +161,17 @@ Severity is low: an attacker with read access to process memory already has full access to the process. The Go runtime does not guarantee memory zeroing and GC-managed runtimes inherently have this limitation. -### Finding F3: Empty ScopedRepos Bypasses Scope Check on Tier 2 (Medium) — Open +### Finding F3: Empty ScopedRepos Bypasses Scope Check on Tier 2 (Medium) — RESOLVED -In `policy.go`, the repo scope check is conditioned on `len(agent.ScopedRepos) > 0`. -A Tier 2 agent with empty `ScopedRepos` (nil or `[]string{}`) is treated as -unrestricted rather than as having no access. If an admin registers a Tier 2 -agent without explicitly setting `ScopedRepos`, it gets access to all repositories -for repo-scoped capabilities (`repo.push`, `pr.create`, `pr.merge`, `secrets.read`). +In `policy.go`, repo-scoped capability access previously skipped checks when +`len(agent.ScopedRepos) == 0`. +A Tier 2 agent with empty `ScopedRepos` (nil or `[]string{}`) was previously treated as +unrestricted rather than as having no access. -Potential remediation: treat empty `ScopedRepos` as no access for Tier 2 agents, -requiring explicit `["*"]` or `["org/**"]` for unrestricted access. This is a -design decision with backward-compatibility implications. +Resolved by requiring an explicit scope for repo-scoped capabilities: +- `[]string{}` / `nil` now denies all repo-scoped access by default. +- `[]string{"*"}` grants unrestricted repo access. +- Pattern matching with `host-uk/*` and `host-uk/**` still applies as before. ### Finding F4: `go vet` Clean — Passed @@ -224,8 +224,6 @@ callers that need structured logs should wrap or replace the cleanup goroutine. `crypt/chachapoly` into a single implementation. - **Hardware key backends**: implement `HardwareKey` for PKCS#11 (via `miekg/pkcs11` or `ThalesIgnite/crypto11`) and YubiKey (via `go-piv`). -- **Resolve Finding F3**: require explicit wildcard for unrestricted Tier 2 - access; treat empty `ScopedRepos` as no-access. - **Structured logging**: replace `fmt.Printf` in `StartCleanup` with an `slog.Logger` option on `Authenticator`. - **Rate limiting enforcement**: the `Agent.RateLimit` field is stored in the diff --git a/trust/policy.go b/trust/policy.go index 85ac374..28cbbc4 100644 --- a/trust/policy.go +++ b/trust/policy.go @@ -117,9 +117,9 @@ func (pe *PolicyEngine) Evaluate(agentName string, cap Capability, repo string) // Check if capability is allowed. for _, allowed := range policy.Allowed { if allowed == cap { - // For repo-scoped capabilities, verify repo access. - if isRepoScoped(cap) && len(agent.ScopedRepos) > 0 { - if !repoAllowed(agent.ScopedRepos, repo) { + // For repo-scoped capabilities, verify repo access for restricted tiers. + if isRepoScoped(cap) && agent.Tier != TierFull { + if len(agent.ScopedRepos) == 0 || !repoAllowed(agent.ScopedRepos, repo) { return EvalResult{ Decision: Deny, Agent: agentName, @@ -247,6 +247,11 @@ func matchScope(pattern, repo string) bool { return true } + // Star means unrestricted access for all repos. + if pattern == "*" { + return true + } + // Check for wildcard patterns. if !strings.Contains(pattern, "*") { return false diff --git a/trust/policy_test.go b/trust/policy_test.go index 2248377..6dce0d6 100644 --- a/trust/policy_test.go +++ b/trust/policy_test.go @@ -270,34 +270,49 @@ func TestDefaultRateLimit(t *testing.T) { // --- Phase 0 Additions --- -// TestEvaluate_Good_Tier2EmptyScopedReposAllowsAll verifies that a Tier 2 -// agent with empty ScopedRepos is treated as "unrestricted" for repo-scoped -// capabilities. NOTE: This is a potential security concern documented in -// FINDINGS.md — empty ScopedRepos bypasses the repo scope check entirely. -func TestEvaluate_Good_Tier2EmptyScopedReposAllowsAll(t *testing.T) { +// TestEvaluate_Bad_Tier2EmptyScopedReposDeniesAll verifies that an empty +// scoped-repo list blocks repo-scoped capabilities by default. +func TestEvaluate_Bad_Tier2EmptyScopedReposDeniesAll(t *testing.T) { r := NewRegistry() require.NoError(t, r.Register(Agent{ Name: "Hypnos", Tier: TierVerified, - ScopedRepos: []string{}, // empty — currently means "unrestricted" + ScopedRepos: []string{}, })) pe := NewPolicyEngine(r) - // Current behaviour: empty ScopedRepos skips scope check (len == 0) result := pe.Evaluate("Hypnos", CapPushRepo, "host-uk/core") - assert.Equal(t, Allow, result.Decision, - "empty ScopedRepos currently allows all repos (potential security finding)") + assert.Equal(t, Deny, result.Decision, + "empty ScopedRepos should deny repo-scoped operations by default") result = pe.Evaluate("Hypnos", CapReadSecrets, "host-uk/core") - assert.Equal(t, Allow, result.Decision) + assert.Equal(t, Deny, result.Decision) result = pe.Evaluate("Hypnos", CapCreatePR, "host-uk/core") assert.Equal(t, Allow, result.Decision) - // Non-repo-scoped capabilities should still work result = pe.Evaluate("Hypnos", CapCreateIssue, "") assert.Equal(t, Allow, result.Decision) - result = pe.Evaluate("Hypnos", CapCommentIssue, "") +} + +func TestEvaluate_Good_Tier2WildcardAllowsAll(t *testing.T) { + r := NewRegistry() + require.NoError(t, r.Register(Agent{ + Name: "Hydrus", + Tier: TierVerified, + ScopedRepos: []string{"*"}, + })) + pe := NewPolicyEngine(r) + + result := pe.Evaluate("Hydrus", CapPushRepo, "host-uk/core") + assert.Equal(t, Allow, result.Decision) + + result = pe.Evaluate("Hydrus", CapReadSecrets, "host-uk/any") + assert.Equal(t, Allow, result.Decision) + + result = pe.Evaluate("Hydrus", CapCreateIssue, "") + assert.Equal(t, Allow, result.Decision) + result = pe.Evaluate("Hydrus", CapCommentIssue, "") assert.Equal(t, Allow, result.Decision) } diff --git a/trust/scope_test.go b/trust/scope_test.go index db9f758..3d623f5 100644 --- a/trust/scope_test.go +++ b/trust/scope_test.go @@ -13,6 +13,11 @@ func TestMatchScope_Good_ExactMatch(t *testing.T) { assert.True(t, matchScope("host-uk/core", "host-uk/core")) } +func TestMatchScope_Good_StarWildcard(t *testing.T) { + assert.True(t, matchScope("*", "host-uk/core")) + assert.True(t, matchScope("*", "core/php/sub")) +} + func TestMatchScope_Good_SingleWildcard(t *testing.T) { assert.True(t, matchScope("core/*", "core/php")) assert.True(t, matchScope("core/*", "core/go-crypt")) diff --git a/trust/trust.go b/trust/trust.go index 3b32de0..4620d4f 100644 --- a/trust/trust.go +++ b/trust/trust.go @@ -71,7 +71,9 @@ type Agent struct { Name string // Tier is the agent's trust level. Tier Tier - // ScopedRepos limits repo access for Tier 2 agents. Empty means no repo access. + // ScopedRepos limits repo access for Tier 2 agents. + // Empty means no repo access. + // Use ["*"] for unrestricted repo scope. // Tier 3 agents ignore this field (they have access to all repos). ScopedRepos []string // RateLimit is the maximum requests per minute. 0 means unlimited.