Bug 551857 - Review Request: fwsnort - Translates Snort rules into equivalent iptables rules
Summary: Review Request: fwsnort - Translates Snort rules into equivalent iptables rules
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Dennis Gilmore
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-01-02 15:45 UTC by Guillermo Gómez
Modified: 2010-09-16 22:28 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-08-02 20:56:42 UTC
Type: ---
dennis: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Guillermo Gómez 2010-01-02 15:45:23 UTC
Spec URL: http://gomix.fedorapeople.org/fwsnort/fwsnort.spec
SRPM URL: http://gomix.fedorapeople.org/fwsnort/fwsnort-1.0.6-1.fc12.src.rpm

Description: 

fwsnort translates Snort rules into equivalent iptables rules and generates
a Bourne shell script that implements the resulting iptables commands. This
ruleset allows network traffic that exhibits Snort signatures to be logged
and/or dropped by iptables directly without putting any interface into
promiscuous mode or queuing packets from kernel to user space. In addition,
fwsnort (optionally) uses the IPTables::Parse module to parse the iptables
ruleset on the machine to determine which Snort rules are applicable to the
specific iptables policy.  After all, if iptables is blocking all inbound
http traffic from external addresses, it is probably not of much use to try
detecting inbound attacks against against tcp/80. By default fwsnort
generates iptables rules that log Snort sid's with --log-prefix to klogd
where the messages can be analyzed with a log watcher such as logwatch or
psad (see http://www.cipherdyne.org/psad). fwsnort relies on the iptables
string match module to match Snort content fields in the application portion
of ip traffic. Since Snort rules can contain hex data in content fields,
fwsnort implements a patch against iptables-1.2.7a which adds a
"--hex-string" option which will accept content fields such as
"|0d0a5b52504c5d3030320d0a|". fwsnort is able to translate approximately 60%
of all rules from the Snort-2.3.3 IDS into equivalent iptables rules. For
more information about the translation strategy as well as
advantages/disadvantages of the method used by fwsnort to obtain intrusion
detection data, see the README included with the fwsnort sources or browse
to: http://www.cipherdyne.org/fwsnort/

Also via git at fedorapeople.org

Comment 1 Colin Coe 2010-02-02 02:45:35 UTC
MUST
----
rpmlint output - MISSING

Package name - OKAY

Spec file matches base package - OKAY

# MUST: The package must meet the Packaging Guidelines .

License must be licensed with a Fedora approved license and meet the Licensing Guidelines - OK

License in spec must match actual license - BAD
Spec file states license is GPLv2 but fwsnort.8 states GPL (no version)

License file include in %doc - BAD
License file not included in RPM

Spec file written in American English - OK

Spec file legible - OK

Tar ball matches upstream - OK

Package successfully builds binary RPMs - OK (tested on RHEL5)

All build dependencies listed in BuildRequires - N/A

Spec file MUST handle locales properly - N/A

Every binary RPM package which stores shared library files must call ldconfig in %post and %postun - N/A

Packages must NOT bundle copies of system libraries - N/A

Pakcage relocatable - N/A

Package must own all directories that it creates - OK

No duplicate files - OK

Permissions on files must be set correctly - OK

Each package must have %clean which contains rm -rf %{buildroot} - OK

Macro use must be consistant - OK

Package must contain code or permissable content - OK

Large documentation files must go in a -doc subpackage - OK

Doc files must not affect runtime - N/A

Header files must be in devel package - N/A

Static Libaries must be in statioc package

Packahes containing pkgconfig files must 'Requirs: pkgconfig' - N/A

If package contains library files with a suffix then library files that end in .so (without suffix) must go in a -devel package - N/A

Devel package to be versioned against base - N/A

Packages must NOT contain any .la libtool archives - N/A

Packages containing GUI applications must include a %{name}.desktop file - N/A

Packages must not own files or directories already owned by other packages - OK

At the beginning of %install, each package MUST run rm -rf %{buildroot} - OK

All filenames in rpm packages must be valid UTF-8 - OK



SHOULD
------
If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it - N/A

The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available - N/A

The reviewer should test that the package builds in mock - OK (tested against RHEL5)

The package should compile and build into binary rpms on all supported architectures - OK

The reviewer should test that the package functions as described - not running snort: not done.

If scriptlets are used, those scriptlets must be sane - N/A

Subpackages other than devel should require the base package using a fully versioned dependency - N/A

The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg - N/A

If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself - N/A

Your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense - OK


RESULTS
-------
Please provide rpmlint output

Please review the license

Please include LICENSE in the %doc section.  Please also consider moving /etc/fwsnort/snort_rules/VERSION to %doc section

Please consider including the following files in the %doc section:
VERSION
README
CREDITS
TODO

Comment 2 Guillermo Gómez 2010-02-03 13:52:05 UTC
Updated spec and srpm, including all the suggestions. I'm waiting for the autor about the issue syncing man and spec files.

/etc/fwsnort/snort_rules/VERSION to %doc section >> included as SNORT-RULES-VERSION to avoid filename clash with fwsnort VERSION file.

rpmlint output of updated spec and srpm

$ rpmlint -i fwsnort-1.0.6-1.fc12.src.rpm fwsnort.spec 
1 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 3 Colin Coe 2010-02-03 23:18:38 UTC
Hi

When you update the spec file, please increment '%release'.  Related to this, the %changelog section should list all the changes in this release.

Please increment the release and update changelog.

CC

Comment 4 Guillermo Gómez 2010-02-04 01:48:03 UTC
(In reply to comment #3)
> Hi
> 
> When you update the spec file, please increment '%release'.  Related to this,
> the %changelog section should list all the changes in this release.
> 
> Please increment the release and update changelog.
> 
> CC    

Done :)

Spec url: http://gomix.fedorapeople.org/fwsnort/fwsnort.spec
SRPM url: http://gomix.fedorapeople.org/fwsnort/fwsnort-1.0.6-2.fc12.src.rpm

Gomix

Comment 5 Colin Coe 2010-02-04 02:08:26 UTC
At the risk of being pedantic, I think the license is wrong in the spec file.

http://fedoraproject.org/wiki/Licensing
---
GNU General Public License (no version)	 
GPL+ 	 
Yes 	 
Yes 	
Yes 	
A GPL or LGPL licensed package that lacks any statement of what version that it's licensed under in the source code/program output/accompanying docs is technically licensed under *any* version of the GPL or LGPL, not just the version in whatever COPYING file they include. 
---

I think the license should be 'GPL+' not 'GPLv2' is the license file lacks a reference to a version.

v1.1 of fwsnort has been released, please consider rebasing to this version.

Other than these couple of items, your RPM and spec file look pretty good.  You look like you've already been sponsored but I'd prefer a second opinion on this RPM and spec to make sure I've not missed anything.

CC

Comment 6 Guillermo Gómez 2010-02-04 02:34:57 UTC
Im not an expert about licenses, but, LICENSE file included begins as follows:

                    GNU GENERAL PUBLIC LICENSE
                       Version 2, June 1991

thats why i did choose GPLv2 short name from the wiki about licencing for the spec (i did not notice the man page file, thanks). The man page mentions only GPL which is the only issue remaining (waiting for the author, i dont want to modify the man page).

About the new release (1.1), i will consider some time later afer this ones goes through the revision process, as an update (if it hits reasonably soon to fedora repos). Also i need to test and review v1.1 as a whole (ipv6 stuff).

Comment 7 Colin Coe 2010-02-04 02:58:36 UTC
Sorry, my mistake, it should definitely 'GPLv2+' because of this in the README file:
---
COPYRIGHT:

Copyright (C) 2003 Michael Rash (mbr@cipherdyne.org)

This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2 of the License, or
(at your option) any later version.
---

OK, so once the license statement is fixed in the spec file I think this will be ready.

A second opinion for a more experienced reviewer would be good though.

CC

Comment 8 manuel wolfshant 2010-02-04 08:56:36 UTC
Actually the mandatory way of deciding the license is the source. In this particular case, the "fwsnort" perl script includes the following paragraph:
Copyright (C) 2003-2007 Michael Rash (mbr@cipherdyne.org)
#
# License (GNU Public License):
#
#    This program is distributed in the hope that it will be useful,
#    but WITHOUT ANY WARRANTY; without even the implied warranty of
#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
#    GNU General Public License for more details.
#
#    You should have received a copy of the GNU General Public License
#    along with this program; if not, write to the Free Software
#    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
#    USA


This one alone would mean GPL+.
However, since the tarball 
- does include the GPLv2 standard file, 
- references it
- specifies the same license in the README file, we can safely assume that GPLv2+ is the correct choice for the spec tag.

Comment 9 Guillermo Gómez 2010-02-04 18:49:22 UTC
License adjusted as recommended

rpmlint output

$ rpmlint -i fwsnort.spec fwsnort-1.0.6-3.fc12.noarch.rpm 
1 packages and 1 specfiles checked; 0 errors, 0 warnings.

spec url: http://gomix.fedorapeople.org/fwsnort/fwsnort.spec
srpm url: http://gomix.fedorapeople.org/fwsnort/fwsnort-1.0.6-3.fc12.src.rpm

Comment 10 manuel wolfshant 2010-02-04 19:00:53 UTC
Not a blocker, but in my opinion the description field is way too long. If you want to include all that marketing stuff, do it in a README or ABOUT file.

Suggested %Description:
fwsnort translates Snort rules into equivalent iptables rules and generates
a Bourne shell script that implements the resulting iptables commands.

In addition, fwsnort (optionally) uses the IPTables::Parse module to parse the iptables ruleset on the machine to determine which Snort rules are applicable to the specific iptables policy.

fwsnort is able to translate approximately 60% of all rules from the Snort-2.3.3 IDS into equivalent iptables rules.

Comment 11 Guillermo Gómez 2010-02-04 19:20:36 UTC
Description in spec in fact was taken from the README, so no problem on accepting the suggestion.

Description adjusted en Release 4

rpmlint output

$ rpmlint -i fwsnort.spec fwsnort-1.0.6-4.fc12.src.rpm 
1 packages and 1 specfiles checked; 0 errors, 0 warnings.

spec url: http://gomix.fedorapeople.org/fwsnort/fwsnort.spec
srpm url: http://gomix.fedorapeople.org/fwsnort/fwsnort-1.0.6-4.fc12.src.rpm

Comment 12 manuel wolfshant 2010-02-04 19:31:08 UTC
Sorry if the answer is obvious, but at the first (and second) glance I do not see what's fwsnortmoddir used for. It does not seem to be used either in %install or in %files

Could you please shed some light ?

Comment 13 Guillermo Gómez 2010-02-04 20:14:19 UTC
No problem, this macro its actually not needed anymore because modules required build are not necesary nor provided by fwsnort, those modules are provided by perl-Net-IPv4Addr & perl-IPTables-Parse in Fedora. So i removed the macro definition :) Thanks for the catch.

Macro definition fwsnortmoddir removed en Release 5

rpmlint output

$ rpmlint -i fwsnort.spec fwsnort-1.0.6-5.fc12.src.rpm 
1 packages and 1 specfiles checked; 0 errors, 0 warnings.

spec url: http://gomix.fedorapeople.org/fwsnort/fwsnort.spec
srpm url: http://gomix.fedorapeople.org/fwsnort/fwsnort-1.0.6-5.fc12.src.rpm

Comment 14 Dennis Gilmore 2010-04-22 03:01:39 UTC
 rpmlint /var/lib/mock/fedora-13-x86_64/result
fwsnort.noarch: W: spelling-error Summary(en_US) iptables -> potables, portables, timetables
fwsnort.noarch: W: spelling-error %description -l en_US iptables -> potables, portables, timetables
fwsnort.noarch: W: spelling-error %description -l en_US ruleset -> rule set, rule-set, ruler
fwsnort.src: W: spelling-error Summary(en_US) iptables -> potables, portables, timetables
fwsnort.src: W: spelling-error %description -l en_US iptables -> potables, portables, timetables
fwsnort.src: W: spelling-error %description -l en_US ruleset -> rule set, rule-set, ruler
I think that these warnings can be ignored.

upstream source matches
sha256sum fwsnort-1.0.6.tar.gz fedora/SOURCES/fwsnort-1.0.6.tar.gz 
b7ce09b815d1dd8ec9518a39ece79883d8099c905be431dc7b6b1d73a523b40f  fwsnort-1.0.6.tar.gz
b7ce09b815d1dd8ec9518a39ece79883d8099c905be431dc7b6b1d73a523b40f  fedora/SOURCES/fwsnort-1.0.6.tar.gz

I would like to see all macros be used consistently  some you have wrapped in {} and some not.  please wrap them all in {} i.e. %_sysconfdir should be %{_sysconfdir}

Comment 15 Guillermo Gómez 2010-04-25 11:58:17 UTC
Done... macros use improved for consistency on release 6

rpmlint output

$ rpmlint -i fwsnort.spec fwsnort-1.0.6-6.fc12.src.rpm 
fwsnort.src: W: spelling-error Summary(en_US) iptables -> potables, portables, timetables
The value of this tag appears to be misspelled. Please double-check.

fwsnort.src: W: spelling-error %description -l en_US iptables -> potables, portables, timetables
The value of this tag appears to be misspelled. Please double-check.

fwsnort.src: W: spelling-error %description -l en_US ruleset -> rule set, rule-set, ruler
The value of this tag appears to be misspelled. Please double-check.

1 packages and 1 specfiles checked; 0 errors, 3 warnings.

spec url: http://gomix.fedorapeople.org/fwsnort/fwsnort.spec
srpm url: http://gomix.fedorapeople.org/fwsnort/fwsnort-1.0.6-6.fc12.src.rpm

Comment 16 Dennis Gilmore 2010-05-16 23:54:46 UTC
the rpmlint warning can be ignored.

you shouldn't own %{_sysconfdir}/logrotate.d instead you should Requires logrotate which provides that directory.  you need it also to have logrotate functional.


Can you please point me to some reviews you have helped with.

Comment 17 Guillermo Gómez 2010-05-17 00:47:51 UTC
(In reply to comment #16)
> the rpmlint warning can be ignored.
> 
> you shouldn't own %{_sysconfdir}/logrotate.d instead you should Requires
> logrotate which provides that directory.  you need it also to have logrotate
> functional.

I'll take care of this asap.

> 
> 
> Can you please point me to some reviews you have helped with.    

Honestly, i did not know i had such privilege in bugzilla, ¿do i? i guess i do, i did not find such recommendation anywhere else :(

However, i've helping with many rpm building process to other new potential contributors, ive been helping others in their way (under rpmdev project scope):

http://rpmdev.proyectofedora.org/projects/dhcp-probe
http://rpmdev.proyectofedora.org/projects/show/clamsmtp (in bugzilla already)
http://rpmdev.proyectofedora.org/projects/abajo 
http://rpmdev.proyectofedora.org/projects/show/smartcam
http://turpial.org.ve/ (in bugzilla, helping richzendy with the pkg, i compiled tested in x86_64 env and produced a better spec/srpm for him, included if pf.org repo)

From most of those pkgs, including my own fwsnort,i already have setup a intermediate yum repo to proove u ive been helping those people to devel their specs/rpms: http://www.proyectofedora.org/repo/12/ (the idea been those pkgs will reach oficial repos and bugzilla, not to be an steady repo, just pre-fedora oficial procedures)

# https://bugzilla.redhat.com/show_bug.cgi?id=555059
# https://bugzilla.redhat.com/show_bug.cgi?id=551857
# https://bugzilla.redhat.com/show_bug.cgi?id=549496
# https://bugzilla.redhat.com/show_bug.cgi?id=549366
# https://bugzilla.redhat.com/show_bug.cgi?id=567713

Ive been helping them, but i did not know i could review them in bugzila.

Comment 18 Guillermo Gómez 2010-05-17 01:25:52 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > the rpmlint warning can be ignored.
> > 
> > you shouldn't own %{_sysconfdir}/logrotate.d instead you should Requires
> > logrotate which provides that directory.  you need it also to have logrotate
> > functional.
> 

Corrected wrong ownsership claim and added Requires to logrotate instead in release 7 of the pkg.

srpm url: http://gomix.fedorapeople.org/fwsnort/fwsnort-1.0.6-7.fc12.src.rpm
spec url: http://gomix.fedorapeople.org/fwsnort/fwsnort.spec

$ rpmlint -i fwsnort.spec fwsnort-1.0.6-7.fc12.src.rpm 
fwsnort.src: W: spelling-error Summary(en_US) iptables -> potables, portables, timetables
The value of this tag appears to be misspelled. Please double-check.

fwsnort.src: W: spelling-error %description -l en_US iptables -> potables, portables, timetables
The value of this tag appears to be misspelled. Please double-check.

fwsnort.src: W: spelling-error %description -l en_US ruleset -> rule set, rule-set, ruler
The value of this tag appears to be misspelled. Please double-check.

1 packages and 1 specfiles checked; 0 errors, 3 warnings.

Comment 19 Guillermo Gómez 2010-07-22 07:08:49 UTC
Updated spec/src.rpm for FC13 (no change)

srpm: http://gomix.fedorapeople.org/fwsnort/1.0.6-7/fc13/fwsnort-1.0.6-7.fc13.src.rpm

spec: http://gomix.fedorapeople.org/fwsnort/1.0.6-7/fc13/fwsnort.spec

$ rpmlint -i fwsnort.spec fwsnort-1.0.6-7.fc13.src.rpm 
fwsnort.src: W: spelling-error Summary(en_US) iptables -> potables, portables, birdtables
The value of this tag appears to be misspelled. Please double-check.

fwsnort.src: W: spelling-error %description -l en_US iptables -> potables, portables, birdtables
The value of this tag appears to be misspelled. Please double-check.

fwsnort.src: W: spelling-error %description -l en_US ruleset -> rule set, rule-set, rules et
The value of this tag appears to be misspelled. Please double-check.

1 packages and 1 specfiles checked; 0 errors, 3 warnings.

Comment 20 Dennis Gilmore 2010-07-31 00:07:32 UTC
one remaining issue

%fwsnortlogdir needs to be %{fwsnortlogdir}

it can be fixed at import time

APPROVED

Comment 21 Guillermo Gómez 2010-07-31 00:38:39 UTC
New Package CVS Request
=======================
Package Name: fwsnort
Short Description: Translates Snort rules into equivalent iptables rules
Owners: gomix
Branches: F-12 F-13
InitialCC: ausil

Comment 22 Kevin Fenzi 2010-07-31 00:54:29 UTC
GIT done (by process-git-requests).

With f14 branch added.

Comment 23 Guillermo Gómez 2010-08-02 20:56:42 UTC
Being pushed to F12 and F13 branches.

Comment 24 Guillermo Gómez 2010-09-16 16:04:27 UTC
======================
Package Name: fwsnort
New Branches: el5

Comment 25 Guillermo Gómez 2010-09-16 20:09:18 UTC
New Package SCM Request
======================
 Package Name: fwsnort
 New Branches: el5

Comment 26 Guillermo Gómez 2010-09-16 22:12:34 UTC
Package Change Request
======================
Package Name: pkgname
New Branches: el5 el6
Owners: gomix

Comment 27 Guillermo Gómez 2010-09-16 22:24:11 UTC
Package Change Request
======================
Package Name: fwsnort
New Branches: el5 el6
Owners: gomix

Comment 28 Kevin Fenzi 2010-09-16 22:28:43 UTC
Git done (by process-git-requests).


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