[Swan-dev] commit war over spec file

[Antony Antony]

Hi Tuomo and Paul,

I am sorry to create tension here. It is not worth loosing sleep over.
It seems I am the only user of libreswan-testing.spec for now.

I am happy to remove it from the repository and keep it local!
By any luck after reading rest of the e-mail, if you think lets keep it in 
the git repository please let me know! Otherwise I plan to remove it in a 
week or two.

--- Why I use libreswan-testing.spec and why remove now ---

It is designed, not there yet, to be close to "make base install-base". I 
know it is using "make programs"! As far I recollect there was some error 
that forced us, Tuomo and me, to settle for "make programs install";
It is not a replacement for a production spec file. And the vice versa.

"make install" defaults to /usr/local so does this spec file. It should 
install where "make install-base" installs. I can see this could be a rpm 
policy conflict and a good reason to keep the file local.

If the rpm install files to /usr/local I can switch between
"make install-base" and rpm install easily. Installed files overwrite each 
other. Otherwise I have to remember to run "rpm -e libreswsn". A  tiny work 
flow optimization. This optimization save lot of time and frustration over 
time.

I do not buy the argument rpm can't install to /usr/local if that is against 
the policy remove lets remove it from upstream.

The target "make kvm-rpm" creates the source tar ball and to be used with 
this spec file.  The source tar ball created by kvm-rpm include 
Makefile.inc.local.
Again this might race eyebrows, and against policy? Since it include 
Makefile.inc.local no need to add extra "USE_*"
It will take the defaults and the rest of USE_* will be set in 
Makefile.inc.local
Our testing use KVM_MAKEFLAGS in mk/kvm-targets.mk not spec file. I indent 
to use KVM_MAKEFLAGS in librewan-testing.spec too.

As far as I recollect Paul is against including the Makefile.inc.local in 
the tar ball for rpm? Editing Makefile.inc.local seems easier than editing 
spec file. Say to flip USE_FIPSCHECK=true to false for a test.

As far as I know production spec file include lot of redundant USE_* which 
is default in our config.mk, e.g. USE_DNSSEC=true and also confusing when I 
want to test with USE_KLIPS=true, production spec file has it false.  
testing.libreswan.org has USE_KLIPS=true for now. USE_KLIPS=true come from 
KVM_MAKEFLAGS, mk/kvm-targets.mk. 

In my experience "make rpm" is hard without Internet connection because of 
cavtest files. Even if I keep cavtest in rpmbuild/SOURCES/, I often rm -fr 
~/rpmbuild/ and building offline would fail.

Production rpm AFIK compile with -O2 which is annoying in development while 
debugging. -O2 cause  "No symbol "foo" in current context." inside gdb. This 
file avoid -O2 by choice. Another main reason I can't use  "make rpm"

IPSECVERSION=%{IPSECVERSION} is a minor annoyance. I would like to keep it 
different from production rpm now. May be fix it in the future. I would not 
go into this discussion for now. We have another thread dedicated to this 
topic. 3.30 is only place holder. Actual version is replaced in "make 
kvm-rpm". Simple diff between spec files may not tell the full use of "make 
kvm-rpm".

For testing it is useful to have other init systems. Some were added Tuomo 
and possibly not complete. This seems to conflict with policy, another 
reason to remove the file.

The use docker initsystem was introduced by Kim and later added to this spec 
file by Tuomo. The idea is to build without linking to systemd lib, also 
keep install clean without service file,.. other systemd files. As far I 
know it similar to use case of kubernets/podman/docker; vpn only container.  
initsystem check would be cleaner not depend on nsenter check.  However it 
need more work, we would have kick out systemd dependencies based on 
initsystem etc. Otherwise install in docker would drag in systemd ...

Overall I feel there are too many conflicts -- misunderstandings, 
differences in work flow preferences, and rpm polices around this file.
So for peace lets remove it from the repo, even though my preference would 
be to keep it and update it from time to time. 
If we keep in the repo it would get better over time, and may be one day 
when it same as production spec file we can remove. Also worth to note this 
file is in use for a long time,  started way before "make rpm" was fixed.

---
I try to catch up with production sepc file. Actually this is what triggered 
Tuomo's commits. And updating it slowly.
I think technically his commits went too far. The spec file is unusable for 
me when it does not install to /usr/local. It breaks my work flow. So I 
reverted the commits.

May be one day I would switch to production spec file, but not now.

Can we optionally instlal to "/usr/local"? may be using options to rpmbuild?  
Would it also violate polices?

I don't know how to build with_cavstests=0 from command line. Any pointers 
how to set global options rpmbuild? I feel if I know that I might re-think 
use of libreswan-testing.spec and possibly minimze the differences to 
production spec file.

regards,
-antony

On Thu, Feb 13, 2020 at 02:49:01PM -0500, Paul Wouters wrote:
> 
> I'm not very happy to see a commit/revert fight.
> 
> I was already not happy with the libreswan-testing.spec file. I believe
> it serves no purpose, and since it is not actively maintained, it will
> start to differentiate from the real fedora spec file. Previously, I
> left it alone to avoid a fight. But now that the fight is hitting git,
> let us re-evaluate this scenario.
> 
> Look at the diff between the "real" and the "testing" spec file:
> 
> 
> --- libreswan-testing.spec	2020-02-13 14:08:55.306432457 -0500
> +++ libreswan.spec	2020-02-12 15:25:00.614531238 -0500
> @@ -1,42 +1,41 @@
>  %global _hardened_build 1
>  # These are rpm macros and are 0 or 1
> -%global with_efence 1
> -%global with_development 1
> +%global with_efence 0
> +%global with_development 0
> +%global with_cavstests 1
>  %global nss_version 3.41
>  %global unbound_version 1.6.6
> -%global with_cavstests 0
> 
> These globals are meant to come in via the rpmbuild command as
> arguments. It should never be needed to manually set these. If
> that does not work with the curent scheme, we should redo this
> so it does work like that.
> 
> -%global _exec_prefix %{_prefix}/local
> 
> rpm installs should never live in /usr/local. They should live in
> the regular root or in /opt. The directory /usr/local is meant for
> local installs that are unmanaged by a package manager.
> 
> -%global initsystem @INITSYSTEM@
> 
> As fedora comes with only one init system, I'm not sure why this
> line is here.
> 
>  # Libreswan config options
>  %global libreswan_config \\\
> -    FINALDOCDIR=%{_pkgdocdir} \\\
> -    FINALEXAMPLECONFDIR=%{_pkgdocdir} \\\
>      FINALLIBEXECDIR=%{_libexecdir}/ipsec \\\
>      FINALMANDIR=%{_mandir} \\\
> -    FINALRUNDIR=%{_rundir}/pluto \\\
>      FIPSPRODUCTCHECK=%{_sysconfdir}/system-fips \\\
>      INC_RCDEFAULT=%{_initrddir} \\\
> -    INC_USRLOCAL=%{_exec_prefix} \\\
> -    IPSECVERSION=%{IPSECVERSION} \\\
> -    INITSYSTEM=%{initsystem} \\\
> -    USE_NSS_IPSEC_PROFILE=true \\\
> +    INC_USRLOCAL=%{_prefix} \\\
> +    INITSYSTEM=systemd \\\
>      PYTHON_BINARY=%{__python3} \\\
> -    SHELL_BINARY=%{_prefix}/bin/sh \\\
> 
> Here we see some new stuff that hasn't been ported to the test version
> yet. We also see IPSECVERSION=%{IPSECVERSION} which I believe is the
> sole reason for this file. As I explained in the past, an rpm version
> is NOT an upstream version. It also can lead to the version reported
> being different than the version from the source used. This override
> is dangerous and should not be used. I argued this before when I
> explained how I build "make rpm" targets. You set the version in the
> tar ball. then you let rpm make its own version based on the proper
> naming for pre-releases so that the upgrade path remains valid.
> 
> 
> 
> -%{nil}
> -
> +    SHELL_BINARY=%{_bindir}/sh \\\
> +    USE_DNSSEC=true \\\
> +    USE_FIPSCHECK=true \\\
> +    USE_KLIPS=false \\\
> +    USE_LABELED_IPSEC=true \\\
> +    USE_LDAP=true \\\
> +    USE_LIBCAP_NG=true \\\
> +    USE_LIBCURL=true \\\
> +    USE_LINUX_AUDIT=true \\\
> +    USE_NM=true \\\
> +    USE_NSS_IPSEC_PROFILE=true \\\
> +    USE_SECCOMP=true \\\
> +    USE_XAUTHPAM=true \\\
>  %{nil}
> 
> These are all missing in the testing version. I do not know why. I see
> no reason why we would build a testing rpm without these features. Also,
> the rpm global vars are ued to switch features on/off, like cavs testing
> or development or efence builds. Anything using the USE_* libreswan
> options is expected to never require changing when we build rpms. So
> omitting these in test versions seem a rather custom hack for one
> person, and not a valid generic way to build "testing" rpms. And again,
> "testing" rpms should be build using the regular fedora spec file,
> and changing the supported options for developing/testing, being:
> 
> 	%global with_efence 1
> 	%global with_development 1
> 	%global with_cavstests 1
> 
> 
>  #global prever rc1
> 
> -%global rel %{?prever:0.}1%{?prever:.%{prever}}
> -# for pluto --version
> -%global IPSECVERSION %{version}-%{rel}
> 
> Again, this is wrong as it is mixing up rpm versioning and upstream
> versioning.
> 
> 
>  Name: libreswan
> -Summary: IPsec implementation with IKEv1 and IKEv2 keying protocols
> -# version is replaced in make target
> -Version: 3.30
> -Release: %{rel}%{?dist}
> +Summary: Internet Key Exchange (IKEv1 and IKEv2) implementation for IPsec
> +# version is generated in the release script
> +Version: IPSECBASEVERSION
> +Release: %{?prever:0.}1%{?prever:.%{prever}}%{?dist}
> 
> It's pretty bad that 3.30 here is hardcoded when we haven't released
> that. It would create builds that mistakenly call themselves 3.30.
> 
>  License: GPLv2
>  Url: https://libreswan.org/
>  Source0: https://download.libreswan.org/%{?prever:development/}%{name}-%{version}%{?prever}.tar.gz
> @@ -45,40 +44,45 @@
>  Source2: https://download.libreswan.org/cavs/ikev1_psk.fax.bz2
>  Source3: https://download.libreswan.org/cavs/ikev2.fax.bz2
>  %endif
> -BuildRequires: gcc
> +
> +BuildRequires: audit-libs-devel
>  BuildRequires: bison
> +BuildRequires: curl-devel
> +BuildRequires: fipscheck-devel
>  BuildRequires: flex
> -BuildRequires: pkgconfig
> -BuildRequires: systemd-devel
> 
> These seems due to re-ordering.
> 
> 
> -Conflicts: openswan < %{version}-%{release}
> -Obsoletes: openswan < %{version}-%{release}
> -Provides: openswan = %{version}-%{release}
> -Provides: openswan-doc = %{version}-%{release}
> 
> Same here.
> 
> -BuildRequires: pkgconfig hostname
> -BuildRequires: nss-devel >= 3.16.1
> -BuildRequires: nspr-devel
> -BuildRequires: pam-devel
> -BuildRequires: libevent-devel
> -BuildRequires: unbound-devel >= 1.5.0-1
> +BuildRequires: gcc
> +BuildRequires: hostname
>  BuildRequires: ldns-devel
> +BuildRequires: libcap-ng-devel
> +BuildRequires: libevent-devel
>  BuildRequires: libseccomp-devel
>  BuildRequires: libselinux-devel
> -BuildRequires: fipscheck-devel
> -Requires: fipscheck%{_isa}
> -Buildrequires: audit-libs-devel
> -BuildRequires: libcap-ng-devel
> +BuildRequires: nspr-devel
> +BuildRequires: nss-devel >= %{nss_version}
>  BuildRequires: openldap-devel
> -BuildRequires: curl-devel
> +BuildRequires: pam-devel
> +BuildRequires: pkgconfig
> +BuildRequires: systemd-devel
> +BuildRequires: unbound-devel >= %{unbound_version}
> +BuildRequires: xmlto
>  %if 0%{with_efence}
>  BuildRequires: ElectricFence
>  %endif
> -BuildRequires: xmlto
> -
> -Requires: nss-tools
> -Requires: nss-softokn
> +Requires: fipscheck%{_isa}
>  Requires: iproute >= 2.6.8
> +Requires: nss >= %{nss_version}
> +Requires: nss-softokn
> +Requires: nss-tools
> +Requires: unbound-libs >= %{unbound_version}
> +Requires(post): bash
> +Requires(post): coreutils
> +Requires(post): systemd
> +Requires(preun): systemd
> +Requires(postun): systemd
> +Conflicts: openswan < %{version}-%{release}
> +Obsoletes: openswan < %{version}-%{release}
> +Provides: openswan = %{version}-%{release}
> +Provides: openswan-doc = %{version}-%{release}
> 
> Same here, but there is additional mismatch of nss version. The "real"
> spec uses >= 3.41 and the testing spec uses nss-devel >= 3.16.1 instead.
> 
>  %description
>  Libreswan is a free implementation of IPsec & IKE for Linux.  IPsec is
> @@ -90,15 +94,18 @@
>  tunnel is a virtual private network or VPN.
> 
>  This package contains the daemons and userland tools for setting up
> -Libreswan. To build KLIPS, see the kmod-libreswan.spec file.
> +Libreswan.
> 
> -Libreswan also supports IKEv2 (RFC4309) and Secure Labeling
> +Libreswan also supports IKEv2 (RFC7296) and Secure Labeling
> 
> 
> More scars from libreswan-testing.spec not updating to updates to the
> real libreswan.spec file.
> 
> 
>  Libreswan is based on Openswan-2.6.38 which in turn is based on FreeS/WAN-2.04
> 
>  %prep
>  %setup -q -n libreswan-%{version}%{?prever}
> +# enable crypto-policies support
>  sed -i "s:#[ ]*include \(.*\)\(/crypto-policies/back-ends/libreswan.config\)$:include \1\2:" programs/configs/ipsec.conf.in
> +# linking to freebl is no longer needed
> +sed -i "s/-lfreebl //" mk/config.mk
> 
> 
> Also fixes to the "real" spec file not represented in the "testing" spec file.
> 
>  %build
>  make %{?_smp_mflags} \
> @@ -110,6 +117,7 @@
>  %if 0%{with_efence}
>      USE_EFENCE=true \
>  %endif
> +    USERLINK="%{?__global_ldflags}" \
>      %{libreswan_config} \
>      programs
>  FS=$(pwd)
> 
> Missing an update again.
> 
> 
> @@ -128,10 +136,10 @@
>      %{libreswan_config} \
>      install
>  FS=$(pwd)
> -# Work around for FINALEXAMPLECONFDIR not working properly
> -rm -rf %{buildroot}%{_prefix}/share/doc
> +rm -rf %{buildroot}/usr/share/doc/libreswan
> 
> A hack needed in the testing spec file only.
> 
> +rm -rf %{buildroot}%{_libexecdir}/ipsec/*check
> 
> testing shipping some check binaries - might be useful or not. I am not
> convinced this was a conscious decision and not just a missed update to
> the real spec file.
> 
> 
> -install -d -m 0700 %{buildroot}%{_rundir}/pluto
> +install -d -m 0755 %{buildroot}%{_rundir}/pluto
> 
> This does not matter since the %files section has %attr() anyway.....
> 
>  # used when setting --perpeerlog without --perpeerlogbase
>  install -d -m 0700 %{buildroot}%{_localstatedir}/log/pluto/peer
>  install -d %{buildroot}%{_sbindir}
> @@ -151,8 +159,8 @@
>  # There is an elaborate upstream testing infrastructure which we do not
>  # run here - it takes hours and uses kvm
>  # We only run the CAVS tests.
> -# cp %{SOURCE1} %{SOURCE2} %{SOURCE3} .
> -# bunzip2 *.fax.bz2
> +cp %{SOURCE1} %{SOURCE2} %{SOURCE3} .
> +bunzip2 *.fax.bz2
> 
> Seems testing avoids CAVS testing - undocumented why that is.
> 
>  # work around for older xen based machines
>  export NSS_DISABLE_HW_GCM=1
> @@ -161,7 +169,7 @@
>  %{buildroot}%{_libexecdir}/ipsec/cavp -v2 ikev2.fax | \
>      diff -u ikev2.fax - > /dev/null
>  : starting CAVS test for IKEv1 RSASIG
> -%{buildroot}%{_libexecdir}/ipsec/cavp -v1sig ikev1_dsa.fax | \
> +%{buildroot}%{_libexecdir}/ipsec/cavp -v1dsa ikev1_dsa.fax | \
> 
> Looks like a bug - possibly an update not pulled in ?
> 
>      diff -u ikev1_dsa.fax - > /dev/null
>  : starting CAVS test for IKEv1 PSK
>  %{buildroot}%{_libexecdir}/ipsec/cavp -v1psk ikev1_psk.fax | \
> @@ -169,7 +177,6 @@
>  : CAVS tests passed
>  %endif
> 
> -%if "%{initsystem}" == "systemd"
>  %post
>  %systemd_post ipsec.service
> 
> @@ -178,7 +185,6 @@
> 
>  %postun
>  %systemd_postun_with_restart ipsec.service
> -%endif
> 
> weirdness done due to setting the INITSYSTEM differently from the real
> spec file ?
> 
> 
>  %files
>  %doc CHANGES COPYING CREDITS README* LICENSE
> @@ -191,22 +197,15 @@
>  %attr(0644,root,root) %config(noreplace) %{_sysconfdir}/sysctl.d/50-libreswan.conf
>  %attr(0700,root,root) %dir %{_localstatedir}/log/pluto
>  %attr(0700,root,root) %dir %{_localstatedir}/log/pluto/peer
> -%attr(0700,root,root) %dir %{_rundir}/pluto
> -%if "%{initsystem}" == "systemd"
> +%attr(0755,root,root) %dir %{_rundir}/pluto
> 
> Same as what I mentiobed before
> 
>  %attr(0644,root,root) %{_tmpfilesdir}/libreswan.conf
>  %attr(0644,root,root) %{_unitdir}/ipsec.service
> -%endif
> -%if "%{initsystem}" == "docker" || "%{initsystem}" == "sysvinit"
> -%attr(0755,root,root) %{_initddir}/ipsec
> -%config(noreplace) %{_sysconfdir}/sysconfig/pluto
> -%endif
> 
> So this seems a specific change. Apparently the rpm has to somehow work
> with docker too. I still have an open unanswered question about why the
> change I did for "ipsec" with detecting namespace testing is not working
> for Antony, but I never received a reply after asking twice.
> 
> 
> 
>  %attr(0644,root,root) %config(noreplace) %{_sysconfdir}/pam.d/pluto
>  %{_sbindir}/ipsec
>  %{_libexecdir}/ipsec
> -%{_mandir}/*/*
> +%doc %{_mandir}/*/*
> 
> Another missed update in the testing version.
> 
>  %{_libdir}/fipscheck/pluto.hmac
> 
>  %changelog
> -* Wed Aug  9 2017 Team Libreswan <team at libreswan.org> - @IPSECBASEVERSION@
> -- Automated build for testing from git tree.
> -- All compile time options are set in Makefile.inc.local not here.
> +* Sun Oct  7 2018 Team Libreswan <team at libreswan.org> - IPSECBASEVERSION-1
> +- Automated build from release tar ball
> 
> 
> 
> 
> If the only reason for this file is to do some docker based testing,
> it would be good to just see how to support this in the regular fedora
> spec file. That way, we can go back to one proper spec file that is
> maintained and won't age and use bad/old options.
> 
> As things stand now, I still feel this file does not belong in our
> repository.  I left it alone as an unimportant item and not worth putting
> more energy in, but I guess some action is now warranted to stop the
> commit war. I would appreciate an explanation of the docker use case
> so we can support Antony's docket setup as well supporting "make rpm"
> and using stock rpms with namespace based testing for fedora/rhel CI/TDD
> using 1 regular spec file.
> 
> Paul
> 
> Paul
> 
> Paul