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

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.

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

(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.

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

(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

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 :/ )

(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.

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?

(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.

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.

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.

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.

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.

(please build for EL6, btw. I'd like to use it from openconnect there)

(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.

New Package SCM Request
=======================
Package Name: oath-toolkit
Short Description: One-time password components
Owners: jskarvad
Branches: f18 f19 el6
InitialCC: dwmw2

Git done (by process-git-requests).

(In reply to Jon Ciesla from comment #18)
> Git done (by process-git-requests).
Thanks.

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

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

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

oath-toolkit-2.0.2-3.fc19 has been pushed to the Fedora 19 stable repository.

oath-toolkit-2.0.2-3.fc18 has been pushed to the Fedora 18 stable repository.

oath-toolkit-2.0.2-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

Package Change Request
======================
Package Name: oath-toolkit
New Branches: epel7
Owners: jskarvad

Git done (by process-git-requests).

Thanks