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

analyze: enable sigs consuming sigs #4327

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

NDStrahilevitz
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz commented Sep 25, 2024

1. Explain what the PR does

df567eb fix(analyze): enable sigs consuming sigs
ad56d06 fix(findings): clone properties before rewriting
84ceba7 chore: move finding event conversion to a package
3b32997 chore: decouple sig init from init

df567eb fix(analyze): enable sigs consuming sigs

Signatures consuming other signatures relied on the single binary
reprocessing finding events into the pipeline. This did not occur in
analyze mode. Introduce that mechanism so that it now works.

Co-authored-by: Asaf Eitani <[email protected]>

84ceba7 chore: move finding event conversion to a package

Opportunistic refactor. Logic does not relate to eBPF and does relate to
event data. Also allows importing this logic without importing eBPF
related code.

3b32997 chore: decouple sig init from init

This allows initializing signature to event data without importing eBPF
initialization logic, which require specific compilation tags.

Co-authored-by: Asaf Eitani <[email protected]>

2. Explain how to test it

Currently we have internal tests for it. Confirmed it was working using those.

3. Other comments

Resolve #4326

geyslan

This comment was marked as outdated.

@geyslan geyslan self-requested a review September 26, 2024 20:13
@geyslan geyslan dismissed their stale review September 26, 2024 20:15

Analyze is emitting warnings due to sig of sigs loading lack.

@geyslan
Copy link
Member

geyslan commented Sep 26, 2024

I tested it again since I noticed you removed the recursive loading what made the initial behaviour stands. For each selected event that is a signature, analyze emits an error:

{"level":"error","ts":1727381548.0349305,"msg":"Failed to load event dependency","event":"internal_sig"}
...

@NDStrahilevitz
Copy link
Collaborator Author

I tested it again since I noticed you removed the recursive loading what made the initial behaviour stands. For each selected event that is a signature, analyze emits an error:

{"level":"error","ts":1727381548.0349305,"msg":"Failed to load event dependency","event":"internal_sig"}
...

Weird, I haven't seen it in my test. Maybe because I checked with grep, let me see.

@NDStrahilevitz NDStrahilevitz force-pushed the fix_sigs_from_sigs branch 2 times, most recently from 7320464 to b9522b5 Compare October 22, 2024 12:41
@NDStrahilevitz
Copy link
Collaborator Author

Manually tested on both tracee-rules and analyze.
@geyslan please review at your pace.

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM

NDStrahilevitz and others added 4 commits October 29, 2024 18:52
This allows initializing signature to event data without importing eBPF
initialization logic, which require specific compilation tags.

Co-authored-by: Asaf Eitani <[email protected]>
Opportunistic refactor. Logic does not relate to eBPF and does relate to
event data. Also allows importing this logic without importing eBPF
related code.
Signatures consuming other signatures relied on the single binary
reprocessing finding events into the pipeline. This did not occur in
analyze mode. Introduce that mechanism so that it now works.

Co-authored-by: Asaf Eitani <[email protected]>
@NDStrahilevitz
Copy link
Collaborator Author

Waiting for a final review by @geyslan before merging.
I've made changes such that the feedback concern is handled entirely in the engine context and not in analyze. Later on, the feedback should be included into derived events and fed into the processing stage.
Further, i've resolved some crash issues that were introduced in initial versions of the PR, a concurrency safety issue in finding->event conversion, and made sure that analyze mode remains as deterministic as it was before (it is msotly deterministic in output size but not in order, which may affect output size given a signature dependent on order).

@geyslan
Copy link
Member

geyslan commented Nov 4, 2024

SGTM.

@NDStrahilevitz
Copy link
Collaborator Author

/fast-forward

Copy link

github-actions bot commented Nov 5, 2024

Triggered from #4327 (comment) by @​NDStrahilevitz.

Trying to fast forward main (c8032d2) to fix_sigs_from_sigs (df567eb).

Target branch (main):

commit c8032d24da01d5d719628a4a6e7fa320591b08df (HEAD -> main, origin/main, origin/HEAD)
Author: Raphael Campos <[email protected]>
Date:   Thu Oct 31 11:04:04 2024 -0500

    chore(k8s): prepare v0.22.3 release

Pull request (fix_sigs_from_sigs):

commit df567ebc7c8da1288f879108c13466cccc776590 (pull_request/fix_sigs_from_sigs)
Author: Nadav Strahilevitz <[email protected]>
Date:   Mon Sep 23 15:44:00 2024 +0000

    fix(analyze): enable sigs consuming sigs
    
    Signatures consuming other signatures relied on the single binary
    reprocessing finding events into the pipeline. This did not occur in
    analyze mode. Introduce that mechanism so that it now works.
    
    Co-authored-by: Asaf Eitani <[email protected]>

Can't fast forward main (c8032d2) to fix_sigs_from_sigs (df567eb). main (c8032d2) is not a direct ancestor of fix_sigs_from_sigs (df567eb). Branches appear to have diverged at 3f60a6f:

* c8032d24da01d5d719628a4a6e7fa320591b08df chore(k8s): prepare v0.22.3 release
* 03208e5a9176333183020cc6a5d90cb1545f075e fix(mount): reintroduce root path requirement
* eb046449fdf135fad069ae9307482665f7dd07e8 changed process filter to scope filters (#4371)
| * df567ebc7c8da1288f879108c13466cccc776590 fix(analyze): enable sigs consuming sigs
| * ad56d06f85cf685d7b3929293503a8448b2c8dbf fix(findings): clone properties before rewriting
| * 84ceba74a7bcf51eed138fbe93d8694dc60cb417 chore: move finding event conversion to a package
| * 3b329970758c15c75d1b9eac0c52ad26b67186f9 chore: decouple sig init from init
|/  
* 3f60a6fae7f0bcca2ecc5e36f3366839dbe8f6e7 fix(time): ticks is uint64

commit 3f60a6fae7f0bcca2ecc5e36f3366839dbe8f6e7
Author: Geyslan Gregório <[email protected]>
Date:   Tue Oct 22 17:23:52 2024 -0300

    fix(time): ticks is uint64
    
    The field started_time fetched from /proc stat file and passed as ticks
    to ClockTicksToNsSinceBootTime() is an uint64.

Rebase locally, and then force push to fix_sigs_from_sigs.

@NDStrahilevitz NDStrahilevitz merged commit 1d99241 into aquasecurity:main Nov 5, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Ad-Hoc analysis of signatures which consume other signatures
2 participants