[REVIEW] rbac-design: require policy precedence and deny conflict evidence

[modelsbridgeaicom-ship-it]

Skill Being Reviewed

Skill name: rbac-design
Skill path: skills/identity/rbac-design/

False Positive Analysis

Benign policy pattern that should remain allowed:

allow {
  input.subject.role == finance-reader
  input.resource.type == finance-report
  input.resource.tenant == input.subject.tenant
}

Why this is not a finding: A role grant plus tenant match is a normal RBAC/ABAC hybrid pattern. The issue is not the existence of permit rules; the issue is when the design does not define how broad permits interact with high-risk deny rules.

Coverage Gaps

Missed variant 1: broad permit overrides account suspension

allow {
  input.subject.role == finance-reader
  input.action == read
}

deny {
  input.subject.status == suspended
}

Why it should be caught: Without a documented combining algorithm, one PDP or gateway may treat allow as sufficient even when a high-risk deny condition exists.

Missed variant 2: missing attribute fails open

allow {
  input.subject.role == support-admin
}
# device_compliance is unavailable during IdP outage

Why it should be caught: Missing PIP attributes should not silently permit sensitive actions. The review should require fail-closed behavior for high-risk resources.

Missed variant 3: tenant admin bypasses tenant boundary

{
  role: tenant-admin,
  tenant: A,
  requested_resource_tenant: B
}

Why it should be caught: Tenant isolation denies must take precedence over broad admin role grants.

Edge Cases

• First-match policies can be safe only if ordering is versioned, reviewed, and covered by conflict tests.
• Break-glass roles may intentionally override normal rules, but they need ticket, expiry, logging, and approval obligations.
• Different enforcement layers can disagree: application code, API gateway, PDP, and database RLS may not use the same precedence model.

Remediation Quality

• Fix resolves the vulnerability
• Fix doesn't introduce new security issues
• Fix doesn't break functionality
• Issues found: Add explicit policy precedence / conflict-resolution gates requiring combining algorithm, default deny, deny precedence, fail-closed behavior, and negative test evidence.

Comparison to Other Tools

Tool	Catches this?	Notes
Semgrep	Partial	Can flag some policy anti-patterns if rules exist, but does not validate the authorization design model.
CodeQL	Partial	Can analyze application authorization paths, but policy precedence across PDP/PEP layers is an architecture review concern.
OPA/Rego tests	Partial	Can prove expected decisions when tests exist; the current skill does not require those conflict tests.
XACML policy tooling	Partial	Supports combining algorithms, but reviewers must verify the chosen algorithm and fail-closed behavior.

Overall Assessment

Strengths: The skill already covers RBAC levels, SoD constraints, permission boundaries, ABAC architecture, role mining, and conflict detection at a high level.

Needs improvement: It says policy conflicts should be detected, but it does not require a documented combining algorithm, deny precedence, missing-attribute behavior, or negative tests that prove high-risk denies cannot be bypassed by broad permits.

Priority recommendations:

1. Add a policy precedence and conflict-resolution section to the ABAC process.
2. Add RBAC-PREC-* findings for missing combining algorithm, broad permit override, fail-open missing attributes, mutable first-match order, and break-glass bypasses.
3. Extend the output template with policy conflict test evidence so reviews produce auditable results.

Bounty Info

• I have read and agree to the CONTRIBUTING.md bounty terms
• Preferred payment method: Payment details can be provided privately after maintainer acceptance.

[Dolpme]

Implemented in PR #1672: #1672