Bug 524386

Summary: Review Request: Intrace - Traceroute-like application for network reconnaisance
Product: [Fedora] Fedora Reporter: Kashyap Chamarthy <kchamart>
Component: Package ReviewAssignee: Tom "spot" Callaway <tcallawa>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting, tcallawa
Target Milestone: ---Flags: tcallawa: 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-01-21 05:00:10 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Kashyap Chamarthy 2009-09-19 17:57:22 UTC
Spec URL: http://kashyapc.fedorapeople.org/intrace.spec
SRPM URL: http://kashyapc.fedorapeople.org/intrace-1.4.2-1.fc12.src.rpm
Description: InTrace is a traceroute-like application that enables users to
enumerate IP hops exploiting existing TCP connections, both initiated
from local network (local system) or from remote hosts. It could be
useful for network reconnaissance and firewall bypassing

Comment 1 Kashyap Chamarthy 2009-09-19 18:09:55 UTC
RPM lint result:

---------------------------------------------------------------------------------
[build@f12-alpha SPECS]$ rpmlint intrace.spec 
intrace.spec: W: no-buildroot-tag
0 packages and 1 specfiles checked; 0 errors, 1 warnings.
---------------------------------------------------------------------------------
[build@f12-alpha SPECS]$ rpmlint ../SRPMS/intrace-1.4.2-1.fc12.src.rpm 
intrace.src: W: no-buildroot-tag
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
---------------------------------------------------------------------------------
[build@f12-alpha SPECS]$ rpmlint ../RPMS/x86_64/intrace-1.4.2-1.fc12.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
---------------------------------------------------------------------------------

Comment 2 Guido Grazioli 2009-09-20 11:40:45 UTC
Hello Kashyap,

just add
BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
to let rpmlint warning go away

You dont need to mkdir %{_datadir}/doc/%{name}-%{version} as %doc takes care of that.

The package is in good shape beyond said things and I'll complete the review after you applied changes.

Comment 3 Guido Grazioli 2009-09-20 12:13:36 UTC
I cannot find your packager group account, do you need a sponsor? In that case please add the FE-NEEDSPONSOR tag (and sorry i can still review your package but cannot sponsor you)

Comment 4 Kashyap Chamarthy 2009-09-20 12:29:45 UTC
Thanks Guido for reviewing, 

I made the changes as per comment #2

updated SPEC and SRPM:

http://kashyapc.fedorapeople.org/intrace.spec
http://kashyapc.fedorapeople.org/intrace-1.4.2-2.fc12.src.rpm

And yes, I need a sponsor. I'd really appreciate it, if somebody is willing to do so.

Comment 5 Guido Grazioli 2009-09-20 13:22:32 UTC
OK - rpmlint output
3 packages and 1 specfiles checked; 0 errors, 0 warnings.
OK - The package must be named according to the Package Naming Guidelines.
OK - The spec file name must match the base package %{name}
OK - The package must meet the Packaging Guidelines
OK - If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc
OK - The package must be licensed with a Fedora approved license and
meet the Licensing Guidelines (license is GPLv2)
NA - Every binary RPM package which stores shared library files must
call ldconfig in %post and %postun 
OK - The package MUST successfully compile and build
koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1692846 
OK - The spec file must be written in American English.
OK - The spec file for the package MUST be legible.
OK - The sources used to build the package must match the upstream source, as
provided in the spec URL. 
01fe19fb9709b30a7a154e528a74d1e3  intrace-1.4.2.tgz
NA - The spec file MUST handle locales properly (no translations)
NA - package not relocatable
OK - A package must own all directories that it creates
OK - A Fedora package must not list a file more than once in the spec file's
%files listings
OK - Permissions on files must be set properly
OK - Each package must have a %clean section
OK - Each package must consistently use macros
OK - The package must contain code, or permissable content (no content)
NA - Large documentation files must go in a -doc subpackage (no large doc)
OK - If a package includes something as %doc, it must not affect the runtime of
the application
NA - Header files must be in a -devel package (no devel package)
NA - Static libraries must be in a -static package (no static package)
NA - Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
OK - Packages must NOT contain any .la libtool archives
NA - Packages containing GUI applications MUST include a .desktop file 
OK - No file conflicts with other packages and no general names.
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 - The package does not yet exist in Fedora. The Review Request is not a
duplicate.
OK - %{?dist} tag is used in release

Package APPROVED, but blocked by FE_NEEDSPONSOR.

Comment 6 Jason Tibbitts 2009-09-23 06:06:12 UTC
Guido, you cannot approve this ticket; only sponsors can review packages where the submitter needs a sponsor.  See the big yellow box at http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored

Clearing the flags and returning the package to the queue.

Comment 7 Guido Grazioli 2009-09-23 08:59:23 UTC
Sorry for the overhead, i thougth the fe-needsponsor flag implied my review being informal

Comment 8 Tom "spot" Callaway 2009-09-29 14:22:46 UTC
The licensing on this package is at issue. None of the source files give any license attribution, nor do any of the documentation files.

The presence of "COPYING" is the only clue at licensing, so you should contact upstream and ask them to clarify what license they intend for this code to have.

On its own, all we can assume from COPYING's presence is "GPL+".

In addition, one file has something rather worrysome:

[sender.c]:

/* The procedure was found on the Internet - unknown license status!!! */
static inline uint16_t sender_cksum(uint16_t * addr, size_t cnt, uint16_t * pseudo, size_t pseudosz)
{

I'm pretty sure that procedure came from here:
http://aluigi.altervista.org/papers/gsmsdisc.zip

http://aluigi.org/about.htm#howuse describes Luigi's licensing, which is effectively "Copyright Only".

You should confirm this with upstream and have them give proper attribution to Luigi Auriemma, along with a link to his website describing the licensing terms.

Comment 9 Kashyap Chamarthy 2009-09-29 14:37:20 UTC
(In reply to comment #8)
> The licensing on this package is at issue. None of the source files give any
> license attribution, nor do any of the documentation files.
> 
> The presence of "COPYING" is the only clue at licensing, so you should contact
> upstream and ask them to clarify what license they intend for this code to
> have.

On this, I already contacted upstream and suggested to add appropriate license block in all source files - and a new tar ball(1.4.3) was released /with/ appropriate licenses. Will respin with an updated SPEC and SRPM 

> 
> On its own, all we can assume from COPYING's presence is "GPL+".
> 
> In addition, one file has something rather worrysome:
> 
> [sender.c]:
> 
> /* The procedure was found on the Internet - unknown license status!!! */
> static inline uint16_t sender_cksum(uint16_t * addr, size_t cnt, uint16_t *
> pseudo, size_t pseudosz)
> {
> 
> I'm pretty sure that procedure came from here:
> http://aluigi.altervista.org/papers/gsmsdisc.zip
> 
> http://aluigi.org/about.htm#howuse describes Luigi's licensing, which is
> effectively "Copyright Only".
> 
> You should confirm this with upstream and have them give proper attribution to
> Luigi Auriemma, along with a link to his website describing the licensing
> terms.  

sure, will confirm with upstream on this.

thanks a lot for comments Tom.

Comment 10 Kashyap Chamarthy 2009-09-30 06:22:19 UTC
The author has re-written the piece of code in question in  sender.c(with appropriate attribution)

re-written sender.c from upstream:
http://code.google.com/p/intrace/source/diff?spec=svn20&r=20&format=side&path=/trunk/sender.c



Rebuilt SPEC and SRPM with tar ball 1.4.3

http://kashyapc.fedorapeople.org/intrace.spec
http://kashyapc.fedorapeople.org/intrace-1.4.3-1.fc12.src.rpm

RPMlint was silent on SPEC and RPMS
-----------------------------------------------------------------------
[build@f12-alpha SPECS]$ rpmlint intrace.spec ../RPMS/x86_64/intrace-1.4.3-1.fc12.x86_64.rpm ../SRPMS/intrace-1.4.3-1.fc12.src.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
-----------------------------------------------------------------------

Comment 11 Tom "spot" Callaway 2009-10-26 17:41:08 UTC
One minor thing I noticed on doing this review: This package is not built with the Fedora optflags. The simplest way to accomplish this is to add this line at the end of %setup:

sed -i 's|-O3|%{optflags}|g' Makefile

Please make that change (or an equivalent change to use the Fedora optflags during compilation), show me the updated SRPM, and I will approve this package and sponsor you.

== Review (done against package with above change) ==

Good:

- rpmlint checks return nothing
- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2+) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream (SHA256: 9b9ed82f6f0f833a72a6acb3ff96a993d87ff59311cb1f219a76e92ec1771885)
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Comment 12 Kashyap Chamarthy 2009-10-26 19:03:52 UTC
Thanks a lot Tom for reviewing.

I made the suggested change  in the spec file for compiler optimization flags. And here are the SRPM & SPEC with the change.

SRPM: http://kashyapc.fedorapeople.org/intrace-1.4.3-2.fc12.src.rpm

SPEC: http://kashyapc.fedorapeople.org/intrace.spec

ran a quick rpmlint:
-------------------------------------------------------------
[build@f12-alpha SPECS]$ rpmlint intrace.spec ../RPMS/x86_64/intrace-1.4.3-2.fc12.x86_64.rpm ../SRPMS/intrace-1.4.3-2.fc12.src.rpm
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
--------------------------------------------------------------

Comment 13 Tom "spot" Callaway 2009-10-27 00:22:54 UTC
Looks good. Approved and sponsored. Please pick it up here: https://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_CVS_and_Set_Owner

Comment 14 Kashyap Chamarthy 2009-10-31 14:59:05 UTC
Thanks Tom.

New Package CVS Request
=======================
Package Name: intrace
Short Description: Traceroute-like application for network reconnaisance
Owners: kashyapc
Branches: F-11 F-12
InitialCC:

Comment 15 Kevin Fenzi 2009-10-31 23:46:37 UTC
cvs done.

Comment 16 Tom "spot" Callaway 2010-01-21 05:00:10 UTC
This one is in rawhide, closing out.