#807 mod_firewall IN_ROSTER slows down all stanzas
Reporter
Thijs Alkemade
Owner
MattJ
Created
Updated
Stars
★ (1)
Tags
Component-Community
Priority-Low
Type-Enhancement
Status-New
Thijs Alkemade
on
The documentation of mod_firewall currently says for IN_ROSTER:
> Performance note: this check can potentially cause storage access (especially if the recipient is currently offline), so you may want to limit its use in high-traffic situations, and place it below other checks (such as a rate limiter).
However, that doesn't actually work.
For example, a filter of:
KIND: message
TYPE: chat
IN_ROSTER: no
LOG=Bouncing spam message.
BOUNCE.
Results in the generated code:
do -- File /etc/prosody/blocklist.pfw
---- Chain deliver
chain_1_deliver = function (definitions, fire_event, log)
local rates = definitions.RATE;
local zones = definitions.ZONE;
local rostermanager = require "core.rostermanager";
local jid_split = require "util.jid".split;
local jid_bare = require "util.jid".bare;
local st = require "util.stanza";
local db = require 'util.debug';
return function (event)
local stanza, session = event.stanza, event.origin;
-- name
local name = stanza.name;
-- type
local type = stanza.attr.type;
-- to
local to = stanza.attr.to or jid_bare(session.full_jid);
-- split_to
local to_node, to_host, to_resource = jid_split(to);
-- from
local from = stanza.attr.from;
-- bare_from
local bare_from = jid_bare(from)
-- roster_entry
local roster_entry = (to_node and rostermanager.load_roster(to_node, to_host) or {})[bare_from];
if (name == "message") and ((type or (name == 'message' and 'normal') or (name == 'presence' and 'available')) == "chat") and (not roster_entry) then
-- LOG=Bouncing spam message.
(session.log or log)("info", "Bouncing spam message.");
-- BOUNCE.
if type == "error" or (name == "iq" and type == "result") then return true; end -- Don't reply to 'error' stanzas, or iq results
local newstanza = st.error_reply(stanza, "cancel", "service-unavailable", nil); session.send(newstanza); return true;
end
end;
end;
---- End of chain deliver
end -- End of file /etc/prosody/blocklist.pfw
Note that the load_roster call is before the check (name == "message") and ((type or (name == 'message' and 'normal') or (name == 'presence' and 'available')) == "chat"). Therefore the call will be done for every stanza.
If load_roster was part of the 'if' conditional, it would not be invoked as the first "false" for the chain of "and"s would terminate it.
(It might be possible to do this efficiently using chains or marking sessions, but I think that even the simple case should be optimized.)
Zash
on
I wonder if compiling into a pyramid like this would be better:
dependencies for first condition here
if (first condition) then
dependencies for second condition here
if (second condition) then
dependencies for third condition
if (third condition) then
etc ... until the actual action
end end end
Zash
on
I believe Matthew considered this kind of issue to be of low priority.
The documentation of mod_firewall currently says for IN_ROSTER: > Performance note: this check can potentially cause storage access (especially if the recipient is currently offline), so you may want to limit its use in high-traffic situations, and place it below other checks (such as a rate limiter). However, that doesn't actually work. For example, a filter of: KIND: message TYPE: chat IN_ROSTER: no LOG=Bouncing spam message. BOUNCE. Results in the generated code: do -- File /etc/prosody/blocklist.pfw ---- Chain deliver chain_1_deliver = function (definitions, fire_event, log) local rates = definitions.RATE; local zones = definitions.ZONE; local rostermanager = require "core.rostermanager"; local jid_split = require "util.jid".split; local jid_bare = require "util.jid".bare; local st = require "util.stanza"; local db = require 'util.debug'; return function (event) local stanza, session = event.stanza, event.origin; -- name local name = stanza.name; -- type local type = stanza.attr.type; -- to local to = stanza.attr.to or jid_bare(session.full_jid); -- split_to local to_node, to_host, to_resource = jid_split(to); -- from local from = stanza.attr.from; -- bare_from local bare_from = jid_bare(from) -- roster_entry local roster_entry = (to_node and rostermanager.load_roster(to_node, to_host) or {})[bare_from]; if (name == "message") and ((type or (name == 'message' and 'normal') or (name == 'presence' and 'available')) == "chat") and (not roster_entry) then -- LOG=Bouncing spam message. (session.log or log)("info", "Bouncing spam message."); -- BOUNCE. if type == "error" or (name == "iq" and type == "result") then return true; end -- Don't reply to 'error' stanzas, or iq results local newstanza = st.error_reply(stanza, "cancel", "service-unavailable", nil); session.send(newstanza); return true; end end; end; ---- End of chain deliver end -- End of file /etc/prosody/blocklist.pfw Note that the load_roster call is before the check (name == "message") and ((type or (name == 'message' and 'normal') or (name == 'presence' and 'available')) == "chat"). Therefore the call will be done for every stanza. If load_roster was part of the 'if' conditional, it would not be invoked as the first "false" for the chain of "and"s would terminate it. (It might be possible to do this efficiently using chains or marking sessions, but I think that even the simple case should be optimized.)
I wonder if compiling into a pyramid like this would be better: dependencies for first condition here if (first condition) then dependencies for second condition here if (second condition) then dependencies for third condition if (third condition) then etc ... until the actual action end end end
I believe Matthew considered this kind of issue to be of low priority.
Changes