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

scripts/analyzer: Introduce requested_analyzers #2507

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

awelzel
Copy link
Contributor

@awelzel awelzel commented Oct 26, 2022

In certain deployment scenarios, all or some analyzers are disabled by default. However, conditionally/optionally loaded scripts may require them.

This change adds a set in the Analyzer module where where external scripts can record the requirement/request for a certain analyzer to subsequently act on these.

@awelzel awelzel marked this pull request as draft October 26, 2022 16:54
@awelzel
Copy link
Contributor Author

awelzel commented Oct 26, 2022

@sethhall @sowmyaramapatruni - I took a first stab around the required/requested analyzer topic.

IIUC, the main driver to have this in Zeek base is for external scripts that use this sort of mechanism to not rely/require a custom implementation and continue to work easily with a vanilla open-source distribution whenever released to the public.

Some points that come up:

  1. Does this work for you?
  2. Naming suggestions?
  3. Would the redef be sufficient, or is a dynamic Analyzer::request_analyzer() still useful?

Could you maybe outline why using Analyzer::enable_analyzer() within zeek_init() in those external scripts wouldn't be enough? The declarative approach is nice, but currently lost why the enable_analyzer() wasn't enough.

@awelzel
Copy link
Contributor Author

awelzel commented Nov 15, 2022

@sowmyaramapatruni / @sethhall - did you have a chance to take a look?

@sowmyaramapatruni
Copy link
Contributor

Hey Arne,

Sorry for the delayed response. Thankyou for writing this. I have couple of questions.

  1. Does this work only for enabling analyzers? why not for disabling. I see an option "redef Analyzer::disable_all = T;" for disabling all analyzers, but not for individual analyzers.
  2. Will the function work for Spicy analyzers too?
  3. Naming looks good and for your last question on redef, I am not sure if redef works for sensor-core. @sethhall could answer better.

@awelzel
Copy link
Contributor Author

awelzel commented Nov 22, 2022

  1. Does this work only for enabling analyzers? why not for disabling. I see an option "redef Analyzer::disable_all = T;" for disabling all analyzers, but not for individual analyzers.

For disabling analyzer there already exists a set Analyzer::disabled_analyzers which can be used to disable individual analyzers. These are disabled at zeek_init() time, something that for requested_analyzers is conditional.

Could certainly see calling it Analyzer::enabled_analyzers as well, but because the request was to have a place to stash analyzer tags and have external scripts enable/disable scripts, the two options seemed more reasonable.

  1. Will the function work for Spicy analyzers too?

Yes.

  1. Naming looks good and for your last question on redef, I am not sure if redef works for sensor-core. @sethhall could answer better.

It should, but happy to hear also Seth's thoughts.

@awelzel awelzel force-pushed the topic/awelzel/analyzer-requested-analyzers branch from 3d29339 to d877cd2 Compare December 12, 2022 17:53
@awelzel
Copy link
Contributor Author

awelzel commented Dec 12, 2022

Chatted with @sethhall out of band. He suggested to remove the enable_requested_analyzer option and do enabling of requested analyzers by default.

I'm moving this out of draft/rfc state to be merged (or better names suggested :-) )

@awelzel awelzel changed the title RFC: scripts/analyzer: Introduce requested_analyzers scripts/analyzer: Introduce requested_analyzers Dec 12, 2022
@awelzel awelzel force-pushed the topic/awelzel/analyzer-requested-analyzers branch from d877cd2 to e8e79d1 Compare December 12, 2022 17:55
@awelzel awelzel marked this pull request as ready for review December 12, 2022 17:56
NEWS Outdated Show resolved Hide resolved
scripts/base/frameworks/analyzer/main.zeek Outdated Show resolved Hide resolved
In certain deployment scenarios, all analyzers are disabled by default.
However, conditionally/optionally loaded scripts may rely on analyzers
functioning and declare a request for them.

Add a global set set to the Analyzer module where external scripts can record
their requirement/request for a certain analyzer. Analyzers found in this
set are enabled at zeek_init() time.
@awelzel awelzel force-pushed the topic/awelzel/analyzer-requested-analyzers branch from e8e79d1 to 4e75d54 Compare December 13, 2022 13:28
Copy link
Member

@ckreibich ckreibich left a comment

Choose a reason for hiding this comment

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

Thanks Arne!

@awelzel awelzel self-assigned this Jan 10, 2023
@awelzel awelzel merged commit ebf1a19 into master Jan 10, 2023
@awelzel awelzel deleted the topic/awelzel/analyzer-requested-analyzers branch January 10, 2023 09:34
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

3 participants