Bug 551857
| Summary: | Review Request: fwsnort - Translates Snort rules into equivalent iptables rules | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Guillermo Gómez <guillermo.gomez> |
| Component: | Package Review | Assignee: | Dennis Gilmore <dennis> |
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | low | ||
| Version: | rawhide | CC: | colin.coe, fedora-package-review, notting, nushio |
| Target Milestone: | --- | Flags: | dennis:
fedora-review+
kevin: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2010-08-02 20:56:42 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Guillermo Gómez
2010-01-02 15:45:23 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
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. 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 (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 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 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).
Sorry, my mistake, it should definitely 'GPLv2+' because of this in the README file: --- COPYRIGHT: Copyright (C) 2003 Michael Rash (mbr) 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 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) # # 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. 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 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. 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 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 ? 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 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}
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 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.
(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. (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. 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. one remaining issue
%fwsnortlogdir needs to be %{fwsnortlogdir}
it can be fixed at import time
APPROVED
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 GIT done (by process-git-requests). With f14 branch added. Being pushed to F12 and F13 branches. ====================== Package Name: fwsnort New Branches: el5 New Package SCM Request ====================== Package Name: fwsnort New Branches: el5 Package Change Request ====================== Package Name: pkgname New Branches: el5 el6 Owners: gomix Package Change Request ====================== Package Name: fwsnort New Branches: el5 el6 Owners: gomix Git done (by process-git-requests). |