server: validate forwarded PD hosts before dialing - #11069
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPD forwarding now validates forwarded hosts against the current PD leader’s advertised client URLs. Unary, TSO, bucket, region heartbeat, and metadata-write paths use the validated delegate client. TSO streams reuse one connection, and tests cover rejected targets. ChangesPD forwarding validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant grpcService
participant PDLeader
participant DelegateClient
Client->>grpcService: Forward request with pd-forwarded-host
grpcService->>PDLeader: Validate against advertised client URLs
PDLeader-->>grpcService: Return leader client URLs
grpcService->>DelegateClient: Dial or reuse validated connection
DelegateClient-->>grpcService: Return forwarded response
grpcService-->>Client: Return response or gRPC status error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ti-chi-bot
Bot
added
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/forward.go (1)
460-468: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider logging rejected forwarded-host attempts.
Since this is the sole gate preventing a caller from directing PD to dial an arbitrary attacker-supplied
pd-forwarded-host, logging rejections (with the offending host) would aid detection of SSRF probing attempts without changing behavior.📝 Optional logging addition
for _, clientURL := range leader.GetClientUrls() { if clientURL == forwardedHost { return s.getDelegateClient(ctx, forwardedHost) } } + log.Warn("rejected forwarded host that is not the PD leader's client URL", zap.String("forwarded-host", forwardedHost)) return nil, status.Errorf(codes.InvalidArgument, "forwarded host %q is not a client URL of the PD leader", forwardedHost)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/forward.go` around lines 460 - 468, Add logging for rejected forwarded-host attempts in the visible leader client URL validation flow, including the offending forwardedHost and enough context to identify the rejection. Log both the unavailable-leader path and the invalid-host path as appropriate, while preserving the existing status codes, messages, and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/forward.go`:
- Around line 460-468: Add logging for rejected forwarded-host attempts in the
visible leader client URL validation flow, including the offending forwardedHost
and enough context to identify the rejection. Log both the unavailable-leader
path and the invalid-host path as appropriate, while preserving the existing
status codes, messages, and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ed819c6-bfcc-4f51-8a9c-15481abff0be
📒 Files selected for processing (3)
server/forward.goserver/grpc_service.gotests/server/tso/tso_proxy_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11069 +/- ##
==========================================
+ Coverage 79.23% 79.24% +0.01%
==========================================
Files 540 541 +1
Lines 76010 76520 +510
==========================================
+ Hits 60226 60638 +412
- Misses 11533 11589 +56
- Partials 4251 4293 +42
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
79702f8 to
9eb68aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/server/tso/tso_proxy_test.go`:
- Around line 144-153: Extend TestRejectFollowerForwardedHost to open a Tso
stream using a forward context containing s.follower.GetAddr(), then call Recv
and assert the returned error has codes.InvalidArgument, while preserving the
existing unary GetAllStores assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c29d1463-5ca5-4851-b371-eeb59139fb14
📒 Files selected for processing (4)
server/forward.goserver/grpc_service.goserver/resource_group_proxy_service.gotests/server/tso/tso_proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/resource_group_proxy_service.go
- server/forward.go
- server/grpc_service.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/resource_group_proxy_service.go`:
- Line 65: Extract the leader-URL validation from getPDForwardedDelegateClient
into a no-dial helper, then invoke it for forwardedHostFromMetadata targets
before the isLocalRequest branch in the request flow. Reject metadata URLs that
do not belong to the current leader with InvalidArgument, and retain local
handling only after validation succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9568c497-5d7e-4aa2-a6a7-f29a8101613e
📒 Files selected for processing (1)
server/resource_group_proxy_service.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
|
/test pull-unit-test-next-gen-3 |
1 similar comment
|
/test pull-unit-test-next-gen-3 |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/test pull-unit-test-next-gen-3 |
|
/test pull-unit-test-next-gen-2 |
|
/test pull-unit-test-next-gen-3 |
|
/test pull-unit-test-next-gen-2 |
|
/test pull-integration-realcluster-test |
|
/test pull-unit-test-next-gen-2 |
|
/test pull-unit-test-next-gen-3 |
ti-chi-bot
Bot
added
the
needs-cherry-pick-release-8.5
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
2 participants
What problem does this PR solve?
Issue Number: close #11070
The
pd-forwarded-hostgRPC metadata is controlled by the caller, but PD previously used it as a dial target without verifying that it is the current PD leader. This could make PD establish outbound gRPC connections to attacker-selected addresses.What is changed and how does it work?
GetMembersor query etcd.InvalidArgumentfor non-leader targets andUnavailablewhen no leader is available.Check List
Tests
Code changes
Side effects
Related changes
Release note
Summary by CodeRabbit