You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Area: policy — security (a shipped validation fix does not reach existing deployments) · found via PR #457 review
Expected: every path that loads a policy into the in-memory cache validates it first, so Evaluate can never be handed a policy Validate would reject — and so a validation rule added later actually takes effect on deployments that are already running.
Actual: only Store.Put (internal/policy/store.go:95) calls Validate. The two paths that populate the same cache from KV do not:
Store.load — internal/policy/store.go:184-188
Store.Watch — internal/policy/store.go:152-159
Both unmarshal the KV value and cache it directly.
Why this is no longer just defense-in-depth
This was originally filed as low-impact — reachable only by writing the KV bucket out-of-band. #457 changed that. It added a config-load rejection for malformed claim templates (a hyphenated or namespaced-OIDC path like {{ jwt.https://app.example.com/tenant_id }}, which the resolver would otherwise bind as literal {{…}} text — a read leak on _neq/_lt and silent row corruption on check). That guard runs in validateRolePerms, so it only fires where Validate is called.
Trace through NewStore (internal/policy/store.go:44-84):
Deployment state
Path
Malformed template outcome
Fresh deploy, policy.file_path set, KV empty
loadPolicyFile → Put → Validate
fatal — server refuses to start ✅
Runtime update
POST /v1/admin/policy → Validate (internal/api/policy.go:79)
400, rejected ✅
Existing deploy, policy already in KV
s.load(ctx) → cache → return s, nil, no Validate
loads unvalidated — the fail-open persists silently ❌
The KV-populated branch is the first thing NewStore tries, and it is documented as intentional ("KV is authoritative when populated" — the bootstrap file is a seed, #229). That is the right call for configuration precedence, but it means a security rule added to validateRolePerms never applies to any deployment that already has a policy — which is precisely the population most likely to be carrying the bad policy already. They stay exposed until someone happens to re-PUT.
This generalizes: every future rule added to validateRolePerms inherits the same blind spot, including the ones proposed in #460.
Scope
Validate on the load and Watch paths, not just Put.
Decide the failure posture explicitly. These are not equivalent:
Reject and keep the previous cached policy — stale but safe; good for Watch, where a bad update shouldn't take down a running server.
Reject and serve nothing — every request fails closed, including admin; matches the "empty KV, no bootstrap" behavior already documented at store.go:33-43.
Refuse to start — appropriate for load at boot, and consistent with how the bootstrap path already behaves.
Log loudly in every case; today a policy that cannot be honored is cached silently.
Cover the migration: existing deployments whose stored policy now fails validation need a defined upgrade path, not a surprise. Decide whether the first release that validates on load should warn-only.
Area: policy — security (a shipped validation fix does not reach existing deployments) · found via PR #457 review
Expected: every path that loads a policy into the in-memory cache validates it first, so
Evaluatecan never be handed a policyValidatewould reject — and so a validation rule added later actually takes effect on deployments that are already running.Actual: only
Store.Put(internal/policy/store.go:95) callsValidate. The two paths that populate the same cache from KV do not:Store.load—internal/policy/store.go:184-188Store.Watch—internal/policy/store.go:152-159Both unmarshal the KV value and cache it directly.
Why this is no longer just defense-in-depth
This was originally filed as low-impact — reachable only by writing the KV bucket out-of-band. #457 changed that. It added a config-load rejection for malformed claim templates (a hyphenated or namespaced-OIDC path like
{{ jwt.https://app.example.com/tenant_id }}, which the resolver would otherwise bind as literal{{…}}text — a read leak on_neq/_ltand silent row corruption oncheck). That guard runs invalidateRolePerms, so it only fires whereValidateis called.Trace through
NewStore(internal/policy/store.go:44-84):policy.file_pathset, KV emptyloadPolicyFile→Put→ValidatePOST /v1/admin/policy→Validate(internal/api/policy.go:79)s.load(ctx)→ cache →return s, nil, noValidateThe KV-populated branch is the first thing
NewStoretries, and it is documented as intentional ("KV is authoritative when populated" — the bootstrap file is a seed, #229). That is the right call for configuration precedence, but it means a security rule added tovalidateRolePermsnever applies to any deployment that already has a policy — which is precisely the population most likely to be carrying the bad policy already. They stay exposed until someone happens to re-PUT.This generalizes: every future rule added to
validateRolePermsinherits the same blind spot, including the ones proposed in #460.Scope
loadandWatchpaths, not justPut.Watch, where a bad update shouldn't take down a running server.store.go:33-43.loadat boot, and consistent with how the bootstrap path already behaves.Log loudly in every case; today a policy that cannot be honored is cached silently.
wavehouse policy validate), or a startup warning that names the offending rule without necessarily refusing to boot.Related
validateRolePermsrule whose protection this gap blocks from reaching existing deployments.policy validatetooling; the natural way to let operators check a live policy pre-upgrade.Found during the PR #457 review; the gap predates it, but #457 is what turned it from theoretical into load-bearing.