Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://www.variantweb.net/pub/review/pam-u2f.spec SRPM URL: https://www.variantweb.net/pub/review/pam-u2f-1.0.3-1.fc23.src.rpm Description: The PAM U2F module provides an easy way to integrate the Yubikey (or other U2F-compliant authenticators) into your existing user authentication infrastructure. PAM is used by GNU/Linux, Solaris and Mac OS X for user authentication. Fedora Account System Username: spartacus06
Created attachment 1096146 [details] fedora-review
Created attachment 1096147 [details] fedora-review
- There is a typo in the description for pamu2fcfg: "too" instead of "tool"
(In reply to Till Maas from comment #3) > - There is a typo in the description for pamu2fcfg: "too" instead of "tool" Fixed. Thanks Till. Spec URL: https://www.variantweb.net/pub/review/pam-u2f.spec SRPM URL: https://www.variantweb.net/pub/review/pam-u2f-1.0.3-2.fc23.src.rpm
find -exec rm -f {} ';' can be replaced with find -delete. It is nice to put BuildRequires/Requires each on a separate line. %description should end in a dot. The module should not Require pam: it is a plugin for pam. It already has an automatically generated dependency on libpam.so, and that is enough. Other pam modules do not Require pam. Where can the environment variable DEFAULT_AUTHFILE_DIR_VAR come from? I didn't look at pamu2fcfg, but the code of the pam module appears to be carefully written, looks correct.
Fixed. Thanks Zbigniew! Spec URL: https://www.variantweb.net/pub/review/pam-u2f.spec SRPM URL: https://www.variantweb.net/pub/review/pam-u2f-1.0.3-3.fc23.src.rpm
> Where can the environment variable DEFAULT_AUTHFILE_DIR_VAR come from? So, this module uses a number of environment variables ($DEFAULT_AUTHFILE_DIR_VAR, $XDG_CONFIG_HOME at least). To try it out, I added "auth require pam_u2f.so debug origin=pam://$HOSTNAME appid=pam://$HOSTNAME" as the first line in /etc/pam.d/su-l, and then I run: $ su - [pam-u2f.c:parse_cfg(48)] called. [pam-u2f.c:parse_cfg(49)] flags 0 argc 3 [pam-u2f.c:parse_cfg(51)] argv[0]=debug [pam-u2f.c:parse_cfg(51)] argv[1]=origin=pam://$HOSTNAME [pam-u2f.c:parse_cfg(51)] argv[2]=appid=pam://$HOSTNAME [pam-u2f.c:parse_cfg(52)] max_devices=0 [pam-u2f.c:parse_cfg(53)] debug=1 [pam-u2f.c:parse_cfg(54)] interactive=0 [pam-u2f.c:parse_cfg(55)] cue=0 [pam-u2f.c:parse_cfg(56)] manual=0 [pam-u2f.c:parse_cfg(57)] nouserok=0 [pam-u2f.c:parse_cfg(58)] alwaysok=0 [pam-u2f.c:parse_cfg(59)] authfile=(null) [pam-u2f.c:parse_cfg(60)] origin=pam://$HOSTNAME [pam-u2f.c:parse_cfg(61)] appid=pam://$HOSTNAME [pam-u2f.c:pam_sm_authenticate(124)] Maximum devices number not set. Using default (24) [pam-u2f.c:pam_sm_authenticate(142)] Requesting authentication for user root [pam-u2f.c:pam_sm_authenticate(153)] Found user root [pam-u2f.c:pam_sm_authenticate(154)] Home directory for root is /root [pam-u2f.c:pam_sm_authenticate(161)] Variable XDG_CONFIG_HOME is not set. Using default value ($HOME/.config/) [pam-u2f.c:pam_sm_authenticate(193)] Using default authentication file /root/.config/Yubico/u2f_keys [util.c:get_devices_from_authfile(34)] Cannot open file: /root/.config/Yubico/u2f_keys (No such file or directory) [pam-u2f.c:pam_sm_authenticate(211)] Unable to get devices from file /root/.config/Yubico/u2f_keys [pam-u2f.c:pam_sm_authenticate(259)] done. [Authentication service cannot retrieve authentication info] Password: Question: I'd expect the auth process to fail, since "require" is used. In the logs I see: Dec 06 19:38:28 rawhide su[9137]: PAM pam_parse: expecting return value; [...require] Looks like an error in the module. Then I run: $ XDG_CONFIG_HOME=/home/test su - [pam-u2f.c:parse_cfg(48)] called. [pam-u2f.c:parse_cfg(49)] flags 0 argc 3 [pam-u2f.c:parse_cfg(51)] argv[0]=debug [pam-u2f.c:parse_cfg(51)] argv[1]=origin=pam://$HOSTNAME [pam-u2f.c:parse_cfg(51)] argv[2]=appid=pam://$HOSTNAME [pam-u2f.c:parse_cfg(52)] max_devices=0 [pam-u2f.c:parse_cfg(53)] debug=1 [pam-u2f.c:parse_cfg(54)] interactive=0 [pam-u2f.c:parse_cfg(55)] cue=0 [pam-u2f.c:parse_cfg(56)] manual=0 [pam-u2f.c:parse_cfg(57)] nouserok=0 [pam-u2f.c:parse_cfg(58)] alwaysok=0 [pam-u2f.c:parse_cfg(59)] authfile=(null) [pam-u2f.c:parse_cfg(60)] origin=pam://$HOSTNAME [pam-u2f.c:parse_cfg(61)] appid=pam://$HOSTNAME [pam-u2f.c:pam_sm_authenticate(124)] Maximum devices number not set. Using default (24) [pam-u2f.c:pam_sm_authenticate(142)] Requesting authentication for user root [pam-u2f.c:pam_sm_authenticate(153)] Found user root [pam-u2f.c:pam_sm_authenticate(154)] Home directory for root is /root [pam-u2f.c:pam_sm_authenticate(178)] Variable XDG_CONFIG_HOME set to /home/test [pam-u2f.c:pam_sm_authenticate(193)] Using default authentication file /home/test/Yubico/u2f_keys [util.c:get_devices_from_authfile(34)] Cannot open file: /home/test/Yubico/u2f_keys (No such file or directory) [pam-u2f.c:pam_sm_authenticate(211)] Unable to get devices from file /home/test/Yubico/u2f_keys [pam-u2f.c:pam_sm_authenticate(259)] done. [Authentication service cannot retrieve authentication info] Password: As you can see, "Requesting authentication for user root", but it's happy to read configuration from a user specified file. This doesn't seem right.
I filed https://github.com/Yubico/pam-u2f/issues/28 to resolve this upstream.
It seems that the issue has been patched upstream. Can you prepare an updated srpm with the patches?
(In reply to Zbigniew Jędrzejewski-Szmek from comment #9) > It seems that the issue has been patched upstream. Can you prepare an > updated srpm with the patches? Thanks, will do!
Updated with upstream patches Spec URL: https://www.variantweb.net/pub/review/pam-u2f.spec SRPM URL: https://www.variantweb.net/pub/review/pam-u2f-1.0.3-4.fc23.src.rpm
INFO: No upstream for (Source2): gpgkey-D3994701.gpg It would be nice to provide a full URL. General recommendation is to have BuildRequires on separate lines, because this makes later diffs much easier to read. %autosetup macro would be nice here (%autosetup -n ... -p1), because then you can remove the %patchN lines. %description should end in a dot (repeated from comment #5). The key verification checking fails, see linked bug. - license is OK (BSD) - license file is present, %license is used - license matches headers - latest version - no scriptlets - provides/requires look sane - I did some small testing and it seems to work as expected (I don't have the hardware, but it seems to be trying to do the right things...) rpmlint: Checking: pam-u2f-1.0.3-4.fc24.x86_64.rpm pamu2fcfg-1.0.3-4.fc24.x86_64.rpm pam-u2f-debuginfo-1.0.3-4.fc24.x86_64.rpm pam-u2f-1.0.3-4.fc24.src.rpm pam-u2f.x86_64: W: spelling-error %description -l en_US authenticators -> authentication, authenticates, authenticate pam-u2f.src: W: spelling-error %description -l en_US authenticators -> authentication, authenticates, authenticate 4 packages and 0 specfiles checked; 0 errors, 2 warnings. So the packaging side looks OK. There are some small issues pointed out above. Package is APPROVED. I think it would be worthwhile to have someone from the security team look this code over. This is normally not part of a review, but I strongly recommend contacting the security team [https://fedoraproject.org/wiki/Security_Team].
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/pam-u2f
pam-u2f-1.0.3-5.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-f047c98286
pam-u2f-1.0.3-5.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-f047c98286
I've tested it on Fedora 23 and it doesn't work with SELinux set to enforce (the default setting). Only after executing semanage permissive -a local_login_t the module worked. Also, a Fedora specific README would be helpful - i.e. one where it is described what files you have to change in what way. For example, I wanted to configure U2F as 2nd factor in addition to password authentication - for locale console logins and gnome shell (including unlocking a locked screen). I've managed to do that via adding this line before the `auth ... pasword-auth` line in /etc/pam.d/{login,gdm-password}: auth requisite pam_u2f.so debug authfile=/etc/u2f_mappings interactive (and filling /etc/u2f_mappings with output from pamu2fcfg) In addition to that, the Fedora README could also mention pamu2fcfg. More SELinux details: The SELinux audit messages looked like this (before executing semanage permissive): type=AVC msg=audit(1452281803.756:2262): avc: denied { read } for pid=11098 comm="login" name="c248:0" dev="tmpfs" ino=14836 scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 type=AVC msg=audit(1452281803.756:2263): avc: denied { read } for pid=11098 comm="login" name="c248:1" dev="tmpfs" ino=14839 scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 type=AVC msg=audit(1452281803.757:2264): avc: denied { read } for pid=11098 comm="login" name="c248:2" dev="tmpfs" ino=894548 scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 type=AVC msg=audit(1452281803.757:2265): avc: denied { read } for pid=11098 comm="login" name="c248:3" dev="tmpfs" ino=895813 scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 type=AVC msg=audit(1452281803.758:2266): avc: denied { read } for pid=11098 comm="login" name="c248:4" dev="tmpfs" ino=894573 scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 type=AVC msg=audit(1452281803.758:2267): avc: denied { read } for pid=11098 comm="login" name="c248:5" dev="tmpfs" ino=910340 scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 type=AVC msg=audit(1452281803.759:2268): avc: denied { read } for pid=11098 comm="login" name="c248:6" dev="tmpfs" ino=908284 scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 The tool audit2allow suggests: #============= local_login_t ============== allow local_login_t udev_var_run_t:file read;
Sorry for the delayed reply. (In reply to Georg Sauthoff from comment #16) > I've tested it on Fedora 23 and it doesn't work with SELinux set to enforce > (the default setting). > > Only after executing > > semanage permissive -a local_login_t > > the module worked. > > Also, a Fedora specific README would be helpful - i.e. one where it is > described what files you have to change in what way. Yes, a Fedora README would be a good idea. > > For example, I wanted to configure U2F as 2nd factor in addition to password > authentication - for locale console logins and gnome shell (including > unlocking a locked screen). I've managed to do that via adding this line > before the `auth ... pasword-auth` line in /etc/pam.d/{login,gdm-password}: > > auth requisite pam_u2f.so debug authfile=/etc/u2f_mappings interactive > > (and filling /etc/u2f_mappings with output from pamu2fcfg) > > In addition to that, the Fedora README could also mention pamu2fcfg. > > More SELinux details: > > The SELinux audit messages looked like this (before executing semanage > permissive): > > type=AVC msg=audit(1452281803.756:2262): avc: denied { read } for > pid=11098 comm="login" name="c248:0" dev="tmpfs" ino=14836 > scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 > type=AVC msg=audit(1452281803.756:2263): avc: denied { read } for > pid=11098 comm="login" name="c248:1" dev="tmpfs" ino=14839 > scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 > type=AVC msg=audit(1452281803.757:2264): avc: denied { read } for > pid=11098 comm="login" name="c248:2" dev="tmpfs" ino=894548 > scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 > type=AVC msg=audit(1452281803.757:2265): avc: denied { read } for > pid=11098 comm="login" name="c248:3" dev="tmpfs" ino=895813 > scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 > type=AVC msg=audit(1452281803.758:2266): avc: denied { read } for > pid=11098 comm="login" name="c248:4" dev="tmpfs" ino=894573 > scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 > type=AVC msg=audit(1452281803.758:2267): avc: denied { read } for > pid=11098 comm="login" name="c248:5" dev="tmpfs" ino=910340 > scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 > type=AVC msg=audit(1452281803.759:2268): avc: denied { read } for > pid=11098 comm="login" name="c248:6" dev="tmpfs" ino=908284 > scontext=system_u:system_r:local_login_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:udev_var_run_t:s0 tclass=file permissive=0 I didn't try to use it for console logins so it seems there is an selinux policy issue there. I'll check it out. > > > The tool audit2allow suggests: > > #============= local_login_t ============== > allow local_login_t udev_var_run_t:file read; Thanks for the testing!
Hi, I just got offered a yubikey, any news on the support front ? Thanks
Fred, the package is already in the repos. The selinux issue is still outstanding.
pam-u2f-1.0.3-5.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Any advance on the selinux issue?
Wow, so I've slacked off on this package like crazy! Apologies. I'm opening new bugs to track the issues raised, since this bug is closed. Was able to recreate the selinux issue. Opened this bug against selinux-policy: https://bugzilla.redhat.com/show_bug.cgi?id=1377451 FYI, full type enforcement needed is: module my-login 1.0; require { type local_login_t; type udev_var_run_t; class file { getattr open read }; } #============= local_login_t ============== allow local_login_t udev_var_run_t:file { getattr open read }; Re: env vars issue, the upstream issue is closed and seems to be address by 1.0.4. Just need to update the package to that version. Opening a bug to track for bodhi purposes: https://bugzilla.redhat.com/show_bug.cgi?id=1377455 Re: README https://bugzilla.redhat.com/show_bug.cgi?id=1377459