Repository navigation
feat: opt-in per-IP limit on pending handshakes - #579
Open
AndreyOsipuk wants to merge 2 commits into
Open
AndreyOsipuk wants to merge 2 commits into
AndreyOsipuk wants to merge 2 commits into
Conversation
A connection holds a worker until it authenticates, goes to the fronting
domain or hits the handshake timeout. One address that opens hundreds of
connections and never completes the handshake can keep the whole
concurrency budget busy and starve everyone else.
[defense.pending-handshakes] max-per-ip limits concurrent unauthenticated
handshakes from one IP. The slot is released as soon as the secret is
verified or the connection is fronted, so authenticated sessions and
bursts of short media connections (authenticated within milliseconds) are
not counted. dry-run observes the limit without enforcing it. Disabled by
default. Event PendingHandshakeLimit, metric pending_handshake_limit{action}.
The allowlist loads asynchronously and rejects every connection until it is ready, which on a slow CI runner looked like a rejection by the limit (0 admitted). Also apply De Morgan's law as staticcheck suggests.
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
1 participant
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.
Why
A connection occupies a worker (
concurrency) until it authenticates, goes to the fronting domain or hits the handshake timeout. A single address that opens hundreds of connections and never completes the handshake can keep the whole budget busy, and every other client getsconcurrency limited. We hit exactly this in production on another MTProto proxy (telemt): one client opened ~1100 TCP connections and the proxy stopped serving everyone. mtg has the same exposure: there is no per-IP limit of any kind.Limiting connections per IP at the firewall (
connlimit) is the wrong tool: behind carrier-grade NAT many real users share one address, and Telegram opens a burst of connections when sending media. We tried it and it cut photo uploads for real users.What
dry-runadmits everything and reports what would have been rejected, to pick a threshold from real traffic first.EventPendingHandshakeLimit; Prometheus/StatsD metricpending_handshake_limit{action="rejected"|"observed"}.The same limit is proposed for telemt (telemt/telemt#950) and for mtg-multi (dolonet#47).
Tests
Unit tests for the limiter (disabled, reject and release, dry-run, independent addresses, concurrent acquire/release never exceeds the limit) and an end-to-end proxy test (stalled handshakes from one IP: the extra connection is closed, other IPs are served, the slot is freed when a handshake ends).
go vet ./...,go test -race ./...andGOOS=windows go build ./...pass. Disabled by default, no behaviour change unless configured.