feat: enforce GAT downstream port and rewrite to upstream port on CONNECT#363
Open
clement0010 wants to merge 14 commits into
Open
feat: enforce GAT downstream port and rewrite to upstream port on CONNECT#363clement0010 wants to merge 14 commits into
clement0010 wants to merge 14 commits into
Conversation
The CONNECT handler previously matched only the destination hostname from the GAT and discarded the port, so a token scoped to host:22 could be replayed to reach any other port on the same host (SEC-215). Model the downstream port from the GAT (resource.gateway_metadata.downstream.port) and require the CONNECT destination port to equal it. Fail closed: reject with 403 when the port mismatches or the downstream port claim is absent. Enforced generically in ParseConnect, before protocol multiplexing, so it covers all resource types. Upstream port and port rewriting on forward remain out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNRnYZx1qdvnSdEWKUuYtf
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 86.06% 86.26% +0.19%
==========================================
Files 40 40
Lines 2828 2853 +25
==========================================
+ Hits 2434 2461 +27
+ Misses 269 265 -4
- Partials 125 127 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Gateway’s GAT token model to carry a client-facing downstream port and enforces that incoming CONNECT requests target that exact downstream port, rejecting mismatches.
Changes:
- Extend
token.Resourceto includegateway_metadata.downstream.portin GAT claims. - Enforce CONNECT destination port equality with the token’s downstream port in
MessageValidator.ParseConnect. - Update tests and test fakes to populate the new downstream port metadata and cover rejection cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/fake/client.go |
Populates GAT request payload with gateway_metadata.downstream.port derived from the resource port. |
internal/token/gat_claims.go |
Adds GatewayMetadata/Downstream to the token claim model under resource.gateway_metadata. |
internal/connect/connect.go |
Parses CONNECT host/port and rejects requests when the port doesn’t match token downstream port. |
internal/connect/connect_test.go |
Updates GAT claim fixtures and adds negative tests for port mismatch / missing downstream port. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback: a non-numeric CONNECT port is a malformed request, not an authorization failure. Split the parse step from the comparison so an unparseable port returns 400 with the parse error in the message (HTTPError only renders Code + Message, so the error must be in the message), and reserve 403 for a genuine port mismatch. Add a test covering the non-numeric port path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNRnYZx1qdvnSdEWKUuYtf
Previously the downstream port was only checked for presence (non-zero) in ParseConnect. A negative or out-of-range value (>65535) would pass and later fail during proxying. Validate the port range [1,65535] when the token is parsed, so a malformed port is rejected up front. The presence check in ParseConnect is now redundant and removed; the port-match check remains. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYz2gKUb3vHW4xzKzAfvEY
The controller issues a backend-facing upstream port in the GAT under resource.gateway_metadata.upstream.port. After verifying the CONNECT destination matches the downstream port, the Gateway rewrites the destination port to the upstream port before forwarding to the backend. Applied at the single shared point in ParseConnect, so it covers all resource types (Kubernetes, SSH, WebApp), which all dial the backend via the validated address. The upstream port range [1,65535] is validated in GATClaims.Validate, so a malformed port is rejected when the token is parsed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYz2gKUb3vHW4xzKzAfvEY
The fake client previously used a single port for the CONNECT destination and both GAT ports, so the port rewrite was a no-op in integration tests. Split them: the client targets a logical downstream port (WebApp 80, SSH 22, Kubernetes 443) while the backend listens on the real upstream port, so the Gateway's rewrite is exercised end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYz2gKUb3vHW4xzKzAfvEY
clement0010
changed the title
clement0010
force-pushed
the
fix/verify-connect-downstream-port
branch
from
9264f5e to
568b024
Compare
The downstream port is a 1:1 mapping from resource type (Kubernetes 443, SSH 22, WebApp 80), so both NewClient callers defined the same const block and threaded the port through as a parameter. Move the mapping into the fake client and derive the port from resourceType, failing fast on an unknown type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017w4FfxqxLjQSHuii3bBCds
Drop omitzero from resource.gateway_metadata: the claim is mandatory, so a zero value should serialize explicitly and fail port validation at parse rather than silently disappear from the payload. Format the invalid-port error as "field": value, and tighten the Validate tests: the missing-downstream case now zeroes the whole metadata and the missing-upstream case provides only a valid downstream port. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017w4FfxqxLjQSHuii3bBCds
Move downstream port verification and the upstream port rewrite out of ParseConnect into rewriteAddress, giving the port-mapping policy a single seam for future changes (e.g. validating against the upstream port once the CONNECT destination arrives already translated). Align the port-mismatch status with the destination address mismatch: both now return 400. Cover rewriteAddress with a standalone table-driven test and keep one ParseConnect test asserting the address is rewritten. Drop the missing-port ParseConnect tests, which duplicated TestGATTokenClaims_Validate coverage in the token package. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017w4FfxqxLjQSHuii3bBCds
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SACLEGn4b97yMUTEws2MyU
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
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.
Related Tickets & Documents
upstreamanddownstreamresource configuration in GAT token #342Changes
The controller issues per-resource ports in the GAT under
resource.gateway_metadata, and this PR makes the Gateway honor both:downstream.port, client-facing): the CONNECT destination port must equal the token's downstream port, otherwise the connection is refused with 400.upstream.port, backend-facing): after the downstream check passes, the destination port is rewritten to the upstream port before forwarding to the backend.Both are enforced at the single shared point in
ParseConnect, before protocol multiplexing, so they cover all resource types (Kubernetes, SSH, WebApp). Port ranges are validated inGATClaims.Validate, so a malformed or missing port claim is rejected when the token is parsed.