feat: support trusted remote auth proxy#1163
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds remote managed HTTPS auth-proxy mode alongside local sidecar mode. Introduces config persistence for trusted proxy hosts, protocol parsing for mode-aware endpoint validation, CLI trust/untrust/list commands, credential provider and transport interceptor updates for remote proxy support, and demo server consolidation to use centralized parsing helpers. ChangesRemote Managed Auth Proxy Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
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 |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sidecar/protocol.go (1)
115-122: ⚡ Quick winReserved host check bypassed by non-default ports.
The
reservedRemoteProxyHostsmap contains hostnames without ports (e.g.,"open.feishu.cn"), butisReservedRemoteProxyHostcanonicalizes the authority which preserves non-default ports (e.g.,"open.feishu.cn:8443"). This meanshttps://open.feishu.cn:8443would not be rejected.While this edge case is unlikely in practice, the defense-in-depth intent suggests blocking these hostnames regardless of port.
Suggested fix to check hostname without port
func isReservedRemoteProxyHost(authority string) bool { - host, err := canonicalHTTPSAuthority(authority) + u, err := url.Parse("https://" + authority) if err != nil { return false } - return reservedRemoteProxyHosts[host] + return reservedRemoteProxyHosts[strings.ToLower(u.Hostname())] }Also applies to: 363-369
🤖 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 `@sidecar/protocol.go` around lines 115 - 122, reservedRemoteProxyHosts currently maps hostnames without ports but isReservedRemoteProxyHost performs its lookup against the canonicalized authority which can include non-default ports (e.g., "open.feishu.cn:8443"), allowing bypass; change the lookup to strip any port and use only the hostname when checking reservedRemoteProxyHosts: in isReservedRemoteProxyHost extract the hostname portion (e.g., call urlObj.Hostname() or use net.SplitHostPort with a fallback) and then check reservedRemoteProxyHosts[hostname], and apply the same hostname-only check to the other reserved-host check mentioned alongside reservedRemoteProxyHosts so ports are ignored for matching.extension/transport/sidecar/interceptor_test.go (1)
187-196: 💤 Low valueConsider adding a test for missing
PROXY_SESSIONin remote mode.The credential provider test file has
TestResolveAccount_RemoteHTTPSMissingProxySession, but there's no equivalent test here forResolveInterceptor. AddingTestProviderResolveInterceptor_RemoteHTTPSMissingProxySessionwould ensure symmetric coverage between the credential and transport layers.📝 Suggested test
func TestProviderResolveInterceptor_RemoteHTTPSMissingProxySession(t *testing.T) { trustRemoteProxy(t, "auth-proxy.example.com") t.Setenv(envvars.CliAuthProxy, "https://auth-proxy.example.com") t.Setenv(envvars.CliProxyKey, "proxy-signing-key") t.Setenv(envvars.CliProxySession, "") if interceptor := (&Provider{}).ResolveInterceptor(context.Background()); interceptor != nil { t.Fatal("expected nil interceptor when remote proxy session is missing") } }🤖 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 `@extension/transport/sidecar/interceptor_test.go` around lines 187 - 196, Add a symmetric test to cover the missing PROXY_SESSION case for the transport layer by creating TestProviderResolveInterceptor_RemoteHTTPSMissingProxySession that mirrors TestResolveAccount_RemoteHTTPSMissingProxySession: set trustRemoteProxy(t, "auth-proxy.example.com"), set envvars.CliAuthProxy to "https://auth-proxy.example.com", set envvars.CliProxyKey to a non-empty value and envvars.CliProxySession to an empty string, then assert that (&Provider{}).ResolveInterceptor(context.Background()) returns nil (fail the test if it is non-nil); reference the Provider type and its ResolveInterceptor method when locating where to add the test.extension/credential/sidecar/provider.go (1)
132-144: ⚡ Quick winDuplicate
requireTrustedRemoteProxyimplementation.This function is duplicated verbatim in
extension/transport/sidecar/interceptor.go(lines 220-232). Consider extracting it to a shared location (e.g., thesidecarpackage or a shared internal helper) to avoid divergence and reduce maintenance burden.♻️ Suggested refactor
Move
requireTrustedRemoteProxyto a shared location, for example in thesidecarpackage:// In sidecar/protocol.go or a new sidecar/trust.go func RequireTrustedRemoteProxy(endpoint ProxyEndpoint, loadConfig func() ([]string, error)) error { if endpoint.Mode != ProxyModeRemote { return nil } trustedHosts, err := loadConfig() if err != nil { return fmt.Errorf("failed to load auth proxy trust config: %w", err) } if IsTrustedRemoteProxyHost(endpoint.Host, trustedHosts) { return nil } return fmt.Errorf("remote auth proxy host %q is not trusted; run `lark-cli config auth-proxy trust https://%s` outside the agent environment", endpoint.Host, endpoint.Host) }Then both
provider.goandinterceptor.gocan call this shared function.🤖 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 `@extension/credential/sidecar/provider.go` around lines 132 - 144, Extract the duplicated requireTrustedRemoteProxy logic into a single shared function in the sidecar package (e.g., sidecar.RequireTrustedRemoteProxy) that accepts a sidecar.ProxyEndpoint and either calls core.LoadAuthProxyConfig internally or accepts a loader func for testability; implement the same behavior (check endpoint.Mode == sidecar.ProxyModeRemote, load trusted hosts, call sidecar.IsTrustedRemoteProxyHost and return the same formatted error). Replace both existing implementations (the requireTrustedRemoteProxy in provider.go and the one in interceptor.go) with calls to sidecar.RequireTrustedRemoteProxy and remove the duplicated code, updating imports accordingly.
🤖 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 `@extension/credential/sidecar/provider.go`:
- Around line 132-144: Extract the duplicated requireTrustedRemoteProxy logic
into a single shared function in the sidecar package (e.g.,
sidecar.RequireTrustedRemoteProxy) that accepts a sidecar.ProxyEndpoint and
either calls core.LoadAuthProxyConfig internally or accepts a loader func for
testability; implement the same behavior (check endpoint.Mode ==
sidecar.ProxyModeRemote, load trusted hosts, call
sidecar.IsTrustedRemoteProxyHost and return the same formatted error). Replace
both existing implementations (the requireTrustedRemoteProxy in provider.go and
the one in interceptor.go) with calls to sidecar.RequireTrustedRemoteProxy and
remove the duplicated code, updating imports accordingly.
In `@extension/transport/sidecar/interceptor_test.go`:
- Around line 187-196: Add a symmetric test to cover the missing PROXY_SESSION
case for the transport layer by creating
TestProviderResolveInterceptor_RemoteHTTPSMissingProxySession that mirrors
TestResolveAccount_RemoteHTTPSMissingProxySession: set trustRemoteProxy(t,
"auth-proxy.example.com"), set envvars.CliAuthProxy to
"https://auth-proxy.example.com", set envvars.CliProxyKey to a non-empty value
and envvars.CliProxySession to an empty string, then assert that
(&Provider{}).ResolveInterceptor(context.Background()) returns nil (fail the
test if it is non-nil); reference the Provider type and its ResolveInterceptor
method when locating where to add the test.
In `@sidecar/protocol.go`:
- Around line 115-122: reservedRemoteProxyHosts currently maps hostnames without
ports but isReservedRemoteProxyHost performs its lookup against the
canonicalized authority which can include non-default ports (e.g.,
"open.feishu.cn:8443"), allowing bypass; change the lookup to strip any port and
use only the hostname when checking reservedRemoteProxyHosts: in
isReservedRemoteProxyHost extract the hostname portion (e.g., call
urlObj.Hostname() or use net.SplitHostPort with a fallback) and then check
reservedRemoteProxyHosts[hostname], and apply the same hostname-only check to
the other reserved-host check mentioned alongside reservedRemoteProxyHosts so
ports are ignored for matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecda802e-d59c-4611-99f8-a628e9b71172
📒 Files selected for processing (19)
cmd/config/auth_proxy.gocmd/config/auth_proxy_test.gocmd/config/config.goextension/credential/sidecar/provider.goextension/credential/sidecar/provider_test.goextension/transport/sidecar/interceptor.goextension/transport/sidecar/interceptor_test.gointernal/core/config.gointernal/core/config_test.gointernal/envvars/envvars.gosidecar/hmac_test.gosidecar/protocol.gosidecar/server-demo/README.mdsidecar/server-demo/forward.gosidecar/server-demo/handler.gosidecar/server-demo/handler_test.gosidecar/server-multi-tenant-demo/forward.gosidecar/server-multi-tenant-demo/handler.gosidecar/server-multi-tenant-demo/handler_test.go
|
@liangshuo-1 PTAL We want to manage internal user login via a single gate. |
|
Thanks for working on this. My main concern is around the subjective / agent-safety risk. Trusting a remote auth proxy is effectively approving a new egress path for credentials and business traffic. Once a host is trusted, subsequent requests, proxy session material, and business data can flow through it. Right now, |
|
Agreed. Treating auth-proxy trust as a normal config write is too weak because it grants a new egress path for credential/session material and business traffic. I’ll change
|
|
Updated the PR to make remote auth-proxy trust an explicit user authorization boundary.
|
# Conflicts: # errs/subtypes.go
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)
cmd/config/auth_proxy.go (1)
77-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn typed
errs.*from the remaining trust-path failures.Line 83 and Line 100 still return legacy
output.ErrValidation/output.Errorf, so this migratedcmd/confighandler can emit different stderr envelopes depending on which branch fails. Please switch those branches to the same typed-error contract used by the new confirmation and agent-refusal paths.Based on learnings: For larksuite/cli command files under
cmd/**/*.go(starting with migrated commands / PR#1135onward), RunE handlers should return typed errors directly and must not useoutput.ErrXxx/output.ErrWithHint.🤖 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 `@cmd/config/auth_proxy.go` around lines 77 - 100, The two legacy output.* returns in runConfigAuthProxyTrust must be replaced with the typed errs.* returns used elsewhere: replace the output.ErrValidation("invalid auth proxy host: %v", err) returned after sidecar.NormalizeRemoteProxyTrustHost with the corresponding errs validation error (e.g., errs.Validation or the project’s validation constructor) including the original err message, and replace the output.Errorf(... "failed to save auth proxy trust config: %v", err) returned from core.UpdateAuthProxyConfig with the corresponding errs internal/operational error (e.g., errs.Internal or the project’s internal constructor) including the original err; update the two return sites in runConfigAuthProxyTrust (the NormalizeRemoteProxyTrustHost error path and the core.UpdateAuthProxyConfig error path) to use those typed errs.* values so the handler consistently returns typed errors.
🤖 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.
Outside diff comments:
In `@cmd/config/auth_proxy.go`:
- Around line 77-100: The two legacy output.* returns in runConfigAuthProxyTrust
must be replaced with the typed errs.* returns used elsewhere: replace the
output.ErrValidation("invalid auth proxy host: %v", err) returned after
sidecar.NormalizeRemoteProxyTrustHost with the corresponding errs validation
error (e.g., errs.Validation or the project’s validation constructor) including
the original err message, and replace the output.Errorf(... "failed to save auth
proxy trust config: %v", err) returned from core.UpdateAuthProxyConfig with the
corresponding errs internal/operational error (e.g., errs.Internal or the
project’s internal constructor) including the original err; update the two
return sites in runConfigAuthProxyTrust (the NormalizeRemoteProxyTrustHost error
path and the core.UpdateAuthProxyConfig error path) to use those typed errs.*
values so the handler consistently returns typed errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8bfc8e9-c7e6-457e-95b1-f547663d1db7
📒 Files selected for processing (5)
cmd/config/auth_proxy.gocmd/config/auth_proxy_test.goerrs/subtypes.gointernal/core/config.gointernal/core/config_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/config_test.go
3 participants
Summary
Adds a trusted remote HTTPS auth proxy path for environments where lark-cli cannot receive app secrets directly.
Changes
lark-cli config auth-proxy trust|untrust|listto store trusted remote proxy hosts in local config instead of env.LARKSUITE_CLI_AUTH_PROXY=https://host[:port]only when the host is locally trusted, while keeping HTTP constrained to same-host sidecar mode.LARKSUITE_CLI_PROXY_SESSIONis sent to the proxy, whileLARKSUITE_CLI_PROXY_KEYsigns requests and is not transmitted.X-Lark-Proxy-Targetparsing, and share target validation with the demo servers.Test Plan
go test ./sidecar -count=1go test -tags authsidecar ./extension/credential/sidecar ./extension/transport/sidecar -count=1go test -tags authsidecar_demo ./sidecar/server-demo -count=1go test -tags authsidecar_multi_tenant_demo ./sidecar/server-multi-tenant-demo -count=1go test ./cmd/... ./internal/... ./shortcuts/... ./extension/... -count=1go test -tags authsidecar ./cmd/... ./internal/... ./shortcuts/... ./extension/... ./sidecar -count=1go vet ./...gofmt -l .go mod tidy(nogo.mod/go.sumdiff)git diff --checkgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainmake unit-testblocked locally: Gogo1.26.0 darwin/arm64crashes inruntime.systemstack_switchunder-race -gcflags="all=-N -l"; observed as runtime SIGSEGV before package assertions run.Related Issues
Summary by CodeRabbit
Release Notes
New Features
config auth-proxycommand to manage trusted remote authentication proxy hosts withtrust,untrust, andlistsubcommands.Tests