Skip to content

fix: preserve policy denies and bound wildcard matching - #4

Merged
Vonng merged 4 commits into
mainfrom
codex/policy-correctness-20260908
Sep 8, 2026
Merged

fix: preserve policy denies and bound wildcard matching#4
Vonng merged 4 commits into
mainfrom
codex/policy-correctness-20260908

Conversation

@Vonng

@Vonng Vonng commented Sep 7, 2026

Copy link
Copy Markdown
Member

Contribution Licensing (no CLA, inbound=outbound, DCO required)

Contributed under AGPL-3.0-or-later with DCO sign-offs. Existing notices and
upstream authorship are retained; no CLA or separate license grant is introduced.

Description and motivation

Policy parsing and merging could silently discard distinct Deny statements
in two ways: hashes omitted NotResource, and string conditions formatted
["a b"] and ["a", "b"] identically. An affected request could be allowed
after parsing or merging even though the original clauses denied it. Hashing
now includes NotResources, and string-condition representations preserve
value boundaries with quoting and escaping. Real duplicates are still removed.

Directly constructed policies also report their Deny clauses before indexing.
Replace exponential wildcard recursion with one star backtracking point,
adapting minio/pkg#261. Preserve byte
matching and historical MatchSimple end-of-name behavior. Check optional
question-mark exits within each star-free segment to avoid repeatedly matching
whole prefixes on long near misses. A trailing star still succeeds immediately.
Matching retains constant auxiliary space.

The four independent commits contain their respective regression tests:

  1. Preserve distinct NotResource statements during parsing and merging.
  2. Detect Deny clauses in directly constructed policies.
  3. Bound wildcard matching while preserving historical semantics.
  4. Preserve string-condition value boundaries during deduplication.

Only four production files change. Default policies, permission names,
condition evaluation, strict/permissive parsing boundaries, JSON formats,
dependencies and the Go floor stay unchanged. String conditions' diagnostic
String() output now quotes each value; it is also used for equality and hashes.

Validation

  • New parser/merge and condition-value regressions fail before their fixes
    and pass after them. Tests cover 3/10/11/20-statement policies, denied
    resources and prefixes, allowed exceptions and real duplicate removal.
  • make test passes, including lint and all-package race tests. All-package
    race tests also pass with Go 1.26.7 and GOTOOLCHAIN=local.
  • The unchanged final matcher passes 254,512 committed exhaustive comparisons
    and previously completed 3,397,739 differential fuzz executions. This review
    adds 2,499,614 exhaustive comparisons against an independent dynamic-programming
    model, 100,000 long random comparisons and 100,000 comparisons based on
    constructed multi-star matches and truncated inputs; all pass.
  • Removing the NotResource hash, Deny fallback, or MatchSimple compatibility
    block in isolated copies causes the respective committed regressions to fail.
  • String-condition identity and JSON round trips pass for 21,483 distinct
    value/operator/qualifier combinations. The new formatter has the same
    allocation count and bytes as the previous formatter in the sampled benchmark;
    escaping adds some formatting cost, outside the request condition evaluator.
  • The unchanged matcher retains the validated local microbenchmarks:
    consecutive stars 40.09 ns/op vs old recursion 3.81 ms/op; the roughly 1 KB
    question-mark near miss 0.85 ms/op vs 0.79 ms/op; trailing star about 12 ns/op.
    Matching allocates zero bytes. These are not server-throughput claims.
  • Isolated downstream snapshots pass the relevant tests: SILO 8a9fd292b
    cmd (152 tests/subtests), Console a632e8e01 (46), and mcli 2af68322 (7).
    Another 16 SILO OpenID cases skip without an external IdP. Coverage includes
    local IAM resource boundaries, policy serialization/session APIs and CORS.
    Console validation uses committed source, preserving local pagination work.
  • go mod tidy -diff and git diff --check pass.

Downstream impact

The maintained server, Console and mcli need no source adaptation; their
version pins can advance when the next pkg version is released. Test-only
local replacements are confined to isolated snapshots.

Existing explicit Deny clauses are preserved, so requests that previously
depended on either deduplication bug can be denied. If an affected policy was
already saved after losing clauses, compare it with the original policy
source and reapply that source: an upgrade cannot recover statements absent
from stored JSON.

Checklist

  • Commits signed off and upstream authorship retained.
  • Regression, compatibility and performance checks pass.
  • Permission and configuration semantics preserved.
  • Downstream, diagnostic-format and policy-restoration implications documented.

Vonng and others added 2 commits September 8, 2026 08:12
Include NotResources in statement hashes so parsing long policies and
merging short policies retain distinct Deny statements. Preserve normal
deduplication and access to the shared exceptions in NotResource sets.

Regression tests cover both parsers around the ten-statement threshold,
short-policy merging, denied resources and an allowed shared exception.
Adapt the hash fix from minio/pkg commit
911bb0d.

Co-authored-by: Klaus Post <klauspost@gmail.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
Scan the statements when the cached Deny flag is unset. Keep the cached
true path and avoid mutating policies during inspection. Cover literal
policies, indexed policies and the built-in readonly definition.

Adapt the fallback from minio/pkg commit
911bb0d.

Co-authored-by: Klaus Post <klauspost@gmail.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
@Vonng
Vonng force-pushed the codex/policy-correctness-20260908 branch from d8b4f28 to df25e6f Compare September 8, 2026 00:14
Replace exponential recursion with a single star backtracking point.
Preserve byte matching and MatchSimple's historical acceptance when a
question mark is reached after the name is exhausted. Check these exits
within each star-free segment to avoid repeatedly matching whole prefixes
on long question-mark near misses. Preserve the immediate success for a
trailing star so long object names do not add needless scanning. Keep
constant auxiliary space.

Adapt the matcher and equivalence tests from minio/pkg commit
911bb0d. Add differential fuzzing,
explicit legacy cases, and star-backtracking and near-miss benchmarks.

Co-authored-by: Klaus Post <klauspost@gmail.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
@Vonng
Vonng force-pushed the codex/policy-correctness-20260908 branch from df25e6f to de7b7c5 Compare September 8, 2026 00:18
Condition equality and statement hashing shared a display string that
rendered ["a b"] and ["a", "b"] identically. Policy parsing and merging
could therefore discard a distinct Deny and allow requests it prohibited.

Quote and escape each string value in that representation. Keep the
existing comparison and hashing paths, condition evaluation and JSON
serialization. Cover small and large policies, merges, allowed prefixes,
real duplicates, spaces, quotes and escapes with regression tests.

Signed-off-by: Feng Ruohang <rh@vonng.com>
@Vonng
Vonng marked this pull request as ready for review September 8, 2026 04:55
@Vonng
Vonng merged commit a92c54d into main Sep 8, 2026
4 checks passed
@Vonng
Vonng deleted the codex/policy-correctness-20260908 branch September 9, 2026 11:42
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