Bug 1419226 - Review Request: prelude-correlator
Summary: Review Request: prelude-correlator
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-02-04 00:30 UTC by Thomas Andrejak
Modified: 2017-03-10 13:01 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-03-10 11:20:06 UTC
Type: Bug
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)

Description Thomas Andrejak 2017-02-04 00:30:03 UTC
This is the continuation of #1386938 . This ticket is for prelude-correlator, the correlator of events received by Prelude.

Description in spec :
Prelude-Correlator allows conducting multi-stream correlations
thanks to a powerful programming language for writing correlation
rules. With any type of alert able to be correlated, event
analysis becomes simpler, quicker and more incisive. This
correlation alert then appears within the Prewikka interface
and indicates the potential target information via the set of
correlation rules.

SPEC : https://fedorapeople.org/~totol/prelude-correlator.spec
SRPM : https://fedorapeople.org/~totol/prelude-correlator-3.1.0-1.fc26.src.rpm
Build : https://koji.fedoraproject.org/koji/taskinfo?taskID=17579929

Thanks for the review

Regards

Thomas

Comment 1 Zbigniew Jędrzejewski-Szmek 2017-02-04 16:25:31 UTC
No python 3 support? Please note that python3 support MUST be enabled if possible.

"install init script" → not to be picky, but it's neither init nor a script ;)

In one place you use install -D, in others the directories are created by hand.

It looks all good, only cosmetic issues, apart from the one big question — python3.

Comment 2 Thomas Andrejak 2017-02-08 08:34:21 UTC
Hello,

Here are the new files :
SPEC : https://fedorapeople.org/~totol/prelude-correlator.spec
SRPM : https://fedorapeople.org/~totol/prelude-correlator-3.1.0-1.fc26.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> No python 3 support? Please note that python3 support MUST be enabled if
> possible.

=> I had a patch to have it :)

> 
> "install init script" → not to be picky, but it's neither init nor a script
> ;)

Done

> 
> In one place you use install -D, in others the directories are created by
> hand.

Done

> 
> It looks all good, only cosmetic issues, apart from the one big question —
> python3.

Comment 3 Zbigniew Jędrzejewski-Szmek 2017-02-09 00:10:40 UTC
s/SystemD/systemd/

+ package name is OK
+ license is acceptable (GPLv2)
- license is specified correctly:
  not, it's GPLv2+, see the headers in the files

Why /etc/prelude-correlator/rules? That python code does not look like something that is configurable, wouldn't it be better to put it somewhere in /usr/?

Does the binary fork anything? If there's just one process, you can do away with the pid file, which would obviate the need for a directory in /run and simplify things.

Could this daemon run as a normal user? What kind of privileges does is require?

Hm, why do the python subpackage require prelude-correlator.rpm? prelude-correlator includes a service file which will not work without python3-p-c, so it seems prelude-correlator.rpm should require python3-prelude-correlator. I'm pretty sure that 'dnf install prelude-correlator' should yield a working setup.

Comment 4 Thomas Andrejak 2017-02-09 09:58:58 UTC
Hello,

Here are the new files :
SPEC : https://fedorapeople.org/~totol/prelude-correlator.spec
SRPM : https://fedorapeople.org/~totol/prelude-correlator-3.1.0-1.fc26.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> s/SystemD/systemd/
> 
> + package name is OK
> + license is acceptable (GPLv2)
> - license is specified correctly:
>   not, it's GPLv2+, see the headers in the files

Indeed, Done

> 
> Why /etc/prelude-correlator/rules? That python code does not look like
> something that is configurable, wouldn't it be better to put it somewhere in
> /usr/?

This is python files but this is really configuration files. This is where the user describes it's correlation rules. The user can add rules, delete rules, modify rules.

> 
> Does the binary fork anything? If there's just one process, you can do away
> with the pid file, which would obviate the need for a directory in /run and
> simplify things.

Yes, Done

> 
> Could this daemon run as a normal user? What kind of privileges does is
> require?

Prelude-Correlator seems to have this functionnality but it dit it in the wrong way. I prefere not to activate it and wait for upstream.

> 
> Hm, why do the python subpackage require prelude-correlator.rpm?
> prelude-correlator includes a service file which will not work without
> python3-p-c, so it seems prelude-correlator.rpm should require
> python3-prelude-correlator. I'm pretty sure that 'dnf install
> prelude-correlator' should yield a working setup.

You are right, Done

Comment 5 Zbigniew Jędrzejewski-Szmek 2017-02-10 00:03:31 UTC
> Prelude-Correlator seems to have this functionnality but it dit it in the wrong way. I prefere not to activate it and wait for upstream.

With systemd's User= setting, running as non-root requires no special support from the application, often. This gives an excellent ratio of security gain vs. inconvenience.

That said, if you prefer to wait for upstream, that makes sense too. (Although they are more likely to implement it "portably", i.e. from scratch, and mess something up ;))

(In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> s/SystemD/systemd/
Still there.

>> Hm, why do the python subpackage require prelude-correlator.rpm?
>> prelude-correlator includes a service file which will not work without
>> python3-p-c, so it seems prelude-correlator.rpm should require
>> python3-prelude-correlator. I'm pretty sure that 'dnf install
>> prelude-correlator' should yield a working setup.
> You are right, Done

I don't think that's good as is. I think the main package (%name.rpm) should have
Requires: python3-%{name}

(Otherwise, if you do dnf install %{name}, systemctl start %{name}, this will crash on failed import).

Comment 6 Thomas Andrejak 2017-02-12 23:11:35 UTC
Hello,

Here are the new files :
SPEC : https://fedorapeople.org/~totol/prelude-correlator.spec
SRPM : https://fedorapeople.org/~totol/prelude-correlator-3.1.0-1.fc26.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
> > Prelude-Correlator seems to have this functionnality but it dit it in the wrong way. I prefere not to activate it and wait for upstream.
> 
> With systemd's User= setting, running as non-root requires no special
> support from the application, often. This gives an excellent ratio of
> security gain vs. inconvenience.
> 
> That said, if you prefer to wait for upstream, that makes sense too.
> (Although they are more likely to implement it "portably", i.e. from
> scratch, and mess something up ;))

I understand but for example, if the root user change things in /etc/prelude-correlator , prelude-correlator may stop working because it can not access / remove / modify files. For now, I prefer let it in this way if it is possible.

> 
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> > s/SystemD/systemd/
> Still there.
> 

I don't see it in your comment, sorry. Done

> >> Hm, why do the python subpackage require prelude-correlator.rpm?
> >> prelude-correlator includes a service file which will not work without
> >> python3-p-c, so it seems prelude-correlator.rpm should require
> >> python3-prelude-correlator. I'm pretty sure that 'dnf install
> >> prelude-correlator' should yield a working setup.
> > You are right, Done
> 
> I don't think that's good as is. I think the main package (%name.rpm) should
> have
> Requires: python3-%{name}
> 
> (Otherwise, if you do dnf install %{name}, systemctl start %{name}, this
> will crash on failed import).

I write it too quickly. It is now OK.

Comment 7 Zbigniew Jędrzejewski-Szmek 2017-02-13 16:07:43 UTC
So now there's a circular dependency, between the main package and the python3 subpackage. That's a bit unusual, but OTOH, it doesn't cause any real issues.

+ package name is OK
+ license is acceptable for Fedora (GPLv2+)
+ license is specified correctly
+ provides and requires are OK
+ scriptlets are OK
+ builds and installs OK

Package is APPROVED.

Comment 8 Fedora Update System 2017-02-21 23:07:54 UTC
prelude-correlator-3.1.0-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-79dc3a256d

Comment 9 Fedora Update System 2017-02-22 06:46:17 UTC
prelude-correlator-3.1.0-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-73969f8acd

Comment 10 Fedora Update System 2017-02-22 18:53:08 UTC
prelude-correlator-3.1.0-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-79dc3a256d

Comment 11 Fedora Update System 2017-02-22 21:09:23 UTC
prelude-correlator-3.1.0-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-73969f8acd

Comment 12 Fedora Update System 2017-02-22 22:55:48 UTC
prelude-correlator-3.1.0-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-fafb6a6624

Comment 13 Fedora Update System 2017-02-23 19:51:40 UTC
prelude-correlator-3.1.0-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-fafb6a6624

Comment 14 Tuomo Soini 2017-02-25 14:59:08 UTC
Epel7 testing package is broken.

package: python3-prelude-correlator-3.1.0-1.el7.noarch
  unresolved deps: 
     python3-prelude >= 0:3.1.0
     python3-netaddr

python3-prelude-correlator should be be python%{python3_pkgversion}-prelude-correlator.

And those dependencies should use %{python3_pkgversion} macro.

Comment 15 Fedora Update System 2017-03-03 03:49:55 UTC
prelude-correlator-3.1.0-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2017-03-03 03:54:37 UTC
prelude-correlator-3.1.0-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2017-03-10 11:20:06 UTC
prelude-correlator-3.1.0-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

Comment 18 Thomas Andrejak 2017-03-10 13:01:33 UTC
Indeed, really strange I don't see this. Thanks !


Note You need to log in before you can comment on or make changes to this bug.