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.

Sign up for GitHub

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

Jump to bottom

comment regarding new rate limiting mechanism #2785

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

comment regarding new rate limiting mechanism #2785

wants to merge 4 commits into from

Conversation

elandorr
Copy link
Contributor

I was confused why you lowered it this dramatically, then saw this one d20c217.

This just changes the comment to make it clear in the generated .env.

I'm still unsure, if such a low number makes sense, though. What about people on VPN/CGNAT?

From the changelog:

Now the rate limiter will only take distinct attempts into account. We have two different types of checks:

to prevent crendential bruteforce (an attacker trying to guess a password), we limit the maximal amount of attempts an attacker has for a given account (from any IP address).

to prevent password spraying (an attacker trying the same common password on all accounts he can enumerate), we limit the maximal number of non-existing accounts an attacker can attempt to authenticate against from a given network subnet.

Maybe the wording is still wrong, and this does not blacklist an IP for trying 5 random accounts, but only the combination of IP+account. Then I can understand why you lowered it so much I suppose. Still seems like that would lock out VPN/TOR/similar users right away. The bots hit all day, thousands of times.

What type of PR?

documentation

What does this PR do?

clarify

@elandorr
comment regarding new rate limiting mechanism
dcee359 
I was confused why you lowered it this dramatically, then saw this one Mailu@d20c217.

This just changes the comment to make it clear in the generated .env.

I'm still unsure, if such a low number makes sense, though. What about people on VPN/CGNAT?

From the changelog:

> Now the rate limiter will only take distinct attempts into account. We have two different types of checks:
> 
>     to prevent crendential bruteforce (an attacker trying to guess a password), we limit the maximal amount of attempts an attacker has for a given account (from any IP address).
> 
>     to prevent password spraying (an attacker trying the same common password on all accounts he can enumerate), we limit the maximal number of non-existing accounts an attacker can attempt to authenticate against from a given network subnet.

Maybe the wording is still wrong, and this does *not* blacklist an IP for trying 5 random accounts, but only the combination of IP+account. Then I can understand why you lowered it so much I suppose. Still seems like that would lock out VPN/TOR/similar users right away. The bots hit all day, thousands of times.
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2023

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Apr 20, 2023
@bors
Try #2785:
9cb5031
@bors
Copy link
Contributor

bors bot commented Apr 20, 2023

try

Build succeeded:

nextgens
nextgens reviewed Apr 20, 2023
View reviewed changes
setup/flavors/compose/mailu.env Outdated
@@ -29,7 +29,7 @@ POSTMASTER={{ postmaster }}
# Choose how secure connections will behave (value: letsencrypt, cert, notls, mail, mail-letsencrypt)
TLS_FLAVOR={{ tls_flavor }}

# Authentication rate limit per IP (per /24 on ipv4 and /48 on ipv6)
# Authentication rate limit per IP for failed login attempts or non-existing accounts (per /24 on ipv4 and /48 on ipv6)
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not accurately reflect what's currently in master; it may if/once #2772 is merged.

Copy link
Member

Choose a reason for hiding this comment

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

@nextgens
#2772 has been merged now. I think this is correct now?

The text itself contains a typo

Suggested change
# Authentication rate limit per IP for failed login attempts or non-existing accounts (per /24 on ipv4 and /48 on ipv6)
# Authentication rate limit per IP for failed login attempts on non-existing accounts (per /24 on ipv4 and /48 on ipv6)

@elandorr
Update mailu.env
03ecd8b 
I took it from the commit in OP. But bb127d1 came right after. Replaced.
elandorr
elandorr commented May 8, 2023
View reviewed changes
Copy link
Contributor Author

@elandorr elandorr left a comment

Choose a reason for hiding this comment

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

I took it from the commit in OP. But bb127d1 came right after, my bad. Replaced.

elandorr
elandorr commented May 8, 2023
View reviewed changes
Copy link
Contributor Author

@elandorr elandorr left a comment •
edited

Choose a reason for hiding this comment

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

or with range

What consequences does this have for CGNAT by the way?
Never mind, unrelated. 1 user can screw a whole /24 which affects even more if CGNAT is used. Maybe I didn't understand this. Why would blocking entire ranges be good? If I happen to be in your subnet be it CGNAT or VPN/TOR, I can ruin your day?

Edit: Sorry for github-mess, I barely got a clue how to navigate this. Not sure I needed to commit to make an edit/suggestion.

ghostwheel42
ghostwheel42 reviewed Jun 23, 2023
View reviewed changes
setup/flavors/compose/mailu.env
@@ -29,7 +29,7 @@ POSTMASTER={{ postmaster }}
# Choose how secure connections will behave (value: letsencrypt, cert, notls, mail, mail-letsencrypt)
TLS_FLAVOR={{ tls_flavor }}

# Authentication rate limit per IP (per /24 on ipv4 and /48 on ipv6)
# Authentication rate limit per IP for failed login attempts on unique non-existing accounts (password spraying counter-measure, per /24 on ipv4 and /48 on ipv6)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also mention AUTH_RATELIMIT_IP_V4_MASK and AUTH_RATELIMIT_IP_V6_MASK.
What about this?

Suggested change
# Authentication rate limit per IP for failed login attempts on unique non-existing accounts (password spraying counter-measure, per /24 on ipv4 and /48 on ipv6)
# Authentication rate limit per network for failed login attempts on unique non-existing accounts (password spraying counter-measure)
# Network size defaults to /24 on IPv4 and to /48 on IPv6 (see also AUTH_RATELIMIT_IP_V4_MASK and AUTH_RATELIMIT_IP_V6_MASK)

@ghostwheel42 ghostwheel42 added the type/documentation Add or updates the documentation label Jun 23, 2023
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 22, 2024

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors-mailu bot added a commit that referenced this pull request Feb 22, 2024
@bors-mailu
Try #2785:
e1b167b
@bors-mailu
Copy link
Contributor

bors-mailu bot commented Feb 22, 2024

try

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nextgens nextgens nextgens left review comments

@ghostwheel42 ghostwheel42 ghostwheel42 left review comments

@Diman0 Diman0 Diman0 left review comments

Assignees
No one assigned
Labels
type/documentation Add or updates the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@elandorr @nextgens @ghostwheel42 @Diman0