Skip to content

feat(cubenet): support arbitrary custom L7 port + scheme in egress rules - #1347

Merged
zhouxianping merged 31 commits into
TencentCloud:masterfrom
zhouxianping:feat/custom_l7_port
Aug 18, 2026
Merged

feat(cubenet): support arbitrary custom L7 port + scheme in egress rules#1347
zhouxianping merged 31 commits into
TencentCloud:masterfrom
zhouxianping:feat/custom_l7_port

Conversation

@zhouxianping

Copy link
Copy Markdown
Collaborator

Summary

L7 egress rules can now pin the TCP port CubeEgress intercepts on, via
Match.port + Match.scheme. This extends the existing host/path/SNI
matching so a rule applies to a specific (host, port, scheme) tuple instead
of always the default {80/http, 443/https} set.

Changes by layer

  • CubeNet dataplane (mvmtap/session/tcp): port+scheme → skb->mark
    on egress SYN; spec tests updated.
  • CubeEgress: Lua policy + iptables mark validation for custom
    (host, port, scheme).
  • Cubelet: embedded network runtime + policy builder handle port/scheme.
  • CubeAPI / CubeMaster: validate egress rule port/scheme at API and master.
  • SDKs (go / node / python): port/scheme validation in egress rule match.
  • deploy/one-click: bind L7 skb-mark config into the install flow.
  • docs (security-proxy.md EN + ZH): document custom L7 ports, the four
    port/scheme quadrants, and constraints.
  • examples: L7 custom-port echo demos.

SDK compatibility e2e (this session)

  • cases/network/test_l7_custom_port.py: 5 cases — custom HTTP port injection,
    scheme-only default-set interception, HTTPS custom-port interception,
    port-without-scheme rejection, subnet-host rejection.
  • framework/capabilities.py: NETWORK_L7_CUSTOM_PORT.
  • CubeNet/cubevs/Makefile: test: gen so stale BPF objects are regenerated
    before make test (prevents verifier failures from outdated .o).

Validation

  • go test ./... under CubeNet/cubevs passes (after make gen).
  • SDK compatibility e2e: test_l7_custom_port.py 5 passed against a template
    that trusts the L7 interception CA.

hankzhou added 13 commits August 14, 2026 16:28
Port the L7 custom-port feature dataplane from L7_support_portconfig
(gitwoa repo) onto the restructured master (TencentCloud#1285):

- V2 -> V3 allow_out migration: net_policy_value_v3 with port in the
  LPM key, key_prefixlen exact-key merge, static-wins expiry semantics
- dns_allow -> dns_allow_v2 rename with legacy/current migration and
  crash-safe rollback
- L7 (host, port, scheme) plan builder: canonical host grouping,
  per-port scheme conflict detection, /48 entry budget accounting
- BPF dataplane: mvmtap/session/tcp/dns_response/dns_query updates,
  legacy default-port drain shim, L7 mark configurability
- Spec tests: DNS learn (kernel BPF), migration rollback over bpffs,
  buildL7Plan matrix, static-wins merge matrix (100 pass, 0 fail)

Reconciliation with TencentCloud#1285: keep the restructured lifecycle split
(CleanupTAPDevicePolicy / InstallTAPDefaultDenyPolicy /
cleanupDNSPolicyFlags, UpsertTAPDeviceMetadata / DeleteTAPDevice)
and re-apply the branch changes on top.

Note: Cubelet/network/runtime does not compile at this commit because
MVMOptions.L7AllowOut changed from *[]string to *[]L7Target; fixed in
the follow-up Cubelet runtime port.

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
Port the L7 custom-port control plane from L7_support_portconfig
(gitwoa network-agent) onto the embedded Cubelet network runtime:

- cubebox.proto (Cubelet + CubeMaster): optional int32 port = 8 on
  EgressRuleMatch; regenerate pb.go with protoc v5.28.3 header kept
- runtime/types.go: EgressRuleMatch.Port *int with the port+scheme
  pairing contract (port requires scheme; both omitted means the legacy
  {80/http, 443/https} default set)
- plugin_policy.go: map proto port into runtime match
- cubeegress wire/adapter: carry Port into MatchInput and render it as
  a numeric JSON field for access_phase.lua dst_port comparison
- policy_builder.go: project SNI+Host rules into []cubevs.L7Target
  (host, port, scheme) with per-rule port/scheme validation
  (extractL7PortScheme, defaultPortForScheme); an invalid pair rejects
  the whole projection so cubevs and CubeEgress never diverge
- controller.go: loadL7MarksConfig overlays CUBE_L7_MARK_{HTTP,HTTPS,MASK}
  from /etc/cubeegress/l7-marks.conf onto cubevs.Params before Init,
  tolerating shell-legal export prefixes and inline comments
- tests: TestExtractL7PortScheme matrix (13 cases), whole-policy
  rejection, L7Target projection shape, TestLoadL7MarksConfig (6 cases)

Note: push/delete semantics keep the restructured fail-fast model from
TencentCloud#1285 (pushEgressForState failure aborts creation; Cleaning state
retries) instead of the legacy best-effort pendingEgressPush model.

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
…lidation

Port the CubeEgress half of the L7 custom-port feature from
L7_support_portconfig (gitwoa):

- lua/port_scheme.lua (new): (port, scheme) normalisation, validation
  and default-set expansion shared by policy and access phases
- lua/policy.lua: validate and render match.port / match.scheme on
  egress rules
- lua/access_phase.lua: compare match.port against ctx.dst_port from
  the tproxy listener so custom-port rules only match their own tuple
- scripts/cube-proxy-iptables-init.sh: validate CUBE_L7_MARK_{HTTP,
  HTTPS,MASK} (marks must differ, bits within mask) before installing
  TPROXY rules
- Makefile: test-lua runs port_scheme_test and port_scheme_extra_test
- tests/: port_scheme_test.lua, port_scheme_extra_test.lua and
  cube-proxy-iptables-init_test.sh (all passing)

All four modified files are unchanged since the branch base (43fa0bf);
the TencentCloud#1285 admin-port move (9090->9091) touched admin.lua, nginx.conf
and start.sh only, so no reconciliation was needed.

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
… master layers

Port the validation layers of the L7 custom-port feature from
L7_support_portconfig (gitwoa):

- CubeAPI models/mod.rs: EgressRuleMatch.port Option<i32> with the
  port+scheme pairing contract documented on the struct
- CubeAPI cubemaster/mod.rs: CubeEgressRuleMatch.port forwarded to the
  master wire format
- CubeAPI services/sandboxes.rs: validate_egress_rule_match rejects
  out-of-range ports, port-without-scheme, and non-http(s) schemes
  before the request reaches CubeMaster; map_egress_rule carries port
  through; 4 new unit tests
- CubeMaster types/types.go: EgressRuleMatch.Port *int, DeepCopy and
  cloneIntPtr cover the new field
- CubeMaster service/sandbox/util.go: checkParam calls
  validateEgressRuleMatch per rule (same contract as CubeAPI);
  mapEgressRuleMatch maps Port into the cubebox proto
- Tests: TestValidateEgressRuleMatch, CLI merge test preserving
  Rules/AllowPublicTraffic with deep-copy leak assertions, and
  templatecenter TestCloneEgressRuleDeepCopiesPort

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
Port the Python SDK validation layer of the L7 custom-port feature
from L7_support_portconfig (gitwoa):

- cubesandbox/_policy.py: port/scheme validation on egress rule
  matches (port in [1, 65535], port requires scheme, scheme must be
  http/https case-insensitively), matching the CubeAPI and CubeMaster
  server-side contract
- tests/test_policy.py (new): 27 cases covering the validation matrix
  and E2B per-host rules compat
- tests/test_l7_custom_port_e2e.py (new): live custom-port end-to-end
- tests/test_l7_custom_port_validation_e2e.py (new): API-edge
  validation end-to-end (skips without a cluster)

_policy.py was untouched since the branch base; the new test files do
not collide with the target test layout (test_sandbox/test_volume).

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
Align the TypeScript SDK with the Python SDK contract for the L7
custom-port feature (the original branch never touched sdk/node):

- Match gains port?: number with the pairing contract documented
- validateScheme normalizes (strip + lowercase) and enforces the
  http/https whitelist; the wire form always carries the canonical
  lowercase scheme
- validateMatchPortScheme enforces: integer port in [1, 65535], port
  requires scheme; scheme alone stays legal (legacy {80,443} filter)
- serializeMatch validates through the single serialization funnel so
  every rule path (typed rules and E2B-converted rules) is checked
  before the network round-trip
- tests: wire emission, scheme normalization, missing-scheme and
  out-of-range/non-integer port rejection, unknown scheme rejection,
  legacy scheme-only acceptance

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
Align the Go SDK with the Python/TypeScript SDK contract for the L7
custom-port feature (the original branch never touched sdk/go):

- Match gains Port int with the pairing contract documented
- Match.validate enforces the http/https whitelist (case-insensitive),
  the [1, 65535] port range, and port-requires-scheme; scheme alone
  stays legal (legacy {80,443} filter)
- buildCreatePayload validates and normalizes each rule match before
  serialization, with errors located by rule index and name; the wire
  form carries the canonical lowercase scheme
- tests: wire serialization with scheme normalization, invalid
  port/scheme rejection matrix (asserting no network call is made),
  and legacy scheme-only acceptance

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
Port the one-click deployment binding of the L7 custom-port feature
from L7_support_portconfig (gitwoa), adapted to the TencentCloud#1285 embedded
network runtime wording:

- install.sh: write_l7_marks_conf persists /etc/cubeegress/l7-marks.conf
  from CUBE_L7_MARK_{HTTP,HTTPS,MASK} (shipped defaults
  0xCE010000/0xCE020000/0xFFFF0000) before the consuming systemd units
  start. Values are validated arithmetically so the same mark in
  different notations (hex case, decimal) is still rejected, and bits
  outside the mask are refused — matching cubevs.resolveL7Marks and
  the cube-proxy-iptables-init checks.
- env.example: documented override block for the three marks.
- README.md / README_zh.md: config placement table gains the
  l7-marks.conf row, referencing the embedded network runtime instead
  of the removed network-agent.

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
Port the L7 egress examples from L7_support_portconfig (gitwoa):

- cube-test-network.py: comprehensive L7 egress network demo
- network_l7_custom_port_echo.py: four-quadrant custom-port demo
  (default http/https, default both-set, custom http on 18080, custom
  https on 1012) with a self-contained local echo server leg

Both are new files; py_compile passes.

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
…cards

Final sweep of the L7_support_portconfig port — files caught by the
full branch-diff audit:

- CubeNet/cubevs/Makefile: widen BPF_GENS/BPF_OBJS wildcards to
  *_bpfel*.{go,o} so test-object artifacts (dnslearn/egresspolicy/
  l7mark/tcpstate) are covered by verify/clean, matching .gitignore
- CubeNet/cubevs/: port the three design documents
  (allow_out_v3_design.md, cubeegress_custom_port_design.md,
  cubeegress_custom_port_test_cases.md)
- Cubelet/doc/cubelet-api.md: add the EgressRuleMatch.port row
  (protoc-gen-doc unavailable locally; row added manually to match the
  regenerated proto)

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
Post-review comment fixes from the port consistency audit:

- tap.go: UpsertTAPDeviceMetadata comment referenced the old map
  names (allow_out_v2, dns_allow) — the actual maps are allow_out_v3
  and dns_allow_v2
- netpolicy.go: cleanupNetPolicy comment still named the pre-rename
  DelTAPDevice; the function is DeleteTAPDevice after the TencentCloud#1285
  reconciliation

No code changes; comment-only.

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
… port move

The restart readiness probe in cube-test-network.py still targeted the
pre-TencentCloud#1285 admin port :9090, which is now served by CubeProxy plaintext
gRPC on all-in-one hosts — the HTTP probe received gRPC frames and the
readiness check failed with BadStatusLine until the 60s timeout,
failing every restart-style case.

Point the probe at :9091 (CUBE_EGRESS_ADMIN_PORT default in
CubeEgress/start.sh + nginx.conf) and update the docstring references
(network-agent :19090 dump is now Cubelet /v1/policies/dump).

Verified live: full suite rerun has the moonshot inject case passing
with restart probe ready in ~1.2s.

Assisted-by: CodeBuddy:Kimi-K3

Signed-off-by: hankzhou <hankzhou@tencent.com>
- Add tests/e2e/sdk_compat/cases/network/test_l7_custom_port.py covering custom
  HTTP/HTTPS port interception, scheme-only default-set, and port/scheme +
  subnet-host create-time validation; add NETWORK_L7_CUSTOM_PORT capability.
- cubevs Makefile: regenerate BPF objects before make test (test: gen).
- docs: document custom L7 ports in the security-proxy guide (en/zh).
- Fold in dataplane policy/runtime touch-ups and drop superseded design docs.

Signed-off-by: hankzhou <hankzhou@tencent.com>
@fslongjin

Copy link
Copy Markdown
Member

Review of PR #1347 — arbitrary custom L7 port + scheme in egress rules

Overall this is a well-engineered change: the unified classify_egress_flow() verdict, the /48 port-in-key LPM trie, the key_prefixlen guard for exact-key merge, the fail-closed unknown-scheme handling, and the static asserts mirroring BPF structs are all high quality. The findings below are what I'd want addressed before merge, in severity order.

Correctness

1. [High] Migration rollback ordering can permanently lose allow_out policy
CubeNet/cubevs/migration.go:98-118 (with Init rollback at miscs.go:299-305)

migratePersistentPolicyMaps() deletes the legacy allow_out/allow_out_v2 pin (line 109) immediately after migrateAllowOutMap succeeds, before migrateDNSAllowMap (line 114). If the DNS migration then fails, Init's deferred rollback removes the freshly-populated allow_out_v3/dns_allow_v2 pins; on the next restart allowOutMigrationSource() returns "" (legacy pin is gone) and the allow_out policy is never re-migrated. The "leave the legacy pin in place so the migration can be retried" comment only covers an allow-map failure, not a later DNS failure. Suggestion: migrate both maps first and only unlink the legacy pins after both succeed. The existing rollback tests (TestMigrateDNSAllowMapFailureKeepsLegacyPin, TestMigrateAllowOutMapFailureKeepsLegacyPin) only exercise single-map failure and miss this combined case.

2. [High] Port-less deny rules are silently narrowed to {80/http, 443/https}; a custom-port allow can bypass a broader deny
CubeEgress/lua/access_phase.lua:144-152 via port_scheme.matches

Pre-PR, a deny rule with a host + scheme matched all intercepted traffic to that host. Post-PR, port_scheme.matches(nil, nil, dst_port, scheme) expands to only {80/http, 443/https}, so a deny rule with no port matches only the default set. Combined with a custom-port allow rule (e.g. deny host="*.example.com" + allow host="api.example.com", port=8443, scheme=https), api.example.com:8443 traffic is allowed because the deny rule never matches port 8443 and decide() is first-match-wins. For deny rules the narrowing is fail-open (for allow rules it's fail-closed). Consider making deny apply port-agnostically within the host, or at minimum documenting the narrowing prominently and auditing existing deny rules on upgrade.

3. [Medium] Non-atomic iptables chain install can leave a fail-open interception state
CubeEgress/scripts/cube-proxy-iptables-init.sh:124-144

The chain is built by iptables -F then two sequential -A TPROXY rules with no rollback and no post-condition check. If the second (HTTPS) rule fails after the first succeeds, the script aborts before install_routing(), so mark-based ip rule routing is never installed and HTTPS-marked packets fall through to the default route — a silent bypass of all L7 interception/deny for the HTTPS scheme. Consider building into a scratch chain + atomic swap (or verifying both rules are present before proceeding).

4. [Medium] Cubelet cloneEgressRules shallow-copies the new Match.Port pointer
Cubelet/network/runtime/policy_builder.go:47-51

match := *r.Match shares Port *int (and the pre-existing Host/Path/SNI/Scheme pointers) with the caller's request, despite the function contract claiming a deep copy. Method is correctly deep-copied. CubeMaster fixed this with cloneIntPtr (types.go:227); Cubelet should mirror it.

Test coverage / CI

5. [High] The new CubeNet/cubevs unit tests (~2000 lines, 8 files) are not executed by CI or run.sh
.github/workflows/unit-test-check.yml:185-189 maps CubeNet/*cubeletcubelet-pkg-test (go test -short ./pkg/... under Cubelet). CubeNet/cubevs is a separate Go module, so custom_port_test.go, dns_learn_test.go, egress_policy_test.go, l7_mark_test.go, migration_test.go, tcp_state_test.go, netpolicy_test.go, dump_test.go are never compiled/run. Add a go test ./... for the cubevs module to the workflow (and tests/unittest/run.sh).

6. [High] The FLOW_HTTP verdict — the primary new plaintext path — is never asserted
CubeNet/cubevs/egress_policy_test.go:12-17

The verdict constants define reject=0, SNAT=1, HTTPS=3 but no HTTP=2, and the seed cases only cover HTTPS. Add flowVerdictHTTP = 2 and a case seeding a /48 with Scheme: L7SchemeHTTP.

7. [High] iptables TPROXY rule generation is stubbed out in tests
CubeEgress/tests/cube-proxy-iptables-init_test.sh:98 overrides install_chain() with echo, so the actual two mark→TPROXY rules (--on-port 8080/8443, mark/mask match) and the gateway-from-CIDR arithmetic are never asserted. A regression in --on-port or the mark match would pass CI.

8. [Medium] Migration success path and dump port rendering are untested

  • CubeNet/cubevs/migration_test.go has full bpffs success tests for DNS migration but only rollback tests for allow_out_v2allow_out_v3; the 16-byte L7+expires → /48 expansion upgrade path is unproven.
  • CubeNet/cubevs/dump_test.go never exercises l7PortEntriesToDump / dumpPolicyInnerMap NBO→host port rendering; a byte-order regression would show 0x20EB instead of 8443.

9. [Medium] HTTPS custom-port e2e asserts nothing that distinguishes interception from passthrough
tests/e2e/sdk_compat/cases/network/test_l7_custom_port.py asserts only code=200 + non-empty body against a public endpoint. Plain SNAT would satisfy the same invariant, and the case is gated behind SDK_E2E_L7_HTTPS_CAP_TRUSTED=1 (silently skipped by default). Consider an endpoint that echoes a request-observable signal, or document that the leg only proves reachability.

Docs / conventions

10. [Medium] Non-English character in a Go comment
CubeNet/cubevs/dump.go:184Scheme string json:"scheme" // "http" | "https" | ""(未知) violates the project's non-English-character policy. Change to "" (unknown).

11. [Medium] "Enforced by the SDK client" overstates SDK coverage
docs/guide/security-proxy.md:148 (and ZH :135): the SDK clients only validate the port range + scheme + port-requires-scheme; the subnet-CIDR rejection, the (host, port) scheme-conflict rejection, and the 8-tuple cap are server-side only. Reword to "validated server-side; the SDK client also pre-validates the port/scheme pair".

Minor

12. [Low] Scheme-only rules silently narrow datapath interception to a single port
Cubelet/network/runtime/policy_builder.go:218 expands scheme-only https to port 443 only. Pre-PR the host stayed intercepted on the full {80,443} set and the scheme was a CubeEgress filter; with a CubeEgress default-deny sandbox, plaintext HTTP to that host now bypasses the proxy entirely. Docs were updated to the new semantics, but the Python SDK Match.__post_init__ docstring (sdk/python/cubesandbox/_policy.py:87-92) and the Go SDK test comment still describe the old behavior — callers will be misled.

13. [Low] Dead validation helpers with a contradictory counting model
CubeNet/cubevs/netpolicy.go:759-805validateNetPolicyEntryCounts/countUniqueLPMEntries/countUniqueDNSAllowEntries are only referenced from tests and count one entry per LPM key, contradicting expandedAllowOutEntryCount (per-port). If re-wired into production they'd reintroduce the E2BIG overflow the PR's own comment describes. Recommend deleting them.

14. [Low] from_envoy reply path now does 2 dependent hash lookups on every TCP reply packet
CubeNet/src/localgw.bpf.c:19-46update_l7_session_from_cubedev runs on every L7-proxy reply packet (pull headers + lookup_session + bpf_ktime_get_ns), where the previous from_envoy did zero map lookups. Gate on control-flag packets (syn||fin||rst) to remove the steady-state data/ACK cost; verify against the tcp-state tests before landing.

15. [Low] Cross-layer validation parity: CubeAPI scheme check doesn't trim whitespace
CubeAPI/src/services/sandboxes.rs:1107 uses eq_ignore_ascii_case without trim(), so " https " is rejected by CubeAPI but accepted by CubeMaster and all SDKs. Also: persistentPolicyGenerationExists (miscs.go:274-291) hard-fails with no self-healing when exactly one of allow_out_v3/dns_allow_v2 is pinned.


Thanks for the thorough spec-test harness and the e2e coverage — these are above average for the repo. The correctness items (#1, #2) and the CI gap (#5) are the ones I'd prioritize.

Comment thread CubeNet/src/session.h
key.prefixlen = 48;
key.ip = daddr;
key.port = dport;
value = bpf_map_lookup_elem(inner_map, &key);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor / plausible edge case: an expired DNS-learned /48 L7 entry shadows a live /32 static allow.

classify_egress_flow performs a single LPM lookup with key.prefixlen = 48. LPM returns the longest-prefix match regardless of the value's expiry, so when a DNS-learned /48 for (ip, port) has expired but has not yet been reaped, this lookup still returns that /48 (it out-ranks the /32). The expiry check then fails, and the function falls through to the deny_out / default-allow phase without ever consulting the /32.

Failure scenario: default-deny (deny_all 0.0.0.0/0), a static /32 plain allow for IP X, and a DNS-learned L7 /48 for (X, 80). While the /48 is expired-but-unreaped, a flow to (X, 80) is rejected — the static allow is effectively revoked — even though the rule author explicitly allowed X. Under default-allow this degrades harmlessly to FLOW_SNAT, which is why it's narrow; but the /32 fallback is unreachable by construction, not by design.

Suggested fix: when the matched value is present but expired, re-lookup with key.prefixlen = 32 (the plain /32/subnet fallback) before evaluating deny_out, or treat "matched but expired" as a miss so the LPM fallback to /32 applies.

Comment thread CubeNet/src/mvmtap.bpf.c
* No session is re-created, so each packet on the drained flow
* re-enters this path (per-packet socket lookup — acceptable for
* draining connections that will eventually close).
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor / plausible gap: custom-port L7 connections are RST'd on session loss even while the policy entry is still live.

This lost-session drain path only covers dport 80/443. For an established custom-port L7 flow whose egress_sessions entry is lost (e.g. CubeNet restarts and re-creates the session map while the pinned allow_out_v3 map survives), the socket lookup succeeds (the TPROXY socket is still ESTABLISHED) but the dport guard excludes it, and the packet falls through to return rst ? TCP_NAT_DROP : TCP_NAT_RESET.

The result is asymmetric: the same restart preserves established 80/443 connections via mark re-stamp + redirect, but resets an established custom-port connection — even though classify_egress_flow would still return FLOW_HTTP/FLOW_HTTPS for this flow, and a fresh SYN on the same tuple would be admitted and re-proxied.

The comment motivates the exclusion with "they should respect the current policy when their allow_out_v3 entry expires" — that covers the expired-policy case, but not the live-policy, lost-session case, where the RST contradicts the still-valid allow.

Suggested fix: in the !sess branch, for non-80/443 ports, consult classify_egress_flow first; if it still returns FLOW_HTTP/FLOW_HTTPS, re-create the L7 session (or at minimum re-stamp the mark and redirect), and fall back to reset only when the policy no longer allows the flow.

Comment thread CubeNet/cubevs/miscs.go Outdated
} else if !errors.Is(err, os.ErrNotExist) {
return false, err
}
if allowExists != dnsExists {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F1 (Medium · availability): half-pinned map generation permanently bricks startup.

When only one of the two new-generation pins exists, this returns an error, and Init returns immediately — before the deferred cleanup that removes orphaned new-generation pins has been registered (that defer is only installed inside the if !generationExists branch below). If a previous boot died between pinning allow_out_v3 and dns_allow_v2 (crash, OOM-kill, power loss mid-migration), the orphan pin survives forever and every restart fails this same consistency check; the node cannot start until an operator manually unlinks bpffs.

Suggestion: register the cleanup defer unconditionally (before the generation check), or treat a mismatch as an incomplete generation — remove the orphan pin(s) and proceed with a fresh migration (generationExists = false) instead of returning an error.

// Leave the legacy pin in place so the migration can be
// retried on the next restart.
return err
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F2 (Medium · data loss): legacy allow_out source is removed before DNS migration completes.

The legacy allow_out pin is deleted here, immediately after migrateAllowOutMap, but before migrateDNSAllowMap runs below. If the DNS migration then fails, Init's deferred cleanup removes the freshly-migrated allow_out_v3 pin, and on the next restart allowOutMigrationSource() returns "" (the legacy pin is already gone) — so the allow migration is silently skipped and all allow_out policy is permanently lost while DNS rules are retried. The comment on the migrateAllowOutMap failure path ("Leave the legacy pin in place so the migration can be retried") shows the intent; the current ordering defeats it.

Suggestion: move removePinnedMap(allowSrc) to the end of the function so the legacy source is removed only after the entire generation (allow + DNS) has been migrated successfully.

Comment thread CubeNet/src/mvmtap.bpf.c
}

/* create new session */
if (classify_egress_flow(skb->ingress_ifindex, key.dst_ip,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F3 (Medium · security/consistency): UDP/ICMP to an L7-pinned (host, port) bypasses the proxy.

Only FLOW_REJECT is handled; FLOW_HTTP/FLOW_HTTPS falls through to plain SNAT. But classify_egress_flow returns an L7 verdict for any flow whose (daddr, dport) matches an L7-required /48 entry, regardless of L4 protocol — while the proxy only listens on TCP 8080/8443. So a UDP flow to a pinned port (QUIC/HTTP-3 on 443, or any custom UDP L7 port) is SNAT'd straight to the internet with no proxy inspection, silently bypassing L7 audit/deny rules for that (host, port). The same pattern exists in do_icmp_nat (~line 473).

Suggestion: for UDP/ICMP, treat FLOW_HTTP/FLOW_HTTPS as an unsupported transport for L7 interception and drop (or explicitly log/audit the bypass) rather than silently passing; at minimum document that L7 pinning is TCP-only and consider rejecting UDP L7 ports at validation time.

migratePersistentPolicyMaps() unlinked the legacy allow_out pin right
after the allow migration succeeded but before the DNS migration ran. If
the DNS migration then failed, Init's rollback dropped the freshly
populated allow_out_v3/dns_allow_v2 pins, and the next restart skipped
re-migration (legacy pin gone), permanently losing the allow_out policy.

Migrate both maps first and only unlink the legacy pins after both
succeed, and make the unlink best-effort so a failed unlink cannot fail
Init and trigger the rollback. Adds a combined-failure rollback test.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
hankzhou added 3 commits August 17, 2026 10:40
A deny rule with a host (+ optional scheme) but no port was narrowed to
the default set {80/http,443/https}, so a custom-port allow rule could
bypass a broader host deny (fail-open). Deny rules are now port-agnostic
within the host, while allow rules keep the default-set narrowing (which
is fail-closed). The port/scheme match is now action-aware via
port_scheme.matches_deny. Documented the allow/deny port semantics in
the security-proxy guide (en/zh).

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
install_chain flushed and rebuilt the live chain in place; if the HTTPS
rule failed after the HTTP rule succeeded, the script aborted before
install_routing() while PREROUTING still jumped at a half-built chain,
a silent fail-open for the HTTPS scheme. Build into an unreferenced
scratch chain, verify both steering rules are present, then swap into
the live chain so the previous config keeps serving until the swap.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
cloneEgressRules did match := *r.Match, which aliased the Match pointer
fields (SNI/Host/Path/Scheme/Port) with the caller's request despite the
deep-copy contract, so mutating the request after the clone could leak
into the stored copy. Mirror CubeMaster's cloneStringPtr/cloneIntPtr.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
…omain

When the same domain appeared in both plain allow_out (L3) and an L7
rule, mergeDNSAllowRules collapsed them into one L7 entry, so the
DNS-learned IP was admitted into allow_out_v3 only on the rule's ports
and lost plain L3 access on every other port under deny-all.

Add NET_POLICY_FLAG_L3_ALLOWED (bit 1). mergeDNSAllowRules sets it when a
domain key is present in both the plain allow_out base set and an L7 rule
set. dns_learn_response_ip then learns the plain /32 any-port entry
alongside the L7 (ip, port)/48 entries: the /48 longest-prefix match wins
for the rule's ports (L7 intercept) while the /32 covers all other ports
(plain SNAT). The two near-identical /32 write blocks are merged into one
shared block (the marker-stripping mask is a no-op for plain allows).

Note this is a deliberate behavior change: a domain in both allow_out and
an L7 rule now keeps its plain L3 access (more permissive than before).

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
Comment thread CubeNet/cubevs/netpolicy.go Outdated
}
for _, e := range l7 {
if idx, ok := byKey[e.key]; ok {
out[idx].flags |= e.flags

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential regression — plain L3 allow for IP/CIDR hosts is silently dropped when an L7 rule is added for the same host.

When a host appears in both plain allow_out (as an IP//32) and as an L7 rule host, this merge ORs the L7 flag onto the entry and replaces its port set. Downstream, populateAllowOutInnerMap writes only the /48 (ip, port) entries for any NET_POLICY_FLAG_L7_REQUIRED entry and skips the plain /32 write entirely (if entry.flags&netPolicyFlagL7Required != 0 { ...continue }). So the explicit plain allow for that IP (any port) is replaced by L7-only /48 entries for exactly the rule's ports.

This is asymmetric with the domain path: mergeDNSAllowRules (below) marks the merged domain entry with NET_POLICY_FLAG_L3_ALLOWED so the datapath learns the plain /32 alongside the L7 /48 entries — its own comment says "otherwise the L7 rule would silently narrow the same-domain plain allow_out to only the rule's ports." The domain behavior is pinned by TestBuildNetPolicyPlanL3AllowedCoexistence; there is no equivalent test for the IP/CIDR path.

Failure scenario: allowOut = ["10.0.0.5"] + L7 rule {host: "10.0.0.5", port: 8443, scheme: https}. After merge, the /32 10.0.0.5 entry is gone. A flow to 10.0.0.5:22 misses /48 (10.0.0.5, 22) and hits the deny check; with any deny rule covering the host (e.g. a default-deny backstop) it is now rejected, whereas the base code allowed it via the explicit /32. Even without a deny rule, the explicit allow is silently removed — contradicting the documented "adding an L7 rule does not remove plain L3 access" coexistence (docs/guide/security-proxy.md, which is tested for domains only).

Suggestion: mirror the domain path — preserve the plain /32 (or covering subnet) entry when merging, and add a coexistence test for the IP/CIDR case analogous to TestBuildNetPolicyPlanL3AllowedCoexistence.

hankzhou added 13 commits August 17, 2026 19:39
persistentPolicyGenerationExists() returned an error when exactly one of
allow_out_v3 / dns_allow_v2 was pinned, and Init returned it before the
recovery defer was registered (that defer is only installed on the
generationExists==false path, which an early error return skips). A boot
that died between pinning the two maps left an orphan pin that survived
every restart, permanently bricking the node until an operator manually
unlinked bpffs.

Treat the half-pinned set as an incomplete generation instead: log a
warning, remove the orphan pin, and report "no generation" so Init
rebuilds a consistent pair and re-migrates from the legacy pins (still
present in this scenario, since the orphan is an empty not-yet-migrated
map). Adds regression tests for the recovery.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
… hosts

The static-IP path (mergeAllowOutWithL7) had the same subsumption the
domain path had: a host present in both plain allow_out and an L7 rule
collapsed to L7-only, so the IP was admitted into allow_out_v3 only on
the rule's ports and lost plain L3 access on every other port under
deny-all.

Reuse netPolicyFlagL3Allowed. mergeAllowOutWithL7 sets it when a static
host key is present in both the plain allow_out base set and an L7 rule
set; populateAllowOutInnerMap then writes the plain /32 any-port entry
alongside the L7 /48 entries (the /48 longest-prefix match wins for the
rule's ports, the /32 covers the rest). expandedAllowOutEntryCount
accounts for the extra /32, and dump surfaces l3_allowed. Subnet
allow_out + single-IP L7 rule already coexisted via distinct LPM keys;
only the exact same single-IP key was collapsing.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
… allow_out

An L7 rule host covered by a leading-"*." plain allow_out domain was still
denied on its non-rule ports under deny-all: the exact rule shadows the
wildcard in the DNS LPM match, so the host's learned IP only got the L7 /48
entry and no plain /32 fallback.

Unify the L3Allowed marking into a single userspace coverage pass.
l7DomainHasPlainCover reports whether an L7 host is covered by a plain
allow_out domain (exact same-host, or a leading-"*." wildcard subdomain at
any depth, apex excluded); markL3AllowedByPlainCover sets the flag on each
covered L7 rule. This replaces the same-key-only marking in
mergeDNSAllowRules and extends it to wildcard coverage. No eBPF change: the
DNS-learn path already writes /32 + /48 when L3Allowed is set.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
CubeNet/cubevs is a separate Go module whose own tests (custom_port,
dns_learn, migration, netpolicy, dump, egress_policy, l7_mark, tcp_state)
were never compiled or run: the CI mapped CubeNet/* to the cubelet target
(go test ./pkg/... under Cubelet), and run.sh's cubelet-network only built
cubevs's generated code before running Cubelet's network/runtime tests.

Add a cubevs-test make target and a cubevs component in run.sh and the
unit-test workflow (CubeNet/cubevs/* now triggers both cubevs and cubelet).
The target regenerates the BPF objects (make gen) then runs go test ./...
(no -coverprofile; the builder lacks covdata). The eBPF-loading tests need
a privileged root builder for bpf()/bpffs, so builder-run gains a
BUILDER_USER override and cubevs-test runs privileged as root.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
The verdict constants defined reject/SNAT/HTTPS but not HTTP, and the L7
exact-match cases only seeded HTTPS, so the plaintext HTTP interception
verdict (FLOW_HTTP) — the primary new data path — was never asserted. Add
flowVerdictHTTP=2 and TestClassifyEgressFlowExactL7MatchHTTP, which seeds a
/48 entry with Scheme=http and asserts FLOW_HTTP (and a non-matching port
still rejects).

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
The TPROXY --on-ip target is derived from CUBE_SANDBOX_NETWORK_CIDR via
sandbox_gateway_ip_from_cidr, but nothing asserted that arithmetic, so a
wrong gateway IP would pass CI. Add cases for several CIDRs (including a
non-/8-aligned mask and a host-bits-set address that must mask to the
network before +1) and assert invalid CIDRs are rejected. The TPROXY rule
content (--on-port / mark match) is already asserted by the atomic-install
test from the fail-open fix.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
…ndering

Migration had full success tests for DNS but only rollback tests for
allow_out_v2 -> allow_out_v3, leaving the 16-byte net_policy_value_v2
(L7 flag + expires) -> /48 expansion upgrade path unproven. Add
TestMigrateAllowOutMapOuterWithBpffs asserting an L7 entry expands to the
default {80,443} /48 set (flag, per-port scheme, expiry preserved) and a
plain entry becomes a /32.

Also add TestL7PortEntriesToDumpRendersHostByteOrder: l7PortEntriesToDump
must convert NBO ports to host order, so a dropped ntohs() would render
the raw wire value instead of the user-facing port.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
The HTTPS custom-port e2e asserted only code=200 + non-empty body, which a
plain-SNAT passthrough also satisfies — it could not distinguish
interception from passthrough. Now also assert the TLS leaf issuer is the
cluster interception CA (configurable via SDK_E2E_L7_INTERCEPTION_ISSUER,
default "CubeSandbox Egress MITM CA"): an intercepted connection is
terminated by CubeEgress's MITM cert, while passthrough would present the
upstream's real public CA. Verified the issuer-extraction command against
the live badssl endpoint.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
update_session() declared old_state/new_state as enum tcp_conntrack (signed
int) holding a u8 state. On clang 14 the signed value is narrowed with
'&= 255' masks that split it into a bounds-checked copy and a separate index
copy, so the verifier reads the tcp_conntracks[dir][index][old_state] .rodata
access as unbounded (umax=255) and rejects the program (invalid access to map
value, value_size=214 off=369). The cubevs unit tests therefore fail in the
ubuntu2004 builder (clang 14) while passing on clang 17.

Use __u8 so old_state stays in a single unsigned register shared by the bounds
check and the index, letting the verifier prove old_state <= 9. Semantics are
unchanged (state values fit in u8); this also keeps the production datapath
objects that include tcp.h verifiable on clang 14.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
cubevs was added to the make_target mapping but not the display_name
mapping, so the matrix-generation jq hit the "unsupported component" error
for cubevs. Add the missing display_name entry (cubevs -> CubeVS).

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
The L7/DNS e2e legs hit public echo endpoints (httpbun.com, httpbingo.org,
badssl) with a single attempt and a fixed timeout, so transient slowness or
rate-limiting flaked them. Add retry-with-backoff for transient transport
failures (timeout / connection reset / temporary DNS) while never retrying
HTTP 4xx/5xx, which are real policy verdicts:

- test_l7_egress.py: _http_json_command and _tls_issuer_command retry loops.
- test_dns_allow.py: _resolve_and_probe_command retries only the DNS resolve
  step; the TCP probe verdict is never retried (a blocked domain still reports
  PROBE:FAIL immediately).
- test_l7_custom_port.py: curl legs get --retry; _fetch_leaf_issuer retries
  when no cert is produced.

Verified: transient bad-host retries 3x then errors; an HTTP 404 returns
immediately without retrying.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
The interception CA's subject CN is "CubeSandbox Egress MITM CA" — that is
what both the one-click prepare (deploy/.../cube-egress-prepare.sh) and the k8s
chart (values caCommonName) generate. test_l7_egress.py defaulted to the
outdated "Cube Sandbox Egress CA". Align it with the authoritative value (and
with test_l7_custom_port.py's SDK_E2E_L7_INTERCEPTION_ISSUER default). The
test's regex fallback still tolerates spacing variants.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
The custom-port leg bound the in-process echo server to a fixed port (18080),
so a leftover server from an interrupted run made the fixture fail with
EADDRINUSE. Default L7_CUSTOM_HTTP_PORT to 0 (OS-assigned free port) and
return the actual port from the l7_echo_server fixture, using it for both the
L7 rule and the probe. SDK_E2E_L7_CUSTOM_HTTP_PORT still overrides when a
fixed port is required.

Assisted-by: CodeBuddy:kimi-k3-ioa
Signed-off-by: hankzhou <hankzhou@tencent.com>
return nil
}

func countUniqueLPMEntries(groups ...[]string) (int, error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — this budget counter undercounts multi-port L7 entries, and the refactored budget tests now rely on it.

countUniqueLPMEntries dedups by lpmKey{Prefixlen, IP} — there is no port dimension. A host with N pinned L7 ports occupies N /48 entries in the allow_out_v3 inner LPM trie but counts as 1 here. The production path is correct: validateNetPolicyPlan uses expandedAllowOutEntryCount (which expands to len(ports), default {80,443}, plus +1 for the L3_ALLOWED /32), and that helper's docstring explicitly warns the per-host count "undercounts multi-port L7 rules."

The problem is that the budget tests were refactored off the production builder (buildNetPolicyPlan) onto this standalone helper, so the 8193 > 8192 boundary assertions and the new TestValidateNetPolicyEntryCountsDeduplicatesByMapKey no longer exercise the counting that actually governs map population. A plan that passes the deduped count but would overflow the inner trie mid-write (E2BIG → half-populated map) can false-pass.

Either make the dedup key port-aware (include the port, e.g. reuse lpmKeyV3), or drop the helper and keep the budget tests on buildNetPolicyPlan. The generic validateNetPolicyEntryCounts name/signature also reads as production-ready, which is a footgun for future callers.

Comment thread CubeNet/src/mvmtap.bpf.c
* because they allocate a fresh reverse-side source port.
*/
if (sess->packet_class == L7PROXY_PACKET)
return TCP_NAT_PACK(0, TCP_NAT_RESET);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2/P3 — terminal L7 sessions block immediate 4-tuple reuse with RST until the userspace reaper removes them.

The rationale in the comment (identity reverse tuple ⇒ delayed packets from the old connection could mutate a fresh session) is sound, but the window is real: the reaper ticks every 5 s, and the terminal-session timeouts are 10 s for CLOSE/active-close and up to 2 min for TIME_WAIT. A client that closes and immediately reconnects on the same tuple is RST'd on its SYN for up to ~10 s in the common close case.

This only affects L7-intercepted custom ports (default 80/443 drain and fresh SNAT sessions are unaffected), but it is a plausible source of transient connection failures for fast-reconnect clients. Consider removing terminal L7 sessions more aggressively (or via a shorter reaper pass), or at least documenting the "avoid immediate client-port reuse" caveat in security-proxy.md.

Comment thread CubeNet/cubevs/reaper.go
}

//nolint:unused
func ingressSession(key *sessionKey, value *ingressSessionValue) string {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 — dead code added in this PR. ingressSession is never referenced anywhere; the //nolint:unused silences exactly the lint that would catch it. Either wire it into an event/log path (it mirrors egressSession) or drop it.

Comment thread CubeNet/cubevs/dump.go
// their policy YAML) even though it is stored in NBO on the datapath.
type L7PortEntryDump struct {
Port uint16 `json:"port"`
Scheme string `json:"scheme"` // "http" | "https" | ""(未知)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (nit) — mixed-language comment. // "http" | "https" | ""(未知) — the rest of the file comments in English; this should be "" (unknown) or similar.

@cubesandboxbot

Copy link
Copy Markdown

Review: feat(cubenet): support arbitrary custom L7 port + scheme in egress rules

PR: #1347 · Author: zhouxianping · Base: master · Head: feat/custom_l7_port
Scope: ~8,604 additions / 575 deletions across ~85 files (CubeNet BPF/userspace, CubeEgress Lua + iptables, Cubelet runtime, CubeAPI/CubeMaster, SDKs go/node/python, docs, examples).

AI-generated review. This review was produced by an automated reviewer; it has not been approved by a human. Findings are ranked by severity; inline comments are posted on the diff. Please verify each item before acting on it.


Overview

The PR extends L7 egress rules so a rule can pin the TCP port CubeEgress intercepts via Match.port + Match.scheme, replacing the hardcoded {80/http, 443/https} set with an arbitrary (host, port, scheme) tuple. This is a deep, cross-cutting change:

  • Dataplane (CubeNet/src): classify_egress_flow returns a unified verdict (FLOW_SNAT / FLOW_HTTP / FLOW_HTTPS / FLOW_REJECT); allow/deny maps migrate v2→v3 with the port moved into the key (lpm_key_v3 /48 exact (ip,port), /32 any-port, /<32 subnet); L7 sessions carry packet_class/l7_scheme and stamp skb->mark so CubeEgress's TPROXY can steer by fwmark instead of dport.
  • CubeEgress: new port_scheme.lua implements the four port/scheme quadrants with fail-closed semantics; cube-proxy-iptables-init.sh now validates the shared /etc/cubeegress/l7-marks.conf and swaps chains atomically via a scratch chain.
  • Orchestration: Cubelet extracts (host, port, scheme) targets and normalizes scheme-only rules to default ports; CubeAPI/CubeMaster/SDKs validate port/scheme consistently.
  • Migration: allow_out and dns_allow migrate one-way with rollback; legacy pins are removed only after both migrations succeed.

The change is unusually well-engineered for its size: validation is consistent across every layer, the port/scheme semantics are fail-closed rather than silently permissive, verifier constraints are handled carefully (#pragma unroll with guard-in-body, __u8 states), and there is a genuine test suite at every layer (BPF spec tests, Lua, iptables shell, Go migration, SDK e2e). The production budget counter expandedAllowOutEntryCount is correct and its docstring is honest about why the naive count is wrong.

The findings below are mostly test-quality and edge-case concerns rather than merge-blocking defects. Finding 1 should be fixed before the budget tests are relied on; Finding 2 deserves a documented mitigation or a faster reaper for terminal L7 sessions.


Findings

1. P2 — validateNetPolicyEntryCounts/countUniqueLPMEntries undercount multi-port L7 entries, and the refactored budget tests no longer exercise the production path

CubeNet/cubevs/netpolicy.go:842

countUniqueLPMEntries dedups by lpmKey{Prefixlen, IP} — there is no port dimension. A host with 8 pinned L7 ports is counted once, even though it occupies 8 /48 entries in the allow_out_v3 inner LPM trie. The production path is correct: validateNetPolicyPlan uses expandedAllowOutEntryCount (expands to len(ports), default {80,443}, +1 for the L3_ALLOWED /32 coexistence entry), and that helper's docstring explicitly warns that a per-host count "undercounts multi-port L7 rules."

The problem is that the budget tests were refactored off the production builder onto the standalone helper:

  • TestValidateNetPolicyEntryCount* now calls validateNetPolicyEntryCounts(allowOutCIDRs, l7AllowOutCIDRs, nil, nil, nil) with raw CIDR slices instead of buildNetPolicyPlan(MVMOptions{...}), so the 8193 > 8192 boundary assertions no longer exercise the counting that actually governs map population.
  • The new TestValidateNetPolicyEntryCountsDeduplicatesByMapKey asserts the undercounting behavior (no error for one host shared across plain-allow + L7), which is exactly the case where production would count 2–9 entries.

Consequence: a policy that passes the deduped budget check but overflows the inner trie mid-write (E2BIG → half-populated map) can false-pass. The helper's generic name and production-looking signature are a footgun for future callers. Either make countUniqueLPMEntries port-aware (include the port in the dedup key, e.g. reuse lpmKeyV3), or delete the helper and keep the budget tests on buildNetPolicyPlan.

2. P2/P3 — L7 session tuple-reuse after CLOSE/TIME_WAIT returns RST until the reaper removes the old session

CubeNet/src/mvmtap.bpf.c:733

In do_tcp_nat, when an existing L7 session is terminal (TCP_CONNTRACK_CLOSE/TIME_WAIT) and a new SYN arrives on the same 4-tuple, the datapath returns TCP_NAT_RESET and leaves the old pair in place until the userspace reaper deletes it. The comment explains the rationale (identity reverse tuple ⇒ delayed packets from the old connection could mutate a fresh session), which is sound. But the window is real: the reaper ticks every 5 s, and the terminal-session timeouts are 10 s (CLOSE / active-close) up to 2 min (TIME_WAIT) — so a client that closes and immediately reconnects on the same tuple can be RST'd for up to ~10 s in the common close case. This only affects L7-intercepted custom ports (the default 80/443 drain path and fresh SNAT sessions are unaffected), but it is a plausible source of transient connection failures for fast-reconnect clients. Consider deleting terminal L7 sessions more aggressively, or documenting the behavior in security-proxy.md alongside the "avoid reusing client ports immediately" caveat.

3. P3 — dead ingressSession helper added in reaper.go

CubeNet/cubevs/reaper.go:227

ingressSession(key, value) string is added but never referenced anywhere; it is silenced with //nolint:unused. Dead code — either wire it into an event/log path or drop it (the //nolint suppresses exactly the lint that would catch it).

4. P3 — stray Chinese comment in dump.go

CubeNet/cubevs/dump.go:187

Scheme string `json:"scheme"` // "http" | "https" | ""(未知)

Mixed-language comment in a Go source file; the rest of the codebase comments in English. Cosmetic but worth a one-line fix.


Behavior & upgrade notes (no change required, but worth knowing)

  • DNS-learned L7 rules are now fail-closed on non-rule ports. In v2, dns_learn_response_ip wrote L7-required domains as /32 any-port allow entries, so any port to a DNS-learned L7 host was allowed. In v3, only /48 per-rule-port entries are written (default {80,443} when the rule has no explicit ports). After upgrade, a non-rule port (e.g. 22, or 8443 when only 443 is pinned) to an L7-only domain that used to pass now falls through to deny_out and is rejected. The migration copies L7 entries as default-port /48 entries, so it does not preserve any-port semantics unless the host is also present in plain allow_out (which adds the L3_ALLOWED /32). This is documented, and the fail-closed direction is defensible, but it is a silent behavior change on upgrade for existing DNS-learned L7 domains.
  • from_cube node-IP corner case. The nodenic_ip branch calls classify_egress_flow(ifindex, daddr, 0) with dport = 0, which can never match a /48 L7 entry. Traffic from a sandbox to an L7-only domain's node IP (the node's own address, e.g. a hostNetworked service) that v2 allowed via the /32 L7 entry is now denied under deny-all. Narrow corner case, but the dport=0 sentinel silently changes semantics for these flows.
  • L7 deny rules only cover intercepted tuples. A deny rule with a pinned (port, scheme) matches only traffic CubeEgress intercepts on that tuple; other ports to the same host are governed by deny_out (L3/L4). Internally consistent with the fail-closed design, but operators should not assume an L7 deny rule blocks the host on all ports.
  • Shared mark config is now a cross-component contract. /etc/cubeegress/l7-marks.conf is parsed by both the iptables init script and Cubelet's loadL7MarksConfig; both tolerate hex, an optional export prefix, and trailing comments. Nice defensive parsing on both sides — just keep the two parsers in sync, since a divergence would silently desync mark-based steering.

Overall

High-quality, coherent, and unusually well-tested PR. The port/scheme model is consistent from API validation down to the BPF key layout and Lua matching, and the fail-closed defaults are the right call for a security proxy. None of the findings block merge on their own; I'd ask for a fix (or an explicit doc/name change) on Finding 1 and a documented mitigation on Finding 2 before considering this fully shippable.

@zhouxianping
zhouxianping merged commit a593963 into TencentCloud:master Aug 18, 2026
60 checks passed
@lkml-likexu

lkml-likexu commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

@FakeLearne Please confirm whether the following test cases are already present in tests/e2e/sdk_compat/ (pls delete for yes). If not, please move them there.

tests/test_l7_custom_port_e2e.py::test_l7_custom_http_inject_and_https_deny SKIPPED (use --run-e2e or set CUBE_E2E=1)
tests/test_l7_custom_port_e2e.py::test_l7_custom_https_allow_reaches_real_upstream SKIPPED (use --run-e2e or set CUBE_E2E=1)
tests/test_l7_custom_port_validation_e2e.py::test_l7_custom_scheme_conflict_rejected SKIPPED (use --run-e2e or set CUBE_E2E=1)
tests/test_l7_custom_port_validation_e2e.py::test_l7_custom_port_budget_exceeded_rejected SKIPPED (use --run-e2e or set CUBE_E2E=1)

FakeLearne added a commit to FakeLearne/CubeSandbox that referenced this pull request Aug 21, 2026
…ive flows

Add PUT /sandboxes/{sandboxID}/network so a running sandbox's egress policy can
be replaced without recreating it, and make the update reach traffic that already
exists rather than only future connections.

The route, the status codes (204/400/404/409) and the full-replacement semantics
follow E2B, so an omitted field clears rather than preserves. One divergence is
deliberate: E2B's nftables chain accepts ESTABLISHED,RELATED first, so tightening
a policy there has no effect on open connections and a long-lived connection can
keep using a revoked destination indefinitely. We re-evaluate instead.

## Datapath

Each sandbox carries a policy generation (mvm_meta.policy_version), bumped only
after both planes hold the new policy. Every session caches the generation it was
admitted under, so the next packet on an established flow is re-judged exactly
once per update:

  - same verdict: restamped and left alone, costing one policy lookup per flow;
  - no longer allowed, or the verdict changed: the session pair is deleted. TCP is
    answered with an RST, matching how every other unreachable TCP packet is
    handled here, so the guest fails fast instead of stalling on retransmits; UDP
    and ICMP have nothing to reset and are dropped.

A verdict *change* retires the flow rather than migrating it. The SNAT and L7
paths disagree about both the reply tuple and which side terminates the TCP
connection, so migration is not expressible; the client reconnects and is judged
as any new flow.

Deleting rather than flagging keeps the retirement self-enforcing, and is what
lets this stay small. A later non-SYN packet finds no session and is reset, so a
revoked flow cannot resume even if a subsequent update re-allows the destination,
while a SYN legitimately opens a fresh connection under the current policy. Both
directions go at once, so reply-path callers need no change either, and neither
does the reaper.

That property holds only once the legacy 80/443 connection drain in do_tcp_nat is
removed, which this does. While that branch is present a deleted session is
indistinguishable from a lost one, and the drain silently undoes the revocation.
It had also lost its own justification independently: written (1b5d2c3) when L7
flows created no session and re-ran the policy lookup per packet, so a DNS-learned
entry expiring broke live connections, it was made redundant by TencentCloud#1347, after which
L7 flows cache their verdict in nat_session and an established flow never consults
the policy maps again. Two of the three causes its comment cites do not hold
either -- both session maps are pinned and no startup path flushes them, and they
are BPF_MAP_TYPE_HASH rather than LRU, so nothing is evicted. The remaining case,
a flow idle past the reaper timeout, contradicts the reaper it depends on:
deleteSessions removes both directions, and the drain then revives the egress
direction alone, without recreating a session, on the sole evidence that the proxy
still holds a socket -- scoped by which flows happen to leave a host socket rather
than by policy, which is why it only ever applied to 80/443. Consequently an L7
keepalive connection left idle past the reaper timeout is now reset rather than
revived, and the application reconnects.

policy_version fits in existing padding, so mvm_meta and nat_session keep their
sizes and the pinned-map layout is unchanged. The kernel verifier accepts
del_session() while the lookup pointer is still held, checked against mvmtap,
nodenic and localgw.

## Control plane

cubevs gains UpdateTAPDevicePolicy, a third apply mode beside applyNetPolicy
(additive) and replaceNetPolicy (flush then refill). Neither fits a live sandbox:
flushing blanks the policy for as long as the refill takes, and swapping the inner
map defeats the HashOfMaps inner cache and pays a synchronize_rcu per update. So
it diffs against the installed maps and writes only what changed. DNS-learned rows
are left untouched, and deny_out's desired set always includes the always-denied
private and link-local ranges.

Ordering is part of the contract: CubeEgress, then CubeVS (which bumps the
generation), then durable state. A failure before the bump leaves flows on their
cached verdict instead of judging them against a half-applied map, and a failure
before the state write replays the previous policy on restart. Nothing is rolled
back; the diff is computed from the live maps, so replaying converges.

For clone, snapshot and pause/resume the new policy is written to all three places
that outlive the update: the network runtime state file (restart recovery), the
Cubelet cubebox store (pause packaging) and Master's sandboxspec (what clone
replays). The sandboxspec write happens synchronously before the response, so
update then snapshot then clone is read-your-writes; a failure there is logged and
does not fail the call, following the same best-effort convention the create path
already applies to that store.

## DNS resolver allowance

The create path folds the sandbox's resolver addresses into allow_out so domain
rules can be resolved at all, but only when the policy names a domain. An update
carries just the caller's targets, so it would silently revoke DNS and black-hole
every domain rule it had just installed. The resolver list is now recorded on
every create -- unconditionally, because a later update may introduce the first
domain -- and folded back in under the same "policy still names a domain"
condition, so dropping every domain also withdraws the implicit DNS access.

Recording is separate from installing: an all-IP policy still gets no resolver
access, since allow_out cannot be narrowed to port 53. The gate therefore asks
whether a target is *installed* as a domain, not whether it looks like a DNS name.
The latter accepts "10.0.0.1" -- digits are valid label characters and nothing
requires a non-numeric top label -- and would fold the resolver into every policy
written with bare IPv4 literals, granting one extra /32 on all ports that the
caller never asked for. cubevs exports that install decision as
IsAllowOutDomainTarget, mirroring splitAllowOutTargets, so the update gate cannot
diverge from the create gate, which has always been IP-first.

An update that neither installs nor clears L7 rules does not contact CubeEgress at
all. That is decided from the old and new rule sets, not from whether the proxy
happens to be configured, so an L3-only sandbox never depends on a component it
does not use.

## Known limitation

Addresses already learned for a domain outlive the rule that produced them until
their DNS TTL expires, so revoking a domain promptly needs a short resolver TTL.
Provenance tracking for learned entries is deferred.

## Testing

Go unit tests cover the map diff (revocation, expanded L7 port sets, deny_out
convergence, DNS port-set replacement), the generation bump and its survival
across metadata rewrites, and the controller's ordering, resolver refold, L3-only
skip and "a failed update keeps the old policy" guarantees. The resolver gate is
exercised over bare, masked, subnet and L7-host target forms, because only the
bare literals distinguish the install decision from a name-shape check. A new BPF
case drives session_policy_revoked through the existing egress-policy harness
across seven verdict/generation combinations.

Ten end-to-end cases were added under a new network_dynamic_update capability.
Two of them are what actually distinguish this feature: a revoked connection must
be torn down, and a still-permitted one must be left alone -- the second is what
stops an implementation that simply kills every session from passing. Their
guest-side holder uses TCP keepalive rather than application bytes, because a peer
that hangs up on unexpected data is indistinguishable from a policy reset, and it
classifies RESET, ALIVE, EOF and DATA separately so only the unambiguous outcomes
are asserted.

Three more cover the compatibility requirement: a clone inherits the updated
policy rather than the create-time one (which also covers read-your-writes, since
clone snapshots immediately after the update returns), a clone of a narrowed
policy is not more permissive, and the policy survives pause/resume. These have to
be end-to-end -- clone and snapshot are not network-aware, they just replay a
stored create request, so nothing under templatecenter changed here and review
alone cannot tell whether the spec write-back landed.

Verified on a live single-node deployment: all ten cases pass on repeated runs,
and examples/network-policy/network_dynamic_update.py walks an IP allow list, a
connection carried across a revoking update (observed as RESET), a domain allow
list, and switching on L7 interception mid-run.

Signed-off-by: yarrischen <yarrischen@tencent.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants