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

[Feature] lua_scanners - Avast Rest API support #4284

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

Conversation

c-rosenberg
Copy link
Contributor

Copy link
Member

@vstakhov vstakhov left a comment

Choose a reason for hiding this comment

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

It looks good in general, but I'd suggest some small neats.

lua_util.debugm(rule.name, task, '%s: retry IP: %s:%s',
rule.log_prefix, addr, addr:get_port())

http.request(request_data)
Copy link
Member

Choose a reason for hiding this comment

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

I'd also suggest to add upstream to request_data as it will simplify handling. You might also need to remove all calls to upstream:ok and upstream:fail as that will be handled internally by lua_http module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for you review and your suggestions. And sorry for my late response.
I have missed your commits about the http upstreams :)
I will adopt to http_upstreams_by_url, but I need a 3.3 test system with avast first.

requery()
else
-- Parse the response
if upstream then upstream:ok() end
Copy link
Member

Choose a reason for hiding this comment

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

E.g. here...

common.yield_result(task, rule, string.format('Bad HTTP code: %s', code), 1.0, 'fail', maybe_part)
return
end
local data = string.gsub(tostring(body), '[\r\n%s]$', '')
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need that? It copies the text and I see no point in that, as you subsequently parse it via ucl module. It is also redundant for debugging purposes as Rspamd has a way to fix that. So I'd suggest to remove that completely.

rule.log_prefix, data)

local ucl_parser = ucl.parser()
local ok, ucl_err = ucl_parser:parse_string(tostring(body))
Copy link
Member

Choose a reason for hiding this comment

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

Same here, double tostring is double copy and interning. Please remove it for performance and sanity by replacing parse_string with parse_text.

lua_util.debugm(N, task, '%s: JSON OBJECT - %s', rule.log_prefix, result)

local threat_tbl = {}
if result and result.issues and #result.issues > 0 then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if result and result.issues and #result.issues > 0 then
if result and type(result.issues) == 'table' and result.issues[1] then

threat_tbl['WARN:'..r.warning_str] = 'fail'
end
else
rspamd_logger.warnx(task, '%s: generic warning: id: %s - msg: %s',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rspamd_logger.warnx(task, '%s: generic warning: id: %s - msg: %s',
rspamd_logger.messagex(task, '%s: generic warning: id: %s - msg: %s',

end
end

if lua_util.nkeys(threat_tbl) > 0 then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if lua_util.nkeys(threat_tbl) > 0 then
if next(threat_tbl) then

@@ -381,7 +381,8 @@ local function gen_extension(fname)

local ext = {}
for n = 1, 2 do
ext[n] = #filename_parts > n and string.lower(filename_parts[#filename_parts + 1 - n]) or nil
ext[n] = #filename_parts > n
and string.lower(string.gsub(filename_parts[#filename_parts + 1 - n],'[%c%s%p]','')) or nil
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a descriptive comment of what you are changing here. For me, it is not very intuitive atm.


local filename
if rule.use_files then
filename = string.format('%s/%s.tmp',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filename = string.format('%s/%s.tmp',
filename = string.format('%s/%s.tmp.ravast',

To help detecting file leaks if any...


local request_data = {
task = task,
timeout = rule.timeout,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
timeout = rule.timeout,
timeout = rule.timeout,
upstream = upstream,

@vstakhov
Copy link
Member

Test failure is irrelevant.

@bauerstefan
Copy link

Any news on this? We would be very happy to see this feature in rspamd :)

@petol777
Copy link

petol777 commented Sep 20, 2023

Get error using this script:
2023-09-20 11:26:22 #10855(normal) lua_redis_push_data: call to lua_redis callback failed (2): /usr/share/rspamd/lualib/lua_scanners/avast_rest.lua:179: attempt to call field 'gen_extension' (a nil value); trace: [1]:{/usr/share/rspamd/lualib/lua_scanners/avast_rest.lua:179 - fn [Lua]}; [2]:{/usr/share/rspamd/lualib/lua_scanners/common.lua:259 - callback [Lua]}; [3]:{/usr/share/rspamd/lualib/lua_redis.lua:917 - [Lua]};

Get back to avast unix socket way.

@bauerstefan
Copy link

Any news on this? We would be very happy to see this feature in rspamd :)

@fm81
Copy link

fm81 commented Apr 19, 2024

Is there anything new here yet?
I would like to use Avast, whether via REST API or socket.

If it works via the socket, then it would be great if the example could be adapted,
or another example for the use of the socket could be created.

I would be happy if Avast would work with rspamd.

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.

None yet

5 participants