Bug 538327

Summary: Review Request: otpd - One Time Password daemon
Product: [Fedora] Fedora Reporter: Giuseppe Paterno <gpaterno>
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: medium    
Version: rawhideCC: brenner+bugzilla, dcolaiac, fedora-package-review, ghisha, gvarisco, herrold, notting, opensource, tomspur, wolfy
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: 2012-06-15 16:06:36 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449    

Description Giuseppe Paterno 2009-11-18 04:43:52 EST
Spec URL: http://www.gpaterno.com/external/otpd.spec
SRPM URL: http://www.gpaterno.com/external/otpd-3.2.0-1.src.rpm
Description: 

Hi! I've created a fork of the original OTP daemon from TRI-D Systems, acquired by Red Hat. The project is at http://otpd.googlecode.com. It is a one time password daemon capable of authenticating HOTP-compliant devices, such as softtokens and hardtokens.

I'd really appreciate if you can review this to be included in fedora extra and (later) in EPEL.
Comment 1 Giuseppe Paterno 2009-11-18 04:50:12 EST
Sorry, I mentioned extra, but I meant fedora :)
Comment 2 Thomas Spura 2009-11-18 17:21:52 EST
Just a few comments for now:

- License GPL is wrong: GPLv2+ is the right one
- The builds fails:
gcc  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -D_REENTRANT -DUSE_SOCKET -DHAVE_CLOCK_GETTIME -DVERSION=\"3.2.0\" -Wp,-U_FORTIFY_SOURCE -U_FORTIFY_SOURCE -Wshadow -Wsign-compare -Werror -c -o failover_thread.o failover_thread.c
cc1: warnings being treated as errors
failover_thread.c: In function 'failover_thread':
failover_thread.c:109: error: dereferencing pointer 'uicp' does break strict-aliasing rules
failover_thread.c:109: note: initialized from here
failover_thread.c:110: error: dereferencing pointer 'uicp' does break strict-aliasing rules
failover_thread.c:110: note: initialized from here
failover_thread.c:162: error: dereferencing pointer 'uicp' does break strict-aliasing rules
failover_thread.c:162: note: initialized from here
failover_thread.c:267: error: dereferencing pointer '({anonymous})' does break strict-aliasing rules
failover_thread.c:267: note: initialized from here
make: *** [failover_thread.o] Error 1

- run make %{?_smp_mflags} and not just make, see https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

- use make install DESTDIR=$RPM_BUILD_ROOT and not %makeinstall, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

- in %clean, please just run rm -rf $RPM_BUILD_ROOT

- Source needs to contains a url, where you downloaded it, e.g.
  http://otpd.googlecode.com/files/%{name}-%{version}.tar.gz

- I'm unsure, if the Packager and Vendor tag is in fedora only not used or not allowed. From https://fedoraproject.org/wiki/Packaging/Guidelines#Rpmlint_Errors it's just not used...


- The description is misleading (at least for me):
  "otpd is part of a suite of software"... Which suite?
Comment 3 R P Herrold 2009-11-18 18:11:36 EST
An inauspicious start as it does not build under CentOS 5, with minimal stuff brought down from RawHide as SRPMs and recompiled to solution

gcc  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -D_REENTRANT -DUSE_SOCKET -DHAVE_CLOCK_GETTIME -DVERSION=\"3.2.0\" -Wp,-U_FORTIFY_SOURCE -U_FORTIFY_SOURCE -Wshadow -Wsign-compare -Werror -c -o resynctool.o resynctool.c
cc1: warnings being treated as errors
resynctool.c: In function 'main':
resynctool.c:219: warning: format '%llu' expects type 'long long unsigned int', but argument 2 has type 'uint64_t'
resynctool.c:223: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type 'uint64_t'
resynctool.c:240: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type 'uint64_t'
make: *** [resynctool.o] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.43739 (%build)

I have to scratch my head at that warning enhancement, but ...

-- Russ herrold
Comment 4 Giuseppe Paterno 2009-11-19 11:32:10 EST
I should have fixed the above:

SRPMS: http://www.gpaterno.com/external/otpd-3.2.1-1.src.rpm
SPEC: http://www.gpaterno.com/external/otpd.spec

I have no way to check if resynctool.c is fixed today under rawhide, however I casted it to long long unsigned int and compile fine under F12.
Comment 5 Giuseppe Paterno 2009-11-20 06:32:34 EST
New SRPMS that fixes manpages to reflect the fork and included missing man page of otppasswd (from: Russ herrold)

Confirmed that this package builds on rawhide (credit of: Russ herrold)

SRPMS: http://www.gpaterno.com/external/otpd-3.2.2-1.src.rpm
SPEC: http://www.gpaterno.com/external/otpd.spec
Comment 6 R P Herrold 2009-11-20 07:56:19 EST
this was what a private email to the maintainer referecned in #5

1) I see a lot of marketing cruft mentioning TRI-D in the files in the
README* --- would it be possible to re-write these to remove the 'marketing'
aspect?

2) I suspect that the 'changelog' entries convention of mentioning the V and
R was not known to you:

compare:
* Mon Feb 11 2008 Frank Cusack <frank@tri-dsystems.com>  3.1.0-1
        to
* Thu Nov 19 2009 Giuseppe Paterno' <gpaterno@redhat.com>

3) the man page URL field is probably stale

4) the man page mentions two additional man pages not present:

[herrold@dhcp-103 otpd]$ man gsmd
No manual entry for gsmd

[herrold@dhcp-103 otpd]$ man otppasswd
No manual entry for otppasswd

5) a commented sample /etc/otppasswd is REALLY needed, to show what is
possible, and marked as a (config) file

6) [herrold@dhcp-103 otpd]$ rpm -ql otpd | grep reqs
The 'reqsynctool' binary -- probably living in either /usr/bin/ or
/usr/sbin/ is missing?

=====================

I an in a TUI only environment presently -- I will file this later today in
the bugzilla, or you might do it for me?
Comment 7 R P Herrold 2009-11-20 08:04:45 EST
sadly, the patch worked an a completely stock C5, i386, but fails a bit later on a souped up x86_64 build box:


gcc  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -D_REENTRANT -DUSE_SOCKET -DHAVE_CLOCK_GETTIME -DVERSION=\"3.2.1\" -Wall -Wp,-U_FORTIFY_SOURCE -U_FORTIFY_SOURCE -Wshadow -Wsign-compare -Werror -c -o failover_thread.o failover_thread.c
cc1: warnings being treated as errors
failover_thread.c: In function 'failover_thread':
failover_thread.c:112: warning: cast to pointer from integer of different size
failover_thread.c:113: warning: cast to pointer from integer of different size
make: *** [failover_thread.o] Error 1
make: *** Waiting for unfinished jobs....
error: Bad exit status from /var/tmp/rpm-tmp.28908 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.28908 (%build)
[herrold@centos-5 otpd]$ gcc -v
Using built-in specs.
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-libgcj-multifile --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --enable-plugin --with-java-home=/usr/lib/jvm/java-1.4.2-gcj-1.4.2.0/jre --with-cpu=generic --host=x86_64-redhat-linux
Thread model: posix
gcc version 4.1.2 20080704 (Red Hat 4.1.2-46)
[herrold@centos-5 otpd]$
Comment 8 Giuseppe Paterno 2009-11-20 10:20:18 EST
Let's try again :)))

SRPM: http://www.gpaterno.com/external/otpd-3.2.3-1.src.rpm
SPEC: http://www.gpaterno.com/external/otpd.spec  

I tried only i386, x86_64
Comment 9 R P Herrold 2009-11-20 11:31:10 EST
compiled on x86_64 -- thank you

Comment 6, item 5 
The /etc/ file mentioned /etc/otppasswd does not exist in the binary RPM

Newly noticed: looks like there is a spelling or man page section error at the bottom of:

   ...
/usr/share/man/man8/otppasswd.5.gz
/usr/share/man/man8/resynctool.8.gz
/var/run/otpd
[herrold@centos-5 ~]$ man otpd
[herrold@centos-5 ~]$ man otppasswd
No manual entry for otppasswd
[herrold@centos-5 ~]$    

but otppasswd(5) and NOT otpasswd or otppasswd(8) appears there ... I suspect you have it mis-placed in examining the file /usr/share/man/man8/otppasswd.5.gz and that sed -e '@man8@man5@' is intended

[herrold@centos-5 otpd]$ sudo rpm -Uvh   /home/herrold/rpmbuild/RPMS/x86_64/otpd-3.2.3-1.x86_64.rpm
Password:
Preparing...                ########################################### [100%]
   1:otpd                   ########################################### [100%]
[herrold@centos-5 otpd]$ rpm -qlp /home/herrold/rpmbuild/RPMS/x86_64/otpd-3.2.3-1.x86_64.rpm | grep etc
/etc/otpd.conf
/etc/rc.d/init.d/otpd
/etc/sysconfig/otpd
[herrold@centos-5 otpd]$       

thank you 

-- Russ herrold
Comment 10 Giuseppe Paterno 2009-11-21 04:19:30 EST
I will fix the manpage.
However I'm torn to include or not to include an empty /etc/otppasswd file.
The file is similar to /etc/passwd and contains the otp secrets in the format:

user:otp-type:secret

And it's not meant to be commented, in a similar way of /etc/passwd.
What should I do??
Create an empty file?
Insert two demo users??? (might be a security concern tough of default conf!)

Any suggestion is appreciated.
Thanks.
Comment 11 Antoine Brenner 2009-11-21 09:25:53 EST
Hello,

Suggestion concerning #10:

As a potential user of otpd, I think that an empty /etc/otppasswd file would be OK provided that an example otppasswd file is made available in the docs since a real-life looking example is always nice to have.

Of course the example file should have comments explaining that comments are not supported and that the secrets in the example file are NOT to be used: a command like "dd if=/dev/random bs=4096 count=1 |sha1sum" should be used instead.

By the way, do you know what TRI-D Systems became ? Their home page said something like "We've been bought, we'll be back soon" and nothing came, and nowadays the domain www.tri-dsystems.com does not resolve any more...
I've read somewhere that Redhat was the buyer, and that they were investigating the "IP"... 

Best regards,
Antoine
Comment 12 Alexander Boström 2009-11-21 19:15:28 EST
(In reply to comment #11)

> not supported and that the secrets in the example file are NOT to be used: a
> command like "dd if=/dev/random bs=4096 count=1 |sha1sum" should be used
> instead.

Try "mkpasswd".
Comment 13 Giuseppe Paterno 2009-11-23 03:18:40 EST
I cannot comment on #11 on TRI-D as I'm a Red Hat employee.

This code came off the TRI-D public web site *before* it has been acquired. Please consider this a fork from the original project and the long-term goal is to get rid of the hooks to the proprietary code and introduce new algorithms and database support.

However, in this relase I created an empty /etc/otppasswd, examples are provided in the man page otppasswd.5 (which has been fixed however for #9).

New pakages
SRPM: http://www.gpaterno.com/external/otpd-3.2.4-1.src.rpm
SPEC: http://www.gpaterno.com/external/otpd.spec
Comment 14 Giuseppe Paterno 2009-11-23 03:19:20 EST
Sorry ... I forgot to thank you!!! :)
Comment 15 R P Herrold 2009-11-23 10:05:26 EST
clean compile, but in a upgrade situation, I get the following error.  I assume a conditional to test state is needed in the relevant script?

[herrold@centos-5 otpd]$ sudo rpm -Uvh  /home/herrold/rpmbuild/RPMS/x86_64/otpd-3.2.4-1.x86_64.rpm
Preparing...                ########################################### [100%]
   1:otpd                   ########################################### [100%]
Stopping /usr/sbin/otpd: [FAILED]
Starting /usr/sbin/otpd: /usr/sbin/otpd: /etc/otpd.conf: statedir '/etc/otpstate': No such file or directory
[FAILED]
error: %postun(otpd-3.2.3-1.x86_64) scriptlet failed, exit status 1
[herrold@centos-5 otpd]$                      

or, perhaps the state directory is now in /var/run ?

-- Russ herrold
Comment 16 Giuseppe Paterno 2009-11-23 10:43:32 EST
Mmmh, I should create an empty state dir, forgot :(((
Getting old ... ;)

SRPM: http://www.gpaterno.com/external/otpd-3.2.4-2.src.rpm 
SPEC: http://www.gpaterno.com/external/otpd.spec
Comment 17 Giuseppe Paterno 2009-11-25 04:14:34 EST
No feedback??
Should I assume everything is fine???
Comment 18 Antoine Brenner 2009-11-29 05:17:38 EST
Hello Giuseppe,

I am trying to build otpd on my centos5 workstation, but the src rpm does not work for me (I guess that the rpm format has changed between fedora12 and rhel5 because even trying to download multiple times did not help):

error: unpacking of archive failed on file rpmbuild/otpd/otpd-3.2.4.tar.gz;4b1247dc: cpio: MD5 sum mismatch

Here is my md5sum for the srpm otpd-3.2.4-2.src.rpm
a8cf26978f45a69c3dbd6c65a353218f  otpd-3.2.4-2.src.rpm


Anyway, I thought I could try to build from the spec file directly. However, the released otpd-3.2.4.tar.gz is not available on google code:

wget http://otpd.googlecode.com/files/otpd-3.2.4.tar.gz 
  Resolving otpd.googlecode.com... 74.125.79.82
  Connecting to otpd.googlecode.com|74.125.79.82|:80... connected.
  HTTP request sent, awaiting response... 404 Not Found
  11:04:21 ERROR 404: Not Found.


Can you please add otpd-3.2.4.tar.gz to the downloadable files on googlecode ?

Best regards,
Antoine
Comment 19 Giandomenico De Tullio 2009-11-29 12:45:19 EST
Hi Anto'
 
From Centos5 you can get-around for install .src.rpms builded on f12 (rpm + XZa payload).

Try install mc, use mc to /enter/ into rpm, /enter/ in the CONTENT.cpio, select and copy the files -but .spec- in SOURCES and spec file in SPECS :)
Comment 20 Antoine Brenner 2009-11-29 13:05:00 EST
Hello,

The mc trick worked like a charm, and the rpm was built without any errors.

Thanks,
Antoine
Comment 21 Antoine Brenner 2009-11-29 19:37:25 EST
Hello,

I am not sure if I should further comment on this bug or open a new one since what I am reporting is not 'packaging' related but more about the software itself. I could also report on the bugtracker of http://otpd.googlecode.com if needed. Please let me know which method is preferred. (I think googlecode might be the best place)

As for my current issue, it is related to resynctool. The man page states that the output of resynctool (using a > redirection) is suitable as /etc/otpstate/username directly.

However on my system, resynctool does not produce a one-liner suitable as state, but two lines. Example below:
$ /usr/sbin/resynctool -1 990878 -2 457035 -u testuser -k 3cd0f53cd4d94b08d249b1f861e655d774fcf0e5
0: 990878
5:testuser:0000000000000002:::0:0:0:
$

I do not know where the first line ("0: 990878\n") comes from, but it needs to be removed to fix the "5 (service error)" I get when testing with otpauth.

IMO, this is a bug in resynctool.

Regards,
Antoine


PS:
A small command line HOTP client in the documentation folder would be a nice addition (for tests).
Comment 22 Giuseppe Paterno 2009-11-30 04:29:49 EST
Sorry for the missing tar.gz, I'm always travelling and it's difficult to cope with everythihg :)

I strongly suggest to try to get out of SVN the tree if the tar.gz is not in googlecode.

the resynctool stuff is not a bug, it's an output of the sequence of OTPs, therefore only the last line has to be taken into consideration to be put in the status file.

However, since it's more practical to "script" resynctool, I might decide to put a "verbose" flag into resynctool and get rid of that output if not requested.
Comment 23 Antoine Brenner 2009-11-30 06:02:55 EST
Hello,

If the resynctool output is not a bug, then the bug is in the manpage of resynctool, that states that the output CAN be used to generate the status file.

I agree with you that a verbose flag in resynctool would be a nicer way of fixing the problem.

Regards,
Antoine

PS:
Concerning the bugtracker choice, should I continue to report all my issues here ?
Comment 24 R P Herrold 2009-11-30 09:48:46 EST
The question was asked about how to unpack a SRPM made by a later RPM version -- the '--nomd5' sub option solved that, by omitting the checksum test.

I see no problems with the packaging at this point.  As I am not eligible to 'sign off' on a packaging, I will ask a friend to do the formal review.

Was it possible to get the 'resynctool' added here, or in a side packaging?

-- Russ herrold

[herrold@centos-5 ~]$ cd build
[herrold@centos-5 build]$ cd otpd/
[herrold@centos-5 otpd]$ wget http://www.gpaterno.com/external/otpd-3.2.4-2.src.rpm
--2009-11-30 09:43:59--  http://www.gpaterno.com/external/otpd-3.2.4-2.src.rpm
Resolving www.gpaterno.com... 78.46.44.108
Connecting to www.gpaterno.com|78.46.44.108|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 1198392 (1.1M) [application/x-rpm]
Saving to: `otpd-3.2.4-2.src.rpm'

100%[======================================>] 1,198,392    280K/s   in 5.4s

2009-11-30 09:44:06 (218 KB/s) - `otpd-3.2.4-2.src.rpm' saved [1198392/1198392]

[herrold@centos-5 otpd]$ rpm -Uvh --nomd5 otpd-3.2.4-2.src.rpm
   1:otpd                   ########################################### [100%]
[herrold@centos-5 otpd]$ rpmbuild -ba ~/rpmbuild/SPECS/otpd.spec
Comment 25 Giuseppe Paterno 2009-11-30 10:16:22 EST
I put a more confortable "-d" (debug) in resynctool in the new version.

SPEC: http://www.gpaterno.com/external/otpd.spec
SRPM: http://www.gpaterno.com/external/otpd-3.2.5-2.src.rpm

Sources: http://otpd.googlecode.com/files/otpd-3.2.5.tar.gz

Thank you all for your cooperation!!!

@Antoine: if are related to this package, please do.
If they are related to features/general issues, please consider also on 
http://code.google.com/p/otpd/issues/list

Thanks!
Comment 26 R P Herrold 2009-11-30 10:58:09 EST
I see that even though I had disabled the otpd, it was started by a upgrade -- should there be a '[condrestart]' rather than an a formal [stop and start always] here?

[herrold@centos-5 otpd]$ sudo rpm -Uvh /home/herrold/rpmbuild/RPMS/x86_64/otpd-3.2.5-2.x86_64.rpm
Password:
Preparing...                ########################################### [100%]
   1:otpd                   ########################################### [100%]
Stopping /usr/sbin/otpd: [FAILED]
Starting /usr/sbin/otpd: [  OK  ]
[herrold@centos-5 otpd]$

An admin might want the man pages or the resynctool, but not the daemon, I think

This is sort of discussed with implementations, at:
   https://bugzilla.redhat.com/show_bug.cgi?id=484345

-- Russ herrold
Comment 27 Giuseppe Paterno 2009-11-30 11:13:24 EST
I'm working to update the sources and the spec for a condrestart.

I don't agree with you on splitting the package, IMHO in this release it doesn't have much sense to have resynctool and man pages without daemon.

In next releases, where I think to use DB support, it might have sense to split packages.

I'll wait for more feedbacks before regenerating the SRPM.
Comment 28 manuel wolfshant 2009-12-02 07:49:17 EST
Couple of things, after a quick glance over the spec
- Unless you have very very good reasons to not do it, please consider using %dist in the release field. It'll save you some headaches later
- I do not see the reason for 
# Create an empty otppasswd file
[ ! -f $RPM_BUILD_ROOT/etc/otppasswd ] && echo "" > $RPM_BUILD_ROOT/etc/otppasswd
Since you are in a clean buildroot, the file does not exist unless make install created it. And a simple "touch $RPM_BUILD_ROOT/etc/otppasswd" seems more elegant to me. Or even "> $RPM_BUILD_ROOT/etc/otppasswd" (no need for the "echo " part ). However, nothing incorrect in your approach either. Just a matter of style
- Please consider the comment #26. It's ugly to restart a daemon which was not specifically allowed to be on by the admin
Comment 29 manuel wolfshant 2009-12-02 07:51:28 EST
And please ditch the packager tag. The fedora buildsystem will fill it for you.
Comment 30 manuel wolfshant 2009-12-02 08:16:21 EST
   With respect to #26 and #28: please make sure the initscript does not install with the daemon started by default. Common Fedora policy is to allow the admin to activate the required services, not enforce them in a "started" state.
   Also, according to http://fedoraproject.org/wiki/Packaging:SysVInitScript#Required_Actions it would be nice if you could implement the missing actions (and also use the exit codes listed in that page)
Comment 31 Antoine Brenner 2009-12-03 03:20:57 EST
I agree with comment #30, it would really be nice to have all the required actions in the init script... (It did look very weird to not have at least status...)
Comment 32 Giuseppe Paterno 2009-12-03 11:22:35 EST
I've tried to implemented what you asked, please try again:

SPEC: http://www.gpaterno.com/external/otpd.spec
SRPM: http://www.gpaterno.com/external/otpd-3.2.6-1.fc11.src.rpm
Comment 33 manuel wolfshant 2009-12-04 15:45:43 EST
Skimming through the output of rpmlint (and skipping the non relevant stuff),
- the new src version has a cosmetic issue:
 otpd.src: W: mixed-use-of-spaces-and-tabs (spaces: line 12, tab: line 1)
- you forgot to upload the new source.tar.gz to googlecode
- you forgot to add a new entry in the changelog (last entry still points to 3.2.5-2, which makes rpmlint unhappy:
 otpd.x86_64: W: incoherent-version-in-changelog 3.2.5-2 ['3.2.6-1.fc13', '3.2.6-1']
- the service is still installed as default enabled
  otpd.x86_64: W: service-default-enabled /etc/rc.d/init.d/otpd
- and you still do a service restart in %post, which, combined with the above entry, makes the service start even on the systems where the admin decided to stop it
- and we still have
 otpd.x86_64: W: no-reload-entry /etc/rc.d/init.d/otpd
I am not familiar at all with the code and I cannot even test as I have no tokens, but maybe reload could be implemented as a synonym to restart ?



As a second matter, do you have any other submissions or did you perform any package reviews? If you have, please be as kind as to point me to those bugzilla entries. If not, please consider doing so, as this is still a mandatory step before being sponsored.
Comment 34 Giuseppe Paterno 2009-12-14 10:57:32 EST
Sorry guys, back from holidays. 
Will do in next days.
Comment 35 Till Maas 2009-12-30 14:41:11 EST
(In reply to comment #34)
> Sorry guys, back from holidays. 
> Will do in next days.  

please remove NotReady from the Whiteboard once you have addressed the mentioned issues.