feat: support trusted remote auth proxy#1163
Open
zhexuany wants to merge 1 commit into
Open
Conversation
|
|
📝 WalkthroughWalkthroughThis PR extends the sidecar authentication architecture to support remote managed HTTPS auth proxies alongside existing local HTTP sidecars. It introduces trust management via CLI commands, persists trusted host configuration, implements mode-aware credential resolution and request interception, and consolidates protocol parsing logic across demo servers. ChangesRemote Managed Auth Proxy Support with Trust Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
Author
|
@liangshuo-1 PTAL We want to manage internal user login via a single gate. |
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
2 participants
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.
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
New Features
auth-proxyCLI commands to trust, untrust, and list remote proxy hosts for authentication.Chores