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

Lua packetlib/v1 #12506

Closed
wants to merge 5 commits into from
Closed

Conversation

victorjulien
Copy link
Member

Example:

```
local packet = require "suricata.packet"

function init (args)
    local needs = {}
    return needs
end

function match (args)
    p = packet.get()
    payload = p:payload()
    ts = p:timestring()

    for line in payload:gmatch("([^\r\n]*)[\r\n]+") do
        if line == "GET /index.html HTTP/1.0" then
            ipver, srcip, dstip, proto, sp, dp = p:tuple()
            SCLogNotice(string.format("%s %s->%s %d->%d (pcap_cnt:%d) match! %s", ts, srcip, dstip, sp, dp, p:pcap_cnt(), line));
            return 1
        end
    end

    return 0
end
```

Methods:
`get` creates the packet object.
`payload` returns the packet payload as a buffer
`packet` returns the whole packet (includes headers)
`pcap_cnt` returns the `pcap_cnt` (pcap file mode only)
`tuple` returns various fields: srcip, dstip, proto, sp, dp
`timestamp` returns time as 2 numbers: seconds and microseconds
`timestring` returns a timestamp as a string

Ticket: OISF#7488.
Moving forward the packetlib is to be used.

Ticket: OISF#7488.
Register internal libs for the case where loading external modules is allowed.
@victorjulien victorjulien requested a review from jasonish January 30, 2025 08:50
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 73.38710% with 33 lines in your changes missing coverage. Please review.

Project coverage is 80.56%. Comparing base (cfbf8fd) to head (1520855).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12506      +/-   ##
==========================================
- Coverage   80.56%   80.56%   -0.01%     
==========================================
  Files         925      926       +1     
  Lines      259292   259329      +37     
==========================================
+ Hits       208906   208929      +23     
- Misses      50386    50400      +14     
Flag Coverage Δ
fuzzcorpus 56.13% <0.80%> (-0.02%) ⬇️
livemode 19.39% <0.00%> (-0.01%) ⬇️
pcap 44.18% <0.00%> (-0.02%) ⬇️
suricata-verify 63.41% <72.58%> (+0.02%) ⬆️
unittests 58.43% <2.41%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24444

Comment on lines -477 to +478
} else {
SCLuaSbLoadLibs(t->luastate);
}
SCLuaSbLoadLibs(t->luastate);
Copy link
Member

@jasonish jasonish Jan 30, 2025

Choose a reason for hiding this comment

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

Keep the if/else. In the case of allowing restricted libs it should look like:

    luaL_openlibs(luastate);
    SCLuaRequirefBuiltIns(luastate);

As SCLuaSbLoadLibs is going to override the requires, plus re-load in already loaded libs, then wipe the ones that are not allowed. SCLuaRequirefBuiltIns just augments the standard libs with ours.

Comment on lines +255 to +256
luaL_setfuncs(luastate, packetlib, 0);
luaL_newlib(luastate, packetlib);
Copy link
Member

Choose a reason for hiding this comment

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

I think the metatable functions and the lib functions should not be shared. Leads a duplicate API and confusion on how it should be used.

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

Comments in-line, plus docs while its top of mind :)

@victorjulien
Copy link
Member Author

Comments in-line, plus docs while its top of mind :)

Agree on the docs, but would like to see first if the current methods make sense.

@victorjulien
Copy link
Member Author

replaced by #12512

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

Successfully merging this pull request may close these issues.

3 participants