Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ssh connection rate limiting feature #1469

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dbathgate
Copy link

@dbathgate dbathgate commented May 2, 2024

Issue

  • The BBR tool opens and closes multiple ssh connections during a single run
  • Issues have been encountered with firewalls considering the multiple ssh connections as a brute force attack and kills the connections
  • This particular firewall is configured to limit the amount of connections to a distinct host per a duration window (example: 20 connections per minute)

Implementation

  • Added a rate limiting feature that will throttle opening of new connections if too many have been opened within a given time frame
  • The amount of connections and the duration window are both configurable, and the rate limiting feature itself can be toggled on

Example Usage:

bbr deployment \
--rate-limiting \
--rate-limiting-max-connections 10 \
--rate-limiting-duration 60s \
--deployment <deployment> backup

Impact

  • Without this feature, backups cannot be completed with the given firewall configuration in place

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@rkoster rkoster requested review from a team, jpalermo, rkoster and dlresende and removed request for a team and jpalermo May 6, 2024 09:23
Copy link

@rkoster rkoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of this PR, but would it be possible to use the go standard library for the limiter?
From briefly looking at the docs: https://pkg.go.dev/golang.org/x/time/rate
It seems like something like a combination of rate.Every and rate.NewLimiter should achieve the same result.

@cniles
Copy link

cniles commented May 9, 2024

Hi @rkoster, I've been working with Darren on this.

An equivalent call with the current args looks like NewLimiter(maxConnections/duration, 1) which is naive to bursting.

Do you have a suggestion for how bursting could be accounted for on the command line args?

Some options are :

  • Just ignore bursting and keep b fixed to 1.
  • Expose the NewLimiter interface by having the user provide r and b arguments, e.g. --rate-limiter-rate and --rate-limiter-burst instead of one for max connections and duration.
  • Keep the current args and just add an additional --rate-limiter-burst arg that defaults to 1.

@dbathgate dbathgate force-pushed the connection-throttling branch 4 times, most recently from 66838c6 to c19e57c Compare June 19, 2024 19:16
@alexbarbato
Copy link

Y'all waiting on another review from @rkoster here @dbathgate?

- allows enabling ssh connection rate limiting
- adds configurable amount of max connections per duration window
- adds configurable duration window

Signed-off-by: Darren Bathgate <[email protected]>
@dbathgate
Copy link
Author

Y'all waiting on another review from @rkoster here @dbathgate?

We attempted to use the rate limiter library provided by Go, but it was not working as expected for our needs. The rate limiter was still tripping the firewall brute force rule, and was running slower than the original version. We reverted back to the version I have in this pull request, and have been running this successfully for over 3 months.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for Changes | Open for Contribution
Development

Successfully merging this pull request may close these issues.

5 participants