Consolidate SNI-DNS check and tighten doctor#474
Draft
dolonet wants to merge 4 commits into9seconds:masterfrom
Draft
Consolidate SNI-DNS check and tighten doctor#474dolonet wants to merge 4 commits into9seconds:masterfrom
dolonet wants to merge 4 commits into9seconds:masterfrom
Conversation
The runtime warning (warnSNIMismatch) and the diagnostic command (doctor checkSecretHost) previously implemented the same SNI-DNS check with different logic: the runtime path was tightened in 9seconds#461 to require every detected IP family to match, but the doctor still accepted any single match. The two now agree. Changes: - Extract the shared check into internal/cli/sni_check.go, returning the resolved addresses and a per-family match status. - Rewrite warnSNIMismatch and checkSecretHost on top of the helper. - Doctor output now reports the mismatched IP family (IPv4, IPv6, or both) and lists the server's public IP alongside the DNS result. - getIP falls back through a short list of public-IP endpoints (ifconfig.co, icanhazip.com, ifconfig.me) instead of relying on a single third-party service.
Review follow-ups: - Run the IPv4 and IPv6 detection in runSNICheck concurrently. With the new three-endpoint fallback in getIP, sequential detection could extend proxy startup by up to 30s per family on a slow/blocked network. Parallel detection bounds the worst case to roughly 30s total instead of 60s. - Make sniCheckResult.OK() self-consistent: it now returns false when the hostname cannot be resolved or no public IP family is known, so callers cannot mistakenly treat 'cannot check' as 'all clear'.
Owner
|
This one is draft so I skip it now |
- Bound public-IP detection with a 10s timeout context. The HTTP fallback chain in getIP could otherwise block proxy startup indefinitely on slow endpoints; the old single DNS lookup could not. Plumbed via context through getIP/fetchPublicIP and added context.WithTimeout in warnSNIMismatch, checkSecretHost, and Access.Run. - Emit a dedicated warning in warnSNIMismatch when the secret hostname resolves successfully but to zero addresses, mirroring the doctor's tplEDNSSNINoResolve branch instead of falling through to a mismatch warning with an empty resolved list. - Allow configuring network.public-ip-endpoints (TOML) / publicIpEndpoints (JSON) so deployments can override the default list (ifconfig.co, icanhazip.com, ifconfig.me). The default is preserved when the option is omitted.
Update example.config.toml: refresh the public-ipv4/public-ipv6 comment to mention the new multi-endpoint detection (no longer solely ifconfig.co), and document the new network.public-ip-endpoints option with the current default values.
Contributor
Author
|
Pushed a follow-up commit:
Will flip out of draft once #3 is settled. |
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
2 participants
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.
Follow-up to #461. The runtime warning now requires every detected IP family to match, but
mtg doctorstill passes on any single match, and the detection logic is duplicated across the two sites.This PR:
internal/cli/sni_check.go.warnSNIMismatchandDoctor.checkSecretHoston top of it, so doctor uses the same all-families-must-match rule.getIPfalls back throughifconfig.co→icanhazip.com→ifconfig.meinstead of depending on a single endpoint.No connectivity-probe or policy changes beyond that — happy to split further if you'd prefer, or fold in more of what you had in mind with "the whole logic there must be revised, especially connectivity part."