https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Bug ID: 1283296 Summary: Review Request: pam-u2f - PAM authentication over U2F Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: spartacus06@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
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
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #1 from Seth Jennings spartacus06@gmail.com --- Created attachment 1096146 --> https://bugzilla.redhat.com/attachment.cgi?id=1096146&action=edit fedora-review
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Seth Jennings spartacus06@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment|0 |1 #1096146 is| | obsolete| |
--- Comment #2 from Seth Jennings spartacus06@gmail.com --- Created attachment 1096147 --> https://bugzilla.redhat.com/attachment.cgi?id=1096147&action=edit fedora-review
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Till Maas opensource@till.name changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |opensource@till.name
--- Comment #3 from Till Maas opensource@till.name --- - There is a typo in the description for pamu2fcfg: "too" instead of "tool"
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #4 from Seth Jennings spartacus06@gmail.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@in.waw.pl
--- Comment #5 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Seth Jennings sjenning@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sjenning@redhat.com
--- Comment #6 from Seth Jennings sjenning@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #7 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl ---
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #8 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- I filed https://github.com/Yubico/pam-u2f/issues/28 to resolve this upstream.
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |zbyszek@in.waw.pl Flags| |fedora-review?
--- Comment #9 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- It seems that the issue has been patched upstream. Can you prepare an updated srpm with the patches?
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #10 from Seth Jennings spartacus06@gmail.com --- (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!
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #11 from Seth Jennings spartacus06@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugzilla.redhat.com | |/show_bug.cgi?id=1292687 Flags|fedora-review? |fedora-review+
--- Comment #12 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- 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].
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #13 from Jon Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/pam-u2f
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #14 from Fedora Update System updates@fedoraproject.org --- pam-u2f-1.0.3-5.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-f047c98286
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #15 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #16 from Georg Sauthoff fedora@georg.so --- 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;
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #17 from Seth Jennings spartacus06@gmail.com --- 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!
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
fred fredoche@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fredoche@gmail.com
--- Comment #18 from fred fredoche@gmail.com --- Hi, I just got offered a yubikey, any news on the support front ? Thanks
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Seth Jennings sjenning@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sjenning@redhat.com
--- Comment #19 from Seth Jennings sjenning@redhat.com --- Fred, the package is already in the repos. The selinux issue is still outstanding.
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #20 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA Last Closed| |2016-05-04 14:54:23
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #21 from James theholyettlz@googlemail.com --- Any advance on the selinux issue?
https://bugzilla.redhat.com/show_bug.cgi?id=1283296
--- Comment #22 from Seth Jennings sjenning@redhat.com --- 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
package-review@lists.fedoraproject.org