Bug 949324 - Review Request: oath-toolkit - One-time password components
Review Request: oath-toolkit - One-time password components
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: David Woodhouse
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-07 14:13 EDT by Jaroslav Škarvada
Modified: 2015-09-22 12:32 EDT (History)
6 users (show)

See Also:
Fixed In Version: oath-toolkit-2.0.2-3.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-06-05 18:10:17 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dwmw2: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jaroslav Škarvada 2013-04-07 14:13:01 EDT
Spec URL: http://fedorapeople.org/~jskarvad/oath-toolkit/oath-toolkit.spec
SRPM URL: http://fedorapeople.org/~jskarvad/oath-toolkit/oath-toolkit-2.0.2-1.fc18.src.rpm
Description: The OATH Toolkit provide components for building one-time password
authentication systems. It contains shared libraries, command line tools and a
PAM module. Supported technologies include the event-based HOTP algorithm
(RFC4226) and the time-based TOTP algorithm (RFC6238). OATH stands for Open
AuTHentication, which is the organization that specify the algorithms. For
managing secret key files, the Portable Symmetric Key Container (PSKC) format
described in RFC6030 is supported.
Fedora Account System Username: jskarvad
Comment 1 Jaroslav Škarvada 2013-04-07 14:53:54 EDT
For PAM module to correctly work with sshd, we will probably need selinux label (for oath users configuration/state file) and appropriate rules:

Apr  7 20:49:26 yarda kernel: [23820.679293] type=1400 audit(1365360566.085:24): avc:  denied  { write } for  pid=24819 comm="sshd" name="lib" dev="dm-1" ino=786434 scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=dir
Apr  7 20:49:26 yarda kernel: [23820.698179] type=1400 audit(1365360566.085:25): avc:  denied  { add_name } for  pid=24819 comm="sshd" name="users.oath.lock" scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=dir
Apr  7 20:49:26 yarda kernel: [23820.718090] type=1400 audit(1365360566.124:26): avc:  denied  { create } for  pid=24819 comm="sshd" name="users.oath.lock" scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=file
Apr  7 20:49:26 yarda kernel: [23820.737756] type=1400 audit(1365360566.144:27): avc:  denied  { write open } for  pid=24819 comm="sshd" path="/var/lib/users.oath.lock" dev="dm-1" ino=806912 scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=file
Apr  7 20:49:26 yarda kernel: [23820.759772] type=1400 audit(1365360566.165:28): avc:  denied  { lock } for  pid=24819 comm="sshd" path="/var/lib/users.oath.lock" dev="dm-1" ino=806912 scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=file
Apr  7 20:49:26 yarda kernel: [23820.783848] type=1400 audit(1365360566.190:29): avc:  denied  { getattr } for  pid=24819 comm="sshd" path="/var/lib/users.oath.new" dev="dm-1" ino=810364 scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=file
Apr  7 20:49:26 yarda kernel: [23820.846464] type=1400 audit(1365360566.253:30): avc:  denied  { remove_name } for  pid=24819 comm="sshd" name="users.oath.new" dev="dm-1" ino=810364 scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=dir
Apr  7 20:49:26 yarda kernel: [23820.867964] type=1400 audit(1365360566.274:31): avc:  denied  { rename } for  pid=24819 comm="sshd" name="users.oath.new" dev="dm-1" ino=810364 scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=file
Apr  7 20:49:26 yarda kernel: [23820.889180] type=1400 audit(1365360566.295:32): avc:  denied  { unlink } for  pid=24819 comm="sshd" name="users.oath" dev="dm-1" ino=272776 scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:etc_t:s0 tclass=file
Apr  7 20:49:26 yarda kernel: [23820.909548] type=1400 audit(1365360566.316:33): avc:  denied  { unlink } for  pid=24819 comm="sshd" name="users.oath.lock" dev="dm-1" ino=806912 scontext=system_u:system_r:sshd_t:s0-s0:c0.c1023 tcontext=system_u:object_r:var_lib_t:s0 tclass=file
Comment 2 Jaroslav Škarvada 2013-04-07 14:56:56 EDT
Configuration and state is stored in one file, it's name is configurable through PAM, in the example from comment 1, the /var/lib/users.oath was used.
Comment 3 Michael Scherer 2013-04-07 18:05:54 EDT
Wouldn't it be useful to pre-create a file with proper permission ( and so selinux label ), have it owned by the rpm and document it ?


also, fedora-review complain that bundled(gnulib) is not provided
Comment 4 Jaroslav Škarvada 2013-04-07 18:26:38 EDT
(In reply to comment #3)
> Wouldn't it be useful to pre-create a file with proper permission ( and so
> selinux label ), have it owned by the rpm and document it ?
>
This probably wouldn't help now, we still need the selinux label and rules (which currently probably doesn't exist) - I will manage this with the selinux guys once the package gets through the review - it's only minor issue with one sub-functionality. Also the user can use the pam_oath module multiple times in his/her PAM stack with multiple configuration files, thus I probably wouldn't like to provide the default file, but we could provide the instructions how to setup this / label the files in the README file.
> 
> also, fedora-review complain that bundled(gnulib) is not provided

Fixed without release bump.
Comment 5 Michael Scherer 2013-04-07 19:03:23 EDT
What about having a directory for holding the various files ? 
This would keep the rules clean ( ie, everything in this directory would be labelled as foo_t, and foot_t is usable by pam_oath ? )

And the point of having a default file is also to make sure the permission are correct (ie 600 ) and to be able to check that using rpm -V
Comment 6 Jaroslav Škarvada 2013-04-08 05:20:31 EDT
(In reply to comment #5)
> What about having a directory for holding the various files ? 
> This would keep the rules clean ( ie, everything in this directory would be
> labelled as foo_t, and foot_t is usable by pam_oath ? )
> 
Nice idea, added /etc/liboath directory to hold the files.

Spec URL: http://fedorapeople.org/~jskarvad/oath-toolkit/oath-toolkit.spec
SRPM URL: http://fedorapeople.org/~jskarvad/oath-toolkit/oath-toolkit-2.0.2-2.fc18.src.rpm
Comment 7 Michael Scherer 2013-04-08 07:50:11 EDT
Mhh, I am not sure if /etc/liboath is the proper location, ( vs /var ), but that's a little details ( since /var do not seems good either for configuration :/ )
Comment 8 Jaroslav Škarvada 2013-04-08 08:13:17 EDT
(In reply to comment #7)
> Mhh, I am not sure if /etc/liboath is the proper location, ( vs /var ), but
> that's a little details ( since /var do not seems good either for
> configuration :/ )

I agree with that, but it is probably the best we could currently do. The liboath needs to patched to separate the state and configuration to two files. I will address this as a RFE to upstream.
Comment 9 Daniel Pocock 2013-05-09 12:34:31 EDT

I would like to submit a review request for dynalogin, it uses liboath as a dependency.  Would you like to review them together to ensure interoperability?
Comment 10 Jaroslav Škarvada 2013-05-10 03:56:56 EDT
(In reply to comment #9)
> 
> I would like to submit a review request for dynalogin, it uses liboath as a
> dependency.  Would you like to review them together to ensure
> interoperability?

Hi Daniel, submit the review request and set it to depend on this BZ. I can do the review, but currently there is no reviewer for the oath-toolkit. We need to handle this first.
Comment 11 David Woodhouse 2013-06-03 12:28:27 EDT
libpskc-doc.noarch: W: manual-page-warning /usr/share/man/man3/pskc_get_key_algparm_chall_min.3.gz 18: warning: macro `Encoding'' not defined
libpskc-doc.noarch: W: manual-page-warning /usr/share/man/man3/pskc_get_key_algparm_chall_min.3.gz 19: warning: macro `ALPHANUMERIC',' not defined
libpskc-doc.noarch: W: manual-page-warning /usr/share/man/man3/pskc_get_key_algparm_chall_max.3.gz 18: warning: macro `Encoding'' not defined
libpskc-doc.noarch: W: manual-page-warning /usr/share/man/man3/pskc_get_key_algparm_chall_max.3.gz 19: warning: macro `ALPHANUMERIC',' not defined
libpskc-doc.noarch: W: manual-page-warning /usr/share/man/man3/pskc_get_key_policy_pinminlength.3.gz 19: warning: macro `DECIMAL',' not defined (possibly missing space after `DE')
libpskc-doc.noarch: W: manual-page-warning /usr/share/man/man3/pskc_get_key_policy_pinmaxlength.3.gz 19: warning: macro `HEXADECIMAL',' not defined
libpskc-doc.noarch: W: manual-page-warning /usr/share/man/man3/pskc_get_key_policy_pinmaxlength.3.gz 21: warning: macro `BASE64'' not defined


Those man pages are actually misdisplayed. Other rpmlint warning seem to be harmless.
Comment 12 David Woodhouse 2013-06-05 06:33:03 EDT
Working through the rest of the review guidelines... should the pam_oath package require %{_libdir}/security (or 'pam')?

And should the Requires: Requires: libpskc = %{version}-%{release} in libpskc-devel actually be Requires: libpskc{?_isa} = %{version}-%{release} 
(and other packages likewise)?

In addition to the rpmlint warnings in comment 11, I also see the following:

libpskc.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpskc.so.0.0.1 /lib64/libltdl.so.7
libpskc.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpskc.so.0.0.1 /lib64/libxslt.so.1
libpskc.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpskc.so.0.0.1 /lib64/libz.so.1
libpskc.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpskc.so.0.0.1 /lib64/libdl.so.2
libpskc.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpskc.so.0.0.1 /lib64/libm.so.6

Other than that, I think everything looks fine. If you can submit an update with these fixed I'll go ahead and approve it.
Comment 13 Jaroslav Škarvada 2013-06-05 10:15:38 EDT
New files:

Spec URL: http://fedorapeople.org/~jskarvad/oath-toolkit/oath-toolkit.spec
SRPM URL: http://fedorapeople.org/~jskarvad/oath-toolkit/oath-toolkit-2.0.2-3.fc18.src.rpm

(In reply to David Woodhouse from comment #12)
> Working through the rest of the review guidelines... should the pam_oath
> package require %{_libdir}/security (or 'pam')?
> 
Yes, I forget :) Fixed.

> And should the Requires: Requires: libpskc = %{version}-%{release} in
> libpskc-devel actually be Requires: libpskc{?_isa} = %{version}-%{release} 
> (and other packages likewise)?
> 
Probably, they reference arch libs, so I fixed both devel packages. Probably no need for other packages.

> In addition to the rpmlint warnings in comment 11, I also see the following:
> 
> libpskc.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpskc.so.0.0.1 /lib64/libltdl.so.7
> libpskc.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpskc.so.0.0.1 /lib64/libxslt.so.1
> libpskc.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpskc.so.0.0.1 /lib64/libz.so.1
> libpskc.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpskc.so.0.0.1 /lib64/libdl.so.2
> libpskc.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpskc.so.0.0.1 /lib64/libm.so.6
> 
I was unable to reproduce this (f18/f19), but to be sure I linked it with --as-needed.

Man pages also fixed, patch sent upstream.
Comment 14 David Woodhouse 2013-06-05 11:49:47 EDT
Now it all looks fine; thanks. Review passed.

The unused-direct-shlib-dependency warning was from running 'rpmlint libpskc' after the package was installed, btw. And is gone now.

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
[x]: Provides: bundled(gnulib) in place as required.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Package complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 133120 bytes in 5 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
Comment 15 David Woodhouse 2013-06-05 11:50:17 EDT
(please build for EL6, btw. I'd like to use it from openconnect there)
Comment 16 Jaroslav Škarvada 2013-06-05 17:15:26 EDT
(In reply to David Woodhouse from comment #15)
> (please build for EL6, btw. I'd like to use it from openconnect there)

OK, np, thanks for the review.
Comment 17 Jaroslav Škarvada 2013-06-05 17:20:59 EDT
New Package SCM Request
=======================
Package Name: oath-toolkit
Short Description: One-time password components
Owners: jskarvad
Branches: f18 f19 el6
InitialCC: dwmw2
Comment 18 Gwyn Ciesla 2013-06-05 17:29:58 EDT
Git done (by process-git-requests).
Comment 19 Jaroslav Škarvada 2013-06-05 17:51:31 EDT
(In reply to Jon Ciesla from comment #18)
> Git done (by process-git-requests).
Thanks.
Comment 20 Fedora Update System 2013-06-05 18:09:19 EDT
oath-toolkit-2.0.2-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/oath-toolkit-2.0.2-3.fc19
Comment 21 Fedora Update System 2013-06-05 18:11:22 EDT
oath-toolkit-2.0.2-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/oath-toolkit-2.0.2-3.fc18
Comment 22 Fedora Update System 2013-06-05 18:12:28 EDT
oath-toolkit-2.0.2-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/oath-toolkit-2.0.2-3.el6
Comment 23 Fedora Update System 2013-06-14 23:05:49 EDT
oath-toolkit-2.0.2-3.fc19 has been pushed to the Fedora 19 stable repository.
Comment 24 Fedora Update System 2013-06-16 01:43:30 EDT
oath-toolkit-2.0.2-3.fc18 has been pushed to the Fedora 18 stable repository.
Comment 25 Fedora Update System 2013-06-21 15:37:20 EDT
oath-toolkit-2.0.2-3.el6 has been pushed to the Fedora EPEL 6 stable repository.
Comment 26 Jaroslav Škarvada 2014-08-05 05:58:57 EDT
Package Change Request
======================
Package Name: oath-toolkit
New Branches: epel7
Owners: jskarvad
Comment 27 Gwyn Ciesla 2014-08-05 08:15:08 EDT
Git done (by process-git-requests).
Comment 28 Jaroslav Škarvada 2014-08-05 11:02:07 EDT
Thanks

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