Bug 753354

Hello, I'm going to informally review your package because I'm not a package maintainer. I also have submited some packages waiting for review and I'm looking for sponsor, so it would be great if you can review some of my packages (#738556 #756445 #756435 #756443)

$ rpmlint  strongswan-4.6.0-1.fc15.src.rpm strongswan.spec
strongswan.src: E: no-changelogname-tag
strongswan.src: W: invalid-license GPL
1 packages and 1 specfiles checked; 1 errors, 1 warnings.

- It fails to create the RPM binary package in x86_64, because the systemd unit dir is wrong in the spec.
- The license name is not valid. I think it should be GPLv2
- In Source0 you must build the url with %{name}-%{version}.tar.gz
- BuildRoot is not necessary if you don't plan to build to EPEL
- Since you ship a systemd unit file, you must add BuildRequires: systemd-units, take a look at http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd to see how to install the unit files.
- If not for EPEL, it's not necessary to clean the build root in %install and the %clean section
- Use %{name} instead of strongswan anywhere you can
- In %files you can drop the %defattr line
- Remove the white line in $files
- You must include in %doc all the COPYING, CREDITS, NEWS, README, etc.
- To reference the systemd unit dir, you must use: %{_unitdir}/%{name}.service
- Use %{_libdir} instead of /usr/%{_lib}
- Don't mark the man pages as %doc
- For the man pages use %{_mandir}/man3/foo.3.gz
- Substitute /usr/libexec for %{_libexecdir}
- Add something to the %changelog section

Hi, thanks for your review. Unfortunately I have lots of work now but I will come back to my two packages later.

If you have time, you can also look at Racoon2:

https://bugzilla.redhat.com/show_bug.cgi?id=752223

As for EPEL, these are important network-related packages and I definitely plan to try to get them to EPEL.

Thanks very much and expect a new version soon. I'll look at your packages as my time allows.

I've modified the strongswan.spec file to get rid of most of the rpmlint messages.

New files are here:

http://data.pavlix.net/fedora/racoon2.spec
http://data.pavlix.net/fedora/strongswan-4.6.0-2.fc16.src.rpm

Strongswan is now able to run in selinux permissive mode because it conflicts with Openswan.

Please test.

http://data.pavlix.net/fedora/strongswan.spec (racoon2 was a mistake here)

Related: bug 752223

Strongswan now build for EPEL6 and has a new version 4.6.1:

http://data.pavlix.net/fedora/strongswan.spec
http://data.pavlix.net/fedora/strongswan-4.6.1-1.el6.src.rpm

* Sun Jan 01 2012 Pavel Šimerda <pavlix - 4.6.1-1
- Bump to version 4.6.1

* Sun Jan 01 2012 Pavel Šimerda <pavlix - 4.6.0-3
- Added systemd scriptlets
- Added conditions to also support EPEL6

I'll check this package formally (and will find a sponsor if passed, as I'm not sponsor). Juan, thanks for your effort with informal review...

formal review is here, see the notes explaining OK* and BAD (BAD*) statuses below:

OK source files match upstream:
   sha256sum strongswan-4.6.1.tar.gz :
   d750ec16bc32c3d7f41fdbc7ac376defb1acde9f4d95d32052cdb15488ca3c34  strongswan-4.6.1.tar.gz

   One comment here - is there any reason for shipping .tar.gz sources? I see upstream provides .tar.bz2 as well, which is about 30% smaller.

  sha256sum strongswan-4.6.1.tar.bz2 :
  3d6dcdb3ce46dab51783b98c9bb54ebc931ff80941a0507d3cf3e3ac813eb439  strongswan-4.6.1.tar.bz2

OK package meets naming and versioning guidelines.
OK specfile is properly named, is cleanly written and uses macros consistently.
OK dist tag is present.
BAD license field matches the actual license.
  Still GPL - You should use GPLv2+
BAD license is open source-compatible (GPLv2+). License text included in
package.
   at least
   %doc COPYING README
   is missing (maybe even NEWS could be helpful)
OK latest version is being packaged.
   Only developer version for 4.6.2 available atm.
OK BuildRequires are proper.
OK compiler flags are appropriate.
OK package builds in mock (Rawhide/i386).
OK debuginfo package looks complete.
BAD* rpmlint is silent.
OK final provides and requires look sane.
N/A %check is present and all tests pass.
BAD shared libraries are added to the regular linker search paths with correct
scriptlets

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

is missing ... package adds some shared libraries ...

Idea - would it make sense to move them into -libs subpackage?

BAD owns the directories it creates.
  /usr/libexec/ipsec/ , /usr/lib/ipsec/ , /usr/lib/ipsec/plugins directories
  are added but not owned.

OK doesn't own any directories it shouldn't.
OK no duplicates in %files.
OK file permissions are appropriate.
OK correct scriptlets present.
OK code, not content.
OK documentation is small, so no -docs subpackage is necessary.
OK %docs are not necessary for the proper functioning of the package.
OK headers in devel subpackage
OK pkgconfig files in devel subpackage.
OK no libtool .la droppings.
OK not a GUI app.

rpmlint output:
strongswan.i686: W: invalid-license GPL

Use GPLv2+

strongswan.i686: E: non-readable /etc/strongswan.conf 0640L

Really strange permissions for the configure file... Is that intentional?
I would recommend changing to 644...

strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 27: warning: `EX' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 31: warning: `EE' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 141: warning: `TQ' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 1274: warning: `UR' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 1276: warning: `UE' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/ipsec.conf.5.gz 1370: warning: `EX' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/ipsec.conf.5.gz 1372: warning: `EE' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/ipsec.secrets.5.gz 135: warning: `TQ' not defined

Please check the manpages, it seems they are not completely gramatically correct.

strongswan.i686: W: no-manual-page-for-binary strongswan

I guess this is caused by incomplete renaming binary from ipsec to strongswan... please adjust the fix...

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

Small comments:
There are trailing spaces in the spec file in %post section

When building on RHEL it would emit also warning about empty sections of post/postun ... but this is something what could be solved later (you want this package in epel6, so it would probably be better if you do that soon)

Please fix the issues.

Created attachment 556578 [details]
New SRPM

This is a new release with the following changes:

All review comments incorporated except manual page errors that I'll be revisiting later. At least if I haven't missed something.

Ad permissions: This is what we get from upstream. The reasoning about non-readable configuration files or even directories is usually that someone
could use the configuration files to include e.g. encryption keys and forget to chmod the file.

For now I'm switching it to 644 but I would like to have some discussion before we get it in Fedora. The question is, whether to hide IPsec configuration from ordinary users so that (a) they don't know what's configured and (b) the admin doesn't leak authentication keys by mistake.

*If* we choose to protect the configuration, I would prefer the 'chmod -x /etc/strongswan/' way so we protect the whole directory.

Ad manpages: Could you please tell me how do I get these warnings with rpmbuild? Where are they put, or is it just its output? I don't see them.

I forgot to add that I made some more changes to the directory structure and strongswan is now not in conflict with openswan. This is mostly important for testing, they are not intended to be used together, but at least you don't have go through repated install/remove cycle when testing or comparing these two.

Created attachment 556587 [details]
New SPEC

Created attachment 556722 [details]
strongswan-4.6.1-4 SRPM

For now I'm implementing the safe way, i.e. to protect the whole configuration directory. This is the same I'm doing to Racoon2 but also the same that is done in Fedora's Openswan package.

This change introduces rpmlint's:

strongswan.i686: E: non-standard-dir-perm /etc/strongswan 0700L

Created attachment 556723 [details]
Updated SPEC

On a different note: why do not compile most of the plugins? I am also rolling a strongswan RPM for RHEL 6 and I could probably not use yours due to missing plugins.

Hi Stefan,

right now we're compiling the default plugins. If you have any suggestions, please drop them here.

Just to document... Debian uses the following configure options for Strongswan:

--prefix=/usr --sysconfdir=/etc --localstatedir=/var \
»·······»·······--libexecdir=/usr/lib \
»·······»·······--enable-ldap --enable-curl \
»·······»·······--with-capabilities=libcap \
»·······»·······--enable-smartcard --enable-pkcs11 \
»·······»·······--with-default-pkcs11=/usr/lib/opensc-pkcs11.so \
»·······»·······--enable-mediation --enable-medsrv --enable-medcli \
»·······»·······--enable-openssl --enable-agent \
»·······»·······--enable-ctr --enable-ccm --enable-gcm --enable-addrblock \
»·······»·······--enable-eap-radius --enable-eap-identity --enable-eap-md5 \
»·······»·······--enable-eap-gtc --enable-eap-aka --enable-eap-mschapv2 \
»·······»·······--enable-eap-tls --enable-eap-ttls --enable-eap-tnc \
»·······»·······--enable-sql --enable-integrity-test \
»·······»·······--enable-ha --enable-dhcp --enable-farp \
»·······»·······--enable-led \
»·······»·······--enable-test-vectors --enable-nat-transport

Sorry for delay...
Second round of formal review is here, see the notes explaining OK* and BAD (BAD*) statuses
below:

OK source files match upstream:
   sha256sum strongswan-4.6.1.tar.gz :
   d750ec16bc32c3d7f41fdbc7ac376defb1acde9f4d95d32052cdb15488ca3c34
strongswan-4.6.1.tar.gz

   One comment here - is there any reason for shipping .tar.gz sources? I see
upstream provides .tar.bz2 as well, which is about 30% smaller.

  sha256sum strongswan-4.6.1.tar.bz2 :
  3d6dcdb3ce46dab51783b98c9bb54ebc931ff80941a0507d3cf3e3ac813eb439
strongswan-4.6.1.tar.bz2

OK package meets naming and versioning guidelines.
OK specfile is properly named, is cleanly written and uses macros consistently.
OK dist tag is present.
OK license field matches the actual license.
  GPLv2+
OK license is open source-compatible (GPLv2+). License text included in
package.
OK latest version is being packaged.
   Only developer version for 4.6.2 available atm.
OK BuildRequires are proper.
OK compiler flags are appropriate.
OK package builds in mock (Rawhide/i386).
OK debuginfo package looks complete.
OK* rpmlint is silent.
OK final provides and requires look sane.
N/A %check is present and all tests pass.
OK shared libraries are added to the regular linker search paths with correct
scriptlets
OK owns the directories it creates.
OK doesn't own any directories it shouldn't.
OK no duplicates in %files.
OK file permissions are appropriate.
OK correct scriptlets present.
OK code, not content.
OK documentation is small, so no -docs subpackage is necessary.
OK %docs are not necessary for the proper functioning of the package.
OK headers in devel subpackage
OK pkgconfig files in devel subpackage.
OK no libtool .la droppings.
OK not a GUI app.

OK* rpmlint output:

[Reset@dhcp-24-196 rpmbuild]$ rpm -q rpmlint
rpmlint-1.4-6.fc18.noarch
[Reset@dhcp-24-196 rpmbuild]$ rpmlint SPECS/strongswan.spec RPMS/i686/strongswan-4.6.1-4.fc18.i686.rpm
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan_ipsec.secrets.5.gz 135: warning: `TQ' not defined
strongswan.i686: E: non-standard-dir-perm /etc/strongswan 0700L
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan_ipsec.conf.5.gz 1370: warning: `EX' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan_ipsec.conf.5.gz 1372: warning: `EE' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 27: warning: `EX' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 31: warning: `EE' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 141: warning: `TQ' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 1274: warning: `UR' not defined
strongswan.i686: W: manual-page-warning /usr/share/man/man5/strongswan.conf.5.gz 1276: warning: `UE' not defined
1 packages and 1 specfiles checked; 1 errors, 8 warnings.

This one error for 700 configuration directory is ok ... for me it seems to be better option to protect the whole directory. But if the upstream has different opinion and they want to have special permissions just on the config file, I'm fine with that.

8 warnings coming from manpages marked TODO in spec file - worksforme as well... I think these are not displayed to you because you are using different version of rpmlint...and they are in the upstream versions of manpages.

You could reproduce these warnings for e.g. strongswan.conf by : zcat strongswan.conf.5.gz | gtbl | groff -mtty-char -Tutf8 -P-c -mandoc -wmac >/dev/null

So the only one comment left - is the format of the source tarball - please consider switch to bzip2 format in next release - it will make srpm smaller :).

Anyway - other than that package looks sane now, APPROVED.

Thanks!

New Package SCM Request
=======================
Package Name: strongswan
Short Description: An implementation of IKEv1 and IKEv2 for IPsec
Owners: pavlix
Branches: f16 f17 el6
InitialCC:

Git done (by process-git-requests).

Strongswan is up an running and updated several times (two new upstream versions, fixed some packaging bugs). What now with this bugreport?

Just close it CURRENTRELEASE / RAWHIDE if you are done...