peer: refuse to start if launch_cfg lacks abuse-handling rules#484
Open
myleshorton wants to merge 1 commit into
Open
peer: refuse to start if launch_cfg lacks abuse-handling rules#484myleshorton wants to merge 1 commit into
myleshorton wants to merge 1 commit into
Conversation
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.
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.
Comment on lines
+189
to
+199
| arr, ok := v.([]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| out := make([]string, 0, len(arr)) | ||
| for _, x := range arr { | ||
| if s, ok := x.(string); ok { | ||
| out = append(out, s) | ||
| } | ||
| } | ||
| return out |
Comment on lines
+203
to
+213
| arr, ok := v.([]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| out := make([]float64, 0, len(arr)) | ||
| for _, x := range arr { | ||
| if f, ok := x.(float64); ok { | ||
| out = append(out, f) | ||
| } | ||
| } | ||
| return out |
Comment on lines
+113
to
+117
| if action, _ := body["action"].(string); action != "reject" { | ||
| continue | ||
| } | ||
| for _, t := range asStringSlice(body["rule_set"]) { | ||
| rejectedTags[t] = true |
Comment on lines
+146
to
+155
| if action, _ := body["action"].(string); action != "reject" { | ||
| continue | ||
| } | ||
| for _, cidr := range asStringSlice(body["ip_cidr"]) { | ||
| if cidr == rfc1918CanaryCIDR { | ||
| gotRFC1918 = true | ||
| } | ||
| } | ||
| for _, p := range asFloatSlice(body["port"]) { | ||
| if p == smtpCanaryPort { |
| // would otherwise turn every peer in the field into one until the | ||
| // next deploy. The peer prefers failing to share over sharing | ||
| // unsafely. See validate.go for the exact checks. | ||
| if err := validateAbuseRules(regResp.ServerConfig); err != nil { |
Comment on lines
+9
to
+14
| // abuseRuleSetTags is the canonical list of abuse rule_set tags that the | ||
| // peer launch_cfg MUST carry. Mirrors abuseTags in | ||
| // lantern-cloud/cmd/api/pcfg/samizdat.go. If samizdat.go grows or | ||
| // renames a tag, this list grows with it — the test in | ||
| // lantern-cloud asserts the server side; this list asserts the client | ||
| // side sees the same shape after registration. |
Comment on lines
+36
to
+37
| // RFC1918 CIDRs, and abuse-prone ports. Those rules live in | ||
| // lantern-cloud/cmd/api/pcfg/samizdat.go. |
Comment on lines
+49
to
+50
| // or that the URLs themselves are trustworthy; those are separate | ||
| // supply-chain concerns tracked in engineering#TODO. |
| // server-side regression that silently shipped an open-proxy config | ||
| // would otherwise turn every peer in the field into one until the | ||
| // next deploy. The peer prefers failing to share over sharing | ||
| // unsafely. See validate.go for the exact checks. |
Comment on lines
+12
to
+13
| // reject rules. Shared by peer_test.go's stubServer so the existing | ||
| // Start-path tests do not regress on the new check. |
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.