Bug 737735 - Review Request: google-authenticator - One-time passcode support using open standards
Summary: Review Request: google-authenticator - One-time passcode support using open s...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-09-13 00:45 UTC by David Woodhouse
Modified: 2013-03-30 03:59 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-03-30 03:59:24 UTC
kevin: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description David Woodhouse 2011-09-13 00:45:15 UTC
Spec URL: http://www.infradead.org/~dwmw2/google-authenticator.spec
SRPM URL: http://www.infradead.org/~dwmw2/google-authenticator-0-0.1.20110830.hgd525a9bab875.fc16.src.rpm
Description:
The Google Authenticator project includes implementations of one-time
passcode generators for several mobile platforms, as well as a
pluggable authentication module (PAM). One-time passcodes are
generated using open standards developed by the Initiative for Open
Authentication (OATH) (which is unrelated to OAuth).

These implementations support the HMAC-Based One-time Password (HOTP)
algorithm specified in RFC 4226 and the Time-based One-time Password
(TOTP) algorithm currently in draft.

Comment 1 Kevin Fenzi 2011-09-18 19:10:38 UTC
I'll look at reviewing this this afternoon. 
Look for a full review in a bit.

Comment 2 Kevin Fenzi 2011-09-18 19:58:51 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
See below - License
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:

OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files. 
See below - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output. 
See below - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should function as described. 
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin

Issues: 

1. You might re-word the summary some to note that this package is the pam module and 
command line tool, not any of the mobile applications? Also, is it worth excluding the 
mobile apps source from the checkout since it's not ever used? 

2. Might ask upstream to ship a copy of the ASL with the project. 

3. Can you please add a spec comment on how to generate the Source0?
http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

4. rpmlint says: 

google-authenticator.src: W: spelling-error Summary(en_US) passcode -> pass code, pass-code, postcode
google-authenticator.src: W: spelling-error %description -l en_US passcode -> pass code, pass-code, postcode
google-authenticator.src: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
google-authenticator.src: W: spelling-error %description -l en_US passcodes -> pass codes, pass-codes, compasses
google-authenticator.src: W: invalid-url Source0: google-authenticator-0.20110830.hgd525a9bab875.tar.gz
google-authenticator.x86_64: W: spelling-error Summary(en_US) passcode -> pass code, pass-code, postcode
google-authenticator.x86_64: W: spelling-error %description -l en_US passcode -> pass code, pass-code, postcode
google-authenticator.x86_64: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
google-authenticator.x86_64: W: spelling-error %description -l en_US passcodes -> pass codes, pass-codes, compasses
google-authenticator.x86_64: W: no-manual-page-for-binary google-authenticator
3 packages and 0 specfiles checked; 0 errors, 10 warnings.

All are bogus. A man page would be nice, but clearly not a blocker. 

5. You shouldn't own
/%{_lib}/security
as thats owned by pam. 

6. you should Require pam? I guess it dlopens, but for the above directory, and 
just to be usable? ;) 

7. Worth running pam_google_authenticator_unittest in %check?

Comment 3 David Woodhouse 2011-09-26 23:47:35 UTC
Spec URL: http://www.infradead.org/~dwmw2/google-authenticator.spec
SRPM URL:
http://www.infradead.org/~dwmw2/google-authenticator-0-0.2.20110830.hgd525a9bab875.fc16.src.rpm

Fixed #1 #3 #5 #6 (we link directly against -lpam now) and #7.

Comment 4 Kevin Fenzi 2011-09-27 02:28:48 UTC
Can you check links? The srpm is giving a 404 here...

Comment 5 David Woodhouse 2011-09-27 07:56:58 UTC
Crap, sorry. It's fc17 not fc16 now:

http://david.woodhou.se/google-authenticator-0-0.2.20110830.hgd525a9bab875.fc17.src.rpm

Comment 6 Kevin Fenzi 2011-09-28 03:38:37 UTC
All looks good. I see no further blockers... this package is APPROVED.

Comment 7 David Woodhouse 2011-10-01 07:21:20 UTC
New Package SCM Request
=======================
Package Name: google-authenticator
Short Description: One-time passcode support using open standards
Owners: dwmw2
Branches: f15 f16 el6

Comment 8 Gwyn Ciesla 2011-10-01 17:19:18 UTC
Git done (by process-git-requests).

Comment 9 Floren Munteanu 2012-08-08 08:07:09 UTC
I don't think this is the proper approach. Please read my comment:
https://bugzilla.redhat.com/show_bug.cgi?id=754978#c25

Regards,

Floren munteanu

Comment 10 Floren Munteanu 2012-08-08 08:08:18 UTC
I don't think this is the proper approach. Please read my comment:
https://bugzilla.redhat.com/show_bug.cgi?id=754978#c25

Regards,

Floren Munteanu

Comment 11 Kevin Fenzi 2013-03-30 03:59:24 UTC
This package was built long ago. Closing now. 

Please file bugs on any issues you have with it.


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