diff --git a/extension/transport/sidecar/interceptor.go b/extension/transport/sidecar/interceptor.go index 6d928bd8a..d8989b28f 100644 --- a/extension/transport/sidecar/interceptor.go +++ b/extension/transport/sidecar/interceptor.go @@ -4,11 +4,12 @@ //go:build authsidecar // Package sidecar provides a transport interceptor for the auth sidecar -// proxy mode. When LARKSUITE_CLI_AUTH_PROXY is set (an HTTP URL), all -// outgoing requests are rewritten to the sidecar address. The interceptor -// strips placeholder credentials, injects proxy headers, and signs each -// request with HMAC-SHA256. No custom DialContext is needed — Go's -// standard http.Transport connects to the sidecar via plain HTTP. +// proxy mode. When LARKSUITE_CLI_AUTH_PROXY is set (an http:// or https:// +// URL), all outgoing requests are rewritten to the sidecar address. The +// interceptor strips placeholder credentials, injects proxy headers, and +// signs each request with HMAC-SHA256. No custom DialContext is needed — +// Go's standard http.Transport connects to the sidecar via HTTP, or via +// HTTPS (TLS) when the sidecar address is an https:// URL. package sidecar import ( @@ -46,15 +47,17 @@ func (p *Provider) ResolveInterceptor(ctx context.Context) transport.Interceptor } key := os.Getenv(envvars.CliProxyKey) return &Interceptor{ - key: []byte(key), - sidecarHost: sidecar.ProxyHost(proxyAddr), + key: []byte(key), + sidecarHost: sidecar.ProxyHost(proxyAddr), + sidecarScheme: sidecar.ProxyScheme(proxyAddr), } } // Interceptor rewrites requests for the sidecar proxy. type Interceptor struct { - key []byte // HMAC signing key - sidecarHost string // sidecar host:port for URL rewriting + key []byte // HMAC signing key + sidecarHost string // sidecar host[:port] for URL rewriting + sidecarScheme string // "http" (same-host) or "https" (remote TLS sidecar) } // PreRoundTrip rewrites the request for sidecar routing when it carries a @@ -130,8 +133,13 @@ func (i *Interceptor) PreRoundTrip(req *http.Request) func(resp *http.Response, req.Header.Set(sidecar.HeaderProxyTimestamp, ts) req.Header.Set(sidecar.HeaderProxySignature, sig) - // 5. Rewrite URL to route through sidecar - req.URL.Scheme = "http" + // 5. Rewrite URL to route through sidecar. Scheme follows the configured + // proxy address: https for a remote (TLS) sidecar, http for a same-host one. + scheme := i.sidecarScheme + if scheme == "" { + scheme = "http" + } + req.URL.Scheme = scheme req.URL.Host = i.sidecarHost return nil // no post-hook needed diff --git a/extension/transport/sidecar/interceptor_test.go b/extension/transport/sidecar/interceptor_test.go index 2cb0def1f..9f882e67f 100644 --- a/extension/transport/sidecar/interceptor_test.go +++ b/extension/transport/sidecar/interceptor_test.go @@ -7,11 +7,13 @@ package sidecar import ( "bytes" + "context" "errors" "io" "net/http" "testing" + "github.com/larksuite/cli/internal/envvars" "github.com/larksuite/cli/sidecar" ) @@ -97,6 +99,54 @@ func TestInterceptor_PreRoundTrip(t *testing.T) { } } +// TestInterceptor_PreRoundTrip_HTTPS verifies that a remote (TLS) sidecar +// rewrites the request to https://, while still preserving the +// original target and signing the request. +func TestInterceptor_PreRoundTrip_HTTPS(t *testing.T) { + key := []byte("test-key-for-hmac-signing-32byte!") + interceptor := &Interceptor{key: key, sidecarHost: "sidecar.mycorp.com", sidecarScheme: "https"} + + req, _ := http.NewRequest("GET", "https://open.feishu.cn/open-apis/im/v1/chats", nil) + req.Header.Set("Authorization", "Bearer "+sidecar.SentinelUAT) + + interceptor.PreRoundTrip(req) + + if req.URL.Scheme != "https" { + t.Errorf("scheme = %q, want %q", req.URL.Scheme, "https") + } + if req.URL.Host != "sidecar.mycorp.com" { + t.Errorf("host = %q, want %q", req.URL.Host, "sidecar.mycorp.com") + } + // Original target still preserved for the sidecar to forward upstream. + if target := req.Header.Get(sidecar.HeaderProxyTarget); target != "https://open.feishu.cn" { + t.Errorf("target = %q, want %q", target, "https://open.feishu.cn") + } + // Request is still signed. + if sig := req.Header.Get(sidecar.HeaderProxySignature); sig == "" { + t.Error("signature header should be set") + } +} + +// TestResolveInterceptor_HTTPSScheme pins the end-to-end env→scheme path: a +// (mixed-case) https proxy address must produce an interceptor that rewrites to +// https, never silently downgrading a remote sidecar to plaintext http. +func TestResolveInterceptor_HTTPSScheme(t *testing.T) { + t.Setenv(envvars.CliAuthProxy, "HTTPS://sidecar.mycorp.com") // uppercase on purpose + t.Setenv(envvars.CliProxyKey, "key") + + ic := (&Provider{}).ResolveInterceptor(context.Background()) + si, ok := ic.(*Interceptor) + if !ok || si == nil { + t.Fatalf("expected *Interceptor, got %T", ic) + } + if si.sidecarScheme != "https" { + t.Errorf("sidecarScheme = %q, want %q (uppercase HTTPS must not downgrade)", si.sidecarScheme, "https") + } + if si.sidecarHost != "sidecar.mycorp.com" { + t.Errorf("sidecarHost = %q, want %q", si.sidecarHost, "sidecar.mycorp.com") + } +} + func TestInterceptor_BotIdentity(t *testing.T) { interceptor := &Interceptor{key: []byte("key"), sidecarHost: "127.0.0.1:16384"} diff --git a/internal/envvars/envvars.go b/internal/envvars/envvars.go index 7b4a23464..a290db158 100644 --- a/internal/envvars/envvars.go +++ b/internal/envvars/envvars.go @@ -13,7 +13,7 @@ const ( CliStrictMode = "LARKSUITE_CLI_STRICT_MODE" // Sidecar proxy (auth proxy mode) - CliAuthProxy = "LARKSUITE_CLI_AUTH_PROXY" // sidecar HTTP address, e.g. "http://127.0.0.1:16384" + CliAuthProxy = "LARKSUITE_CLI_AUTH_PROXY" // sidecar address http(s)://host[:port]; plaintext http is same-host only, a remote sidecar must use https. e.g. "http://127.0.0.1:16384" or "https://sidecar.mycorp.com" CliProxyKey = "LARKSUITE_CLI_PROXY_KEY" // HMAC signing key shared with sidecar // Content safety scanning mode diff --git a/sidecar/hmac_test.go b/sidecar/hmac_test.go index f5a691c2e..247f372d7 100644 --- a/sidecar/hmac_test.go +++ b/sidecar/hmac_test.go @@ -161,6 +161,11 @@ func TestValidateProxyAddr(t *testing.T) { "http://gateway.docker.internal:16384", // trailing slash is tolerated "http://127.0.0.1:8080/", + // https: any valid host (including remote, cross-machine) is allowed + "https://127.0.0.1:16384", + "https://sidecar.mycorp.com", + "https://sidecar.mycorp.com:8443", + "https://sidecar.corp.internal:443/", } for _, addr := range valid { if err := ValidateProxyAddr(addr); err != nil { @@ -242,6 +247,8 @@ func TestValidateProxyAddr_RejectsUserinfo(t *testing.T) { "http://user@127.0.0.1:16384", "http://user:pass@127.0.0.1:16384", "http://127.0.0.1@attacker.com:16384", + "https://x@evil.com", + "https://user:pass@sidecar.mycorp.com", } { err := ValidateProxyAddr(addr) if err == nil { @@ -259,23 +266,99 @@ func TestValidateProxyAddr_RejectsUserinfo(t *testing.T) { } } -// TestValidateProxyAddr_HTTPSRejected pins the current contract: https is -// rejected explicitly (not lumped into a generic "bad scheme" error) because -// the interceptor hardcodes http and would silently downgrade an https URL -// otherwise. The message must mention https so users understand why their -// perfectly-looking config is refused. -func TestValidateProxyAddr_HTTPSRejected(t *testing.T) { +// TestValidateProxyAddr_HTTPSAllowed pins the contract: https addresses are +// accepted, including a remote sidecar on another machine. TLS provides +// confidentiality over the network and the HMAC signature provides +// integrity/auth, so cross-machine https is supported. +func TestValidateProxyAddr_HTTPSAllowed(t *testing.T) { for _, addr := range []string{ - "https://127.0.0.1:16384", + "https://127.0.0.1:16384", // same-host over TLS "https://sidecar.corp.internal:443", + "https://sidecar.mycorp.com", // remote, no explicit port + "https://sidecar.mycorp.com:8443", + } { + if err := ValidateProxyAddr(addr); err != nil { + t.Errorf("ValidateProxyAddr(%q): expected accepted, got: %v", addr, err) + } + } +} + +// TestValidateProxyAddr_HTTPRemoteRejected: plaintext http to a non-same-host +// address stays rejected — a remote sidecar must use https. +func TestValidateProxyAddr_HTTPRemoteRejected(t *testing.T) { + for _, addr := range []string{ + "http://sidecar.mycorp.com", + "http://sidecar.mycorp.com:8080", + "http://10.0.0.1:16384", } { err := ValidateProxyAddr(addr) if err == nil { - t.Errorf("ValidateProxyAddr(%q): expected error, got nil", addr) + t.Errorf("ValidateProxyAddr(%q): expected rejection (http remote), got nil", addr) continue } - if !strings.Contains(err.Error(), "https") { - t.Errorf("ValidateProxyAddr(%q): error should mention https, got: %v", addr, err) + msg := err.Error() + if !strings.Contains(msg, "https") && !strings.Contains(msg, "same-host") && !strings.Contains(msg, "loopback") { + t.Errorf("ValidateProxyAddr(%q): error should point to https/same-host, got: %v", addr, err) + } + } +} + +// TestProxyScheme: scheme is https only for https:// addresses, http otherwise. +// Case-insensitive: HTTPS:// must resolve to https, otherwise a remote sidecar +// would silently downgrade to plaintext http (see ProxyScheme doc). +func TestProxyScheme(t *testing.T) { + tests := map[string]string{ + "https://sidecar.mycorp.com": "https", + "https://127.0.0.1:16384": "https", + "http://127.0.0.1:16384": "http", + "127.0.0.1:16384": "http", + // case-insensitive scheme + "HTTPS://sidecar.mycorp.com": "https", + "Https://sidecar.mycorp.com": "https", + "HtTp://127.0.0.1:16384": "http", + } + for in, want := range tests { + if got := ProxyScheme(in); got != want { + t.Errorf("ProxyScheme(%q) = %q, want %q", in, got, want) + } + } +} + +// TestValidateProxyAddr_SchemeCaseInsensitive: mixed-case scheme must follow the +// same policy as lower-case — HTTPS accepted (remote allowed), HTTP remote +// rejected — so case can't be used to bypass the plaintext same-host rule. +func TestValidateProxyAddr_SchemeCaseInsensitive(t *testing.T) { + for _, addr := range []string{"HTTPS://sidecar.mycorp.com", "Https://sidecar.corp.internal:443"} { + if err := ValidateProxyAddr(addr); err != nil { + t.Errorf("ValidateProxyAddr(%q): expected accepted, got: %v", addr, err) + } + } + for _, addr := range []string{"HtTp://sidecar.mycorp.com", "HTTP://10.0.0.1:16384"} { + if err := ValidateProxyAddr(addr); err == nil { + t.Errorf("ValidateProxyAddr(%q): expected rejection (http remote), got nil", addr) + } + } +} + +// TestValidateProxyAddr_IPv6HTTPS pins IPv6 https forms. +func TestValidateProxyAddr_IPv6HTTPS(t *testing.T) { + for _, addr := range []string{"https://[::1]:443", "https://[::1]"} { + if err := ValidateProxyAddr(addr); err != nil { + t.Errorf("ValidateProxyAddr(%q): expected accepted, got: %v", addr, err) + } + } +} + +// TestValidateProxyAddr_RejectsQueryFragment: a proxy address must not carry a +// query or fragment, for either scheme. +func TestValidateProxyAddr_RejectsQueryFragment(t *testing.T) { + for _, addr := range []string{ + "https://sidecar.mycorp.com?x=1", + "https://sidecar.mycorp.com#frag", + "http://127.0.0.1:16384?x=1", + } { + if err := ValidateProxyAddr(addr); err == nil { + t.Errorf("ValidateProxyAddr(%q): expected rejection, got nil", addr) } } } @@ -289,6 +372,10 @@ func TestProxyHost(t *testing.T) { {"http://0.0.0.0:8080", "0.0.0.0:8080"}, {"http://host.docker.internal:16384/", "host.docker.internal:16384"}, {"127.0.0.1:16384", "127.0.0.1:16384"}, // no scheme + // https forms (remote sidecar) + {"https://sidecar.mycorp.com", "sidecar.mycorp.com"}, + {"https://sidecar.mycorp.com:8443/", "sidecar.mycorp.com:8443"}, + {"HTTPS://sidecar.mycorp.com", "sidecar.mycorp.com"}, } for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { diff --git a/sidecar/protocol.go b/sidecar/protocol.go index 4b8f9c62f..c74138d47 100644 --- a/sidecar/protocol.go +++ b/sidecar/protocol.go @@ -3,7 +3,8 @@ // Package sidecar defines the wire protocol shared between the CLI client // (running inside a sandbox) and the auth sidecar proxy (running in a -// trusted environment). Communication uses plain HTTP. +// trusted environment). Communication uses HTTP for a same-host sidecar, or +// HTTPS (TLS) for a remote sidecar. package sidecar import ( @@ -103,32 +104,31 @@ func isSameHost(host string) bool { return false } -// errNotSameHost is the shared error returned when the sidecar address does -// not resolve to the same physical host as the sandbox. Kept in one place so -// tests can look for a stable marker. +// errNotSameHost is the shared error returned when a plaintext (http) sidecar +// address does not resolve to the same physical host as the sandbox. Kept in +// one place so tests can look for a stable marker. func errNotSameHost(addr string) error { - return fmt.Errorf("invalid proxy address %q: host must be loopback "+ - "(127.0.0.1 / ::1) or a recognized same-host alias "+ + return fmt.Errorf("invalid proxy address %q: a plaintext (http) sidecar must be "+ + "loopback (127.0.0.1 / ::1) or a recognized same-host alias "+ "(localhost, host.docker.internal, host.containers.internal, "+ "host.lima.internal, gateway.docker.internal). "+ - "The sidecar must run on the same physical machine as the sandbox — "+ - "cross-machine deployment is not a sidecar and is not supported", addr) + "For a remote sidecar on another machine, use an https:// address instead", addr) } // ValidateProxyAddr validates the LARKSUITE_CLI_AUTH_PROXY value. // Accepted formats: -// - http://host:port -// - host:port (bare address, treated as http) +// - https://host[:port] (remote sidecar; cross-machine allowed) +// - http://host:port (plaintext; same-host only) +// - host:port (bare address, treated as plaintext http; same-host only) // -// Host must be loopback or in sameHostAliases. The sidecar pattern is -// inherently same-machine; cross-machine deployment is a different product -// and is not supported by this feature. -// -// https:// is rejected because sidecar is a same-host pattern: loopback -// and virtual same-host bridges don't traverse any untrusted medium, so -// TLS adds no security. Cross-machine deployment is out of scope (see the -// host constraint above), so there is no scenario today where https -// provides a real benefit over http on loopback. +// Scheme policy: +// - https:// — any valid host is allowed, including a remote central sidecar +// on another machine. TLS provides confidentiality over the untrusted +// network; the per-request HMAC signature provides integrity/auth. +// - http:// (or bare host:port) — plaintext, allowed only when the host is +// loopback (127.0.0.1 / ::1) or a recognized same-host alias (a virtual +// same-host bridge that stays on the physical machine). For a remote +// sidecar, use an https:// address instead. // // userinfo (user:pass@) is rejected unconditionally — the sidecar protocol // does not use basic auth, and the syntactic slot exists only as a phishing @@ -140,11 +140,11 @@ func ValidateProxyAddr(addr string) error { return fmt.Errorf("proxy address is empty") } - // Bare host:port (no scheme) — validate as a net address. + // Bare host:port (no scheme) — treated as plaintext http, so same-host only. if !strings.Contains(addr, "://") { host, port, err := net.SplitHostPort(addr) if err != nil { - return fmt.Errorf("invalid proxy address %q: expected host:port or http://host:port", addr) + return fmt.Errorf("invalid proxy address %q: expected host:port or http(s)://host[:port]", addr) } if host == "" || port == "" { return fmt.Errorf("invalid proxy address %q: host and port must not be empty", addr) @@ -159,33 +159,47 @@ func ValidateProxyAddr(addr string) error { if err != nil { return fmt.Errorf("invalid proxy address %q: %w", addr, err) } + // userinfo (user:pass@) is rejected unconditionally (phishing vector). if u.User != nil { return fmt.Errorf("invalid proxy address %q: userinfo is not allowed", addr) } - if u.Scheme == "https" { - return fmt.Errorf("invalid proxy address %q: use http:// — sidecar is "+ - "same-host only (loopback or virtual same-host bridge), so TLS adds "+ - "no security; cross-machine deployment is out of scope", addr) - } - if u.Scheme != "http" { - return fmt.Errorf("invalid proxy address %q: scheme must be http", addr) - } if u.Host == "" { return fmt.Errorf("invalid proxy address %q: missing host", addr) } if u.Path != "" && u.Path != "/" { return fmt.Errorf("invalid proxy address %q: path is not allowed", addr) } - // u.Hostname() strips the port and unwraps IPv6 brackets. - if !isSameHost(u.Hostname()) { - return errNotSameHost(addr) + if u.RawQuery != "" { + return fmt.Errorf("invalid proxy address %q: query is not allowed", addr) + } + if u.Fragment != "" { + return fmt.Errorf("invalid proxy address %q: fragment is not allowed", addr) + } + + switch u.Scheme { + case "https": + // Remote sidecar over TLS. Cross-machine is allowed: https provides + // confidentiality over the network and the per-request HMAC signature + // provides integrity/authentication, so a remote central sidecar is + // supported without exposing credentials or signing material in clear. + return nil + case "http": + // Plaintext: only safe on the same physical host (loopback or a virtual + // same-host bridge). For a remote sidecar use an https:// address. + // u.Hostname() strips the port and unwraps IPv6 brackets. + if !isSameHost(u.Hostname()) { + return errNotSameHost(addr) + } + return nil + default: + return fmt.Errorf("invalid proxy address %q: scheme must be http or https", addr) } - return nil } // ProxyHost extracts the host:port from an AUTH_PROXY URL. -// Input is expected to be an HTTP URL like "http://127.0.0.1:16384". -// Returns the host:port portion for URL rewriting. +// Input is expected to be an http:// or https:// URL like +// "http://127.0.0.1:16384" or "https://sidecar.mycorp.com". +// Returns the host[:port] portion for URL rewriting. func ProxyHost(authProxy string) string { // Strip scheme host := authProxy @@ -196,3 +210,19 @@ func ProxyHost(authProxy string) string { host = strings.TrimRight(host, "/") return host } + +// ProxyScheme returns the URL scheme the CLI must use when routing to the +// sidecar: "https" for a TLS (remote) sidecar, otherwise "http" (same-host +// plaintext). Input is a value already accepted by ValidateProxyAddr. +// +// It parses the address (rather than a case-sensitive prefix check) so the +// result stays consistent with ValidateProxyAddr, which relies on url.Parse +// normalizing the scheme. Otherwise "HTTPS://host" — accepted as https by +// ValidateProxyAddr — would silently downgrade to plaintext http here, +// breaking the "remote must use TLS" boundary. +func ProxyScheme(authProxy string) string { + if u, err := url.Parse(authProxy); err == nil && strings.EqualFold(u.Scheme, "https") { + return "https" + } + return "http" +} diff --git a/sidecar/server-demo/README.md b/sidecar/server-demo/README.md index 23516a70d..3999a0d5a 100644 --- a/sidecar/server-demo/README.md +++ b/sidecar/server-demo/README.md @@ -114,18 +114,23 @@ export LARKSUITE_CLI_STRICT_MODE="user" # optional: lock sandbox to o **`LARKSUITE_CLI_AUTH_PROXY` constraints** — validated by the CLI on startup: -- Scheme must be `http://` (or bare `host:port`). `https://` is rejected - today because the interceptor does not yet perform TLS; a future PR that - wires up real TLS will relax this. -- Host must be loopback (`127.0.0.1`, `::1`) or one of the recognized - same-host aliases: `localhost`, `host.docker.internal`, - `host.containers.internal`, `host.lima.internal`, `gateway.docker.internal`. - The sidecar pattern is inherently same-machine; cross-machine deployment - is a different product (auth broker / STS) with different security - requirements (mTLS, cert rotation, per-client keys) and is not supported - by this feature. +- Scheme must be `http://` / `https://` (or bare `host:port`, treated as + plaintext http). + - `https://` is allowed, **including a remote sidecar on another + machine**: TLS provides confidentiality over the network and the + per-request HMAC signature provides integrity/authentication. + - Plaintext `http://` (and bare `host:port`) is allowed **only same-host**: + loopback (`127.0.0.1`, `::1`) or a recognized same-host alias + (`localhost`, `host.docker.internal`, `host.containers.internal`, + `host.lima.internal`, `gateway.docker.internal`). For a remote sidecar, + use an `https://` address. - No path, query, fragment, or `user:pass@` in the URL. +> Note: this demo server itself terminates plain HTTP and is meant to run +> locally. A production **remote** sidecar must terminate TLS (its own +> `https://` endpoint, e.g. behind a load balancer or with a real +> certificate); the CLI-side policy above is what enables pointing at it. + **How auto identity detection works in sidecar mode**: on every invocation the CLI asks the sidecar to look up the logged-in user's `open_id` via `/open-apis/authen/v1/user_info`. If that succeeds, `--as` defaults to `user`;