Mitigate DoS by limiting concurrent handshakes #444
Conversation
This commit introduces a semaphore to limit the number of concurrent SSH handshakes to 20. This prevents resource exhaustion (goroutines/memory) during a SYN flood or connection flood attack where attackers stall the handshake process. Additionally, the initial handshake timeout is reduced from 20s to 10s to clear stuck connections faster. Backpressure is applied by blocking the Accept loop when the limit is reached, allowing the kernel backlog to handle the excess load.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
| } | ||
|
|
||
| // Acquire semaphore | ||
| l.handshakeLimit <- struct{}{} |
There was a problem hiding this comment.
Can we do more here to throttle many connections from the same IP address while letting through connections from new IP addresses?
There was a problem hiding this comment.
Added per-IP tracking. Now we limit to 3 concurrent handshakes per IP, while keeping a global safety limit of 20 concurrent handshakes (which is now correctly scoped to the handshake phase only).
There was a problem hiding this comment.
Use github.com/sethvargo/go-limiter for this with the memory backend.
There was a problem hiding this comment.
Switched to using github.com/sethvargo/go-limiter with memory backend for per-IP rate limiting (3 tokens per second). This effectively throttles rapid connection attempts from the same IP while keeping the implementation simple.
This commit introduces mechanism to limit the number of concurrent SSH
handshakes to prevent resource exhaustion (goroutines/memory) during a
SYN flood or connection flood attack.
1. **Global Limit:** A semaphore limits total concurrent handshakes to 20.
This applies backpressure to the `Accept` loop.
2. **Per-IP Limit:** A map limits concurrent handshakes to 3 per IP address.
Excess connections are rejected immediately.
3. **Timeout:** The initial handshake timeout is reduced from 20s to 10s.
The global semaphore is acquired before the handshake and released
immediately after the handshake completes (or fails), ensuring that
long-lived sessions do not consume a handshake slot.
This commit introduces mechanism to limit the number of concurrent SSH
handshakes to prevent resource exhaustion (goroutines/memory) during a
SYN flood or connection flood attack.
1. **Global Limit:** A semaphore limits total concurrent handshakes to 20.
This applies backpressure to the `Accept` loop.
2. **Per-IP Limit:** Using `github.com/sethvargo/go-limiter`, we now
limit the rate of connection attempts per IP to 3 per second.
3. **Timeout:** The initial handshake timeout is reduced from 20s to 10s.
The global semaphore is acquired before the handshake and released
immediately after the handshake completes (or fails), ensuring that
long-lived sessions do not consume a handshake slot.
sshd/net.go
Outdated
| term, err := l.handleConn(conn) | ||
|
|
||
| // Handshake is done (success or failure). Release limits. | ||
| release() |
There was a problem hiding this comment.
Why release() here if we're already doing defer release on L128?
There was a problem hiding this comment.
The defer release() acts as a safety net in case l.handleConn panics or returns early. However, we need to explicitly call release() before l.HandlerFunc(term) is executed. HandlerFunc is the long-running session handler; if we waited for the defer (which runs when the anonymous function exits), the semaphore would be held for the entire duration of the user's session, limiting the server to 20 total active users instead of 20 concurrent handshakes.
I've added a comment to clarify this.
There was a problem hiding this comment.
We don't recover from panics, does it still make sense to be so defensive?
There was a problem hiding this comment.
You're right. If we don't recover from panics, the extra defensiveness adds unnecessary complexity. I've simplified it to just release the semaphore explicitly after the handshake.
This commit introduces mechanism to limit the number of concurrent SSH
handshakes to prevent resource exhaustion (goroutines/memory) during a
SYN flood or connection flood attack.
1. **Global Limit:** A semaphore limits total concurrent handshakes to 20.
This applies backpressure to the `Accept` loop.
2. **Per-IP Limit:** Using `github.com/sethvargo/go-limiter`, we now
limit the rate of connection attempts per IP to 3 per second.
3. **Timeout:** The initial handshake timeout is reduced from 20s to 10s.
The global semaphore is acquired before the handshake and released
immediately after the handshake completes (or fails), ensuring that
long-lived sessions do not consume a handshake slot.
This commit introduces mechanism to limit the number of concurrent SSH
handshakes to prevent resource exhaustion (goroutines/memory) during a
SYN flood or connection flood attack.
1. **Global Limit:** A semaphore limits total concurrent handshakes to 20.
This applies backpressure to the `Accept` loop.
2. **Per-IP Limit:** Using `github.com/sethvargo/go-limiter`, we now
limit the rate of connection attempts per IP to 3 per second.
3. **Timeout:** The initial handshake timeout is reduced from 20s to 10s.
The global semaphore is acquired before the handshake and released
immediately after the handshake completes (or fails), ensuring that
long-lived sessions do not consume a handshake slot.
| host, _, err := net.SplitHostPort(conn.RemoteAddr().String()) | ||
| if err != nil { | ||
| logger.Printf("Failed to split remote addr: %v", err) | ||
| } else { | ||
| // Context with timeout is not strictly needed for memory store, but good practice | ||
| // Although Take is non-blocking for memory store usually. | ||
| _, _, _, ok, err := l.limiter.Take(context.Background(), host) | ||
| if err != nil { | ||
| // Store error (shouldn't happen with memory store unless closed) | ||
| logger.Printf("Rate limiter error: %v", err) | ||
| } else if !ok { | ||
| logger.Printf("[%s] Rejected connection: rate limit exceeded", conn.RemoteAddr()) | ||
| conn.Close() | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Invert the branching to avoid deep nesting, try to keep as flat as possible.
There was a problem hiding this comment.
Good point. I've flattened the branching by removing the else block.
This commit introduces mechanism to limit the number of concurrent SSH
handshakes to prevent resource exhaustion (goroutines/memory) during a
SYN flood or connection flood attack.
1. **Global Limit:** A semaphore limits total concurrent handshakes to 20.
This applies backpressure to the `Accept` loop.
2. **Per-IP Limit:** Using `github.com/sethvargo/go-limiter`, we now
limit the rate of connection attempts per IP to 3 per second.
3. **Timeout:** The initial handshake timeout is reduced from 20s to 10s.
The global semaphore is acquired before the handshake and released
immediately after the handshake completes (or fails), ensuring that
long-lived sessions do not consume a handshake slot.
1 participant
Implemented a semaphore to limit concurrent handshakes and reduced handshake timeout to mitigate DoS attacks.
PR created automatically by Jules for task 16807271499997138569 started by @shazow