Conversation
4d46ff7 to
02e512b
Compare
forwardproxy.go
Outdated
| h.udpProxyServer = &udpProxyServer{} | ||
| // parse http2/http3 uri template | ||
| if h.URITemplate == "" { | ||
| h.uriTemplate = uritemplate.MustNew("https://{host}/.well-known/masque/udp/{target_host}/{target_port}/") |
There was a problem hiding this comment.
Is it necessary to introduce a library for this? Can you use simpler functions?
forwardproxy_udp.go
Outdated
| return Request(net.JoinHostPort(targetHost, strconv.Itoa(targetPort))), nil | ||
| } | ||
|
|
||
| func (h *Handler) checkACL(hostPort string) (bool, error) { |
There was a problem hiding this comment.
Seems to have duplicated forwardproxy.go.
I think much of this should be embedded along side the logic of tcp/http.
0aa5ca1 to
95737ac
Compare
30eb2ff to
6b9937e
Compare
e7abc00 to
c3819b0
Compare
|
any update? |
|
Review comments are not addressed yet. |
|
How is it now? This PR looks very interesting, does the naiveproxy client need to be modified? |
|
I asked for not duplicating forwardproxy.go into a udp version, and there was no answer for that. |
Hi, I misunderstood your request there. I believe this is done now. |
|
Hi @imgk, Does this feature require any changes to the client? Or does it already support the client that complies with the RFC definition? |
Hi, this modified |
Fixes caddyserver#170 - Update `dial_timeout` example to use duration unit (30s) - Change parameter type from [integer] to [Duration] to match Go type - Update description to clarify duration unit requirement This change makes the documentation more accurate and consistent with Go's time.Duration type usage.
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v5) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/golang/glog](https://github.com/golang/glog) from 1.2.0 to 1.2.4. - [Release notes](https://github.com/golang/glog/releases) - [Commits](golang/glog@v1.2.0...v1.2.4) --- updated-dependencies: - dependency-name: github.com/golang/glog dependency-version: 1.2.4 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 5 to 6. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v5...v6) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
How is the status of the PR now? |
Bumps [github.com/quic-go/quic-go](https://github.com/quic-go/quic-go) from 0.44.0 to 0.49.1. - [Release notes](https://github.com/quic-go/quic-go/releases) - [Commits](quic-go/quic-go@v0.44.0...v0.49.1) --- updated-dependencies: - dependency-name: github.com/quic-go/quic-go dependency-version: 0.49.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
naiba
left a comment
There was a problem hiding this comment.
Code Review (AI-assisted review by Claude)
Thanks for the implementation! Reviewed against RFC 9298/9297 and the existing codebase. A few issues found:
Blocking
1. HTTP/2 path uses Hijack() — breaks HTTP/2 multiplexing
case 2:
conn, _, err := rc.Hijack()Hijack() drops down to the raw TCP connection, bypassing HTTP/2 framing entirely. This destroys all other streams on the same connection. The existing TCP CONNECT handler uses dualStream() for HTTP/2 (reading from r.Body + writing to ResponseWriter). The UDP handler should follow the same pattern.
2. ReceiveBuffer — remote crash via malicious Length
data.Length, err = quicvarint.Read(rr) // up to 2^62-1
bb := b[:data.Length] // panic if Length > len(b)A malicious client sending a large Length value will panic the server. Needs bounds check:
if data.Length > uint64(len(b)) {
return fmt.Errorf("datagram too large: %d > %d", data.Length, len(b))
}3. HTTP/1.1 Upgrade header typo
w.Header().Set("Upgrade:", RequestProtocol) // extra colon in header nameShould be "Upgrade", not "Upgrade:".
Should Fix
4. ACL logic duplicated — The anonymous function in tryUDPoverHTTP duplicates dialContextCheckACL. Should extract a shared helper to avoid divergence.
5. target_port range not validated — strconv.Atoi succeeds for 0, negative, or >65535 values. RFC 9298 requires 1-65535.
6. ReadPacket lacks bounds checking — Parsing bb[1], bb[2]...bb[19] without checking len(bb) first. Short payloads will panic.
7. Non-standard extensions not documented — CompressionAssignValue (0x1C0FE323) / CompressionCloseValue (0x1C0FE324) don't match current draft-ietf-masque-connect-udp-listen values (0x11/0x12/0x13), and COMPRESSION_ACK is missing. Connect-UDP-Bind is also draft-only. These should be clearly documented as experimental.
8. HandlePacketBind returns error at runtime — HTTP/3 + bind mode should be rejected at request parsing time with a proper HTTP status code, not deep in the handler.
Minor
- All types (
Payload,Datagram,BytePayload, etc.) and vars (CapsuleProtocolHeaderValue, etc.) are exported but are internal implementation details — should be unexported. HandlePacketrelies onb[0]being zero-initialized for Context ID 0 — should explicitly setb[0] = 0.RequestMatcheruses(.+)which matches across path segments — consider([^/]+).
1. What does this change do, exactly?
Add UDP in HTTP support
2. Please link to the relevant issues.
klzgrad/naiveproxy#617
3. Which documentation changes (if any) need to be made because of this PR?
4. Checklist