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

feat: convert status handler to match existing handlers #175

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

faymard
Copy link

@faymard faymard commented Jan 13, 2025

Hi,

I recently found your plugin and thought it would be useful to easily secure one of my websites against bruteforce attacks.

Expected behaviour

I want a request receiving a given status code to be blocked by the plugin after a few retries.

Bug noticed

While trying it, I noticed that no matter what configuration I put in terms of status codes, I was always getting 403-ed when hitting the maxretry limit. I think #136 relates to my issue.

What I did

Instead of complaining and adding noise to the issue, I thought it would be more interesting to submit a fix :)

What I did is :

  • Converting the status handler to the format used by the others (returning (*chain.Status, error))
  • Adding the handler if the status configuration is there, otherwise only adding the rest (this can change, I wasn't sure of this implementation, I think the handler could always be inserted and simply do nothing if no status codes are given in the configuration)
  • If the status code is allowed, break the chain

Go is clearly not in my field of expertise so please feel free to give implementation advice.

Behaviour with this fix

Now, requests go through if their status code is not contained in the configuration. If they do, they go through the last fail2ban handler before going through.

Thanks in advance for your review !

@tomMoulard tomMoulard self-assigned this Jan 13, 2025
@tomMoulard
Copy link
Owner

Hello @faymard,

Thanks for your interest in this Traefik Plugin!

Do you mind adding a non regression test to make sure your use case does not come back?

Thanks,
Tom

@faymard
Copy link
Author

faymard commented Jan 13, 2025

Hi Tom,

Sure, I'll get on this ASAP.

@faymard
Copy link
Author

faymard commented Jan 15, 2025

Well I realized yesterday (while trying to add some tests) that my implementation was not working as intended. Removing the call to f2b.ShouldAllow in status.go::ServeHttp and relying on the chain did not have the expected impact. I'm going to put this PR in draft mode and I will come back to it

@faymard faymard marked this pull request as draft January 15, 2025 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants