This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 753354 - Review Request: strongswan - IKEv1 and IKEv2 daemon
Review Request: strongswan - IKEv1 and IKEv2 daemon
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Ondrej Vasik
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-11 18:33 EST by Pavel Šimerda (pavlix)
Modified: 2012-05-29 09:14 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-29 09:14:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
ovasik: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
New SRPM (4.59 MB, application/x-rpm)
2012-01-20 14:16 EST, Pavel Šimerda (pavlix)
no flags Details
New SPEC (5.95 KB, text/x-rpm-spec)
2012-01-20 14:31 EST, Pavel Šimerda (pavlix)
no flags Details
strongswan-4.6.1-4 SRPM (4.59 MB, application/x-rpm)
2012-01-21 17:13 EST, Pavel Šimerda (pavlix)
no flags Details
Updated SPEC (6.23 KB, text/x-rpm-spec)
2012-01-21 17:14 EST, Pavel Šimerda (pavlix)
no flags Details

  None (edit)
Description Pavel Šimerda (pavlix) 2011-11-11 18:33:17 EST
Spec URL: http://data.pavlix.net/fedora/strongswan.spec
SRPM URL: http://data.pavlix.net/fedora/strongswan-4.6.0-1.fc15.src.rpm
Description: The strongSwan 4.6 branch supports both the IKEv1 and IKEv2 key exchange
protocols in conjunction with the native NETKEY IPsec stack of the Linux
kernel.

Please test with permissive selinux for now. I appreciate any help making selinux happy with this package.

Strongswan is not Openswan. The main difference that prevents me from using Openswan is that Openswan doesn't work well with IPv6 (just try to use road warrior mode).

Therefore I believe there are use cases for Strongswan in Fedora. Strongswan and Racoon2 are the only two implementation that fully implement IPv6 and IKEv2 that I know.
Comment 1 Juan Orti 2011-11-29 07:24:36 EST
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
Comment 2 Pavel Šimerda (pavlix) 2011-11-29 09:37:22 EST
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.
Comment 3 Pavel Šimerda (pavlix) 2011-12-10 11:15:56 EST
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.
Comment 4 Pavel Šimerda (pavlix) 2011-12-10 11:16:42 EST
http://data.pavlix.net/fedora/strongswan.spec (racoon2 was a mistake here)
Comment 5 Pavel Šimerda (pavlix) 2011-12-10 11:26:55 EST
Related: bug 752223
Comment 6 Pavel Šimerda (pavlix) 2012-01-01 15:03:24 EST
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@pavlix.net - 4.6.1-1
- Bump to version 4.6.1

* Sun Jan 01 2012 Pavel Šimerda <pavlix@pavlix.net - 4.6.0-3
- Added systemd scriptlets
- Added conditions to also support EPEL6
Comment 7 Ondrej Vasik 2012-01-17 03:53:17 EST
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...
Comment 8 Ondrej Vasik 2012-01-20 05:58:10 EST
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.
Comment 9 Pavel Šimerda (pavlix) 2012-01-20 14:16:54 EST
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.
Comment 10 Pavel Šimerda (pavlix) 2012-01-20 14:19:30 EST
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.
Comment 11 Pavel Šimerda (pavlix) 2012-01-20 14:31:16 EST
Created attachment 556587 [details]
New SPEC
Comment 12 Pavel Šimerda (pavlix) 2012-01-21 17:13:28 EST
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
Comment 13 Pavel Šimerda (pavlix) 2012-01-21 17:14:03 EST
Created attachment 556723 [details]
Updated SPEC
Comment 14 stefan 2012-02-13 04:55:23 EST
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.
Comment 15 Pavel Šimerda (pavlix) 2012-02-13 06:58:39 EST
Hi Stefan,

right now we're compiling the default plugins. If you have any suggestions, please drop them here.
Comment 16 Pavel Šimerda (pavlix) 2012-02-13 08:02:25 EST
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
Comment 17 Ondrej Vasik 2012-02-13 12:46:18 EST
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.
Comment 18 Pavel Šimerda (pavlix) 2012-02-14 04:09:36 EST
Thanks!
Comment 19 Pavel Šimerda (pavlix) 2012-02-14 04:10:52 EST
New Package SCM Request
=======================
Package Name: strongswan
Short Description: An implementation of IKEv1 and IKEv2 for IPsec
Owners: pavlix
Branches: f16 f17 el6
InitialCC:
Comment 20 Jon Ciesla 2012-02-14 08:09:36 EST
Git done (by process-git-requests).
Comment 21 Pavel Šimerda (pavlix) 2012-05-27 10:22:24 EDT
Strongswan is up an running and updated several times (two new upstream versions, fixed some packaging bugs). What now with this bugreport?
Comment 22 Ondrej Vasik 2012-05-28 00:48:37 EDT
Just close it CURRENTRELEASE / RAWHIDE if you are done...

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