Skip to content

[security] Reserve spike/system/* from substring policy matches - #301

Merged
v0lkan merged 2 commits into
mainfrom
fix/policy-reserved-system-namespaces
Jul 25, 2026
Merged

[security] Reserve spike/system/* from substring policy matches#301
v0lkan merged 2 commits into
mainfrom
fix/policy-reserved-system-namespaces

Conversation

@v0lkan

@v0lkan v0lkan commented Jul 25, 2026

Copy link
Copy Markdown
Member

Attribution

Reported by @kanywst, who followed the private disclosure process in SECURITY.md and included a reproduction that ran cleanly against the tree. Their report concerned unanchored policy patterns matching on substrings, which is documented and intended behavior. Assessing it surfaced something that was not: the same matcher gates spike/system/acl, so a pattern like spike reached policy management by accident. This PR takes a different fix than the one proposed, but the finding underneath belongs to them.

Thank you for reporting it carefully and privately.

Reviewer orientation

123 files sounds worse than it is. 99 of them are the regenerated static
site under docs/, which is mechanical output.

Surface Files Review?
Code, docs source, specs, examples 24 Yes
Rendered site (docs/, make docs) 99 Skim

The whole review is really four files:

  • app/nexus/internal/state/base/reserved.go (new) — the guard
  • app/nexus/internal/state/base/policy.go — two call sites
  • app/nexus/internal/state/base/reserved_test.go (new) — 11 tests
  • docs-src/content/usage/commands/policy.md — the rewritten guidance

This PR contains two independent pieces of work in one commit. They are
unrelated. Part 2 had been sitting uncommitted in the working tree since
2026-07-18, and folding it in beat stranding it further behind an
unrelated branch. Both are described below; review them separately.


Part 1: reserved system namespaces

Spec: specs/policy-pattern-anchoring.md · ADR: adr-0033

The problem

Policy patterns are regular expressions matched with MatchString, so an
unanchored pattern is a substring test. That is intended, documented
behavior for ordinary paths and it does not change here.

It was not acceptable for the three paths through which SPIKE authorizes
its own privileged operations. Policy management is gated by
CheckPolicyAccess(peer, "spike/system/acl", [write]). A policy whose
PathPattern was spike, system, or acl therefore matched that gate
by substring and authorized the workload to create and modify any
policy, including one granting itself super on every path.
PathPattern: "spike" is a plausible thing to write when your own
secrets live under a spike/ namespace. The same reached
spike/system/secret and spike/system/cipher/exec. Nothing reserved
those namespaces.

The identity side compounds it: a delegation written for
spiffe://example\.org/admin without anchors also admits
spiffe://example.org/admin-attacker.

The fix

A policy may reach a reserved path only when it describes that path
rather than merely containing it. The test is semantic, not syntactic: a
pattern describes a path when its full-match form, ^(?:pattern)$, still
matches.

Pattern Reaches spike/system/acl?
^spike/system/acl$ Yes, deliberate
spike/system/acl Yes, deliberate
^spike/system/.*$ Yes, deliberate
.* Yes, deliberate
acl No, substring only
system No, substring only
spike No, substring only

The SPIFFE ID pattern must additionally be anchored, or be an
unambiguous catch-all such as .*.

Ordinary paths are untouched and keep plain substring semantics.

Two things worth pushing back on in review

Why the guard is in two places. UpsertPolicy rejects a violating
policy at authoring time, and CheckPolicyAccess declines to honor one
independently. The second is not redundant. The SQLite backend
(sqlite/persist/regex.go) recompiles both patterns from the stored
strings on every access check, so a guard placed only at creation holds
for the in-memory backend and does nothing in production. There is a
regression test that fails without it
(TestCheckPolicyAccess_ReservedGuardHoldsUnderSQLite).

Why not just anchor everything. Blanket-wrapping both patterns in
^(?:...)$ at compile time was the obvious fix and is wrong twice over.
It converts a working prefix policy like ^tenants/acme/ into
^(?:^tenants/acme/)$, which matches nothing usable — a silent access
revocation on upgrade, with no error naming the cause; this repo ships
eight such ^-only patterns. It also redefines PathPattern from "a
regular expression" into "a regular expression that is implicitly
full-match", silently reinterpreting every policy users have written. A
purely syntactic "must start with ^, end with $" variant was
implemented first and also rejected: .* and ^.*$ are the same regular
expression, so accepting one and refusing the other polices spelling
without changing what is granted, and it broke the deliberate wildcard
policies already in the test suite.

Documentation

The guidance was self-contradictory, which is how a careful operator
could land on an unanchored pattern. Of 147 concrete pattern examples in
the repo, 88% were already anchored and three documents taught anchoring
correctly — but the gaps were in the worst places:

  • usage/commands/policy.md had a "Path Pattern Examples" block of
    ^-only prefixes, one annotated # Only the specific creds resource
    for a pattern that also matches secrets/database/credsXYZ.
  • Its "Common Errors" section offered unanchored patterns as the
    correct remediation.
  • CLAUDE.md framed the only correctness axis as regex-versus-glob and
    marked two unanchored patterns as ✅ correct.
  • examples/federation/workload-set-policies.sh shipped a literal glob
    (tenants/demo/db/*) with write permission.
  • examples/policies/sample-policy.yaml used keys spiffeid:/path:
    that do not exist in PolicySpec; it could never have loaded.

All fixed, plus a new section stating plainly that patterns are matched
as substrings, that supplying anchors is the operator's responsibility,
and that patterns should be narrowed to the smallest workable set.

Rendered site

make docs was run. Beyond this PR's own changes it picks up the
Recipes section and the multi-tenancy page, neither of which had ever
been rendered, and drops ten orphaned pages under
docs/getting-started/ that had no source in docs-src. One of those
orphans was still serving the superseded policy guidance at a live URL.
build-docs.sh does cp -r public/* docs/, a merge that never deletes,
which is how they accumulated.


Part 2: live Pilot integration suite (Slice B)

Spec: specs/integration-tests.md

Written 2026-07-18, left uncommitted, unmodified here apart from being
re-verified. Adds app/spike/internal/cmd/integration, which drives the
built spike binary end to end against a running make start
environment rather than importing command internals.

Double-gated so it never runs in the normal suite: the integration
build tag keeps it out of ordinary builds, and TestMain exits early
unless SPIKE_INTEGRATION_TEST=1.

  • TestPilotSmokePass — secret put/get/delete, policy create/get by
    name, cipher round trip; asserts data lands on stdout. Cleans up.
  • TestPilotWarnsWhenNexusUnreachable — points one invocation at a
    closed port, leaving the running Nexus untouched, and asserts the
    Pilot warns without hanging or panicking. The no-hang assertion
    doubles as a guard on the open SVID-acquisition-timeout task.
  • TestPilotDeniesWhenNexusUninitialized — destructive, gated behind a
    second flag SPIKE_INTEGRATION_DESTRUCTIVE=1. Reaching a
    reachable-but-uninitialized Nexus means killing Nexus and every Keeper
    and restarting Nexus alone. It kills the Nexus it spawned, which sits
    outside make start's process table and would otherwise hold the port
    past a Ctrl+C.

Both error paths funnel through stdout.HandleAPIError and exit 0 (the
subcommands use cobra Run, not RunE), so assertions key on the
stderr message and the no-hang property, not the exit code.

Adds make integration-test and make integration-test-destructive.


Test plan

  • make lint-go — clean.
  • make test — full suite, zero failures.
  • The 11 new tests in reserved_test.go were verified to fail
    against a neutered guard and pass with it, so they are genuine
    regression tests rather than tautologies. Coverage includes both the
    memory and SQLite backends.
  • Part 2 is invisible to both make test and make lint-go because of
    its build tag, so it was checked separately:
    go vet -tags=integration ./... and
    golangci-lint run --build-tags=integration ./..., both clean.

Compatibility

Behavior changes only for policies that reach a reserved system path
through an unanchored pattern. Those now fail loudly: UpsertPolicy
returns an error naming the reserved path and the required form, and the
runtime refusal is logged. Deliberate delegation continues to work,
spelled ^spike/system/acl$ with an anchored SPIFFE ID pattern.
Wildcard policies (.*, ^.*$) are unaffected.

Credit

Reported by kanywst. The substring
behavior they reported is documented and intended; the escalation path
underneath it was not, and was found while assessing the report.

Follow-ups (not in this PR)

  • docs/internals/nil-source-handling-and-go-concurrency/index.html is
    also an orphan with no docs-src source. Left alone deliberately.
  • CONTRIBUTING_DCO.md is broken: it is a near-duplicate of
    CONTRIBUTING.md that links to itself as "our Developer Certificate
    of Origin" and never states the DCO text or the Signed-off-by
    mechanics. A contributor following CONTRIBUTING.md:23 cannot learn
    what to do from it.
  • build-docs.sh merges rather than syncs, so deleted pages linger in
    docs/ until someone notices. Worth an rsync --delete or a clean
    rebuild.

v0lkan added 2 commits July 18, 2026 10:49
Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
This commit has two independent parts. They are unrelated to each other
and are together only because the integration suite had been sitting
uncommitted in the working tree since 2026-07-18; folding it in now
avoids stranding it further behind an unrelated branch.

Part 1 is the substantive change. Part 2 is previously written work,
committed here unmodified apart from being verified to still build,
vet, and lint under its build tag.

================================================================
Part 1: reserved system namespaces  (Spec: policy-pattern-anchoring)
================================================================

Policy patterns are regular expressions matched with MatchString, so an
unanchored pattern is a substring test. That is intended behavior for
ordinary paths and stays unchanged. It was not acceptable for the three
paths through which SPIKE authorizes its own privileged operations.

Policy management is gated by CheckPolicyAccess against the literal path
spike/system/acl. A policy whose PathPattern was "spike", "system", or
"acl" therefore matched that gate by substring and authorized the
workload to create and modify any policy, including one granting itself
super on every path. PathPattern "spike" is plausible for an operator
whose own secrets live under a spike/ namespace. The same reached
spike/system/secret and spike/system/cipher/exec. Nothing reserved those
namespaces.

A policy may now reach a reserved path only when it describes that path
rather than merely containing it: its full-match form, ^(?:pattern)$,
must still match. So ^spike/system/acl$, spike/system/acl,
^spike/system/.*$ and .* all qualify, while acl, system and spike do
not. The SPIFFE ID pattern must be anchored or an unambiguous catch-all,
so a delegation written for spiffe://example.org/admin cannot be claimed
by spiffe://example.org/admin-attacker.

Enforcement sits at two points. UpsertPolicy rejects a violating policy
so the operator learns at authoring time, and CheckPolicyAccess declines
to honor one independently, covering policies stored before the rule
existed. The second point is load-bearing rather than redundant: the
SQLite backend recompiles both patterns from the stored strings on every
access check, so a guard placed only at creation would hold for the
in-memory backend and do nothing in production.

A purely syntactic "must start with ^ and end with $" rule was
implemented first and rejected. .* and ^.*$ are the same regular
expression, so accepting one and refusing the other polices spelling
without changing what is granted, and it broke deliberate wildcard
policies already under test.

Documentation is corrected throughout. The reference page carried a
"Path Pattern Examples" block of ^-only prefixes, one annotated "Only
the specific creds resource" for a pattern that also matches
secrets/database/credsXYZ, and a "Common Errors" section that offered
unanchored patterns as the remediation. CLAUDE.md framed the only
correctness axis as regex-versus-glob and marked two unanchored patterns
correct. Also fixes a shipped glob in the federation example and the
non-existent YAML keys in sample-policy.yaml, which could never have
loaded.

The rendered site is rebuilt. That picks up the Recipes section and the
multi-tenancy page, neither of which had ever been rendered, and drops
ten orphaned pages under docs/getting-started/ that had no source in
docs-src; one of them was still serving the superseded policy guidance
at a live URL. This accounts for most of the file count here.

Reported by kanywst. The substring behavior they reported is documented
and intended; the escalation path underneath it was not, and was found
while assessing the report.

================================================================
Part 2: live Pilot integration suite, Slice B  (Spec: integration-tests)
================================================================

Written 2026-07-18 and left uncommitted. Adds
app/spike/internal/cmd/integration, which drives the built spike binary
end to end against a running `make start` environment rather than
importing command internals, so it exercises the seams an operator does.

Double-gated so it never runs in the normal suite: the `integration`
build tag keeps it out of ordinary builds, and TestMain exits early
unless SPIKE_INTEGRATION_TEST=1 confirms a live environment may be
probed.

Three cases. TestPilotSmokePass covers secret put/get/delete, policy
create/get by name, and a cipher round trip, asserting data lands on
stdout. TestPilotWarnsWhenNexusUnreachable points one invocation at a
closed port, leaving the running Nexus untouched, and asserts the Pilot
warns without hanging or panicking; the no-hang assertion doubles as a
guard on the open SVID-acquisition-timeout task. Both are
non-destructive and clean up after themselves.

TestPilotDeniesWhenNexusUninitialized is destructive and gated behind a
second flag, SPIKE_INTEGRATION_DESTRUCTIVE=1: reaching a
reachable-but-uninitialized Nexus means killing Nexus and every Keeper,
losing the in-memory shards, then restarting Nexus alone. It kills the
Nexus it spawned, since that process is outside `make start`'s process
table and would otherwise hold the port past a Ctrl+C.

Both error paths funnel through stdout.HandleAPIError and exit 0 (the
subcommands use cobra Run, not RunE), so the assertions key on the
stderr message and the no-hang property, not the exit code.

Adds `make integration-test` and `make integration-test-destructive`,
marks the TASKS.md Phase 3 item done, and records the spec's two open
questions as resolved: land in-repo now behind the opt-in gate, and stay
complementary to the recovery drill rather than subsuming it.

Spec: specs/policy-pattern-anchoring.md
Spec: specs/integration-tests.md
Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
@v0lkan v0lkan self-assigned this Jul 25, 2026
@github-actions

Copy link
Copy Markdown

@v0lkan
v0lkan merged commit 9f33b47 into main Jul 25, 2026
14 of 15 checks passed
@v0lkan
v0lkan deleted the fix/policy-reserved-system-namespaces branch July 25, 2026 21:50
v0lkan added a commit that referenced this pull request Jul 26, 2026
CI on main fails at the "Go - Lint" job, which runs `make audit` and so
includes govulncheck. The failure is not caused by a code change. Run
#300 (2026-07-18) passed and run #301 (2026-07-25) failed, but the merge
in between never touched go.mod or go.sum, and golang.org/x/text was
v0.37.0 on both sides. The advisory was published in the interval, so
the build broke on a timer rather than on a commit.

GO-2026-5970 is an infinite loop on invalid input in golang.org/x/text.
It is called rather than merely present, reached through
recovery.sendShardsToKeepers -> net.Post -> norm.Form.*, so govulncheck
exits non-zero and takes the job with it.

Bumps x/text v0.37.0 -> v0.39.0, which fixes it, and x/net v0.55.0 ->
v0.56.0 to clear the uncalled GO-2026-5942 in the same pass. `go mod
tidy` transitively lifts x/crypto to v0.53.0, x/sys to v0.46.0, and
x/term to v0.44.0. No application code changes.

One finding is deliberately left behind. GO-2026-5932 reports that
golang.org/x/crypto/openpgp is unmaintained and unsafe by design, with
no fixed version: the package is deprecated, not patched, so no bump can
clear it. It arrives transitively and SPIKE does not call it, so
govulncheck exits 0 with it present.

That makes Round 1's "zero vulnerabilities total, not merely zero
called" criterion unreachable, so the spec amends it rather than
silently missing it. The standing bar is now zero *called*
vulnerabilities plus a recorded justification for every uncalled one
left in place, with clearing uncalled findings still preferred wherever
a fixed version exists.

Verified with the same command CI runs: `make audit` exits 0, `make
test` passes on the upgraded graph, and `go build ./...` is clean.

Spec: specs/vuln-remediation.md

Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant