fix(policy-engine,controller): reject requests when no policy chain found#1424
Open
renuka-fernando wants to merge 1 commit intowso2:mainfrom
Open
fix(policy-engine,controller): reject requests when no policy chain found#1424renuka-fernando wants to merge 1 commit intowso2:mainfrom
renuka-fernando wants to merge 1 commit intowso2:mainfrom
Conversation
…ound Previously, routes with no policy chain in the policy engine were silently allowed through (skipAllProcessing), creating a security gap where requests could bypass configured policies. Now: - Controller sends empty policy chains via xDS instead of skipping them - Policy engine returns HTTP 500 when no policy chain is found for a route, preventing unauthorized passthrough - initializeExecutionContext returns an error instead of setting nil
Contributor
WalkthroughThe changes implement mandatory policy chain validation across the gateway system. Empty policy chains are now included in xDS resources instead of being skipped. When no policy chain exists during request processing, the system returns an HTTP 500 response instead of attempting to proceed with default behavior. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as Request Handler
participant InitCtx as initializeExecutionContext
participant PolicyMgr as Policy Manager
participant Response as Response Builder
Client->>Handler: HTTP Request (headers phase)
Handler->>InitCtx: initializeExecutionContext(req, execCtx)
InitCtx->>PolicyMgr: Lookup policy chain for route
alt Policy Chain Found
PolicyMgr-->>InitCtx: RouteMetadata + nil error
InitCtx-->>Handler: RouteMetadata, nil error
Handler->>Handler: Continue processing
else No Policy Chain Found
PolicyMgr-->>InitCtx: nil RouteMetadata + errNoPolicyChain
InitCtx-->>Handler: nil, errNoPolicyChain
Handler->>Response: Return ImmediateResponse
Response->>Response: Set HTTP 500 Status
Response-->>Client: HTTP 500 Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go (1)
412-441:⚠️ Potential issue | 🟡 MinorRemove the unused
skipAllProcessingfunction.The function has no callers in production code—only test files reference it. With the current implementation rejecting requests via HTTP 500 when no policy chain is found (line 168-183), this function is unreachable and should be deleted along with its tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go` around lines 412 - 441, Delete the unused skipAllProcessing method from ExternalProcessorServer (function skipAllProcessing in extproc.go) and remove its associated test(s); ensure any references in tests are deleted or updated so there are no compile-time references to skipAllProcessing, and run unit tests to confirm nothing else depends on it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go`:
- Around line 412-441: Delete the unused skipAllProcessing method from
ExternalProcessorServer (function skipAllProcessing in extproc.go) and remove
its associated test(s); ensure any references in tests are deleted or updated so
there are no compile-time references to skipAllProcessing, and run unit tests to
confirm nothing else depends on it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6f7d3ff-22a7-4c7b-b2c6-9685b62e97fc
📒 Files selected for processing (4)
gateway/gateway-controller/pkg/policyxds/policyxds_test.gogateway/gateway-controller/pkg/policyxds/snapshot.gogateway/gateway-runtime/policy-engine/internal/kernel/extproc.gogateway/gateway-runtime/policy-engine/internal/kernel/extproc_test.go
💤 Files with no reviewable changes (1)
- gateway/gateway-controller/pkg/policyxds/snapshot.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fix a security gap where requests bypass all configured policies when the policy engine has no policy chain for a route. Previously, if a route had an empty policy chain (no policies configured), the gateway-controller skipped sending that route's policy chain resource via xDS, and the policy engine silently passed the request through (
skipAllProcessing). This means a route configured with policies (e.g., API Key validation) could have its policies bypassed if the policy chain wasn't properly propagated.Resolves #1422, #1423
Goals
Approach
snapshot.go): Remove thecontinuethat skipped empty policy chains inTranslateRuntimeConfigs, so all routes get their policy chain resource sent via xDSextproc.go):errNoPolicyChainreturned byinitializeExecutionContextwhen no chain is foundImmediateResponseinhandleProcessingPhaseinstead of callingskipAllProcessingctxparameter frominitializeExecutionContext(logging moved to call site)User stories
As a platform operator, I want the gateway to reject requests when policy chains are missing, so that misconfigured routes cannot bypass security policies.
Documentation
N/A — internal behavioral change, no user-facing API changes.
Automation tests
TestInitializeExecutionContext_NoPolicyChain— verifieserrNoPolicyChainreturnedTestInitializeExecutionContext_WithPolicyChain— verifies no error on valid chainTestProcess_RequestHeaders_NoPolicyChain— verifies 500ImmediateResponseTestTranslator_TranslateRuntimeConfigs/empty_policy_chain_is_sent— verifies empty chains produce xDS resourcesSecurity checks
Samples
N/A
Related PRs
N/A
Test environment
Summary by CodeRabbit
Tests
Bug Fixes