zeek / package-template Goto Github PK

Using the template to create a new protocol analyzer does not work as expected:

$ zkg --user create --template https://github.com/zeek/package-template --version v2.0.0 --features spicy-protocol-analyzer --packagedir spicy-template-test
...
$ mkdir build && cd build && cmake ..
$ make
[ 50%] Compiling Template analyzer
error: PCH file uses an older PCH format that is no longer supported
1 error generated.
error: PCH file uses an older PCH format that is no longer supported
1 error generated.
error: PCH file uses an older PCH format that is no longer supported
1 error generated.
error: PCH file uses an older PCH format that is no longer supported
1 error generated.
error: PCH file uses an older PCH format that is no longer supported
1 error generated.
[error] <Spicy Plugin for Zeek>: JIT compilation failed
make[2]: *** [spicy-modules/template.hlto] Error 1
make[1]: *** [analyzer/CMakeFiles/Template.dir/all] Error 2
make: *** [all] Error 2

And while I'm here. Dropping spicy-protocol-analyzer from the list of selected features doesn't seem to have any effect. Zkg is still complaining of a mutual exclusion conflict for spicy-protocol-analyzer even though it was not selected. Perhaps it was pulled in through a dependency?

% zkg create --packagedir foobar --features license plugin spicy-file-analyzer spicy-packet-analyzer
"package-template" requires a "name" value (the name of the package, e.g. "FooBar" or "spicy-http"):
name: FooBar
"package-template" requires a "namespace" value (a namespace for the package, e.g. "MyOrg"):
namespace: MyOrg
"package-template" requires a "analyzer" value (name of the Spicy analyzer, which typically corresponds to the protocol/format being parsed (e.g. "HTTP", "PNG")):
analyzer: HTTP
"package-template" requires a "unit" value (name of the top-level Spicy parsing unit for the file/packet format (e.g. "File" or "Packet")):
unit: File
"package-template" requires a "author" value (your name and email address):
author: [email protected]
"package-template" requires a "license" value (one of apache, bsd-2, bsd-3, mit, mpl-2):
license: bsd-3
error: template input invalid, the "plugin" and "spicy-protocol-analyzer" features are mutually exclusive

Originally posted by @pauldokas in zeek/package-manager#167 (comment)

There's supposed to be a pointer here: https://github.com/zeek/package-template/blob/master/features/plugin/plugin/src/%40SLUG%40.bif#L9

The package template needs CI coverage — the combination of features, flags, moving Zeek versions etc is too fragile to assume it just works. @rsmmr got bitten by this recently, and I once nearly pushed a breaking change to master because I didn't try out an offending combination of settings.

This relates to zeek/action-zkg-install#5: we should verify that the CI feature actually runs successfully.

IIUC, the spicy:: prefix causes the analyzer tag to be prefixed with SPICY_ which in turn ends up in the conn.log service field as spicy_<analyzer>. This causes the "tech" used to write an analyzer to leak through into the logs.

Should we remove the spicy:: prefix from the template?

Reference zeek/zeek#2651

Since packages with plugins know their build directory, they should add it, not the package's toplevel, to ZEEK_PLUGIN_DIR. Being more specific here will avoid Zeek traversing into unexpected and possibly transient directories.

See zeek/btest#98 for related discussion.

Right now we generate code where ports are specified in EVT files. This makes it hard for users to change ports after the fact (e.g., impossible to disable analyzer for hardcoded ports). We should change the template to instead declare a redef'able script ports constant and on zeek_init call Analyzer::register_for_ports; the EVT port setting should then go.

For the plugin feature, including a header from the plugin sources doesn't work in BiF files:

module SomePlugin;

%%{
#include "MyPluginHeader.h"
%%}

@awelzel found this to be an include path issue: BiFs seems to be looking at src while the sources are in plugin/src.

I believe the culprit here is the CMake revamp removing support for BRO_PLUGIN_BASE, but opening a ticket in the template repo as I'm not sure we want to reintroduce that and/or maybe there's a better idea how to go about that.

Today, when creating a package with a plugin feature, there's two scripts directories - top-level and plugin specific.

~/foobar$ tree scripts/ ./plugin
scripts/
├── __load__.zeek
└── main.zeek
./plugin
├── scripts
│   ├── __load__.zeek
│   ├── __preload__.zeek
│   └── types.zeek
└── src
    ├── config.h.in
     ...

When running zkg install (or make install within build), the scripts/ directory next to the installed lib/ directory contains the content of the top-level ./scripts directory rather than ./plugin/scripts (which would have the __preload__.zeek or types.zeek files). This seems wrong: The plugin/scripts should've been sued for this.

Concretely, after a make install within ./build:

/opt/zeek-6.2/lib/zeek/plugins/Zeek_FooBar/
├── COPYING
├── lib
│   ├── bif
│   │   ├── foobar.bif.zeek
│   │   └── __load__.zeek
│   └── Zeek-FooBar.linux-x86_64.so
├── README
├── scripts
│   ├── __load__.zeek
│   └── main.zeek
├── VERSION
└── __zeek_plugin__

So 1) we won't have and load the right __load__.zeek, __preload__.zeek and types.zeek files automatically when the shared library is loaded and also 2) Zeek automatically attempts to load the top-level ./scripts directory instead (which should require an explicit @load).

$ zeek -e 'event zeek_init(){}'
Hello world!

@ckreibich - thoughts? Bringing back BRO_PLUGIN_BASE could work, but its usage seems brittle (as this ticket proofs).

I don't know. Chatting with @bbannier and @J-Gras this is pretty messy. Given there have been no complaints so far, I wonder if we can do some more drastic changes rather than trying to fix anything up.

I'm fairly used to build and install plugins via ./configure && make && make install, but the plugin feature doesn't seem to create a top-level Makefile.

Was that somehow on purpose or could we consider adding one?

Given Zeek's deterministic option:

$ zeek --help 2>&1 | grep -- -D
    -D|--deterministic              | initialize random seeds to zero

Does it make sense to remove the random.seed file from the templates and use -D instead? How can the option be passed automatically? It wasn't clear to me how the RANDOM_SEED var is used.

The current template provides a skeleton connection_state_remove() handler for final log writing. As far as I understand Zeek base uses removal hooks throughout so that only hooks for appropriate connections are executed and not for every connection.

We should update the template to adhere to this.

We've historically long had an unconditional ZEEK_PLUGIN_PATH line in the tests' btest.cfg, but it shouldn't be there unless the package has a plugin feature.

See zeek/btest#98 for related discussion.