Bug 441378

rpmlint says smokeping.src: E: unknown-key GPG#7666df64

Otherwise is ok on F8

> rpmlint says smokeping.src: E: unknown-key GPG#7666df64

Yeah, my build scripts is adding a internal only gpg key.


 

*** Bug 187326 has been marked as a duplicate of this bug. ***

FYI: #441359 blocking testing in rawhide is now fixed.

Would you consider updating to 2.4.1  and fixing up the URL and download URLs to
point to their current locations?

Updated to 2.4.2, urls fixed.

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://terjeros.fedorapeople.org/smokeping/smokeping-2.4.2-1.fc9.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=785262

I am not sure how did you manage to build in koji, but for me it failed because the new rpm is picky (needs %Patch0 even where before %Patch was enough : http://koji.fedoraproject.org/koji/getfile?taskID=785303&name=build.log )

Now I am going to be picky, too: README.Fedora includes the following sentence "Fix you want to access from a remote host [...]". I can swear this is not proper English, even if all the words are in English :)

I'll try to give it a spin one of these days and come back with a review, unless someone beats me to it.

Great!

Picky fixes:

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://terjeros.fedorapeople.org/smokeping/smokeping-2.4.2-2.fc9.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=785424

What's the reasoning behind using 
  %attr(2775, root, apache) %{_localstatedir}/lib/%{name}/images
and not
  %attr(755, apache, root) %{_localstatedir}/lib/%{name}/images
?

This will allow apache to write in the images directory (as required by the cgi) and avoid using any suid.


Look for a full review soon.

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: devel/x86_64
 [x] Rpmlint output:
source RPM:E: unknown-key GPG#7666df64
--> ignoreable
binary RPM:
smokeping.noarch: W: non-standard-gid /var/lib/smokeping/images apache
-> needed, the cgi provided by smokeping must be able to create apache readable images
smokeping.noarch: E: non-standard-dir-perm /var/lib/smokeping/images 02775
--> needed, in order to have the provided CGI create images, but avoidable by changing the owner/group settings
smokeping.noarch: E: non-readable /etc/smokeping/smokeping_secrets 0640
--> needed, the file contains passwords
 [x] Package is not relocatable.
 [x] Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [!] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
--> see issue 1
 [!] License field in the package spec file matches the actual license.
     License type: see issue 1
 [x] 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 is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     SHA1SUM of package: 055d65c7e3c49cd0d6e8f96242131fe69dc3110e  /tmp/smokeping-2.4.2.tar.gz
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [!] Permissions on files are set properly.
--> see issue 2
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: all archs suported by koji scratch build
 [x] Package should compile and build into binary rpms on all supported architectures.
     Tested on: all archs suported by koji scratch build
 [x] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.


=== Issues ===
1. Smokeping itself is GPLv2+ but it relies and includes several modules which either have no license specified (/usr/share/smokeping/lib/Smokeping/probes/FPing.pm, /usr/share/smokeping/lib/Smokeping/probes/EchoPingSmtp.pm and many many others) or claim to be under the artistic license (/usr/share/smokeping/lib/BER.pm, /usr/share/smokeping/lib/SNMP_Session.pm, /usr/share/smokeping/lib/SNMP_util.pm). Unfortunately, despite the fact the sources claim that the ""Artistic License" is included, the tar file does not really included it. AFAIK the original Artistic license is not acceptable in Fedora, so I must ask you to verify that all the included modules do have an acceptable license.
2. The permissions/ownership as proposed are functional and acceptable, but I see no reason to use 2775 for the folder where smokeping writes the images. I think that using apache.root as owner/group and 755 as modes will work just fine. Please correct me if I am wrong.

The original tarball includes a private copy/version of files from at least perl-CGI-Session, perl-SNMP_Session, perl-JSON. And it seems the rpm produced with this spec does too.

This should not be allowed without a really good reason.

Good catch, Yanko! I've re-analyzed the perl modules that are included and the following ones are private copies of CPAN modules and should no be included:
- CGI::Session
- Digest:HMAC,Digest:HMAC_MD5, Digest::HMAC_SHA1
- JSON + submodules (note that the latest version available in CPAN is 2.12, while smokeping includes 1.14; according to it's documentation, 2.12 includes incompatibilities towards the older version)
- qooxdoo is included in the binary rpm, but qooxdoo/README.txt from the source files claims it is only needed for development, not for runtime

SNMP_Session and SNMP_Util seem to be own implementations, not the CPAN versions. I could not find their author, Simon Leinen, in http://search.cpan.org/search?m=author&q=+Simon+Leinen

The qooxdoo/README.txt seems to be misleading. SmokeTrace contains the following lines:
use Qooxdoo::JSONRPC;
Qooxdoo::JSONRPC::handle_request ($cgi, $session);

So it seems that
  /usr/share/smokeping/lib/Qooxdoo
  /usr/share/smokeping/lib/Qooxdoo/JSONRPC.pm
  /usr/share/smokeping/lib/Qooxdoo/Services
  /usr/share/smokeping/lib/Qooxdoo/Services/Tr.pm
should be made available, but I think that is should exist as a separate package. Fortunately it is licensed as LGPL so it is compatible with the GPLv2+ of smokeping.

Could you please add to the README.fedora file a couple of lines indicating how to configure the system to allow for smoketrace usage?
a) the traceroute binary must be suid (chmod u+s ), otherwise the cgi will display an errror message ("The specified type of tracerouting is allowed for superuser only") and bail out.
b) selinux might also complain with errors similar to:
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "net_raw" to <Unknown> (httpd_t).      For complete SELinux messages. run sealert -l 5e473b24-c127-4c3f-903d-510ead101bec
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "connect" to <Unknown> (httpd_t).      For complete SELinux messages. run sealert -l e9b14398-4979-43c9-8ea3-9ca46bae5034
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "rawip_send" to <Unknown> (node_t).      For complete SELinux messages. run sealert -l 1185b418-256b-4340-983c-e5a071804fe8
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "node_bind" to <Unknown> (inaddr_any_node_t).      For complete SELinux messages. run sealert -l 2c034994-cf52-4993-aa97-c6d28bba3562
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "setopt" to <Unknown> (httpd_t).      For complete SELinux messages. run sealert -l 3ca926de-8227-4473-81c3-3d50dce61550
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "read" to <Unknown> (httpd_t).      For complete SELinux messages. run sealert -l 56c93fca-006d-40e6-88b6-72492f474042
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing setroubleshootd (httpd_t) "rawip_recv" to <Unknown> (node_t).      For complete SELinux messages. run sealert -l ab9af5f0-874d-4521-9d2b-3a314af3f8dd

However the selinux messages suddenly disappeared, only that I am not yet sure what I did to fix...

Ok, some issues here...

1) I will removed the CGI, JSON and Digest modules
2) I am tempted to drop smoketrace support, I don't see the big value over
   the very common traceroute command line tool. 
   I don't see this issue as blocker. Do you?
3) Will send a mail to Simon Leinen about the licenses on the SNMP* files

1) I am tempted to say "OK, but...". If those perl modules are not already available, they should be packaged and smokeping should be built against them. Perl is not my favorite subject but should you submit any of these as separate packages I'll try to help with the review.
2) I will not object, but it's a "nice to have".  The idea is that you get a traceroute issued via the web interface from a system to which the users might not have other forms of access. It's not so difficult to configure the system to allow it. Up to you to decide.
3) excellent

3) SNMP_Session, SNMP_Util and BER is already in Fedora under a Artistic 2.0 license. spot saw the problem some time ago and asked for the license change.

Upstream is here:
 http://www.switch.ch/misc/leinen/snmp/perl/

Thanks to Simon for providing this information.

I need some time to rearrange the package and do some testing.

This package needs a lot of work, specifically patching the perl code to be packageable in a sane way.

We did this for an in-house installation of smokeping in the past. If you're interested, I can see if we still have the spec file.

Smokeping need Config::Grammar, submitted for review.

New update fixing most issues:

o all external modules removed (however not Qooxdoo::JSONRPC, noted in README.fedora)

o README.fedora improved

o smoketrace support

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://koji.fedoraproject.org/koji/getfile?taskID=826553&name=smokeping-2.4.2-3.fc10.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=826553

Thanks so far guys, the package has improved a lot.

It looks better now, but I have found a few small issues. And I also have a question about which I am not sure.
a) perl has become an useless BR, because it is brought in by perl(Grammar);
b) glibc-common is definitely not needed as BR;
c) you use lots of macros in the {__xx} form, but "mv" is used as such. Nor a blocker neither incorrect, but not consistent from a stylish point of view.
d) in the lines
   %{__rm} -rf in lib/{CGI,Config,Digest,JSON}
   %{__rm} -rf in lib/{SNMP_Session,SNMP_util,BER,JSON}.pm

 I assume there is a confusion or a leftover from using find, as rm does not need or use the "in" argument. rm -fR in lib/*.. does not fail because of the -f flag, but you should remove the word "in" from these lines


Now the preamble of the question: as far as I can see, the newer rpm (2.4.2-3) removed the requires for
 perl(B)
 perl(CGI::Session::Driver)
 perl(CGI::Session::Driver::DBI)
 perl(CGI::Session::ErrorHandler)
 perl(Class::Struct)
 perl(DBD::Pg)
 perl(DB_File)
 perl(DBI)
 perl(Digest::HMAC)
 perl(Digest::SHA1)
 perl(File::Spec)
 perl(FreezeThaw)
 perl(overload)
 perl(Scalar::Util)
 perl(Test::More)

I do not have the infrastructure on which to test and my perl-fu is rather weak, so I have to rely to an answer from others: is all the previous functionality preserved ?

> a) perl has become an useless BR, because it is brought in by perl(Grammar);

perl(Grammar) is req, not br? 
perl is not used in building anyway, removed.

> b) glibc-common is definitely not needed as BR;

needed for iconv (or do you mean glibc-common is always available?)

> c) you use lots of macros in the {__xx} form, but "mv" is used as such. Nor a
> blocker neither incorrect, but not consistent from a stylish point of view.

fixed.

> d) in the lines
>    %{__rm} -rf in lib/{CGI,Config,Digest,JSON}
>    %{__rm} -rf in lib/{SNMP_Session,SNMP_util,BER,JSON}.pm

cut-n-paste error, fixed.

> Now the preamble of the question: as far as I can see, the newer rpm (2.4.2-3)
> removed the requires for
>  perl(B)
>  perl(CGI::Session::Driver)
>  perl(CGI::Session::Driver::DBI)
>  perl(CGI::Session::ErrorHandler)
>  perl(Class::Struct)
>  perl(DBD::Pg)
>  perl(DB_File)
>  perl(DBI)
>  perl(Digest::HMAC)
>  perl(Digest::SHA1)
>  perl(File::Spec)
>  perl(FreezeThaw)
>  perl(overload)
>  perl(Scalar::Util)
>  perl(Test::More)
> 
> I do not have the infrastructure on which to test and my perl-fu is rather
> weak, so I have to rely to an answer from others: is all the previous
> functionality preserved ?

With all the external modules removed this is not surprising, these requirements
are now pushed into the modules smokeping depends on (I hope :-).

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://koji.fedoraproject.org/koji/getfile?taskID=828581&name=smokeping-2.4.2-4.fc10.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=828580

ref glibc-common: it is safe to assume it is always present in the buildroot. how many systems or build logs have you seen which work without THE core set of libs ?

except for the Qooxdoo problem (it is still a private lib and the guidelines forbit that), everything else seems fine to me. I think it's time to call for external help, because I am not sure how to proceed. Strictly following the rules would impose that the older Qooxdoo should be made available as a package and become a "Require" of smokeping

I think it's so easy to comply with the packaging rules that we really should do it.

The only required thing to do is to package JSONRPC.pm from http://downloads.sourceforge.net/qooxdoo/qooxdoo-0.7.3-backend.zip (linked from http://qooxdoo.org/download_0_7 ). Just name it qooxdoo-backend-0.7 or qooxdoo-backend-compat or something similar ( in case someone packages version 0.8 which is not directly API compatible), make sure it provides JSONRPC.pm and I think that is all.

Opinions, anyone ?

Updated package:

- move Qooxdoo::JSONRPC to separate package, see #465710

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://terjeros.fedorapeople.org/smokeping/smokeping-2.4.2-5.fc9.src.rpm
koji: N/A, koji seems to be down.

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=864417

One more update to celebrate that ¤465710 is closed.

- fix README.fedora

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://koji.fedoraproject.org/koji/getfile?taskID=887265&name=smokeping-2.4.2-6.fc11.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=887263

srpm: http://terjeros.fedorapeople.org/smokeping/smokeping-2.4.2-6.fc9.src.rpm

I am sorry for the delay, Terje. I'll finish the review ASAP. Please bear with me, I have $WORK stuff to finish first

No problemo, I can wait.

Excellent, everything seems OK, package is APPROVED


However, for a smoother experience, please add to the README.fedora file that SELinux will prevent by default smokeping from working, unless a couple of commands are used to change the context. My preliminary tests in rawhide have shown that the following two commands are needed at least:
   chcon -v -R --type=httpd_sys_script_exec_t /usr/share/smokeping/cgi/
   chcon -v -R --type=httpd_sys_content_t /var/lib/smokeping/
A few other rules are needed, because I still get some errors. audit2allow says the following policy is needed:

module smokeudpsocket 1.0;

require {
        type httpd_sys_script_t;
        class udp_socket connect;
}

#============= httpd_sys_script_t ==============
allow httpd_sys_script_t self:udp_socket connect;


As my config is very small and uses very few of smokeping's facilities, I am almost sure other rules are also needed.

Thanks!

Will add SElinux info to README.



New Package CVS Request
=======================
Package Name: smokeping
Short Description: Latency Logging and Graphing System
Owners: terjeros
Branches: F-9
InitialCC:

For the record: I am using it for several months on F7 without any issues, so I guess (provided the deps are met), F-8 is also a branch which could make use of this package.

Ok, adding F-8.

New Package CVS Request
=======================
Package Name: smokeping
Short Description: Latency Logging and Graphing System
Owners: terjeros
Branches: F-8 F-9
InitialCC:

CVS Done

Imported and built.

Package Change Request
======================
Package Name: smokeping
New Branches: el5 el6
Owners: micklweiss sheltren

*** Bug 758443 has been marked as a duplicate of this bug. ***

Git done (by process-git-requests).