[management,proxy] Add per-target options to reverse proxy#5501
Open
[management,proxy] Add per-target options to reverse proxy#5501
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds per-target reverse-proxy options (skip TLS verify, request timeout, path-rewrite, custom headers), propagates them through API/proto/OpenAPI/types, service model, mapping and runtime (per-request insecure transport and timeouts), adds validation/tests, and makes manager abort on invalid API input. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Layer"
participant Service as "Service Model"
participant Proxy as "ReverseProxy"
participant RoundTripper as "RoundTrip Transport"
participant Backend
Client->>API: POST /api/reverse-proxies/services (targets + options)
API->>Service: FromAPIRequest(req, accountID)
Service->>Service: validateTargetOptions(...)
alt validation fails
Service-->>API: error
API-->>Client: 400 InvalidArgument
else validation passes
Service->>API: success (persist/mapping)
API-->>Client: 200 OK
end
Client->>Proxy: Incoming request for path
Proxy->>Proxy: findTargetForRequest -> PathTarget (URL + options)
Proxy->>Proxy: determine PathRewriteMode & CustomHeaders
Proxy->>Proxy: rewriteFunc applies path rewrite and sets headers
alt SkipTLSVerify set
Proxy->>RoundTripper: RoundTrip using insecure transport (rgba(255,0,0,0.5))
else
Proxy->>RoundTripper: RoundTrip using default transport (rgba(0,128,0,0.5))
end
RoundTripper->>Backend: outbound request
Backend-->>RoundTripper: response
RoundTripper-->>Proxy: response
Proxy-->>Client: proxied response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
proxy/internal/proxy/proxy_bench_test.go (1)
31-38: Consider adding a benchmark variant with non-defaultPathTargetoptions.These fixtures only benchmark default option values. Adding at least one case with
RequestTimeout,PathRewritePreserve, andCustomHeaderswould better track regressions in the new per-target code paths.Also applies to: 72-79, 107-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/proxy/proxy_bench_test.go` around lines 31 - 38, Add a benchmark variant that uses a non-default proxy.PathTarget to exercise the per-target code paths: create an entry where PathTarget has RequestTimeout set to a non-zero duration, PathRewritePreserve set to true, and CustomHeaders populated (e.g., map[string]string with at least one header), and use that PathTarget in the same benchmark harness (the map literal that currently builds Paths with a default PathTarget). Make the same change for the other test fixtures in this file that construct PathTarget instances so benchmarks at the other locations also include a non-default option case.shared/management/client/rest/reverse_proxy_services_test.go (1)
148-199: Also assert response decoding of per-target options in this test.This test deeply validates request encoding, but the mocked response (
testService) doesn’t include options, so response-side decoding of nested options is not exercised.🧪 Suggested test strengthening
- retBytes, _ := json.Marshal(testService) + pathRewrite := api.ServiceTargetOptionsPathRewrite("preserve") + respSvc := testService + respSvc.Targets = []api.ServiceTarget{ + { + TargetId: "peer-123", + TargetType: "peer", + Protocol: "https", + Port: 8443, + Enabled: true, + Options: &api.ServiceTargetOptions{ + SkipTlsVerify: ptr(true), + RequestTimeout: ptr("30s"), + PathRewrite: &pathRewrite, + CustomHeaders: &map[string]string{"X-Foo": "bar"}, + }, + }, + } + retBytes, _ := json.Marshal(respSvc) _, err = w.Write(retBytes) require.NoError(t, err) }) @@ require.NoError(t, err) assert.Equal(t, testService.Id, ret.Id) + require.Len(t, ret.Targets, 1) + require.NotNil(t, ret.Targets[0].Options) + assert.True(t, *ret.Targets[0].Options.SkipTlsVerify) + assert.Equal(t, "30s", *ret.Targets[0].Options.RequestTimeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/client/rest/reverse_proxy_services_test.go` around lines 148 - 199, The test TestReverseProxyServices_Create_WithPerTargetOptions only validates request encoding but the mocked response (testService) lacks per-target options, so response decoding of nested api.ServiceTargetOptions isn’t exercised; update the mock response written in the mux.HandleFunc to include the same Options (SkipTlsVerify, RequestTimeout, PathRewrite, CustomHeaders) for the returned service target (either extend testService or construct a response object with those fields), then after c.ReverseProxyServices.Create returns assert that ret.Targets[0].Options is non-nil and that its SkipTlsVerify, RequestTimeout, PathRewrite and CustomHeaders match the expected values (use the same symbols api.ServiceTargetOptions, api.ServiceTargetOptionsPathRewrite and fields on ret.Targets[0].Options) so the response-side decoding is verified.proxy/server.go (1)
739-747: Cloningcustom_headersis optional defensive programming, not required.Lines 739-747 assign
opts.GetCustomHeaders()directly. While cloning the map would be defensive, it's not necessary because: (1) the proto message is not retained afterprotoToMapping()completes, (2) theCustomHeadersmap is never mutated after assignment, and (3)rewriteFunc()only reads from it. This is a reasonable best practice to consider, but the code is safe as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/server.go` around lines 739 - 747, Reviewer notes that assigning opts.GetCustomHeaders() directly to pt.CustomHeaders is safe and no change is required; keep the current assignment in the protoToMapping/pathMapping block (the GetOptions/pt.CustomHeaders assignment and usage by rewriteFunc). If you prefer the defensive approach, copy the map before assigning by creating a new map and copying each key/value from opts.GetCustomHeaders() into it (perform this copy where pt.CustomHeaders is set), but this is optional — no functional change is required.proxy/internal/roundtrip/netbird.go (1)
274-276: Optional: Explicitly pin MinVersion for code clarity and defense-in-depth hardening.Go 1.25 defaults to TLS 1.2 when
MinVersionis unset, so the current code is safe. However, setting an explicitMinVersiononinsecureTransportimproves readability and explicitly enforces your security posture.💡 Suggested enhancement (optional)
insecureTransport := transport.Clone() - insecureTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec + insecureTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec + MinVersion: tls.VersionTLS12, // explicit for clarity; Go 1.25 defaults to this + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/roundtrip/netbird.go` around lines 274 - 276, The TLS config created for insecureTransport currently sets only InsecureSkipVerify; update the tls.Config used for insecureTransport (created after transport.Clone() into insecureTransport) to explicitly set MinVersion (e.g., tls.VersionTLS12 or desired constant) alongside InsecureSkipVerify so the minimum TLS version is clear and enforced in insecureTransport's TLSClientConfig.shared/management/http/api/openapi.yml (1)
3030-3047: OpenAPI spec should document existingServiceTargetOptionsvalidation constraints.Validation for
request_timeout,custom_headers, header names, and header values already exists in the code (management/internals/modules/reverseproxy/service/service.go:495–574). Unsafe/reserved headers (Connection, Transfer-Encoding, Host, X-Forwarded-*, etc.) and CRLF injection are already blocked at runtime. Adding schema constraints to the OpenAPI spec would improve documentation clarity but is not a security fix.Optional OpenAPI documentation enhancement
ServiceTargetOptions: type: object properties: skip_tls_verify: type: boolean description: Skip TLS certificate verification for this backend request_timeout: type: string description: Per-target response timeout as a Go duration string (e.g. "30s", "2m") + pattern: '^(?:[0-9]+(?:\.[0-9]+)?(?:ns|us|µs|ms|s|m|h))+$' path_rewrite: type: string description: Controls how the request path is rewritten before forwarding to the backend. Default strips the matched prefix. "preserve" keeps the full original request path. enum: [preserve] custom_headers: type: object description: Extra headers sent to the backend. Hop-by-hop and reserved headers (Connection, Transfer-Encoding, Host, X-Forwarded-*, etc.) are rejected. Maximum 16 headers; key max 128 chars, value max 4096 chars. + propertyNames: + type: string + pattern: '^[!#$%&''*+.^_`|~0-9A-Za-z-]+$' additionalProperties: type: string + pattern: '^[^\r\n]*$'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 3030 - 3047, The OpenAPI schema for ServiceTargetOptions is missing constraints that mirror runtime validation; update the ServiceTargetOptions definition to document those constraints: (1) for request_timeout (property request_timeout) add a pattern or description indicating Go duration format (e.g., regex for "<number>[smh]" or explicit duration pattern) and a sensible max length, (2) for custom_headers (property custom_headers) add additionalProperties constraints for header name and value — restrict header names via a pattern to valid token characters and disallow reserved/unsafe header names (Connection, Transfer-Encoding, Host, Upgrade, Proxy-*, X-Forwarded-*) by listing them in enum or documenting they are forbidden, and (3) add a pattern or maxLength to header values to prevent CRLF injection. Reference ServiceTargetOptions, request_timeout, and custom_headers in your changes so the OpenAPI doc reflects the runtime validation in management/internals/modules/reverseproxy/service/service.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/internals/modules/reverseproxy/service/service.go`:
- Around line 504-514: The hopByHopHeaders map incorrectly lists "Trailers"
(plural) instead of the standard hop-by-hop header "Trailer", allowing the real
Trailer header to bypass filtering; update the hopByHopHeaders variable to
replace "Trailers" with "Trailer" so the Trailer header is correctly recognized
and blocked by the reverse proxy logic.
- Around line 547-567: The loop over opts.CustomHeaders currently checks length
and CRLF but doesn't reject empty or syntactically invalid header names; add a
check at the start of the iteration (inside the for key, value := range
opts.CustomHeaders loop) that the header name is non-empty (e.g.,
strings.TrimSpace(key) != "") and matches valid token syntax per RFC7230 (e.g.,
a regex restricting characters to token characters like
^[!#$%&'*+\\-.^_`|~0-9A-Za-z]+$); if the check fails return an error similar to
the existing ones (including idx and the offending key). This validation should
run before computing canonical := http.CanonicalHeaderKey(key) and before other
reserved/hop-by-hop checks.
In `@proxy/internal/proxy/servicemapping.go`:
- Around line 78-81: The code dereferences pt and pt.URL when matching a path
(m.Paths[path] → pt) which can panic if the mapping is malformed; update the
path-matching block in servicemapping.go to nil-check pt and pt.URL before
logging or returning: if pt == nil or pt.URL == nil, log a warning via
p.logger.Warnf/Debugf with context (host, path) and continue searching or return
a safe targetResult (nil target or an explicit error) instead of dereferencing;
ensure the targetResult return path handles a nil target consistently elsewhere.
---
Nitpick comments:
In `@proxy/internal/proxy/proxy_bench_test.go`:
- Around line 31-38: Add a benchmark variant that uses a non-default
proxy.PathTarget to exercise the per-target code paths: create an entry where
PathTarget has RequestTimeout set to a non-zero duration, PathRewritePreserve
set to true, and CustomHeaders populated (e.g., map[string]string with at least
one header), and use that PathTarget in the same benchmark harness (the map
literal that currently builds Paths with a default PathTarget). Make the same
change for the other test fixtures in this file that construct PathTarget
instances so benchmarks at the other locations also include a non-default option
case.
In `@proxy/internal/roundtrip/netbird.go`:
- Around line 274-276: The TLS config created for insecureTransport currently
sets only InsecureSkipVerify; update the tls.Config used for insecureTransport
(created after transport.Clone() into insecureTransport) to explicitly set
MinVersion (e.g., tls.VersionTLS12 or desired constant) alongside
InsecureSkipVerify so the minimum TLS version is clear and enforced in
insecureTransport's TLSClientConfig.
In `@proxy/server.go`:
- Around line 739-747: Reviewer notes that assigning opts.GetCustomHeaders()
directly to pt.CustomHeaders is safe and no change is required; keep the current
assignment in the protoToMapping/pathMapping block (the
GetOptions/pt.CustomHeaders assignment and usage by rewriteFunc). If you prefer
the defensive approach, copy the map before assigning by creating a new map and
copying each key/value from opts.GetCustomHeaders() into it (perform this copy
where pt.CustomHeaders is set), but this is optional — no functional change is
required.
In `@shared/management/client/rest/reverse_proxy_services_test.go`:
- Around line 148-199: The test
TestReverseProxyServices_Create_WithPerTargetOptions only validates request
encoding but the mocked response (testService) lacks per-target options, so
response decoding of nested api.ServiceTargetOptions isn’t exercised; update the
mock response written in the mux.HandleFunc to include the same Options
(SkipTlsVerify, RequestTimeout, PathRewrite, CustomHeaders) for the returned
service target (either extend testService or construct a response object with
those fields), then after c.ReverseProxyServices.Create returns assert that
ret.Targets[0].Options is non-nil and that its SkipTlsVerify, RequestTimeout,
PathRewrite and CustomHeaders match the expected values (use the same symbols
api.ServiceTargetOptions, api.ServiceTargetOptionsPathRewrite and fields on
ret.Targets[0].Options) so the response-side decoding is verified.
In `@shared/management/http/api/openapi.yml`:
- Around line 3030-3047: The OpenAPI schema for ServiceTargetOptions is missing
constraints that mirror runtime validation; update the ServiceTargetOptions
definition to document those constraints: (1) for request_timeout (property
request_timeout) add a pattern or description indicating Go duration format
(e.g., regex for "<number>[smh]" or explicit duration pattern) and a sensible
max length, (2) for custom_headers (property custom_headers) add
additionalProperties constraints for header name and value — restrict header
names via a pattern to valid token characters and disallow reserved/unsafe
header names (Connection, Transfer-Encoding, Host, Upgrade, Proxy-*,
X-Forwarded-*) by listing them in enum or documenting they are forbidden, and
(3) add a pattern or maxLength to header values to prevent CRLF injection.
Reference ServiceTargetOptions, request_timeout, and custom_headers in your
changes so the OpenAPI doc reflects the runtime validation in
management/internals/modules/reverseproxy/service/service.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4036cfb2-c363-4217-af4e-3952dc89f997
⛔ Files ignored due to path filters (2)
shared/management/proto/management.pb.gois excluded by!**/*.pb.goshared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
management/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.goproxy/internal/proxy/proxy_bench_test.goproxy/internal/proxy/reverseproxy.goproxy/internal/proxy/reverseproxy_test.goproxy/internal/proxy/servicemapping.goproxy/internal/roundtrip/context_test.goproxy/internal/roundtrip/netbird.goproxy/server.goshared/management/client/rest/reverse_proxy_services_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
lixmal
force-pushed
the
feat/per-target-proxy-options
branch
2 times, most recently
from
30fc3f1 to
2b341f7
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
management/internals/modules/reverseproxy/service/service.go (2)
504-514:⚠️ Potential issue | 🟠 MajorFix hop-by-hop key names so canonicalized checks cannot be bypassed.
http.CanonicalHeaderKey-based matching will miss"TE"and"Trailers". Use canonical/standard names ("Te"and"Trailer"), otherwise these hop-by-hop headers can slip through validation.🛠️ Proposed fix
var hopByHopHeaders = map[string]struct{}{ "Connection": {}, "Keep-Alive": {}, "Proxy-Authenticate": {}, "Proxy-Authorization": {}, "Proxy-Connection": {}, - "TE": {}, - "Trailers": {}, + "Te": {}, + "Trailer": {}, "Transfer-Encoding": {}, "Upgrade": {}, }#!/bin/bash # Verify current hop-by-hop blocklist entries in service validation. rg -n -A15 -B2 'var hopByHopHeaders = map\[string\]struct\{' management/internals/modules/reverseproxy/service/service.goAlso applies to: 557-560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/service/service.go` around lines 504 - 514, The hopByHopHeaders map uses non-canonical header names causing http.CanonicalHeaderKey checks to miss entries (e.g., "TE" vs canonical "Te", "Trailers" vs "Trailer"); update the keys in the hopByHopHeaders map (and the duplicate block around the other declaration at lines noted) to use canonical/standard header names such as "Connection", "Keep-Alive", "Proxy-Authenticate", "Proxy-Authorization", "Proxy-Connection", "Te", "Trailer", "Transfer-Encoding", and "Upgrade" so header matching using http.CanonicalHeaderKey cannot be bypassed; ensure you update both occurrences (the hopByHopHeaders variable and the duplicate block referenced) to the canonical forms.
547-567:⚠️ Potential issue | 🟠 MajorReject empty/invalid custom header names during validation.
Current checks allow malformed names (including empty/non-token keys), which defers failure to runtime forwarding instead of returning a deterministic validation error.
🛠️ Proposed fix
const ( maxRequestTimeout = 5 * time.Minute maxCustomHeaders = 16 maxHeaderKeyLen = 128 maxHeaderValueLen = 4096 ) + +var httpHeaderNameRegexp = regexp.MustCompile("^[!#$%&'*+\\-.^_`|~0-9A-Za-z]+$") @@ for key, value := range opts.CustomHeaders { + if !httpHeaderNameRegexp.MatchString(strings.TrimSpace(key)) { + return fmt.Errorf("target %d: custom header key %q is not a valid HTTP header name", idx, key) + } if len(key) > maxHeaderKeyLen { return fmt.Errorf("target %d: custom header key %q exceeds maximum length of %d", idx, key, maxHeaderKeyLen) }#!/bin/bash # Verify current custom-header validation checks and absence/presence of explicit name-token validation. rg -n -A35 -B5 'for key, value := range opts.CustomHeaders' management/internals/modules/reverseproxy/service/service.go rg -n 'httpHeaderNameRegexp|valid HTTP header name|MatchString\(.*key' management/internals/modules/reverseproxy/service/service.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/service/service.go` around lines 547 - 567, The loop validating opts.CustomHeaders allows empty or non-token header names; add an explicit name validation before canonicalization: reject empty keys and keys that do not match the HTTP header name token regexp (use the existing httpHeaderNameRegexp or add one) and return a descriptive validation error (e.g., "target %d: custom header name %q is invalid"). Place this check in the same block iterating over opts.CustomHeaders (the for key, value := range opts.CustomHeaders loop) before calling http.CanonicalHeaderKey(key) and before containsCRLF/hopByHopHeaders/reservedHeaders checks so malformed names fail at validation time.
🧹 Nitpick comments (1)
proxy/internal/roundtrip/netbird.go (1)
274-275: Consider settingMinVersionfor defense-in-depth.While this transport intentionally skips certificate verification, setting
MinVersion: tls.VersionTLS12(or TLS 1.3) would still protect against protocol downgrade attacks on the insecure transport. This aligns with the static analysis hint and provides layered security.🛡️ Optional hardening
insecureTransport := transport.Clone() - insecureTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec + insecureTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec + MinVersion: tls.VersionTLS12, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/roundtrip/netbird.go` around lines 274 - 275, The insecureTransport TLS config currently only sets InsecureSkipVerify; update the TLS config used by insecureTransport (created via transport.Clone() in this function) to also set a minimum TLS version (e.g., MinVersion: tls.VersionTLS12 or tls.VersionTLS13) to defend against protocol downgrade attacks while retaining InsecureSkipVerify for this path; modify the tls.Config passed to insecureTransport.TLSClientConfig accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@management/internals/modules/reverseproxy/service/service.go`:
- Around line 504-514: The hopByHopHeaders map uses non-canonical header names
causing http.CanonicalHeaderKey checks to miss entries (e.g., "TE" vs canonical
"Te", "Trailers" vs "Trailer"); update the keys in the hopByHopHeaders map (and
the duplicate block around the other declaration at lines noted) to use
canonical/standard header names such as "Connection", "Keep-Alive",
"Proxy-Authenticate", "Proxy-Authorization", "Proxy-Connection", "Te",
"Trailer", "Transfer-Encoding", and "Upgrade" so header matching using
http.CanonicalHeaderKey cannot be bypassed; ensure you update both occurrences
(the hopByHopHeaders variable and the duplicate block referenced) to the
canonical forms.
- Around line 547-567: The loop validating opts.CustomHeaders allows empty or
non-token header names; add an explicit name validation before canonicalization:
reject empty keys and keys that do not match the HTTP header name token regexp
(use the existing httpHeaderNameRegexp or add one) and return a descriptive
validation error (e.g., "target %d: custom header name %q is invalid"). Place
this check in the same block iterating over opts.CustomHeaders (the for key,
value := range opts.CustomHeaders loop) before calling
http.CanonicalHeaderKey(key) and before
containsCRLF/hopByHopHeaders/reservedHeaders checks so malformed names fail at
validation time.
---
Nitpick comments:
In `@proxy/internal/roundtrip/netbird.go`:
- Around line 274-275: The insecureTransport TLS config currently only sets
InsecureSkipVerify; update the TLS config used by insecureTransport (created via
transport.Clone() in this function) to also set a minimum TLS version (e.g.,
MinVersion: tls.VersionTLS12 or tls.VersionTLS13) to defend against protocol
downgrade attacks while retaining InsecureSkipVerify for this path; modify the
tls.Config passed to insecureTransport.TLSClientConfig accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 509aab99-1e25-4aa3-9ee7-ab084b80f61c
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
management/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.goproxy/internal/proxy/proxy_bench_test.goproxy/internal/proxy/reverseproxy.goproxy/internal/proxy/reverseproxy_test.goproxy/internal/proxy/servicemapping.goproxy/internal/roundtrip/context_test.goproxy/internal/roundtrip/netbird.goproxy/server.goshared/management/client/rest/reverse_proxy_services_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
✅ Files skipped from review due to trivial changes (1)
- shared/management/client/rest/reverse_proxy_services_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- proxy/internal/proxy/reverseproxy_test.go
- shared/management/http/api/openapi.yml
lixmal
force-pushed
the
feat/per-target-proxy-options
branch
from
2b341f7 to
cb00dcf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
shared/management/proto/proxy_service.proto (1)
59-64: Consider documenting zero-value behavior for new options.A short proto comment for
request_timeout(unset/zero semantics) andcustom_headersconstraints would reduce client/server ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/proto/proxy_service.proto` around lines 59 - 64, Add short proto comments to PathTargetOptions documenting zero-value semantics for request_timeout and constraints for custom_headers: state that an unset or zero request_timeout means "no timeout" (or server default) and specify allowed behaviors (e.g., non-negative durations only) and that custom_headers is a case-insensitive map where empty keys are forbidden and duplicate header names are merged/overwritten with defined precedence (or that the server rejects invalid entries). Update the message comment lines for request_timeout and custom_headers to make those behaviors explicit so clients and servers interpret unset/zero values and header handling consistently.proxy/internal/roundtrip/netbird.go (1)
275-275: Set an explicit TLS minimum version on the insecure transport.Line 275 intentionally skips certificate verification, but protocol version should still be pinned explicitly. Go 1.25 fully supports TLS 1.3, and this hardening measure ensures secure protocol negotiation.
🔧 Suggested hardening
- insecureTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec + insecureTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec + MinVersion: tls.VersionTLS13, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/roundtrip/netbird.go` at line 275, The TLS config on insecureTransport currently only sets InsecureSkipVerify; update the TLSClientConfig (in the insecureTransport setup) to explicitly pin a minimum TLS version (e.g., set MinVersion: tls.VersionTLS13 or at least tls.VersionTLS12) so protocol negotiation is constrained even when certificate verification is disabled; modify the assignment to insecureTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS13} (or tls.VersionTLS12 if compatibility required).shared/management/http/api/openapi.yml (1)
3036-3038: Tightenrequest_timeoutcontract for client-side validation.
request_timeoutis documented as Go duration but schema currently accepts any string. Add a validation hint (pattern) and an explicit example so SDKs can fail early instead of at runtime.Proposed OpenAPI tweak
request_timeout: type: string description: Per-target response timeout as a Go duration string (e.g. "30s", "2m") + example: "30s" + pattern: '^[0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h)([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))*$'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 3036 - 3038, The request_timeout schema currently allows any string; tighten it by adding a pattern that matches Go duration formats (e.g., optional number + unit sequences like "30s", "2m", "1h", with fractional seconds allowed) and add an explicit example value to the request_timeout property so client SDKs can validate earlier; update the request_timeout field in the OpenAPI definition (the request_timeout property in this schema) to include a suitable regex pattern and an example such as "30s".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/internals/modules/reverseproxy/service/service.go`:
- Around line 523-530: The hopByHopHeaders map contains "TE" which mismatches
http.CanonicalHeaderKey canonicalization ("Te"), causing the hop-by-hop filter
to miss TE headers; update the hopByHopHeaders map (variable hopByHopHeaders) to
use "Te" instead of "TE" so lookups that use http.CanonicalHeaderKey(key)
correctly match and block TE headers.
In `@shared/management/http/api/openapi.yml`:
- Around line 3043-3047: custom_headers currently allows arbitrary header
names/values; change its schema to constrain property names and values: replace
the free-form object with propertyNames using a strict regex (e.g. token-like
header-name pattern) and additionalProperties as a string schema with a value
pattern that forbids CR/LF and control characters, and add a description
documenting and enforcing a denylist of hop-by-hop/sensitive headers (e.g. Host,
Connection, Keep-Alive, Proxy-Authenticate, Proxy-Authorization, TE, Trailers,
Transfer-Encoding, Upgrade) that must not be forwarded; reference the
custom_headers schema and update propertyNames/additionalProperties and the
description to reflect these rules so the OpenAPI validator will reject invalid
header names/values and callers are aware of the denylist.
---
Nitpick comments:
In `@proxy/internal/roundtrip/netbird.go`:
- Line 275: The TLS config on insecureTransport currently only sets
InsecureSkipVerify; update the TLSClientConfig (in the insecureTransport setup)
to explicitly pin a minimum TLS version (e.g., set MinVersion: tls.VersionTLS13
or at least tls.VersionTLS12) so protocol negotiation is constrained even when
certificate verification is disabled; modify the assignment to
insecureTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true,
MinVersion: tls.VersionTLS13} (or tls.VersionTLS12 if compatibility required).
In `@shared/management/http/api/openapi.yml`:
- Around line 3036-3038: The request_timeout schema currently allows any string;
tighten it by adding a pattern that matches Go duration formats (e.g., optional
number + unit sequences like "30s", "2m", "1h", with fractional seconds allowed)
and add an explicit example value to the request_timeout property so client SDKs
can validate earlier; update the request_timeout field in the OpenAPI definition
(the request_timeout property in this schema) to include a suitable regex
pattern and an example such as "30s".
In `@shared/management/proto/proxy_service.proto`:
- Around line 59-64: Add short proto comments to PathTargetOptions documenting
zero-value semantics for request_timeout and constraints for custom_headers:
state that an unset or zero request_timeout means "no timeout" (or server
default) and specify allowed behaviors (e.g., non-negative durations only) and
that custom_headers is a case-insensitive map where empty keys are forbidden and
duplicate header names are merged/overwritten with defined precedence (or that
the server rejects invalid entries). Update the message comment lines for
request_timeout and custom_headers to make those behaviors explicit so clients
and servers interpret unset/zero values and header handling consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa801910-32e3-4a49-9ca2-68f6e1c0417e
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
management/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.goproxy/internal/proxy/proxy_bench_test.goproxy/internal/proxy/reverseproxy.goproxy/internal/proxy/reverseproxy_test.goproxy/internal/proxy/servicemapping.goproxy/internal/roundtrip/context_test.goproxy/internal/roundtrip/netbird.goproxy/server.goshared/management/client/rest/reverse_proxy_services_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
🚧 Files skipped from review as they are similar to previous changes (2)
- proxy/server.go
- proxy/internal/proxy/proxy_bench_test.go
lixmal
force-pushed
the
feat/per-target-proxy-options
branch
from
cb00dcf to
b0a3c42
Compare
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shared/management/http/api/openapi.yml (1)
3043-3047:⚠️ Potential issue | 🟠 MajorConstrain
custom_headersschema to reduce header-injection risk.
custom_headersstill allows arbitrary header keys/values in the OpenAPI contract. Add schema constraints for header-name syntax and reject CR/LF in values.Proposed OpenAPI hardening
custom_headers: type: object - description: Extra headers sent to the backend + description: Extra headers sent to the backend. Hop-by-hop/sensitive headers (e.g. Host, Connection, Transfer-Encoding) must be rejected by validation. + propertyNames: + type: string + pattern: '^[!#$%&''*+.^_`|~0-9A-Za-z-]+$' additionalProperties: type: string + pattern: '^[^\r\n]*$'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 3043 - 3047, The custom_headers schema currently allows arbitrary header names/values; tighten it by adding a propertyNames constraint to enforce a valid header-name pattern (e.g., alphanumerics, hyphen: reference the custom_headers schema name) and restrict additionalProperties to a string schema that disallows CR/LF (add a pattern rejecting \r or \n), and set a reasonable maxLength for header values (e.g., 256–512). Update the custom_headers block in openapi.yml to include propertyNames with a header-name regex and change additionalProperties to an object with type: string, pattern: "^[^\r\n]*$", and maxLength to mitigate header-injection risk.
🧹 Nitpick comments (2)
management/internals/modules/reverseproxy/service/service.go (1)
348-368: Avoid sharingCustomHeadersmap references across API/proto boundaries.These conversions currently pass map references through directly. Cloning avoids accidental aliasing/mutation coupling between request models, service state, and proto payloads.
♻️ Suggested refactor
+func cloneStringMap(in map[string]string) map[string]string { + if len(in) == 0 { + return nil + } + out := make(map[string]string, len(in)) + for k, v := range in { + out[k] = v + } + return out +} + func targetOptionsToAPI(opts TargetOptions) *api.ServiceTargetOptions { @@ if len(opts.CustomHeaders) > 0 { - apiOpts.CustomHeaders = &opts.CustomHeaders + h := cloneStringMap(opts.CustomHeaders) + apiOpts.CustomHeaders = &h } return apiOpts } @@ popts := &proto.PathTargetOptions{ SkipTlsVerify: opts.SkipTLSVerify, PathRewrite: pathRewriteToProto(opts.PathRewrite), - CustomHeaders: opts.CustomHeaders, + CustomHeaders: cloneStringMap(opts.CustomHeaders), } @@ if o.CustomHeaders != nil { - opts.CustomHeaders = *o.CustomHeaders + opts.CustomHeaders = cloneStringMap(*o.CustomHeaders) } return opts, nil }Also applies to: 370-383, 385-404
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/service/service.go` around lines 348 - 368, In targetOptionsToAPI, stop assigning the CustomHeaders map by reference (apiOpts.CustomHeaders = &opts.CustomHeaders); instead allocate a new map, copy each key/value from opts.CustomHeaders into it, and set apiOpts.CustomHeaders to a pointer to that new map to avoid aliasing; apply the same pattern to the reverse conversion(s) and any other converters mentioned (the similar functions around lines 370-383 and 385-404) so all CustomHeaders assignments create independent copies rather than sharing map references across the API/proto boundary.proxy/server.go (1)
739-743: Clonecustom_headerswhen buildingPathTargetto avoid map aliasing.This currently keeps a direct reference to the proto map. Cloning is safer for long-lived runtime state.
♻️ Suggested change
if opts := pathMapping.GetOptions(); opts != nil { pt.SkipTLSVerify = opts.GetSkipTlsVerify() pt.PathRewrite = protoToPathRewrite(opts.GetPathRewrite()) - pt.CustomHeaders = opts.GetCustomHeaders() + if src := opts.GetCustomHeaders(); len(src) > 0 { + pt.CustomHeaders = make(map[string]string, len(src)) + for k, v := range src { + pt.CustomHeaders[k] = v + } + } if d := opts.GetRequestTimeout(); d != nil { pt.RequestTimeout = d.AsDuration() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/server.go` around lines 739 - 743, When building the PathTarget from proto options (pathMapping.GetOptions()), do not assign the proto map directly to pt.CustomHeaders; instead create a new map and copy each key/value from opts.GetCustomHeaders() to avoid sharing the proto-backed map and accidental aliasing in long-lived PathTarget state. Update the code around pathMapping.GetOptions() -> pt.CustomHeaders to allocate a fresh map, iterate over opts.GetCustomHeaders(), copy entries, and assign that new map to pt.CustomHeaders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shared/management/http/api/openapi.yml`:
- Around line 3043-3047: The custom_headers schema currently allows arbitrary
header names/values; tighten it by adding a propertyNames constraint to enforce
a valid header-name pattern (e.g., alphanumerics, hyphen: reference the
custom_headers schema name) and restrict additionalProperties to a string schema
that disallows CR/LF (add a pattern rejecting \r or \n), and set a reasonable
maxLength for header values (e.g., 256–512). Update the custom_headers block in
openapi.yml to include propertyNames with a header-name regex and change
additionalProperties to an object with type: string, pattern: "^[^\r\n]*$", and
maxLength to mitigate header-injection risk.
---
Nitpick comments:
In `@management/internals/modules/reverseproxy/service/service.go`:
- Around line 348-368: In targetOptionsToAPI, stop assigning the CustomHeaders
map by reference (apiOpts.CustomHeaders = &opts.CustomHeaders); instead allocate
a new map, copy each key/value from opts.CustomHeaders into it, and set
apiOpts.CustomHeaders to a pointer to that new map to avoid aliasing; apply the
same pattern to the reverse conversion(s) and any other converters mentioned
(the similar functions around lines 370-383 and 385-404) so all CustomHeaders
assignments create independent copies rather than sharing map references across
the API/proto boundary.
In `@proxy/server.go`:
- Around line 739-743: When building the PathTarget from proto options
(pathMapping.GetOptions()), do not assign the proto map directly to
pt.CustomHeaders; instead create a new map and copy each key/value from
opts.GetCustomHeaders() to avoid sharing the proto-backed map and accidental
aliasing in long-lived PathTarget state. Update the code around
pathMapping.GetOptions() -> pt.CustomHeaders to allocate a fresh map, iterate
over opts.GetCustomHeaders(), copy entries, and assign that new map to
pt.CustomHeaders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19033e44-7cdf-47e9-8ddd-e2603c04b92b
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
.github/workflows/golangci-lint.ymlmanagement/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.goproxy/internal/proxy/proxy_bench_test.goproxy/internal/proxy/reverseproxy.goproxy/internal/proxy/reverseproxy_test.goproxy/internal/proxy/servicemapping.goproxy/internal/roundtrip/context_test.goproxy/internal/roundtrip/netbird.goproxy/server.goshared/management/client/rest/reverse_proxy_services_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
🚧 Files skipped from review as they are similar to previous changes (4)
- proxy/internal/proxy/servicemapping.go
- shared/management/client/rest/reverse_proxy_services_test.go
- proxy/internal/roundtrip/context_test.go
- management/internals/modules/reverseproxy/service/manager/api.go
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)
proxy/internal/proxy/reverseproxy.go (1)
117-135:⚠️ Potential issue | 🟠 Major
PathRewritePreservestill prepends the target base path.In preserve mode,
SetURL(target)joins the target base path with the incoming request path viajoinURLPath, so the backend receives a modified path instead of the original client path when target URLs include a base path.🛠️ Proposed fix
func (p *ReverseProxy) rewriteFunc(target *url.URL, matchedPath string, passHostHeader bool, pathRewrite PathRewriteMode, customHeaders map[string]string) func(r *httputil.ProxyRequest) { return func(r *httputil.ProxyRequest) { + originalPath := r.Out.URL.Path + originalRawPath := r.Out.URL.RawPath + switch pathRewrite { case PathRewritePreserve: - // Keep the full original request path as-is. + // Preserve path exactly as received from client. default: if matchedPath != "" && matchedPath != "/" { // Strip the matched path prefix from the incoming request path before // SetURL joins it with the target's base path, avoiding path duplication. r.Out.URL.Path = strings.TrimPrefix(r.Out.URL.Path, matchedPath) if r.Out.URL.Path == "" { r.Out.URL.Path = "/" } r.Out.URL.RawPath = "" } } r.SetURL(target) + if pathRewrite == PathRewritePreserve { + r.Out.URL.Path = originalPath + r.Out.URL.RawPath = originalRawPath + if r.Out.URL.Path == "" { + r.Out.URL.Path = "/" + } + } if passHostHeader { r.Out.Host = r.In.Host } else { r.Out.Host = target.Host }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/proxy/reverseproxy.go` around lines 117 - 135, The PathRewritePreserve branch currently calls r.SetURL(target) which causes the target's base path to be joined with the incoming path; change rewriteFunc so that in the PathRewritePreserve case you preserve the original client path by copying r.In.URL.Path and r.In.URL.RawPath into r.Out.URL.Path and r.Out.URL.RawPath (or override them after calling r.SetURL(target)) so the backend receives the exact incoming path without the target base path; reference rewriteFunc, PathRewritePreserve, r.SetURL, r.Out.URL.Path and r.In.URL.Path/RawPath when making the change.
🧹 Nitpick comments (4)
proxy/internal/roundtrip/netbird.go (1)
274-276: Preserve base TLS settings when creating the insecure transport clone.Overwriting
TLSClientConfigwith a fresh struct can silently drop future transport TLS settings (e.g., custom root CAs/min-version/server-name). Prefer cloning existing TLS config and only togglingInsecureSkipVerify.♻️ Proposed change
insecureTransport := transport.Clone() - insecureTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec + insecureTLS := &tls.Config{} + if transport.TLSClientConfig != nil { + insecureTLS = transport.TLSClientConfig.Clone() + } + insecureTLS.InsecureSkipVerify = true //nolint:gosec + insecureTransport.TLSClientConfig = insecureTLS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/roundtrip/netbird.go` around lines 274 - 276, The current code replaces transport.TLSClientConfig when creating insecureTransport (in netbird.go around the transport.Clone() usage), which can drop existing TLS settings; instead, preserve and clone the existing TLSClientConfig: if transport.TLSClientConfig != nil, make a shallow copy of that struct, set its InsecureSkipVerify = true and assign it to insecureTransport.TLSClientConfig, otherwise create a new tls.Config with InsecureSkipVerify = true; update the insecureTransport assignment that currently does insecureTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} to follow this clone-or-create approach so custom root CAs, MinVersion, ServerName, etc. are retained.shared/management/client/rest/reverse_proxy_services_test.go (1)
166-166: Use the generated enum constant instead of string casting forpath_rewrite.Using
api.ServiceTargetOptionsPathRewritePreserveavoids drift and keeps the test aligned with generated API symbols.🧹 Suggested tweak
- assert.Equal(t, api.ServiceTargetOptionsPathRewrite("preserve"), *opts.PathRewrite) + assert.Equal(t, api.ServiceTargetOptionsPathRewritePreserve, *opts.PathRewrite) ... - pathRewrite := api.ServiceTargetOptionsPathRewrite("preserve") + pathRewrite := api.ServiceTargetOptionsPathRewritePreserveAlso applies to: 175-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/client/rest/reverse_proxy_services_test.go` at line 166, Replace the string-cast usage api.ServiceTargetOptionsPathRewrite("preserve") in the test assertions with the generated enum constant api.ServiceTargetOptionsPathRewritePreserve; update any other assertions using the string-cast (e.g., the similar assertion around line 175) to compare against this constant so the test references the generated API symbol instead of a hard-coded string.management/internals/modules/reverseproxy/service/service.go (2)
385-404: Consider adding nil check for defensive coding.The function works correctly as called (caller guards with
if apiTarget.Options != nil), but adding a nil check would make it safer for future callers.🛡️ Suggested defensive check
func targetOptionsFromAPI(idx int, o *api.ServiceTargetOptions) (TargetOptions, error) { var opts TargetOptions + if o == nil { + return opts, nil + } if o.SkipTlsVerify != nil { opts.SkipTLSVerify = *o.SkipTlsVerify }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/service/service.go` around lines 385 - 404, Add a defensive nil check at the start of targetOptionsFromAPI to avoid dereferencing a nil *api.ServiceTargetOptions; e.g., in function targetOptionsFromAPI(idx int, o *api.ServiceTargetOptions) return a zero TargetOptions and nil error immediately if o == nil, then proceed with the existing field reads (this prevents panics if future callers omit the caller-side guard).
554-561: Consider simplifying the redundant timeout check.The nested conditions are logically correct but redundant. Since the outer check confirms
RequestTimeout != 0, the inner<= 0check is effectively< 0.♻️ Suggested simplification
- if opts.RequestTimeout != 0 { - if opts.RequestTimeout <= 0 { - return fmt.Errorf("target %d: request_timeout must be positive", idx) - } - if opts.RequestTimeout > maxRequestTimeout { - return fmt.Errorf("target %d: request_timeout exceeds maximum of %s", idx, maxRequestTimeout) - } + if opts.RequestTimeout < 0 { + return fmt.Errorf("target %d: request_timeout must be positive", idx) + } + if opts.RequestTimeout > maxRequestTimeout { + return fmt.Errorf("target %d: request_timeout exceeds maximum of %s", idx, maxRequestTimeout) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/service/service.go` around lines 554 - 561, The nested timeout checks are redundant; allow zero (meaning no timeout) and only reject negative values or values above maxRequestTimeout by replacing the current block with two simple checks on opts.RequestTimeout: one that returns fmt.Errorf("target %d: request_timeout must be positive", idx) when opts.RequestTimeout < 0 and another that returns fmt.Errorf("target %d: request_timeout exceeds maximum of %s", idx, maxRequestTimeout) when opts.RequestTimeout > maxRequestTimeout; update the checks around opts.RequestTimeout and remove the outer opts.RequestTimeout != 0 nesting in service.go so the logic is clearer (use the same identifiers opts.RequestTimeout, idx and maxRequestTimeout).
🤖 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 `@proxy/internal/proxy/reverseproxy.go`:
- Around line 117-135: The PathRewritePreserve branch currently calls
r.SetURL(target) which causes the target's base path to be joined with the
incoming path; change rewriteFunc so that in the PathRewritePreserve case you
preserve the original client path by copying r.In.URL.Path and r.In.URL.RawPath
into r.Out.URL.Path and r.Out.URL.RawPath (or override them after calling
r.SetURL(target)) so the backend receives the exact incoming path without the
target base path; reference rewriteFunc, PathRewritePreserve, r.SetURL,
r.Out.URL.Path and r.In.URL.Path/RawPath when making the change.
---
Nitpick comments:
In `@management/internals/modules/reverseproxy/service/service.go`:
- Around line 385-404: Add a defensive nil check at the start of
targetOptionsFromAPI to avoid dereferencing a nil *api.ServiceTargetOptions;
e.g., in function targetOptionsFromAPI(idx int, o *api.ServiceTargetOptions)
return a zero TargetOptions and nil error immediately if o == nil, then proceed
with the existing field reads (this prevents panics if future callers omit the
caller-side guard).
- Around line 554-561: The nested timeout checks are redundant; allow zero
(meaning no timeout) and only reject negative values or values above
maxRequestTimeout by replacing the current block with two simple checks on
opts.RequestTimeout: one that returns fmt.Errorf("target %d: request_timeout
must be positive", idx) when opts.RequestTimeout < 0 and another that returns
fmt.Errorf("target %d: request_timeout exceeds maximum of %s", idx,
maxRequestTimeout) when opts.RequestTimeout > maxRequestTimeout; update the
checks around opts.RequestTimeout and remove the outer opts.RequestTimeout != 0
nesting in service.go so the logic is clearer (use the same identifiers
opts.RequestTimeout, idx and maxRequestTimeout).
In `@proxy/internal/roundtrip/netbird.go`:
- Around line 274-276: The current code replaces transport.TLSClientConfig when
creating insecureTransport (in netbird.go around the transport.Clone() usage),
which can drop existing TLS settings; instead, preserve and clone the existing
TLSClientConfig: if transport.TLSClientConfig != nil, make a shallow copy of
that struct, set its InsecureSkipVerify = true and assign it to
insecureTransport.TLSClientConfig, otherwise create a new tls.Config with
InsecureSkipVerify = true; update the insecureTransport assignment that
currently does insecureTransport.TLSClientConfig =
&tls.Config{InsecureSkipVerify: true} to follow this clone-or-create approach so
custom root CAs, MinVersion, ServerName, etc. are retained.
In `@shared/management/client/rest/reverse_proxy_services_test.go`:
- Line 166: Replace the string-cast usage
api.ServiceTargetOptionsPathRewrite("preserve") in the test assertions with the
generated enum constant api.ServiceTargetOptionsPathRewritePreserve; update any
other assertions using the string-cast (e.g., the similar assertion around line
175) to compare against this constant so the test references the generated API
symbol instead of a hard-coded string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2075b0a3-5ee1-4855-a574-319aed3dca4b
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
.github/workflows/golangci-lint.ymlmanagement/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.goproxy/internal/proxy/proxy_bench_test.goproxy/internal/proxy/reverseproxy.goproxy/internal/proxy/reverseproxy_test.goproxy/internal/proxy/servicemapping.goproxy/internal/roundtrip/context_test.goproxy/internal/roundtrip/netbird.goproxy/server.goshared/management/client/rest/reverse_proxy_services_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
🚧 Files skipped from review as they are similar to previous changes (5)
- management/internals/modules/reverseproxy/service/service_test.go
- management/internals/modules/reverseproxy/service/manager/api.go
- .github/workflows/golangci-lint.yml
- shared/management/proto/proxy_service.proto
- shared/management/http/api/openapi.yml
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.
Describe your changes
Add per-target options to the reverse proxy, allowing each service target to configure:
skip_tls_verify: skip TLS certificate verification for the backend connectionrequest_timeout: per-target request timeout durationpath_rewrite: control how the request path is rewritten (e.g.preserveto keep the full original path)custom_headers: inject custom headers into proxied requestsOptions flow end-to-end: API request -> DB storage (embedded in Target) -> protobuf mapping -> proxy server -> reverse proxy handler. The roundtripper maintains a separate insecure transport clone per client, selected via context when
skip_tls_verifyis set.Checklist
Documentation
Select exactly one:
Internal proxy feature, no public-facing docs needed yet.
Summary by CodeRabbit
New Features
Public API / Schemas
Runtime
Bug Fixes
Tests