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

[Proposal] Update flood protection logic #1559

Open
HumorBaby opened this issue Apr 15, 2019 · 4 comments
Open

[Proposal] Update flood protection logic #1559

HumorBaby opened this issue Apr 15, 2019 · 4 comments

Comments

@HumorBaby
Copy link
Contributor

Flood-prevention measures need to be refactored.

Proposed minimum changes:

  • Move logic into irc.Bot.write
  • Calculate writing backoff (penalty) based on an adequately massaged message (e.g., post-truncation)

Background

RFC 1459 spec on flood control of clients:

8.10 Flood control of clients

   With a large network of interconnected IRC servers, it is quite easy
   for any single client attached to the network to supply a continuous
   stream of messages that result in not only flooding the network, but
   also degrading the level of service provided to others.  Rather than
   require every 'victim' to be provide their own protection, flood
   protection was written into the server and is applied to all clients
   except services.  The current algorithm is as follows:

        * check to see if client's `message timer' is less than
          current time (set to be equal if it is);

        * read any data present from the client;

        * while the timer is less than ten seconds ahead of the current
          time, parse any present messages and penalize the client by
          2 seconds for each message;

   which in essence means that the client may send 1 message every 2
   seconds without being adversely affected.

Current bot layout: (I may have missed some 😯)

** = flood-prevention logic is here
+----------------+    +---------------+    +-------------+
|  Sopel.action  +--->+ **Sopel.say** +--->+             |
|  Sopel.msg     |    +---------------+    |             |    +---------------+    +--------------------------+
|  Sopel.reply   |                         | Sopel.write +--->+ irc.Bot.write +--->+ asynchat.async_chat.send |
+----------------+    +---------------+    |             |    +---------------+    +--------------------------+
                      | Sopel.notice  +--->+             |            .
                      | Sopel.part    |    +-------------+           /|\
                      | Sopel.join    |                               |
                      | Sopel.kick ?  |                               |
                      +---------------+                         +-----------+ 
                                                                | CAP,NICK, |
                                                                | PASS,...  |
                                                                +-----------+

Rationale

A few reasons this change is needed:

  1. The current logic calculates a penalty based on the untruncated message. This led to the fiasco of .dice denial of service #1550 (comment) and .py denial of service #1551.
    • The penalty should be based only on the actual part of the message that is being sent to the server.
  2. All the methods that don't use Sopel.say currently, bypass flood-prevention logic.

Future plans

Potential additional changes:

  • Add configurable backoff
  • Add a configurable maximum penalty
  • Allow message burst (with a configurable number and subsequent delays)
@Exirel
Copy link
Contributor

Exirel commented May 31, 2019

I'm working on that, but I need #1518 to be merged before I can publish anything.

@dgw
Copy link
Member

dgw commented Nov 16, 2019

@Exirel Do you still want to work on this for 7.0, or should we defer it to 7.1?

@Exirel
Copy link
Contributor

Exirel commented Nov 16, 2019

Ok, after looking for it, I think it's best to postpone to 7.1 (if not later).

@dgw dgw modified the milestones: 7.0.0, 7.1.0 Nov 16, 2019
@dgw dgw modified the milestones: 7.1.0, 8.0.0 May 8, 2020
@dgw
Copy link
Member

dgw commented Sep 30, 2020

@HumorBaby I'm about to look at making a small PR for 7.1 that changes the flood penalty calculation to use the truncated message, but regardless of whether that ships or not… I think most of your "Future plans" section is already done, actually.

We have flood_burst_lines (burst quota before triggering Sopel's send-limits), flood_empty_rate (delay between messages when flood-limited), and flood_refill_rate (burst message quota recovery per second) in 7.0 (from #1518); flood_max_wait takes care of a configurable maximum wait time for 7.1 (from #1929). The only thing that might not be present yet is "configurable backoff", but you might consider it partially/wholly obviated by one of the above or remaining new options (flood_penalty_ratio or flood_text_length).

It would benefit Sopel most in the long term to move its flood prevention logic into the IRC backend (so it applies to everything sent, not just what the bot object sends)—a step further than your proposed move into bot.write(). I'm just looking at the other stuff in this issue to see if it's still relevant, or already done in 7.x; those main changes will still wait until 8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants