peer: refuse to start if launch_cfg lacks abuse-handling rules#484
Open
myleshorton wants to merge 2 commits into
Open
peer: refuse to start if launch_cfg lacks abuse-handling rules#484myleshorton wants to merge 2 commits into
myleshorton wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a peer-side safety gate that validates the server-supplied sing-box config before starting Share My Connection, so peers refuse configs missing abuse-blocking structure.
Changes:
- Adds
validateAbuseRulesand unit tests for required abuse rule sets, reject rules, and static canaries. - Calls the validator during
Client.Startbefore patching and starting sing-box. - Updates peer test fixtures to use a config that passes the new validation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
peer/validate.go |
New structural validator for peer launch config abuse-blocking rules. |
peer/validate_test.go |
Unit tests and shared minimal valid launch config fixture. |
peer/peer.go |
Invokes abuse-rule validation during peer startup. |
peer/peer_test.go |
Updates stub registration response to include valid abuse-rule config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3b19ec0 to
8cb3713
Compare
6348606 to
6f95dc2
Compare
8cb3713 to
5959bbe
Compare
2351fa8 to
57e7cd4
Compare
be86ee9 to
7d616d6
Compare
Phase 1 Share My Connection abuse-handling lives entirely in the
sing-box options that lantern-cloud sends back from
/v1/peer/register. The peer client trusts that JSON and hands it
straight to libbox. If a future regression in
lantern-cloud/cmd/api/pcfg/samizdat.go silently shipped a
launch_cfg without those rules, every newly-registered peer would
become an open residential proxy until someone noticed — and the
class of bug that triggers it is one missing function call in a
file most reviewers don't routinely audit.
validateAbuseRules adds defence-in-depth on the client side. After
Register returns, before BuildBoxService is called, parse the
JSON and assert:
- route.rule_set declares all four abuse tags (geosite-malware,
geoip-malware, geosite-phishing, geosite-cryptominers).
- route.rules has a matching reject action for each tag —
otherwise sing-box downloads the rule_set but never enforces it.
- route.rules contains an RFC1918 reject (canary 10.0.0.0/8) and
an SMTP-port reject (canary :25). One sentinel per static block
in samizdat.go's peerEgressBlockRules, picked to detect "whole
block was dropped" rather than fail on legitimate additions.
The check is structural-only and permissive about JSON shape (sing-
box marshals "default" rules in both inlined and nested forms;
TestValidateAbuseRules_NestedDefaultForm asserts both work). It
does NOT verify the .srs files at the rule_set URLs or that the
URLs themselves are trustworthy — those are separate supply-chain
concerns to track.
errors.Join means a thoroughly-broken config surfaces every missing
piece in one report so the deployer triaging "why won't my peer
start?" doesn't have to fix-one-thing-find-the-next.
Existing peer_test.go uses a minimal `{"inbounds":[…]}` fixture
that would now fail the check. Migrated it to minimalValidLaunchCfg
(shared with validate_test.go) — same shape as a real samizdat
launch_cfg as far as the routing layer is concerned.
57e7cd4 to
cdd286d
Compare
Ten findings: 5 substantive validator bugs, 5 doc-lint violations.
Substantive:
1. asStringSlice / asFloatSlice now accept scalar OR array. sing-box
route rules encode single-element matches as a scalar (e.g.
'"rule_set": "sr-direct"' or '"port": 25'), and treating only
the array form as valid would false-positive a launch_cfg that
uses the scalar form. Mirrors how sing-box itself decodes these
fields.
2. Same for asFloatSlice — scalar number support.
3. New isUnconditionalReject helper. The old check counted any reject
rule that mentioned an abuse tag, which would let through:
- {action:reject, rule_set:[tag], invert:true} — inverted match
rejects EVERYTHING EXCEPT the tag, not the tag.
- {action:reject, rule_set:[tag], port:80} — narrows the reject
to port-80 traffic only; abuse traffic on other ports passes.
isUnconditionalReject(body, matchKey) requires action=reject,
invert!=true, and no body keys outside {action, invert, matchKey}.
Explicit invert=false is allowed (treated as the canonical no-op).
4. Same predicate-narrowing concern applies to the static-canary
reject rules (RFC1918, SMTP). Updated validateStaticRejectCanaries
to check each canary against isUnconditionalReject scoped to its
own match field (ip_cidr or port).
5. New TestClient_Start_AbuseRuleValidationFailureUnwinds integration
test mirrors the existing *FailureUnwinds tests: stub a launch_cfg
without a route block, assert Start returns 'abuse-rule sanity
check' error, c.IsActive() is false, the port forward is unmapped,
the box was never started, and the route was deregistered.
Doc-lint (AGENTS.md:13-17 forbids code-location and ticket refs):
6. validate.go:14 — dropped the explicit reference to lantern-cloud's
server-side file; replaced with 'the server-side abuseTags list'.
7. validate.go:37 — same; 'the server-side static peer-egress-block
list' replaces the path reference.
8. validate.go:50 — dropped 'engineering#TODO' placeholder. The
supply-chain concern is real but unactionable as written; replaced
with 'separate supply-chain concerns and are not in scope for this
gate.'
9. peer.go:215 — dropped 'See validate.go for the exact checks';
replaced with 'the validator's docstring enumerates the exact
rule shapes it requires.'
10. validate_test.go:13 — dropped peer_test.go reference; reworded as
'the stub server used in Start-path tests'.
Six new tests added:
- TestValidateAbuseRules_AcceptsScalarRuleSet — scalar matches valid
- TestValidateAbuseRules_RejectsInverted — invert=true not credited
- TestValidateAbuseRules_RejectsExtraConstraint — narrowed reject not credited
- TestValidateAbuseRules_RejectsStaticCanaryWithExtraConstraint — same for canaries
- TestValidateAbuseRules_AcceptsExplicitInvertFalse — explicit false credited
- TestClient_Start_AbuseRuleValidationFailureUnwinds — Start-path unwind coverage
All tests pass under -race -count=1.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Defence-in-depth for Share My Connection abuse handling. Refuses to start the peer sing-box if the server-supplied
launch_cfgis missing the expected abuse rule_set / reject rules.Why this matters: Phase 1 SmC abuse handling lives entirely in the sing-box JSON that
lantern-cloudsends back from/v1/peer/register(seepcfg/samizdat.go—abuseRejectRules+abuseRemoteRuleSets+peerEgressBlockRules). The peer client currently hands that JSON straight tolibboxafter one VPN-bypass tweak. A regression insamizdat.gothat ever shipped a launch_cfg without those rules would turn every newly-registered peer into an open residential proxy until someone noticed.How the check works
sequenceDiagram autonumber participant LC as lantern-cloud<br/>pcfg/samizdat.go participant P as peer.Client.Start<br/>radiance/peer/peer.go:187 participant V as validateAbuseRules<br/>radiance/peer/validate.go ✨ participant LB as libbox.NewServiceWithContext P->>LC: POST /v1/peer/register LC-->>P: launch_cfg JSON (with abuse rules) P->>V: peer/peer.go:202<br/>validateAbuseRules(regResp.ServerConfig) ✨ Note over V: parse route.rule_set + route.rules<br/>assert 4× abuse tags present in both<br/>assert RFC1918 + SMTP canary rejects rect rgba(180, 230, 200, 0.3) alt Happy path — rules intact V-->>P: nil P->>LB: build + start sing-box else Server-side regression — rules missing V-->>P: errors.Join(missing tags, missing canaries) Note over P: Start returns error,<br/>peer refuses to share end endThe check is purely structural and permissive about sing-box's JSON shape (it marshals
"default"rules in both inlined and{"type":"default","default":{...}}forms —TestValidateAbuseRules_NestedDefaultFormcovers both).What it asserts
route.rule_set:geosite-malware,geoip-malware,geosite-phishing,geosite-cryptominers. The canonical list mirrorsabuseTagsinlantern-cloud/cmd/api/pcfg/samizdat.go.route.rules. A rule_set without a reject rule is a no-op — sing-box downloads the list and never enforces it.peerEgressBlockRules—10.0.0.0/8(RFC1918 canary) and port25(SMTP canary). Spot-check rather than full enumeration so legitimate additions in samizdat.go don't break this check.What it does NOT cover
.srsfiles at the rule_set URLs (could be corrupted/tampered)These are flagged in the audit notes but are out of scope for a structural-validation PR.
Errors are joined, not first-failure
errors.Joinmeans a thoroughly broken config (e.g. someone accidentally deleted half ofsamizdat.go's route block) surfaces every missing piece in one error report. Whoever is triaging "why won't my peer start?" doesn't have to fix-one-thing-find-the-next.Keeping the validator in sync with the server
abuseRuleSetTagsin this PR mirrorsabuseTagsinlantern-cloud/cmd/api/pcfg/samizdat.go. If samizdat.go grows or renames a tag, this list grows with it. A future test that exercises an actual lantern-cloud-generated launch_cfg throughvalidateAbuseRuleswould close the drift gap entirely — left as follow-up since it spans two modules.Test plan
go test ./peer/ -count=1— full peer test suite greengo test ./peer/ -run TestValidateAbuseRules -v— 10/10 validator unit tests passrouteinstead ofreject)stubServerfixture updated tominimalValidLaunchCfg— no peer test regressions.pcfg.GenerateSamizdatoutput throughvalidateAbuseRulesto catch drift between the two files.