Bug 759855

Summary: Review Request: sslh - A SSL/SSH multiplexer
Product: [Fedora] Fedora Reporter: Christian Vanguers <christian.vanguers>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED DUPLICATE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: alpha, andreas, cwickert, goeran, kitgerrits, leamas.alec, limburgher, notting, package-review, pbrobinson, volker27
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard: NotReady
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-05-06 11:38:23 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description Christian Vanguers 2011-12-04 06:56:57 EST
Spec URL: http://wangee.opsyx.com/sslh.spec
SRPM URL: http://wangee.opsyx.com/sslh-1.10-1.fc16.src.rpm
Description: sslh lets one accept both HTTPS and SSH connections on the same port.
sslh makes it possible to connect to an SSH server on port 443 (e.g. from
inside a corporate firewall) while still serving HTTPS on that port.
Comment 1 Christian Vanguers 2011-12-04 07:01:33 EST
This is my first RPM package, and I'm looking for a sponsor.
Comment 2 Aleksandra Bookwar 2011-12-05 07:29:45 EST
I am not yet a packager myself, but I have some comments on the spec you've posted.

1) There is no need for _cc and _install macros in your spec files.

http://fedoraproject.org/wiki/Packaging:Guidelines#Macros says:
Macro forms of system executables SHOULD NOT be used except when there is a need to allow the location of those executables to be configurable. For example, rm should be used in preference to %{__rm}, but %{__python} is acceptable.

2) Fedora 15 and later use systemd. So you need to provide systemd unit instead of initscript.
http://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscripts_in_addition_to_systemd_unit_files

3) %clean section and %defattr no longer needed in the spec-files.

And about %build section - isn't it better to have the proper Makefile in the upstream sources and use make and make install instead of explicit build rules?
Comment 3 Christian Vanguers 2011-12-07 17:12:57 EST
Thanks for the comments Aleksandra.

I'm currently reviewing the spec file to take your remarks in account, and I'll come back with a new spec and srpm.

Regards,
Chris
Comment 4 Volker Fröhlich 2011-12-08 20:26:36 EST
* License is GPLv2+, not GPL

* Please put Name before Summary

* You can use the name macro in Source0

* If you're not out for EPEL 5, you can drop buildroot, clean section and the rm in the install section, see: http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag

* Include the README file using %doc

* Don't gzip the man page on your own, rpmbuild takes care of it.

* Drop defattr and correct permissions in the install section, if necessary

* I suppose, you'll need to follow the guidelines for systemd: http://fedoraproject.org/wiki/Packaging:Systemd

* %{_sysconfdir}/sysconfig/sslh -- Really?

* Use the optflags: http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

[makerpm@desktop SPECS]$ rpmlint /home/makerpm/rpmbuild/RPMS/x86_64/sslh-1.10-1.fc16.x86_64.rpm
sslh.x86_64: W: spelling-error Summary(en_US) ssl -> isl, sol, ssh
sslh.x86_64: W: summary-not-capitalized C ssl/ssh multiplexer
sslh.x86_64: W: invalid-license GPL
sslh.x86_64: W: conffile-without-noreplace-flag /etc/rc.d/init.d/sslh
sslh.x86_64: E: executable-marked-as-config-file /etc/rc.d/init.d/sslh
sslh.x86_64: W: service-default-enabled /etc/rc.d/init.d/sslh
sslh.x86_64: W: no-reload-entry /etc/rc.d/init.d/sslh
sslh.x86_64: E: subsys-not-used /etc/rc.d/init.d/sslh
1 packages and 0 specfiles checked; 2 errors, 6 warnings.

[makerpm@desktop SPECS]$ rpmlint /home/makerpm/rpmbuild/RPMS/x86_64/sslh-debuginfo-1.10-1.fc16.x86_64.rpm
sslh-debuginfo.x86_64: W: invalid-license GPL
sslh-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/sslh-1.10/common.h
sslh-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/sslh-1.10/common.c
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
Comment 5 Christoph Wickert 2011-12-17 08:40:29 EST
(In reply to comment #4)
> * If you're not out for EPEL 5, you can drop buildroot, clean section and the
> rm in the install section, see:
> http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag
>
> * Drop defattr and correct permissions in the install section, if necessary

I'd like to point out that there is no need to remove these or the %clean section. It's up to the packager. Personally I'd like to keep them to ensure backwards compatibility with older RPM versions or distributions.
Comment 6 Christoph Wickert 2011-12-17 08:44:55 EST
Cool you have packaged this, I was planning to do it but I didn't find the time.

Is there a reason you are not using the included Makefile. I'd rather patch it if necessary (e.g. to not compress the manpage) and use it.
Comment 7 Christian Vanguers 2011-12-17 17:18:24 EST
Hi!  I've reworked the package and added the support for systemd.
I've focused on the remarks from Aleksandra, Volker and Christoph, but I want to point out that I used again the install commands in the %install part of the spec because the Makefile needs some changes.  
In order to have this package available asap for the FC16 users, I did it this way.  

I'll focus on reworking the Makefile in collaboration with the developer of sslh.

SPEC URL: http://wangee.opsyx.com/sslh.spec
SRPM URL: http://wangee.opsyx.com/sslh-1.10-2.fc16.src.rpm

As Volker did for the first package, I ran rpmlint and debug rpm is giving a better report : 

[builder@hellboy i686]$ rpmlint sslh-debuginfo-1.10-2.fc16.i686.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.


Thanks for your support guys :)
Chris
Comment 8 Christian Vanguers 2011-12-17 17:22:25 EST
@(In reply to comment #6)
> Cool you have packaged this, I was planning to do it but I didn't find the
> time.
> 
> Is there a reason you are not using the included Makefile. I'd rather patch it
> if necessary (e.g. to not compress the manpage) and use it.

You're right, I should use the makefile, but as said above, it needs some changes.  I'll do it asap.

Regards,
Chris
Comment 9 Christoph Wickert 2011-12-17 18:00:21 EST
You are not allowed to change /etc/rsyslog.conf. No package must ever modify files that belong to other packages. Instead, add the configuration to /etc/rsyslog.d/sslh.conf (to be created by your package).

I think this package could be very useful in EPEL, too. If you want to maintain it for RHEL as well, consider adding support for both SysVinit and systemd through conditionals. For more info see http://fedoraproject.org/wiki/Packaging/DistTag#Conditionals
Comment 10 Christoph Wickert 2011-12-17 18:05:23 EST
When using install oc cp, please preserve timstamps, see https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

You are still compressing the manpage. Please don't, rpmbuild will do it automatically. And don't use an extension (gz) in the %files section. We might switch to bz or xz compression and then your spec will fail. Just %{_mandir}/man8/sslh.8.* or %{_mandir}/man8/* is fine. 

Please use the macro %{_unitdir} instead of hardcoding /lib/systemd/system.
Comment 11 Christian Vanguers 2011-12-21 18:27:11 EST
Hi Christoph,

Thanks again for your support.
I've made changes to the spec regarding EPEL and your remarks.

SPEC URL: http://wangee.opsyx.com/sslh.spec
SRPM URL: http://wangee.opsyx.com/sslh-1.10-3.fc16.src.rpm

About the man page, it's zipped by the Makefile.  what is your advise in that case ?


Regards,
Chris
Comment 12 Christoph Wickert 2011-12-21 18:37:53 EST
(In reply to comment #11)

> About the man page, it's zipped by the Makefile.  what is your advise in that
> case ?

Patch the Makefile to not compress it. I'd patch it anyway, e.g. to include optimization flags.
Comment 13 Alec Leamas 2012-02-23 05:02:48 EST
Just looking at this, I notice that you start and enable the service at installation time. As I understand it this is not the Fedora Way, although I'm just a newbie.
Comment 14 Jon Ciesla 2012-02-23 07:55:53 EST
Alec is correct, this may not be started or enabled by default.

https://fedoraproject.org/wiki/Starting_services_by_default
Comment 15 Jon Ciesla 2012-02-23 07:57:25 EST
Oh, and while I'm thinking of if, you could drop the version conditional when checking for fedora with regard to systemd, systemd has been the default since f15, which is the oldest currently supported version.
Comment 16 Christian Vanguers 2012-04-02 07:56:41 EDT
Hi!  I've reworked again the package.  Hoping that this time it will be ok.

SPEC URL: http://wangee.opsyx.com/sslh.spec
SRPM URL: http://wangee.opsyx.com/sslh-1.10-4.fc16.src.rpm

Regards,
Chris
Comment 17 Andreas Thienemann 2012-05-05 06:35:02 EDT
I noticed you are using the following statements to disable the service in rhel:

%if 0%{?rhel}
	if [ $1 = 0 ]; then
		/etc/init.d/sslh stop
		rm -f /etc/init.d/sslh
%endif

I would suggest continuing the "usual" practice of using chkconfig and service to do this:

postinst (which is missing) should be:

/sbin/chkconfig --add sslh

and preuninstall can be:

if [ $1 = 0 ]; then
	/sbin/service sslh stop > /dev/null 2>&1
	/sbin/chkconfig --del sslh
fi

This not only deletes the initscript but also all the symlinks pointing to it in the different runlevels.
Comment 18 Kit Gerrits 2012-08-08 05:23:49 EDT
(In reply to comment #17)
> and preuninstall can be:
> 
> if [ $1 = 0 ]; then
> 	/sbin/service sslh stop > /dev/null 2>&1
> 	/sbin/chkconfig --del sslh
> fi
> 
> This not only deletes the initscript but also all the symlinks pointing to
> it in the different runlevels.


I thought 'chkconfig --add' and 'chkconfig --del' only added and removed links in /etc/rc*..d/ ?
AFAIK, /etc/init.d/sslh still needs to be removed by hand / packages.

After placing the file in /etc/init.d, I useally do 'chkconfig --add <service>' and 'service <service> start'.
(currently, this points to systemd)
Comment 19 Peter Robinson 2012-08-08 06:04:06 EDT
> SPEC URL: http://wangee.opsyx.com/sslh.spec
> SRPM URL: http://wangee.opsyx.com/sslh-1.10-4.fc16.src.rpm

I can't access these.
Comment 20 Peter Robinson 2012-08-08 06:06:42 EDT
> I would suggest continuing the "usual" practice of using chkconfig and
> service to do this:

That's correct. For <= EL6 it needs to follow these guidelines
https://fedoraproject.org/wiki/Packaging:SysVInitScript

Please also ensure that for Fedora the package is using the latest guidlines for systemd.
https://fedoraproject.org/wiki/Packaging:Systemd
Comment 21 Volker Fröhlich 2012-09-30 14:23:48 EDT
Christian, your files are still unaccessible. Are you still interested?
Comment 22 Volker Fröhlich 2013-05-06 11:38:23 EDT
Closing, due to lack of response for more than a year
Comment 23 Christopher Meng 2014-03-26 10:01:45 EDT

*** This bug has been marked as a duplicate of bug 1081026 ***