config: apply default frontingsnis without a country code; preserve baked-in SNI

[myleshorton]

Summary

ExpandedProvider resolves each masquerade's SNI. Two changes so providers can actually send a (legit) SNI, instead of the conspicuous no-SNI every client uses today.

Background

The production client (radiance/kindling/fronted/fronted.go::NewFronted, the only non-test domainfront.New caller) passes no country code. The old code generated an SNI only when countryCode != "":

if countryCode != "" && p.FrontingSNIs != nil { ... }

So every provider's arbitrary-SNI strategy was inert and all fronting went out with no SNI — measured directly across akamai, aliyun, cloudfront (all masquerades carry sni: ""). A no-SNI TLS ClientHello that otherwise looks like Chrome is itself anomalous (real Chrome always sends SNI), so this is a fingerprinting liability.

Changes

1. default frontingsnis applies without a country code. Drop the countryCode != "" guard; a country-specific entry still overrides default. This activates a provider's default arbitrary-SNI strategy for every client.

   • akamai: its default is usearbitrarysnis: true (4,638 real akamai-customer SNIs) — now active globally. Verified from a live client: 20/20 sampled edges front (202) with an arbitrary SNI, identical to no-SNI (20/20) → no regression, and stealthier.
   • akamai's explicit cn entry (usearbitrarysnis: false) still wins for China → no-SNI preserved there (existing intent).

2. Baked-in masquerade SNI is preserved through expansion unless an arbitrary SNI is generated. This lets the config pin a per-edge front SNI without a country code — needed for aliyun, whose edges reject most SNIs but accept the specific service domain they host (e.g. www.mobgslb.tbcache.com).

Precedence per masquerade: generated arbitrary SNI > baked-in SNI > empty (omit).

Safety

Audited the live config's providers: only akamai (default: true, desired) and aliyun (configured separately) have a truthy default; cloudfront is frontingsnis: {}. Providers with no frontingsnis / a false default / no baked-in SNI are unchanged (SNI stays empty). Backward-compatible at rollout: an old client (pre-change) reading a config with usearbitrarysnis: true still omits SNI (its guard blocks it), so nothing breaks before the client updates.

Tests

TestExpandedProvider gains: default arbitrary SNI applies with no country code; baked-in SNI preserved when none generated; generated SNI overrides baked-in. Existing cases (no-SNI default, country override, lowercasing) still pass.

Follow-ups (separate PRs)

• lantern-cloud provider_map.yaml: set aliyun's front SNI to www.mobgslb.tbcache.com (works on ~83% of edges vs ~17% for img.alicdn.com; supersedes the omit-SNI direction of lantern-cloud#2896).
• Bump this dep through kindling → radiance → lantern to deploy.
• (B) scan-side per-edge SNI discovery for aliyun (bake the SNI each edge actually accepts).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved SNI selection during provider expansion so the default strategy applies even when no country code is provided.
  • Preserved a baked-in masquerade SNI when expansion can’t generate a replacement; overrides only when a non-empty SNI is produced.
  • Updated masquerade hostname verification resolution to prioritize per-masquerade settings with appropriate fallbacks when unset.
• Tests
  • Added coverage for default SNI behavior, baked-in SNI preservation vs override, and masquerade VerifyHostname fallback/precedence.
• Documentation
  • Refreshed expanded-provider behavior documentation to match current SNI expansion rules.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ad683e0d-2535-43cf-a612-1815e6bdef3d

📥 Commits

Reviewing files that changed from the base of the PR and between 19f6ed1 and 14c9267.

📒 Files selected for processing (1)

• config.go

🚧 Files skipped from review as they are similar to previous changes (1)

• config.go

---

📝 Walkthrough

Walkthrough

ExpandedProvider now applies default fronting-SNI selection without requiring a country code, preserves existing masquerade SNI unless generation produces a value, and updates VerifyHostname fallback behavior. Tests cover the revised expansion rules.

Changes

ExpandedProvider SNI handling

Layer / File(s)	Summary
SNI selection and precedence config.go	ExpandedProvider docs and formatting are updated, default SNI selection no longer depends on a country code, and masquerade expansion preserves existing SNI while refining VerifyHostname fallback behavior.
ExpandedProvider tests config_test.go	TestExpandedProvider adds cases for default SNI selection, preserved and overridden masquerade SNI, and the updated VerifyHostname resolution rules.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A rabbit hopped through config trees,
found defaults breeze with ease. 🐇
Old SNI stays unless anew,
and hostname paths now follow through.
The tests do binky, bright and keen,
in tidy hops of code serene.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the main SNI behavior changes: applying the default frontingsnis without a country code and preserving baked-in SNI.
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 fisk/preserve-baked-sni

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

config.go (1)

165-171: 🎯 Functional Correctness | 🟡 Minor

Normalize countryCode before the SNI map lookup. WithCountryCode stores the value as-is, so an upper- or mixed-case code will miss FrontingSNIs["br"] and fall back to "default"; strings.ToLower(countryCode) avoids that.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@config.go` around lines 165 - 171, Normalize the country code before looking
up entries in FrontingSNIs so mixed- or upper-case values still match the
intended SNI config. Update the lookup logic in the code that uses
WithCountryCode and FrontingSNIs to convert countryCode to lower case before
indexing the map, while preserving the existing default fallback behavior.

🧹 Nitpick comments (1)

config_test.go (1)

92-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for a no-SNI country override against a baked-in SNI.

The new tests cover baked-in preservation when there's no fronting strategy and override when one generates a value, but not the interaction flagged in config.go: a country entry with UseArbitrarySNIs: false (intended to suppress SNI) applied to a masquerade that carries a baked-in SNI. This is the scenario whose behavior changed; a test would pin down the intended outcome.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@config_test.go` around lines 92 - 101, Add a test in config_test.go that
exercises ExpandedProvider with a country override using UseArbitrarySNIs: false
on a Provider whose Masquerade already has a baked-in SNI. The current coverage
in ExpandedProvider only checks preservation with no override and replacement
when arbitrary SNIs are generated, so add a case that applies the no-SNI country
override and asserts the intended final SNI behavior for the baked-in
masquerade.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@config.go`:
- Around line 165-171: Normalize the country code before looking up entries in
FrontingSNIs so mixed- or upper-case values still match the intended SNI config.
Update the lookup logic in the code that uses WithCountryCode and FrontingSNIs
to convert countryCode to lower case before indexing the map, while preserving
the existing default fallback behavior.

---

Nitpick comments:
In `@config_test.go`:
- Around line 92-101: Add a test in config_test.go that exercises
ExpandedProvider with a country override using UseArbitrarySNIs: false on a
Provider whose Masquerade already has a baked-in SNI. The current coverage in
ExpandedProvider only checks preservation with no override and replacement when
arbitrary SNIs are generated, so add a case that applies the no-SNI country
override and asserts the intended final SNI behavior for the baked-in
masquerade.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 81fcbfa9-5017-4aaf-8752-3b7ff5e5a655

📥 Commits

Reviewing files that changed from the base of the PR and between f9f3917 and 923e22c.

📒 Files selected for processing (2)

• config.go
• config_test.go

[Copilot]

Pull request overview

This PR updates ExpandedProvider’s masquerade expansion so providers can emit a legitimate TLS SNI in more cases (notably when the production client provides no country code), while also preserving any SNI baked into individual masquerades unless an arbitrary SNI is generated.

Changes:

• Apply frontingsnis.default even when countryCode == "", while preserving country-specific overrides.
• Preserve baked-in masquerade sni unless a generated arbitrary SNI is produced (generated > baked-in > empty).
• Extend TestExpandedProvider to cover default-without-country, baked-in preservation, and precedence rules.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
config.go	Updates SNI strategy selection and masquerade SNI precedence during provider expansion.
config_test.go	Adds test cases validating the new default/baked-in SNI behavior and precedence.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

🧹 Nitpick comments (1)

config_test.go (1)

124-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a provider-level VerifyHostname precedence case.

This covers the middle branch in ExpandedProvider: per-masquerade wins, then provider default, then derived Domain.

Suggested test case

+	t.Run("provider VerifyHostname wins over the SNI default", func(t *testing.T) {
+		pinned := "provider.example"
+		pp := &Provider{
+			VerifyHostname: &pinned,
+			Masquerades: []*Masquerade{{Domain: "img.alicdn.com", IpAddress: "1.2.3.4", SNI: "www.mobgslb.tbcache.com"}},
+		}
+		ep := ExpandedProvider(pp, "")
+		require.Len(t, ep.Masquerades, 1)
+		require.NotNil(t, ep.Masquerades[0].VerifyHostname)
+		assert.Equal(t, "provider.example", *ep.Masquerades[0].VerifyHostname)
+	})
+
 	t.Run("per-masquerade VerifyHostname wins over the SNI default", func(t *testing.T) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@config_test.go` around lines 124 - 133, Add a provider-level VerifyHostname
precedence test for ExpandedProvider by extending config_test.go with a case
that sets Provider.VerifyHostname and leaves Masquerade.VerifyHostname unset;
assert the expanded masquerade inherits the provider default. Keep the existing
per-masquerade VerifyHostname check in place so the precedence order in
ExpandedProvider is covered: per-masquerade first, then provider-level default,
then derived Domain fallback.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@config_test.go`:
- Around line 124-133: Add a provider-level VerifyHostname precedence test for
ExpandedProvider by extending config_test.go with a case that sets
Provider.VerifyHostname and leaves Masquerade.VerifyHostname unset; assert the
expanded masquerade inherits the provider default. Keep the existing
per-masquerade VerifyHostname check in place so the precedence order in
ExpandedProvider is covered: per-masquerade first, then provider-level default,
then derived Domain fallback.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4d41e67a-c64f-41ee-b002-d0c78e222644

📥 Commits

Reviewing files that changed from the base of the PR and between 923e22c and 19f6ed1.

📒 Files selected for processing (2)

• config.go
• config_test.go

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.