Skip to content

fix(authz): keep shared policy values immutable - #3987

Merged
strantalis merged 1 commit into
mainfrom
codex/authz-perf/immutable-policy
Oct 5, 2026
Merged

strantalis merged 1 commit into
mainfrom
codex/authz-perf/immutable-policy

Conversation

@strantalis

@strantalis strantalis commented Sep 5, 2026 •

Copy link
Copy Markdown
Member

PDP construction appends subject mappings to caller-owned values. Repeated construction accumulates mappings and mutates shared policy. Clone values before enrichment so concurrent constructors leave the source snapshot unchanged.

Layer 1 of 5, rebased onto main at b6163f92. The regression checks 16 concurrent constructors, mapping counts, and unchanged input objects.

Rebase validation (2026-10-04): authorization race tests, SDK README tests, formatting, and lint for the stack diff passed. Full repository check limitations are recorded in #3991. Current CI is pending.

Summary by CodeRabbit

  • Bug Fixes
    • Policy setup now preserves supplied attribute and mapping values, including when multiple policies are created concurrently. This prevents unintended changes to shared configuration and helps ensure each policy uses consistent inputs.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 6e7bc8aa-df38-41d2-95d6-4993a79fa3fd
📥 Commits

Reviewing files that changed from the base of the PR and between 52f2ece and ac44300.

📒 Files selected for processing (2)
  • service/internal/access/v2/pdp.go
  • service/internal/access/v2/pdp_immutable_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

NewPolicyDecisionPoint clones attribute and subject-mapping values before adding them to its in-memory entitlement index. A concurrent test checks that construction does not change shared input values.

Changes

Policy Value Immutability

Layer / File(s) Summary
Clone indexed values and verify shared inputs
service/internal/access/v2/pdp.go, service/internal/access/v2/pdp_immutable_test.go
Construction clones attribute and subject-mapping values before indexing them. A concurrent test checks construction results and confirms the shared values remain unchanged.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: elizabethhealy

Merge Risk: ⚪ Minimal · up to ac443

The change preserves indexed subject mappings while avoiding mutation of shared policy values. No actionable issue remains before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ac443

The change prevents constructors from modifying shared policy values while preserving existing authorization checks. No introduced attack path was established. Protection against concurrent updates to other shared policy objects remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The security-relevant scope is authorization decisions using the constructed entitlement index. Full-policy and targeted-policy construction paths can benefit from preventing source-value mutation; no new caller, privilege, tenant scope, or external endpoint was identified in the inspected paths.

Trust Boundaries and Controls

  • observed — Public authorization handlers continue through the just-in-time PDP. Policy inputs come from the store or policy services, while subject-mapping validation, strict namespace filtering, and FQN-based indexing remain in the constructor. Cloning changes value ownership rather than these control decisions.

Resilience and Maintainability Implications

  • inferred — Private value enrichment prevents repeated or concurrent constructors from accumulating mappings in shared source values. This does not establish safety against external mutation of retained attributes or mappings; the inspected store interface supplies no explicit read-only ownership contract.

Hardening Proposals

  • proposed — Document whether policy-store results remain read-only for the lifetime of a PDP and how refresh replaces or synchronizes shared objects. This would clarify the remaining ownership assumption, not remediate a verified PR-introduced vulnerability.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: preserving shared policy values during authorization policy construction.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit watched the values stay
Cloned before they joined the array
Sixteen builds ran side by side
Shared inputs kept their shape and stride
Then the rabbit hopped away!

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 169.965114ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 91.810697ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 331.67094ms
Throughput 301.50 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 31.4865355s
Average Latency 314.101303ms
Throughput 158.80 requests/second

@strantalis
strantalis force-pushed the codex/authz-perf/immutable-policy branch from be8b337 to 468e63e Compare October 1, 2026 16:06
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

X-Test Failure Report

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 186.827193ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 95.102442ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 347.1317ms
Throughput 288.08 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 41.190008762s
Average Latency 410.929745ms
Throughput 121.39 requests/second

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 233.248135ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 134.88303ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 418.801945ms
Throughput 238.78 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 47.855838128s
Average Latency 477.858267ms
Throughput 104.48 requests/second

@strantalis
strantalis force-pushed the codex/authz-perf/immutable-policy branch from 468e63e to d8ffaa8 Compare October 1, 2026 21:03
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

X-Test Failure Report

@strantalis
strantalis removed this pull request from stack #3992 October 1, 2026 21:07
@strantalis
strantalis added this pull request to stack #4131 October 1, 2026 21:07
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 234.601316ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 132.521227ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 412.898016ms
Throughput 242.19 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 44.079900501s
Average Latency 439.756121ms
Throughput 113.43 requests/second

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 232.262455ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 136.495863ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 411.185986ms
Throughput 243.20 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 44.875900601s
Average Latency 447.683569ms
Throughput 111.42 requests/second

@strantalis
strantalis force-pushed the codex/authz-perf/immutable-policy branch from d8ffaa8 to 379629b Compare October 1, 2026 22:10
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

X-Test Failure Report

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 228.057396ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 120.104606ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 410.791455ms
Throughput 243.43 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 43.547724073s
Average Latency 434.729484ms
Throughput 114.82 requests/second

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 225.488859ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 125.827746ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 406.4956ms
Throughput 246.01 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 44.267336261s
Average Latency 441.854127ms
Throughput 112.95 requests/second

Base automatically changed from codex/authz-perf/targeted-policy to main October 2, 2026 19:44
@jakedoublev
jakedoublev force-pushed the codex/authz-perf/immutable-policy branch from 379629b to ac44300 Compare October 2, 2026 19:45
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 237.985892ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 171.05619ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 422.42373ms
Throughput 236.73 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 46.03708325s
Average Latency 459.129058ms
Throughput 108.61 requests/second

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • otdfctl
  • service
  • tests-bdd

See the workflow run for details.

@strantalis
strantalis marked this pull request as ready for review October 4, 2026 22:40
@strantalis
strantalis requested a review from a team as a code owner October 4, 2026 22:40
Signed-off-by: strantalis <strantalis@virtru.com>
@strantalis
strantalis force-pushed the codex/authz-perf/immutable-policy branch from ac44300 to 1be62e9 Compare October 4, 2026 22:47
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 165.759202ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 96.719771ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 339.127168ms
Throughput 294.87 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.025648234s
Average Latency 379.647423ms
Throughput 131.49 requests/second

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • otdfctl
  • service
  • tests-bdd

See the workflow run for details.

@strantalis
strantalis added this pull request to the merge queue Oct 5, 2026
Merged via the queue into main with commit f992b60 Oct 5, 2026
50 checks passed
@strantalis
strantalis deleted the codex/authz-perf/immutable-policy branch October 5, 2026 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants