Bug 566757

Summary: Review Request: strongswan - IKEv1 and IKEv2 based VPN suite
Product: [Fedora] Fedora Reporter: Johannes Russek <johannes.russek>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: c719711, fedora-package-review, gerd, i.grok, notting, shreyankg, sixy, smookher, taljurf
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-12-17 12:52:04 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Johannes Russek 2010-02-19 11:20:25 EST
Spec URL: http://developer.intra2net.com/git/?p=strongswan-rpm;a=blob_plain;f=strongswan.spec;hb=cb3f9a2c18ee8b2398737dcc6ce096221c87e782
SRPM URL: http://developer.intra2net.com/git/?p=strongswan-rpm;a=blob_plain;f=strongswan-4.3.6-0.src.rpm;hb=cb3f9a2c18ee8b2398737dcc6ce096221c87e782
Description: Taken from the website: 

The strongSwan 4.3 branch supports both the IKEv1 and IKEv2 key exchange protocols in conjunction with the native NETKEY IPsec stack of the Linux 2.6 kernel. The IKEv1 keying daemon pluto has been inherited from the strongSwan 2.8 branch whereas the IKEv2 keying daemon charon is based on a totally new object-oriented and multi-threaded concept, with 100% of the code being written in C. strongSwan's IKEv2 functionality has been successfully tested against 15 IKEv2 vendors during the third and fourth IKEv2 Interoperability Workshops in 2007 and 2008, respectively. 

Please check http://www.strongswan.org for a more detailed description of it's features. 

I've been maintaining a more or less satisfactory spec file for our own usage, but decided to try to maintain strongswan for fedora and epel with help of another strongswan user (Gerd v. Egidy) who agreed to be a co-maintainer of this package.

Most functionality is tested, except for the NetworkManager stuff since neither me nor Gerd have the possibilites to test this extensively.
We've split up most of the functionalities into different subpackages to avoid having to pull in a lot of dependencies if those features aren't used.

We are currently maintaing a git repository for our working on the spec at http://developer.intra2net.com/git/?p=strongswan-rpm

Thanks,

Johannes
Comment 1 Johannes Russek 2010-02-19 11:25:53 EST
*** Bug 566756 has been marked as a duplicate of this bug. ***
Comment 2 Bill Nottingham 2010-02-19 11:34:04 EST
Any reason why this can't obsolete or be merged with openswan?
Comment 3 Johannes Russek 2010-02-19 11:48:59 EST
well, they both use /etc/ipsec.d, /etc/ipsec.conf and /etc/ipsec.secrets, both provide /usr/sbin/ipsec and both provide a daemon named pluto that binds to the same port to name a few.

it would basically mean renaming almost everything to make it non-conflicting and tbh i don't much see the point of having both openswan and strongswan on the same machine.
Comment 4 Johannes Russek 2010-02-24 05:12:18 EST
i forgot to write in the original report that this would be my/our first package and we need a sponsor.
Comment 5 James Findley 2010-03-03 10:36:05 EST
Firstly, the rpmlint stuff:

> E: description-line-too-long 

There are actually a lot of lines that are too long.  Try to keep lines to >= 80 chars.

> E: explicit-lib-dependency libgcrypt
> E: explicit-lib-dependency libxml2
> E: explicit-lib-dependency NetworkManager-glib
You generally don't want to explicitly require a lib rpm, instead require the soname:
http://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires 

Also, in accordance with the explicit requires guidelines, where you require a specific version of something (e.g. sqlite-devel >= 3.3.1 ) there should be a comment explaining why.

> E: library-without-ldconfig-postin /usr/lib64/libfast.so.0.0.0
> E: library-without-ldconfig-postun /usr/lib64/libfast.so.0.0.0

ldconfig isn't called in %post for this

> E: non-readable /etc/ipsec.conf 0600
> E: non-readable /etc/strongswan.conf 0600
> E: non-standard-dir-perm /etc/ipsec.d/aacerts 0700
> E: non-standard-dir-perm /etc/ipsec.d/acerts 0700
> E: non-standard-dir-perm /etc/ipsec.d/cacerts 0700
> E: non-standard-dir-perm /etc/ipsec.d/certs 0700
> E: non-standard-dir-perm /etc/ipsec.d/crls 0700
> E: non-standard-dir-perm /etc/ipsec.d/ocspcerts 0700
> E: non-standard-dir-perm /etc/ipsec.d/private 0700
> E: non-standard-dir-perm /etc/ipsec.d/reqs 0700

Do these all need to be only readable by root, or can you use 644 for the .confs?

> E: zero-length /usr/share/doc/strongswan-4.3.6/AUTHORS

If it's blank, probably don't need to ship it.

> W: incoherent-init-script-name ipsec ('strongswan', 'strongswand')

initscript should be the same as the packagename.

> W: incoherent-version-in-changelog 4.3.6 ['4.3.6-0.fc12', '4.3.6-0']

version string in changelog should match the release.  So 4.3.6-0 instead of 4.3.6.

> W: invalid-license GPL

Should be GPLv2+

> W: macro-in-%changelog %build

Shouldn't use a macro in the changelog.

> W: name-repeated-in-summary C StrongSwan

No need to put the name in the description

> W: spurious-executable-perm /usr/src/debug/strongswan-4.3.6/src/medsrv/controller/peer_controller.c
> W: spurious-executable-perm /usr/src/debug/strongswan-4.3.6/src/medsrv/controller/peer_controller.h
> W: spurious-executable-perm /usr/src/debug/strongswan-4.3.6/src/medsrv/controller/user_controller.c
> W: spurious-executable-perm /usr/src/debug/strongswan-4.3.6/src/medsrv/controller/user_controller.h
> W: spurious-executable-perm /usr/src/debug/strongswan-4.3.6/src/medsrv/filter/auth_filter.c
> W: spurious-executable-perm /usr/src/debug/strongswan-4.3.6/src/medsrv/filter/auth_filter.h
> W: spurious-executable-perm /usr/src/debug/strongswan-4.3.6/src/openac/openac.c

There isn't really a need for these to be executable.  You could chmod these to get rid of the +x bit.

> W: strange-permission ipsec.init 0755

You don't need to have the source executable, instead do:
install -D -m 0755 %{SOURCE1} $RPM_BUILD_ROOT%{_initrddir}/%{name}

> W: summary-not-capitalized C strongSwan Internet Key Exchange (v1) daemon
> W: summary-not-capitalized C strongSwan Internet Key Exchange (v2) daemon
> W: summary-not-capitalized C strongSwan plugin for LDAP
> W: summary-not-capitalized C strongSwan plugin for MySQL
> W: summary-not-capitalized C strongSwan plugin for sqlite
> W: summary-not-capitalized C strongSwan utility and crypto library

These are just formatting issues in the summaries.

Also, why are you appending  || :
to your chkconfig lines?  It looks like an attempt to suppress non-zero exit codes, but if they fail, there is something wrong, so an error is appropriate.

make %{?_smp_mflags} install DESTDIR=$RPM_BUILD_ROOT
Is there a need to include %{?_smp_mflags} there?

Other than that, looks good.
Comment 6 Gerd v. Egidy 2010-03-07 19:20:45 EST
Hi James,

thanks for looking into our package.

> > E: non-readable /etc/ipsec.conf 0600
> > E: non-readable /etc/strongswan.conf 0600
> > E: non-standard-dir-perm /etc/ipsec.d/aacerts 0700
> > E: non-standard-dir-perm /etc/ipsec.d/acerts 0700
> > E: non-standard-dir-perm /etc/ipsec.d/cacerts 0700
> > E: non-standard-dir-perm /etc/ipsec.d/certs 0700
> > E: non-standard-dir-perm /etc/ipsec.d/crls 0700
> > E: non-standard-dir-perm /etc/ipsec.d/ocspcerts 0700
> > E: non-standard-dir-perm /etc/ipsec.d/private 0700
> > E: non-standard-dir-perm /etc/ipsec.d/reqs 0700
> 
> Do these all need to be only readable by root, or can you use 644 for the
> .confs?

There could be secret keys in these dirs. They should go in /etc/ipsec.d/private but I've already seen some scripts which create one file containing secret key and cert. So I think we should be a bit cautious with these dirs.

I changed the permissions to 0750 user:root group:ipsec now. So users or daemons who should get some control over strongswan could be put in that group. This is the recommended setup for the manager web-gui.
 
> > W: incoherent-init-script-name ipsec ('strongswan', 'strongswand')
> 
> initscript should be the same as the packagename.

hmm. I'd prefer to use "ipsec" because this was used in FreeS/WAN and is still used in openswan. So there are a lot of documentations out there using stuff like "/etc/init.d/ipsec restart". Most users of a *swan are expecting the initscript to be called ipsec.

Of course we could change it if it is really important.
 
> > W: strange-permission ipsec.init 0755
> 
> You don't need to have the source executable, instead do:
> install -D -m 0755 %{SOURCE1} $RPM_BUILD_ROOT%{_initrddir}/%{name}

Installing it like this is already done. The file doesn't have 0755 in my repo. Johannes, could you check if that is some glitch in your tree?
 
> > W: summary-not-capitalized C strongSwan Internet Key Exchange (v1) daemon
> > W: summary-not-capitalized C strongSwan Internet Key Exchange (v2) daemon
> > W: summary-not-capitalized C strongSwan plugin for LDAP
> > W: summary-not-capitalized C strongSwan plugin for MySQL
> > W: summary-not-capitalized C strongSwan plugin for sqlite
> > W: summary-not-capitalized C strongSwan utility and crypto library
> 
> These are just formatting issues in the summaries.

The official way of capitalizing it is "strongSwan", see www.strongswan.org.
Comment 7 Gerd v. Egidy 2010-03-14 17:31:16 EDT
Just to keep you updated:

I consider all issues mentioned here either fixed in our repository or explained in my last comment.

But we have found an issue when running strongSwan with SElinux enabled on F12 or newer. Johannes plans to look into this soon. 

We plan to post a new spec/srpm here as soon as this is fixed. If you are eager to look at the other stuff before, just look into our git (URL above).
Comment 8 Tareq Al Jurf 2010-04-28 15:07:47 EDT
*** Bug 586913 has been marked as a duplicate of this bug. ***
Comment 9 Bill Nottingham 2010-04-28 15:29:18 EDT
(In reply to comment #3)
> well, they both use /etc/ipsec.d, /etc/ipsec.conf and /etc/ipsec.secrets, both
> provide /usr/sbin/ipsec and both provide a daemon named pluto that binds to the
> same port to name a few.
> 
> it would basically mean renaming almost everything to make it non-conflicting
> and tbh i don't much see the point of having both openswan and strongswan on
> the same machine.    

Right, the point being is if there's 90-95% feature overlap, why couldn't the missing feature just be added into openswan, to save effort and confusion.
Comment 10 Jason Tibbitts 2010-11-17 18:22:37 EST
It doesn't appear that the new spec/srpm were ever posted.
Comment 11 Account closed by user 2011-10-02 10:59:21 EDT
ping!