Skip to content

feat(sidecar): support remote HTTPS sidecar addresses#1225

Open
sang-neo03 wants to merge 2 commits into
mainfrom
feat/sidecar-remote-https
Open

feat(sidecar): support remote HTTPS sidecar addresses#1225
sang-neo03 wants to merge 2 commits into
mainfrom
feat/sidecar-remote-https

Conversation

@sang-neo03
Copy link
Copy Markdown
Collaborator

@sang-neo03 sang-neo03 commented Jun 2, 2026

Summary

Relax the auth-sidecar proxy address policy so the CLI can route through a remote central sidecar over TLS, while keeping the existing same-host plaintext sidecar working unchanged.

Previously LARKSUITE_CLI_AUTH_PROXY only accepted a same-host (loopback / alias) plaintext http:// address; https:// and cross-machine hosts were rejected. That blocked a remote/central sidecar deployment, even though the multi-tenant sidecar pattern can route requests to a gateway and attach a trusted per-user identity.

Policy after this change

Address Allowed Why
https://<any-host> (incl. remote / cross-machine) TLS gives confidentiality over the network; the per-request HMAC signature gives integrity/auth
http://<same-host> or bare host:port plaintext, same-host only (loopback / recognized same-host alias)
http://<remote-host> a remote sidecar must use https
userinfo / path / query / fragment rejected

Same-host behavior is unchanged (backward compatible).

Changes

  • sidecar/protocol.go: rewrite ValidateProxyAddr scheme policy; add ProxyScheme (parses the address so a mixed-case HTTPS:// cannot silently downgrade to plaintext); refresh package / errNotSameHost / doc comments.
  • extension/transport/sidecar/interceptor.go: the interceptor now carries the resolved scheme and rewrites req.URL.Scheme to https for a remote sidecar (was hardcoded http).
  • internal/envvars/envvars.go, sidecar/server-demo/README.md: doc updates for the new policy.

Tests

  • New / updated unit tests: case-insensitive scheme (validate + ProxyScheme + end-to-end interceptor), IPv6 https, remote-http rejection, https userinfo rejection, query/fragment rejection, ProxyHost https forms.
  • go build ./... and go build -tags authsidecar ./... pass; go vet clean.
  • go test ./sidecar/... and go test -tags authsidecar ./extension/transport/sidecar/... ./extension/credential/sidecar/... all pass.
  • Manual: a -tags authsidecar build now accepts https://sidecar.mycorp.com (and mixed-case HTTPS://…) where it previously errored.

Security notes

The boundary is "plaintext only same-host; cross-machine requires TLS". The CLI→sidecar hop carries only a sentinel placeholder + HMAC signature (no real token); the real token is injected by the trusted sidecar. Remote plaintext stays rejected so signed / credential-bearing traffic is never sent in the clear.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Support for HTTPS/TLS connections to remote sidecars while preserving HTTP for same-host sidecars; proxy scheme is honored case-insensitively.
  • Documentation

    • Clarified configuration guidance: HTTP for local sidecars, HTTPS required for remote sidecars; updated examples and notes.
  • Tests

    • Expanded tests covering proxy scheme validation, scheme normalization, and remote vs. local proxy behavior.

Relax the auth-sidecar proxy address policy so a remote central sidecar
reachable over TLS can be used, while keeping existing same-host plaintext
behavior unchanged.

- ValidateProxyAddr: allow https:// to any host (cross-machine); http://
  and bare host:port stay same-host only; userinfo/path/query/fragment
  remain rejected.
- Add ProxyScheme and route the interceptor URL rewrite through the
  configured scheme (https for remote, http for same-host). ProxyScheme
  parses the address so a mixed-case HTTPS:// cannot silently downgrade to
  plaintext HTTP.
- Update LARKSUITE_CLI_AUTH_PROXY doc and server-demo README for the new
  policy; refresh the package comment.
- Tests: case-insensitive scheme, IPv6 https, https userinfo rejection,
  query/fragment rejection, ProxyHost https forms, and end-to-end
  interceptor scheme selection.
@sang-neo03 sang-neo03 added the enhancement New feature or request label Jun 2, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2d1e219b-8d0a-42eb-b9b4-b6214d46b2cb

📥 Commits

Reviewing files that changed from the base of the PR and between 07da0c8 and 52dc09a.

📒 Files selected for processing (1)
  • sidecar/hmac_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • sidecar/hmac_test.go

📝 Walkthrough

Walkthrough

This PR extends sidecar proxy support from HTTP-only to dual HTTP/HTTPS by extracting and propagating the configured proxy URL scheme; it tightens proxy validation, updates the interceptor to honor the scheme, and expands tests and docs to reflect same-host HTTP vs remote HTTPS rules.

Changes

HTTPS support for remote sidecars

Layer / File(s) Summary
Protocol layer: scheme extraction and validation
sidecar/protocol.go
Adds ProxyScheme() and updates ValidateProxyAddr() to accept https:// for remote sidecars while enforcing same-host-only for http://, rejecting userinfo, non-root paths, queries, and fragments.
Protocol validation test coverage
sidecar/hmac_test.go
Expands tests to validate HTTPS acceptance (including remote), rejection of remote plaintext HTTP, case-insensitive scheme handling, IPv6 HTTPS forms, and scheme-stripping behavior in ProxyHost.
Interceptor scheme propagation
extension/transport/sidecar/interceptor.go
Adds sidecarScheme to Interceptor, derives it in ResolveInterceptor(), and uses it in PreRoundTrip() to set req.URL.Scheme (defaulting to http when unset).
Interceptor test coverage
extension/transport/sidecar/interceptor_test.go
Adds tests verifying HTTPS request rewriting to remote sidecars while preserving the original target and validating scheme derivation from uppercase HTTPS:// proxy URLs.
Configuration and documentation updates
internal/envvars/envvars.go, sidecar/server-demo/README.md
Clarifies LARKSUITE_CLI_AUTH_PROXY/CliAuthProxy documentation to specify HTTP for same-host and HTTPS for remote sidecars; updates demo notes about local plain-HTTP termination vs production TLS.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • larksuite/cli#532: Introduced the original sidecar transport client logic; this PR extends that work to support HTTPS for remote sidecars.

Suggested labels

size/M

Suggested reviewers

  • liangshuo-1
  • albertnusouo

Poem

🐰 A rabbit hums about schemes and hops,
Local paths stay plain where the garden stops.
For distant nests, wrap TLS tight,
One URL decides the scheme tonight.
Hops, schemes, and signatures — all set right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly describes the main change: enabling support for remote HTTPS sidecar addresses, which is the core objective of this PR.
Description check ✅ Passed The description is comprehensive and well-structured, covering all required sections: summary, changes, test plan with verification results, and security notes. It thoroughly explains the policy shift and implementation details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/sidecar-remote-https

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the size/M Single-domain feat or fix with limited business impact label Jun 2, 2026
Align comment spacing flagged by the fast-gate gofmt check.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@52dc09af954a9030ff3608aa11ad91ef0779a292

🧩 Skill update

npx skills add larksuite/cli#feat/sidecar-remote-https -y -g

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.20%. Comparing base (0aa9e96) to head (52dc09a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1225   +/-   ##
=======================================
  Coverage   69.19%   69.20%           
=======================================
  Files         634      634           
  Lines       59482    59489    +7     
=======================================
+ Hits        41161    41168    +7     
  Misses      15007    15007           
  Partials     3314     3314           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant